Просмотр исходного кода

Fixes #23280: Fix change log REST API error for objects without a serializer (#23298)

The ObjectChange serializer's changed_object field raised SerializerNotFound
for change-logged models which have no REST API serializer (e.g. PortMapping
and PortTemplateMapping), causing an HTTP 500 when listing object changes.
Return null for changed_object in this case, as JobSerializer does.
Jeremy Stretch 1 день назад
Родитель
Сommit
e53906b784

+ 3 - 1
netbox/core/api/serializers_/change_logging.py

@@ -24,8 +24,10 @@ class ObjectChangeSerializer(BaseModelSerializer):
     changed_object_type = ContentTypeField(
     changed_object_type = ContentTypeField(
         read_only=True
         read_only=True
     )
     )
+    # Some change-logged models (e.g. PortMapping) are private and have no REST API serializer
     changed_object = GFKSerializerField(
     changed_object = GFKSerializerField(
-        read_only=True
+        read_only=True,
+        allow_missing_serializer=True
     )
     )
     object_repr = serializers.CharField(
     object_repr = serializers.CharField(
         read_only=True
         read_only=True

+ 53 - 1
netbox/core/tests/test_changelog.py

@@ -12,7 +12,7 @@ from rest_framework import status
 from core.choices import ObjectChangeActionChoices
 from core.choices import ObjectChangeActionChoices
 from core.jobs import SystemHousekeepingJob
 from core.jobs import SystemHousekeepingJob
 from core.models import ObjectChange, ObjectType
 from core.models import ObjectChange, ObjectType
-from dcim.choices import InterfaceTypeChoices, ModuleStatusChoices, SiteStatusChoices
+from dcim.choices import InterfaceTypeChoices, ModuleStatusChoices, PortTypeChoices, SiteStatusChoices
 from dcim.models import (
 from dcim.models import (
     Cable,
     Cable,
     CableTermination,
     CableTermination,
@@ -24,6 +24,8 @@ from dcim.models import (
     Module,
     Module,
     ModuleBay,
     ModuleBay,
     ModuleType,
     ModuleType,
+    PortMapping,
+    RearPort,
     Site,
     Site,
 )
 )
 from extras.choices import *
 from extras.choices import *
@@ -702,6 +704,56 @@ class ChangeLogAPITestCase(APITestCase):
         self.assertEqual(changes[3].changed_object_id, module.pk)
         self.assertEqual(changes[3].changed_object_id, module.pk)
         self.assertEqual(changes[3].action, ObjectChangeActionChoices.ACTION_DELETE)
         self.assertEqual(changes[3].action, ObjectChangeActionChoices.ACTION_DELETE)
 
 
+    def test_list_changes_for_object_without_serializer(self):
+        """
+        Listing ObjectChanges for a change-logged model which has no REST API serializer (e.g. PortMapping)
+        should not raise an exception.
+        """
+        device = create_test_device('device1')
+        rear_port = RearPort.objects.create(device=device, name='Rear Port 1', type=PortTypeChoices.TYPE_8P8C)
+        self.add_permissions('dcim.add_frontport', 'core.view_objectchange')
+
+        # Create a FrontPort mapped to the RearPort (creates a PortMapping)
+        data = {
+            'device': device.pk,
+            'name': 'Front Port 1',
+            'type': PortTypeChoices.TYPE_8P8C,
+            'rear_ports': [
+                {'position': 1, 'rear_port': rear_port.pk, 'rear_port_position': 1},
+            ],
+        }
+        url = reverse('dcim-api:frontport-list')
+        response = self.client.post(url, data, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_201_CREATED)
+        port_mapping = PortMapping.objects.get(front_port_id=response.data['id'])
+        self.assertTrue(
+            ObjectChange.objects.filter(
+                changed_object_type=ContentType.objects.get_for_model(PortMapping),
+                changed_object_id=port_mapping.pk,
+            ).exists()
+        )
+
+        front_port_id = response.data['id']
+
+        url = reverse('core-api:objectchange-list')
+        response = self.client.get(url, **self.header)
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+
+        # The FrontPort has a serializer, so its nested representation should be included
+        result = next(
+            r for r in response.data['results']
+            if r['changed_object_type'] == 'dcim.frontport' and r['changed_object_id'] == front_port_id
+        )
+        self.assertEqual(result['changed_object']['id'], front_port_id)
+
+        # The PortMapping has no serializer, so changed_object should be null
+        result = next(
+            r for r in response.data['results']
+            if r['changed_object_type'] == 'dcim.portmapping' and r['changed_object_id'] == port_mapping.pk
+        )
+        self.assertIsNone(result['changed_object'])
+        self.assertEqual(result['object_repr'], str(port_mapping))
+
 
 
 class ChangelogPruneRetentionTestCase(TestCase):
 class ChangelogPruneRetentionTestCase(TestCase):
     """Test suite for Changelog pruning retention settings."""
     """Test suite for Changelog pruning retention settings."""

+ 18 - 2
netbox/netbox/api/gfk_fields.py

@@ -1,6 +1,7 @@
 from drf_spectacular.utils import extend_schema_field
 from drf_spectacular.utils import extend_schema_field
 from rest_framework import serializers
 from rest_framework import serializers
 
 
+from netbox.api.exceptions import SerializerNotFound
 from utilities.api import get_serializer_for_model
 from utilities.api import get_serializer_for_model
 
 
 __all__ = (
 __all__ = (
@@ -10,8 +11,16 @@ __all__ = (
 
 
 @extend_schema_field(serializers.JSONField(allow_null=True, read_only=True))
 @extend_schema_field(serializers.JSONField(allow_null=True, read_only=True))
 class GFKSerializerField(serializers.Field):
 class GFKSerializerField(serializers.Field):
-    def __init__(self, **kwargs):
+    """
+    Represents a generic foreign key using the nested serializer for the related object's model.
+
+    Args:
+        allow_missing_serializer: If True, return None for objects whose model has no REST API serializer,
+            rather than raising SerializerNotFound.
+    """
+    def __init__(self, allow_missing_serializer=False, **kwargs):
         super().__init__(**kwargs)
         super().__init__(**kwargs)
+        self.allow_missing_serializer = allow_missing_serializer
         self._serializer_cache = {}
         self._serializer_cache = {}
 
 
     def to_representation(self, instance, **kwargs):
     def to_representation(self, instance, **kwargs):
@@ -19,8 +28,15 @@ class GFKSerializerField(serializers.Field):
             return None
             return None
         context = {'request': self.context['request']}
         context = {'request': self.context['request']}
         if instance.__class__ not in self._serializer_cache:
         if instance.__class__ not in self._serializer_cache:
-            serializer = get_serializer_for_model(instance)(nested=True, context=context)
+            try:
+                serializer = get_serializer_for_model(instance)(nested=True, context=context)
+            except SerializerNotFound:
+                if not self.allow_missing_serializer:
+                    raise
+                serializer = None
             self._serializer_cache[instance.__class__] = serializer
             self._serializer_cache[instance.__class__] = serializer
         else:
         else:
             serializer = self._serializer_cache[instance.__class__]
             serializer = self._serializer_cache[instance.__class__]
+        if serializer is None:
+            return None
         return serializer.to_representation(instance)
         return serializer.to_representation(instance)