Replies: 18 comments 10 replies
-
|
I tried generating the code using openapitools and got this error On the NetBox console, I'm seeing the following warnings |
Beta Was this translation helpful? Give feedback.
-
|
EDIT: I was on the wrong branch here. See comments in bold for update I tried to generate a client with https://openapi-generator.tech for go. I got the following errors: This seems fixed After fixing those by hand the API was generated without problems. I did not yet try to use the generated code. Manually inspecting the generated schema, I found the following regressions:
Since this deprecates the old API definition, this is a pretty major change for developers of API consumers based on the definition. Especially for Go, as the tool used by https://github.com/netbox-community/go-netbox/ and https://github.com/smutel/go-netbox/ does not support Openapi 3.0. Any tools I found produce different code, so all users of those libraries need to update their code as well. |
Beta Was this translation helpful? Give feedback.
-
|
@arthanson After looking at the openapi definition I have to take back This is actually stated as legal in the definition. Readonly fields MUST be ignored for write requests, so required does not matter. Sorry for the noise. As stated in the spectacular documentation, wether code generators handle this gracefully is another matter. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, I am the maintainer of https://github.com/e-breuninger/terraform-provider-netbox. By necessity I am also maintainer of https://github.com/fbreckle/go-netbox, because too many adjustments were needed for https://github.com/netbox-community/go-netbox. I suppose that goes for https://github.com/smutel/go-netbox as well. I think this change could be an opportunity to get the "official" https://github.com/netbox-community/go-netbox up to speed. Are there plans to get that repo up to speed, support wise? Previously, the devs did not put very much importance on the correctness of the openapi spec. Will it stay this way? Can someone with more openapi experience suggest a replacement tool for https://github.com/go-swagger/go-swagger? Otherwise what #11808 (comment) said. |
Beta Was this translation helpful? Give feedback.
-
|
@fbreckle Several questions here: "Are there plans to get that repo up to speed, support wise?" "Previously, the devs did not put very much importance on the correctness of the openapi spec. Will it stay this way?" Can someone with more openapi experience suggest a replacement tool for https://github.com/go-swagger/go-swagger? From playing with several of the generators for different languages I think it depends on the features you want from the generator and need to play with some and check the generated code to see if it meets your needs. I am sure there are probably issues with the NetBox OpenAPI 3 schema (as there are issues with the OpenAPI 2 schema) but in spot checks the OpenAPI v3 schema looks more accurate in several places. If you are actually using it, please submit any issues so they can get tracked and fixed. |
Beta Was this translation helpful? Give feedback.
-
|
I use https://github.com/deepmap/oapi-codegen as my code generator for OpenAPI v3.0 specs. That one works quite well with the spec provided by netbox 3.5.1. I already have less problems than with https://github.com/smutel/go-netbox, also I have to maintain only a patch for this #12644 However, I now need to migrate my code, because the types generated by the oapi-codegen generator are different than the previously generated types. My current generation looks like this: //go:generate curl -s -L https://our-internal-netbox.example.og/api/schema/ -o openapi.yaml
//go:generate bash -c "../../../../bin/yq -i '.components.schemas.IPAddress.properties.family.properties.value.type = \"integer\" ' openapi.yaml"
//go:generate rm -rf client.gen.go
//go:generate bash -c "../../../../bin/oapi-codegen --exclude-tags extras,virtualization,wireless,circuits,schema,status,users -generate types,client,skip-fmt -package openapi openapi.yaml > client.gen.go"
// oapi-codegen will generate some lines with 'const Foo int = <nil>' I couldn't track down this bug yet.
//go:generate sed -i "/= <nil>/d" client.gen.go
//go:generate go fmt client.gen.go
//go:generate ../../../../bin/goimports -w client.gen.gocc @v0ctor |
Beta Was this translation helpful? Give feedback.
-
|
Just to be sure: Do you want current issues with the swagger definition reported as well? Of course only if they are still present in the openapi definition. I just got reminded of an example:
Until now I focused more on getting the Openapi definition to the same level as the Swagger definition. As for generating the API, I used oapi-codegen in a different project and it worked. One drawback of using it with netbox is, that it produces a single HUGE file, because of the size of the netbox-API. Not sure yet if I like this one better or openapi-generator. Both are not as comprehensive as swagger, but this might allow to work around specific errors in a spec. As for the maintenance of a go library: The amount of work needed to provide a library. which is a shim to the REST API, depends heavily on the correctness of the API definition. The workflow in smutel/go-netbox is quite automated, the build grabs the latest netbox docker image with the right version (unfortunately 3.3.* at the moment), downloads the swagger.json, applies some patches, generates the library and creates a new commit with the updated library. All that is needed, is to maintain the patches. Sorry for the late reply. Work got in the way. I am very interested in getting a 'correct' definition into netbox and willing to do some work towards it (as work and other duties allow). |
Beta Was this translation helpful? Give feedback.
-
|
While we are talking about the API. I was wondering if it would be possible to make certain changes to the API which would make it easier to use, perhaps even allowing to generate a terraform-provider for netbox automatically. Basically to achieve this, the writable model should look more like the model that is read from the API. This could be achieved by including an _id field for every foreign key field. The foreign key field would then be marked read only and the write request would use the _id field. Another change could be to allow the tags to specified as simple list (of ids) instead of objects with name and slug. Those would be breaking changes, but could make the life of library maintainers easier, while not detracting from the ease of use for casual users. Perhaps a special mode could be introduced to allow this behaviour, like |
Beta Was this translation helpful? Give feedback.
-
|
@amhn "Just to be sure: Do you want current issues with the swagger definition reported as well? Of course only if they are still present in the openapi definition." - yes, please also note in the description that it still occurs with the OpenAPI 3 stuff in beta. "Unfortunately some issues were not picked up by netbox" - let me know which ones, I'm assuming there are issues filed for them. PRs are also greatly appreciated ;). Can't guarantee everything will get fixed quickly, but if there isn't an issue for it then it def won't get fixed. The other changes do sound like something that would need to be 4.0 changes if they are accepted, since they are breaking I'd open an issue on them with a justification and they can be discussed, I'm not sure I follow the _id field. |
Beta Was this translation helpful? Give feedback.
-
|
What I meant with _id field: Then one could write a script which extracts all fields for every model and generates for example a terraform provider. This is currently quite hard because one would have to find the ID on read to match it to the field on write. Though I am not yet sure this would be possible, I need to think about it further. But the current way to support a new model from netbox in a terraform provider involves a lot of copy and paste. As you said, this is more for 4.0. I will check if I can find my previous Issues. Some of them were fixed, but if I remember correctly some were not. For #4986 I will open a new Issue. |
Beta Was this translation helpful? Give feedback.
-
|
This post is mostly irrelevant. I found out, that this is already mostly supported. Most fields can already be posted to in the same way a get receives them. For the rest I opened #12196 These changes can be implemented without breaking changes. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, I'm a maintainer of netbox-community/go-netbox. Since a few weeks ago, the project is up-to-date and the code generation is fully automated. I'm working on making the necessary changes to generate the library from the OpenAPI 3 specification. As some people have already pointed out, it'll be necessary to replace go-swagger by another compatible option (maybe openapi-generator). It's a breaking change, so I'd like to evaluate the available options, so that the transition is as painless as possible for users. If I find any errors in the specification, I'll report them. The problem is that these errors usually appear with usage. I believe that, from go-netbox, we can improve communication so that these errors are reported to the appropriate project (Netbox itself). |
Beta Was this translation helpful? Give feedback.
-
|
@v0ctor - good to hear things are going in this direction. Do you have an issue you are tracking the work against in go-netbox? I do'nt see anything currently, so it's kinda hard to know the currentl status. From comments above, it seems like @fbreckle is doing similar work as well. Would be great if we could get a unified client with many people working on it, instead of several different forks of the same project. I would also be glad to assist in any way to keep this moving forward. Please let me know if I can. |
Beta Was this translation helpful? Give feedback.
-
|
@tagur87 I've been able to make progress on this. I created a discussion and an experimental PR to test openapi-generator, as it seems the most viable option. Everyone is welcome to participate! |
Beta Was this translation helpful? Give feedback.
-
|
Created issues #12149 #12148 #12116 concerning the openapi spec and swagger/redoc UI. |
Beta Was this translation helpful? Give feedback.
-
|
Another one: #12151 |
Beta Was this translation helpful? Give feedback.
-
|
I looked into OpenAPI a bit more and it turns out, there are some improvements that could be added. Probably not for 3.5. But that would be nice because these changes change the way the code is generated from the spec. At least in go. The actual behaviour is not modified, since this only documents actual behaviour. Except for the type field for GFK fields First change would be for the GenericForeignKey fields like assigned_object: Add a type field showing the type of the object and change the definition from (for ipaddress.assigned_object) assigned_object:
type: object
additionalProperties: {}
nullable: true
readOnly: trueto assigned_object:
allOf:
- type: object
properties:
type:
type: string
description: Type of assigned object
enum:
- virtualization.vminterface
- dcim.interface
- ipam.fhrpgroup
required:
- type
- oneOf:
- $ref: '#/components/schemas/NestedInterface'
- $ref: '#/components/schemas/NestedVMInterface'
- $ref: '#/components/schemas/NestedFHRPGroup'
discriminator:
propertyname: type
mapping:
virtualization.vminterface: '#/components/schemas/NestedVirtualMachine'
dcim.interface: '#/components/schemas/NestedInterface'
ipam.fhrpgroup: '#/components/schemas/NestedFHRPGroup'(Not sure yet about the discriminator part, because this is not yet supported for oapi-codegen) This change together with the eventual addition of a GFKSerializer would allow deprecating and getting rid of the duplicated information for GFK fields. (assigned_object_id, assigned_object_type) vs. assigned_object. Another change would be getting rid of (most) Writable(Nested)Serializers. This would require #12196 to be implemented plus additional API spec refinements. This also fixes #12149 without the need to keep track of Choices fields. For the nested serializers they could then be modified from, for example: NestedVRF:
type: object
description: |-
Represents an object related through a ForeignKey field. On write, it accepts a primary key (PK) value or a
dictionary of attributes which can be used to uniquely identify the related object. This class should be
subclassed to return a full representation of the related object on read.
properties:
id:
type: integer
url:
type: string
format: uri
readOnly: true
display:
type: string
readOnly: true
name:
type: string
maxLength: 100
rd:
type: string
nullable: true
title: Route distinguisher
description: Unique route distinguisher (as defined in RFC 4364)
maxLength: 21
required:
- display
- name
- urlto NestedVRF:
oneOf:
- type: null
description: Send null to unset this value
- type: integer
description: ID of the desired VRF object
write_only: true
- type: object
description: |-
Represents an object related through a ForeignKey field. On write, it accepts a primary key (PK) value or a
dictionary of attributes which can be used to uniquely identify the related object. This class should be
subclassed to return a full representation of the related object on read.
properties:
id:
type: integer
url:
type: string
format: uri
readOnly: true
display:
type: string
readOnly: true
name:
type: string
maxLength: 100
rd:
type: string
nullable: true
title: Route distinguisher
description: Unique route distinguisher (as defined in RFC 4364)
maxLength: 21
required:
- display
- name
- url(ID needs to be read/write here because it can be passed to select the desired object.) Let me know what you think about these changes. I would be willing to investigate further. As mentioned apart from adding a type field to GFK fields, no changes in behaviour would be necessary. For now I am looking into fixing the remaining warnings from the spec generation and will open issues shortly. |
Beta Was this translation helpful? Give feedback.
-
|
Created issues for the warnings:
The remaining warnings are all about IPAddressField: I'm not sure how to best fix them. One solution is to add a hint to BaseModelSerializer for the IPAddressField. Like this: diff --git a/netbox/netbox/api/serializers/base.py b/netbox/netbox/api/serializers/base.py
index 5ee74bf8c..256f59771 100644
--- a/netbox/netbox/api/serializers/base.py
+++ b/netbox/netbox/api/serializers/base.py
@@ -3,6 +3,8 @@ from rest_framework import serializers
from drf_spectacular.utils import extend_schema_field
from drf_spectacular.types import OpenApiTypes
+from ipam.fields import BaseIPField
+from rest_framework.fields import BooleanField, CharField
__all__ = (
'BaseModelSerializer',
'ValidatedModelSerializer',
@@ -12,6 +14,10 @@ __all__ = (
class BaseModelSerializer(serializers.ModelSerializer):
display = serializers.SerializerMethodField(read_only=True)
+ def __init__(self, instance=None, data=..., **kwargs):
+ super().__init__(instance, data, **kwargs)
+ self.serializer_field_mapping[BaseIPField] = CharField
+
@extend_schema_field(OpenApiTypes.STR)
def get_display(self, obj):
return str(obj)This gets rid of the warning, but adds |
Beta Was this translation helpful? Give feedback.
-
|
Hope this is as useful to you as it was frustrating for me. after a minute or so of no action ... |
Beta Was this translation helpful? Give feedback.
-
|
After a few days of trying I have come to the conclusion that relying on either Swagger or OpenAPI CodeGen to regenerate the API client will not work. If they don't crash on yaml issues then the code they produce (C#) is polluted with all kinds of trash-classes (eg the "anyOf" bug) and missing pieces. It is a Herculean task to compare to previous versions, especially those generated from a V2 spec. I feel the only practical way to update the API is to manually find the changes and implement them. Does there exist anywhere an archive of all the YAMLs for each version of the API? |
Beta Was this translation helpful? Give feedback.
-
|
@CADbloke I'm not sure if you are reporting a bug in the NetBox OpenAPI spec or if you are referencing bugs in the generators. If there are bugs in the NetBox OpenAPI spec, please file a bug report and we can look at getting them addressed. There is not an archive of the previous yaml's that I am aware of, however the last OpenAPI 2 yaml (for NetBox 3.4) is frozen in the tree at https://github.com/netbox-community/netbox/blob/develop/contrib/openapi2.yaml this was the version just before the switch to OpenAPI 3. |
Beta Was this translation helpful? Give feedback.
-
|
If you want to see the bugs in the OpenAPI just use https://app.swaggerhub.com/home & load the API from https://demo.netbox.dev/api/schema - I got 94 errors: "DELETE operations cannot have a requestBody" (refer swagger-api/swagger-editor#2749) and 5 warnings "Definition was declared but never used in document". Also, I also noticed in the C# code I generated from https://github.com/netbox-community/netbox/blob/develop/contrib/openapi2.yaml that |
Beta Was this translation helpful? Give feedback.
-
|
Has anybody had success generating client code with OpenAPI v3? Me, no. The c# code it is generating (if/when I can get it to) is so wrong in so many places it is worse than useless. I am at the stage of scrolling through release notes and manually changing the API model. V2 worked pretty well for me, it needed massaging but it wasn't fundamentally borked. Just checking if it's just me but after scrolling through https://github.com/swagger-api/swagger-codegen/issues & https://github.com/OpenAPITools/openapi-generator/issues I'm feeling v3 is a non-starter, at least nowadays. Has anybody had success generating client code with OpenAPI v3? |
Beta Was this translation helpful? Give feedback.
-
|
I have successfully generated a go client with it. I have some minor yaml patches to make the generated code more pleasant to use. The generated client is easier and better to use I use https://github.com/deepmap/oapi-codegen for the generation. I can later post my complete generation instructions. |
Beta Was this translation helpful? Give feedback.
-
|
Here is my script to generate the go client: Around the generated code I have a small library to have an abstraction from the generated code and not leak the types into my codebase. This allowed me to move seamlessly from https://github.com/smutel/go-netbox/ to my generated client. The manuall |
Beta Was this translation helpful? Give feedback.
-
|
Can I see somewhere how do you use the client.gen.go ? |
Beta Was this translation helpful? Give feedback.
-
|
https://github.com/microsoft/OpenAPI.NET/tree/vnext/src/Microsoft.OpenApi.Workbench reports no errors from https://demo.netbox.dev/api/schema version: 3.5.3 (3.5). They have more re$ource$ than Swagger & OpenAPI who just killed their c# .NET framework codegen: OpenAPITools/openapi-generator#15708 Path Items: 223 |
Beta Was this translation helpful? Give feedback.
-
|
Hey guys, sorry to necro this, but how is everyone working around the Swagger to csharp client generation issues with the DELETE (etc?) RequestBody conflict? |
Beta Was this translation helpful? Give feedback.
-
For NetBox v3.5 we are potentially looking at enabling support for OpenAPI 3 schema, however this requires removing the OpenAPI 2 schema that is currently in NetBox. If you currently rely on the swagger schema, please help us check the new schema to make sure it meets your needs and provide any feedback before the release so we can make sure it doesn't disrupt people relying on it.
Note: OpenAPI 3 schema format is different than OpenAPI 2 format, so any tools that rely on it (including API generators) will need to be updated.
The schema can be tried locally by using the NetBox in this PR: #11626, I have also attached the generated yaml file zip below. Please reply here if you have tried it successfully, or if you have encountered any problems with it. Any help would be greatly appreciated.
NetBox API (3.5) (2).yaml.zip
Beta Was this translation helpful? Give feedback.
All reactions