Merge pull request #657 from lsst/tickets/DM-33904

DM-33904: Fix bug in Registry.getCollectionParentChains

python/lsst/daf/butler/registry/interfaces/_collections.py

@@ -206,7 +206,13 @@ def refresh(self, manager: CollectionManager) -> None:
             The object that manages this records instance and all records
             instances that may appear as its children.
         """
+        # Clear out the old reverse mapping (from child to parents).
+        for child in self._children:
+            manager._parents_by_child[manager.find(child).key].discard(self.key)
         self._children = self._load(manager)
+        # Update the reverse mapping (from child to parents) in the manager.
+        for child in self._children:
+            manager._parents_by_child[manager.find(child).key].add(self.key)
 
     @abstractmethod
     def _update(self, manager: CollectionManager, children: CollectionSearch) -> None:

python/lsst/daf/butler/registry/tests/_registry.py

@@ -114,8 +114,22 @@ def makeRegistryConfig(self) -> RegistryConfig:
         return config
 
     @abstractmethod
-    def makeRegistry(self) -> Registry:
-        """Return the Registry instance to be tested."""
+    def makeRegistry(self, share_repo_with: Optional[Registry] = None) -> Optional[Registry]:
+        """Return the Registry instance to be tested.
+
+        Parameters
+        ----------
+        share_repo_with : `Registry`, optional
+            If provided, the new registry should point to the same data
+            repository as this existing registry.
+
+        Returns
+        -------
+        registry : `Registry`
+            New `Registry` instance, or `None` *only* if `share_repo_with` is
+            not `None` and this test case does not support that argument
+            (e.g. it is impossible with in-memory SQLite DBs).
+        """
         raise NotImplementedError()
 
     def loadData(self, registry: Registry, filename: str):
@@ -664,6 +678,7 @@ def testComponentLookups(self):
     def testCollections(self):
         """Tests for registry methods that manage collections."""
         registry = self.makeRegistry()
+        other_registry = self.makeRegistry(share_repo_with=registry)
         self.loadData(registry, "base.yaml")
         self.loadData(registry, "datasets.yaml")
         run1 = "imported_g"
@@ -746,6 +761,15 @@ def testCollections(self):
         self.assertEqual(list(registry.getCollectionChain(chain1)), [tag1, run2])
         self.assertEqual(registry.getCollectionParentChains(tag1), {chain1})
         self.assertEqual(registry.getCollectionParentChains(run2), {chain1})
+        # Refresh the other registry that points to the same repo, and make
+        # sure it can see the things we've done (note that this does require
+        # an explicit refresh(); that's the documented behavior, because
+        # caching is ~impossible otherwise).
+        if other_registry is not None:
+            other_registry.refresh()
+            self.assertEqual(list(other_registry.getCollectionChain(chain1)), [tag1, run2])
+            self.assertEqual(other_registry.getCollectionParentChains(tag1), {chain1})
+            self.assertEqual(other_registry.getCollectionParentChains(run2), {chain1})
         # Searching for dataId1 or dataId2 in the chain should return ref1 and
         # ref2, because both are in tag1.
         self.assertEqual(registry.findDataset(datasetType, dataId1, collections=chain1), ref1)

tests/test_postgresql.py

@@ -26,6 +26,7 @@
 import unittest
 import warnings
 from contextlib import contextmanager
+from typing import Optional
 
 import astropy.time
 
@@ -233,12 +234,18 @@ def tearDownClass(cls):
     def getDataDir(cls) -> str:
         return os.path.normpath(os.path.join(os.path.dirname(__file__), "data", "registry"))
 
-    def makeRegistry(self) -> Registry:
-        namespace = f"namespace_{secrets.token_hex(8).lower()}"
+    def makeRegistry(self, share_repo_with: Optional[Registry] = None) -> Registry:
+        if share_repo_with is None:
+            namespace = f"namespace_{secrets.token_hex(8).lower()}"
+        else:
+            namespace = share_repo_with._db.namespace
         config = self.makeRegistryConfig()
         config["db"] = self.server.url()
         config["namespace"] = namespace
-        return Registry.createFromConfig(config)
+        if share_repo_with is None:
+            return Registry.createFromConfig(config)
+        else:
+            return Registry.fromConfig(config)
 
 
 class PostgresqlRegistryNameKeyCollMgrTestCase(PostgresqlRegistryTests, unittest.TestCase):

tests/test_sqlite.py

@@ -25,6 +25,7 @@
 import tempfile
 import unittest
 from contextlib import contextmanager
+from typing import Optional
 
 import sqlalchemy
 from lsst.daf.butler import ddl
@@ -195,11 +196,17 @@ def tearDown(self):
     def getDataDir(cls) -> str:
         return os.path.normpath(os.path.join(os.path.dirname(__file__), "data", "registry"))
 
-    def makeRegistry(self) -> Registry:
-        _, filename = tempfile.mkstemp(dir=self.root, suffix=".sqlite3")
+    def makeRegistry(self, share_repo_with: Optional[Registry] = None) -> Registry:
+        if share_repo_with is None:
+            _, filename = tempfile.mkstemp(dir=self.root, suffix=".sqlite3")
+        else:
+            filename = share_repo_with._db.filename
         config = self.makeRegistryConfig()
         config["db"] = f"sqlite:///{filename}"
-        return Registry.createFromConfig(config, butlerRoot=self.root)
+        if share_repo_with is None:
+            return Registry.createFromConfig(config, butlerRoot=self.root)
+        else:
+            return Registry.fromConfig(config, butlerRoot=self.root)
 
 
 class SqliteFileRegistryNameKeyCollMgrTestCase(SqliteFileRegistryTests, unittest.TestCase):
@@ -257,7 +264,9 @@ class SqliteMemoryRegistryTests(RegistryTests):
     def getDataDir(cls) -> str:
         return os.path.normpath(os.path.join(os.path.dirname(__file__), "data", "registry"))
 
-    def makeRegistry(self) -> Registry:
+    def makeRegistry(self, share_repo_with: Optional[Registry] = None) -> Optional[Registry]:
+        if share_repo_with is not None:
+            return None
         config = self.makeRegistryConfig()
         config["db"] = "sqlite://"
         return Registry.createFromConfig(config)