Skip to content

Closes: #388 - CustomObjectType schema exporter#450

Merged
jnovinger merged 7 commits into386-schema-exportfrom
388-schema-exporter
Apr 8, 2026
Merged

Closes: #388 - CustomObjectType schema exporter#450
jnovinger merged 7 commits into386-schema-exportfrom
388-schema-exporter

Conversation

@bctiemann
Copy link
Copy Markdown
Contributor

@bctiemann bctiemann commented Apr 7, 2026

Closes: #388

Implements the COT schema exporter: a public Python API that serialises one or more CustomObjectType instances to the portable schema document format defined in #386 (PR #449).

Public API — exporter.py

export_cot(cot)   → dict   # single COT definition (inner types[] entry)
export_cots(cots) → dict   # full {schema_version, types} document

Behaviour

  • Default elision — attribute values equal to FIELD_DEFAULTS are omitted from the output, keeping documents minimal. Round-trip safe: the importer re-applies the same defaults on load.
  • Field ordering — fields are emitted in schema_id order for stable diffs.
  • Fields without schema_id — skipped with a WARNING log entry. These are fields created before the schema-format feature was introduced and cannot be tracked portably.
  • Tombstonesremoved_fields entries are read from cot.schema_document (populated by the apply endpoint, Custom Object Type upgrade plan/execute API endpoints #390, PR Closes: #390 - Schema validation/apply endpoints #454). Until apply is wired up this list is always empty in fresh exports.
  • related_object_type encoding — built-in NetBox objects encode as "app_label/model" (e.g. "dcim/device"); Custom Object Types encode as "custom-objects/<slug>" using the COT slug, not its internal table model name.

Data migration — 0006_backfill_schema_ids.py

Assigns sequential schema_id values to existing CustomObjectTypeField rows that have schema_id IS NULL (i.e. created before migration 0005). Also updates next_schema_id on each parent COT so the auto-assign counter is never lower than the highest assigned ID.

The reverse migration is a no-op (IDs are not revoked).

Tests — test_exporter.py (32 tests, 8 classes)

Class What it covers
ExporterBasicTestCase Name, slug, optional attrs omitted when blank
ExporterDefaultElisionTestCase Each FIELD_DEFAULTS entry is elided when equal to default; non-default values are included
ExporterFieldTypesTestCase All 13 field types serialise correctly, including related_object_type encoding for both built-in and COT targets
ExporterDeprecationTestCase deprecated, deprecated_since, scheduled_removal round-trip
ExporterSchemaIdTestCase Fields are ordered by schema_id; fields without schema_id are skipped with a warning
ExporterTombstoneTestCase removed_fields read from schema_document and included in output
ExporterSchemaValidationTestCase Every exported document validates against cot_schema_v1.json (skipped if jsonschema not installed)
SchemaIdBackfillTestCase Data migration assigns IDs sequentially, updates counter, handles gaps after deletion

@bctiemann
Copy link
Copy Markdown
Contributor Author

@claude /review

@claude
Copy link
Copy Markdown

claude bot commented Apr 7, 2026

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


PR Review: CustomObjectType Schema Exporter (#450)

  • Read changed files
  • Analyze code quality and correctness
  • Check test coverage
  • Review migration
  • Post final review

Overview

This PR introduces a clean, well-structured schema exporter for CustomObjectType instances. The implementation is solid overall — good module-level documentation, a minimal public API, proper default-value elision, and extensive test coverage. A few issues are worth addressing before merge.


Issues

1. ExporterSchemaIdTestCase MRO is backwards (functional bug)

tests/test_exporter.py:326-327

class ExporterSchemaIdTestCase(CustomObjectsTestCase, TransactionTestCase,
                               TransactionCleanupMixin):

Compare with the correct ordering used elsewhere:

class SchemaIdAutoAssignmentTestCase(
    TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase
):

With the current MRO in ExporterSchemaIdTestCase, tearDown executes in this order:

  1. CustomObjectsTestCase.tearDown()clears the model cache
  2. TransactionTestCase.tearDown() — Django cleanup
  3. TransactionCleanupMixin.tearDown()then tries to delete COTs

This means the model cache is cleared before COTs are deleted, reversing the correct order. TransactionCleanupMixin must come first so COTs (and their backing tables) are dropped before the model cache is cleared. The current ordering risks leaving backing tables behind when the TransactionTestCase DB flush runs.

Fix this →


2. choice_set.name accessed without a null guard

exporter.py:119

result["choice_set"] = field.choice_set.name

If a select or multiselect field has no choice_set (e.g. corrupted data or an import edge case), this raises an unguarded AttributeError. A clearer error would help:

if field.choice_set is None:
    raise ValueError(
        f"Field {field.name!r} is type {schema_type!r} but has no choice_set assigned."
    )
result["choice_set"] = field.choice_set.name

3. N+1 DB queries in _encode_related_object_type

exporter.py:83

slug = CustomObjectType.objects.values_list('slug', flat=True).get(pk=cot_id)

This fires one DB query per field that references a custom COT. For an export of many COTs with cross-references this adds up. The caller (export_cot) doesn't prefetch related COTs. This is low-risk for typical usage but worth noting — a simple {id: slug} lookup dict built once in export_cots would eliminate the N+1.


4. _removed_fields_from_document matches by COT name — fragile on rename

exporter.py:146

for type_def in cot.schema_document.get("types", []):
    if type_def.get("name") == cot.name:
        return list(type_def.get("removed_fields", []))

If a COT is renamed after tombstones are written into schema_document, those tombstones will be silently lost on next export. This is a known limitation (the apply endpoint (#390) isn't wired up yet), but it's worth a comment here:

# NOTE: matches by COT name. If the COT is renamed after tombstones
# are persisted, they will not be found. This will be addressed when
# #390 (apply) is implemented and tombstones are managed more explicitly.

5. export_cots([]) produces a schema-invalid document

exporter.py:212-215

return {
    "schema_version": SCHEMA_FORMAT_VERSION,
    "types": [export_cot(cot) for cot in cots],
}

An empty cots iterable produces {"schema_version": "1", "types": []}, which fails the JSON Schema's minItems: 1 constraint on types. This is a minor edge case but calling export_cots([]) would create an invalid document that silently passes without a validator. A guard or at least a docstring note would be helpful.


Minor Observations

  • _BASE_ATTRS tuple (exporter.py:45-62): Could equally live in schema_format.py alongside FIELD_TYPE_ATTRS and FIELD_DEFAULTS to keep all schema-format constants co-located. Not a blocking issue.
  • Migration N+1: assign_schema_ids does one update() call per NULL field. For a one-time backfill this is fine, but a bulk_update() would be cleaner.
  • Test coverage: Excellent — default elision, field types, deprecation lifecycle, tombstones, schema validation, migration idempotency, and scoping are all covered. The FieldDefaultsConsistencyTestCase is particularly valuable as a regression guard.

Summary

The core exporter logic is correct and well-tested. The MRO issue in ExporterSchemaIdTestCase is the only functional bug — the rest are defensive hardening and documentation suggestions. Recommend fixing issues 1–2 before merge; 3–5 can be tracked as follow-ups.

@bctiemann
Copy link
Copy Markdown
Contributor Author

Fixed 1, 2, 4, 5.

@bctiemann bctiemann requested review from a team and jnovinger and removed request for a team April 7, 2026 13:10
@jnovinger jnovinger merged commit 921ccd4 into 386-schema-export Apr 8, 2026
2 checks passed
@jnovinger jnovinger deleted the 388-schema-exporter branch April 8, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants