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

Fields conveying annotated values are missing from REST API serialization of newly-created objects #14953

Closed
kendallzhu opened this issue Jan 26, 2024 · 4 comments · Fixed by #16158
Assignees
Labels
severity: medium Results in substantial degraded or broken functionality for specfic workflows status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@kendallzhu
Copy link

Deployment Type

Self-hosted

NetBox Version

v3.7.1

Python Version

3.11

Steps to Reproduce

  1. Run the server
  2. Generate the openapi spec by hitting api/schema endpoint
    (observe "tenant_count" field listed as required for the TenantGroup spec)
  3. Run a creation query for TenantGroup (observe the response does not contain tenant_count field)

This can also be reproduced for many of the other readonly fields in the schema.

Expected Behavior

Expected the generated spec and behavior to match, in one of two ways:

  • Creation query response should contain tenant_count field
  • "tenant_count" should not appear in the required fields for TenantGroup in the openapi spec
    (Similarly for many other readonly fields in the schema)

I suspect it is easier to change the spec than the behavior. It can be done by adding the drf-spectacular setting COMPONENT_NO_READ_ONLY_REQUIRED in the default settings.py, and marking all readonly fields that are required explicitly as required=True.

Relevant line of code specifying serializer properties: https://github.com/netbox-community/netbox/blob/develop/netbox/tenancy/api/serializers.py#L22

Note that setting required='False' in the line^ above does NOT change the generated spec because drf-spectacular defaults to applying a heuristic that readonly fields are required, unless COMPONENT_NO_READ_ONLY_REQUIRED is set. That is why we think the solution is to flip this default and label readonly fields as required except for the ones that will sometimes not be returned.
(More details here: tfranzel/drf-spectacular#1155).

Furthermore I think the fields that are sometimes not returned are the ones added to responses via annotations, such as the one here:

For full context, the use case here is deriving generated code from the generated openapi spec, which would be easier if the spec matched with server behavior. Currently it requires manually editing the spec to adjust the aforementioned fields after observing errors.

Observed Behavior

See "Steps to Reproduce"

@kendallzhu kendallzhu added the type: bug A confirmed report of unexpected behavior in the application label Jan 26, 2024
@abhi1693 abhi1693 self-assigned this Feb 21, 2024
@abhi1693 abhi1693 added status: accepted This issue has been accepted for implementation severity: low Does not significantly disrupt application functionality, or a workaround is available labels Feb 21, 2024
@jeremystretch
Copy link
Member

  1. Generate the openapi spec by hitting api/schema endpoint
    (observe "tenant_count" field listed as required for the TenantGroup spec)

Where are you seeing tenant_count listed as a required field?

screenshot

AFAICT it's only marked as a required field for the response (which is correct; it will always be included):

screenshot

@kendallzhu
Copy link
Author

The issue is that tenant_count does not appear in the response for a creation request, even though it is listed as required.

@jeremystretch
Copy link
Member

Ok, so this has nothing to do with the spec; the field should always exist. The issue is that the field is missing from the object's serialized representation immediately upon being created.

I suppose the root issue is in the create() method of DRF's CreateModelMixin, which uses only the POSTed data when rendering the serializer, rather than the saved object's new database record:

https://github.com/encode/django-rest-framework/blob/a45432b54de88b28bd2e6db31acfe3dbcfc748dd/rest_framework/mixins.py#L18-L23

The update logic is similarly affected.

We could potentially override this logic to read the newly-created object from the database rather than relying on the input data, which seems like a good idea, however we'll need to consider the ramifications of doing so.

@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed status: accepted This issue has been accepted for implementation topic: OpenAPI severity: low Does not significantly disrupt application functionality, or a workaround is available labels Feb 22, 2024
@jeremystretch jeremystretch changed the title Create Response Mismatch with Generated OpenAPI Spec Fields conveying annotated values are missing from REST API serialization of newly-created objects Feb 22, 2024
@jqueuniet
Copy link

jqueuniet commented Mar 8, 2024

Some users of the netbox-go library seem to have run into a similar issue with the following endpoints:

  • dcim/manufacturers
  • dcim/device-roles
  • dcim/device-types
  • dcim/sites

The netbox-go library is generated from the OpenAPI spec, and enforces the required status of the various missing *_count fields. So this leads to a hard deserialization error right now.

For reference:

netbox-community/go-netbox#165
netbox-community/go-netbox#166 (reply in thread)

@jeremystretch jeremystretch added the severity: medium Results in substantial degraded or broken functionality for specfic workflows label May 13, 2024
@arthanson arthanson self-assigned this May 15, 2024
@arthanson arthanson removed the status: under review Further discussion is needed to determine this issue's scope and/or implementation label May 15, 2024
@jeremystretch jeremystretch added the status: accepted This issue has been accepted for implementation label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity: medium Results in substantial degraded or broken functionality for specfic workflows status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
5 participants