Closes: #389 - Upgrade executor tool

[bctiemann]

Closes: #389

Implements the COT upgrade executor: a public Python API that takes a schema document (or pre-computed COTDiff list from the comparator, #387) and applies all changes to the live DB atomically.

Public API — executor.py

apply_document(schema_doc, *, allow_destructive=False) → list[COTDiff]
apply_diffs(diffs, type_defs_by_slug, *, allow_destructive=False) → None

apply_document is the primary entry point. apply_diffs is the lower-level primitive used by the schema apply API endpoint (#390, PR #454) when diffs are pre-computed for preview then applied separately.

Two-phase execution

All DB writes are wrapped in a single transaction.atomic() block. Any exception causes a full rollback — including DDL (table creates/drops) since PostgreSQL supports transactional DDL.

Phase 1 — COT records
New CustomObjectType instances are created (which triggers CustomObjectType.create_model() and the backing CREATE TABLE). Existing COTs have their top-level attributes updated. New COTs are created in topologically-sorted order with respect to cross-COT related_object_type references, so a referenced COT's table always exists when the referencing COT's field is added in Phase 2.

Phase 2 — Fields
All ADD, ALTER, and REMOVE field operations are applied using the existing CustomObjectTypeField.save() / delete() mechanisms, which encapsulate all required DDL (add_field, alter_field, remove_field). By this point all COT tables are guaranteed to exist, so cross-COT FK resolution never fails due to ordering.

Finalisation
After Phase 2, for each COT: schema_document is updated (via QuerySet.update() to avoid clearing the model cache) and next_schema_id is synced to cover any schema_id values explicitly assigned during the apply.

Dependency ordering

_build_dep_order(diffs) performs a DFS-based topological sort over the new-COT subset of the diff list. Only custom-objects/<slug> references within the document are treated as ordering constraints; references to existing COTs and built-in NetBox models are ignored. Self-references are not treated as cycles.

Raises CircularDependencyError if a cycle is detected before any DB writes.

Exceptions

Exception	Raised when
DestructiveChangesError	REMOVE operations present and allow_destructive=False (checked before the transaction opens)
CircularDependencyError	Cross-COT object-field references form a cycle among new COTs
UnknownChoiceSetError	A choice_set name cannot be resolved to a CustomFieldChoiceSet
UnknownObjectTypeError	A related_object_type string cannot be resolved to an ObjectType

Tests — test_executor.py (50 tests, 11 classes)

Class	Count	What it covers
ExecutorBuildDepOrderTestCase	9	_build_dep_order logic without DB: empty list, single diff, chain ordering, circular detection, self-reference, built-in ROT ignored
ExecutorDestructiveGuardTestCase	4	allow_destructive flag, DestructiveChangesError.diffs attribute, field survives rejected apply
ExecutorNewCOTTestCase	8	COT created in DB with correct attrs, fields created, next_schema_id synced, schema_document persisted
ExecutorCOTAttrsTestCase	4	Description, version, verbose_name updated; no-op leaves COT unchanged
ExecutorFieldAddTestCase	7	ADD with all field types; choice_set / ObjectType resolution; UnknownChoiceSetError / UnknownObjectTypeError; next_schema_id update
ExecutorFieldAlterTestCase	5	ALTER description, weight, required; rename; validation_minimum
ExecutorFieldRemoveTestCase	3	Field removed from DB; surviving fields unaffected; tombstone persisted in schema_document
ExecutorSchemaDocumentTestCase	3	schema_document set after no-op, contains correct slug and schema_version
ExecutorDependencyTestCase	3	Two new COTs with cross-reference succeed in both document orderings; object field resolves correctly
ExecutorAtomicityTestCase	2	Failed ADD rolls back new COT; failed mid-apply leaves field count unchanged
ExecutorApplyDiffsTestCase	2	apply_diffs low-level interface works; raises DestructiveChangesError correctly

[bctiemann]

@claude /review

[claude]

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

---

Code Review — PR #453: Upgrade Executor Tool

• Gather context: read changed files and diff
• Analyze executor implementation
• Analyze test coverage
• Post review feedback

---

Overview

This PR implements a well-structured two-phase upgrade executor for COT schemas. The design is sound: Phase 1 creates/updates COT records (establishing backing DB tables), Phase 2 applies all field operations — this neatly avoids FK-ordering problems. The public API is clean, the exception hierarchy is well-typed, and the test suite is thorough.

---

Issues

1. KeyError for unknown field types — executor.py:197

schema_type = schema_def["type"]
type_choice = SCHEMA_TYPE_TO_CHOICES[schema_type]  # bare dict lookup

If the schema contains an unknown type string, this raises a bare KeyError rather than a descriptive error. It would be cleaner to raise a ValueError or a new UnknownFieldTypeError with the offending value. There's no test for this path.

Fix this →

---

2. Silent REMOVE skip in _phase2_fields — executor.py:442-445

elif fc.op is FieldOp.REMOVE:
    # allow_destructive=False would have raised before we got here;
    # the conditional is a defensive belt-and-suspenders check.
    if allow_destructive:
        _apply_field_remove(cot, fc)

The comment is accurate when called through apply_diffs, but _phase2_fields is a module-level function. If someone calls it directly without going through the pre-flight guard (e.g., from a future test or internal use), REMOVE operations are silently swallowed rather than raising. Replacing the comment with an assert allow_destructive would make the assumption explicit and catch misuse immediately.

---

3. _update_schema_document fires for every COT including no-ops — executor.py:491-494

for diff in ordered:
    cot = cot_map[diff.slug]
    _update_schema_document(cot, type_defs_by_slug[diff.slug])
    _sync_next_schema_id(cot, diff)

For a document with 20 COTs where only 1 has changed, _update_schema_document still writes to every COT row. Since _sync_next_schema_id already has a conditional update (next_schema_id__lt=max_assigned), the same pattern could be applied to the schema doc update — only write it if it actually changed. Low-priority as these are QuerySet.update() calls, but worth noting at scale.

---

4. apply_diffs docstring references old module path — executor.py:461,464

:class:`~netbox_custom_objects.comparator.COTDiff`
...
:func:`~netbox_custom_objects.comparator.diff_document`

After the rename to netbox_custom_objects.schema.comparator, these cross-references are stale. They should read ~netbox_custom_objects.schema.comparator.COTDiff etc.

Fix this →

---

5. _apply_field_alter calls full .save() without update_fields — executor.py:309

field.save()

Unlike _phase1_cots which correctly uses cot.save(update_fields=...), _apply_field_alter saves the whole field instance. For non-DDL attrs (description, weight, etc.), this is safe but writes every field column. If CustomObjectTypeField.save() can be passed update_fields for pure-metadata changes (i.e., when name and type are not in changed_attrs), this would be a minor win. If save() always needs to run DDL hooks regardless, this is a non-issue.

---

Minor Observations

• _resolve_related_object_type makes two sequential DB queries for custom-objects refs (fetch COT → fetch ObjectType). This is unavoidable given how the model name is encoded, but a comment noting the 2-query cost would be helpful for future readers.

• Test imports a private function directly (from ... import _build_dep_order in test line 29). This is intentional and appropriate for thorough unit testing — just worth keeping in mind that it's an internal test seam.

• _ExecutorTestBase MRO (TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase) — the ordering of TransactionCleanupMixin before TransactionTestCase ensures tearDown calls the cleanup mixin first (good), and the mixin's super().tearDown() eventually reaches TransactionTestCase.tearDown(). This pattern is correct.

• test_no_dependencies_preserves_any_valid_order (test line 72) doesn't assert any specific order — it only checks that all three slugs are in the result. This is correct (topological sort allows any valid ordering when there are no deps), but adding a comment makes the intent explicit.

---

Strengths

• The two-phase design is well-reasoned and cleanly separates concerns.
• Topological sort is DFS-based with sorted(deps) for determinism — solid.
• Lazy imports with # noqa: PLC0415 everywhere are the right pattern for this plugin's circular-import sensitivity.
• Atomicity tests using TransactionTestCase (not TestCase) are correct — DDL-rollback tests can't work inside a regular test transaction.
• _sync_next_schema_id with conditional QuerySet.update() is an elegant touch.
• Self-reference exclusion from cycle detection (dep_slug != diff.slug) is a subtle correctness detail that's correctly tested.

---

Overall: Ready to merge with the stale docstring paths fixed (issue 4). The KeyError path (issue 1) and silent REMOVE skip (issue 2) are worth addressing but aren't blockers.

[bctiemann]

Issues 1-4 are addressed; 5 is not changed:

For the executor PR, the honest answer is: this is a non-issue. field.save() must be called unconditionally because save() always invokes schema_editor.alter_field()
regardless. Passing update_fields to save() would only restrict which columns Django writes to the row — it would not suppress the DDL call, so no correctness
improvement is possible at the executor layer.

I'd leave this as a comment on the PR rather than making a code change:

  ▎ _apply_field_alter can't use update_fields to skip the DDL. CustomObjectTypeField.save() calls schema_editor.alter_field() unconditionally for existing instances  
  ▎ (models.py:1792), regardless of which attrs changed. Suppressing the column writes via update_fields wouldn't help. The real optimization would be to gate the 
  ▎ alter_field call inside save() on whether DDL-relevant attrs actually differ — but that's a separate models.py refactor, not an executor concern.                  

[arthanson]

Blocked by #449