Simplify group graph construction

Only store group names and edge types in the graph.

This also changes behaviour slightly, so that masterlist metadata cannot
have a group loading after another group that is only defined in user
metadata. This was previously allowed.

src/api/sorting/group_sort.cpp

@@ -34,39 +34,10 @@
 #include "loot/exception/undefined_group_error.h"
 
 namespace loot {
-class GroupSortingData {
-public:
-  GroupSortingData() {}
-  GroupSortingData(std::string name) : name_(name) {}
-
-  std::string GetName() const { return name_; }
-
-  std::unordered_set<std::string> GetMasterlistAfterGroups() const {
-    return masterlistAfterGroups_;
-  }
-
-  std::unordered_set<std::string> GetUserAfterGroups() const {
-    return userAfterGroups_;
-  }
-
-  void SetMasterlistAfterGroups(std::unordered_set<std::string> groups) {
-    masterlistAfterGroups_ = groups;
-  }
-
-  void SetUserAfterGroups(std::unordered_set<std::string> groups) {
-    userAfterGroups_ = groups;
-  }
-
-private:
-  std::string name_;
-  std::unordered_set<std::string> masterlistAfterGroups_;
-  std::unordered_set<std::string> userAfterGroups_;
-};
-
 typedef boost::adjacency_list<boost::vecS,
                               boost::vecS,
                               boost::directedS,
-                              GroupSortingData,
+                              std::string,
                               EdgeType>
     GroupGraph;
 typedef boost::graph_traits<GroupGraph>::vertex_descriptor vertex_t;
@@ -79,7 +50,7 @@ public:
       visitedGroups_(visitedGroups) {}
   void tree_edge(edge_t edge, const GroupGraph& graph) {
     auto target = boost::target(edge, graph);
-    visitedGroups_.insert(graph[target].GetName());
+    visitedGroups_.insert(graph[target]);
   }
 
   std::unordered_set<std::string> get_visited_groups() const {
@@ -90,6 +61,47 @@ private:
   std::unordered_set<std::string>& visitedGroups_;
 };
 
+class CycleDetector : public boost::dfs_visitor<> {
+public:
+  void tree_edge(edge_t edge, const GroupGraph& graph) {
+    auto source = boost::source(edge, graph);
+
+    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);
+
+    auto vertex = Vertex(graph[source], graph[edge]);
+    trail.push_back(vertex);
+
+    auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) {
+      return v.GetName() == graph[target];
+    });
+
+    if (it != trail.end()) {
+      throw CyclicInteractionError(std::vector<Vertex>(it, trail.end()));
+    }
+  }
+
+private:
+  std::vector<Vertex> trail;
+};
+
 std::string join(const std::unordered_set<std::string>& set) {
   std::string output;
   for (const auto& element : set) {
@@ -105,57 +117,52 @@ GroupGraph BuildGraph(const std::unordered_set<Group>& masterlistGroups,
 
   std::unordered_map<std::string, vertex_t> groupVertices;
   for (const auto& group : masterlistGroups) {
-    auto groupSortingData = GroupSortingData(group.GetName());
-    groupSortingData.SetMasterlistAfterGroups(group.GetAfterGroups());
-
-    auto vertex = boost::add_vertex(groupSortingData, graph);
+    auto vertex = boost::add_vertex(group.GetName(), graph);
     groupVertices.emplace(group.GetName(), vertex);
   }
-  for (const auto& group : userGroups) {
-    auto it = groupVertices.find(group.GetName());
-    if (it != groupVertices.end()) {
-      graph[it->second].SetUserAfterGroups(group.GetAfterGroups());
-    } else {
-      auto groupSortingData = GroupSortingData(group.GetName());
-      groupSortingData.SetUserAfterGroups(group.GetAfterGroups());
 
-      auto vertex = boost::add_vertex(groupSortingData, graph);
+  auto logger = getLogger();
+  for (const auto& group : masterlistGroups) {
+    if (logger) {
+      logger->trace(
+          "Masterlist group \"{}\" directly loads after groups \"{}\"",
+          group.GetName(),
+          join(group.GetAfterGroups()));
+    }
+
+    auto vertex = groupVertices.at(group.GetName());
+    for (const auto& otherGroupName : group.GetAfterGroups()) {
+      auto otherVertex = groupVertices.find(otherGroupName);
+      if (otherVertex == groupVertices.end()) {
+        throw UndefinedGroupError(otherGroupName);
+      }
+
+      boost::add_edge(
+          vertex, otherVertex->second, EdgeType::masterlistLoadAfter, graph);
+    }
+  }
+
+  for (const auto& group : userGroups) {
+    if (groupVertices.find(group.GetName()) == groupVertices.end()) {
+      auto vertex = boost::add_vertex(group.GetName(), graph);
       groupVertices.emplace(group.GetName(), vertex);
     }
   }
 
-  auto logger = getLogger();
-  for (const vertex_t& vertex :
-       boost::make_iterator_range(boost::vertices(graph))) {
-    auto group = graph[vertex];
-
+  for (const auto& group : userGroups) {
     if (logger) {
-      logger->trace(
-          "Group \"{}\" directly loads after masterlist groups \"{}\" and user "
-          "groups \"{}\"",
-          group.GetName(),
-          join(group.GetMasterlistAfterGroups()),
-          join(group.GetUserAfterGroups()));
+      logger->trace("Userlist group \"{}\" directly loads after groups \"{}\"",
+                    group.GetName(),
+                    join(group.GetAfterGroups()));
     }
 
-    for (const auto& otherGroupName : group.GetMasterlistAfterGroups()) {
+    auto vertex = groupVertices.at(group.GetName());
+    for (const auto& otherGroupName : group.GetAfterGroups()) {
       auto otherVertex = groupVertices.find(otherGroupName);
       if (otherVertex == groupVertices.end()) {
         throw UndefinedGroupError(otherGroupName);
       }
 
-      auto vertex = groupVertices[group.GetName()];
-      boost::add_edge(
-          vertex, otherVertex->second, EdgeType::masterlistLoadAfter, graph);
-    }
-
-    for (const auto& otherGroupName : group.GetUserAfterGroups()) {
-      auto otherVertex = groupVertices.find(otherGroupName);
-      if (otherVertex == groupVertices.end()) {
-        throw UndefinedGroupError(otherGroupName);
-      }
-
-      auto vertex = groupVertices[group.GetName()];
       boost::add_edge(
           vertex, otherVertex->second, EdgeType::userLoadAfter, graph);
     }
@@ -178,7 +185,7 @@ GetTransitiveAfterGroups(const std::unordered_set<Group>& masterlistGroups,
   if (logger) {
     logger->trace("Checking for cycles in the group graph");
   }
-  boost::depth_first_search(graph, boost::visitor(CycleDetector<GroupGraph>()));
+  boost::depth_first_search(graph, boost::visitor(CycleDetector()));
 
   std::unordered_map<std::string, std::unordered_set<std::string>>
       transitiveAfterGroups;
@@ -193,11 +200,11 @@ GetTransitiveAfterGroups(const std::unordered_set<Group>& masterlistGroups,
         colorVec.begin(), boost::get(boost::vertex_index, graph), colorVec[0]);
 
     boost::depth_first_visit(graph, vertex, afterGroupsVisitor, colorMap);
-    transitiveAfterGroups[graph[vertex].GetName()] = visitedGroups;
+    transitiveAfterGroups[graph[vertex]] = visitedGroups;
 
     if (logger) {
       logger->trace("Group \"{}\" transitively loads after groups \"{}\"",
-                    graph[vertex].GetName(),
+                    graph[vertex],
                     join(visitedGroups));
     }
   }
@@ -208,7 +215,7 @@ GetTransitiveAfterGroups(const std::unordered_set<Group>& masterlistGroups,
 vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) {
   for (const auto& vertex :
        boost::make_iterator_range(boost::vertices(graph))) {
-    if (graph[vertex].GetName() == name) {
+    if (graph[vertex] == name) {
       return vertex;
     }
   }
@@ -221,7 +228,6 @@ vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) {
   throw std::invalid_argument("Can't find group with name \"" + name + "\"");
 }
 
-
 std::vector<Vertex> GetGroupsPath(
     const std::unordered_set<Group>& masterlistGroups,
     const std::unordered_set<Group>& userGroups,
@@ -264,8 +270,8 @@ std::vector<Vertex> GetGroupsPath(
       if (logger) {
         logger->error(
             "Unreachable vertex {} encountered while looking for vertex {}",
-            graph[currentVertex].GetName(),
-            graph[toVertex].GetName());
+            graph[currentVertex],
+            graph[toVertex]);
       }
       return std::vector<Vertex>();
     }
@@ -273,15 +279,15 @@ std::vector<Vertex> GetGroupsPath(
     auto pair = boost::edge(nextVertex, currentVertex, graph);
     if (!pair.second) {
       throw std::runtime_error("Unexpectedly couldn't find edge between \"" +
-                               graph[currentVertex].GetName() + "\" and \"" +
-                               graph[nextVertex].GetName() + "\"");
+                               graph[currentVertex] + "\" and \"" +
+                               graph[nextVertex] + "\"");
     }
-    auto vertex = Vertex(graph[currentVertex].GetName(), graph[pair.first]);
+    auto vertex = Vertex(graph[currentVertex], graph[pair.first]);
     path.push_back(vertex);
 
     currentVertex = nextVertex;
   }
-  path.push_back(Vertex(graph[currentVertex].GetName()));
+  path.push_back(Vertex(graph[currentVertex]));
 
   return path;
 }

src/api/sorting/group_sort.h

@@ -47,49 +47,5 @@ std::vector<Vertex> GetGroupsPath(
     const std::unordered_set<Group>& userGroups,
     const std::string& fromGroupName,
     const std::string& toGroupName);
-
-template<typename G>
-class CycleDetector : public boost::dfs_visitor<> {
-public:
-  void tree_edge(typename boost::graph_traits<G>::edge_descriptor edge,
-                 const G& graph) {
-    auto source = boost::source(edge, graph);
-
-    auto vertex = Vertex(graph[source].GetName(), 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].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);
-  }
-
-  void back_edge(typename boost::graph_traits<G>::edge_descriptor edge,
-                 const G& graph) {
-    auto source = boost::source(edge, graph);
-    auto target = boost::target(edge, graph);
-
-    auto vertex = Vertex(graph[source].GetName(), graph[edge]);
-    trail.push_back(vertex);
-
-    auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) {
-      return v.GetName() == graph[target].GetName();
-    });
-
-    if (it != trail.end()) {
-      throw CyclicInteractionError(std::vector<Vertex>(it, trail.end()));
-    }
-  }
-
-private:
-  std::vector<Vertex> trail;
-};
 }
 #endif

src/api/sorting/plugin_sorter.cpp

@@ -49,6 +49,47 @@ typedef boost::graph_traits<PluginGraph>::vertex_iterator vertex_it;
 typedef boost::graph_traits<PluginGraph>::edge_descriptor edge_t;
 typedef boost::graph_traits<PluginGraph>::edge_iterator edge_it;
 
+class CycleDetector : public boost::dfs_visitor<> {
+public:
+  void tree_edge(edge_t edge, const PluginGraph& graph) {
+    auto source = boost::source(edge, graph);
+
+    auto vertex = Vertex(graph[source].GetName(), 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].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);
+  }
+
+  void back_edge(edge_t edge, const PluginGraph& graph) {
+    auto source = boost::source(edge, graph);
+    auto target = boost::target(edge, graph);
+
+    auto vertex = Vertex(graph[source].GetName(), graph[edge]);
+    trail.push_back(vertex);
+
+    auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) {
+      return v.GetName() == graph[target].GetName();
+    });
+
+    if (it != trail.end()) {
+      throw CyclicInteractionError(std::vector<Vertex>(it, trail.end()));
+    }
+  }
+
+private:
+  std::vector<Vertex> trail;
+};
+
 std::string describeEdgeType(EdgeType edgeType) {
   switch (edgeType) {
     case EdgeType::hardcoded:
@@ -264,8 +305,7 @@ void PluginSorter::CheckForCycles() const {
     logger_->trace("Checking plugin graph for cycles...");
   }
   boost::depth_first_search(
-      graph_,
-      visitor(CycleDetector<PluginGraph>()).vertex_index_map(vertexIndexMap_));
+      graph_, visitor(CycleDetector()).vertex_index_map(vertexIndexMap_));
 }
 
 bool PluginSorter::EdgeCreatesCycle(const vertex_t& fromVertex,

src/tests/api/internals/sorting/group_sort_test.h

@@ -36,9 +36,7 @@ namespace loot {
 namespace test {
 TEST(GetTransitiveAfterGroups, shouldMapGroupsToTheirTransitiveAfterGroups) {
   std::unordered_set<Group> groups(
-      {Group("a"),
-       Group("b", std::unordered_set<std::string>({"a"})),
-       Group("c", std::unordered_set<std::string>({"b"}))});
+      {Group("a"), Group("b", {"a"}), Group("c", {"b"})});
 
   auto mapped = GetTransitiveAfterGroups(groups, {});
 
@@ -48,18 +46,14 @@ TEST(GetTransitiveAfterGroups, shouldMapGroupsToTheirTransitiveAfterGroups) {
 }
 
 TEST(GetTransitiveAfterGroups, shouldThrowIfAnAfterGroupDoesNotExist) {
-  std::unordered_set<Group> groups(
-      {Group("b", std::unordered_set<std::string>({"a"}))});
+  std::unordered_set<Group> groups({Group("b", {"a"})});
 
   EXPECT_THROW(GetTransitiveAfterGroups(groups, {}), UndefinedGroupError);
 }
 
 TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) {
-  std::unordered_set<Group> groups(
-      {Group("a", std::unordered_set<std::string>({"c"})),
-       Group("b", std::unordered_set<std::string>({"a"}))});
-  std::unordered_set<Group> userGroups(
-      {Group("c", std::unordered_set<std::string>({"b"}))});
+  std::unordered_set<Group> groups({Group("a"), Group("b", {"a"})});
+  std::unordered_set<Group> userGroups({Group("a", {"c"}), Group("c", {"b"})});
 
   try {
     GetTransitiveAfterGroups(groups, userGroups);
@@ -69,7 +63,7 @@ TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) {
 
     // Vertices can be added in any order, so which group is first is undefined.
     if (e.GetCycle()[0].GetName() == "a") {
-      EXPECT_EQ(EdgeType::masterlistLoadAfter,
+      EXPECT_EQ(EdgeType::userLoadAfter,
                 e.GetCycle()[0].GetTypeOfEdgeToNextVertex());
 
       EXPECT_EQ("c", e.GetCycle()[1].GetName());
@@ -84,7 +78,7 @@ TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) {
                 e.GetCycle()[0].GetTypeOfEdgeToNextVertex());
 
       EXPECT_EQ("a", e.GetCycle()[1].GetName());
-      EXPECT_EQ(EdgeType::masterlistLoadAfter,
+      EXPECT_EQ(EdgeType::userLoadAfter,
                 e.GetCycle()[1].GetTypeOfEdgeToNextVertex());
 
       EXPECT_EQ("c", e.GetCycle()[2].GetName());
@@ -100,23 +94,23 @@ TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) {
                 e.GetCycle()[1].GetTypeOfEdgeToNextVertex());
 
       EXPECT_EQ("a", e.GetCycle()[2].GetName());
-      EXPECT_EQ(EdgeType::masterlistLoadAfter,
+      EXPECT_EQ(EdgeType::userLoadAfter,
                 e.GetCycle()[2].GetTypeOfEdgeToNextVertex());
     }
   }
 }
 
 TEST(GetGroupsPath, shouldThrowIfTheFromGroupDoesNotExist) {
-  std::unordered_set<Group> groups({Group("a", {"c"}), Group("b", {"a"})});
-  std::unordered_set<Group> userGroups({Group("c", {"b"})});
+  std::unordered_set<Group> groups({Group("a"), Group("b", {"a"})});
+  std::unordered_set<Group> userGroups({Group("a", {"c"}), Group("c", {"b"})});
 
   EXPECT_THROW(GetGroupsPath(groups, userGroups, "d", "a"),
                std::invalid_argument);
 }
 
 TEST(GetGroupsPath, shouldThrowIfTheToGroupDoesNotExist) {
-  std::unordered_set<Group> groups({Group("a", {"c"}), Group("b", {"a"})});
-  std::unordered_set<Group> userGroups({Group("c", {"b"})});
+  std::unordered_set<Group> groups({Group("a"), Group("b", {"a"})});
+  std::unordered_set<Group> userGroups({Group("a", {"c"}), Group("c", {"b"})});
 
   EXPECT_THROW(GetGroupsPath(groups, userGroups, "a", "d"),
                std::invalid_argument);
@@ -161,8 +155,8 @@ TEST(GetGroupsPath,
   std::unordered_set<Group> groups({Group("a", {}),
                                     Group("b", {"a"}),
                                     Group("c", {"a"}),
-                                    Group("e", {"b", "d"})});
-  std::unordered_set<Group> userGroups({Group("d", {"c"})});
+                                    Group("e", {"b"})});
+  std::unordered_set<Group> userGroups({Group("d", {"c"}), Group("e", {"d"})});
 
   auto path = GetGroupsPath(groups, userGroups, "a", "e");
 
@@ -174,11 +168,22 @@ TEST(GetGroupsPath,
   EXPECT_EQ(EdgeType::userLoadAfter,
             path[1].GetTypeOfEdgeToNextVertex().value());
   EXPECT_EQ("d", path[2].GetName());
-  EXPECT_EQ(EdgeType::masterlistLoadAfter,
+  EXPECT_EQ(EdgeType::userLoadAfter,
             path[2].GetTypeOfEdgeToNextVertex().value());
   EXPECT_EQ("e", path[3].GetName());
   EXPECT_FALSE(path[3].GetTypeOfEdgeToNextVertex().has_value());
 }
+
+TEST(GetGroupsPath, shouldThrowIfMasterlistGroupLoadsAfterAUserlistGroup) {
+  std::unordered_set<Group> groups({Group("a", {}),
+                                    Group("b", {"a"}),
+                                    Group("c", {"a"}),
+                                    Group("e", {"b", "d"})});
+  std::unordered_set<Group> userGroups({Group("d", {"c"})});
+
+  EXPECT_THROW(GetGroupsPath(groups, userGroups, "a", "e"),
+               UndefinedGroupError);
+}
 }
 }