Conversation
…ranch connection (#680)
Use TaggableManager.model when running post_through_setup() so Django's lazy registry lookup cannot bind the tagged_items relation to a stale dynamic model class. This restores cascading deletion of TaggedItem records for regenerated custom object models in both main and branch contexts. Add regression coverage for direct and bulk deletion, deletion through a regenerated model, relation setup, and tag filtering after deletion.
… after (#681) * Fixes #677: Set polymorphic GFK field values before full_clean(), not after CUSTOM_VALIDATORS' validate(instance, request) runs inside instance.clean(), called by Django's full_clean() -- but the polymorphic single-object (GFK) field's value was applied to the instance strictly after full_clean() had already run, in both the form and API paths: - Form: custom_save() (called by form.save(), after form.is_valid() has already run full_clean() via ModelForm._post_clean()) was the only place setattr(instance, field_name, obj_val) happened for the field. - API: the serializer's validate() popped the polymorphic field out of the data dict entirely before delegating to ValidatedModelSerializer.validate() (which builds/mutates the instance and calls instance.full_clean()), only restoring the field afterward for create()/update() to consume. Net effect: a CUSTOM_VALIDATORS validator always saw the field as blank (new object) or the pre-edit value (existing object), regardless of what was actually submitted. Fix: - views.py: custom_clean() (the form's own clean(), which runs inside full_clean() but before Django calls _post_clean()'s instance.full_clean()) now also does setattr(self.instance, field_name, obj_val) for each polymorphic single-object field. custom_save()'s existing assignment is left in place (idempotent, reapplies the same cleaned_data value before the actual DB write). - api/serializers.py: validate() now substitutes the field's two real backing scalar columns (<name>_content_type_id, <name>_object_id) for the resolved object before delegating to the parent validate(), then restores the resolved object under the original field name afterward. Those two columns are real Django model fields (unlike the field itself, a private/virtual GFK descriptor), so an unsaved instance can hold them fine -- unlike the polymorphic M2M case (still popped/restored as before), which genuinely has no home on an instance without a PK. Adds 4 regression tests (test_polymorphic_fields.py) covering both create and edit/update through both the form and API paths. All 4 fail without the fix, reproducing the exact reported symptom (None on create, stale pre-edit value on update), and pass with it. Verified: test_polymorphic_fields.py (82 tests, only the 1 known unrelated netbox_branching environment error) and the full plugin suite in a clean venv: 1165 tests, 0 failures/errors, 2 skipped. * Trim excessive comments and docstrings Condense the serializer/form explanatory comments and one test docstring down to the essential why, per repo style. * Address automated PR review comment on #681 - Guard the setattr in custom_clean() behind an else: it previously ran unconditionally, so on the "type selected but no object chosen" error path it also overwrote self.instance's pre-edit GFK value with None. Harmless to data (the form won't save with errors present), but a CUSTOM_VALIDATORS validator would still see None during that same full_clean() call instead of the object's actual current value. - The flagged comment typo was already fixed by the earlier comment-trim commit. - Add test_custom_validator_sees_cleared_gfk_value_on_api_update, covering the previously-untested None-clear path through the API: PATCHing poly_obj to null must clear it on the instance validate() runs against, and the restore-after-validate() step must still round-trip None correctly into update().
Co-authored-by: Martin Hauser <mhauser@netboxlabs.com>
Release v0.6.1
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add Suspected Cause and Proposed Fix fields to bug report template
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # netbox_custom_objects/tests/schema/test_schema_api.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merges
mainintofeaturein prep for the v0.7.0 release, bringing in everything merged tomainsince the last sync, including:get_model()'s own class after a re-entrant regenerationschema/apply/raises RecursionError when a document creates multiple new, cross-referencing Custom Object Types in one request #685 — guardget_models()against re-entrancyConflicts
One genuine conflict, in
netbox_custom_objects/tests/schema/test_schema_api.py: both branches independently added a new test class at the same insertion point (feature'sSchemaYAMLFormatTestCasefor #665,main'sSchemaApplyMultiCOTRecursionTestCasefor #685/#688). Not a real logical conflict -- resolved by keeping both classes intact as separate, complete definitions (each with its ownsetUp(), since the two happened to share an identical-looking tail that made git render them as overlapping).Testing
netbox_custom_objects.tests.schema.test_schema_api: 41/41 passing post-merge.netbox_custom_objects.tests.test_polymorphic_fields+test_models: same 3 pre-existing, unrelated errors as before the merge (all fromJob.delete()'snetbox_branchingimport failing under the non-branching test config -- confirmed these test methods are unchanged frommain, not introduced by this merge).