/ipam/prefixes/{id}/available-ips/ wrong model returned

[Atoms]

NetBox version

v3.4.2

Python version

3.10

Steps to Reproduce

Check API documentation regarding what endpoint POST /ipam/prefixes/{id}/available-ips/ returns, it shows:

[IPAddress{...}]

Create request and inspect output:

{
  "id": 1655,
  "url": "https://xxxxxx/api/ipam/ip-addresses/1655/",
  "display": "x.x.x.x/x",
  "family": {
    "value": 4,
    "label": "IPv4"
  },
  "address": "x.x.x.x/x",
  "vrf": null,
  "tenant": null,
  "status": {
    "value": "active",
    "label": "Active"
  },
  "role": null,
  "assigned_object_type": null,
  "assigned_object_id": null,
  "assigned_object": null,
  "nat_inside": null,
  "nat_outside": [],
  "dns_name": "",
  "description": "",
  "comments": "",
  "tags": [],
  "custom_fields": {},
  "created": "2023-01-25T13:06:34.482571Z",
  "last_updated": "2023-01-25T13:06:34.482595Z"
}

Expected Behavior

as swagger.json is used to generate go-netbox client it's not possible to request new ip address as function return error it cannot serialize *models.IPAddress as []*models.IPAddress

Observed Behavior

json: cannot unmarshal object into Go value of type []*models.IPAddress

[jeremystretch]

This endpoint does return multiple objects. Why do you believe the schema is incorrect?

[Atoms]

When you request ip in specific prefix check returned value and model it describes, it describes list should be returned, instead it returns object, as go-netbox uses swagger to generate client, it expects list there and fails with error, cause object is received.

Screenshot describing issue in netbox demo also.

[Atoms]

You provide single prefix id to request one ip, i don't see or cannot create request which could return multiple items, can you show example how i can create request to this endpoint and get back list with objects?

[kkthxbye-code]

@Atoms - The following request body creates two IP's:

[
  {"description": "ip1"},
  {"description": "ip2"}
]

I think the issue has been raised before, you might want to search for it as I can't remember the conclusion, but the crux is that we return the straight object if only one IP is requested and we return a list of objects if multiple are requested. I possible solution would be to always return a list, but I vaguely recall that there was a reason we don't, maybe because it would be a breaking change.

Can you remember @jeremystretch?

As a workaround @Atoms, I think you can just always pass a list. If you are setting no fields, you can just do [{}].

[Atoms]

It does not make sense changing model depending on input what you provide, it's not consistent that way and we cannot relay on it. What will be those cases when it return object and what those cases when it's returning list of objects.

Yes, i can add workaround to always pass list, but that's just a workaround, why not fix it if it could be done.

For available prefixes create this is also same behaviour. it says [{}] will be removed, but {} is removed instead. Why then create swagger/doc if it's giving false information.

If it introduces breaking change, then it's still a bug. Can this be fixed in a way, that in request body there is always empty list, like default value [{}] not {}

[kkthxbye-code]

It does not make sense changing model depending on input what you provide, it's not consistent that way and we cannot relay on it. What will be those cases when it return object and what those cases when it's returning list of objects.

Not sure who you are arguing with here.

Yes, i can add workaround to always pass list, but that's just a workaround, why not fix it if it could be done.

I apologize for suggesting a workaround. I never said we were not going to fix it.

it says [{}] will be removed, but {} is removed instead

What/who is it in this context? What do you mean removed?

If it introduces breaking change, then it's still a bug.

Not sure how to parse this and how it is relevant.

I think there's a communication/culture barrier here, so I'll refrain from owning this issue. When another maintainer gets time, it's possible it'll be picked up.

[Atoms]

Sorry if my comment seemed harsh, I just want this fixed to not make workarounds. I have workaround in place that is not a question here. Thanks for your effort to look into matter.

[Atoms]

As go-netbox client is generated from swagger, swagger spec is first place where i look, and your suggestion to add request body [{}] or even [{ "description": "ip1"}] is not defined in schema WritableAvailableIP:

...
"post": {
    "operationId": "ipam_prefixes_available-ips_create",
    "description": "",
    "parameters": [
        {
            "name": "data",
            "in": "body",
            "required": true,
            "schema": {
                "$ref": "#/definitions/WritableAvailableIP"
            }
        }
    ],
....

WritableAvailableIP definition even does not have field description defined.

        "WritableAvailableIP": {
            "type": "object",
            "properties": {
                "family": {
                    "title": "Family",
                    "type": "integer",
                    "readOnly": true
                },
                "address": {
                    "title": "Address",
                    "type": "string",
                    "readOnly": true,
                    "minLength": 1
                }
            }
        },

in Go code generated from swagger this looks like:

type WritableAvailableIP struct {

	// Address
	// Read Only: true
	// Min Length: 1
	Address string `json:"address,omitempty"`

	// Family
	// Read Only: true
	Family int64 `json:"family,omitempty"`
}

and even current structure allows adding to request data which is object not list. I understand that this works for python as it's not so strict about input data type.

Currently as i'm requesting just one ip, i'm changing to return object not list of objects

[amhn]

Just found this bug. I made an earlier attempt to get this fixed in #10311

https://github.com/smutel/go-netbox/blob/main/patchs/swagger-v3.3.10-available-ip.patch is a patch to the schema to allow a list to be used as input. Don't remember how this would be fixed in netbox. But I think I had a working copy at some point.

In the meantime you can use smutel/go-netbox instead of netbox-community/go-netbox. That one contains the fix. You will have to use a list though. But a list of one ist still one. Usage example is in https://github.com/smutel/terraform-provider-netbox/blob/master/netbox/ipam/util.go#L31

[kkthxbye-code]

The issue is also present for ip-ranges, see #12381.

[jeremystretch]

As we've migrated to OpenAPI 3.0 since this bug was opened, is there any outstanding work to be done here? @arthanson do you recall if this has been tackled already?

[amhn]

The problem persists. But with OpenAPI 3.0 there are more possibilities to fix it.

Request:

                            "schema": {
                                "$ref": "#/components/schemas/WritableIPAddressRequest"
                            }

Response:

                                "schema": {
                                    "type": "array",
                                    "items": {
                                        "$ref": "#/components/schemas/IPAddress"
                                    }
                                }

So the definition still says to send a single object and expect a list of objects in return. Previously my idea to get a definition that is usable, was to change the request to a list. With OpenAPI 3.0 it is possible to use oneOf for both request and response, although you can not specify that a request with a list gets answered with a list and vice versa.

A possible approach could look like this:

diff --git a/netbox/ipam/api/views.py b/netbox/ipam/api/views.py
index f432e0e6b..67d54667b 100644
--- a/netbox/ipam/api/views.py
+++ b/netbox/ipam/api/views.py
@@ -2,7 +2,7 @@ from django.core.exceptions import ObjectDoesNotExist, PermissionDenied
 from django.db import transaction
 from django.shortcuts import get_object_or_404
 from django_pglocks import advisory_lock
-from drf_spectacular.utils import extend_schema
+from drf_spectacular.utils import extend_schema, PolymorphicProxySerializer
 from rest_framework import status
 from rest_framework.response import Response
 from rest_framework.routers import APIRootView
@@ -388,7 +388,24 @@ class AvailableIPAddressesView(ObjectValidationMixin, APIView):
 
         return Response(serializer.data)
 
-    @extend_schema(methods=["post"], responses={201: serializers.IPAddressSerializer(many=True)})
+    @extend_schema(
+        methods=["post"],
+        request=PolymorphicProxySerializer(
+            "AvailableIP",
+            serializers=[serializers.IPAddressSerializer,
+                         serializers.IPAddressSerializer(many=True)],
+            resource_type_field_name=None,
+            many=False,
+        ),
+        responses={201: PolymorphicProxySerializer(
+            "AvailableIPResponse",
+            serializers=[serializers.IPAddressSerializer,
+                         serializers.IPAddressSerializer(many=True)],
+            resource_type_field_name=None,
+            many=False,
+        )
+        }
+    )
     @advisory_lock(ADVISORY_LOCK_KEYS['available-ips'])
     def post(self, request, pk):
         self.queryset = self.queryset.restrict(request.user, 'add')

There are problems with this approach:

• the usage is more complex, especially in typed languages like go
• this diff references IPAddressSerializer, because WritableIPAddressSerializer is dynamically generated and
  can not be referenced in the code (at least as far as my python skills go). This means the request is typed like
  a response, with nested object as objects and not as int.

The schema diff then looks like this.

diff --git a/schema.json b/schema.json
index 31938a3..412afec 100644
--- a/schema.json
+++ b/schema.json
@@ -86454,16 +86454,15 @@
                     "content": {
                         "application/json": {
                             "schema": {
-                                "$ref": "#/components/schemas/WritableIPAddressRequest"
+                                "$ref": "#/components/schemas/AvailableIPRequest"
                             }
                         },
                         "multipart/form-data": {
                             "schema": {
-                                "$ref": "#/components/schemas/WritableIPAddressRequest"
+                                "$ref": "#/components/schemas/AvailableIPRequest"
                             }
                         }
-                    },
-                    "required": true
+                    }
                 },
                 "security": [
                     {
@@ -86478,10 +86477,7 @@
                         "content": {
                             "application/json": {
                                 "schema": {
-                                    "type": "array",
-                                    "items": {
-                                        "$ref": "#/components/schemas/IPAddress"
-                                    }
+                                    "$ref": "#/components/schemas/AvailableIPResponse"
                                 }
                             }
                         },
@@ -90670,16 +90666,15 @@
                     "content": {
                         "application/json": {
                             "schema": {
-                                "$ref": "#/components/schemas/WritableIPAddressRequest"
+                                "$ref": "#/components/schemas/AvailableIPRequest"
                             }
                         },
                         "multipart/form-data": {
                             "schema": {
-                                "$ref": "#/components/schemas/WritableIPAddressRequest"
+                                "$ref": "#/components/schemas/AvailableIPRequest"
                             }
                         }
-                    },
-                    "required": true
+                    }
                 },
                 "security": [
                     {
@@ -90694,10 +90689,7 @@
                         "content": {
                             "application/json": {
                                 "schema": {
-                                    "type": "array",
-                                    "items": {
-                                        "$ref": "#/components/schemas/IPAddress"
-                                    }
+                                    "$ref": "#/components/schemas/AvailableIPResponse"
                                 }
                             }
                         },
@@ -122584,6 +122576,32 @@
                     "vrf"
                 ]
             },
+            "AvailableIPRequest": {
+                "oneOf": [
+                    {
+                        "$ref": "#/components/schemas/IPAddressRequest"
+                    },
+                    {
+                        "type": "array",
+                        "items": {
+                            "$ref": "#/components/schemas/IPAddressRequest"
+                        }
+                    }
+                ]
+            },
+            "AvailableIPResponse": {
+                "oneOf": [
+                    {
+                        "$ref": "#/components/schemas/IPAddress"
+                    },
+                    {
+                        "type": "array",
+                        "items": {
+                            "$ref": "#/components/schemas/IPAddress"
+                        }
+                    }
+                ]
+            },
             "AvailablePrefix": {
                 "type": "object",
                 "description": "Representation of a prefix which does not exist in the database.",

[hikhvar]

I think this diff is much simpler:

diff --git a/netbox/ipam/api/views.py b/netbox/ipam/api/views.py
index f432e0e6b..40b4065ce 100644
--- a/netbox/ipam/api/views.py
+++ b/netbox/ipam/api/views.py
@@ -388,7 +388,10 @@ class AvailableIPAddressesView(ObjectValidationMixin, APIView):
 
         return Response(serializer.data)
 
-    @extend_schema(methods=["post"], responses={201: serializers.IPAddressSerializer(many=True)})
+    @extend_schema(methods=["post"],
+                   responses={201: serializers.IPAddressSerializer(many=True)},
+                   request=serializers.IPAddressSerializer(many=True),
+                   )
     @advisory_lock(ADVISORY_LOCK_KEYS['available-ips'])
     def post(self, request, pk):
         self.queryset = self.queryset.restrict(request.user, 'add')

In the generated model, only the lists usecase is supported. But I think that is suffcient. It generates easy to use models in typed languages. You put in a list, and get back a list. If you only have to request a single IP, just use a list with a singel object as request. Also we don't break existing clients relying on the current support for "it's either a list or object" magic.

Also oneOf is not supported in a popular Go OpenAPI generator: oapi-codegen/oapi-codegen#46