fix: include active branch in find_existing_object cache keys (#163)

mfiedorowicz · web-flow · commit 8a42751f2dd8 · 2026-04-23T16:02:35.000+02:00
The Redis-backed find_existing_object cache keyed entries by
(object_type, scalar_field_fingerprint) only. The active NetBox
Branching schema was not part of the key, allowing cross-branch
cache pollution when concurrent ingests target different branches.

Read the active_branch contextvar from netbox_branching to namespace
both the lookup cache key and the reverse-index key per branch.
Non-branching deployments are unaffected (key format unchanged).
diff --git a/netbox_diode_plugin/api/matcher.py b/netbox_diode_plugin/api/matcher.py
@@ -32,14 +32,32 @@ def _get_find_obj_cache_ttl() -> int:
     return get_plugin_config("netbox_diode_plugin", "find_obj_cache_ttl")
 
 
+def _get_active_branch_schema() -> str | None:
+    try:
+        from netbox_branching.contextvars import active_branch
+    except ImportError:
+        return None
+    branch = active_branch.get()
+    if branch is not None:
+        return branch.schema_id
+    return None
+
+
+def _find_obj_rev_key(object_type: str, object_id: int) -> str:
+    branch_schema = _get_active_branch_schema()
+    if branch_schema:
+        return f"diode:fobj:rev:{branch_schema}:{object_type}:{object_id}"
+    return f"diode:fobj:rev:{object_type}:{object_id}"
+
+
 def invalidate_find_obj_entry(object_type: str, object_id: int):
     """
     Delete a cached find_existing_object result by PK.
 
     Uses a reverse-index (PK → cache key) to find and delete the
     lookup cache entry. Call this after updating an existing object.
     """
-    rev_key = f"diode:fobj:rev:{object_type}:{object_id}"
+    rev_key = _find_obj_rev_key(object_type, object_id)
     lookup_key = django_cache.get(rev_key)
     if lookup_key:
         django_cache.delete(lookup_key)
@@ -921,7 +939,11 @@ def _find_obj_cache_key(data: dict, object_type: str) -> str | None:
     if not items:
         return None
 
-    raw = f"{object_type}:{items}"
+    branch_schema = _get_active_branch_schema()
+    if branch_schema:
+        raw = f"{branch_schema}:{object_type}:{items}"
+    else:
+        raw = f"{object_type}:{items}"
     key_hash = hashlib.sha256(raw.encode()).hexdigest()[:20]
     return f"diode:fobj:{key_hash}"
 
@@ -955,8 +977,7 @@ def find_existing_object(data: dict, object_type: str): # noqa: C901
                 # Object deleted since cached — clean up and fall through
                 cache_hit = False
                 django_cache.delete(cache_key)
-                rev_key = f"diode:fobj:rev:{object_type}:{cached_id}"
-                django_cache.delete(rev_key)
+                django_cache.delete(_find_obj_rev_key(object_type, cached_id))
 
     if not cache_hit:
         for matcher in get_model_matchers(model_class):
@@ -973,8 +994,7 @@ def find_existing_object(data: dict, object_type: str): # noqa: C901
 
         if cache_key and result is not None:
             django_cache.set(cache_key, result.id, cache_ttl)
-            rev_key = f"diode:fobj:rev:{object_type}:{result.id}"
-            django_cache.set(rev_key, cache_key, cache_ttl)
+            django_cache.set(_find_obj_rev_key(object_type, result.id), cache_key, cache_ttl)
 
     if ctx:
         ctx.record_timing("find_obj", (time.monotonic() - start) * 1000)
diff --git a/netbox_diode_plugin/tests/v4.4.x/tests/test_matcher_cache.py b/netbox_diode_plugin/tests/v4.4.x/tests/test_matcher_cache.py
@@ -11,6 +11,7 @@
 from netbox_diode_plugin.api.common import UnresolvedReference
 from netbox_diode_plugin.api.matcher import (
     _find_obj_cache_key,
+    _find_obj_rev_key,
     find_existing_object,
     invalidate_find_obj_entry,
 )
@@ -292,3 +293,154 @@ def test_invalidate_does_not_affect_other_entries(self, _mock_ttl):
         # Main should be gone
         cache_key_main = _find_obj_cache_key(data_main, "dcim.manufacturer")
         self.assertIsNone(django_cache.get(cache_key_main))
+
+
+BRANCH_SCHEMA_MOCK = "netbox_diode_plugin.api.matcher._get_active_branch_schema"
+
+
+class BranchAwareCacheKeyTestCase(TestCase):
+    """Tests that cache keys are isolated per branch."""
+
+    def setUp(self):
+        """Clear cache before each test."""
+        django_cache.clear()
+
+    def tearDown(self):
+        """Clear cache after each test."""
+        django_cache.clear()
+
+    def test_different_branch_different_cache_key(self):
+        """Same data under different branches produces different cache keys."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            key_b = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertNotEqual(key_a, key_b)
+
+    def test_same_branch_same_cache_key(self):
+        """Same data under the same branch produces identical cache keys."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            key1 = _find_obj_cache_key(data, "dcim.manufacturer")
+            key2 = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertEqual(key1, key2)
+
+    def test_no_branch_unchanged_from_legacy_format(self):
+        """Without active branch, cache key matches the original format."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            key = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertTrue(key.startswith("diode:fobj:"))
+
+    def test_branch_vs_no_branch_different_keys(self):
+        """A branched key differs from a non-branched key for the same data."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            key_main = _find_obj_cache_key(data, "dcim.manufacturer")
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            key_branch = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertNotEqual(key_main, key_branch)
+
+    def test_different_branch_different_rev_key(self):
+        """Reverse-index keys are isolated per branch."""
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            rev_a = _find_obj_rev_key("dcim.manufacturer", 100)
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            rev_b = _find_obj_rev_key("dcim.manufacturer", 100)
+        self.assertNotEqual(rev_a, rev_b)
+        self.assertIn("branch_a", rev_a)
+        self.assertIn("branch_b", rev_b)
+
+    def test_no_branch_rev_key_unchanged(self):
+        """Without active branch, rev key matches the original format."""
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            rev = _find_obj_rev_key("dcim.manufacturer", 100)
+        self.assertEqual(rev, "diode:fobj:rev:dcim.manufacturer:100")
+
+
+class BranchAwareFindExistingObjectTestCase(TestCase):
+    """Tests that find_existing_object cache does not cross branches."""
+
+    def setUp(self):
+        """Set up test fixtures."""
+        self.manufacturer = Manufacturer.objects.create(
+            name="BranchTestMfr",
+            slug="branch-test-mfr",
+        )
+        django_cache.clear()
+
+    def tearDown(self):
+        """Clean up cache after each test."""
+        django_cache.clear()
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_cache_does_not_cross_branches(self, _mock_ttl):
+        """Cache populated under branch A is a miss under branch B."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            result_a = find_existing_object(data, "dcim.manufacturer")
+            self.assertEqual(result_a.id, self.manufacturer.id)
+            cache_key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            cache_key_b = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNone(django_cache.get(cache_key_b))
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_invalidate_does_not_cross_branches(self, _mock_ttl):
+        """Invalidation under branch B does not evict branch A's cache."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            find_existing_object(data, "dcim.manufacturer")
+            cache_key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            invalidate_find_obj_entry("dcim.manufacturer", self.manufacturer.id)
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_invalidate_within_same_branch(self, _mock_ttl):
+        """Invalidation under the same branch correctly evicts cache."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            find_existing_object(data, "dcim.manufacturer")
+            cache_key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+            invalidate_find_obj_entry("dcim.manufacturer", self.manufacturer.id)
+            self.assertIsNone(django_cache.get(cache_key_a))
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_branch_and_main_caches_independent(self, _mock_ttl):
+        """Cache entries for main (no branch) and a branch are independent."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            find_existing_object(data, "dcim.manufacturer")
+            cache_key_main = _find_obj_cache_key(data, "dcim.manufacturer")
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            cache_key_branch = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNone(django_cache.get(cache_key_branch))
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            self.assertIsNotNone(django_cache.get(cache_key_main))
diff --git a/netbox_diode_plugin/tests/v4.5.x/tests/test_matcher_cache.py b/netbox_diode_plugin/tests/v4.5.x/tests/test_matcher_cache.py
@@ -11,6 +11,7 @@
 from netbox_diode_plugin.api.common import UnresolvedReference
 from netbox_diode_plugin.api.matcher import (
     _find_obj_cache_key,
+    _find_obj_rev_key,
     find_existing_object,
     invalidate_find_obj_entry,
 )
@@ -292,3 +293,154 @@ def test_invalidate_does_not_affect_other_entries(self, _mock_ttl):
         # Main should be gone
         cache_key_main = _find_obj_cache_key(data_main, "dcim.manufacturer")
         self.assertIsNone(django_cache.get(cache_key_main))
+
+
+BRANCH_SCHEMA_MOCK = "netbox_diode_plugin.api.matcher._get_active_branch_schema"
+
+
+class BranchAwareCacheKeyTestCase(TestCase):
+    """Tests that cache keys are isolated per branch."""
+
+    def setUp(self):
+        """Clear cache before each test."""
+        django_cache.clear()
+
+    def tearDown(self):
+        """Clear cache after each test."""
+        django_cache.clear()
+
+    def test_different_branch_different_cache_key(self):
+        """Same data under different branches produces different cache keys."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            key_b = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertNotEqual(key_a, key_b)
+
+    def test_same_branch_same_cache_key(self):
+        """Same data under the same branch produces identical cache keys."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            key1 = _find_obj_cache_key(data, "dcim.manufacturer")
+            key2 = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertEqual(key1, key2)
+
+    def test_no_branch_unchanged_from_legacy_format(self):
+        """Without active branch, cache key matches the original format."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            key = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertTrue(key.startswith("diode:fobj:"))
+
+    def test_branch_vs_no_branch_different_keys(self):
+        """A branched key differs from a non-branched key for the same data."""
+        data = {"name": "Cisco"}
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            key_main = _find_obj_cache_key(data, "dcim.manufacturer")
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            key_branch = _find_obj_cache_key(data, "dcim.manufacturer")
+        self.assertNotEqual(key_main, key_branch)
+
+    def test_different_branch_different_rev_key(self):
+        """Reverse-index keys are isolated per branch."""
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            rev_a = _find_obj_rev_key("dcim.manufacturer", 100)
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            rev_b = _find_obj_rev_key("dcim.manufacturer", 100)
+        self.assertNotEqual(rev_a, rev_b)
+        self.assertIn("branch_a", rev_a)
+        self.assertIn("branch_b", rev_b)
+
+    def test_no_branch_rev_key_unchanged(self):
+        """Without active branch, rev key matches the original format."""
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            rev = _find_obj_rev_key("dcim.manufacturer", 100)
+        self.assertEqual(rev, "diode:fobj:rev:dcim.manufacturer:100")
+
+
+class BranchAwareFindExistingObjectTestCase(TestCase):
+    """Tests that find_existing_object cache does not cross branches."""
+
+    def setUp(self):
+        """Set up test fixtures."""
+        self.manufacturer = Manufacturer.objects.create(
+            name="BranchTestMfr",
+            slug="branch-test-mfr",
+        )
+        django_cache.clear()
+
+    def tearDown(self):
+        """Clean up cache after each test."""
+        django_cache.clear()
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_cache_does_not_cross_branches(self, _mock_ttl):
+        """Cache populated under branch A is a miss under branch B."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            result_a = find_existing_object(data, "dcim.manufacturer")
+            self.assertEqual(result_a.id, self.manufacturer.id)
+            cache_key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            cache_key_b = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNone(django_cache.get(cache_key_b))
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_invalidate_does_not_cross_branches(self, _mock_ttl):
+        """Invalidation under branch B does not evict branch A's cache."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            find_existing_object(data, "dcim.manufacturer")
+            cache_key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_b"):
+            invalidate_find_obj_entry("dcim.manufacturer", self.manufacturer.id)
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_invalidate_within_same_branch(self, _mock_ttl):
+        """Invalidation under the same branch correctly evicts cache."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            find_existing_object(data, "dcim.manufacturer")
+            cache_key_a = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNotNone(django_cache.get(cache_key_a))
+            invalidate_find_obj_entry("dcim.manufacturer", self.manufacturer.id)
+            self.assertIsNone(django_cache.get(cache_key_a))
+
+    @mock.patch(
+        "netbox_diode_plugin.api.matcher._get_find_obj_cache_ttl",
+        return_value=5,
+    )
+    def test_branch_and_main_caches_independent(self, _mock_ttl):
+        """Cache entries for main (no branch) and a branch are independent."""
+        data = {"name": "BranchTestMfr", "_object_type": "dcim.manufacturer"}
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            find_existing_object(data, "dcim.manufacturer")
+            cache_key_main = _find_obj_cache_key(data, "dcim.manufacturer")
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value="branch_a"):
+            cache_key_branch = _find_obj_cache_key(data, "dcim.manufacturer")
+            self.assertIsNone(django_cache.get(cache_key_branch))
+
+        with mock.patch(BRANCH_SCHEMA_MOCK, return_value=None):
+            self.assertIsNotNone(django_cache.get(cache_key_main))
