DRAFT: Contrib Attibute Handling Overhaul #920
Conversation
Renrut5
changed the title
| """Standard attributes return the value stored within the model, no additional processing required.""" | ||
| value = getattr(db_obj, self.name) | ||
|
|
||
| if isinstance(value, EUI): |
There was a problem hiding this comment.
This needs to be aligned with the current approach of handling these conversion: https://docs.nautobot.com/projects/ssot/en/latest/dev/jobs/?h=load_#step-21-creating-the-nautobot-adapter
I prefer this one, because its opinionated, but there needs to be some way for users to build their own custom loading conversions when needed as we can't anticipate all possible data types, right?
| if self.custom_annotation.name in db_obj.cf: | ||
| return db_obj.cf[self.custom_annotation.key] | ||
| return None |
There was a problem hiding this comment.
| if self.custom_annotation.name in db_obj.cf: | |
| return db_obj.cf[self.custom_annotation.key] | |
| return None | |
| return db_obj.cf.get(self.custom_annotation.key, None) |
A little simplification, what do you think?
| from diffsync import DiffSyncModel | ||
|
|
||
|
|
||
| _cache = ORMCache() |
There was a problem hiding this comment.
I think the cache should probably be per-adapter. If it is here, I am not quite sure when it will be cleared, and there might be interactions between different SSoTs which may be problematic. Can you somehow tie its lifetime to the NautobotAdapter again?
There was a problem hiding this comment.
@Kircheneer, I was planning on phasing this one out for the same concerns.
Instead, I am not using the cache in this file as it'll only need to retrieve the data once per attribute interface. Additionally, I didn't want to make these classes dependent on an input specifically from the adapter so they can be isolated.
| def relationship(self) -> Relationship: | ||
| """""" | ||
| if not self._relationship: | ||
| self._relationship = Relationship.objects.get( | ||
| label=self.relationship_annotation.name, | ||
| ) | ||
| return self._relationship |
There was a problem hiding this comment.
The ORM cache essentially does the same thing, would it be possible to use that instead?
There was a problem hiding this comment.
@Kircheneer
The idea here is essentially how the attribute classes are created in __init_subclass__ and the apparent inability to access the database at this time, so creating the relationship var on instantiation does not seem possible.
To resolve the issue, I am using this approach instead of the caching class so that it can load the relationship object on the first access attempt (outside of the model's __init_subclass__ and when the database is accessible.
| relationship_annotation=custom_annotation, | ||
| ) | ||
| else: | ||
| if annotation.__name__.lower() == "list": |
There was a problem hiding this comment.
Can't we somehow isinstance against list here instead of comparing to a string?
| try: | ||
| association = adapter.get_from_orm_cache(association_parameters, RelationshipAssociation) | ||
| except RelationshipAssociation.DoesNotExist: | ||
| association = RelationshipAssociation(**association_parameters) | ||
| association.validated_save() | ||
| associations.append(association) |
There was a problem hiding this comment.
Do we need a get_or_create_with_orm_cache?
| parameters["destination_id"] = destination_object.id | ||
| defaults = {"source_id": source_object.id} | ||
|
|
||
| obj, _ = RelationshipAssociation.objects.update_or_create( |
There was a problem hiding this comment.
The comment I had on the other MR regarding when an update to a relationship association makes sense still applies here. Perhaps you can elaborate?
jdrew82
added
type: technical debt
integration: contrib
Closes: #834