From 4d8c0bf80c4f7494f83014e18e58f79e2bdb4bc4 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 1 Sep 2026 11:32:00 -0400 Subject: [PATCH] Fix omission of Service protocol field from the OpenAPI request schema (#23085) --- contrib/openapi.json | 48 +++++++++++++++++ netbox/core/api/schema.py | 48 ++++++++++++++++- netbox/core/tests/test_openapi_schema.py | 66 ++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 1 deletion(-) diff --git a/contrib/openapi.json b/contrib/openapi.json index 1a3e6621a..6f1804021 100644 --- a/contrib/openapi.json +++ b/contrib/openapi.json @@ -341573,6 +341573,18 @@ "minLength": 1 } }, + "protocol": { + "enum": [ + "tcp", + "udp", + "sctp", + null + ], + "type": "string", + "description": "Deprecated; use port_mappings. Reported only for single-protocol services.", + "x-spec-enum-id": "e4b15bec749a2a32", + "nullable": true + }, "ports": { "type": "array", "items": { @@ -341640,6 +341652,18 @@ "minLength": 1 } }, + "protocol": { + "enum": [ + "tcp", + "udp", + "sctp", + null + ], + "type": "string", + "description": "Deprecated; use port_mappings. Reported only for single-protocol services.", + "x-spec-enum-id": "e4b15bec749a2a32", + "nullable": true + }, "ports": { "type": "array", "items": { @@ -366195,6 +366219,18 @@ "minLength": 1 } }, + "protocol": { + "enum": [ + "tcp", + "udp", + "sctp", + null + ], + "type": "string", + "description": "Deprecated; use port_mappings. Reported only for single-protocol services.", + "x-spec-enum-id": "e4b15bec749a2a32", + "nullable": true + }, "ports": { "type": "array", "items": { @@ -366267,6 +366303,18 @@ "minLength": 1 } }, + "protocol": { + "enum": [ + "tcp", + "udp", + "sctp", + null + ], + "type": "string", + "description": "Deprecated; use port_mappings. Reported only for single-protocol services.", + "x-spec-enum-id": "e4b15bec749a2a32", + "nullable": true + }, "ports": { "type": "array", "items": { diff --git a/netbox/core/api/schema.py b/netbox/core/api/schema.py index 5383f80a9..c95fad8d9 100644 --- a/netbox/core/api/schema.py +++ b/netbox/core/api/schema.py @@ -1,7 +1,9 @@ +import copy import re import typing from collections import OrderedDict +from django.core.exceptions import ImproperlyConfigured from django.utils.translation import gettext_lazy as _ from drf_spectacular.contrib.django_filters import DjangoFilterExtension from drf_spectacular.extensions import OpenApiSerializerExtension, OpenApiSerializerFieldExtension, _SchemaType @@ -16,6 +18,8 @@ from drf_spectacular.plumbing import ( ) from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import Direction, OpenApiParameter, OpenApiResponse +from rest_framework.fields import ReadOnlyField +from rest_framework.utils import model_meta from netbox.api.fields import ChoiceField from netbox.api.serializers import BulkOperationErrorSerializer, WritableNestedSerializer @@ -339,6 +343,36 @@ class NetBoxAutoSchema(AutoSchema): ref_name = ref_name[: -len('Serializer')] return ref_name + @staticmethod + def _rebuilds_as_writable(serializer, field_name): + """ + Return True if DRF would rebuild the named field in writable form if the field declared on + the serializer class were removed (see get_writable_class()). + + This defers to ModelSerializer.build_field(), which is what get_fields() itself calls for + any field not explicitly declared on the class -- rather than testing the model for a field + of that name, which is a weaker condition. A name backed only by a model property, by a + non-editable model field, or by a generic foreign key (which lives in Meta.private_fields + and so is absent from DRF's field info) is rebuilt read-only, and is then dropped from the + request body altogether. + """ + model = getattr(getattr(serializer, 'Meta', None), 'model', None) + if model is None or not hasattr(serializer, 'build_field'): + return False + + depth = getattr(serializer.Meta, 'depth', 0) + try: + field_class, field_kwargs = serializer.build_field( + field_name, model_meta.get_field_info(model), model, depth + ) + except ImproperlyConfigured: + # build_unknown_field(): the model has nothing of this name at all + return False + + if isinstance(field_class, type) and issubclass(field_class, ReadOnlyField): + return False + return not field_kwargs.get('read_only', False) + def get_writable_class(self, serializer): properties = {} fields = {} if hasattr(serializer, 'child') else serializer.fields @@ -352,7 +386,19 @@ class NetBoxAutoSchema(AutoSchema): if 'read_only' in dir(child) and child.read_only: remove_fields.append(child_name) if isinstance(child, (ChoiceField, WritableNestedSerializer)): - properties[child_name] = None + if child.read_only or self._rebuilds_as_writable(serializer, child_name): + properties[child_name] = None + else: + # DRF cannot rebuild this one writably: it is backed by a read-only property + # (e.g. Service.protocol, derived from port_mappings). Nulling it would leave + # DRF to rebuild it as a ReadOnlyField, which is then omitted from the request + # body altogether -- silently dropping a field the serializer does accept on + # write. Keep the declared field instead; ChoiceFieldFix already renders it + # correctly for the request direction. The copy leaves the bound original + # untouched (Field.__deepcopy__ returns an unbound field built from the same + # arguments), and keeps `properties` non-empty so the writable variant is still + # generated rather than collapsing to None below. + properties[child_name] = copy.deepcopy(child) if not properties: return None diff --git a/netbox/core/tests/test_openapi_schema.py b/netbox/core/tests/test_openapi_schema.py index acfb50611..5ee2d5e55 100644 --- a/netbox/core/tests/test_openapi_schema.py +++ b/netbox/core/tests/test_openapi_schema.py @@ -7,6 +7,10 @@ import json from django.test import TestCase +from core.api.schema import NetBoxAutoSchema +from ipam.api.serializers import ServiceSerializer +from netbox.api.serializers import BulkOperationErrorSerializer + class OpenAPISchemaTestCase(TestCase): """Tests for OpenAPI schema generation.""" @@ -218,3 +222,65 @@ class OpenAPISchemaTestCase(TestCase): schema.get('$ref'), '#/components/schemas/BulkOperationError', f"{method.upper()} {path} ({code}) should not reference the bulk error body" ) + + def test_service_request_documents_legacy_protocol_and_ports(self): + """ + The deprecated protocol/ports pair remains writable on application services (the serializer + translates it into port_mappings), so both must appear in the request body alongside + port_mappings. protocol is backed by a read-only model property rather than a model field, + which previously caused it to be dropped from the generated writable variant. + + Refs: #20285 + """ + for path in ('/api/ipam/services/', '/api/ipam/service-templates/'): + with self.subTest(path=path): + schema = self.schema['paths'][path]['post']['requestBody']['content']['application/json']['schema'] + ref = schema['oneOf'][0]['$ref'].split('/')[-1] + properties = self.schema['components']['schemas'][ref]['properties'] + + for field in ('port_mappings', 'protocol', 'ports'): + self.assertIn(field, properties, f"{ref} should document the '{field}' field") + + +class WritableFieldRebuildTestCase(TestCase): + """ + Tests for NetBoxAutoSchema._rebuilds_as_writable(), which decides whether a declared + ChoiceField/WritableNestedSerializer can be nulled out on the generated writable variant and + left for DRF to rebuild from the model. Getting this wrong drops the field from the request + body silently, so the predicate must match DRF's own build_field() behavior rather than merely + testing the model for a field of that name. + + Refs: #23083 + """ + + def test_rebuildable_fields(self): + """Fields DRF can rebuild writably should be reported as such.""" + serializer = ServiceSerializer() + + for field_name in ('name', 'description', 'ipaddresses'): + with self.subTest(field_name=field_name): + self.assertTrue(NetBoxAutoSchema._rebuilds_as_writable(serializer, field_name)) + + def test_non_rebuildable_fields(self): + """ + Fields DRF rebuilds as read-only (or cannot rebuild at all) must be reported as not + rebuildable, so that the declared field is retained instead. + """ + serializer = ServiceSerializer() + cases = { + 'protocol': "backed by a read-only model property, not a model field", + 'parent': "a GenericForeignKey, absent from DRF's field info", + 'created': "a non-editable model field", + 'no_such_field': "not present on the model at all", + } + + for field_name, reason in cases.items(): + with self.subTest(field_name=field_name): + self.assertFalse( + NetBoxAutoSchema._rebuilds_as_writable(serializer, field_name), + f"'{field_name}' should not be considered rebuildable ({reason})" + ) + + def test_serializer_without_model(self): + """A serializer with no Meta.model has nothing to rebuild from.""" + self.assertFalse(NetBoxAutoSchema._rebuilds_as_writable(BulkOperationErrorSerializer(), 'id'))