From 0dd9dd00cc649a626a1ff8cc2d44056c12798b1a Mon Sep 17 00:00:00 2001
From: rly <rly@lbl.gov>
Date: Mon, 24 Jul 2023 23:47:25 -0400
Subject: [PATCH] Refactor DTR warning

---
 src/hdmf/common/table.py        | 11 +++++++++++
 src/hdmf/container.py           | 25 +++++++++++++++++++------
 tests/unit/common/test_table.py |  2 +-
 tests/unit/test_container.py    | 12 ++++++++++++
 4 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/src/hdmf/common/table.py b/src/hdmf/common/table.py
index 1b4fe76d1..e624713e8 100644
--- a/src/hdmf/common/table.py
+++ b/src/hdmf/common/table.py
@@ -1421,6 +1421,17 @@ def __repr__(self):
                                                               id(self.table))
         return template
 
+    def _validate_on_set_parent(self):
+        # when this DynamicTableRegion is added to a parent, check whether one of the ancestors is
+        # also an ancestor of the linked-to table.
+        table_ancestor_ids = [id(x) for x in self.table.get_ancestors()]
+        self_ancestor_ids = [id(x) for x in self.get_ancestors()]
+        if set(table_ancestor_ids).isdisjoint(self_ancestor_ids):
+            msg = (f"The table for DynamicTableRegion '{self.name}' does not share an ancestor with the "
+                   "DynamicTableRegion.")
+            warn(msg)
+        return super()._validate_on_set_parent()
+
 
 def _uint_precision(elements):
     """ Calculate the uint precision needed to encode a set of elements """
diff --git a/src/hdmf/container.py b/src/hdmf/container.py
index dc93ff95d..86d725685 100644
--- a/src/hdmf/container.py
+++ b/src/hdmf/container.py
@@ -299,6 +299,15 @@ def get_ancestor(self, **kwargs):
             p = p.parent
         return None
 
+    @docval()
+    def get_ancestors(self, **kwargs):
+        p = self.parent
+        ret = []
+        while p is not None:
+            ret.append(p)
+            p = p.parent
+        return tuple(ret)
+
     @property
     def fields(self):
         '''
@@ -411,12 +420,8 @@ def parent(self, parent_container):
                 parent_container.__children.append(self)
                 parent_container.set_modified()
             for child in self.children:
-                if type(child).__name__ == "DynamicTableRegion":
-                    if child.table.parent is None:
-                        msg = "The table for this DynamicTableRegion has not been added to the parent."
-                        warn(msg)
-                    else:
-                        continue
+                # used by hdmf.common.table.DynamicTableRegion to check for orphaned tables
+                child._validate_on_set_parent()
 
     def _remove_child(self, child):
         """Remove a child Container. Intended for use in subclasses that allow dynamic addition of child Containers."""
@@ -442,6 +447,14 @@ def reset_parent(self):
         else:
             raise ValueError("Cannot reset parent when parent is not an AbstractContainer: %s" % repr(self.parent))
 
+    def _validate_on_set_parent(self):
+        """Validate this Container after setting the parent.
+
+        This method is called by the parent setter. It can be overridden in subclasses to perform additional
+        validation. The default implementation does nothing.
+        """
+        pass
+
 
 class Container(AbstractContainer):
     """A container that can contain other containers and has special functionality for printing."""
diff --git a/tests/unit/common/test_table.py b/tests/unit/common/test_table.py
index af6b6357e..311e01f8b 100644
--- a/tests/unit/common/test_table.py
+++ b/tests/unit/common/test_table.py
@@ -1124,7 +1124,7 @@ def setUp(self):
         super().setUp()
 
     def setUpContainer(self):
-        multi_container = SimpleMultiContainer(name='multi', containers=[self.table, self.target_table])
+        multi_container = SimpleMultiContainer(name='multi', containers=[self.target_table, self.table])
         return multi_container
 
     def _get(self, arg):
diff --git a/tests/unit/test_container.py b/tests/unit/test_container.py
index 4027ecb4e..83fcb5196 100644
--- a/tests/unit/test_container.py
+++ b/tests/unit/test_container.py
@@ -369,6 +369,18 @@ def test_reset_parent_no_parent(self):
         obj.reset_parent()
         self.assertIsNone(obj.parent)
 
+    def test_get_ancestors(self):
+        """Test that get_ancestors returns the correct ancestors.
+        """
+        grandparent_obj = Container('obj1')
+        parent_obj = Container('obj2')
+        child_obj = Container('obj3')
+        parent_obj.parent = grandparent_obj
+        child_obj.parent = parent_obj
+        self.assertTupleEqual(grandparent_obj.get_ancestors(), tuple())
+        self.assertTupleEqual(parent_obj.get_ancestors(), (grandparent_obj, ))
+        self.assertTupleEqual(child_obj.get_ancestors(), (parent_obj, grandparent_obj))
+
 
 class TestHTMLRepr(TestCase):