netbox-community / go-netbox

The official Go API client for Netbox IPAM and DCIM service.
Other
194 stars 144 forks source link

Setting optional attributes to their "empty" value #107

Closed fbreckle closed 7 months ago

fbreckle commented 3 years ago

Hi,

this is a generalization of tickets like #105 and #106 because it applies to way more attributes than mentioned in these issues.

The problem Setting an optional attribute to its "empty" value (false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string) will remove the attribute from the resulting JSON that is sent to netbox. This means that these values can not be set to their empty values via go-netbox.

The cause This happens because in the swagger.json for optional attributes looks like this (e.g. WritableVirtualMachineWithConfigContext):

"platform": {
          "title": "Platform",
          "type": "integer",
          "x-nullable": true
        },

which translates to

    // Platform
    Platform *int64 `json:"platform,omitempty"`

which in turn will cause Go's json marshaller to omit empty values, which is intended behavior.

This is explained further in https://goswagger.io/faq/faq_model.html#non-required-or-nullable-property

A solution One possible solution which I employ in a personal fork of go-netbox is to add

        "platform": {
          "title": "Platform",
          "type": "integer",
          "x-nullable": true,
          "x-omitempty": false
        },

which will remove the omitempty annotation from the Go struct.

The problem with this solution is that it requires post-procession of the netbox-generated swaggerfile for all optional attributes. I do not know the stance of this repo's maintainers about post-processing the swagger.json file. It definitely feels less "pure" when compared to just fetching the swagger.json from netbox and regenerating the client. On the other hand, a client that cannot update certain attributes to their empty value is really not valuable.

Ideally, this would be fixed upstream by making netbox return a correct swagger file, but their swagger support is on a best-effort basis only.

v0ctor commented 1 year ago

This issue is related to the way the code is implemented from the OpenAPI specification by go-swagger.

We have to migrate this project from go-swagger to openapi-generator for Netbox 3.5. Maybe this will fix the issue.

I leave the issue open until further notice.

FlxPeters commented 1 year ago

@fbreckle we should try this. Would be nice to get rid of the custom go netbox client

BegBlev commented 1 year ago

@fbreckle, @FlxPeters it seems the problem comes from the use of omitempty with PATCH HTTP method. omitempty says "don't send the parameter if it is empty" the seconf says "Only uptade the parameters I send to you". Hence, empty parameters are not updated.

I saw in go-netbox code that each "update" fonction exists in 2 "flavors":

"full" update uses "PUT" method "partial" update uses "PATCH" method

I don't really understand why those 2 falvors and when should we use one or the other. But I wonder if this wouldn't be the solution. When you have omitempty parameters you should probably use "full" update function. Isn't it?

Thank you Vince

zeddD1abl0 commented 1 year ago

I don't really understand why those 2 flavors and when should we use one or the other.

A "full" update SETS the object to the values you send in the request. Anything that isn't included is set to null. A "partial" update only updates the fields you send, and won't update anything that's outside the request.

If you send a request with (in JSON cause lazy):

{
"id":1,
"name":"Switch01",
"asset_tag":"123456"
}

A PUT request will set the name and asset_tag as per the request, and also set all the other values to null. It is PUTting the update over the top of the previous object.

A PATCH request will set the name and asset_tag as per the request, and not touch another value. It is PATCHing the original object.

Essentially, use PATCH when you want to adjust something, and PUT when you want to replace something.

BegBlev commented 1 year ago

Hi @zeddD1abl0 and thank you for your reply, I know the difference between a PUT and PUSH, my question was about when or what for using "full" or "partial" update flavors of the library? From what I understand, if you know exactly which filed you will change, for instance using a form, and you know there are no "omitempty" fields then you should use the "partial" update flavor. If some of the field you can modify are "omitempty" fields then you must use "full" update flavor.

Hence, if my understanding is right, probably Netbox Terraform provider (which uses go-netbox library) should use "full" update.

v0ctor commented 7 months ago

A new alpha version has been released with a different software to generate the library, so hopefully this bug has been resolved.

Please feel free to test it and to provide feedback.