From 76c885f0802f34ad42921dc4a29025dc6f0c4f12 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 11 Jan 2024 15:36:01 -0800 Subject: [PATCH] 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 --- ...01e2ba04e59229c93c2768167253ea30abb32.json | 40 -------- ...4e413ffefa607a47be92b592d465b15b61006.json | 40 -------- ...12784b121db47612bf809d0a0fe0b5d99b681.json | 46 +++++++++ ...fef0f4ac78e4efb046cc77dcdf43522ef72e2.json | 46 +++++++++ src/database/models/loader_fields.rs | 16 ++- src/database/models/project_item.rs | 91 +++++++++++------ src/database/models/version_item.rs | 94 +++++++++++------- src/models/v2/search.rs | 15 ++- src/models/v3/projects.rs | 17 ++-- src/routes/v2/project_creation.rs | 2 +- src/routes/v2/version_creation.rs | 17 +++- src/routes/v2_reroute.rs | 41 ++++++-- src/search/indexing/local_import.rs | 3 + src/search/indexing/mod.rs | 1 + src/search/mod.rs | 2 + tests/common/api_v3/project.rs | 4 +- tests/loader_fields.rs | 97 ++++++++++++++++++- tests/project.rs | 25 +---- tests/v2/search.rs | 36 ++++--- tests/v2/version.rs | 46 +++++++++ 20 files changed, 470 insertions(+), 209 deletions(-) delete mode 100644 .sqlx/query-777b3dcb5f45db64393476b0f9401e2ba04e59229c93c2768167253ea30abb32.json delete mode 100644 .sqlx/query-82d3a8a3bb864cbeda459065f7d4e413ffefa607a47be92b592d465b15b61006.json create mode 100644 .sqlx/query-e1df7bf2edd30d501a48686c00712784b121db47612bf809d0a0fe0b5d99b681.json create mode 100644 .sqlx/query-f3729149bd174541ec4f7ec2145fef0f4ac78e4efb046cc77dcdf43522ef72e2.json diff --git a/.sqlx/query-777b3dcb5f45db64393476b0f9401e2ba04e59229c93c2768167253ea30abb32.json b/.sqlx/query-777b3dcb5f45db64393476b0f9401e2ba04e59229c93c2768167253ea30abb32.json deleted file mode 100644 index 523b41082..000000000 --- a/.sqlx/query-777b3dcb5f45db64393476b0f9401e2ba04e59229c93c2768167253ea30abb32.json +++ /dev/null @@ -1,40 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n SELECT DISTINCT mod_id,\n ARRAY_AGG(DISTINCT l.loader) filter (where l.loader is not null) loaders,\n ARRAY_AGG(DISTINCT pt.name) filter (where pt.name is not null) project_types,\n ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games\n FROM versions v\n INNER JOIN loaders_versions lv ON v.id = lv.version_id\n INNER JOIN loaders l ON lv.loader_id = l.id\n INNER JOIN loaders_project_types lpt ON lpt.joining_loader_id = l.id\n INNER JOIN project_types pt ON pt.id = lpt.joining_project_type_id\n INNER JOIN loaders_project_types_games lptg ON lptg.loader_id = l.id AND lptg.project_type_id = pt.id\n INNER JOIN games g ON lptg.game_id = g.id\n WHERE v.id = ANY($1)\n GROUP BY mod_id\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "mod_id", - "type_info": "Int8" - }, - { - "ordinal": 1, - "name": "loaders", - "type_info": "VarcharArray" - }, - { - "ordinal": 2, - "name": "project_types", - "type_info": "VarcharArray" - }, - { - "ordinal": 3, - "name": "games", - "type_info": "VarcharArray" - } - ], - "parameters": { - "Left": [ - "Int8Array" - ] - }, - "nullable": [ - false, - null, - null, - null - ] - }, - "hash": "777b3dcb5f45db64393476b0f9401e2ba04e59229c93c2768167253ea30abb32" -} diff --git a/.sqlx/query-82d3a8a3bb864cbeda459065f7d4e413ffefa607a47be92b592d465b15b61006.json b/.sqlx/query-82d3a8a3bb864cbeda459065f7d4e413ffefa607a47be92b592d465b15b61006.json deleted file mode 100644 index 5a9b68220..000000000 --- a/.sqlx/query-82d3a8a3bb864cbeda459065f7d4e413ffefa607a47be92b592d465b15b61006.json +++ /dev/null @@ -1,40 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n SELECT DISTINCT version_id,\n ARRAY_AGG(DISTINCT l.loader) filter (where l.loader is not null) loaders,\n ARRAY_AGG(DISTINCT pt.name) filter (where pt.name is not null) project_types,\n ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games\n FROM versions v\n INNER JOIN loaders_versions lv ON v.id = lv.version_id\n INNER JOIN loaders l ON lv.loader_id = l.id\n INNER JOIN loaders_project_types lpt ON lpt.joining_loader_id = l.id\n INNER JOIN project_types pt ON pt.id = lpt.joining_project_type_id\n INNER JOIN loaders_project_types_games lptg ON lptg.loader_id = l.id AND lptg.project_type_id = pt.id\n INNER JOIN games g ON lptg.game_id = g.id\n WHERE v.id = ANY($1)\n GROUP BY version_id\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "version_id", - "type_info": "Int8" - }, - { - "ordinal": 1, - "name": "loaders", - "type_info": "VarcharArray" - }, - { - "ordinal": 2, - "name": "project_types", - "type_info": "VarcharArray" - }, - { - "ordinal": 3, - "name": "games", - "type_info": "VarcharArray" - } - ], - "parameters": { - "Left": [ - "Int8Array" - ] - }, - "nullable": [ - false, - null, - null, - null - ] - }, - "hash": "82d3a8a3bb864cbeda459065f7d4e413ffefa607a47be92b592d465b15b61006" -} diff --git a/.sqlx/query-e1df7bf2edd30d501a48686c00712784b121db47612bf809d0a0fe0b5d99b681.json b/.sqlx/query-e1df7bf2edd30d501a48686c00712784b121db47612bf809d0a0fe0b5d99b681.json new file mode 100644 index 000000000..793918efb --- /dev/null +++ b/.sqlx/query-e1df7bf2edd30d501a48686c00712784b121db47612bf809d0a0fe0b5d99b681.json @@ -0,0 +1,46 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT DISTINCT mod_id,\n ARRAY_AGG(DISTINCT l.loader) filter (where l.loader is not null) loaders,\n ARRAY_AGG(DISTINCT pt.name) filter (where pt.name is not null) project_types,\n ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games,\n ARRAY_AGG(DISTINCT lfl.loader_field_id) filter (where lfl.loader_field_id is not null) loader_fields\n FROM versions v\n INNER JOIN loaders_versions lv ON v.id = lv.version_id\n INNER JOIN loaders l ON lv.loader_id = l.id\n INNER JOIN loaders_project_types lpt ON lpt.joining_loader_id = l.id\n INNER JOIN project_types pt ON pt.id = lpt.joining_project_type_id\n INNER JOIN loaders_project_types_games lptg ON lptg.loader_id = l.id AND lptg.project_type_id = pt.id\n INNER JOIN games g ON lptg.game_id = g.id\n LEFT JOIN loader_fields_loaders lfl ON lfl.loader_id = l.id\n WHERE v.id = ANY($1)\n GROUP BY mod_id\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "mod_id", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "loaders", + "type_info": "VarcharArray" + }, + { + "ordinal": 2, + "name": "project_types", + "type_info": "VarcharArray" + }, + { + "ordinal": 3, + "name": "games", + "type_info": "VarcharArray" + }, + { + "ordinal": 4, + "name": "loader_fields", + "type_info": "Int4Array" + } + ], + "parameters": { + "Left": [ + "Int8Array" + ] + }, + "nullable": [ + false, + null, + null, + null, + null + ] + }, + "hash": "e1df7bf2edd30d501a48686c00712784b121db47612bf809d0a0fe0b5d99b681" +} diff --git a/.sqlx/query-f3729149bd174541ec4f7ec2145fef0f4ac78e4efb046cc77dcdf43522ef72e2.json b/.sqlx/query-f3729149bd174541ec4f7ec2145fef0f4ac78e4efb046cc77dcdf43522ef72e2.json new file mode 100644 index 000000000..2863c6bf2 --- /dev/null +++ b/.sqlx/query-f3729149bd174541ec4f7ec2145fef0f4ac78e4efb046cc77dcdf43522ef72e2.json @@ -0,0 +1,46 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT DISTINCT version_id,\n ARRAY_AGG(DISTINCT l.loader) filter (where l.loader is not null) loaders,\n ARRAY_AGG(DISTINCT pt.name) filter (where pt.name is not null) project_types,\n ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games,\n ARRAY_AGG(DISTINCT lfl.loader_field_id) filter (where lfl.loader_field_id is not null) loader_fields\n FROM versions v\n INNER JOIN loaders_versions lv ON v.id = lv.version_id\n INNER JOIN loaders l ON lv.loader_id = l.id\n INNER JOIN loaders_project_types lpt ON lpt.joining_loader_id = l.id\n INNER JOIN project_types pt ON pt.id = lpt.joining_project_type_id\n INNER JOIN loaders_project_types_games lptg ON lptg.loader_id = l.id AND lptg.project_type_id = pt.id\n INNER JOIN games g ON lptg.game_id = g.id\n LEFT JOIN loader_fields_loaders lfl ON lfl.loader_id = l.id\n WHERE v.id = ANY($1)\n GROUP BY version_id\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "version_id", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "loaders", + "type_info": "VarcharArray" + }, + { + "ordinal": 2, + "name": "project_types", + "type_info": "VarcharArray" + }, + { + "ordinal": 3, + "name": "games", + "type_info": "VarcharArray" + }, + { + "ordinal": 4, + "name": "loader_fields", + "type_info": "Int4Array" + } + ], + "parameters": { + "Left": [ + "Int8Array" + ] + }, + "nullable": [ + false, + null, + null, + null, + null + ] + }, + "hash": "f3729149bd174541ec4f7ec2145fef0f4ac78e4efb046cc77dcdf43522ef72e2" +} diff --git a/src/database/models/loader_fields.rs b/src/database/models/loader_fields.rs index ee5529b69..5f3f72d4a 100644 --- a/src/database/models/loader_fields.rs +++ b/src/database/models/loader_fields.rs @@ -836,13 +836,19 @@ impl VersionField { } pub fn from_query_json( + // A list of all version fields to extract data from query_version_field_combined: Vec, - query_loader_fields: &[QueryLoaderField], + // A list of all loader fields to reference when extracting data + // Note: any loader field in here that is not in query_version_field_combined will be still considered + // (For example, game_versions in query_loader_fields but not in query_version_field_combined would produce game_versions: []) + query_loader_fields: &[&QueryLoaderField], + // enum values to reference when parsing enum values query_loader_field_enum_values: &[QueryLoaderFieldEnumValue], - allow_many: bool, // If true, will allow multiple values for a single singleton field, returning them as separate VersionFields - // allow_many = true, multiple Bools => two VersionFields of Bool - // allow_many = false, multiple Bools => error - // multiple Arraybools => 1 VersionField of ArrayBool + // If true, will allow multiple values for a single singleton field, returning them as separate VersionFields + // allow_many = true, multiple Bools => two VersionFields of Bool + // allow_many = false, multiple Bools => error + // multiple Arraybools => 1 VersionField of ArrayBool + allow_many: bool, ) -> Vec { query_loader_fields .iter() diff --git a/src/database/models/project_item.rs b/src/database/models/project_item.rs index 0334f578a..8a8251bc7 100644 --- a/src/database/models/project_item.rs +++ b/src/database/models/project_item.rs @@ -607,7 +607,6 @@ impl Project { ) .await?; - let loader_field_ids = DashSet::new(); let loader_field_enum_value_ids = DashSet::new(); let version_fields: DashMap> = sqlx::query!( " @@ -630,7 +629,6 @@ impl Project { string_value: m.string_value, }; - loader_field_ids.insert(LoaderFieldId(m.field_id)); if let Some(enum_value) = m.enum_value { loader_field_enum_value_ids.insert(LoaderFieldEnumValueId(enum_value)); } @@ -641,27 +639,6 @@ impl Project { ) .await?; - let loader_fields: Vec = sqlx::query!( - " - SELECT DISTINCT id, field, field_type, enum_type, min_val, max_val, optional - FROM loader_fields lf - WHERE id = ANY($1) - ", - &loader_field_ids.iter().map(|x| x.0).collect::>() - ) - .fetch(&mut *exec) - .map_ok(|m| QueryLoaderField { - id: LoaderFieldId(m.id), - field: m.field, - field_type: m.field_type, - enum_type: m.enum_type.map(LoaderFieldEnumId), - min_val: m.min_val, - max_val: m.max_val, - optional: m.optional, - }) - .try_collect() - .await?; - let loader_field_enum_values: Vec = sqlx::query!( " SELECT DISTINCT id, enum_id, value, ordering, created, metadata @@ -735,13 +712,22 @@ impl Project { } ).await?; - type StringTriple = (Vec, Vec, Vec); - let loaders_ptypes_games: DashMap = sqlx::query!( + #[derive(Default)] + struct VersionLoaderData { + loaders: Vec, + project_types: Vec, + games: Vec, + loader_loader_field_ids: Vec, + } + + let loader_field_ids = DashSet::new(); + let loaders_ptypes_games: DashMap = sqlx::query!( " SELECT DISTINCT mod_id, ARRAY_AGG(DISTINCT l.loader) filter (where l.loader is not null) loaders, ARRAY_AGG(DISTINCT pt.name) filter (where pt.name is not null) project_types, - ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games + ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games, + ARRAY_AGG(DISTINCT lfl.loader_field_id) filter (where lfl.loader_field_id is not null) loader_fields FROM versions v INNER JOIN loaders_versions lv ON v.id = lv.version_id INNER JOIN loaders l ON lv.loader_id = l.id @@ -749,6 +735,7 @@ impl Project { INNER JOIN project_types pt ON pt.id = lpt.joining_project_type_id INNER JOIN loaders_project_types_games lptg ON lptg.loader_id = l.id AND lptg.project_type_id = pt.id INNER JOIN games g ON lptg.game_id = g.id + LEFT JOIN loader_fields_loaders lfl ON lfl.loader_id = l.id WHERE v.id = ANY($1) GROUP BY mod_id ", @@ -756,15 +743,47 @@ impl Project { ).fetch(&mut *exec) .map_ok(|m| { let project_id = ProjectId(m.mod_id); - let loaders = m.loaders.unwrap_or_default(); - let project_types = m.project_types.unwrap_or_default(); - let games = m.games.unwrap_or_default(); - (project_id, (loaders, project_types, games)) + // Add loader fields to the set we need to fetch + let loader_loader_field_ids = m.loader_fields.unwrap_or_default().into_iter().map(LoaderFieldId).collect::>(); + for loader_field_id in loader_loader_field_ids.iter() { + loader_field_ids.insert(*loader_field_id); + } + + // Add loader + loader associated data to the map + let version_loader_data = VersionLoaderData { + loaders: m.loaders.unwrap_or_default(), + project_types: m.project_types.unwrap_or_default(), + games: m.games.unwrap_or_default(), + loader_loader_field_ids, + }; + + (project_id, version_loader_data) } ).try_collect().await?; + let loader_fields: Vec = sqlx::query!( + " + SELECT DISTINCT id, field, field_type, enum_type, min_val, max_val, optional + FROM loader_fields lf + WHERE id = ANY($1) + ", + &loader_field_ids.iter().map(|x| x.0).collect::>() + ) + .fetch(&mut *exec) + .map_ok(|m| QueryLoaderField { + id: LoaderFieldId(m.id), + field: m.field, + field_type: m.field_type, + enum_type: m.enum_type.map(LoaderFieldEnumId), + min_val: m.min_val, + max_val: m.max_val, + optional: m.optional, + }) + .try_collect() + .await?; + let db_projects: Vec = sqlx::query!( " SELECT m.id id, m.name name, m.summary summary, m.downloads downloads, m.follows follows, @@ -791,11 +810,21 @@ impl Project { Ok(e.right().map(|m| { let id = m.id; let project_id = ProjectId(id); - let (loaders, project_types, games) = loaders_ptypes_games.remove(&project_id).map(|x| x.1).unwrap_or_default(); + let VersionLoaderData { + loaders, + project_types, + games, + loader_loader_field_ids, + } = loaders_ptypes_games.remove(&project_id).map(|x|x.1).unwrap_or_default(); let mut versions = versions.remove(&project_id).map(|x| x.1).unwrap_or_default(); let mut gallery = mods_gallery.remove(&project_id).map(|x| x.1).unwrap_or_default(); let urls = links.remove(&project_id).map(|x| x.1).unwrap_or_default(); let version_fields = version_fields.remove(&project_id).map(|x| x.1).unwrap_or_default(); + + let loader_fields = loader_fields.iter() + .filter(|x| loader_loader_field_ids.contains(&x.id)) + .collect::>(); + QueryProject { inner: Project { id: ProjectId(id), diff --git a/src/database/models/version_item.rs b/src/database/models/version_item.rs index 811f42fc2..eeb6a965d 100644 --- a/src/database/models/version_item.rs +++ b/src/database/models/version_item.rs @@ -510,7 +510,6 @@ impl Version { } if !version_ids_parsed.is_empty() { - let loader_field_ids = DashSet::new(); let loader_field_enum_value_ids = DashSet::new(); let version_fields: DashMap> = sqlx::query!( " @@ -532,7 +531,6 @@ impl Version { string_value: m.string_value, }; - loader_field_ids.insert(LoaderFieldId(m.field_id)); if let Some(enum_value) = m.enum_value { loader_field_enum_value_ids.insert(LoaderFieldEnumValueId(enum_value)); } @@ -543,6 +541,57 @@ impl Version { ) .await?; + #[derive(Default)] + struct VersionLoaderData { + loaders: Vec, + project_types: Vec, + games: Vec, + loader_loader_field_ids: Vec, + } + + let loader_field_ids = DashSet::new(); + let loaders_ptypes_games: DashMap = sqlx::query!( + " + SELECT DISTINCT version_id, + ARRAY_AGG(DISTINCT l.loader) filter (where l.loader is not null) loaders, + ARRAY_AGG(DISTINCT pt.name) filter (where pt.name is not null) project_types, + ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games, + ARRAY_AGG(DISTINCT lfl.loader_field_id) filter (where lfl.loader_field_id is not null) loader_fields + FROM versions v + INNER JOIN loaders_versions lv ON v.id = lv.version_id + INNER JOIN loaders l ON lv.loader_id = l.id + INNER JOIN loaders_project_types lpt ON lpt.joining_loader_id = l.id + INNER JOIN project_types pt ON pt.id = lpt.joining_project_type_id + INNER JOIN loaders_project_types_games lptg ON lptg.loader_id = l.id AND lptg.project_type_id = pt.id + INNER JOIN games g ON lptg.game_id = g.id + LEFT JOIN loader_fields_loaders lfl ON lfl.loader_id = l.id + WHERE v.id = ANY($1) + GROUP BY version_id + ", + &version_ids_parsed + ).fetch(&mut *exec) + .map_ok(|m| { + let version_id = VersionId(m.version_id); + + // Add loader fields to the set we need to fetch + let loader_loader_field_ids = m.loader_fields.unwrap_or_default().into_iter().map(LoaderFieldId).collect::>(); + for loader_field_id in loader_loader_field_ids.iter() { + loader_field_ids.insert(*loader_field_id); + } + + // Add loader + loader associated data to the map + let version_loader_data = VersionLoaderData { + loaders: m.loaders.unwrap_or_default(), + project_types: m.project_types.unwrap_or_default(), + games: m.games.unwrap_or_default(), + loader_loader_field_ids, + }; + (version_id,version_loader_data) + + } + ).try_collect().await?; + + // Fetch all loader fields from any version let loader_fields: Vec = sqlx::query!( " SELECT DISTINCT id, field, field_type, enum_type, min_val, max_val, optional @@ -588,36 +637,6 @@ impl Version { .try_collect() .await?; - type StringTriple = (Vec, Vec, Vec); - let loaders_ptypes_games: DashMap = sqlx::query!( - " - SELECT DISTINCT version_id, - ARRAY_AGG(DISTINCT l.loader) filter (where l.loader is not null) loaders, - ARRAY_AGG(DISTINCT pt.name) filter (where pt.name is not null) project_types, - ARRAY_AGG(DISTINCT g.slug) filter (where g.slug is not null) games - FROM versions v - INNER JOIN loaders_versions lv ON v.id = lv.version_id - INNER JOIN loaders l ON lv.loader_id = l.id - INNER JOIN loaders_project_types lpt ON lpt.joining_loader_id = l.id - INNER JOIN project_types pt ON pt.id = lpt.joining_project_type_id - INNER JOIN loaders_project_types_games lptg ON lptg.loader_id = l.id AND lptg.project_type_id = pt.id - INNER JOIN games g ON lptg.game_id = g.id - WHERE v.id = ANY($1) - GROUP BY version_id - ", - &version_ids_parsed - ).fetch(&mut *exec) - .map_ok(|m| { - let version_id = VersionId(m.version_id); - let loaders = m.loaders.unwrap_or_default(); - let project_types = m.project_types.unwrap_or_default(); - let games = m.games.unwrap_or_default(); - - (version_id, (loaders, project_types, games)) - - } - ).try_collect().await?; - #[derive(Deserialize)] struct Hash { pub file_id: FileId, @@ -729,12 +748,21 @@ impl Version { Ok(e.right().map(|v| { let version_id = VersionId(v.id); - let (loaders, project_types, games) = loaders_ptypes_games.remove(&version_id).map(|x|x.1).unwrap_or_default(); + let VersionLoaderData { + loaders, + project_types, + games, + loader_loader_field_ids, + } = loaders_ptypes_games.remove(&version_id).map(|x|x.1).unwrap_or_default(); let files = files.remove(&version_id).map(|x|x.1).unwrap_or_default(); let hashes = hashes.remove(&version_id).map(|x|x.1).unwrap_or_default(); let version_fields = version_fields.remove(&version_id).map(|x|x.1).unwrap_or_default(); let dependencies = dependencies.remove(&version_id).map(|x|x.1).unwrap_or_default(); + let loader_fields = loader_fields.iter() + .filter(|x| loader_loader_field_ids.contains(&x.id)) + .collect::>(); + QueryVersion { inner: Version { id: VersionId(v.id), diff --git a/src/models/v2/search.rs b/src/models/v2/search.rs index 6d8b3f8a9..5f556afb9 100644 --- a/src/models/v2/search.rs +++ b/src/models/v2/search.rs @@ -42,8 +42,11 @@ pub struct LegacyResultSearchProject { impl LegacyResultSearchProject { pub fn from(result_search_project: ResultSearchProject) -> Self { let mut categories = result_search_project.categories; + categories.extend(result_search_project.loaders); if categories.contains(&"mrpack".to_string()) { - if let Some(mrpack_loaders) = result_search_project.loader_fields.get("mrpack_loaders") + if let Some(mrpack_loaders) = result_search_project + .project_loader_fields + .get("mrpack_loaders") { categories.extend( mrpack_loaders @@ -56,7 +59,9 @@ impl LegacyResultSearchProject { } let mut display_categories = result_search_project.display_categories; if display_categories.contains(&"mrpack".to_string()) { - if let Some(mrpack_loaders) = result_search_project.loader_fields.get("mrpack_loaders") + if let Some(mrpack_loaders) = result_search_project + .project_loader_fields + .get("mrpack_loaders") { categories.extend( mrpack_loaders @@ -93,9 +98,9 @@ impl LegacyResultSearchProject { og_project_type.clone() }; - let loader_fields = result_search_project.loader_fields.clone(); + let project_loader_fields = result_search_project.project_loader_fields.clone(); let get_one_bool_loader_field = |key: &str| { - loader_fields + project_loader_fields .get(key) .cloned() .unwrap_or_default() @@ -119,7 +124,7 @@ impl LegacyResultSearchProject { let server_side = server_side.to_string(); let versions = result_search_project - .loader_fields + .project_loader_fields .get("game_versions") .cloned() .unwrap_or_default() diff --git a/src/models/v3/projects.rs b/src/models/v3/projects.rs index 621037084..f0862bee3 100644 --- a/src/models/v3/projects.rs +++ b/src/models/v3/projects.rs @@ -272,11 +272,16 @@ impl Project { // Loaders let mut loaders = m.loaders; - let mrpack_loaders_strings = m.loader_fields.get("mrpack_loaders").cloned().map(|v| { - v.into_iter() - .filter_map(|v| v.as_str().map(String::from)) - .collect_vec() - }); + let mrpack_loaders_strings = + m.project_loader_fields + .get("mrpack_loaders") + .cloned() + .map(|v| { + v.into_iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect_vec() + }); + // If the project has a mrpack loader, keep only 'loaders' that are not in the mrpack_loaders if let Some(ref mrpack_loaders) = mrpack_loaders_strings { loaders.retain(|l| !mrpack_loaders.contains(l)); @@ -375,7 +380,7 @@ impl Project { thread_id, monetization_status, fields: m - .loader_fields + .project_loader_fields .into_iter() .map(|(k, v)| (k, v.into_iter().collect())) .collect(), diff --git a/src/routes/v2/project_creation.rs b/src/routes/v2/project_creation.rs index 7e1c50536..2fc728ef8 100644 --- a/src/routes/v2/project_creation.rs +++ b/src/routes/v2/project_creation.rs @@ -146,7 +146,7 @@ pub async fn project_create( let payload = v2_reroute::alter_actix_multipart( payload, req.headers().clone(), - |legacy_create: ProjectCreateData| async move { + |legacy_create: ProjectCreateData, _| async move { // Side types will be applied to each version let client_side = legacy_create.client_side; let server_side = legacy_create.server_side; diff --git a/src/routes/v2/version_creation.rs b/src/routes/v2/version_creation.rs index 3ea1497bc..9a634e064 100644 --- a/src/routes/v2/version_creation.rs +++ b/src/routes/v2/version_creation.rs @@ -9,8 +9,10 @@ use crate::models::projects::{ use crate::models::v2::projects::LegacyVersion; use crate::queue::session::AuthQueue; use crate::routes::v3::project_creation::CreateError; +use crate::routes::v3::version_creation; use crate::routes::{v2_reroute, v3}; use actix_multipart::Multipart; +use actix_web::http::header::ContentDisposition; use actix_web::web::Data; use actix_web::{post, web, HttpRequest, HttpResponse}; use serde::{Deserialize, Serialize}; @@ -89,7 +91,7 @@ pub async fn version_create( let payload = v2_reroute::alter_actix_multipart( payload, req.headers().clone(), - |legacy_create: InitialVersionData| { + |legacy_create: InitialVersionData, content_dispositions: Vec| { let client = client.clone(); let redis = redis.clone(); async move { @@ -176,6 +178,19 @@ pub async fn version_create( } } + // Similarly, check actual content disposition for mrpacks, in case file_parts is wrong + for content_disposition in content_dispositions { + // Uses version_create functions to get the file name and extension + let (_, file_extension) = version_creation::get_name_ext(&content_disposition)?; + crate::util::ext::project_file_type(file_extension) + .ok_or_else(|| CreateError::InvalidFileType(file_extension.to_string()))?; + + if file_extension == "mrpack" { + project_type = Some("modpack"); + break; + } + } + // Modpacks now use the "mrpack" loader, and loaders are converted to loader fields. // Setting of 'project_type' directly is removed, it's loader-based now. if project_type == Some("modpack") { diff --git a/src/routes/v2_reroute.rs b/src/routes/v2_reroute.rs index 39c2dcb44..dae4f4101 100644 --- a/src/routes/v2_reroute.rs +++ b/src/routes/v2_reroute.rs @@ -5,7 +5,7 @@ use super::ApiError; use crate::models::v2::projects::LegacySideType; use crate::util::actix::{generate_multipart, MultipartSegment, MultipartSegmentData}; use actix_multipart::Multipart; -use actix_web::http::header::{HeaderMap, TryIntoHeaderPair}; +use actix_web::http::header::{ContentDisposition, HeaderMap, TryIntoHeaderPair}; use actix_web::HttpResponse; use futures::{stream, Future, StreamExt}; use serde_json::{json, Value}; @@ -43,10 +43,15 @@ pub fn flatten_404_error(res: ApiError) -> Result { } } +// Allows internal modification of an actix multipart file +// Expected: +// 1. A json segment +// 2. Any number of other binary segments +// 'closure' is called with the json value, and the content disposition of the other segments pub async fn alter_actix_multipart( mut multipart: Multipart, mut headers: HeaderMap, - mut closure: impl FnMut(T) -> Fut, + mut closure: impl FnMut(T, Vec) -> Fut, ) -> Result where T: serde::de::DeserializeOwned, @@ -55,6 +60,10 @@ where { let mut segments: Vec = Vec::new(); + let mut json = None; + let mut json_segment = None; + let mut content_dispositions = Vec::new(); + if let Some(field) = multipart.next().await { let mut field = field?; let content_disposition = field.content_disposition().clone(); @@ -71,16 +80,15 @@ where { let json_value: T = serde_json::from_slice(&buffer)?; - let json_value: U = closure(json_value).await?; - buffer = serde_json::to_vec(&json_value)?; + json = Some(json_value); } - segments.push(MultipartSegment { + json_segment = Some(MultipartSegment { name: field_name.to_string(), filename: field_filename.map(|s| s.to_string()), content_type: field_content_type, - data: MultipartSegmentData::Binary(buffer), - }) + data: MultipartSegmentData::Binary(vec![]), // Initialize to empty, will be finished after + }); } while let Some(field) = multipart.next().await { @@ -97,6 +105,7 @@ where buffer.extend_from_slice(&data); } + content_dispositions.push(content_disposition.clone()); segments.push(MultipartSegment { name: field_name.to_string(), filename: field_filename.map(|s| s.to_string()), @@ -105,6 +114,24 @@ where }) } + // Finishes the json segment, with aggregated content dispositions + { + let json_value = json.ok_or(CreateError::InvalidInput( + "No json segment found in multipart.".to_string(), + ))?; + let mut json_segment = json_segment.ok_or(CreateError::InvalidInput( + "No json segment found in multipart.".to_string(), + ))?; + + // Call closure, with the json value and names of the other segments + let json_value: U = closure(json_value, content_dispositions).await?; + let buffer = serde_json::to_vec(&json_value)?; + json_segment.data = MultipartSegmentData::Binary(buffer); + + // Insert the json segment at the beginning + segments.insert(0, json_segment); + } + let (boundary, payload) = generate_multipart(segments); match ( diff --git a/src/search/indexing/local_import.rs b/src/search/indexing/local_import.rs index 7508762b7..6636ed5d5 100644 --- a/src/search/indexing/local_import.rs +++ b/src/search/indexing/local_import.rs @@ -128,6 +128,8 @@ pub async fn index_local( .map(|vf| (vf.field_name.clone(), vf.value.serialize_internal())) .collect(); let mut loader_fields = models::projects::from_duplicate_version_fields(version_fields); + let project_loader_fields = + models::projects::from_duplicate_version_fields(m.aggregate_version_fields.clone()); let license = match m.inner.license.split(' ').next() { Some(license) => license.to_string(), None => m.inner.license.clone(), @@ -240,6 +242,7 @@ pub async fn index_local( links: m.urls.clone(), gallery_items: m.gallery_items.clone(), loaders, + project_loader_fields, }; uploads.push(usp); diff --git a/src/search/indexing/mod.rs b/src/search/indexing/mod.rs index ec4f9f107..8492e8b98 100644 --- a/src/search/indexing/mod.rs +++ b/src/search/indexing/mod.rs @@ -400,6 +400,7 @@ const DEFAULT_DISPLAYED_ATTRIBUTES: &[&str] = &[ "links", "gallery_items", "loaders", // search uses loaders as categories- this is purely for the Project model. + "project_loader_fields", ]; const DEFAULT_SEARCHABLE_ATTRIBUTES: &[&str] = &["name", "summary", "author", "slug"]; diff --git a/src/search/mod.rs b/src/search/mod.rs index 74d2ee0c1..5c64b3c55 100644 --- a/src/search/mod.rs +++ b/src/search/mod.rs @@ -133,6 +133,7 @@ pub struct UploadSearchProject { pub gallery_items: Vec, // Gallery *only* urls are stored in gallery, but the gallery items are stored here- required for the Project model. pub games: Vec, // Todo: in future, could be a searchable field. pub organization_id: Option, // Todo: in future, could be a searchable field. + pub project_loader_fields: HashMap>, // Aggregation of loader_fields from all versions of the project, allowing for reconstruction of the Project model. #[serde(flatten)] pub loader_fields: HashMap>, @@ -184,6 +185,7 @@ pub struct ResultSearchProject { pub gallery_items: Vec, // Gallery *only* urls are stored in gallery, but the gallery items are stored here- required for the Project model. pub games: Vec, // Todo: in future, could be a searchable field. pub organization_id: Option, // Todo: in future, could be a searchable field. + pub project_loader_fields: HashMap>, // Aggregation of loader_fields from all versions of the project, allowing for reconstruction of the Project model. #[serde(flatten)] pub loader_fields: HashMap>, diff --git a/tests/common/api_v3/project.rs b/tests/common/api_v3/project.rs index 7c1dc51d0..8382ae1e6 100644 --- a/tests/common/api_v3/project.rs +++ b/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 diff --git a/tests/loader_fields.rs b/tests/loader_fields.rs index 5f4b1d037..53ca5f858 100644 --- a/tests/loader_fields.rs +++ b/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| 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 = 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 = 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; +} diff --git a/tests/project.rs b/tests/project.rs index 9ab5a3825..7ef74e3f9 100644 --- a/tests/project.rs +++ b/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); diff --git a/tests/v2/search.rs b/tests/v2/search.rs index c4d4066f4..fbad4fd94 100644 --- a/tests/v2/search.rs +++ b/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()); } }) diff --git a/tests/v2/version.rs b/tests/v2/version.rs index 12ac86cc4..c4eceea1c 100644 --- a/tests/v2/version.rs +++ b/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| 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; +}