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

Custom relationship sorting #496

Draft
wants to merge 31 commits into
base: develop
Choose a base branch
from

Conversation

Renrut5
Copy link
Contributor

@Renrut5 Renrut5 commented Aug 1, 2024

Closes #457

@Renrut5 Renrut5 requested a review from a team as a code owner August 1, 2024 19:49
@Renrut5 Renrut5 requested a review from Kircheneer August 1, 2024 19:50
@Renrut5 Renrut5 marked this pull request as draft August 1, 2024 19:50
@Renrut5
Copy link
Contributor Author

Renrut5 commented Aug 1, 2024

@Kircheneer this is an idea of mine for sorting out relationships using contrib and avoiding false updates. The function can be used by both the Contrib NautobotAdapter and also by custom adapters for implementations.

The only thing that is needed for the NautobotAdapter is to specify the _sorted_relationships attribute, which should be the same value in both source and target adapters.

The custom adapters will need to be sure to import and call the function in load().

Thoughts?

@Renrut5 Renrut5 marked this pull request as ready for review August 5, 2024 13:24
@Kircheneer
Copy link
Contributor

What do you think of making this the default behaviour? I don't think to-many relationships are ordered ever, so maybe we can just default to this?

@Kircheneer
Copy link
Contributor

In this case though, I would probably like to do the sorting right in the loading

@jdrew82 jdrew82 added type: enhancement New feature or request integration: contrib Contrib related issues and PRs labels Aug 21, 2024
@Renrut5
Copy link
Contributor Author

Renrut5 commented Oct 10, 2024

@Kircheneer the issue is when the source and target load to-many relationships into an unordered list as is typically done. The problem is when that order varies between source and target. When that happens, the SSoT process gives a false update. All the items in the two comparing lists are exactly the same except for the order of the items themselves.

That's what this particular PR aims to resolve. The challenge is the to-many relationships must be ordered after everything is loaded into the DiffSync store and must be done on both source and target adapters to ensure they both match.

@Kircheneer
Copy link
Contributor

So I have been thinking about this again, and I don't see a scenario in which we would not want to do this. Do you think we can omit defining sorted_relationships on the model and just automatically sort anything that is a list?

Also, since you have tuples in here, what kind of use case is there for using tuples with contrib? I don't think I have hit this before, hence the asking.

Thanks!

@Renrut5
Copy link
Contributor Author

Renrut5 commented Oct 29, 2024

@Kircheneer Have been thinking about this. Not sure if omitting the sorted_relationships is the best idea.

The challenge comes when sorting the actual list of dictionaries, you need to define what key of the dictionaries to sort by. Not every dictionary has a name key, and so each relationship would need it's on configuration.

Unless there is a different way to sort the relationships that would be better?

@Kircheneer
Copy link
Contributor

You're right, I totally missed that. I want to try and have less implicit configuration (like the first element of the tuples in this list matching the field name on the model) and move towards explicit configuration. Do you think it would be possible that we somehow encode this information into the TypedDict we have to define anyway for the relationship? Say for example

class TenantDict(TypedDict):
    name: typing.Annotated[str, FieldType.SortBy]

where Fieldtype is a new Enum?

Let me know what you think of the approach, I know it makes the implementation a bit more complicated but the user interface is cleaner in my opinion.

@Renrut5
Copy link
Contributor Author

Renrut5 commented Nov 12, 2024

@Kircheneer what if we did the annotation in the model creation? I think I have a working case on this.

@dataclass
class SortedRelationship:
    sort_key: str

class MyModel(NautobotModel):
    ...
    relationship_field: typing.Annotated[list[dict], SortedRelationship("name")]

@Kircheneer
Copy link
Contributor

I thought about that option, too, but didn't like how you'd end up having to define two data structures (TypedDict + Dataclass) for the same relationship

@Renrut5
Copy link
Contributor Author

Renrut5 commented Nov 13, 2024

@Kircheneer my main issue with including the annotation into the TypedDict is that every entry in that list will have that same metadata (so it'll be repeated a significant number of times), and more importantly is that you'd have to pull the first item from the list, then search it each key in the dictionary for the appropriate annotation to sort by it. That would make the code more complex and difficult to follow than putting the annotation into the model, even if it is two data structures in the annotation.

@Kircheneer
Copy link
Contributor

@Kircheneer my main issue with including the annotation into the TypedDict is that every entry in that list will have that same metadata (so it'll be repeated a significant number of times)

The actual instances of the typed dict wouldn't have the metadata, you would only define it once on the definition of the typed dict, or am I missing something?

and more importantly is that you'd have to pull the first item from the list, then search it each key in the dictionary for the appropriate annotation to sort by it

I think you would only have to look at X in m2m_field: List[X] where class X(TypedDict), would you?

@Renrut5
Copy link
Contributor Author

Renrut5 commented Dec 30, 2024

@Kircheneer what are your thoughts on the changes made since last review?

Basically, I have it set to where sort_relationships will loop throug hthe top_level attribute of the target adapter. It will then check each entry for attributes that have the FieldType.SORTED_FIELD enum in the metadata.

If it finds it, it'll sort then look for a key inside the dictionary, if applicable, and look for FieldType.SORT_KEY enum. This will sort the list of dictionaries by that key.

For example:

class TagsDict(TypedDict):
   name: Annotated[str, FieldType.SORT_KEY]

class NautobotTenantModel(NautobotModel):
   _attributes = ("name", "tags",)
   tags: Annotated[List[str], FieldType.SORT_KEY] = []

@Renrut5 Renrut5 marked this pull request as draft December 31, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration: contrib Contrib related issues and PRs type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom relationships items ordering is not ignored when calculating changes
3 participants