2
0
Эх сурвалжийг харах

Fixes #22683: Prevent server errors when bulk import validation references an omitted field (#22784)

During partial bulk updates, fields omitted from the CSV are removed
from the import form before validation. Model validation can still
return an error for one of these fields, causing Django to raise a
ValueError instead of displaying the validation error.

Remap errors for absent fields to prefixed non-field errors on
NetBoxModelImportForm while preserving their codes, parameters, lazy
pluralization, and literal percent values. Genuine non-field errors
remain unchanged.

Add form-level and view-level regression coverage for mixed and
parameterized errors and for the reported interface bulk-update
workflow, including verification that invalid updates leave the object
unchanged.
Sri Chandraja Reddy Allala 1 долоо хоног өмнө
parent
commit
e9405d8f47

+ 32 - 0
netbox/dcim/tests/test_views.py

@@ -3463,6 +3463,38 @@ class InterfaceTestCase(ViewTestCases.DeviceComponentViewTestCase):
         self.assertHttpStatus(response, 302)
         self.assertHttpStatus(response, 302)
         self.assertEqual(Interface.objects.filter(device=device, name__startswith='xe').count(), 37)
         self.assertEqual(Interface.objects.filter(device=device, name__startswith='xe').count(), 37)
 
 
+    @override_settings(EXEMPT_VIEW_PERMISSIONS=['*'])
+    def test_bulk_import_omitted_field_validation_error(self):
+        """Surface omitted-field validation errors during bulk updates."""
+        device = Device.objects.first()
+        wireless_interface = Interface.objects.create(
+            device=device,
+            name='Wireless-22683',
+            type=InterfaceTypeChoices.TYPE_80211AC,
+            rf_channel_width=Decimal('20.0'),
+        )
+        self.add_permissions('dcim.add_interface', 'dcim.change_interface')
+        csv_data = '\n'.join([
+            'id,type',
+            f'{wireless_interface.pk},{InterfaceTypeChoices.TYPE_1GE_GBIC}',
+        ])
+        response = self.client.post(
+            self._get_url('bulk_import'),
+            data={
+                'data': csv_data,
+                'format': ImportFormatChoices.CSV,
+                'csv_delimiter': CSVDelimiterChoices.AUTO,
+            },
+        )
+        self.assertHttpStatus(response, 200)
+        self.assertContains(
+            response,
+            'rf_channel_width: Channel width may be set only on wireless interfaces.',
+        )
+        wireless_interface.refresh_from_db()
+        self.assertEqual(wireless_interface.type, InterfaceTypeChoices.TYPE_80211AC)
+        self.assertEqual(wireless_interface.rf_channel_width, Decimal('20.0'))
+
 
 
 class FrontPortTestCase(ViewTestCases.DeviceComponentViewTestCase):
 class FrontPortTestCase(ViewTestCases.DeviceComponentViewTestCase):
     model = FrontPort
     model = FrontPort

+ 26 - 0
netbox/netbox/forms/bulk_import.py

@@ -1,4 +1,5 @@
 from django import forms
 from django import forms
+from django.core.exceptions import NON_FIELD_ERRORS, ValidationError
 from django.db import models
 from django.db import models
 from django.utils.translation import gettext_lazy as _
 from django.utils.translation import gettext_lazy as _
 
 
@@ -70,6 +71,31 @@ class NetBoxModelImportForm(CSVModelForm, NetBoxModelForm):
 
 
         return cleaned
         return cleaned
 
 
+    def _update_errors(self, errors):
+        """Convert errors for fields absent from the form to prefixed non-field errors."""
+        if hasattr(errors, 'error_dict'):
+            remapped = []
+            passthrough = {}
+            for field, error_list in errors.error_dict.items():
+                if field == NON_FIELD_ERRORS or field in self.fields:
+                    passthrough[field] = error_list
+                else:
+                    for error in error_list:
+                        message = next(iter(error))
+                        if error.params:
+                            message = message.replace('%', '%%')
+                        remapped.append(ValidationError(
+                            '{field}: {message}'.format(field=field, message=message),
+                            code=error.code,
+                            params=error.params,
+                        ))
+            if passthrough:
+                super()._update_errors(ValidationError(passthrough))
+            for error in remapped:
+                self.add_error(None, error)
+        else:
+            super()._update_errors(errors)
+
 
 
 class OwnerCSVMixin(forms.Form):
 class OwnerCSVMixin(forms.Form):
     owner = CSVModelChoiceField(
     owner = CSVModelChoiceField(

+ 103 - 6
netbox/netbox/tests/test_forms.py

@@ -1,3 +1,8 @@
+from unittest.mock import patch
+
+from django.core.exceptions import NON_FIELD_ERRORS
+from django.core.exceptions import ValidationError as DjangoValidationError
+from django.core.validators import MaxLengthValidator
 from django.test import TestCase
 from django.test import TestCase
 
 
 from dcim.choices import InterfaceTypeChoices
 from dcim.choices import InterfaceTypeChoices
@@ -5,12 +10,8 @@ from dcim.forms import InterfaceImportForm
 from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site
 from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site
 
 
 
 
-class NetBoxModelImportFormCleanTestCase(TestCase):
-    """
-    Test the clean() method of NetBoxModelImportForm to ensure it properly converts
-    empty strings to None for nullable fields during CSV import.
-    Uses InterfaceImportForm as the concrete implementation to test.
-    """
+class NetBoxModelImportFormTestCase(TestCase):
+    """Test NetBoxModelImportForm."""
 
 
     @classmethod
     @classmethod
     def setUpTestData(cls):
     def setUpTestData(cls):
@@ -301,3 +302,99 @@ class NetBoxModelImportFormCleanTestCase(TestCase):
         )
         )
         self.assertTrue(form.is_valid(), f'Form errors: {form.errors}')
         self.assertTrue(form.is_valid(), f'Form errors: {form.errors}')
         self.assertIsNone(form.cleaned_data['wwn'])
         self.assertIsNone(form.cleaned_data['wwn'])
+
+    def test_missing_field_validation_error_becomes_non_field_error(self):
+        """Convert validation errors for absent fields to non-field errors."""
+        form = InterfaceImportForm(
+            data={
+                'device': self.device,
+                'name': 'Test Interface',
+                'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            }
+        )
+        with patch.object(
+            form.instance,
+            'full_clean',
+            side_effect=DjangoValidationError({'absent_field': ['Field error.']}),
+        ):
+            result = form.is_valid()
+
+        self.assertFalse(result)
+        self.assertIn('absent_field: Field error.', form.non_field_errors())
+
+    def test_non_field_error_not_overwritten_by_remapped_missing_field_error(self):
+        """Preserve remapped and existing non-field errors."""
+        form = InterfaceImportForm(
+            data={
+                'device': self.device,
+                'name': 'Test Interface Mixed',
+                'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            }
+        )
+        self.assertTrue(form.is_valid(), f'Form errors: {form.errors}')
+        # absent_field appears first to expose the former overwrite bug
+        form._update_errors(DjangoValidationError({
+            'absent_field': ['Field error.'],
+            NON_FIELD_ERRORS: ['A general error.'],
+        }))
+        non_field_errors = form.non_field_errors()
+        self.assertIn('A general error.', non_field_errors)
+        self.assertIn('absent_field: Field error.', non_field_errors)
+
+    def test_remapped_error_preserves_code_and_params(self):
+        """Preserve the original ValidationError code and params when remapping."""
+        form = InterfaceImportForm(
+            data={
+                'device': self.device,
+                'name': 'Test Interface Params',
+                'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            }
+        )
+        self.assertTrue(form.is_valid(), f'Form errors: {form.errors}')
+        form._update_errors(DjangoValidationError({
+            'absent_field': [
+                DjangoValidationError(
+                    '%(value)s is not a valid value.',
+                    code='invalid_value',
+                    params={'value': '100%'},
+                ),
+            ],
+        }))
+        non_field_errors = form.non_field_errors()
+        self.assertIn(
+            'absent_field: 100% is not a valid value.',
+            non_field_errors,
+        )
+        error_data = form.errors[NON_FIELD_ERRORS].as_data()
+        matching = [error for error in error_data if error.code == 'invalid_value']
+        self.assertEqual(len(matching), 1)
+        self.assertEqual(matching[0].params, {'value': '100%'})
+
+    def test_remapped_error_preserves_pluralized_message(self):
+        """Preserve pluralization order when remapping an ngettext_lazy message."""
+        form = InterfaceImportForm(
+            data={
+                'device': self.device,
+                'name': 'Test Interface Plural',
+                'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            }
+        )
+        self.assertTrue(form.is_valid(), f'Form errors: {form.errors}')
+        form._update_errors(DjangoValidationError({
+            'absent_field': [
+                DjangoValidationError(
+                    MaxLengthValidator.message,
+                    code=MaxLengthValidator.code,
+                    params={'limit_value': 20, 'show_value': 25},
+                ),
+            ],
+        }))
+        non_field_errors = form.non_field_errors()
+        self.assertIn(
+            'absent_field: Ensure this value has at most 20 characters (it has 25).',
+            non_field_errors,
+        )
+        error_data = form.errors[NON_FIELD_ERRORS].as_data()
+        matching = [error for error in error_data if error.code == MaxLengthValidator.code]
+        self.assertEqual(len(matching), 1)
+        self.assertEqual(matching[0].params, {'limit_value': 20, 'show_value': 25})