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

Implementing base models/adapters for Nautobot side functionality. #153

Merged
merged 3 commits into from Sep 1, 2023
Merged

Implementing base models/adapters for Nautobot side functionality. #153

merged 3 commits into from Sep 1, 2023

Conversation

Kircheneer
Copy link
Contributor

@Kircheneer Kircheneer commented Jul 28, 2023

Similar approach and borrowing lots of ideas from #69 but with automatic inference.

@Kircheneer Kircheneer requested a review from a team as a code owner July 28, 2023 18:52
@Kircheneer Kircheneer changed the title Draft implementing base models/adapters for Nautobot side functionality. Draft: Implementing base models/adapters for Nautobot side functionality. Jul 28, 2023
@Kircheneer Kircheneer marked this pull request as draft July 28, 2023 18:53
@lampwins
Copy link
Member

Is the goal here to get to dynamic pydantic models based on the underlying django models? Curious if you have explored https://github.com/jordaneremieff/djantic/?

@itdependsnetworks
Copy link
Contributor

The inference of data types would be great but the inference of the full model would not. This does not conflict with anything that @lampwins indicated, but just want to be clear of the intention as auto discovery of the whole model would be problematic given how complex Nautobot's data model is.

@Kircheneer
Copy link
Contributor Author

Is the goal here to get to dynamic pydantic models based on the underlying django models? Curious if you have explored https://github.com/jordaneremieff/djantic/?

This looks interesting. I will give usage of this a quick exploration, it may serve to make the model definition even simpler.

@itdependsnetworks your point is very valid, djantic appears to allow usage with include to explicitly state which fields we are interested in and this should be the recommended way.

@glennmatthews glennmatthews self-requested a review August 3, 2023 13:37
@Kircheneer Kircheneer marked this pull request as ready for review August 16, 2023 19:05
@Kircheneer Kircheneer changed the title Draft: Implementing base models/adapters for Nautobot side functionality. Implementing base models/adapters for Nautobot side functionality. Aug 16, 2023

### Putting it Together in a Job

Develop a Job class, derived from either the `nautobot_ssot.jobs.base.DataSource` or `DataTarget` classes provided by this plugin, and implement the adapters to populate the self.source_adapter and self.target_adapter that are used by the built-in implementation of sync_data. This sync_data method is an opinionated way of running the process including some performance data, more in next section, but you could overwrite it completely or any of the key hooks that it calls:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have trouble following this one, mind taking another crack at it?


## Many-to-one Relationships

For many-to-one relationships (i.e. [foreign keys](https://docs.djangoproject.com/en/3.2/topics/db/examples/many_to_one/) or [generic foreign keys](https://docs.djangoproject.com/en/3.2/ref/contrib/contenttypes/#generic-relations)) a slightly different approach is employed. We need to add a field on our model for each field that we need in order to uniquely identify our related object behind the many-to-one relationship. We can do this using a [familiar synta](https://docs.djangoproject.com/en/3.2/topics/db/queries/#lookups-that-span-relationships) of double underscore separated paths. Assuming we want to synchronize prefixes and associate them with locations, we may be faced with the problems that locations aren't uniquely identified by name alone, but rather need the location type as well, we can address this as follows.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For many-to-one relationships (i.e. [foreign keys](https://docs.djangoproject.com/en/3.2/topics/db/examples/many_to_one/) or [generic foreign keys](https://docs.djangoproject.com/en/3.2/ref/contrib/contenttypes/#generic-relations)) a slightly different approach is employed. We need to add a field on our model for each field that we need in order to uniquely identify our related object behind the many-to-one relationship. We can do this using a [familiar synta](https://docs.djangoproject.com/en/3.2/topics/db/queries/#lookups-that-span-relationships) of double underscore separated paths. Assuming we want to synchronize prefixes and associate them with locations, we may be faced with the problems that locations aren't uniquely identified by name alone, but rather need the location type as well, we can address this as follows.
For many-to-one relationships (i.e. [foreign keys](https://docs.djangoproject.com/en/3.2/topics/db/examples/many_to_one/) or [generic foreign keys](https://docs.djangoproject.com/en/3.2/ref/contrib/contenttypes/#generic-relations)) a slightly different approach is employed. We need to add a field on our model for each field that we need in order to uniquely identify our related object behind the many-to-one relationship. We can do this using a [familiar syntax](https://docs.djangoproject.com/en/4.2/topics/db/queries/#lookups-that-span-relationships) of double underscore separated paths. Assuming we want to synchronize `prefixes` and associate them with `locations`, we may be faced with the problems that locations aren't uniquely identified by name alone, but rather need the location type as well, we can address this as follows.

location__location_type__name: str
```

Now, on model `create` or `update`, the SSoT framework will dynamically pull in the location with the specified name and location type name, uniquely identifying the location and populating the foreign key.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "how" is not immediately clear to me to for how to add a field on our model is exactly constructed.


### Special Case: Generic Foreign Key

In the case of a [generic foreign key](https://docs.djangoproject.com/en/3.2/topics/db/examples/many_to_many/), we are faced with a problem. Normally, the [content type](https://docs.djangoproject.com/en/3.2/ref/contrib/contenttypes/) of a relationship field can be inferred from the model class. In the case of generic foreign keys however, this is not the case. In the example of the [IP address](https://docs.nautobot.com/projects/core/en/stable/models/ipam/ipaddress/?h=ip+address) model in Nautobot, the `assigned_object` field can point to either a device's or a VM's interface. We address this the following way:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In the case of a [generic foreign key](https://docs.djangoproject.com/en/3.2/topics/db/examples/many_to_many/), we are faced with a problem. Normally, the [content type](https://docs.djangoproject.com/en/3.2/ref/contrib/contenttypes/) of a relationship field can be inferred from the model class. In the case of generic foreign keys however, this is not the case. In the example of the [IP address](https://docs.nautobot.com/projects/core/en/stable/models/ipam/ipaddress/?h=ip+address) model in Nautobot, the `assigned_object` field can point to either a device's or a VM's interface. We address this the following way:
In the case of a [generic foreign key](https://docs.djangoproject.com/en/3.2/topics/db/examples/many_to_many/), we are faced with a problem. Normally, the [content type](https://docs.djangoproject.com/en/3.2/ref/contrib/contenttypes/) of a relationship field can be inferred from the model class. In the case of generic foreign key**s** however, this is not the case. In the example of the [IP address](https://docs.nautobot.com/projects/core/en/stable/models/ipam/ipaddress/?h=ip+address) model in Nautobot, the `assigned_object` field can point to either a device's or a VM's interface. We address this the following way:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, why is it important to highlight the "s" here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First you write:

In the case of a [generic foreign key]

Then you write:

In the case of generic foreign keys however

After reading that 5 times I am still not sure what two things are being compared. It seemed logical that you were comparing generic foreign key to generic foreign keys based on the paragraph structure ... but perhaps that is wrong?? So emphasis is my best guess at what is being stated as different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say a common theme is to be explicit, often by removing pronouns but in this case a bit different. It's very complex topic and I am not always understanding "what exactly" is being referred to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, this is wording problem. The "however" corresponds to the "normally", not to the "In the case of a [generic foreign key]". I will re-word this. Thanks!

nautobot_ssot/contrib.py Outdated Show resolved Hide resolved
nautobot_ssot/contrib.py Outdated Show resolved Hide resolved
nautobot_ssot/contrib.py Outdated Show resolved Hide resolved
@Kircheneer
Copy link
Contributor Author

Really well done!!

I think an example would be great as well. In my mind it showing how to reuse the model for the other side will be pretty helpful.

I have put a full example into the "Develop Jobs" section of the docs with the latest revision

docs/user/modeling.md Outdated Show resolved Hide resolved
docs/user/modeling.md Outdated Show resolved Hide resolved
docs/user/modeling.md Outdated Show resolved Hide resolved
docs/user/modeling.md Outdated Show resolved Hide resolved
docs/user/modeling.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the PR even though I still added some comments in terms of documentation.

@Kircheneer Kircheneer mentioned this pull request Aug 25, 2023
@itdependsnetworks
Copy link
Contributor

I think it makes sense to call into the bullpen and ask @glennmatthews to take a look? I don't fully understand everything that is happening here, with his experience building DiffSync, he likely has the best perspective.

docs/user/developing_jobs.md Outdated Show resolved Hide resolved
docs/user/developing_jobs.md Outdated Show resolved Hide resolved
docs/user/developing_jobs.md Outdated Show resolved Hide resolved
docs/user/developing_jobs.md Outdated Show resolved Hide resolved
docs/user/developing_jobs.md Show resolved Hide resolved
nautobot_ssot/contrib.py Show resolved Hide resolved
nautobot_ssot/contrib.py Show resolved Hide resolved
nautobot_ssot/contrib.py Outdated Show resolved Hide resolved
nautobot_ssot/contrib.py Outdated Show resolved Hide resolved
nautobot_ssot/contrib.py Show resolved Hide resolved
@Kircheneer Kircheneer merged commit 2e6e265 into nautobot:develop Sep 1, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants