Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API variable type mismatch at creating/modifying an entry #2400

Closed
tatsushid opened this issue Aug 30, 2018 · 5 comments
Closed

API variable type mismatch at creating/modifying an entry #2400

tatsushid opened this issue Aug 30, 2018 · 5 comments
Labels
type: documentation A change or addition to the documentation

Comments

@tatsushid
Copy link
Contributor

Environment

  • Python version: 3.5.2
  • NetBox version: 2.4.3

Steps to Reproduce

  1. Open /api/docs/ URL of NetBox
  2. For example, open POST /ipam/ip-addresses/
  3. It shows
    tenant: {
        id: integer ...,
        url: string ...,
        name: string ...,
        slug: string ...
    }
    

Expected Behavior

In the example above, it should be

tenant: integer ...

Observed Behavior

It shows a parameter which is only compatible at reading but incompatible at writing like

tenant: {
    id: integer ...,
    url: string ...,
    name: string ...,
    slug: string ...
}

Details

Thanks for providing a great software! I'm enjoy using it.

After upgrading to 2.4.3, I found all "WritableXXX" data models had gone away from API JSON representation taken from /api/docs/?format=openapi and these had been replaced by models which were used at reading (GET request). It causes API document mismatch with NetBox internal behavior and a generated code by Swagger doesn't work especially in static type system language like Go etc.

In the above, I explain about a tenant parameter but there are some other parameters which have the same issue.

As far as I checked, it seems to have been introduced at commit 7241783, 821fb1e etc. I think a solution should be

  • Change API JSON description at POST, PUT etc. if a parameter inherits WritableNestedSerializer
  • Just allow a reading parameter and ignore unneeded arguments at writing

I'm not much familiar with Python so the ideas are just my thought.

@jeremystretch
Copy link
Member

The API documentation is generated automatically using drf-yasg. If someone can come up with a non-intrusive way of enabling Swagger to recognize the different read/write behaviors, I'll be happy to incorporate it.

@jeremystretch jeremystretch added help wanted type: documentation A change or addition to the documentation labels Sep 4, 2018
@tatsushid
Copy link
Contributor Author

I've written a fix and tested it in our system. It passed unit tests and worked with our API client written in Go. I'll send a pull-request once I clean up commits. Let's discuss about the implementation there.

tatsushid added a commit to tatsushid/netbox that referenced this issue Sep 14, 2018
In API view, representation of a parameter defined as a sub class of
`WritableNestedSerializer` should be vary between a request and a
response. For example, `tenant` field in `IPAddressSerializer` should be
shown like following as a request body:

```
tenant: integer ...
```

while it should be shown like following as a response body:

```
tenant: {
    id: integer ...,
    url: string ...,
    name: string ...,
    slug: string ...
}
```

But in both cases, it is shown as a response body type expression. This
causes an error at sending an API request with that type value.

It is only an API view issue, API can handle a request if a request
parameter is structured as an expected request body by ignoring the
wrong expression.

This fixes the issue by replacing an implicitly used default auto schema
generator class by its sub class and returning a pseudo serializer with
'Writable' prefix at generating a request body. The reason to introduce
a new generator class is that there is no other point which can
distinguish a request and a response. It is not enough to distinguish
POST, PUT, PATCH methods from GET because former cases may return a JSON
object as a response but it is also represented as same as a request
body, causes another mismatch.

This also fixes `SerializedPKRelatedField` type field representation. It
should be shown as an array of primary keys in a request body.

Fixed netbox-community#2400
tatsushid added a commit to tatsushid/netbox that referenced this issue Sep 14, 2018
In API view, representation of a parameter defined as a sub class of
`WritableNestedSerializer` should be vary between a request and a
response. For example, `tenant` field in `IPAddressSerializer` should be
shown like following as a request body:

```
tenant: integer ...
```

while it should be shown like following as a response body:

```
tenant: {
    id: integer ...,
    url: string ...,
    name: string ...,
    slug: string ...
}
```

But in both cases, it is shown as a response body type expression. This
causes an error at sending an API request with that type value.

It is only an API view issue, API can handle a request if a request
parameter is structured as an expected request body by ignoring the
wrong expression.

This fixes the issue by replacing an implicitly used default auto schema
generator class by its sub class and returning a pseudo serializer with
'Writable' prefix at generating a request body. The reason to introduce
a new generator class is that there is no other point which can
distinguish a request and a response. It is not enough to distinguish
POST, PUT, PATCH methods from GET because former cases may return a JSON
object as a response but it is also represented as same as a request
body, causes another mismatch.

This also fixes `SerializedPKRelatedField` type field representation. It
should be shown as an array of primary keys in a request body.

Fixed netbox-community#2400
@sdktr
Copy link
Contributor

sdktr commented Oct 18, 2018

and worked with our API client written in Go.

Is that a swagger/openapi auto generated client? In other words: does your fix apply to other places where this behavior is observed (and other swagger based clients)?

@tatsushid
Copy link
Contributor Author

Yes, it is generated from the swagger definition.

My purpose of the patch is generating swagger definition properly especially for a static type language and using it in my client. I've written it for NetBox 2.4.4, applied to it, downloaded swagger definition and generated a Go client library by go-swagger from it.

@jeremystretch
Copy link
Member

Django REST Framework 3.9 now includes native OpenAPI schema support. I haven't had a chance to dig into it yet, but we might end up ditching Swagger entirely in favor of the native implementation.

jeremystretch added a commit that referenced this issue Nov 27, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: documentation A change or addition to the documentation
Projects
None yet
Development

No branches or pull requests

3 participants