Skip to content

Fixes #406: Resolve correct to_field_name for CSV/YAML/JSON bulk import#557

Merged
arthanson merged 3 commits into
mainfrom
406-csv-import-to-field-name
Jun 5, 2026
Merged

Fixes #406: Resolve correct to_field_name for CSV/YAML/JSON bulk import#557
arthanson merged 3 commits into
mainfrom
406-csv-import-to-field-name

Conversation

@bctiemann
Copy link
Copy Markdown
Contributor

@bctiemann bctiemann commented Jun 5, 2026

Fixes: #406

Summary

Bulk import of Object and MultiObject fields referencing models without a name field (e.g. dcim.ModuleType, which uses model as its identifier) failed with "invalid accessor field name" because to_field_name was hardcoded to fall back to 'name'.

Root Cause

In ObjectFieldType.get_form_field() and MultiObjectFieldType.get_form_field(), the CSV import path used:

to_field_name = getattr(field, 'to_field_name', None) or 'name'

ModuleType has no name field — it uses model — so CSVModelChoiceField received an invalid accessor and raised FieldError, surfaced to the user as "invalid accessor field name".

NetBox core's own bulk import form for ModuleType explicitly sets to_field_name='model', confirming 'name' is not a universal default.

Fix

Add _csv_import_to_field_name(model_class, explicit=None) which probes the target model's fields in priority order ('name', 'slug', 'model', 'identifier') and returns the first match, falling back to 'pk'. Both get_form_field() call sites now use this helper.

With this fix, importing a CO instance that references a ModuleType works as expected:

---
testname: my-component
module_type: CPU567

Tests

  • test_csv_import_field_uses_name_for_models_with_nameDevice (has name) → 'name'
  • test_csv_import_field_uses_model_for_module_typeModuleType (no name) → 'model'
  • test_csv_import_form_field_for_module_type_uses_model_field — end-to-end: get_form_field(for_csv_import=True) on a ModuleType COTF produces to_field_name='model'

🤖 Generated with Claude Code

bctiemann and others added 2 commits June 5, 2026 07:23
…odel fields

Bulk import for Object/MultiObject fields referencing models without a
'name' field (e.g. dcim.ModuleType, which uses 'model') failed with
'invalid accessor field name' because to_field_name was hardcoded to
fall back to 'name'.

Add _csv_import_to_field_name() which probes the target model's fields
in order ('name', 'slug', 'model', 'identifier') and uses the first
match, falling back to 'pk'. This mirrors how NetBox core handles
ModuleType in its own bulk import forms (explicit to_field_name='model').

Both ObjectFieldType and MultiObjectFieldType get_form_field() now use
this helper instead of the hardcoded 'name' fallback.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- except Exception -> except FieldDoesNotExist (already imported at top)
- if explicit: -> if explicit is not None: to handle empty-string correctly
- Add logger.warning() when falling back to 'pk' (user-unfriendly edge case)
- Hoist inline test imports to module level (dcim.models, _csv_import_to_field_name)
- Remove now-redundant inline imports from test methods
- Add test_csv_import_field_uses_slug_before_model_for_device_type to
  document that DeviceType resolves to 'slug' before 'model', matching
  NetBox core behaviour
- Add MultiObjectFieldType.test_csv_import_form_field_for_module_type_uses_model_field
  to cover the multi-object import path

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bctiemann
Copy link
Copy Markdown
Contributor Author

@claude /review and limit response to areas of potential concern.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

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


Review: Areas of Potential Concern

  • Read changed files
  • Analyze implementation
  • Post review

1. Field existence vs. uniqueness (field_types.py:64–68)

_csv_import_to_field_name probes for field presence only — it does not verify the selected field is unique. CSVModelChoiceField requires the to_field_name to identify exactly one record; if it matches a field that is only contextually unique (scoped to a parent, for example), imports will raise MultipleObjectsReturned at runtime with no indication that the wrong to_field_name was chosen. The current candidates (name, slug, model, identifier) are globally unique on all common NetBox models, so the practical risk is low, but a future addition to _CSV_IDENTIFIER_FIELD_PRECEDENCE could silently introduce this bug. Consider adding a uniqueness check:

field_obj = model_class._meta.get_field(candidate)
if getattr(field_obj, 'unique', False):
    return candidate

Fix this →


2. explicit override is not validated (field_types.py:62–63)

When explicit is non-None it is returned immediately without checking that the field actually exists on model_class. If a stored to_field_name on a CustomObjectTypeField is stale (e.g. the referenced model was updated and the field was renamed), this silently passes an invalid accessor to CSVModelChoiceField, which will then raise a FieldError at import time — the same user-visible error the PR is trying to fix, just from a different path. A one-line guard would close this:

if explicit is not None:
    try:
        model_class._meta.get_field(explicit)
        return explicit
    except FieldDoesNotExist:
        logger.warning('Stored to_field_name %r not found on %s; probing.', explicit, model_class.__name__)

Fix this →


3. Test gaps (tests/test_field_types.py)

Two paths in _csv_import_to_field_name are not covered by the new tests:

  • explicit= path: There is no test asserting that a non-None explicit value is returned directly (bypassing the probe loop). This is the primary path for any CustomObjectTypeField that has a stored to_field_name.
  • pk fallback path: No test for the warning + 'pk' return when no candidate field matches. This is reachable with a mock model.

4. PR title says "CSV/YAML/JSON" — only CSV form field is affected

The fix is in get_form_field(for_csv_import=True), which covers NetBox's unified bulk import form (used for CSV, YAML, and JSON). That's correct — the title is accurate. Just confirming this isn't three separate code paths being claimed as fixed.


Overall: The core fix is correct and clearly targeted. Concerns 1 and 2 are worth addressing before merge; concern 3 is a test quality improvement.

… gaps

Concern 1 (uniqueness): the probe loop now checks getattr(field_obj,
'unique', False) and returns on the first unique match. Non-unique
candidates are kept as a secondary fallback so behaviour is preserved
for edge cases where no globally-unique field exists.

Concern 2 (explicit validation): the explicit path now calls
_meta.get_field() and falls through to the probe loop with a warning if
the stored value is stale (field no longer exists on the model).

Concern 3 (test gaps):
- test_csv_import_to_field_name_explicit_returned_when_valid: non-None
  explicit that exists on the model is returned directly.
- test_csv_import_to_field_name_explicit_falls_through_when_stale: stale
  explicit triggers a warning and falls through to the probe loop.
- test_csv_import_to_field_name_pk_fallback_when_no_candidate_exists:
  mock model with no matching fields triggers warning and returns 'pk'.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bctiemann bctiemann requested review from a team and arthanson and removed request for a team June 5, 2026 12:14
@arthanson arthanson merged commit eccead6 into main Jun 5, 2026
10 checks passed
@arthanson arthanson deleted the 406-csv-import-to-field-name branch June 5, 2026 19:02
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.

module_type: "name" is an invalid accessor field name

2 participants