add extra checks for cycles + tests

Core/src/Geometry/BlueprintNode.cpp

@@ -19,12 +19,41 @@
 
 namespace Acts {
 
+namespace {
+bool hasDescendent(const BlueprintNode& descendent,
+                   const BlueprintNode& ancestor) {
+  if (&descendent == &ancestor) {
+    return true;
+  }
+
+  for (const auto& child : ancestor.children()) {
+    if (hasDescendent(descendent, child)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+}  // namespace
+
 void BlueprintNode::toStream(std::ostream& os) const {
   os << "BlueprintNode(" << name() << ")";
 }
 
 BlueprintNode& BlueprintNode::addChild(std::shared_ptr<BlueprintNode> child) {
-  child->m_depth = m_depth + 1;
+  if (!child) {
+    throw std::invalid_argument("Child is nullptr");
+  }
+
+  if (child->depth() != 0) {
+    throw std::invalid_argument("Child has already been added to another node");
+  }
+
+  if (hasDescendent(*this, *child)) {
+    throw std::invalid_argument("Adding child would create a cycle");
+  }
+
+  child->incrementDepth();
   m_children.push_back(std::move(child));
   return *this;
 }
@@ -41,6 +70,13 @@ std::size_t BlueprintNode::depth() const {
   return m_depth;
 }
 
+void BlueprintNode::incrementDepth() {
+  m_depth++;
+  for (auto& child : m_children) {
+    child->incrementDepth();
+  }
+}
+
 std::string BlueprintNode::indent() const {
   return std::string(m_depth * 2, ' ');
 }

Tests/UnitTests/Core/Geometry/BlueprintTests.cpp

@@ -13,6 +13,7 @@
 
 #include "Acts/Definitions/Units.hpp"
 #include "Acts/Geometry/Blueprint.hpp"
+#include "Acts/Geometry/BlueprintNode.hpp"
 #include "Acts/Geometry/CylinderContainerBlueprintNode.hpp"
 #include "Acts/Geometry/CylinderVolumeBounds.hpp"
 #include "Acts/Geometry/CylinderVolumeStack.hpp"
@@ -90,6 +91,73 @@ BOOST_AUTO_TEST_CASE(InvalidRoot) {
   BOOST_CHECK_THROW(root.construct({}, gctx, *logger), std::logic_error);
 }
 
+class DummyNode : public BlueprintNode {
+ public:
+  DummyNode(const std::string& name) : m_name(name) {}
+
+  const std::string& name() const override { return m_name; }
+
+  Volume& build(const BlueprintOptions& /*options*/,
+                const GeometryContext& /*gctx*/,
+                const Acts::Logger& /*logger*/) override {
+    throw std::logic_error("Not implemented");
+  }
+
+  PortalShellBase& connect(const BlueprintOptions& /*options*/,
+                           const GeometryContext& /*gctx*/,
+                           const Logger& /*logger */) override {
+    throw std::logic_error("Not implemented");
+  }
+
+  void finalize(const BlueprintOptions& /*options*/,
+                const GeometryContext& /*gctx*/, TrackingVolume& /*parent*/,
+                const Logger& /*logger*/) override {
+    throw std::logic_error("Not implemented");
+  }
+
+ private:
+  std::string m_name;
+};
+
+BOOST_AUTO_TEST_CASE(AddChildInvalid) {
+  auto node = std::make_shared<DummyNode>("node");
+
+  // Add self
+  BOOST_CHECK_THROW(node->addChild(node), std::invalid_argument);
+
+  // Add nullptr
+  BOOST_CHECK_THROW(node->addChild(nullptr), std::invalid_argument);
+
+  auto nodeB = std::make_shared<DummyNode>("nodeB");
+  auto nodeC = std::make_shared<DummyNode>("nodeC");
+
+  node->addChild(nodeB);
+  nodeB->addChild(nodeC);
+  BOOST_CHECK_THROW(nodeC->addChild(node), std::invalid_argument);
+
+  // already has parent, can't be added as a child anywhere else
+  BOOST_CHECK_THROW(node->addChild(nodeC), std::invalid_argument);
+}
+
+BOOST_AUTO_TEST_CASE(Depth) {
+  auto node1 = std::make_shared<DummyNode>("node1");
+  auto node2 = std::make_shared<DummyNode>("node2");
+  auto node3 = std::make_shared<DummyNode>("node3");
+
+  BOOST_CHECK_EQUAL(node1->depth(), 0);
+  BOOST_CHECK_EQUAL(node2->depth(), 0);
+  BOOST_CHECK_EQUAL(node3->depth(), 0);
+
+  node2->addChild(node3);
+  BOOST_CHECK_EQUAL(node2->depth(), 0);
+  BOOST_CHECK_EQUAL(node3->depth(), 1);
+
+  node1->addChild(node2);
+  BOOST_CHECK_EQUAL(node1->depth(), 0);
+  BOOST_CHECK_EQUAL(node2->depth(), 1);
+  BOOST_CHECK_EQUAL(node3->depth(), 2);
+}
+
 BOOST_AUTO_TEST_CASE(Static) {
   Blueprint::Config cfg;
   cfg.envelope[BinningValue::binZ] = {20_mm, 20_mm};