From 922b68be632705efb300dbd17523490e9159528e Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sun, 5 Feb 2023 20:10:29 +0000 Subject: [PATCH] Fix cyclic interaction errors listing too many vertices --- src/api/sorting/group_sort.cpp | 29 +++++----- src/api/sorting/plugin_graph.cpp | 24 ++++---- .../api/internals/sorting/group_sort_test.h | 40 +++++++++++++ .../api/internals/sorting/plugin_graph_test.h | 58 +++++++++++++++++++ 4 files changed, 124 insertions(+), 27 deletions(-) diff --git a/src/api/sorting/group_sort.cpp b/src/api/sorting/group_sort.cpp index 9499679f..d3cae619 100644 --- a/src/api/sorting/group_sort.cpp +++ b/src/api/sorting/group_sort.cpp @@ -91,23 +91,13 @@ public: auto vertex = Vertex(graph[source], graph[edge]); - // Check if the vertex already exists in the recorded trail. - auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { - return v.GetName() == graph[source]; - }); - - if (it != end(trail)) { - // Erase everything from this position onwards, as it doesn't - // contribute to a forward-cycle. - trail.erase(it, end(trail)); - } - trail.push_back(vertex); } void back_edge(edge_t edge, const GroupGraph& graph) { auto source = boost::source(edge, graph); - auto target = boost::target(edge, graph); + const auto target = boost::target(edge, graph); + const auto targetGroupName = graph[target]; auto vertex = Vertex(graph[source], graph[edge]); trail.push_back(vertex); @@ -116,8 +106,19 @@ public: return v.GetName() == graph[target]; }); - if (it != trail.end()) { - throw CyclicInteractionError(std::vector(it, trail.end())); + if (it == trail.end()) { + throw std::logic_error( + "The target of a back edge cannot be found in the current edge path. " + "The target group is \"" + + targetGroupName + "\""); + } + + throw CyclicInteractionError(std::vector(it, trail.end())); + } + + void finish_vertex(vertex_t, const GroupGraph&) { + if (!trail.empty()) { + trail.pop_back(); } } diff --git a/src/api/sorting/plugin_graph.cpp b/src/api/sorting/plugin_graph.cpp index f1509cb0..abbaf5b5 100644 --- a/src/api/sorting/plugin_graph.cpp +++ b/src/api/sorting/plugin_graph.cpp @@ -47,17 +47,6 @@ public: const auto vertex = Vertex(graph[source].GetName(), graph[edge]); - // Check if the vertex already exists in the recorded trail. - const auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { - return v.GetName() == graph[source].GetName(); - }); - - if (it != end(trail)) { - // Erase everything from this position onwards, as it doesn't - // contribute to a forward-cycle. - trail.erase(it, end(trail)); - } - trail.push_back(vertex); } @@ -72,8 +61,17 @@ public: return v.GetName() == graph[target].GetName(); }); - if (it != trail.end()) { - throw CyclicInteractionError(std::vector(it, trail.end())); + if (it == trail.end()) { + throw std::logic_error( + "The target of a back edge cannot be found in the current edge path"); + } + + throw CyclicInteractionError(std::vector(it, trail.end())); + } + + void finish_vertex(vertex_t, const RawPluginGraph&) { + if (!trail.empty()) { + trail.pop_back(); } } diff --git a/src/tests/api/internals/sorting/group_sort_test.h b/src/tests/api/internals/sorting/group_sort_test.h index dccafe6f..313b648a 100644 --- a/src/tests/api/internals/sorting/group_sort_test.h +++ b/src/tests/api/internals/sorting/group_sort_test.h @@ -165,6 +165,46 @@ TEST(GetPredecessorGroups, shouldThrowIfAfterGroupsAreCyclic) { } } +TEST(GetPredecessorGroups, shouldNotThrowIfThereIsNoCycle) { + std::vector groups({Group("a"), Group("b", {"a"})}); + + EXPECT_NO_THROW(GetPredecessorGroups(groups, {})); +} + +TEST(GetPredecessorGroups, shouldThrowIfThereIsACycle) { + std::vector groups({Group("a", {"b"}), Group("b", {"a"})}); + + try { + GetPredecessorGroups(groups, {}); + } catch (const CyclicInteractionError& e) { + ASSERT_EQ(2, e.GetCycle().size()); + EXPECT_EQ("a", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("b", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + } +} + +TEST(GetPredecessorGroups, + exceptionThrownShouldOnlyRecordGroupsThatArePartOfTheCycle) { + std::vector groups( + {Group("a", {"b"}), Group("b", {"a"}), Group("c", {"b"})}); + + try { + GetPredecessorGroups(groups, {}); + } catch (const CyclicInteractionError& e) { + ASSERT_EQ(2, e.GetCycle().size()); + EXPECT_EQ("a", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("b", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + } +} + TEST(GetGroupsPath, shouldThrowIfTheFromGroupDoesNotExist) { std::vector groups({Group("a"), Group("b", {"a"})}); std::vector userGroups({Group("a", {"c"}), Group("c", {"b"})}); diff --git a/src/tests/api/internals/sorting/plugin_graph_test.h b/src/tests/api/internals/sorting/plugin_graph_test.h index b943fd07..d32509ed 100644 --- a/src/tests/api/internals/sorting/plugin_graph_test.h +++ b/src/tests/api/internals/sorting/plugin_graph_test.h @@ -194,6 +194,64 @@ private: std::map> plugins; }; +TEST_F(PluginGraphTest, checkForCyclesShouldNotThrowIfThereIsNoCycle) { + PluginGraph graph; + + const auto a = graph.AddVertex(CreatePluginSortingData("A.esp")); + const auto b = graph.AddVertex(CreatePluginSortingData("B.esp")); + + graph.AddEdge(a, b, EdgeType::master); + + EXPECT_NO_THROW(graph.CheckForCycles()); +} + +TEST_F(PluginGraphTest, checkForCyclesShouldThrowIfThereIsACycle) { + PluginGraph graph; + + const auto a = graph.AddVertex(CreatePluginSortingData("A.esp")); + const auto b = graph.AddVertex(CreatePluginSortingData("B.esp")); + + graph.AddEdge(a, b, EdgeType::master); + graph.AddEdge(b, a, EdgeType::masterFlag); + + try { + graph.CheckForCycles(); + FAIL(); + } catch (const CyclicInteractionError& e) { + ASSERT_EQ(2, e.GetCycle().size()); + EXPECT_EQ("A.esp", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::master, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("B.esp", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::masterFlag, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + } +} + +TEST_F(PluginGraphTest, + checkForCyclesShouldOnlyRecordPluginsThatArePartOfTheCycle) { + PluginGraph graph; + + const auto a = graph.AddVertex(CreatePluginSortingData("A.esp")); + const auto b = graph.AddVertex(CreatePluginSortingData("B.esp")); + const auto c = graph.AddVertex(CreatePluginSortingData("C.esp")); + + graph.AddEdge(a, b, EdgeType::master); + graph.AddEdge(b, c, EdgeType::master); + graph.AddEdge(b, a, EdgeType::masterFlag); + + try { + graph.CheckForCycles(); + FAIL(); + } catch (const CyclicInteractionError& e) { + ASSERT_EQ(2, e.GetCycle().size()); + EXPECT_EQ("A.esp", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::master, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("B.esp", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::masterFlag, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + } +} + TEST_F(PluginGraphTest, topologicalSortWithNoLoadedPluginsShouldReturnAnEmptyList) { PluginGraph graph;