From 9e06db4d16ed44839ea8be3981bd39505b9e0a64 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Fri, 13 Jan 2023 17:20:14 +0000 Subject: [PATCH] Fix avoidance of group cycles involving user metadata This bug was introduced in 53e2dbba1fbd55660f1e73dae9b5e7bc7a90a69a, and affects libloot v0.19.1. --- src/api/api_database.cpp | 31 +---------- src/api/sorting/group_sort.cpp | 36 +++++++++++++ src/api/sorting/group_sort.h | 3 ++ src/api/sorting/plugin_sort.cpp | 7 ++- .../api/internals/sorting/plugin_sort_test.h | 54 +++++++++++++++++++ 5 files changed, 98 insertions(+), 33 deletions(-) diff --git a/src/api/api_database.cpp b/src/api/api_database.cpp index 1419a062..2618be13 100644 --- a/src/api/api_database.cpp +++ b/src/api/api_database.cpp @@ -139,38 +139,11 @@ std::vector ApiDatabase::GetGeneralMessages( } std::vector ApiDatabase::GetGroups(bool includeUserMetadata) const { - auto groups = masterlist_.Groups(); - if (includeUserMetadata) { - std::vector newGroups; - for (const auto& userlistGroup : userlist_.Groups()) { - auto groupIt = std::find_if( - groups.begin(), groups.end(), [&](const Group& existingGroup) { - return existingGroup.GetName() == userlistGroup.GetName(); - }); - - if (groupIt == groups.end()) { - newGroups.push_back(userlistGroup); - } else { - // Replace the masterlist group description with the userlist group - // description if the latter is not empty. - auto description = userlistGroup.GetDescription().empty() - ? groupIt->GetDescription() - : userlistGroup.GetDescription(); - - auto afterGroups = groupIt->GetAfterGroups(); - auto userAfterGroups = userlistGroup.GetAfterGroups(); - afterGroups.insert( - afterGroups.end(), userAfterGroups.begin(), userAfterGroups.end()); - - *groupIt = Group(userlistGroup.GetName(), afterGroups, description); - } - } - - groups.insert(groups.end(), newGroups.cbegin(), newGroups.cend()); + return MergeGroups(masterlist_.Groups(), userlist_.Groups()); } - return groups; + return masterlist_.Groups(); } std::vector ApiDatabase::GetUserGroups() const { diff --git a/src/api/sorting/group_sort.cpp b/src/api/sorting/group_sort.cpp index c1f0cea9..9499679f 100644 --- a/src/api/sorting/group_sort.cpp +++ b/src/api/sorting/group_sort.cpp @@ -327,4 +327,40 @@ std::vector GetGroupsPath(const std::vector& masterlistGroups, return path; } + +std::vector MergeGroups(const std::vector& masterlistGroups, + const std::vector& userGroups) { + auto mergedGroups = masterlistGroups; + + std::vector newGroups; + for (const auto& userGroup : userGroups) { + auto groupIt = + std::find_if(mergedGroups.begin(), + mergedGroups.end(), + [&](const Group& existingGroup) { + return existingGroup.GetName() == userGroup.GetName(); + }); + + if (groupIt == mergedGroups.end()) { + newGroups.push_back(userGroup); + } else { + // Replace the masterlist group description with the userlist group + // description if the latter is not empty. + auto description = userGroup.GetDescription().empty() + ? groupIt->GetDescription() + : userGroup.GetDescription(); + + auto afterGroups = groupIt->GetAfterGroups(); + auto userAfterGroups = userGroup.GetAfterGroups(); + afterGroups.insert( + afterGroups.end(), userAfterGroups.begin(), userAfterGroups.end()); + + *groupIt = Group(userGroup.GetName(), afterGroups, description); + } + } + + mergedGroups.insert(mergedGroups.end(), newGroups.cbegin(), newGroups.cend()); + + return mergedGroups; +} } diff --git a/src/api/sorting/group_sort.h b/src/api/sorting/group_sort.h index 16c22be6..4d1337da 100644 --- a/src/api/sorting/group_sort.h +++ b/src/api/sorting/group_sort.h @@ -47,5 +47,8 @@ std::vector GetGroupsPath(const std::vector& masterlistGroups, const std::vector& userGroups, const std::string& fromGroupName, const std::string& toGroupName); + +std::vector MergeGroups(const std::vector& masterlistGroups, + const std::vector& userGroups); } #endif diff --git a/src/api/sorting/plugin_sort.cpp b/src/api/sorting/plugin_sort.cpp index 856946a9..be5c98df 100644 --- a/src/api/sorting/plugin_sort.cpp +++ b/src/api/sorting/plugin_sort.cpp @@ -90,11 +90,10 @@ std::vector GetPluginsWithHardcodedPositions( std::unordered_map GetGroupsMap( const std::vector masterlistGroups, const std::vector userGroups) { + const auto mergedGroups = MergeGroups(masterlistGroups, userGroups); + std::unordered_map groupsMap; - for (const auto& group : masterlistGroups) { - groupsMap.emplace(group.GetName(), group); - } - for (const auto& group : userGroups) { + for (const auto& group : mergedGroups) { groupsMap.emplace(group.GetName(), group); } diff --git a/src/tests/api/internals/sorting/plugin_sort_test.h b/src/tests/api/internals/sorting/plugin_sort_test.h index 85ecdf1b..5e5aef0f 100644 --- a/src/tests/api/internals/sorting/plugin_sort_test.h +++ b/src/tests/api/internals/sorting/plugin_sort_test.h @@ -266,6 +266,60 @@ TEST_P(PluginSortTest, sortingShouldResolveGroupsAsTransitiveLoadAfterSets) { EXPECT_EQ(expectedSortedOrder, sorted); } +TEST_P(PluginSortTest, + sortingShouldAccountForUserGroupMetadataWhenTryingToAvoidCycles) { + using std::endl; + + ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); + + const auto masterlistPath = metadataFilesPath / "masterlist.yaml"; + std::ofstream masterlist(masterlistPath); + masterlist << "groups:" << endl + << " - name: default" << endl + << " - name: B" << endl + << " after:" << endl + << " - default" << endl; + masterlist.close(); + + game_.GetDatabase().LoadLists(masterlistPath); + + game_.GetDatabase().SetUserGroups( + {Group("A", {"default"}), Group("B", {"A"})}); + + PluginMetadata plugin(blankEsm); + plugin.SetGroup("B"); + game_.GetDatabase().SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankMasterDependentEsm); + plugin.SetGroup("default"); + game_.GetDatabase().SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankDifferentEsm); + plugin.SetGroup("A"); + game_.GetDatabase().SetPluginUserMetadata(plugin); + + std::vector expectedSortedOrder({ + masterFile, + blankDifferentEsm, + blankDifferentMasterDependentEsm, + blankEsm, + blankMasterDependentEsm, + blankEsp, + blankDifferentEsp, + blankMasterDependentEsp, + blankDifferentMasterDependentEsp, + blankPluginDependentEsp, + blankDifferentPluginDependentEsp, + }); + + if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) { + expectedSortedOrder.insert(expectedSortedOrder.begin() + 1, blankEsl); + } + + std::vector sorted = SortPlugins(game_, game_.GetLoadOrder()); + EXPECT_EQ(expectedSortedOrder, sorted); +} + TEST_P(PluginSortTest, sortingShouldThrowIfAPluginHasAGroupThatDoesNotExist) { ASSERT_NO_THROW(loadInstalledPlugins(game_, false));