Skip to content

feat: adopt duplicate module creates and guard all-None matcher lookups - #201

Open
leoparente wants to merge 5 commits into
developfrom
feat/module-pre-save-adoption
Open

feat: adopt duplicate module creates and guard all-None matcher lookups#201
leoparente wants to merge 5 commits into
developfrom
feat/module-pre-save-adoption

Conversation

@leoparente

Copy link
Copy Markdown
Contributor

Fixes MON-305.

Problem

Two related defects around NULL handling in object matching:

  1. Duplicate module creates. In plan-ahead ingest topologies (plan-all-then-apply-all, e.g. BulkPlanView + BulkApplyView), a sub-ModuleBay's nested module= ref plans a second dcim.module CREATE for a bay whose module was created by an earlier changeset in the same run. Since v1.12.0 the failed INSERT is recovered by adopting the existing module, but the recovery is lossy (the second CREATE's divergent fields are silently discarded for that round) and noisy (a failed INSERT plus a swallowed Postgres error per duplicate). Agents work around it by dropping the sub-bay's module= parent link, losing UI nesting.
  2. NULL-lookup hijacks. A matcher fed an explicit null in its unique field binds an arbitrary row: asset_tag: null adopts and physically relocates an unrelated module, master: null binds an arbitrary masterless VirtualChassis of any name, rd: null binds an unrelated rd-null VRF. A sibling defect fuses two different-identity entities that share only a None-keyed fingerprint (e.g. two null-asset_tag modules), failing the plan with a merge-conflict 400. Explicitly-cleared generic refs (assigned_object, scope) crash content_type_id(None) and 500 the plan.

Fix

All-None matcher guard (ObjectMatchCriteria), landed first: a matcher is skipped (queryset and fingerprint) only when every referenced lookup value is None. Partial-null lookups keep today's behavior byte-identically (Django renders =None as IS NULL, which live clear-FK dedupe depends on — e.g. rack location: null). Includes None-safe ContentType preparation (cleared generic refs no longer 500) and a None-safe case-insensitive fingerprint path (a null in an insensitive ref no longer crashes). Consequence worth knowing: the logical_mac_address_within_parent isnull-variant's queryset path is functional for the first time — explicit-null MAC assignments now match by MAC + assigned-object-isnull (its path previously crashed before querying; absent-key payloads are unaffected).

dcim.module joins _REQUIRES_PRE_SAVE_MATCH: the applier's find-first pass converts the plan-ahead duplicate CREATE into a partial update of the bay's module via the DB-backed unique_module_bay matcher — payload applied instead of discarded, sub-bay nesting restored, idempotent, no failed-INSERT noise.

Behavior changes

  • Occupied-bay CREATEs adopt-update, last-writer-wins including module_type (previously the incoming payload was silently discarded for that round). A type swap retains the old type's auto-created children and does not instantiate the new type's templates — parity with today's plan-time UPDATE semantics.
  • Validation failures against the adopted instance now surface as a 400 that aborts the whole changeset atomically — sibling changes in the same changeset roll back, and retries fail deterministically until the producer fixes the payload. Previously the same changeset returned 200 with the bad payload silently dropped. Other changesets in a bulk batch are unaffected (207 multi-status). This is the most visible change for producers.
  • These semantics apply to raw apply-change-set clients as well, not only the diode pipeline.
  • A real-value asset_tag match may physically move a module at apply time (plan-time already had move semantics).
  • IntegrityError noise is gone for the plan-ahead duplicate; genuine conflict races can still surface one as a generic 400.

Tests

  • Unit (guard): hijack skips pinned at matcher level (asset_tag/master); partial-null multi-field dedupe preserved (rack); macaddress isnull-variant matches without crashing; condition-scoped null matchers unaffected; cleared-GFK payloads no longer raise; None-fingerprint fusion removed; partial-null fingerprints preserved (mutation-tested: allany fails).
  • E2E (adoption): the plan-ahead repro with load-bearing asserts (divergent payload applied, re-plan empty); null-asset_tag relocation hazard pin; occupied-bay last-writer-wins; deterministic atomic-abort test (400 + sibling rolled back, previously 200 + sibling applied); miss-then-create.
  • Byte-identical across the v4.4.x/v4.5.x/v4.6.x trees; green on v4.6.0, v4.5.5, v4.4.10 plus existing matcher/apply/update suites.
  • Verified end-to-end through the real ingest pipeline (SDK → gRPC ingress → reconciler → plugin → NetBox): a device + linecard + sub-bay carrying its module= parent link + installed transceiver lands with correct nesting, and a second identical ingest converges with no changes.

Follow-up

Agent-side (tracked in MON-305): with nesting proven end-to-end, discovery agents can drop the strip-the-parent-link workaround for transceiver sub-bays.

🤖 Generated with Claude Code

leoparente and others added 5 commits August 11, 2026 20:27
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Skip .lower() on None values in the case-insensitive fingerprint
  branch; previously any None in an insensitive ref field crashed
  fingerprinting with AttributeError.
- Move the expressions all-None skip before _prepare_data and reuse
  the cached _get_refs() ref set, mirroring the fields-path guard so
  both read identical raw inputs.
- Pin the all-vs-any semantics of the fingerprint all-None guard with
  a regression test on the dcim.macaddress isnull-variant matcher, and
  a regression test for the None-safe case-insensitive fix. Also drop
  an unused Location import from the test module.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

☂️ Code Coverage

current status: ✅

Overall Coverage

Statements Covered Coverage Threshold Status
7592 7024 93% 0% 🟢

New Files

File Coverage Status
netbox_diode_plugin/tests/v4.6.x/tests/test_matcher_none_guard.py 97% 🟢
netbox_diode_plugin/tests/v4.6.x/tests/test_module_adoption.py 100% 🟢
TOTAL 98% 🟢

Modified Files

File Coverage Status
netbox_diode_plugin/api/matcher.py 88% 🟢
TOTAL 88% 🟢

updated for commit: 4c462da by action🐍

@leoparente

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 4c462dab26

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@leoparente
leoparente marked this pull request as ready for review August 13, 2026 13:23
@leoparente leoparente self-assigned this Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants