Merge pull request #21072 from netbox-community/21071-exception-request-url

Closes #21071: Include the request method & URL when displaying a server error

docs/administration/permissions.md

@@ -88,7 +88,7 @@ While permissions are typically assigned to specific groups and/or users, it is
 
 ### Viewing Objects
 
-Object-based permissions work by filtering the database query generated by a user's request to restrict the set of objects returned. When a request is received, NetBox first determines whether the user is authenticated and has been granted to perform the requested action. For example, if the requested URL is `/dcim/devices/`, NetBox will check for the `dcim.view_device` permission. If the user has not been assigned this permission (either directly or via a group assignment), NetBox will return a 403 (forbidden) HTTP response.
+Object-based permissions work by filtering the database query generated by a user's request to restrict the set of objects returned. When a request is received, NetBox first determines whether the user is authenticated and has been granted permission to perform the requested action. For example, if the requested URL is `/dcim/devices/`, NetBox will check for the `dcim.view_device` permission. If the user has not been assigned this permission (either directly or via a group assignment), NetBox will return a 403 (forbidden) HTTP response.
 
 If the permission _has_ been granted, NetBox will compile any specified constraints for the model and action. For example, suppose two permissions have been assigned to the user granting view access to the device model, with the following constraints:
 
@@ -102,9 +102,9 @@ If the permission _has_ been granted, NetBox will compile any specified constrai
 This grants the user access to view any device that is assigned to a site named NYC1 or NYC2, **or** which has a status of "offline" and has no tenant assigned. These constraints are equivalent to the following ORM query:
 
 ```no-highlight
-Site.objects.filter(
+Device.objects.filter(
     Q(site__name__in=['NYC1', 'NYC2']),
-    Q(status='active', tenant__isnull=True)
+    Q(status='offline', tenant__isnull=True)
 )
 ```
 

netbox/extras/forms/model_forms.py

@@ -189,22 +189,22 @@ class CustomFieldChoiceSetForm(ChangelogMessageMixin, forms.ModelForm):
         # if standardize these, we can simplify this code
 
         # Convert extra_choices Array Field from model to CharField for form
-        if 'extra_choices' in self.initial and self.initial['extra_choices']:
-            extra_choices = self.initial['extra_choices']
+        if extra_choices := self.initial.get('extra_choices', None):
             if isinstance(extra_choices, str):
                 extra_choices = [extra_choices]
-            choices = ""
+            choices = []
             for choice in extra_choices:
                 # Setup choices in Add Another use case
                 if isinstance(choice, str):
                     choice_str = ":".join(choice.replace("'", "").replace(" ", "")[1:-1].split(","))
-                    choices += choice_str + "\n"
+                    choices.append(choice_str)
                 # Setup choices in Edit use case
                 elif isinstance(choice, list):
-                    choice_str = ":".join(choice)
-                    choices += choice_str + "\n"
+                    value = choice[0].replace(':', '\\:')
+                    label = choice[1].replace(':', '\\:')
+                    choices.append(f'{value}:{label}')
 
-            self.initial['extra_choices'] = choices
+            self.initial['extra_choices'] = '\n'.join(choices)
 
     def clean_extra_choices(self):
         data = []

netbox/extras/models/customfields.py

@@ -878,6 +878,16 @@ class CustomFieldChoiceSet(CloningMixin, ExportTemplatesMixin, ChangeLoggedModel
         if not self.base_choices and not self.extra_choices:
             raise ValidationError(_("Must define base or extra choices."))
 
+        # Check for duplicate values in extra_choices
+        choice_values = [c[0] for c in self.extra_choices] if self.extra_choices else []
+        if len(set(choice_values)) != len(choice_values):
+            # At least one duplicate value is present. Find the first one and raise an error.
+            _seen = []
+            for value in choice_values:
+                if value in _seen:
+                    raise ValidationError(_("Duplicate value '{value}' found in extra choices.").format(value=value))
+                _seen.append(value)
+
         # Check whether any choices have been removed. If so, check whether any of the removed
         # choices are still set in custom field data for any object.
         original_choices = set([

netbox/extras/tests/test_customfields.py

@@ -1506,19 +1506,18 @@ class CustomFieldModelTest(TestCase):
 
     def test_invalid_data(self):
         """
-        Setting custom field data for a non-applicable (or non-existent) CustomField should raise a ValidationError.
+        Any invalid or stale custom field data should be removed from the instance.
         """
         site = Site(name='Test Site', slug='test-site')
 
         # Set custom field data
         site.custom_field_data['foo'] = 'abc'
         site.custom_field_data['bar'] = 'def'
-        with self.assertRaises(ValidationError):
-            site.clean()
-
-        del site.custom_field_data['bar']
         site.clean()
 
+        self.assertIn('foo', site.custom_field_data)
+        self.assertNotIn('bar', site.custom_field_data)
+
     def test_missing_required_field(self):
         """
         Check that a ValidationError is raised if any required custom fields are not present.

netbox/extras/tests/test_forms.py

@@ -5,6 +5,7 @@ from dcim.forms import SiteForm
 from dcim.models import Site
 from extras.choices import CustomFieldTypeChoices
 from extras.forms import SavedFilterForm
+from extras.forms.model_forms import CustomFieldChoiceSetForm
 from extras.models import CustomField, CustomFieldChoiceSet
 
 
@@ -90,6 +91,31 @@ class CustomFieldModelFormTest(TestCase):
             self.assertIsNone(instance.custom_field_data[field_type])
 
 
+class CustomFieldChoiceSetFormTest(TestCase):
+
+    def test_escaped_colons_preserved_on_edit(self):
+        choice_set = CustomFieldChoiceSet.objects.create(
+            name='Test Choice Set',
+            extra_choices=[['foo:bar', 'label'], ['value', 'label:with:colons']]
+        )
+
+        form = CustomFieldChoiceSetForm(instance=choice_set)
+        initial_choices = form.initial['extra_choices']
+
+        # colons are re-escaped
+        self.assertEqual(initial_choices, 'foo\\:bar:label\nvalue:label\\:with\\:colons')
+
+        form = CustomFieldChoiceSetForm(
+            {'name': choice_set.name, 'extra_choices': initial_choices},
+            instance=choice_set
+        )
+        self.assertTrue(form.is_valid())
+        updated = form.save()
+
+        # cleaned extra choices are correct, which does actually mean a list of tuples
+        self.assertEqual(updated.extra_choices, [('foo:bar', 'label'), ('value', 'label:with:colons')])
+
+
 class SavedFilterFormTest(TestCase):
 
     def test_basic_submit(self):

netbox/netbox/models/features.py

@@ -288,12 +288,13 @@ class CustomFieldsMixin(models.Model):
             cf.name: cf for cf in CustomField.objects.get_for_model(self)
         }
 
+        # Remove any stale custom field data
+        self.custom_field_data = {
+            k: v for k, v in self.custom_field_data.items() if k in custom_fields.keys()
+        }
+
         # Validate all field values
         for field_name, value in self.custom_field_data.items():
-            if field_name not in custom_fields:
-                raise ValidationError(_("Unknown field name '{name}' in custom field data.").format(
-                    name=field_name
-                ))
             try:
                 custom_fields[field_name].validate(value)
             except ValidationError as e:

netbox/netbox/views/errors.py

@@ -52,6 +52,7 @@ def handler_500(request, template_name=ERROR_500_TEMPLATE_NAME):
     type_, error = sys.exc_info()[:2]
 
     return HttpResponseServerError(template.render({
+        'request': request,
         'error': error,
         'exception': str(type_),
         'netbox_version': settings.RELEASE.full_version,

netbox/templates/500.html

@@ -35,6 +35,12 @@
 {% trans "Plugins" %}: {% for plugin, version in plugins.items %}
   {{ plugin }}: {{ version }}{% empty %}{% trans "None installed" %}{% endfor %}
 </pre>
+            <p>
+              {% trans "The request which yielded the above error is shown below:" %}
+            </p>
+            <p>
+              <code>{{ request.method }} {{ request.build_absolute_uri }}</code>
+            </p>
             <p>
               {% trans "If further assistance is required, please post to the" %} <a href="https://github.com/netbox-community/netbox/discussions">{% trans "NetBox discussion forum" %}</a> {% trans "on GitHub" %}.
             </p>