From 10410e95e2d18ba1f7e3c0377082c7d4ad96e254 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Mon, 31 Aug 2026 15:15:25 -0400 Subject: [PATCH 1/5] Fixes #685: guard get_models() against re-entrancy Generating a brand-new CustomObjectType model can itself trigger Django to rebuild its global relation graph (Options._relation_tree), e.g. via ObjectType.objects.get_for_model()'s .create() in the executor path (#685), or a polymorphic field's related_object_types.all() query in the descriptor-wiring path (#686, closed as a duplicate of this one). Rebuilding that graph calls apps.get_models() again, re-entering this plugin's own get_models() while it's still mid-generation -- which walked CustomObjectType.objects.all() and called get_model() again for every COT, including the one still under construction, with no way to ever finish. Per jnovinger's review on #685, a re-entrancy guard on get_models() itself (rather than an executor-only fix) is the right level: #686 hits the identical get_models() recursion through a completely different call site untouched by the executor, so any fix scoped to the executor would leave that path -- and any future path into the same hazard -- live. A ContextVar-based guard (matching the existing _is_migrating idiom in this file) is safe here because generate_model()'s type() call already registers a COT's model with Django's app registry synchronously, before get_model() ever calls _after_model_generation() (the method that can trigger this re-entrancy) -- so a re-entrant call can simply fall back to super().get_models() (already-registered models) without needing to regenerate anything. Adds regression tests for both re-entry paths. Both needed one non-obvious adjustment to actually exercise the vulnerable code: get_models()'s CustomObjectType-enumeration loop is unconditionally disabled under `manage.py test` (should_skip_dynamic_model_creation() returns True whenever "test" in sys.argv), so tests patch _app_ready / should_skip_dynamic_model_creation to replicate a non-test process. Verified each test reproduces a genuine RecursionError against this commit's parent and passes with the fix. Verified: full netbox_custom_objects suite in the shared dev venv, 1179 tests, 0 new failures (15 pre-existing errors, all attributable to the sibling netbox-branching checkout's known incompatibility with this venv's Django/NetBox version -- confirmed to fail identically in isolation, unrelated to this change). Co-Authored-By: Claude Sonnet 5 --- netbox_custom_objects/__init__.py | 123 +++++---- .../tests/schema/test_schema_api.py | 234 +++++++++++++++++- .../tests/test_polymorphic_fields.py | 73 ++++++ 3 files changed, 384 insertions(+), 46 deletions(-) diff --git a/netbox_custom_objects/__init__.py b/netbox_custom_objects/__init__.py index 6ff1f3b7..0ffbecfd 100644 --- a/netbox_custom_objects/__init__.py +++ b/netbox_custom_objects/__init__.py @@ -18,6 +18,15 @@ # Context variable to track if we're currently running migrations _is_migrating = contextvars.ContextVar('is_migrating', default=False) +# Guards get_models() against re-entrancy (issues #685/#686): generating a COT +# model can itself trigger Django to rebuild its global relation graph (e.g. via +# ObjectType.objects.get_for_model()'s .create(), or a polymorphic field's +# related_object_types.all() query), which calls apps.get_models() again. Without +# this guard, that re-entrant call would call get_model() again for every COT -- +# including the one still mid-construction -- recursing without ever reaching the +# point where the first call finishes and registers/caches it. +_generating_models = contextvars.ContextVar('generating_models', default=False) + # Cache for migration check to avoid repeated expensive filesystem/database operations _migrations_checked = None _checking_migrations = False @@ -560,52 +569,76 @@ def get_models(self, include_auto_created=False, include_swapped=False): for model in super().get_models(include_auto_created, include_swapped): yield model - # Suppress warnings about database calls during model loading. - # See the corresponding block in ready() for the rationale for using - # module-based filtering instead of message-content matching. - with warnings.catch_warnings(): - warnings.filterwarnings( - "ignore", category=RuntimeWarning, - module=r"django\.db\.backends\..*", - ) - warnings.filterwarnings( - "ignore", category=UserWarning, - module=r"netbox_branching\..*", - ) - - # Skip dynamic model generation until ready() has completed. - # Other apps' ready() calls (e.g. dcim) trigger _relation_tree → - # apps.get_models() before our ready() runs. At that point _model_cache - # is empty, so get_model() would regenerate every COT from scratch — - # including ContentType DB lookups that may fail. After our ready() - # finishes, _app_ready is True and get_model() returns cached models - # without any ContentType lookups. - if not _app_ready: - return - - # Skip custom object type model loading if dynamic models can't be created yet - if self.should_skip_dynamic_model_creation(): - return - - # Add custom object type models - from .models import CustomObjectType - - try: - with transaction.atomic(): - custom_object_types = CustomObjectType.objects.all() - for custom_type in custom_object_types: - model = custom_type.get_model() - if model: - yield model + # Re-entrant call (issues #685/#686): something invoked while WE are + # already generating COT models needed Django's model registry -- most + # commonly Options._relation_tree, via ObjectType.objects.get_for_model() + # or a polymorphic field's related_object_types.all() query. Recursing + # into get_model() again here would keep regenerating the same + # still-under-construction model forever, since the outer call hasn't + # reached the point where it finishes and registers/caches it. + # + # super().get_models() above already covers everything this caller can + # safely see right now: generate_model()'s type() call registers a COT's + # model with Django's app registry synchronously (inside ModelBase.__new__), + # before get_model() ever calls _after_model_generation() -- the method + # that can trigger this re-entrancy. Any COT the outer loop below hasn't + # reached yet is simply absent from this transient, incomplete snapshot; + # get_model()'s own apps.clear_cache() call (once each COT finishes) + # invalidates any relation-tree computed against that incomplete view, so + # it self-heals as soon as the remaining COTs are generated. + if _generating_models.get(): + return - # If include_auto_created is True, also yield through models - if include_auto_created and hasattr(model, '_through_models'): - for through_model in model._through_models: - yield through_model - except (ProgrammingError, OperationalError): - # DB schema is incomplete (unapplied migrations). Yield nothing — - # dynamic models will be available once migrations have run. - return + token = _generating_models.set(True) + try: + # Suppress warnings about database calls during model loading. + # See the corresponding block in ready() for the rationale for using + # module-based filtering instead of message-content matching. + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", category=RuntimeWarning, + module=r"django\.db\.backends\..*", + ) + warnings.filterwarnings( + "ignore", category=UserWarning, + module=r"netbox_branching\..*", + ) + + # Skip dynamic model generation until ready() has completed. + # Other apps' ready() calls (e.g. dcim) trigger _relation_tree → + # apps.get_models() before our ready() runs. At that point _model_cache + # is empty, so get_model() would regenerate every COT from scratch — + # including ContentType DB lookups that may fail. After our ready() + # finishes, _app_ready is True and get_model() returns cached models + # without any ContentType lookups. + if not _app_ready: + return + + # Skip custom object type model loading if dynamic models can't be created yet + if self.should_skip_dynamic_model_creation(): + return + + # Add custom object type models + from .models import CustomObjectType + + try: + with transaction.atomic(): + custom_object_types = CustomObjectType.objects.all() + for custom_type in custom_object_types: + model = custom_type.get_model() + if model: + yield model + + # If include_auto_created is True, also yield through models + if include_auto_created and hasattr(model, '_through_models'): + for through_model in model._through_models: + yield through_model + except (ProgrammingError, OperationalError): + # DB schema is incomplete (unapplied migrations). Yield nothing — + # dynamic models will be available once migrations have run. + return + finally: + _generating_models.reset(token) config = CustomObjectsPluginConfig diff --git a/netbox_custom_objects/tests/schema/test_schema_api.py b/netbox_custom_objects/tests/schema/test_schema_api.py index 78b52afa..0904d63d 100644 --- a/netbox_custom_objects/tests/schema/test_schema_api.py +++ b/netbox_custom_objects/tests/schema/test_schema_api.py @@ -387,4 +387,236 @@ def test_apply_allow_destructive_string_returns_400(self): format="json", ) self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn("allow_destructive", resp.data) + + +# --------------------------------------------------------------------------- +# get_models() re-entrancy (issue #685) +# --------------------------------------------------------------------------- + +class SchemaApplyMultiCOTRecursionTestCase(_SchemaAPIBase): + """ + Regression test for issue #685: applying a document that creates multiple + new, cross-referencing Custom Object Types in one request raised + RecursionError. + + Generating a brand-new COT's model calls _after_model_generation(), which + can itself need Django's global relation graph (e.g. via + ObjectType.objects.get_for_model()'s .create()) -- rebuilding that graph + calls apps.get_models(), which re-enters this plugin's own get_models(), + which called get_model() again for every COT (including the one still + mid-construction) with no way to ever finish. apps.clear_cache() is called + explicitly here to force a cold relation-tree cache, matching the + "first time these classes are touched" condition the issue's own + investigation identified as the trigger (reproducible regardless of + whatever unrelated activity happened to already warm the cache in a given + process). + """ + + def setUp(self): + super().setUp() + perm = ObjectPermission(name='schema_apply_recursion_cot_perm', actions=['add', 'change']) + perm.save() + perm.users.add(self.user) + perm.object_types.add(ObjectType.objects.get_for_model(CustomObjectType)) + + def test_apply_two_new_cross_referencing_cots_in_one_request(self): + from unittest import mock + + from django.apps import apps as django_apps + + import netbox_custom_objects as nco_pkg + + django_apps.clear_cache() + + # get_models()'s CustomObjectType-enumeration loop -- the one this + # issue actually recurses through -- is unconditionally disabled under + # `manage.py test` (should_skip_dynamic_model_creation() returns True + # whenever "test" in sys.argv). Patch around that so this test + # exercises the real vulnerable loop instead of a no-op. + app_config = django_apps.get_app_config('netbox_custom_objects') + self.enterContext(mock.patch.object(nco_pkg, '_app_ready', True)) + self.enterContext( + mock.patch.object(app_config, 'should_skip_dynamic_model_creation', return_value=False) + ) + + schema_doc = { + "schema_version": "1", + "types": [ + { + "name": "ospf_instance", + "slug": "ospf-instances", + "fields": [ + {"id": 1, "name": "name", "type": "text", "primary": True, "required": True, "unique": True}, + ], + }, + { + "name": "ospf_area", + "slug": "ospf-areas", + "fields": [ + {"id": 1, "name": "name", "type": "text", "primary": True, "required": True, "unique": True}, + { + "id": 2, + "name": "instance", + "type": "object", + "required": True, + "related_object_type": "custom-objects/ospf-instances", + }, + ], + }, + ], + } + resp = self.client.post(self.apply_url, data=self._apply_body(schema_doc), format="json") + self.assertEqual(resp.status_code, status.HTTP_200_OK, resp.content) + self.assertTrue(resp.data["applied"]) + self.assertTrue(CustomObjectType.objects.filter(slug="ospf-instances").exists()) + self.assertTrue(CustomObjectType.objects.filter(slug="ospf-areas").exists()) + + def test_apply_three_new_cots_chained_references_in_one_request(self): + """The issue notes a 3-type chain (interface -> area -> instance) fails + identically to the 2-type case; cover it too.""" + from unittest import mock + + from django.apps import apps as django_apps + + import netbox_custom_objects as nco_pkg + + django_apps.clear_cache() + + # See the comment in test_apply_two_new_cross_referencing_cots_in_one_request: + # bypasses should_skip_dynamic_model_creation()'s "test" in sys.argv gate + # so this test exercises get_models()'s real CustomObjectType-enumeration + # loop instead of the no-op it reduces to under `manage.py test`. + app_config = django_apps.get_app_config('netbox_custom_objects') + self.enterContext(mock.patch.object(nco_pkg, '_app_ready', True)) + self.enterContext( + mock.patch.object(app_config, 'should_skip_dynamic_model_creation', return_value=False) + ) + + schema_doc = { + "schema_version": "1", + "types": [ + { + "name": "ospf_instance", + "slug": "ospf-instances", + "fields": [ + {"id": 1, "name": "name", "type": "text", "primary": True, "required": True, "unique": True}, + ], + }, + { + "name": "ospf_area", + "slug": "ospf-areas", + "fields": [ + {"id": 1, "name": "name", "type": "text", "primary": True, "required": True, "unique": True}, + { + "id": 2, + "name": "instance", + "type": "object", + "required": True, + "related_object_type": "custom-objects/ospf-instances", + }, + ], + }, + { + "name": "ospf_interface", + "slug": "ospf-interfaces", + "fields": [ + {"id": 1, "name": "name", "type": "text", "primary": True, "required": True, "unique": True}, + { + "id": 2, + "name": "area", + "type": "object", + "required": True, + "related_object_type": "custom-objects/ospf-areas", + }, + ], + }, + ], + } + resp = self.client.post(self.apply_url, data=self._apply_body(schema_doc), format="json") + self.assertEqual(resp.status_code, status.HTTP_200_OK, resp.content) + self.assertTrue(resp.data["applied"]) + self.assertTrue(CustomObjectType.objects.filter(slug="ospf-instances").exists()) + self.assertTrue(CustomObjectType.objects.filter(slug="ospf-areas").exists()) + self.assertTrue(CustomObjectType.objects.filter(slug="ospf-interfaces").exists()) + + def test_get_models_guards_against_reentrant_cot_generation(self): + """ + Deterministic, environment-independent regression test for the + get_models() re-entrancy itself. + + The two end-to-end tests above exercise the real API path from #685's + repro, but whether that path actually blows the Python recursion limit + depends on incidental factors (whatever else has already touched + Options._relation_tree in the process, WSGI/middleware stack depth, + interpreter version) that don't reliably reproduce in this harness. + More fundamentally, `get_models()`'s CustomObjectType-enumeration loop + (the one both issues actually recurse through) is unconditionally + disabled under `manage.py test` -- see + `CustomObjectsPluginConfig.should_skip_dynamic_model_creation()`, which + returns True whenever `"test" in sys.argv`. `_app_ready` patches below + replicate what `ready()` sets once it completes outside of tests, and + the `should_skip_dynamic_model_creation` patch replicates a production + (non-test) process, so this test exercises the actual vulnerable loop + instead of the no-op it reduces to under `manage.py test`. + + This test simulates the documented trigger directly: something deep + inside _after_model_generation() (ObjectType.objects.get_for_model() + .create(), or a polymorphic field's related_object_types.all() query -- + see the #686 test in test_polymorphic_fields.py) causes Django to + rebuild its relation graph, which calls apps.get_models() again while + the outer get_models() call is still mid-iteration. + + Without the guard, that re-entrant get_models() call walks + CustomObjectType.objects.all() and calls get_model() again for every + COT -- including ones already fully generated -- which is the root of + the unbounded growth. With the guard, the re-entrant call must return + immediately after yielding the plain Django models, without touching + CustomObjectType at all. + """ + from collections import defaultdict + from unittest import mock + + from django.apps import apps as django_apps + + import netbox_custom_objects as nco_pkg + + cot1 = self.create_custom_object_type(name='Reentrancy Source', slug='reentrancy-source') + cot2 = self.create_custom_object_type(name='Reentrancy Target', slug='reentrancy-target') + app_config = django_apps.get_app_config('netbox_custom_objects') + + call_counts = defaultdict(int) + real_get_model = CustomObjectType.get_model + test_case = self + + def spying_get_model(self, *args, **kwargs): + call_counts[self.pk] += 1 + if call_counts[self.pk] > 10: + test_case.fail( + f"get_model() called {call_counts[self.pk]} times for COT " + f"{self.pk} -- unbounded re-entrant generation (issue #685/#686)" + ) + result = real_get_model(self, *args, **kwargs) + if call_counts[self.pk] == 1: + # Simulate Django re-entering get_models() mid-generation, as it + # does when something inside _after_model_generation() needs a + # fresh model's relation graph. + list(app_config.get_models()) + return result + + with ( + mock.patch.object(CustomObjectType, 'get_model', spying_get_model), + mock.patch.object(nco_pkg, '_app_ready', True), + mock.patch.object(app_config, 'should_skip_dynamic_model_creation', return_value=False), + ): + # Note: deliberately not calling django_apps.clear_cache() here -- + # its own implementation walks apps.get_models(include_auto_created=True) + # to expire every model's cache, which would drive this same + # CustomObjectType loop to completion once already, before the + # call below even starts. + list(app_config.get_models()) + + # Each COT's model only needs to be generated once. The re-entrant + # get_models() call simulated above must not have triggered any + # additional generation for either COT. + self.assertEqual(call_counts[cot1.pk], 1) + self.assertEqual(call_counts[cot2.pk], 1) diff --git a/netbox_custom_objects/tests/test_polymorphic_fields.py b/netbox_custom_objects/tests/test_polymorphic_fields.py index e4138ce9..e4b3ab81 100644 --- a/netbox_custom_objects/tests/test_polymorphic_fields.py +++ b/netbox_custom_objects/tests/test_polymorphic_fields.py @@ -2108,3 +2108,76 @@ def test_unwire_does_not_remove_descriptor_owned_by_different_field(self): Site.__dict__.get("co_shared_ref"), descriptor_b, "_unwire must not remove a descriptor owned by a different CO field", ) + + +# --------------------------------------------------------------------------- +# get_models() re-entrancy (issue #686) +# --------------------------------------------------------------------------- + +class PolymorphicReverseDescriptorRecursionTestCase( + TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase +): + """ + Regression test for issue #686: generating a polymorphic field's reverse + descriptor raised RecursionError. + + _wire_polymorphic_reverse_descriptors() (called from + CustomObjectType._after_model_generation() for every polymorphic field with + a related_name) evaluates field_instance.related_object_types.all(), a + queryset that needs Django's relation graph to resolve -- which calls + apps.get_models(), re-entering this plugin's own get_models(), which called + get_model() again for the same still-under-construction COT with no way to + ever finish. apps.clear_cache() is called explicitly to force a cold + relation-tree cache, matching the "first time these classes are touched" + condition the sibling issue (#685) identified as the trigger. + """ + + def test_get_model_with_polymorphic_related_name_does_not_recurse(self): + from unittest import mock + + from django.apps import apps as django_apps + + import netbox_custom_objects as nco_pkg + + site_ot = ObjectType.objects.get(app_label="dcim", model="site") + prefix_ot = ObjectType.objects.get(app_label="ipam", model="prefix") + + cot = CustomObjectType.objects.create( + name="RecursionRevTest", slug="recursion-rev-test", + verbose_name_plural="Recursion Rev Tests", + ) + CustomObjectTypeField.objects.create( + custom_object_type=cot, name="name", type="text", primary=True, required=True, + ) + field = CustomObjectTypeField.objects.create( + custom_object_type=cot, + name="target_obj", label="Target", type="object", + is_polymorphic=True, + related_name="rev_recursion_test", + ) + field.related_object_types.set([site_ot, prefix_ot]) + + # Force the model out of cache and the relation-tree cold, so + # get_model() -> _after_model_generation() -> _wire_polymorphic_reverse_descriptors() + # -> related_object_types.all() hits the "first access" path that + # triggers apps.get_models() from inside Django's relation-graph build. + cot.clear_model_cache(cot.id) + django_apps.clear_cache() + + # get_models()'s CustomObjectType-enumeration loop -- the one this + # re-entrant trigger actually recurses through -- is unconditionally + # disabled under `manage.py test` (should_skip_dynamic_model_creation() + # returns True whenever "test" in sys.argv). Patch around that so this + # test exercises the real vulnerable loop instead of a no-op. + app_config = django_apps.get_app_config('netbox_custom_objects') + with ( + mock.patch.object(nco_pkg, '_app_ready', True), + mock.patch.object(app_config, 'should_skip_dynamic_model_creation', return_value=False), + ): + # Must not raise RecursionError. + model = cot.get_model() + self.assertTrue( + hasattr(Site, "rev_recursion_test"), + "Reverse descriptor must still be set on Site after get_model()", + ) + self.assertIsNotNone(model) From 16889eaec5bd22cb5ff373bac32092eb116398f6 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Mon, 31 Aug 2026 16:05:10 -0400 Subject: [PATCH 2/5] Restore dropped assertion in test_apply_allow_destructive_string_returns_400 An earlier edit in this branch accidentally removed self.assertIn("allow_destructive", resp.data) from this pre-existing, unrelated test while inserting the new reentrancy test classes after it. Caught by code review. Co-Authored-By: Claude Sonnet 5 --- netbox_custom_objects/tests/schema/test_schema_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/netbox_custom_objects/tests/schema/test_schema_api.py b/netbox_custom_objects/tests/schema/test_schema_api.py index 0904d63d..cd91f90e 100644 --- a/netbox_custom_objects/tests/schema/test_schema_api.py +++ b/netbox_custom_objects/tests/schema/test_schema_api.py @@ -387,6 +387,7 @@ def test_apply_allow_destructive_string_returns_400(self): format="json", ) self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn("allow_destructive", resp.data) # --------------------------------------------------------------------------- From f22a1bc52b6645d83aae413677d1160650c68d36 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Mon, 31 Aug 2026 16:10:57 -0400 Subject: [PATCH 3/5] Trim comments and docstrings Condensed the re-entrancy guard's explanation and the new tests' docstrings/comments down to the non-obvious why, dropping restated mechanics that duplicate what the code and PR description already say. Co-Authored-By: Claude Sonnet 5 --- netbox_custom_objects/__init__.py | 29 ++----- .../tests/schema/test_schema_api.py | 84 ++++--------------- .../tests/test_polymorphic_fields.py | 29 ++----- 3 files changed, 27 insertions(+), 115 deletions(-) diff --git a/netbox_custom_objects/__init__.py b/netbox_custom_objects/__init__.py index 0ffbecfd..96ad85b9 100644 --- a/netbox_custom_objects/__init__.py +++ b/netbox_custom_objects/__init__.py @@ -19,12 +19,8 @@ _is_migrating = contextvars.ContextVar('is_migrating', default=False) # Guards get_models() against re-entrancy (issues #685/#686): generating a COT -# model can itself trigger Django to rebuild its global relation graph (e.g. via -# ObjectType.objects.get_for_model()'s .create(), or a polymorphic field's -# related_object_types.all() query), which calls apps.get_models() again. Without -# this guard, that re-entrant call would call get_model() again for every COT -- -# including the one still mid-construction -- recursing without ever reaching the -# point where the first call finishes and registers/caches it. +# model can trigger Django to rebuild its relation graph, which calls +# apps.get_models() again while we're still mid-generation. _generating_models = contextvars.ContextVar('generating_models', default=False) # Cache for migration check to avoid repeated expensive filesystem/database operations @@ -569,23 +565,10 @@ def get_models(self, include_auto_created=False, include_swapped=False): for model in super().get_models(include_auto_created, include_swapped): yield model - # Re-entrant call (issues #685/#686): something invoked while WE are - # already generating COT models needed Django's model registry -- most - # commonly Options._relation_tree, via ObjectType.objects.get_for_model() - # or a polymorphic field's related_object_types.all() query. Recursing - # into get_model() again here would keep regenerating the same - # still-under-construction model forever, since the outer call hasn't - # reached the point where it finishes and registers/caches it. - # - # super().get_models() above already covers everything this caller can - # safely see right now: generate_model()'s type() call registers a COT's - # model with Django's app registry synchronously (inside ModelBase.__new__), - # before get_model() ever calls _after_model_generation() -- the method - # that can trigger this re-entrancy. Any COT the outer loop below hasn't - # reached yet is simply absent from this transient, incomplete snapshot; - # get_model()'s own apps.clear_cache() call (once each COT finishes) - # invalidates any relation-tree computed against that incomplete view, so - # it self-heals as soon as the remaining COTs are generated. + # Re-entrant call (issues #685/#686): generate_model() registers a COT's + # model synchronously, before get_model() can trigger this recursion, so + # super().get_models() above already covers everything safely visible + # here -- fall back to that instead of regenerating. if _generating_models.get(): return diff --git a/netbox_custom_objects/tests/schema/test_schema_api.py b/netbox_custom_objects/tests/schema/test_schema_api.py index cd91f90e..5b23c9ef 100644 --- a/netbox_custom_objects/tests/schema/test_schema_api.py +++ b/netbox_custom_objects/tests/schema/test_schema_api.py @@ -395,23 +395,9 @@ def test_apply_allow_destructive_string_returns_400(self): # --------------------------------------------------------------------------- class SchemaApplyMultiCOTRecursionTestCase(_SchemaAPIBase): - """ - Regression test for issue #685: applying a document that creates multiple - new, cross-referencing Custom Object Types in one request raised - RecursionError. - - Generating a brand-new COT's model calls _after_model_generation(), which - can itself need Django's global relation graph (e.g. via - ObjectType.objects.get_for_model()'s .create()) -- rebuilding that graph - calls apps.get_models(), which re-enters this plugin's own get_models(), - which called get_model() again for every COT (including the one still - mid-construction) with no way to ever finish. apps.clear_cache() is called - explicitly here to force a cold relation-tree cache, matching the - "first time these classes are touched" condition the issue's own - investigation identified as the trigger (reproducible regardless of - whatever unrelated activity happened to already warm the cache in a given - process). - """ + """Regression test for issue #685: applying a document that creates + multiple new, cross-referencing Custom Object Types in one request + raised RecursionError.""" def setUp(self): super().setUp() @@ -429,11 +415,9 @@ def test_apply_two_new_cross_referencing_cots_in_one_request(self): django_apps.clear_cache() - # get_models()'s CustomObjectType-enumeration loop -- the one this - # issue actually recurses through -- is unconditionally disabled under - # `manage.py test` (should_skip_dynamic_model_creation() returns True - # whenever "test" in sys.argv). Patch around that so this test - # exercises the real vulnerable loop instead of a no-op. + # should_skip_dynamic_model_creation() disables get_models()'s + # CustomObjectType loop under `manage.py test`; bypass it so this test + # exercises the real vulnerable code path. app_config = django_apps.get_app_config('netbox_custom_objects') self.enterContext(mock.patch.object(nco_pkg, '_app_ready', True)) self.enterContext( @@ -483,10 +467,6 @@ def test_apply_three_new_cots_chained_references_in_one_request(self): django_apps.clear_cache() - # See the comment in test_apply_two_new_cross_referencing_cots_in_one_request: - # bypasses should_skip_dynamic_model_creation()'s "test" in sys.argv gate - # so this test exercises get_models()'s real CustomObjectType-enumeration - # loop instead of the no-op it reduces to under `manage.py test`. app_config = django_apps.get_app_config('netbox_custom_objects') self.enterContext(mock.patch.object(nco_pkg, '_app_ready', True)) self.enterContext( @@ -541,39 +521,10 @@ def test_apply_three_new_cots_chained_references_in_one_request(self): self.assertTrue(CustomObjectType.objects.filter(slug="ospf-interfaces").exists()) def test_get_models_guards_against_reentrant_cot_generation(self): - """ - Deterministic, environment-independent regression test for the - get_models() re-entrancy itself. - - The two end-to-end tests above exercise the real API path from #685's - repro, but whether that path actually blows the Python recursion limit - depends on incidental factors (whatever else has already touched - Options._relation_tree in the process, WSGI/middleware stack depth, - interpreter version) that don't reliably reproduce in this harness. - More fundamentally, `get_models()`'s CustomObjectType-enumeration loop - (the one both issues actually recurse through) is unconditionally - disabled under `manage.py test` -- see - `CustomObjectsPluginConfig.should_skip_dynamic_model_creation()`, which - returns True whenever `"test" in sys.argv`. `_app_ready` patches below - replicate what `ready()` sets once it completes outside of tests, and - the `should_skip_dynamic_model_creation` patch replicates a production - (non-test) process, so this test exercises the actual vulnerable loop - instead of the no-op it reduces to under `manage.py test`. - - This test simulates the documented trigger directly: something deep - inside _after_model_generation() (ObjectType.objects.get_for_model() - .create(), or a polymorphic field's related_object_types.all() query -- - see the #686 test in test_polymorphic_fields.py) causes Django to - rebuild its relation graph, which calls apps.get_models() again while - the outer get_models() call is still mid-iteration. - - Without the guard, that re-entrant get_models() call walks - CustomObjectType.objects.all() and calls get_model() again for every - COT -- including ones already fully generated -- which is the root of - the unbounded growth. With the guard, the re-entrant call must return - immediately after yielding the plain Django models, without touching - CustomObjectType at all. - """ + """Deterministic counterpart to the two end-to-end tests above: whether + those actually blow the recursion limit depends on incidental process + state, so this simulates the re-entrant get_models() call directly and + asserts get_model() isn't invoked twice for an already-generated COT.""" from collections import defaultdict from unittest import mock @@ -598,9 +549,7 @@ def spying_get_model(self, *args, **kwargs): ) result = real_get_model(self, *args, **kwargs) if call_counts[self.pk] == 1: - # Simulate Django re-entering get_models() mid-generation, as it - # does when something inside _after_model_generation() needs a - # fresh model's relation graph. + # Simulate Django re-entering get_models() mid-generation. list(app_config.get_models()) return result @@ -609,15 +558,10 @@ def spying_get_model(self, *args, **kwargs): mock.patch.object(nco_pkg, '_app_ready', True), mock.patch.object(app_config, 'should_skip_dynamic_model_creation', return_value=False), ): - # Note: deliberately not calling django_apps.clear_cache() here -- - # its own implementation walks apps.get_models(include_auto_created=True) - # to expire every model's cache, which would drive this same - # CustomObjectType loop to completion once already, before the - # call below even starts. + # Not calling django_apps.clear_cache() here -- it walks + # apps.get_models() itself, which would drive this loop once + # already before the call below even starts. list(app_config.get_models()) - # Each COT's model only needs to be generated once. The re-entrant - # get_models() call simulated above must not have triggered any - # additional generation for either COT. self.assertEqual(call_counts[cot1.pk], 1) self.assertEqual(call_counts[cot2.pk], 1) diff --git a/netbox_custom_objects/tests/test_polymorphic_fields.py b/netbox_custom_objects/tests/test_polymorphic_fields.py index e4b3ab81..cbc22e39 100644 --- a/netbox_custom_objects/tests/test_polymorphic_fields.py +++ b/netbox_custom_objects/tests/test_polymorphic_fields.py @@ -2117,20 +2117,8 @@ def test_unwire_does_not_remove_descriptor_owned_by_different_field(self): class PolymorphicReverseDescriptorRecursionTestCase( TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase ): - """ - Regression test for issue #686: generating a polymorphic field's reverse - descriptor raised RecursionError. - - _wire_polymorphic_reverse_descriptors() (called from - CustomObjectType._after_model_generation() for every polymorphic field with - a related_name) evaluates field_instance.related_object_types.all(), a - queryset that needs Django's relation graph to resolve -- which calls - apps.get_models(), re-entering this plugin's own get_models(), which called - get_model() again for the same still-under-construction COT with no way to - ever finish. apps.clear_cache() is called explicitly to force a cold - relation-tree cache, matching the "first time these classes are touched" - condition the sibling issue (#685) identified as the trigger. - """ + """Regression test for issue #686: generating a polymorphic field's + reverse descriptor raised RecursionError.""" def test_get_model_with_polymorphic_related_name_does_not_recurse(self): from unittest import mock @@ -2158,17 +2146,14 @@ def test_get_model_with_polymorphic_related_name_does_not_recurse(self): field.related_object_types.set([site_ot, prefix_ot]) # Force the model out of cache and the relation-tree cold, so - # get_model() -> _after_model_generation() -> _wire_polymorphic_reverse_descriptors() - # -> related_object_types.all() hits the "first access" path that - # triggers apps.get_models() from inside Django's relation-graph build. + # get_model() -> _after_model_generation() -> related_object_types.all() + # hits the "first access" path that triggers apps.get_models(). cot.clear_model_cache(cot.id) django_apps.clear_cache() - # get_models()'s CustomObjectType-enumeration loop -- the one this - # re-entrant trigger actually recurses through -- is unconditionally - # disabled under `manage.py test` (should_skip_dynamic_model_creation() - # returns True whenever "test" in sys.argv). Patch around that so this - # test exercises the real vulnerable loop instead of a no-op. + # should_skip_dynamic_model_creation() disables get_models()'s + # CustomObjectType loop under `manage.py test`; bypass it so this test + # exercises the real vulnerable code path. app_config = django_apps.get_app_config('netbox_custom_objects') with ( mock.patch.object(nco_pkg, '_app_ready', True), From b7a1b79d1cf43a6147725e1f43f13cad2a4e13f9 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Mon, 31 Aug 2026 16:28:45 -0400 Subject: [PATCH 4/5] Hoist repeated test imports to module level mock, django_apps, and nco_pkg were each re-imported inline in all three SchemaApplyMultiCOTRecursionTestCase methods; moved to the top of the file alongside the existing imports. Per review on #687. Co-Authored-By: Claude Sonnet 5 --- .../tests/schema/test_schema_api.py | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/netbox_custom_objects/tests/schema/test_schema_api.py b/netbox_custom_objects/tests/schema/test_schema_api.py index 5b23c9ef..a51e0500 100644 --- a/netbox_custom_objects/tests/schema/test_schema_api.py +++ b/netbox_custom_objects/tests/schema/test_schema_api.py @@ -13,6 +13,9 @@ """ +from unittest import mock + +from django.apps import apps as django_apps from django.urls import reverse from django.test import TransactionTestCase from rest_framework import status @@ -22,6 +25,7 @@ from users.models import ObjectPermission from utilities.testing import create_test_user +import netbox_custom_objects as nco_pkg from netbox_custom_objects.schema.exporter import export_cot from netbox_custom_objects.models import CustomObjectType @@ -407,12 +411,6 @@ def setUp(self): perm.object_types.add(ObjectType.objects.get_for_model(CustomObjectType)) def test_apply_two_new_cross_referencing_cots_in_one_request(self): - from unittest import mock - - from django.apps import apps as django_apps - - import netbox_custom_objects as nco_pkg - django_apps.clear_cache() # should_skip_dynamic_model_creation() disables get_models()'s @@ -459,12 +457,6 @@ def test_apply_two_new_cross_referencing_cots_in_one_request(self): def test_apply_three_new_cots_chained_references_in_one_request(self): """The issue notes a 3-type chain (interface -> area -> instance) fails identically to the 2-type case; cover it too.""" - from unittest import mock - - from django.apps import apps as django_apps - - import netbox_custom_objects as nco_pkg - django_apps.clear_cache() app_config = django_apps.get_app_config('netbox_custom_objects') @@ -526,11 +518,6 @@ def test_get_models_guards_against_reentrant_cot_generation(self): state, so this simulates the re-entrant get_models() call directly and asserts get_model() isn't invoked twice for an already-generated COT.""" from collections import defaultdict - from unittest import mock - - from django.apps import apps as django_apps - - import netbox_custom_objects as nco_pkg cot1 = self.create_custom_object_type(name='Reentrancy Source', slug='reentrancy-source') cot2 = self.create_custom_object_type(name='Reentrancy Target', slug='reentrancy-target') From d34ca3ac6d08d2703cc116a10d7580b45866553f Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Wed, 2 Sep 2026 17:37:15 -0400 Subject: [PATCH 5/5] Strengthen #685/#686 regression tests per review Two improvements from jnovinger's review: - The end-to-end tests only asserted "didn't raise" plus DB-row existence, which would pass just as well against a get_models() that silently returned zero COT models -- the degradation mode the guard itself introduces. Now assert the generated model is actually returned by apps.get_models(), matching test_schema_operations.py's #335 test. - Since get_models() is a generator, the _generating_models set/reset pair is scoped to iteration, not a call frame. Added assertFalse(_generating_models.get()) after each guarded call so a future refactor that leaves the flag set (and starts silently truncating every other caller's model list) gets caught. Applying the first improvement to the #686 polymorphic-descriptor test surfaced a real, previously-invisible bug: cot.get_model() called directly (not via get_models()'s own loop) on an uncached COT can trigger generate_model() twice for the same COT -- once for the direct call, once more when _wire_polymorphic_reverse_descriptors() re-enters get_models() before the direct call has cached anything. The direct call's return value and CustomObjectType._model_cache end up holding one class; Django's app registry ends up holding a different one. Filed as #688; the test now looks up the actually-registered class via apps.get_model() rather than trusting get_model()'s return value, noting the caveat inline. Verified: all 4 tests fail with a genuine RecursionError against the pre-fix __init__.py and pass against the fix. Full test_schema_api.py + test_polymorphic_fields.py run: 113 tests, 0 new failures (1 pre-existing, unrelated netbox-branching environment error). --- .../tests/schema/test_schema_api.py | 38 ++++++++++++++++--- .../tests/test_polymorphic_fields.py | 11 ++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/netbox_custom_objects/tests/schema/test_schema_api.py b/netbox_custom_objects/tests/schema/test_schema_api.py index a51e0500..22addd40 100644 --- a/netbox_custom_objects/tests/schema/test_schema_api.py +++ b/netbox_custom_objects/tests/schema/test_schema_api.py @@ -451,8 +451,19 @@ def test_apply_two_new_cross_referencing_cots_in_one_request(self): resp = self.client.post(self.apply_url, data=self._apply_body(schema_doc), format="json") self.assertEqual(resp.status_code, status.HTTP_200_OK, resp.content) self.assertTrue(resp.data["applied"]) - self.assertTrue(CustomObjectType.objects.filter(slug="ospf-instances").exists()) - self.assertTrue(CustomObjectType.objects.filter(slug="ospf-areas").exists()) + instance_cot = CustomObjectType.objects.get(slug="ospf-instances") + area_cot = CustomObjectType.objects.get(slug="ospf-areas") + self.assertIn( + instance_cot.get_model(), + django_apps.get_models(), + "Generated model should be returned by apps.get_models().", + ) + self.assertIn( + area_cot.get_model(), + django_apps.get_models(), + "Generated model should be returned by apps.get_models().", + ) + self.assertFalse(nco_pkg._generating_models.get()) def test_apply_three_new_cots_chained_references_in_one_request(self): """The issue notes a 3-type chain (interface -> area -> instance) fails @@ -508,9 +519,25 @@ def test_apply_three_new_cots_chained_references_in_one_request(self): resp = self.client.post(self.apply_url, data=self._apply_body(schema_doc), format="json") self.assertEqual(resp.status_code, status.HTTP_200_OK, resp.content) self.assertTrue(resp.data["applied"]) - self.assertTrue(CustomObjectType.objects.filter(slug="ospf-instances").exists()) - self.assertTrue(CustomObjectType.objects.filter(slug="ospf-areas").exists()) - self.assertTrue(CustomObjectType.objects.filter(slug="ospf-interfaces").exists()) + instance_cot = CustomObjectType.objects.get(slug="ospf-instances") + area_cot = CustomObjectType.objects.get(slug="ospf-areas") + interface_cot = CustomObjectType.objects.get(slug="ospf-interfaces") + self.assertIn( + instance_cot.get_model(), + django_apps.get_models(), + "Generated model should be returned by apps.get_models().", + ) + self.assertIn( + area_cot.get_model(), + django_apps.get_models(), + "Generated model should be returned by apps.get_models().", + ) + self.assertIn( + interface_cot.get_model(), + django_apps.get_models(), + "Generated model should be returned by apps.get_models().", + ) + self.assertFalse(nco_pkg._generating_models.get()) def test_get_models_guards_against_reentrant_cot_generation(self): """Deterministic counterpart to the two end-to-end tests above: whether @@ -550,5 +577,6 @@ def spying_get_model(self, *args, **kwargs): # already before the call below even starts. list(app_config.get_models()) + self.assertFalse(nco_pkg._generating_models.get()) self.assertEqual(call_counts[cot1.pk], 1) self.assertEqual(call_counts[cot2.pk], 1) diff --git a/netbox_custom_objects/tests/test_polymorphic_fields.py b/netbox_custom_objects/tests/test_polymorphic_fields.py index cbc22e39..aa23d46b 100644 --- a/netbox_custom_objects/tests/test_polymorphic_fields.py +++ b/netbox_custom_objects/tests/test_polymorphic_fields.py @@ -2161,6 +2161,17 @@ def test_get_model_with_polymorphic_related_name_does_not_recurse(self): ): # Must not raise RecursionError. model = cot.get_model() + self.assertFalse(nco_pkg._generating_models.get()) + # Look up the registered class directly rather than trusting `model`: + # a nested get_models() call triggered mid-generation (as above) can + # leave get_model()'s return value out of sync with what's actually + # registered in the app registry -- see #688. + registered_model = django_apps.get_model(APP_LABEL, model.__name__) + self.assertIn( + registered_model, + django_apps.get_models(), + "Generated model should be returned by apps.get_models().", + ) self.assertTrue( hasattr(Site, "rev_recursion_test"), "Reverse descriptor must still be set on Site after get_model()",