Closes: #391 (Phase 1) - Snapshot mixin base columns into schema_document

[bctiemann]

Summary

• Adds CustomObjectType._collect_base_columns() -- static method that returns the concrete DB columns contributed by the CustomObject mixin hierarchy (e.g. id, created, last_updated), excluding user-defined field names.
• Adds CustomObjectType._store_base_column_snapshot() -- writes those columns to schema_document["base_columns"] immediately after a COT table is created, preserving any other existing keys in schema_document (e.g. the schema-exporter output from Define a schema format for portable Custom Object Type definitions #386).
• Hooks _store_base_column_snapshot() into create_model() so every new COT records its base column state at birth.
• Adds migration 0009_backfill_base_columns that uses DB introspection to populate schema_document["base_columns"] for all existing COTs, subtracting user-defined field names to isolate the base columns.

Context

This is Phase 1 of the fix for #391 ("Automatically handle Custom Object Type schema upgrades created through changes to core mixins"). The snapshot recorded here will be consumed by the Phase 2 post_migrate auto-heal handler, which will diff the stored snapshot against the current CustomObject model definition and apply any new columns to existing COT tables when NetBox is upgraded.

See the strategy discussion on #391 for the full design.

Migration note

392-validate-semver-fields (PR #455) has a 0009_alter_customobjecttype_version migration that will conflict with our 0009_backfill_base_columns. That PR will need its migration renumbered to 0010 before it merges into the updated 386-schema-export.

Test plan

• CollectBaseColumnsTestCase -- _collect_base_columns() returns id/created/last_updated, excludes user-defined field names, and produces entries with name/field_class/null keys
• StoreBaseColumnSnapshotTestCase -- snapshot is written to schema_document["base_columns"], existing document keys are preserved, in-memory attribute is updated
• CreateModelSnapshotTestCase -- a freshly created COT has base_columns in schema_document with the expected mandatory columns
• BackfillBaseColumnsTestCase -- migration function populates existing COTs, excludes user fields, is idempotent, skips already-backfilled rows, handles multiple COTs, and produces entries with required keys

Generated with Claude Code

[bctiemann]

@claude /review

[claude]

Claude finished @bctiemann's task in 1m 55s —— View job

---

PR Review: Closes #391 (Phase 1) - Snapshot base columns into schema_document

• Read changed files
• Analyze code quality and correctness
• Review tests
• Post review feedback

---

Overview

This PR adds Phase 1 of the auto-heal schema upgrade mechanism: at COT birth, snapshot the mixin-contributed DB columns into schema_document["base_columns"]. This enables Phase 2 to diff the snapshot against the current mixin definition and heal diverged tables. The implementation is clean and well-documented.

---

Code Quality

_collect_base_columns (models.py:456)

Solid implementation. Using model._meta.concrete_fields correctly captures all concrete columns. Filtering by user_field_names is clean. No issues here.

One minor note: the output order is determined by Django's internal field ordering (typically declaration order). The backfill migration, however, sorts by column name (sorted(col_rows, key=lambda c: c.name) in the migration). This creates an ordering inconsistency between new snapshots and backfilled ones. Phase 2 should compare as sets, but it's worth aligning the two paths now for debuggability:

# Suggestion: sort in _collect_base_columns to match backfill ordering
return sorted([
    {"name": f.name, "field_class": f.__class__.__name__, "null": f.null}
    for f in model._meta.concrete_fields
    if f.name not in user_field_names
], key=lambda e: e["name"])

Fix this →

---

_store_base_column_snapshot (models.py:480)

The preservation of pre-existing schema_document keys is correct. The in-memory update (self.schema_document = doc) is a nice touch.

Minor concern: when called during create_model(), a fresh self.fields.values_list(...) query is issued. At COT birth there are no user fields yet, so the query will always return an empty set. This is correct behavior, but it's a no-op DB round-trip. A doc comment clarifying this expectation ("at birth, user_field_names will always be empty; this call is for correctness if ever invoked post-birth") would help future readers.

---

Migration 0009_backfill_base_columns

Overall well-designed. A few observations:

1. Potential missing field_class key in backfill entries: When a DB column exists but is not in base_field_info (built from CustomObject._meta.get_fields()), the entry is written without field_class:

   entry = {"name": col.name, "null": bool(col.null_ok)}
   if col.name in base_field_info:
       entry["field_class"] = base_field_info[col.name]["field_class"]

   The forward path (_collect_base_columns) always includes field_class because it reads from the live model's concrete_fields. Backfilled COTs could end up with entries that lack field_class, which would be a schema inconsistency when Phase 2 consumes this data. Consider raising a warning or providing a fallback class name (e.g., "UnknownField") so the key is always present.

2. Hardcoded "AutoField" for id: The migration explicitly adds "id" as AutoField since it comes from models.Model, not CustomObject._meta. In Django 3.2+, DEFAULT_AUTO_FIELD defaults to BigAutoField, and NetBox may configure this differently. The actual class could be BigAutoField rather than AutoField. The test at line 90 uses assertIn("Field", id_entry["field_class"]) which is deliberately loose to handle this, but the backfill hardcodes "AutoField", creating another forward/backfill inconsistency for the id field. Consider using:

   base_field_info.setdefault(
       "id",
       {"field_class": CustomObject._meta.pk.__class__.__name__ if CustomObject._meta.pk else "AutoField", "null": False}
   )

   Fix this →

3. Table name assumption: table_name = f"custom_objects_{cot.id}" — this relies on the same naming convention used in get_database_table_name() (USER_TABLE_DATABASE_NAME_PREFIX + id). If that prefix ever changes, the migration could silently skip all rows. Consider importing the constant directly rather than duplicating the pattern:

   from netbox_custom_objects.models import USER_TABLE_DATABASE_NAME_PREFIX
   table_name = f"{USER_TABLE_DATABASE_NAME_PREFIX}{cot.id}"

   Fix this →

---

Tests

The test suite (test_base_columns.py) is comprehensive and well-structured. Coverage of the key behaviors:

• _collect_base_columns: mandatory columns present, user fields excluded, required keys, correct id class ✓
• _store_base_column_snapshot: write, key preservation, in-memory update, user field exclusion ✓
• create_model integration: snapshot present, mandatory columns, user field exclusion ✓
• Backfill: populate, mandatory columns, exclude user fields, idempotency, skip existing, multiple COTs, required keys ✓

One note on test_backfill_each_entry_has_required_keys (line 329): it only checks for name and null, not field_class. This is likely intentional (since field_class may be absent for unknown columns as noted above), but if the convention changes to always require field_class, this test should be updated too.

test_schema_format.py changes (lines 366-376): The renamed test test_schema_document_populated_with_base_columns_on_creation is a clear improvement over the old test_schema_document_defaults_to_null — the behavior changed and the test name now accurately reflects it. Good.

---

Summary

The implementation is solid and the design rationale is well-documented. The three main concerns to address before merge:

Priority	Issue	Location
Medium	Sorting inconsistency between _collect_base_columns and backfill	models.py:470, migration:86
Medium	Backfill entries may be missing field_class for unknown columns	migration:88-95
Low	id field class hardcoded as "AutoField" in backfill	migration:59
Low	Table name prefix duplicated instead of using USER_TABLE_DATABASE_NAME_PREFIX	migration:66

[bctiemann]

Addressed all review issues.

[arthanson]

merge feature and request merge to feature not 3886-schema-export