From f68c6b2d1cf8699786769ca7c67edff917609f0b Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 5 Jun 2026 07:23:45 -0400 Subject: [PATCH 1/3] Fixes #406: Resolve correct to_field_name for CSV import by probing model 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 --- netbox_custom_objects/field_types.py | 31 ++++++++++++++++--- .../tests/test_field_types.py | 31 +++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/netbox_custom_objects/field_types.py b/netbox_custom_objects/field_types.py index 626784c9..72158853 100644 --- a/netbox_custom_objects/field_types.py +++ b/netbox_custom_objects/field_types.py @@ -44,6 +44,30 @@ # PostgreSQL's hard limit for identifier names is 63 bytes. _PG_MAX_IDENTIFIER_LEN = 63 +# Ordered list of field names tried when resolving the natural text identifier for a +# model during CSV/YAML/JSON bulk import. The first name that exists on the model +# wins. This handles models like ModuleType that use 'model' rather than 'name'. +_CSV_IDENTIFIER_FIELD_PRECEDENCE = ('name', 'slug', 'model', 'identifier') + + +def _csv_import_to_field_name(model_class, explicit=None): + """Return the field name to use as ``to_field_name`` for CSV import lookups. + + If *explicit* is provided (e.g. from a stored field configuration), it is + returned directly. Otherwise the target model's fields are probed in + ``_CSV_IDENTIFIER_FIELD_PRECEDENCE`` order and the first match is used. + Falls back to ``'pk'`` when none of the candidates exist. + """ + if explicit: + return explicit + for candidate in _CSV_IDENTIFIER_FIELD_PRECEDENCE: + try: + model_class._meta.get_field(candidate) + return candidate + except Exception: + pass + return 'pk' + def _safe_pg_identifier(full_name: str) -> str: """ @@ -721,12 +745,10 @@ def get_form_field(self, field, for_csv_import=False, **kwargs): if for_csv_import: field_class = CSVModelChoiceField - # For CSV import, determine to_field_name from the field configuration - to_field_name = getattr(field, 'to_field_name', None) or 'name' + to_field_name = _csv_import_to_field_name(model, explicit=getattr(field, 'to_field_name', None)) return field_class( queryset=model.objects.all(), required=field.required, - # Remove initial=field.default to allow Django to handle instance data properly to_field_name=to_field_name, ) else: @@ -1239,8 +1261,7 @@ def get_form_field(self, field, for_csv_import=False, **kwargs): if for_csv_import: field_class = CSVModelMultipleChoiceField - # For CSV import, determine to_field_name from the field configuration - to_field_name = getattr(field, 'to_field_name', None) or 'name' + to_field_name = _csv_import_to_field_name(model, explicit=getattr(field, 'to_field_name', None)) return field_class( queryset=model.objects.all(), required=field.required, diff --git a/netbox_custom_objects/tests/test_field_types.py b/netbox_custom_objects/tests/test_field_types.py index d4f76f86..3464e60e 100644 --- a/netbox_custom_objects/tests/test_field_types.py +++ b/netbox_custom_objects/tests/test_field_types.py @@ -802,6 +802,37 @@ def test_object_field_model_generation(self): instance = model.objects.create(name="Test", device=device) self.assertEqual(instance.device, device) + def test_csv_import_field_uses_name_for_models_with_name(self): + """get_form_field(for_csv_import=True) uses 'name' as to_field_name for models + that have a name field (e.g. Device).""" + from dcim.models import Device + from netbox_custom_objects.field_types import ObjectFieldType, _csv_import_to_field_name + self.assertEqual(_csv_import_to_field_name(Device), 'name') + + def test_csv_import_field_uses_model_for_module_type(self): + """get_form_field(for_csv_import=True) uses 'model' as to_field_name for + ModuleType, which has no 'name' field — regression for issue #406.""" + from dcim.models import ModuleType + from netbox_custom_objects.field_types import ObjectFieldType, _csv_import_to_field_name + self.assertEqual(_csv_import_to_field_name(ModuleType), 'model') + + def test_csv_import_form_field_for_module_type_uses_model_field(self): + """get_form_field(for_csv_import=True) on an Object field referencing ModuleType + produces a CSVModelChoiceField with to_field_name='model', not 'name'.""" + from core.models import ObjectType + from netbox_custom_objects.field_types import ObjectFieldType + module_type_ot = ObjectType.objects.get(app_label='dcim', model='moduletype') + cotf = self.create_custom_object_type_field( + self.custom_object_type, + name="module_type", + label="Module Type", + type="object", + related_object_type=module_type_ot, + ) + form_field = ObjectFieldType().get_form_field(cotf, for_csv_import=True) + self.assertEqual(form_field.to_field_name, 'model', + "ModuleType CSV import must use 'model', not 'name'") + class MultiObjectFieldTypeTestCase(FieldTypeTestCase): """Test cases for multiobject field type.""" From 80bf3d02e584e17b5ddf51cabb132daa15ed90ca Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 5 Jun 2026 07:31:47 -0400 Subject: [PATCH 2/3] Address review feedback on _csv_import_to_field_name - 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 --- netbox_custom_objects/field_types.py | 13 +++++++-- .../tests/test_field_types.py | 28 +++++++++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/netbox_custom_objects/field_types.py b/netbox_custom_objects/field_types.py index 72158853..ae1b146f 100644 --- a/netbox_custom_objects/field_types.py +++ b/netbox_custom_objects/field_types.py @@ -56,16 +56,23 @@ def _csv_import_to_field_name(model_class, explicit=None): If *explicit* is provided (e.g. from a stored field configuration), it is returned directly. Otherwise the target model's fields are probed in ``_CSV_IDENTIFIER_FIELD_PRECEDENCE`` order and the first match is used. - Falls back to ``'pk'`` when none of the candidates exist. + Falls back to ``'pk'`` when none of the candidates exist (rare; emits a + warning because importing by PK is user-unfriendly). """ - if explicit: + if explicit is not None: return explicit for candidate in _CSV_IDENTIFIER_FIELD_PRECEDENCE: try: model_class._meta.get_field(candidate) return candidate - except Exception: + except FieldDoesNotExist: pass + logger.warning( + 'No natural identifier field found on %s for CSV import ' + '(tried %s); falling back to pk.', + model_class.__name__, + ', '.join(_CSV_IDENTIFIER_FIELD_PRECEDENCE), + ) return 'pk' diff --git a/netbox_custom_objects/tests/test_field_types.py b/netbox_custom_objects/tests/test_field_types.py index 3464e60e..a2906db2 100644 --- a/netbox_custom_objects/tests/test_field_types.py +++ b/netbox_custom_objects/tests/test_field_types.py @@ -9,11 +9,13 @@ from django.test import TestCase from core.models import ObjectType +from dcim.models import Device, DeviceType, ModuleType from netbox_custom_objects.field_types import ( MultiObjectFieldType, MultiSelectFieldType, ObjectFieldType, SelectFieldType, + _csv_import_to_field_name, ) from netbox_custom_objects.models import CustomObjectType, CustomObjectTypeField from .base import CustomObjectsTestCase @@ -805,22 +807,21 @@ def test_object_field_model_generation(self): def test_csv_import_field_uses_name_for_models_with_name(self): """get_form_field(for_csv_import=True) uses 'name' as to_field_name for models that have a name field (e.g. Device).""" - from dcim.models import Device - from netbox_custom_objects.field_types import ObjectFieldType, _csv_import_to_field_name self.assertEqual(_csv_import_to_field_name(Device), 'name') def test_csv_import_field_uses_model_for_module_type(self): """get_form_field(for_csv_import=True) uses 'model' as to_field_name for ModuleType, which has no 'name' field — regression for issue #406.""" - from dcim.models import ModuleType - from netbox_custom_objects.field_types import ObjectFieldType, _csv_import_to_field_name self.assertEqual(_csv_import_to_field_name(ModuleType), 'model') + def test_csv_import_field_uses_slug_before_model_for_device_type(self): + """DeviceType has both 'slug' and 'model'; slug takes precedence per the + _CSV_IDENTIFIER_FIELD_PRECEDENCE ordering, matching NetBox core behaviour.""" + self.assertEqual(_csv_import_to_field_name(DeviceType), 'slug') + def test_csv_import_form_field_for_module_type_uses_model_field(self): """get_form_field(for_csv_import=True) on an Object field referencing ModuleType produces a CSVModelChoiceField with to_field_name='model', not 'name'.""" - from core.models import ObjectType - from netbox_custom_objects.field_types import ObjectFieldType module_type_ot = ObjectType.objects.get(app_label='dcim', model='moduletype') cotf = self.create_custom_object_type_field( self.custom_object_type, @@ -912,6 +913,21 @@ def test_multiobject_field_model_generation(self): self.assertIn(device1, instance.devices.all()) self.assertIn(device2, instance.devices.all()) + def test_csv_import_form_field_for_module_type_uses_model_field(self): + """MultiObjectFieldType.get_form_field(for_csv_import=True) on a field + referencing ModuleType produces to_field_name='model', not 'name'.""" + module_type_ot = ObjectType.objects.get(app_label='dcim', model='moduletype') + cotf = self.create_custom_object_type_field( + self.custom_object_type, + name="module_types", + label="Module Types", + type="multiobject", + related_object_type=module_type_ot, + ) + form_field = MultiObjectFieldType().get_form_field(cotf, for_csv_import=True) + self.assertEqual(form_field.to_field_name, 'model', + "ModuleType multi-object CSV import must use 'model', not 'name'") + class SelfReferentialFieldTestCase(FieldTypeTestCase): """Test cases for self-referential object fields. From 8c106df525bf4396f6dd287c844d174d9b54b448 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 5 Jun 2026 08:01:47 -0400 Subject: [PATCH 3/3] Address follow-up review: uniqueness check, explicit validation, test 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 --- netbox_custom_objects/field_types.py | 35 +++++++++++++++---- .../tests/test_field_types.py | 27 +++++++++++++- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/netbox_custom_objects/field_types.py b/netbox_custom_objects/field_types.py index ae1b146f..e8b298b8 100644 --- a/netbox_custom_objects/field_types.py +++ b/netbox_custom_objects/field_types.py @@ -54,19 +54,40 @@ def _csv_import_to_field_name(model_class, explicit=None): """Return the field name to use as ``to_field_name`` for CSV import lookups. If *explicit* is provided (e.g. from a stored field configuration), it is - returned directly. Otherwise the target model's fields are probed in - ``_CSV_IDENTIFIER_FIELD_PRECEDENCE`` order and the first match is used. - Falls back to ``'pk'`` when none of the candidates exist (rare; emits a - warning because importing by PK is user-unfriendly). + validated against the model and returned if the field exists. If the stored + value is stale (field was renamed on the target model), a warning is logged + and the function falls through to the probe loop. + + The probe loop iterates ``_CSV_IDENTIFIER_FIELD_PRECEDENCE`` and returns the + first candidate that exists **and is unique** on the model, guaranteeing that + ``CSVModelChoiceField`` can resolve a single record. If no unique match is + found the first existing candidate (unique or not) is used as a fallback. + Falls back to ``'pk'`` when none of the candidates exist at all. """ if explicit is not None: - return explicit + try: + model_class._meta.get_field(explicit) + return explicit + except FieldDoesNotExist: + logger.warning( + 'Stored to_field_name %r not found on %s; probing for a natural identifier.', + explicit, model_class.__name__, + ) + + first_match = None # best non-unique candidate, used only if no unique field found for candidate in _CSV_IDENTIFIER_FIELD_PRECEDENCE: try: - model_class._meta.get_field(candidate) - return candidate + field_obj = model_class._meta.get_field(candidate) + if getattr(field_obj, 'unique', False): + return candidate + if first_match is None: + first_match = candidate except FieldDoesNotExist: pass + + if first_match is not None: + return first_match + logger.warning( 'No natural identifier field found on %s for CSV import ' '(tried %s); falling back to pk.', diff --git a/netbox_custom_objects/tests/test_field_types.py b/netbox_custom_objects/tests/test_field_types.py index a2906db2..64ab346a 100644 --- a/netbox_custom_objects/tests/test_field_types.py +++ b/netbox_custom_objects/tests/test_field_types.py @@ -5,7 +5,7 @@ from unittest.mock import Mock from datetime import date, datetime from decimal import Decimal -from django.core.exceptions import ValidationError +from django.core.exceptions import FieldDoesNotExist, ValidationError from django.test import TestCase from core.models import ObjectType @@ -834,6 +834,31 @@ def test_csv_import_form_field_for_module_type_uses_model_field(self): self.assertEqual(form_field.to_field_name, 'model', "ModuleType CSV import must use 'model', not 'name'") + def test_csv_import_to_field_name_explicit_returned_when_valid(self): + """An explicit to_field_name that exists on the model is returned directly.""" + self.assertEqual(_csv_import_to_field_name(Device, explicit='id'), 'id') + + def test_csv_import_to_field_name_explicit_falls_through_when_stale(self): + """A stale explicit to_field_name (field no longer exists) triggers a warning + and falls through to the probe loop.""" + import logging + with self.assertLogs('netbox_custom_objects.field_types', level=logging.WARNING): + result = _csv_import_to_field_name(Device, explicit='nonexistent_field_xyz') + # Falls through to probe loop — Device has 'name' + self.assertEqual(result, 'name') + + def test_csv_import_to_field_name_pk_fallback_when_no_candidate_exists(self): + """Falls back to 'pk' and emits a warning when none of the candidate fields + exist on the model.""" + import logging + from unittest.mock import MagicMock + mock_model = MagicMock() + mock_model.__name__ = 'FakeModel' + mock_model._meta.get_field.side_effect = FieldDoesNotExist + with self.assertLogs('netbox_custom_objects.field_types', level=logging.WARNING): + result = _csv_import_to_field_name(mock_model) + self.assertEqual(result, 'pk') + class MultiObjectFieldTypeTestCase(FieldTypeTestCase): """Test cases for multiobject field type."""