nautobot-app-ssot icon indicating copy to clipboard operation
nautobot-app-ssot copied to clipboard

Custom relationship sorting

Open Renrut5 opened this issue 1 year ago • 3 comments

Closes #457

Renrut5 avatar Aug 01 '24 19:08 Renrut5

@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 avatar Aug 01 '24 20:08 Renrut5

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 avatar Aug 20 '24 13:08 Kircheneer

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

Kircheneer avatar Aug 20 '24 13:08 Kircheneer

@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.

Renrut5 avatar Oct 10 '24 17:10 Renrut5

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!

Kircheneer avatar Oct 23 '24 06:10 Kircheneer

@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?

Renrut5 avatar Oct 29 '24 19:10 Renrut5

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.

Kircheneer avatar Nov 01 '24 06:11 Kircheneer

@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")]

Renrut5 avatar Nov 12 '24 21:11 Renrut5

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

Kircheneer avatar Nov 13 '24 09:11 Kircheneer

@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.

Renrut5 avatar Nov 13 '24 15:11 Renrut5

@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?

Kircheneer avatar Nov 15 '24 07:11 Kircheneer

@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 avatar Dec 30 '24 18:12 Renrut5

@Kircheneer @jdrew82 , this is ready for review now though it's failing unittests for integrations outside of what I've worked on.

Renrut5 avatar Mar 27 '25 17:03 Renrut5

@Renrut5 can you rebase this against develop to see if the failures you're seeing are resolved?

jdrew82 avatar Apr 04 '25 20:04 jdrew82

Closed in lieu of https://github.com/Renrut5/nautobot-plugin-ssot/tree/custom-relationship-sorting

Renrut5 avatar Apr 08 '25 13:04 Renrut5