Fix omission of Service protocol field from the OpenAPI request schema (#23085)

This commit is contained in:
Jeremy Stretch 2026-09-01 11:32:00 -04:00 committed by GitHub
parent f64bf0b217
commit 4d8c0bf80c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 161 additions and 1 deletions

View File

@ -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": {

View File

@ -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

View File

@ -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'))