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

Replace NestedFooSerializer and brief=True pattern with DRF built-in Meta.depth #3042

Closed
Tracked by #254
jathanism opened this issue Dec 22, 2022 · 7 comments · Fixed by #3500
Closed
Tracked by #254

Replace NestedFooSerializer and brief=True pattern with DRF built-in Meta.depth #3042

jathanism opened this issue Dec 22, 2022 · 7 comments · Fixed by #3500
Assignees
Labels
impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release type: housekeeping Changes to the application which do not directly impact the end user
Milestone

Comments

@jathanism
Copy link
Contributor

Proposed Changes

The proposed change is:

  • Remove all nested_serializers modules and NestedFooSerializer classes
  • Remove all code to process them along with the concept of ?brief=True and setting self.brief on viewsets
  • Remove all overloaded foo = NestedFooSerializer() fields on API serializers and allow them to fall back to the default field from the model
  • Leverage the built-in nested serialization feature in DRF e.g. depth=0 is default (showing PKs), depth=1 shows first-level objects nested representations, etc.
  • Find a way to provide desired nested depth using request query parameters (e.g. /api/dcim/sites/?depth=1)

Justification

This bespoke implementation is hindering our ability to leverage existing community extensions to Django for things like JSON schema generation, API import/export using CSV, among others. Additionally it is extra boilerplate code that must be written for each new model, and is custom code that must continue to be maintained and tested.

We will be better off in the long run 1.) using existing features of DRF, 2.) leveraging community resources where reasonably possible.

@jathanism jathanism added the type: housekeeping Changes to the application which do not directly impact the end user label Dec 22, 2022
@jathanism jathanism added this to the v2.0.0 milestone Dec 22, 2022
@bryanculver
Copy link
Member

What is the impact to plugins and integrators to the schemas as well as things like pynautobot?

@glennmatthews
Copy link
Contributor

https://www.linkedin.com/pulse/friday-quick-tip-dynamic-depth-serialization-django-rest-neidinger is probably relevant as far as dynamic depth based on request query parameters.

One thing we might lose in making this change (and should seek to preserve) is the ability to identify related objects by natural keys or otherwise by properties other than PK, e.g.:

POST api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": {
        "site": {
            "name": "Equinix DC6"
        },
        "name": "R204"
    },
    ...
}

instead of requiring:

POST /api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": "7f3ca431-8103-45cc-a9ce-b94c1f784a1d",
    ...
}

@jathanism
Copy link
Contributor Author

https://www.linkedin.com/pulse/friday-quick-tip-dynamic-depth-serialization-django-rest-neidinger is probably relevant as far as dynamic depth based on request query parameters.

One thing we might lose in making this change (and should seek to preserve) is the ability to identify related objects by natural keys or otherwise by properties other than PK, e.g.:

POST api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": {
        "site": {
            "name": "Equinix DC6"
        },
        "name": "R204"
    },
    ...
}

instead of requiring:

POST /api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": "7f3ca431-8103-45cc-a9ce-b94c1f784a1d",
    ...
}

This is writable nested serialization, which I think could still be supported. See: https://www.django-rest-framework.org/api-guide/serializers/#writable-nested-representations

@jathanism
Copy link
Contributor Author

jathanism commented Dec 29, 2022

What is the impact to plugins and integrators to the schemas as well as things like pynautobot?

I think we should still marry this with other work to provide PK or Natural Key across the board as best we can. There are some other options here to consider as well. See:

@glennmatthews
Copy link
Contributor

https://www.linkedin.com/pulse/friday-quick-tip-dynamic-depth-serialization-django-rest-neidinger is probably relevant as far as dynamic depth based on request query parameters.
One thing we might lose in making this change (and should seek to preserve) is the ability to identify related objects by natural keys or otherwise by properties other than PK, e.g.:

POST api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": {
        "site": {
            "name": "Equinix DC6"
        },
        "name": "R204"
    },
    ...
}

instead of requiring:

POST /api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": "7f3ca431-8103-45cc-a9ce-b94c1f784a1d",
    ...
}

This is writable nested serialization, which I think could still be supported. See: https://www.django-rest-framework.org/api-guide/serializers/#writable-nested-representations

I may be misunderstanding but that seems different - that seems to be about e.g. creating the nested-serialized objects when creating the base object (although that's also a cool idea!), whereas what we have currently is the ability to reference existing nested-serialized objects by their natural key(s).

@jathanism
Copy link
Contributor Author

https://www.linkedin.com/pulse/friday-quick-tip-dynamic-depth-serialization-django-rest-neidinger is probably relevant as far as dynamic depth based on request query parameters.
One thing we might lose in making this change (and should seek to preserve) is the ability to identify related objects by natural keys or otherwise by properties other than PK, e.g.:

POST api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": {
        "site": {
            "name": "Equinix DC6"
        },
        "name": "R204"
    },
    ...
}

instead of requiring:

POST /api/dcim/devices/

{
    "name": "MyNewDevice",
    "rack": "7f3ca431-8103-45cc-a9ce-b94c1f784a1d",
    ...
}

This is writable nested serialization, which I think could still be supported. See: https://www.django-rest-framework.org/api-guide/serializers/#writable-nested-representations

I may be misunderstanding but that seems different - that seems to be about e.g. creating the nested-serialized objects when creating the base object (although that's also a cool idea!), whereas what we have currently is the ability to reference existing nested-serialized objects by their natural key(s).

We can do both in fact. Being able to support nested retrieval and updates using either primary keys, natural keys, or nested representations.

@bryanculver
Copy link
Member

Done with #3500

@bryanculver bryanculver added the impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release label May 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release type: housekeeping Changes to the application which do not directly impact the end user
Projects
No open projects
Archived in project
4 participants