Closes: #471 - selectable on_delete behavior for Object-type fields

[bctiemann]

Summary

Extends the fix from #484 by replacing the blanket SET NULL behavior with a user-selectable on_delete_behavior for single-Object-type fields.

• Adds on_delete_behavior CharField to CustomObjectTypeField with three choices: Set null (default), Cascade, Protect
• The DB-level FK constraint in _ensure_field_fk_constraint now uses the field's choice (SET NULL / CASCADE / RESTRICT)
• The Django field definition in ObjectFieldType.get_model_field() also uses the choice
• Changing on_delete_behavior on an existing field re-applies the FK constraint on save
• Exposed in the UI form (single-object fields only) and REST API
• Migration 0011 adds the column; all existing fields default to set_null (consistent with what Fixes: #471 - Selectable on_delete behavior for Object-type fields (and: Change Object-type FK constraints from CASCADE to SET NULL) #484 set in the DB)
• Tests cover all three behaviors via raw SQL DELETE to exercise the actual DB constraints

Notes

• MultiObject fields (M2M through-table) are unaffected — CASCADE on the join-table FKs is correct for those
• PROTECT maps to PostgreSQL RESTRICT (raises IntegrityError on delete attempt, preventing deletion)
• This PR depends on Fixes: #471 - Selectable on_delete behavior for Object-type fields (and: Change Object-type FK constraints from CASCADE to SET NULL) #484 and should be reviewed/merged after it

Test plan

• Run python manage.py test netbox_custom_objects.tests.test_deletion --keepdb
• Verify 8/8 tests pass including the new cascade and protect tests
• Manually verify on_delete_behavior dropdown appears for Object-type fields in the UI
• Verify multiobject fields do NOT show the on_delete_behavior dropdown

Generated with Claude Code

[bctiemann]

@claude /review

[claude]

Claude finished @bctiemann's task in 2m 0s —— View job

---

Code Review — PR #485: Selectable on_delete behavior for Object-type fields

• Gathering context
• Reading changed files
• Analyzing code quality, correctness, and test coverage
• Posting review feedback

---

Overview

This PR adds a user-selectable on_delete_behavior (SET NULL / CASCADE / PROTECT) for single-Object-type fields, replacing the fixed SET NULL from #484. The implementation is thorough and the test coverage is excellent. A few issues worth addressing below.

---

Issues

1. SQL injection via f-string in _ensure_field_fk_constraint (models.py:738)

constraint_name, table_name, and column_name are all interpolated directly into SQL via f-strings without quoting protection:

cursor.execute(f'ALTER TABLE "{table_name}" DROP CONSTRAINT IF EXISTS "{constraint_name}"')

While the values come from internal model metadata (not user input), on_delete_sql is also interpolated:

ON DELETE {on_delete_sql}

on_delete_sql is resolved through the _ON_DELETE_SQL mapping, so values are statically known strings ("CASCADE", "SET NULL", "RESTRICT"). However, there's a subtle issue: if on_delete_behavior is a value not in the mapping, the fallback is "SET NULL":

on_delete_sql = self._ON_DELETE_SQL.get(on_delete_behavior, "SET NULL")  # line 719

That's fine, but the table and column identifiers are still interpolated as f-strings rather than as parameterized identifiers. While not an immediate vulnerability (they come from _meta), it would be safer to use connection.ops.quote_name() consistently. Fix this →

---

2. clean() validation allows set_null on non-object fields (models.py:1388–1400)

The validation only rejects non-default on_delete_behavior on non-object fields:

if (
    self.on_delete_behavior
    and self.on_delete_behavior != ObjectFieldOnDeleteChoices.SET_NULL
    and self.type != CustomFieldTypeChoices.TYPE_OBJECT
):

This means if someone saves a text field with on_delete_behavior="set_null" via the API, no error is raised — even though it's meaningless. The default is set_null so this situation is hard to accidentally reach through the UI, but an API consumer could set it explicitly. Consider also rejecting explicit set_null on non-object fields, or simply clearing the field silently. Fix this →

---

3. _ensure_field_fk_constraint queries information_schema without schema filter (models.py:728–734)

cursor.execute("""
    SELECT constraint_name
    FROM information_schema.table_constraints
    WHERE table_name = %s
    AND constraint_type = 'FOREIGN KEY'
    AND constraint_name LIKE %s
""", [table_name, f"%{column_name}%"])

This query searches information_schema.table_constraints across all schemas in the database. If there are multiple schemas, this could match constraints from another schema. Adding AND table_schema = current_schema() (or the equivalent for the connection) would prevent false positives. Fix this →

---

4. on_delete_behavior accepted via API for multiobject fields (api/serializers.py)

The serializer's validate() doesn't reject on_delete_behavior being set on a multiobject field. The model's clean() rejects non-default values, so this will eventually error, but not with a clean API validation message. Consider adding a check in the serializer's validate() to produce a friendlier error. Low severity.

---

5. to_representation nulls on_delete_behavior but doesn't prevent writes (api/serializers.py:129–133)

The to_representation returns None for non-object fields, but the serializer still accepts on_delete_behavior as input for any field type. If a client sends "on_delete_behavior": "cascade" for a text field, it will flow through to clean() and raise a validation error there instead of at the serializer layer. Not a bug, but not ideal API UX.

---

Observations / Minor Points

• _ON_DELETE_MAP placement (field_types.py:490): The module-level _ON_DELETE_MAP dict is defined between two classes. This is fine but slightly unusual; a class-level dict on ObjectFieldType or a constant at the top of the module would be more conventional.

• Duplicate default in migration (migrations/0011): The migration specifies default="set_null" as a string, while the model uses default=ObjectFieldOnDeleteChoices.SET_NULL. These are equivalent (SET_NULL = "set_null"), but if the constant value were ever changed, the migration would be out of sync. Migrations are snapshots, so this is expected, but worth noting.

• CO-to-CO CASCADE uses Django ORM delete, core object tests use raw SQL (test_deletion.py:309 vs. :246): The asymmetry is intentional and well-commented — CO-to-CO CASCADE works through Django's collector, while core-object FKs must be tested via raw SQL to bypass Django's Python-level pre-delete handling. The tests correctly exercise both paths. Good approach.

• test_protect_co_to_co_enforced_at_db_level (test_deletion.py:502): This test deletes via raw SQL to confirm the DB-level RESTRICT constraint is in effect for CO-to-CO, which is good. But it's duplicating what test_delete_co_referenced_by_another_co_protect already tests via ORM. Both tests are valuable — one proves the Django PROTECT behavior, the other proves the DB-level constraint.

• should_ensure_fk not set for on_delete_changed when _state.adding (models.py:1895–1910): On a new field, should_ensure_fk is True unconditionally when the type is OBJECT, so the on_delete_changed check is only evaluated on updates. This is correct — no issue here.

• transaction.on_commit in ensure_constraint — this correctly defers the FK constraint re-creation until after the Django transaction commits, avoiding "pending trigger events" errors. Good choice.

---

Test Coverage

Test coverage is comprehensive:

• SET NULL, CASCADE, PROTECT for core object references (raw SQL DELETE path) ✅
• SET NULL, CASCADE for CO-to-CO references (ORM path) ✅
• PROTECT for CO-to-CO references (both ORM and raw SQL) ✅
• Changing on_delete_behavior updates the DB constraint ✅
• Cache invalidation on on_delete_behavior change ✅
• Non-object field save doesn't affect unrelated COT timestamps ✅

---

Summary

The implementation is solid. The main issues to address are:

1. Add a schema filter to the information_schema query (Dynamic models #3 above — correctness in multi-schema setups)
2. The validation gap allowing set_null to be set on non-object fields via API (Return null QuerySet if creating a new CustomObject #2 — minor)
3. SQL identifier safety in _ensure_field_fk_constraint (Custom objects UI #1 — low risk but best practice)

The architecture of using on_commit to apply FK constraints and cache-busting the related COT model is well thought out.

[bctiemann]

Addressed all actionable issues from the review; also noticed that the portable schema needed to be updated to include the new deletion behavior, and at the same time hardened to detect future infrastructural changes to COT/COTF/other internal models. Now has tests to detect such changes.

[bctiemann]

@arthanson Note that this is based off of another branch that initially fixes the "bug" by setting SET_NULL as the default behavior (a much smaller change). But if you approve this we can merge that into this and then merge both into feature, if we want to simplify the migration path.