[bctiemann]

Fixes: #902

A polymorphic Object field is a generic foreign key: a content type column and an object ID column, with no database FK. So on_delete_behavior did nothing for it. Deleting the referenced object always succeeded and left a dangling reference, whatever the field said. That's true even for set_null: the object ID and content type were never cleared.

Changes

• Protect (polymorphic_on_delete() in models.py): the deletion collector's delete() is wrapped. Before anything else, it looks up in one query the polymorphic fields that may point at any type being deleted. It then checks the objects about to be deleted, with one query per protecting field. If a polymorphic field set to protect points at one of them, it raises ProtectedError listing the referencing custom objects. Custom objects deleted in the same operation, for example by a real FK's cascade, don't count. This is the same point where Django refuses a deletion that a real FK protects: before any signal is sent or the deletion's transaction starts. So nothing has been logged or queued, and a caller's own transaction stays usable. NetBox's CustomCollector opens its transaction in its own delete() on current NetBox, so the check wraps that too, and runs once per collector. The plugin already patches the collector, in install_collector_patch(). NetBox shows its usual "Unable to delete" message, and the REST API returns 409.

• Cascade and set null (apply_polymorphic_on_delete(), a pre_delete receiver): uses the fields that polymorphic_on_delete() looked up for the deletion, so a delete of many objects adds one field lookup, not one per object. It finds the custom objects whose polymorphic field points at the object being deleted, then:

  • cascade: deletes them one by one, so their own signals, change logging and polymorphic references are handled too. Objects already being deleted further up the stack are skipped, so a cycle or a self-reference doesn't recurse.
  • set null: clears the reference. It snapshots, clears and saves each object, as core does for a nullable FK, so the change is logged. Objects already being deleted in the same request are skipped, as core skips them.

  Both skip migrations, auto-created through models and NetBox's private models (such as CablePath).

• Receiver order (__init__.py): ready() reconnects core's handle_deleted_object so it runs after the new receiver. What a cascade deletes or set null clears is then logged before the deletion that caused it. That's the order Django's collector logs a real FK's dependents in. A branch revert relies on it: it undoes changes newest first, and restoring a custom object's generic FK fails if its target hasn't been restored yet. The plugin already reconnects this handler whenever a type is deleted, so core's handler already ended up last after any type deletion.

• Form and model: the field form now offers on_delete_behavior for polymorphic Object fields. clean() no longer resets it to set_null for them; it still does for non-Object fields. The REST API and schema apply already accepted it.

• Docs: field-attributes.md says the setting applies to polymorphic Object fields too.

Existing polymorphic fields saved with protect or cascade through the REST API or schema apply will start enforcing it. Fields created in the UI were always reset to set_null.

A protect reached only through a cascade, such as a custom object that is itself protected and is deleted by another field's cascade, is refused by the nested deletion, inside the outer one's transaction. Callers that catch the error outside their transaction, as NetBox's views and REST API do, aren't affected.

Polymorphic multi-object fields are unchanged: their stale through rows after a target is deleted are a separate, documented gap.

Testing

New module tests/test_polymorphic_on_delete.py (17 tests):

• Through the ORM:
  • set null, cascade and protect, each applied when the referenced site is deleted. A refused deletion leaves the caller's transaction usable, which the tests check by querying in the same one;
  • deleting an unreferenced object is allowed;
  • a reference to another type that shares the deleted object's pk is left alone;
  • QuerySet.delete() is handled;
  • deleting five sites looks up the polymorphic fields once, where the per-object lookup took six;
  • protect on one field stops a cascade on another;
  • a custom object deleted in the same operation, through a real FK's cascade, doesn't protect;
  • a cascade runs through custom objects that point at other custom objects;
  • a cycle and a self-reference are each deleted once;
  • a referenced custom object can be deleted;
  • clean() keeps the setting.
• Through the UI and REST API:
  • a protected delete is refused (redirect, and 409);
  • a cascade is logged before the deletion that caused it;
  • set null is logged as an update, before the deletion.

Against unmodified feature, 13 of the original 15 fail. The two that pass are the controls: the unreferenced delete, and the other type with the same pk. Without the receiver reorder, the two log-order tests fail.

test_forms.py's visibility test now expects the setting on polymorphic Object fields.

Both full suites pass, each with the 17 new tests: 1,768 tests without netbox-branching and 1,862 with it (6 skipped), 1 expected failure (#892) in each.

ReferencedObjectDeletionTest in test_polymorphic_fields.py still passes, but its docstring says deleting a site leaves a stale object ID. #953 is moving that file. I'll correct the docstring there once #953 merges, to avoid a conflict git couldn't resolve.

🤖 Generated with Claude Code

[bctiemann]

@claude /review and report only on actionable issues.

[github-actions]

Claude finished @bctiemann's task in 5m 44s —— View job

---

PR Review: Fixes #902 — Apply on_delete_behavior to polymorphic Object fields

• Read changed files
• Analyze diff vs origin/feature
• Review code quality and correctness
• Post findings

---

The core logic is sound: the ContextVar-based cycle guard works correctly, the ProtectedError is raised before any cascade begins, and the signal re-ordering in ready() correctly ensures handle_deleted_object runs after apply_polymorphic_on_delete. Tests are thorough and cover the important edge cases. Three actionable issues below.

---

1. _signals_received import has no fallback — will break on older NetBox

netbox_custom_objects/models.py:33

from core.signals import _signals_received, handle_deleted_object

_signals_received is a private (underscore-prefixed) internal of NetBox core. If it doesn't exist in NetBox 4.4.x (the plugin's min_version), this ImportError will prevent the plugin from loading entirely, on every startup.

The access site is:

deleted = getattr(_signals_received, 'pre_delete', set())

If _signals_received is not present in older versions, the correct fix is a guarded import with a safe fallback:

try:
    from core.signals import _signals_received
except ImportError:
    _signals_received = None  # COMPAT(netbox<X.Y.Z): _signals_received added in X.Y.Z

And at the use site:

deleted = getattr(_signals_received, 'pre_delete', set()) if _signals_received is not None else set()

If _signals_received is present across all supported NetBox versions (4.4.0–4.6.x), a comment confirming this would satisfy the concern. Otherwise, the fallback to an empty set() is safe: it means set-null will occasionally log an update after a deletion log rather than before, which is the pre-existing behaviour.

Fix this →

---

2. @receiver(pre_delete) with no sender adds a DB query to every deletion in the system

netbox_custom_objects/models.py:5402

@receiver(pre_delete)
def apply_polymorphic_on_delete(sender, instance, **kwargs):

Without a sender, this fires for every model deletion across all of NetBox — DCIM, IPAM, everything. The early-exit check:

fields = list(
    CustomObjectTypeField.objects.filter(
        type=CustomFieldTypeChoices.TYPE_OBJECT,
        is_polymorphic=True,
        related_object_types=object_type.pk,
    ).select_related('custom_object_type')
)
if not fields:
    return

...issues a real database query for every delete. For bulk operations (e.g., deleting hundreds of IPs), this adds one query per object.

A content-type cache keyed by sender (or object_type.pk) would avoid the repeated query for types that have never had polymorphic fields pointing at them. Something like a module-level set of content-type PKs known to be referenced, populated on field save/delete, then cleared on type cache invalidation, would reduce the query to a set lookup in the common case (no polymorphic fields reference this type). This is an optimisation rather than a correctness fix, but worth flagging given the blast radius.

---

3. Minor: _netbox_private origin undocumented

netbox_custom_objects/models.py:5415

or getattr(instance, '_netbox_private', False)

The getattr default makes this safe if the attribute doesn't exist. But a comment saying where this attribute comes from (which NetBox class sets it and why) would help maintainers. As-is it reads as magic. For example:

# NetBox sets _netbox_private = True on internal models (e.g. CablePath) that should
# not participate in change logging or plugin-side signal handlers.
or getattr(instance, '_netbox_private', False)

---

No correctness bugs found. The cycle guard, protect-beats-cascade ordering, and signal re-ordering all look right. Issues 1 and 3 are the most pressing; issue 2 is an optimisation for later.

[bctiemann]

Before merging

Two follow-ups depend on other PRs:

• If Closes #952: Split the polymorphic field tests by layer #953 merges first: correct the docstrings of ReferencedObjectDeletionTest and test_deleting_referenced_site_nulls_gfk_accessor, which will be in tests/models/test_polymorphic_fields.py. They say deleting a referenced site leaves a stale object ID, but with this PR the default set_null clears it. I left them alone here so this PR doesn't conflict with Closes #952: Split the polymorphic field tests by layer #953's move of that file. If this PR merges first, the correction goes in its own small PR once Closes #952: Split the polymorphic field tests by layer #953 lands.
• Once Closes #950: Split test_branching.py into a tests/branching package by area #951 merges: add a branching test in tests/branching/. It deletes a site that a cascading polymorphic field references, in a branch, then reverts the branch, and checks that the site and the custom object both come back. This PR depends on the order that makes that work: core's handle_deleted_object now runs after apply_polymorphic_on_delete(), so the custom object's deletion is logged first. test_cascade_is_logged_before_the_deletion_that_caused_it covers the log order, but not the revert itself. This test can come after this PR as a separate one.

Also check that CI is green at the head you merge. The v4.5.2 jobs are the ones that caught the protect transaction issue.

[bctiemann]

Thanks. Taking these in turn:

1. _signals_received: no change. The plugin's min_version is 4.5.2, not 4.4, and core.signals._signals_received has existed since July 2025. Every supported NetBox has it, and the v4.5.2 CI jobs import it and pass. It's a private name, but the plugin already imports handle_deleted_object from the same module.
2. A query on every deletion: fixed in fc9ce38, without a cache that would have to be invalidated across workers. The deletion collector's delete() is already wrapped for the protect check, and it sees every object a deletion removes. polymorphic_on_delete() now looks up, in one query, the polymorphic fields for all the types being deleted, checks protect, and hands that lookup to the pre_delete handler for the rest of the deletion. A bulk delete now adds one field lookup rather than one per object. A new test pins this: deleting five sites queries the link table once, where the previous commit took six.
3. _netbox_private: added a comment saying it's NetBox's marker for internal models such as CablePath, which core's change logging skips too.

Note that this review ran on the first commit. Protect has since moved out of the pre_delete handler into the collector, so it now raises before the deletion's transaction starts, as for a real foreign key. The PR description covers that.