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: Custom Relationships API Serializer Mixin #369

Closed
FragmentedPacket opened this issue Apr 23, 2021 · 21 comments
Closed

API: Custom Relationships API Serializer Mixin #369

FragmentedPacket opened this issue Apr 23, 2021 · 21 comments
Assignees
Labels
type: feature Introduction of new or enhanced functionality to the application

Comments

@FragmentedPacket
Copy link
Contributor

FragmentedPacket commented Apr 23, 2021

Environment

  • Python version: 3.7
  • Nautobot version: 1.0.0b4

Proposed Functionality

Provide a custom relationships API serializer and attach to core models.
Add API serializer mixin to allow serialization of custom defined relationships.

May make use of https://github.com/nautobot/nautobot/blob/develop/nautobot/extras/models/relationships.py#L35-L146 to find the relationships.

Use Case

Serialize custom relationships and be able to use in inventory sources, etc.

Database Changes

None

External Dependencies

None

TODO:

In this first implementation we will support only read-only. A user will continue to use the relationship associations endpoints to modify and create relationships.

@glennmatthews glennmatthews added the type: feature Introduction of new or enhanced functionality to the application label Apr 23, 2021
@glennmatthews glennmatthews added this to the v1.1.0 milestone Apr 23, 2021
@FragmentedPacket
Copy link
Contributor Author

@dgarros
Copy link
Contributor

dgarros commented Apr 23, 2021

Not sure that I fully understand that is being proposed here but extending the core model serializers with Relationships data sounds dangerous and redundant with the GraphQL interface.

@itdependsnetworks
Copy link
Contributor

At a minimum, should document that relationships about primarily about enforcement.

@FragmentedPacket
Copy link
Contributor Author

Sorry I didn't explain this very well and came from an internal discussion with @lvrfrc87.

A quick example is tying VLANs to a device via a custom relationship defined.

Here is the custom relationship.

image

And now we edit a device and add VLANS to it. We can then see from either the device page or the VLAN page that there is a relationship.

image

But if we make an API call, that information isn't available, and is only available via the GUI or GraphQL via the rel_example query.

image

Should this information be available via all interfaces we currently support such as the API? And maybe @lvrfrc87 can explain his use case better.

@itdependsnetworks
Copy link
Contributor

That was my understanding of the use case as well, but since there is an alternative via graphql, I think that should suffice. Just my opinion though.

@lvrfrc87
Copy link
Contributor

I do not have a deep understanding of Nautobot architecture so I am not the best person to advise on this but from a user point of view, I would expect to have all the device info available also on API, so I agree with @FragmentedPacket as the info should be available via all interface.

@dgarros
Copy link
Contributor

dgarros commented Apr 26, 2021

@lvrfrc87 You can get all the information from the REST API but not everything at once :) this is by design for the REST API
The same way that you don't get the list of interfaces for a device by querying /api/dcim/devices, instead you need to query /api/dcim/interfaces to get this information.
You can query /api​/extras​/relationships​/ to get the list of all relationships defined in a system and /api/extras/relationship-associations/ to get the list of all object to object relationships.

GraphQL has been created specifically to address this limitation in a REST API

@lvrfrc87
Copy link
Contributor

@dgarros thanks for the explanation! The original issue was raised with nb_inventory though as would be great to get relationship under host_vars when run nb_inventory. Hope it make sense :)

@FragmentedPacket
Copy link
Contributor Author

@dgarros Thanks for the explanation. I will work with @lvrfrc87 offline, but that makes sense to either use the GraphQL interface or update the inventory plugin to grab the necessary relationships.

@glennmatthews
Copy link
Contributor

I actually think this bears a bit more discussion before closing it. We do serialize custom fields onto the model in the REST API, and it would be useful to serialize relationships in the REST API as well. We already have nested serializers for including related objects in "brief" form within the serialization of a base model, and it would seem appropriate to follow that same pattern for objects that are related via a Relationship as well as those that are related by a built-in field.

@jathanism
Copy link
Contributor

I actually think this bears a bit more discussion before closing it. We do serialize custom fields onto the model in the REST API, and it would be useful to serialize relationships in the REST API as well. We already have nested serializers for including related objects in "brief" form within the serialization of a base model, and it would seem appropriate to follow that same pattern for objects that are related via a Relationship as well as those that are related by a built-in field.

Seconded. I don't want to have different data from the API and in GraphQL. They should have parity.

@itdependsnetworks
Copy link
Contributor

Just my 2 cents. There is parity in that the data exists in both API and GraphQL. To me, they serve different purposes. Certainly don't feel too strong about it, but it seems like we have a solution for the use case, and this would be added work.

@lvrfrc87
Copy link
Contributor

I want to take the opportunity of this discussion to reinforce my understanding of GraphQL.
Is not GraphQL a "tool" to make API queries easier? If that is the case, why should we some data in API and not in GraphQL?
From user point of you, that could be misleading as he/she would expected to have same data available on all interfaces.

@itdependsnetworks
Copy link
Contributor

My understanding is all data is exposed in graphql and api, the delta being that how data is returned in both systems.

@jathanism
Copy link
Contributor

After some deliberation we have decided that custom relationships should never be emitted by default. We're not quite clear yet on the exact approach but the two key ones are:

  1. Supporting a query parameter that results in relationships being emitted explicitly by the caller. For example GET /api/dcim/devices/:uuid/?include=relationships
  2. Implement nested API detail endpoints for looking up relationships. For example GET /api/dcim/devices/:uuid/relationships/

If it were up to me, it would be option 2. I find the nested endpoint pattern to be very clean and predictable. As we already have API endpoints to be able to filter relationships based on the related content type object per the comment by @dgarros...

You can query /api​/extras​/relationships​/ to get the list of all relationships defined in a system and /api/extras/relationship-associations/ to get the list of all object to object relationships.

... A nested endpoint exposes the forward relationship from the object's point of view pretty naturally, without having to fret about an arcane query parameter.

@smk4664
Copy link
Contributor

smk4664 commented Jun 19, 2021

I like option #2 that @jathanism proposed.

@jvanderaa
Copy link
Contributor

To resolve the issue found on the Ansible inventory, nautobot/nautobot-ansible#53 was opened up on the Nautobot-Ansible project to create an inventory that would use the GraphQL interface. Would that solve for this from an SDK front?

If Option 1 is used, I think we may need to evaluate if there would need to be some updates to the pynautobot SDK to support for the parameters, specifically on the all endpoint of it. Today that does not take in anything that would allow for params to be passed. That may be something that may be of need in general.

@jathanism
Copy link
Contributor

To resolve the issue found on the Ansible inventory, nautobot/nautobot-ansible#53 was opened up on the Nautobot-Ansible project to create an inventory that would use the GraphQL interface. Would that solve for this from an SDK front?

Short answer: Yes. Long answer: No.

I want to make sure we are being cautious about disparate interfaces. Certainly we should try to tackle problems in the most optimized or efficient way, but we need to also assert feature parity across all interfaces.

If Option 1 is used, I think we may need to evaluate if there would need to be some updates to the pynautobot SDK to support for the parameters, specifically on the all endpoint of it. Today that does not take in anything that would allow for params to be passed. That may be something that may be of need in general.

100%. Whichever route we take, it must be discoverable in the OpenAPI schema and in the user-facing documentation.

@lampwins
Copy link
Member

lampwins commented Apr 8, 2022

Need to refine the acceptance criteria.

@lampwins lampwins assigned lampwins and unassigned jathanism Apr 8, 2022
@bryanculver bryanculver removed this from the v1.4.0 milestone Jul 12, 2022
@glennmatthews
Copy link
Contributor

@bryanculver @lampwins Can we close this as a duplicate of #1463?

@lampwins
Copy link
Member

lampwins commented Sep 2, 2022

Yes

@lampwins lampwins closed this as completed Sep 2, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature Introduction of new or enhanced functionality to the application
Projects
No open projects
Archived in project
Development

No branches or pull requests