Closes: #637 - Fix model-registry errors when get_model() runs outside ready()'s two-pass loop

[bctiemann]

Closes: #637

Summary

A COT with a cross-COT Object-type field, when .get_model() is called directly on the model instance (the pattern shown in docs/index.md -- a module-level CustomObjectType.objects.get(...).get_model()) while it's unsafe to touch the DB (e.g. during manage.py migrate), left the field's LazyForeignKey target unresolved. Django's system checks then raised fields.E300/fields.E307 for that field, aborting the upgrade.

Root cause: every other dynamic-model entry point (ready(), PluginConfig.get_model()/get_models()) already guards against generating COT models before migrations have completed. CustomObjectType.get_model() -- the one method third-party plugins call directly -- didn't check this at all, so it could register a COT model outside of ready()'s two-pass cross-COT FK resolution, leaving a dangling reference that nothing else ever re-resolves during that migrate process.

Changes

• CustomObjectType.get_model() now checks a new _dynamic_model_creation_unsafe() predicate and forces skip_object_fields=True when it's set, omitting the cross-COT field entirely rather than leaving a dangling lazy reference. The resulting degraded model is never cached (existing behavior for skip_object_fields=True), so a real request after startup regenerates it in full via ready()'s normal two-pass path in the actual (separate) worker process.
• _dynamic_model_creation_unsafe() is the actual DB-readiness check (mid-migration, migrate/makemigrations/collectstatic, or this app's own migrations incomplete) -- deliberately excluding "test", since get_model() is the method tests use directly and the test database is always fully migrated by the time a test body runs.
• should_skip_dynamic_model_creation() -- used by ready(), PluginConfig.get_model()/get_models(), and navigation -- keeps its existing public signature and behavior, now expressed as _dynamic_model_creation_unsafe() OR "test" in sys.argv.

Testing

• New CrossCOTGetModelOutsideReadyTestCase in test_models.py:
  • test_get_model_omits_cross_cot_field_when_unsafe -- the field is omitted, and the degraded model isn't cached.
  • test_get_model_system_checks_pass_after_migrate_time_call -- reproduces the exact fields.E300/E307 symptom and confirms it's gone.
  • test_get_model_regenerates_in_full_on_next_normal_call -- once the target COT is registered again (mirroring a real restart's ready() pass), a subsequent call fully resolves the field.
  • test_get_model_unaffected_during_ordinary_test_run -- ordinary test-suite get_model() calls stay fully hydrated.
• New test_dynamic_model_creation_unsafe_excludes_test in PluginConfigGetModelTestCase.
• Verified locally against netbox-community/netbox main (Django 6.0.7) and feature (Django 6.1), plus the broader test_models/test_api/test_filtersets/test_polymorphic_fields/test_field_types/test_views suites.

[bctiemann]

@claude /review and limit response to actionable issues.

[github-actions]

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

---

PR #664 Review

• Gathering context
• Reading changed files
• Posting review

---

Overview

The fix is correct and well-motivated. Splitting should_skip_dynamic_model_creation() into _dynamic_model_creation_unsafe() (excludes "test") and the original public wrapper (adds "test" back) is a clean separation of concerns, and forcing skip_object_fields=True in get_model() during the unsafe window is the right way to prevent dangling LazyForeignKey references.

---

Actionable Issues

1. assertRaises(Exception) is too broad — test_models.py:1838

with self.assertRaises(Exception):
    model._meta.get_field('forwarder_profile')

_meta.get_field() raises django.core.exceptions.FieldDoesNotExist on a missing field. Using bare Exception hides whether the right thing failed — the test would pass even if the call raises e.g. AttributeError from unrelated model corruption. Use FieldDoesNotExist explicitly.

Fix this →

---

2. _dynamic_model_creation_unsafe() check fires before the cache lookup — models.py:1673

if apps.get_app_config(APP_LABEL)._dynamic_model_creation_unsafe():
    skip_object_fields = True

branch_id = self._active_branch_id()
with self._global_lock:
    if self.is_model_cached(...) and not no_cache:
        ...
        return model   # ← returns cached full model regardless

When there's a cache hit, skip_object_fields is computed but never used — the cached model is returned unchanged. This is harmless (and correct), but computing the unsafe-check for every cache-hit call adds a minor overhead (ContextVar + sys.argv scan + global check) on the hot path. Consider moving the check inside the cache-miss branch, just before model generation:

# inside the cache-miss path, before calling generate_model()
if apps.get_app_config(APP_LABEL)._dynamic_model_creation_unsafe():
    skip_object_fields = True

Not a blocker, but worth the minor restructure for clarity. Fix this →

---

3. Minor: PR description mentions include_test_skip parameter that doesn't exist

The description says:

should_skip_dynamic_model_creation() gains an include_test_skip parameter

The actual implementation uses two separate functions instead. The PR description is stale — not a code issue but may confuse future git log readers.

---

Summary

The core logic is sound. Issue 1 is worth fixing before merge. Issue 2 is a small optimization. Issue 3 is cosmetic.

[bctiemann]

Addressed items 1 and 2.

[arthanson]

From Claude:

The PR's guard drops all Object/Multi-object fields when it fires, but only COT-targeting non-polymorphic ones can dangle.

Consequence: migration 0003_ensure_fk_constraints calls get_model() inside a migrate process, so it now finds no object fields and creates no FK constraints - silently (one logger.error). Core-model targets (dcim.Site etc.) worked there before; nothing downstream backfills a missing constraint, so affected COTs lose DB-level on_delete_behavior enforcement. Reachable on installs still at plugin ≤0.4.0 that have COTs.

Fix: gate on "target is another COT" instead of "is an object field"

Secondary: the degradation isn't logged, and _migrations_checked caches True for the process lifetime, so a worker started mid-upgrade serves field-less models until restart.

Here is what is came up for as a fix:

One file for the behavior: netbox_custom_objects/models.py

1. A new predicate on CustomObjectTypeField (next to related_object_type_label, which already open-codes this pattern at models.py:2605, as does :3049):

      @property
      def targets_custom_object_type(self):
          """
          True when this field points at another COT's dynamic model, i.e. its FK is a
          LazyForeignKey that only resolves once the target is in the app registry.
          Polymorphic fields use a GFK and need no resolution, so they are excluded.
          """
          if self.is_polymorphic or not self.related_object_type_id:
              return False
          try:
              object_type = ContentType.objects.get_for_id(self.related_object_type_id)
          except ContentType.DoesNotExist:
              return False
          return (
              object_type.app_label == APP_LABEL
              and extract_cot_id_from_model_name(object_type.model) is not None
          )

ContentType and extract_cot_id_from_model_name are already imported there. get_for_id() is the process-cached lookup, so this doesn't add a query per field the way _get_related_content_type() (field_types.py:293) would. The extract_cot_id_from_model_name half matters: an Object field targeting netbox_custom_objects.customobjecttype shares the app label but isn't a dynamic model, and field_types.py:780 treats it differently.

2. _fetch_and_generate_field_attrs() gains a second, narrower mode - the existing skip_object_fields=True semantics are untouched, because field_types.py:1446 and views.py:922 rely on them to build stub models that break FK recursion:

           for field in fields:
  -            if skip_object_fields:
  -                if field.type in [CustomFieldTypeChoices.TYPE_OBJECT, CustomFieldTypeChoices.TYPE_MULTIOBJECT]:
  +            if field.type in [CustomFieldTypeChoices.TYPE_OBJECT, CustomFieldTypeChoices.TYPE_MULTIOBJECT]:
  +                if skip_object_fields:
  +                    continue
  +                # Only a COT target produces a LazyForeignKey that needs ready()'s
  +                # second pass; core-model targets and polymorphic GFKs resolve on
  +                # their own, so they stay even in the unsafe window (#637).
  +                if skip_cot_object_fields and field.targets_custom_object_type:
                       continue

3. get_model() sets the narrow flag instead of overwriting the caller's - three small hunks: the guard becomes skip_cot_object_fields = apps.get_app_config(APP_LABEL)._dynamic_model_creation_unsafe(), that flag is passed through to _fetch_and_generate_field_attrs(), and the cache condition becomes if not (skip_object_fields or skip_cot_object_fields) so a narrowed model still never gets cached.