Merge #286: Fix different info-hashes for uploaded and downloaded torrents

josecelano · josecelano · commit 92e844c78a33 · 2023-09-13T16:52:41.000+01:00
d7cc040 refactor: [#282] tests (Jose Celano) 6ff3600 fix: [#282] downloaded torrent info-hash matches uploaded one (Jose Celano) b34c7d8 test: [#282] compare uploaded and downloaded torrent info-hashes (Jose Celano) Pull request description: If you upload a torrent and you download it, the downloaded torrent info-hash is not the same if the origin one didn't contain the `private` field in the `info` dictionary. The Index always includes that field in the response because it always stores a `TRUE` or `FALSE` value in the database, even when the field is not in the original torrent file. - [x] Fix the test. It was passing with a different info-hash for no reason. It should fail with the current implementation. - [x] Change the handler to set `private` to `NULL` when not in the original torrent. And do not include it in the response when it's `NULL` in the database. - [x] Refactor all tests using the `e2e::web::api::v1::contexts::torrent::asserts::expected_torrent` function to compare the info-hashes. ACKs for top commit: josecelano: ACK d7cc040 Tree-SHA512: 126e628326430b0d36ef84adaded7fcbee7cb31b70ee866a42164ebefa111c55dc7b43638a4d3b59f4bc84fe20aba19ac4137acc5f03fe7269bd0f8728f1ea61
diff --git a/src/databases/mysql.rs b/src/databases/mysql.rs
@@ -442,8 +442,6 @@ impl Database for Mysql {
             (root_hash.to_string(), true)
         };
 
-        let private = torrent.info.private.unwrap_or(0);
-
         // add torrent
         let torrent_id = query("INSERT INTO torrust_torrents (uploader_id, category_id, info_hash, size, name, pieces, piece_length, private, root_hash, `source`, date_uploaded) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, UTC_TIMESTAMP())")
             .bind(uploader_id)
@@ -453,7 +451,7 @@ impl Database for Mysql {
             .bind(torrent.info.name.to_string())
             .bind(pieces)
             .bind(torrent.info.piece_length)
-            .bind(private)
+            .bind(torrent.info.private)
             .bind(root_hash)
             .bind(torrent.info.source.clone())
             .execute(&mut tx)
diff --git a/src/databases/sqlite.rs b/src/databases/sqlite.rs
@@ -432,8 +432,6 @@ impl Database for Sqlite {
             (root_hash.to_string(), true)
         };
 
-        let private = torrent.info.private.unwrap_or(0);
-
         // add torrent
         let torrent_id = query("INSERT INTO torrust_torrents (uploader_id, category_id, info_hash, size, name, pieces, piece_length, private, root_hash, `source`, date_uploaded) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, strftime('%Y-%m-%d %H:%M:%S',DATETIME('now', 'utc')))")
             .bind(uploader_id)
@@ -443,7 +441,7 @@ impl Database for Sqlite {
             .bind(torrent.info.name.to_string())
             .bind(pieces)
             .bind(torrent.info.piece_length)
-            .bind(private)
+            .bind(torrent.info.private)
             .bind(root_hash)
             .bind(torrent.info.source.clone())
             .execute(&mut tx)
diff --git a/src/models/torrent_file.rs b/src/models/torrent_file.rs
@@ -113,8 +113,6 @@ impl Torrent {
     /// This function will panic if the `torrent_info.pieces` is not a valid hex string.
     #[must_use]
     pub fn from_new_torrent_info_request(torrent_info: NewTorrentInfoRequest) -> Self {
-        let private = u8::try_from(torrent_info.private.unwrap_or(0)).ok();
-
         // the info part of the torrent file
         let mut info = TorrentInfo {
             name: torrent_info.name.to_string(),
@@ -123,7 +121,7 @@ impl Torrent {
             md5sum: None,
             length: None,
             files: None,
-            private,
+            private: torrent_info.private,
             path: None,
             root_hash: None,
             source: None,
@@ -296,7 +294,7 @@ pub struct DbTorrentInfo {
     pub pieces: String,
     pub piece_length: i64,
     #[serde(default)]
-    pub private: Option<i64>,
+    pub private: Option<u8>,
     pub root_hash: i64,
 }
 
diff --git a/src/services/torrent_file.rs b/src/services/torrent_file.rs
@@ -12,7 +12,7 @@ pub struct NewTorrentInfoRequest {
     pub name: String,
     pub pieces: String,
     pub piece_length: i64,
-    pub private: Option<i64>,
+    pub private: Option<u8>,
     pub root_hash: i64,
     pub files: Vec<TorrentFile>,
     pub announce_urls: Vec<Vec<String>>,
@@ -77,7 +77,7 @@ mod tests {
                 md5sum: None,
                 length: Some(37),
                 files: None,
-                private: Some(0),
+                private: None,
                 path: None,
                 root_hash: None,
                 source: None,
diff --git a/tests/common/contexts/torrent/asserts.rs b/tests/common/contexts/torrent/asserts.rs
@@ -35,11 +35,3 @@ pub fn assert_expected_torrent_details(torrent: &TorrentDetails, expected_torren
         "torrent `magnet_link` mismatch"
     );
 }
-
-/// Full assert for test debugging purposes.
-pub fn _assert_eq_torrent_details(torrent: &TorrentDetails, expected_torrent: &TorrentDetails) {
-    assert_eq!(
-        torrent, expected_torrent,
-        "\nUnexpected torrent details:\n{torrent:#?} torrent details should match the previously indexed torrent:\n{expected_torrent:#?}"
-    );
-}
diff --git a/tests/common/contexts/torrent/fixtures.rs b/tests/common/contexts/torrent/fixtures.rs
@@ -137,7 +137,7 @@ impl TestTorrent {
         }
     }
 
-    pub fn info_hash(&self) -> InfoHash {
+    pub fn info_hash_as_hex_string(&self) -> InfoHash {
         self.file_info.info_hash.clone()
     }
 }
diff --git a/tests/common/responses.rs b/tests/common/responses.rs
@@ -53,11 +53,11 @@ impl BinaryResponse {
             bytes: response.bytes().await.unwrap().to_vec(),
         }
     }
-    pub fn is_bittorrent_and_ok(&self) -> bool {
-        self.is_ok() && self.is_bittorrent()
+    pub fn is_a_bit_torrent_file(&self) -> bool {
+        self.is_ok() && self.is_bittorrent_content_type()
     }
 
-    pub fn is_bittorrent(&self) -> bool {
+    pub fn is_bittorrent_content_type(&self) -> bool {
         if let Some(content_type) = &self.content_type {
             return content_type == "application/x-bittorrent";
         }
diff --git a/tests/e2e/config.rs b/tests/e2e/config.rs
@@ -19,7 +19,7 @@ pub const ENV_VAR_E2E_CONFIG_PATH: &str = "TORRUST_IDX_BACK_E2E_CONFIG_PATH";
 
 // Default values
 
-pub const ENV_VAR_E2E_DEFAULT_CONFIG_PATH: &str = "./config-idx-back.local.toml";
+pub const ENV_VAR_E2E_DEFAULT_CONFIG_PATH: &str = "./config-idx-back.sqlite.local.toml";
 
 /// Initialize configuration from file or env var.
 ///
diff --git a/tests/e2e/web/api/v1/contexts/torrent/asserts.rs b/tests/e2e/web/api/v1/contexts/torrent/asserts.rs
@@ -8,26 +8,29 @@ use crate::common::contexts::user::responses::LoggedInUserData;
 use crate::e2e::environment::TestEnv;
 
 /// The backend does not generate exactly the same torrent that was uploaded.
-/// So we need to update the expected torrent to match the one generated by
-/// the backend.
-pub async fn expected_torrent(mut uploaded_torrent: Torrent, env: &TestEnv, downloader: &Option<LoggedInUserData>) -> Torrent {
-    // code-review: The backend does not generate exactly the same torrent
-    // that was uploaded and created by the `imdl` command-line tool.
-    // So we need to update the expected torrent to match the one generated
-    // by the backend. For some of them it makes sense (`announce`  and `announce_list`),
-    // for others it does not.
-
+///
+/// The backend stores the canonical version of the uploaded torrent. So we need
+/// to update the expected torrent to match the one generated by the backend.
+pub async fn canonical_torrent_for(
+    mut uploaded_torrent: Torrent,
+    env: &TestEnv,
+    downloader: &Option<LoggedInUserData>,
+) -> Torrent {
     let tracker_url = env.server_settings().unwrap().tracker.url.to_string();
 
     let tracker_key = match downloader {
         Some(logged_in_user) => get_user_tracker_key(logged_in_user, env).await,
         None => None,
     };
 
-    uploaded_torrent.info.private = Some(0);
     uploaded_torrent.announce = Some(build_announce_url(&tracker_url, &tracker_key));
-    uploaded_torrent.encoding = None;
     uploaded_torrent.announce_list = Some(build_announce_list(&tracker_url, &tracker_key));
+
+    // These fields are not persisted in the database yet.
+    // See https://github.com/torrust/torrust-index-backend/issues/284
+    // They are ignore when the user uploads the torrent. So the stored
+    // canonical torrent does not contain them.
+    uploaded_torrent.encoding = None;
     uploaded_torrent.creation_date = None;
     uploaded_torrent.created_by = None;
 
diff --git a/tests/e2e/web/api/v1/contexts/torrent/contract.rs b/tests/e2e/web/api/v1/contexts/torrent/contract.rs
@@ -16,7 +16,6 @@ Get torrent info:
 
 mod for_guests {
 
-    use torrust_index_backend::utils::parse_torrent::decode_torrent;
     use torrust_index_backend::web::api;
 
     use crate::common::client::Client;
@@ -28,7 +27,6 @@ mod for_guests {
     };
     use crate::common::http::{Query, QueryParam};
     use crate::e2e::environment::TestEnv;
-    use crate::e2e::web::api::v1::contexts::torrent::asserts::expected_torrent;
     use crate::e2e::web::api::v1::contexts::torrent::steps::upload_random_torrent_to_index;
     use crate::e2e::web::api::v1::contexts::user::steps::new_logged_in_user;
 
@@ -166,7 +164,7 @@ mod for_guests {
         let uploader = new_logged_in_user(&env).await;
         let (test_torrent, uploaded_torrent) = upload_random_torrent_to_index(&uploader, &env).await;
 
-        let response = client.get_torrent(&test_torrent.info_hash()).await;
+        let response = client.get_torrent(&test_torrent.info_hash_as_hex_string()).await;
 
         let torrent_details_response: TorrentDetailsResponse = serde_json::from_str(&response.body).unwrap();
 
@@ -215,29 +213,96 @@ mod for_guests {
         assert!(response.is_json_and_ok());
     }
 
-    #[tokio::test]
-    async fn it_should_allow_guests_to_download_a_torrent_file_searching_by_info_hash() {
-        let mut env = TestEnv::new();
-        env.start(api::Version::V1).await;
+    mod it_should_allow_guests_download_a_torrent_file_searching_by_info_hash {
 
-        if !env.provides_a_tracker() {
-            println!("test skipped. It requires a tracker to be running.");
-            return;
+        use torrust_index_backend::utils::parse_torrent::{calculate_info_hash, decode_torrent};
+        use torrust_index_backend::web::api;
+
+        use crate::common::client::Client;
+        use crate::e2e::environment::TestEnv;
+        use crate::e2e::web::api::v1::contexts::torrent::asserts::canonical_torrent_for;
+        use crate::e2e::web::api::v1::contexts::torrent::steps::upload_random_torrent_to_index;
+        use crate::e2e::web::api::v1::contexts::user::steps::new_logged_in_user;
+
+        #[tokio::test]
+        async fn returning_a_bittorrent_binary_ok_response() {
+            let mut env = TestEnv::new();
+            env.start(api::Version::V1).await;
+
+            if !env.provides_a_tracker() {
+                println!("test skipped. It requires a tracker to be running.");
+                return;
+            }
+
+            let client = Client::unauthenticated(&env.server_socket_addr().unwrap());
+            let uploader = new_logged_in_user(&env).await;
+
+            // Upload
+            let (test_torrent, _torrent_listed_in_index) = upload_random_torrent_to_index(&uploader, &env).await;
+
+            // Download
+            let response = client.download_torrent(&test_torrent.info_hash_as_hex_string()).await;
+
+            assert!(response.is_a_bit_torrent_file());
         }
 
-        let client = Client::unauthenticated(&env.server_socket_addr().unwrap());
+        #[tokio::test]
+        async fn the_downloaded_torrent_should_keep_the_same_info_hash_if_the_torrent_does_not_have_non_standard_fields_in_the_info_dict(
+        ) {
+            let mut env = TestEnv::new();
+            env.start(api::Version::V1).await;
 
-        let uploader = new_logged_in_user(&env).await;
-        let (test_torrent, _torrent_listed_in_index) = upload_random_torrent_to_index(&uploader, &env).await;
+            if !env.provides_a_tracker() {
+                println!("test skipped. It requires a tracker to be running.");
+                return;
+            }
 
-        let response = client.download_torrent(&test_torrent.info_hash()).await;
+            let client = Client::unauthenticated(&env.server_socket_addr().unwrap());
+            let uploader = new_logged_in_user(&env).await;
 
-        let torrent = decode_torrent(&response.bytes).expect("could not decode downloaded torrent");
-        let uploaded_torrent =
-            decode_torrent(&test_torrent.index_info.torrent_file.contents).expect("could not decode uploaded torrent");
-        let expected_torrent = expected_torrent(uploaded_torrent, &env, &None).await;
-        assert_eq!(torrent, expected_torrent);
-        assert!(response.is_bittorrent_and_ok());
+            // Upload
+            let (test_torrent, _torrent_listed_in_index) = upload_random_torrent_to_index(&uploader, &env).await;
+
+            // Download
+            let response = client.download_torrent(&test_torrent.info_hash_as_hex_string()).await;
+
+            let downloaded_torrent_info_hash = calculate_info_hash(&response.bytes);
+
+            assert_eq!(
+                downloaded_torrent_info_hash.to_hex_string(),
+                test_torrent.info_hash_as_hex_string(),
+                "downloaded torrent info-hash does not match uploaded torrent info-hash"
+            );
+        }
+
+        #[tokio::test]
+        async fn the_downloaded_torrent_should_be_the_canonical_version_of_the_uploaded_one() {
+            let mut env = TestEnv::new();
+            env.start(api::Version::V1).await;
+
+            if !env.provides_a_tracker() {
+                println!("test skipped. It requires a tracker to be running.");
+                return;
+            }
+
+            let client = Client::unauthenticated(&env.server_socket_addr().unwrap());
+            let uploader = new_logged_in_user(&env).await;
+
+            // Upload
+            let (test_torrent, _torrent_listed_in_index) = upload_random_torrent_to_index(&uploader, &env).await;
+
+            let uploaded_torrent =
+                decode_torrent(&test_torrent.index_info.torrent_file.contents).expect("could not decode uploaded torrent");
+
+            // Download
+            let response = client.download_torrent(&test_torrent.info_hash_as_hex_string()).await;
+
+            let downloaded_torrent = decode_torrent(&response.bytes).expect("could not decode downloaded torrent");
+
+            let expected_downloaded_torrent = canonical_torrent_for(uploaded_torrent, &env, &None).await;
+
+            assert_eq!(downloaded_torrent, expected_downloaded_torrent);
+        }
     }
 
     #[tokio::test]
@@ -283,7 +348,7 @@ mod for_guests {
         let uploader = new_logged_in_user(&env).await;
         let (test_torrent, _uploaded_torrent) = upload_random_torrent_to_index(&uploader, &env).await;
 
-        let response = client.delete_torrent(&test_torrent.info_hash()).await;
+        let response = client.delete_torrent(&test_torrent.info_hash_as_hex_string()).await;
 
         assert_eq!(response.status, 401);
     }
@@ -319,7 +384,7 @@ mod for_authenticated_users {
         let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token);
 
         let test_torrent = random_torrent();
-        let info_hash = test_torrent.info_hash().clone();
+        let info_hash = test_torrent.info_hash_as_hex_string().clone();
 
         let form: UploadTorrentMultipartForm = test_torrent.index_info.into();
 
@@ -462,7 +527,7 @@ mod for_authenticated_users {
         let client = Client::authenticated(&env.server_socket_addr().unwrap(), &downloader.token);
 
         // When the user downloads the torrent
-        let response = client.download_torrent(&test_torrent.info_hash()).await;
+        let response = client.download_torrent(&test_torrent.info_hash_as_hex_string()).await;
 
         let torrent = decode_torrent(&response.bytes).expect("could not decode downloaded torrent");
 
@@ -504,7 +569,7 @@ mod for_authenticated_users {
 
             let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token);
 
-            let response = client.delete_torrent(&test_torrent.info_hash()).await;
+            let response = client.delete_torrent(&test_torrent.info_hash_as_hex_string()).await;
 
             assert_eq!(response.status, 403);
         }
@@ -532,7 +597,7 @@ mod for_authenticated_users {
 
             let response = client
                 .update_torrent(
-                    &test_torrent.info_hash(),
+                    &test_torrent.info_hash_as_hex_string(),
                     UpdateTorrentFrom {
                         title: Some(new_title.clone()),
                         description: Some(new_description.clone()),
@@ -577,7 +642,7 @@ mod for_authenticated_users {
 
             let response = client
                 .update_torrent(
-                    &test_torrent.info_hash(),
+                    &test_torrent.info_hash_as_hex_string(),
                     UpdateTorrentFrom {
                         title: Some(new_title.clone()),
                         description: Some(new_description.clone()),
@@ -624,7 +689,7 @@ mod for_authenticated_users {
             let admin = new_logged_in_admin(&env).await;
             let client = Client::authenticated(&env.server_socket_addr().unwrap(), &admin.token);
 
-            let response = client.delete_torrent(&test_torrent.info_hash()).await;
+            let response = client.delete_torrent(&test_torrent.info_hash_as_hex_string()).await;
 
             let deleted_torrent_response: DeletedTorrentResponse = serde_json::from_str(&response.body).unwrap();
 
@@ -653,7 +718,7 @@ mod for_authenticated_users {
 
             let response = client
                 .update_torrent(
-                    &test_torrent.info_hash(),
+                    &test_torrent.info_hash_as_hex_string(),
                     UpdateTorrentFrom {
                         title: Some(new_title.clone()),
                         description: Some(new_description.clone()),

Original file line number	Diff line number	Diff line change
`@@ -137,7 +137,7 @@ impl TestTorrent {`
`137`	`137`	`}`
`138`	`138`	`}`
`139`	`139`
`140`		`- pub fn info_hash(&self) -> InfoHash {`
	`140`	`+ pub fn info_hash_as_hex_string(&self) -> InfoHash {`
`141`	`141`	`self.file_info.info_hash.clone()`
`142`	`142`	`}`
`143`	`143`	`}`
Original file line number	Diff line number	Diff line change
`@@ -53,11 +53,11 @@ impl BinaryResponse {`
`53`	`53`	`bytes: response.bytes().await.unwrap().to_vec(),`
`54`	`54`	`}`
`55`	`55`	`}`
`56`		`- pub fn is_bittorrent_and_ok(&self) -> bool {`
`57`		`- self.is_ok() && self.is_bittorrent()`
	`56`	`+ pub fn is_a_bit_torrent_file(&self) -> bool {`
	`57`	`+ self.is_ok() && self.is_bittorrent_content_type()`
`58`	`58`	`}`
`59`	`59`
`60`		`- pub fn is_bittorrent(&self) -> bool {`
	`60`	`+ pub fn is_bittorrent_content_type(&self) -> bool {`
`61`	`61`	`if let Some(content_type) = &self.content_type {`
`62`	`62`	`return content_type == "application/x-bittorrent";`
`63`	`63`	`}`