Fixes incorrect loader fields (#849)

* loader_fields fix

* tested, fixed

* added direct file check for invalid file_parts

* search fixes

* removed printlns

* Adds check for loaders

* removes println

tests/common/api_v3/project.rs

@@ -133,7 +133,7 @@ impl ApiProject for ApiV3 {
         let ids_or_slugs = serde_json::to_string(ids_or_slugs).unwrap();
         let req = test::TestRequest::get()
             .uri(&format!(
-                "/v2/projects?ids={encoded}",
+                "/v3/projects?ids={encoded}",
                 encoded = urlencoding::encode(&ids_or_slugs)
             ))
             .append_pat(pat)
@@ -147,7 +147,7 @@ impl ApiProject for ApiV3 {
         pat: Option<&str>,
     ) -> ServiceResponse {
         let req = TestRequest::get()
-            .uri(&format!("/v2/project/{id_or_slug}/dependencies"))
+            .uri(&format!("/v3/project/{id_or_slug}/dependencies"))
             .append_pat(pat)
             .to_request();
         self.call(req).await

tests/loader_fields.rs

@@ -1,11 +1,15 @@
 use std::collections::HashSet;
 
 use actix_http::StatusCode;
+use actix_web::test;
 use common::api_v3::ApiV3;
 use common::environment::{with_test_environment, TestEnvironment};
+use itertools::Itertools;
+use labrinth::models::v3;
 use serde_json::json;
 
-use crate::common::api_common::ApiVersion;
+use crate::common::api_common::{ApiProject, ApiVersion};
+use crate::common::api_v3::request_data::get_public_project_creation_data;
 use crate::common::database::*;
 
 use crate::common::dummy_data::{DummyProjectAlpha, DummyProjectBeta, TestFile};
@@ -461,3 +465,94 @@ async fn get_available_loader_fields() {
     })
     .await;
 }
+
+#[actix_rt::test]
+async fn test_multi_get_redis_cache() {
+    // Ensures a multi-project get including both modpacks and mods ddoes not
+    // incorrectly cache loader fields
+    with_test_environment(None, |test_env: TestEnvironment<ApiV3>| async move {
+        let api = &test_env.api;
+
+        // Create 5 modpacks
+        let mut modpacks = Vec::new();
+        for i in 0..5 {
+            let slug = format!("test-modpack-{}", i);
+
+            let creation_data = get_public_project_creation_data(
+                &slug,
+                Some(TestFile::build_random_mrpack()),
+                None,
+            );
+            let resp = api.create_project(creation_data, USER_USER_PAT).await;
+            assert_status!(&resp, StatusCode::OK);
+            modpacks.push(slug);
+        }
+
+        // Create 5 mods
+        let mut mods = Vec::new();
+        for i in 0..5 {
+            let slug = format!("test-mod-{}", i);
+
+            let creation_data =
+                get_public_project_creation_data(&slug, Some(TestFile::build_random_jar()), None);
+            let resp = api.create_project(creation_data, USER_USER_PAT).await;
+            assert_status!(&resp, StatusCode::OK);
+            mods.push(slug);
+        }
+
+        // Get all 10 projects
+        let project_slugs = modpacks
+            .iter()
+            .map(|x| x.as_str())
+            .chain(mods.iter().map(|x| x.as_str()))
+            .collect_vec();
+        let resp = api.get_projects(&project_slugs, USER_USER_PAT).await;
+        assert_status!(&resp, StatusCode::OK);
+        let projects: Vec<v3::projects::Project> = test::read_body_json(resp).await;
+        assert_eq!(projects.len(), 10);
+
+        // Ensure all 5 modpacks have 'mrpack_loaders', and all 5 mods do not
+        for project in projects.iter() {
+            if modpacks.contains(project.slug.as_ref().unwrap()) {
+                assert!(project.fields.contains_key("mrpack_loaders"));
+            } else if mods.contains(project.slug.as_ref().unwrap()) {
+                assert!(!project.fields.contains_key("mrpack_loaders"));
+            } else {
+                panic!("Unexpected project slug: {:?}", project.slug);
+            }
+        }
+
+        // Get a version from each project
+        let version_ids_modpacks = projects
+            .iter()
+            .filter(|x| modpacks.contains(x.slug.as_ref().unwrap()))
+            .map(|x| x.versions[0])
+            .collect_vec();
+        let version_ids_mods = projects
+            .iter()
+            .filter(|x| mods.contains(x.slug.as_ref().unwrap()))
+            .map(|x| x.versions[0])
+            .collect_vec();
+        let version_ids = version_ids_modpacks
+            .iter()
+            .chain(version_ids_mods.iter())
+            .map(|x| x.to_string())
+            .collect_vec();
+        let resp = api.get_versions(version_ids, USER_USER_PAT).await;
+        assert_status!(&resp, StatusCode::OK);
+        let versions: Vec<v3::projects::Version> = test::read_body_json(resp).await;
+        assert_eq!(versions.len(), 10);
+
+        // Ensure all 5 versions from modpacks have 'mrpack_loaders', and all 5 versions from mods do not
+        for version in versions.iter() {
+            if version_ids_modpacks.contains(&version.id) {
+                assert!(version.fields.contains_key("mrpack_loaders"));
+            } else if version_ids_mods.contains(&version.id) {
+                assert!(!version.fields.contains_key("mrpack_loaders"));
+            } else {
+                panic!("Unexpected version id: {:?}", version.id);
+            }
+        }
+    })
+    .await;
+}

tests/project.rs

@@ -1,5 +1,3 @@
-use std::collections::HashMap;
-
 use actix_http::StatusCode;
 use actix_web::test;
 use common::api_v3::ApiV3;
@@ -1218,7 +1216,7 @@ async fn align_search_projects() {
             )
             .await;
 
-        for mut project in projects.hits {
+        for project in projects.hits {
             let project_model = api
                 .get_project(&project.id.to_string(), USER_USER_PAT)
                 .await;
@@ -1229,27 +1227,6 @@ async fn align_search_projects() {
             // (Search should return "")
             project_model.description = "".into();
 
-            // Aggregate project loader fields will not match exactly,
-            // because the search will only return the matching version, whereas the project returns the aggregate.
-            // So, we remove them from both.
-            let project_model_mrpack_loaders: Vec<_> = project_model
-                .fields
-                .remove("mrpack_loaders")
-                .unwrap_or_default()
-                .into_iter()
-                .filter_map(|v| v.as_str().map(|v| v.to_string()))
-                .collect();
-            project_model.fields = HashMap::new();
-            project.fields = HashMap::new();
-
-            // For a similar reason we also remove the mrpack loaders from the additional categories of the search model
-            // (Becasue they are not returned by the search)
-            // TODO: get models to match *exactly* without an additional project fetch,
-            // including these fields removed here
-            project
-                .additional_categories
-                .retain(|x| !project_model_mrpack_loaders.contains(x));
-
             let project_model = serde_json::to_value(project_model).unwrap();
             let searched_project_serialized = serde_json::to_value(project).unwrap();
             assert_eq!(project_model, searched_project_serialized);

tests/v2/search.rs

@@ -170,8 +170,9 @@ async fn search_projects() {
         ));
 
         // Test project 7 (testing the search bug)
-        // This project has an initial private forge version that is 1.20.3, and a fabric 1.20.5 version.
-        // This means that a search for fabric + 1.20.3 or forge + 1.20.5 should not return this project.
+        // This project has an initial private forge version that is 1.20.2, and a fabric 1.20.1 version.
+        // This means that a search for fabric + 1.20.1 or forge + 1.20.1 should not return this project,
+        // but a search for fabric + 1.20.1 should, and it should include both versions in the data.
         let id = 7;
         let modify_json = serde_json::from_value(json!([
             { "op": "add", "path": "/categories", "value": DUMMY_CATEGORIES[5..6] },
@@ -231,15 +232,6 @@ async fn search_projects() {
         // 1. vec of search facets
         // 2. expected project ids to be returned by this search
         let pairs = vec![
-            // For testing: remove me
-            (
-                json!([
-                    ["client_side:required"],
-                    ["versions:1.20.5"],
-                    [&format!("categories:{}", DUMMY_CATEGORIES[5])]
-                ]),
-                vec![],
-            ),
             (
                 json!([["categories:fabric"]]),
                 vec![0, 1, 2, 3, 4, 5, 6, 7, 8],
@@ -277,6 +269,14 @@ async fn search_projects() {
                 ]),
                 vec![],
             ),
+            (
+                json!([
+                    // But it does have a 1.20.2 forge version, so this should return it.
+                    ["categories:forge"],
+                    ["versions:1.20.2"]
+                ]),
+                vec![7],
+            ),
             // Project type change
             // Modpack should still be able to search based on former loader, even though technically the loader is 'mrpack'
             // (json!([["categories:mrpack"]]), vec![4]),
@@ -382,15 +382,25 @@ async fn search_projects() {
             assert_eq!(hit.server_side, "optional".to_string());
         }
 
+        // Ensure game_versions return correctly, but also correctly aggregated
+        // over all versions of a project
         let game_versions = api
             .search_deserialized(
                 Some(&format!("\"&{test_name}\"")),
-                Some(json!([["versions:1.20.5"]])),
+                Some(json!([["categories:forge"], ["versions:1.20.2"]])),
                 USER_USER_PAT,
             )
             .await;
+        assert_eq!(game_versions.hits.len(), 1);
         for hit in game_versions.hits {
-            assert_eq!(hit.versions, vec!["1.20.5".to_string()]);
+            assert_eq!(
+                hit.versions,
+                vec!["1.20.1".to_string(), "1.20.2".to_string()]
+            );
+            assert!(hit.categories.contains(&"forge".to_string()));
+            assert!(hit.categories.contains(&"fabric".to_string()));
+
+            // Also, ensure author is correctly capitalized
             assert_eq!(hit.author, "User".to_string());
         }
     })

tests/v2/version.rs

@@ -12,6 +12,7 @@ use crate::assert_status;
 use crate::common::api_common::{ApiProject, ApiVersion};
 use crate::common::api_v2::ApiV2;
 
+use crate::common::api_v2::request_data::get_public_project_creation_data;
 use crate::common::dummy_data::{DummyProjectAlpha, DummyProjectBeta};
 use crate::common::environment::{with_test_environment, TestEnvironment};
 use crate::common::{
@@ -469,3 +470,48 @@ async fn add_version_project_types_v2() {
     })
     .await;
 }
+
+#[actix_rt::test]
+async fn test_incorrect_file_parts() {
+    // Ensures that a version get that 'should' have mrpack_loaders does still display them
+    //   if the file is 'mrpack' but the file_parts are incorrect
+    with_test_environment(None, |test_env: TestEnvironment<ApiV2>| async move {
+        let api = &test_env.api;
+
+        // Patch to set the file_parts to something incorrect
+        let patch = json!([{
+            "op": "add",
+            "path": "/file_parts",
+            "value": ["invalid.zip"] // one file, wrong non-mrpack extension
+        }]);
+
+        // Create an empty project
+        let slug = "test-project";
+        let creation_data = get_public_project_creation_data(slug, None, None);
+        let resp = api.create_project(creation_data, USER_USER_PAT).await;
+        assert_status!(&resp, StatusCode::OK);
+
+        // Get the project
+        let project = api.get_project_deserialized(slug, USER_USER_PAT).await;
+        assert_eq!(project.project_type, "project");
+
+        // Create a version with a mrpack file, but incorrect file_parts
+        let resp = api
+            .add_public_version(
+                project.id,
+                "1.0.0",
+                TestFile::build_random_mrpack(),
+                None,
+                Some(serde_json::from_value(patch).unwrap()),
+                USER_USER_PAT,
+            )
+            .await;
+        assert_status!(&resp, StatusCode::OK);
+
+        // Get the project now, which should be now correctly identified as a modpack
+        let project = api.get_project_deserialized(slug, USER_USER_PAT).await;
+        assert_eq!(project.project_type, "modpack");
+        assert_eq!(project.loaders, vec!["fabric"]);
+    })
+    .await;
+}