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

Don't allow shortname conflict in store #201

Open
jakubkrysl opened this issue Jan 25, 2023 · 1 comment
Open

Don't allow shortname conflict in store #201

jakubkrysl opened this issue Jan 25, 2023 · 1 comment
Labels
status: accepted This issue has been accepted by the maintainers team for implementation type: bug

Comments

@jakubkrysl
Copy link

Environment

  • DiffSync version: 1.7.0
  • Python version: 3.10.8
  • nautobot-ssot version: 1.2.0

Context

I have an Nautobot SSoT Job.
There is an issue where DiffSync store handles objects by uid but later for diff switches uid for shortname as name of the element:

def get_shortname(self) -> Text:
"""Get the (not guaranteed-unique) shortname of an object, if any.
By default the shortname is built based on all the keys defined in `_shortname`.
If `_shortname` is not specified, then this function is equivalent to `get_unique_id()`.
Returns:
str: Shortname of this object
"""
if self._shortname:
return "__".join([str(getattr(self, key)) for key in self._shortname])
return self.get_unique_id()

But there is no way to check if given shortname is already used in DiffSync store while adding it, I can only check for uid. And when I run diff calculation, it fails on shortname conflict, but catching ObjectAlreadyExists at this point aborts whole diff calculation.

Expected Behavior

Fail on step 2, DiffSync should not allow to add object to store if given shortname is already stored.

Observed Behavior

Fail on diff calculation, which is too late to do anything about it.

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/nautobot_ssot/jobs/base.py", line 332, in run
    self.sync_data()
  File "/usr/local/lib/python3.10/site-packages/nautobot_ssot/jobs/base.py", line 155, in sync_data
    self.calculate_diff()
  File "/usr/local/lib/python3.10/site-packages/nautobot_ssot/jobs/base.py", line 82, in calculate_diff
    self.diff = self.source_adapter.diff_to(self.target_adapter, flags=self.diffsync_flags)
  File "/usr/local/lib/python3.10/site-packages/diffsync/__init__.py", line 644, in diff_to
    return target.diff_from(self, diff_class=diff_class, flags=flags, callback=callback)
  File "/usr/local/lib/python3.10/site-packages/diffsync/__init__.py", line 626, in diff_from
    return differ.calculate_diffs()
  File "/usr/local/lib/python3.10/site-packages/diffsync/helpers.py", line 93, in calculate_diffs
    self.diff.add(diff_element)
  File "/usr/local/lib/python3.10/site-packages/diffsync/diff.py", line 60, in add
    raise ObjectAlreadyExists(f"Already storing a {element.type} named {element.name}", element)
diffsync.exceptions.ObjectAlreadyExists: ('Already storing a virtualmachine named *SHORTNAME*', )

Steps to Reproduce

  1. Have a model with shortname set
  2. Add 2 objects for that model with different identifiers but same shortnames
  3. Run diff calculation
@Kircheneer Kircheneer added status: accepted This issue has been accepted by the maintainers team for implementation type: bug labels Jan 26, 2023
@Kircheneer
Copy link
Collaborator

Thanks for submitting! This would need to be handled in

  • DiffSync.add
  • DiffSync.update
  • DiffSync.get_or_instantiate
  • DiffSync.get_or_add_model_instance
  • DiffSync.update_or_instantiate
  • DiffSync.update_or_add_model_instance

As such, its probably best to handle this in the classes inheriting from BaseStore as then it would only need to be handled for add and update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepted This issue has been accepted by the maintainers team for implementation type: bug
Projects
None yet
Development

No branches or pull requests

2 participants