Skip to content

Add new model flags #87

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

Merged
merged 2 commits into from
Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 51 additions & 12 deletions diffsync/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,29 @@ def set_status(self, status: DiffSyncStatus, message: Text = ""):
self._status = status
self._status_message = message

@classmethod
def create_base(cls, diffsync: "DiffSync", ids: Mapping, attrs: Mapping) -> Optional["DiffSyncModel"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the latest changes I'm back to not seeing why we need to have these _base methods added at all. If the subclass overrides create() then by default this method won't be called unless they call super().create(), which is now exactly identical to calling super().create_base() - so why have two different methods that do the same thing?

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 can remove them if you really want but I know if would have solved a situation for one of my project we had 3 layers of classes, like this

  1. DiffSyncModel
  2. NetworkImporterModel
  3. CustomerDefinedNetworkImporterModel

In this scenario, the goal was to redefine NetworkImporterModel.update() with CustomerDefinedNetworkImporterModel.update() to change it's behavior so calling super() wasn't possible but we still have to call DiffSyncModel.update()
We solved it by calling DiffSyncModel.update() directly but I think this isn't ideal either.
I think., having the base method provides a clean way to identify the code that must be executed.

Again, happy to remove if you prefer ... I'm not gonna fall on my sword for this one

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually ran into something similar with the ssot-servicenow plugin and the create() method, where I didn't want to call the immediate superclass's create() but still needed to call the base DiffSyncModel.create(). This would have been potentially useful there as well; I'm just not entirely persuaded that needing to do this is a sign of a good code smell. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think it would be redesign the API for 2.0 to manage this situation better

"""Instantiate this class, along with any platform-specific data creation.

This method is not meant to be subclassed, users should redefine create() instead.

Args:
diffsync: The master data store for other DiffSyncModel instances that we might need to reference
ids: Dictionary of unique-identifiers needed to create the new object
attrs: Dictionary of additional attributes to set on the new object

Returns:
DiffSyncModel: instance of this class.
"""
model = cls(**ids, diffsync=diffsync, **attrs)
model.set_status(DiffSyncStatus.SUCCESS, "Created successfully")
return model

@classmethod
def create(cls, diffsync: "DiffSync", ids: Mapping, attrs: Mapping) -> Optional["DiffSyncModel"]:
"""Instantiate this class, along with any platform-specific data creation.

Subclasses must call `super().create()`; they may wish to then override the default status information
Subclasses must call `super().create()` or `self.create_base()`; they may wish to then override the default status information
by calling `set_status()` to provide more context (such as details of any interactions with underlying systems).

Args:
Expand All @@ -194,14 +212,30 @@ def create(cls, diffsync: "DiffSync", ids: Mapping, attrs: Mapping) -> Optional[
Raises:
ObjectNotCreated: if an error occurred.
"""
model = cls(**ids, diffsync=diffsync, **attrs)
model.set_status(DiffSyncStatus.SUCCESS, "Created successfully")
return model
return cls.create_base(diffsync=diffsync, ids=ids, attrs=attrs)

def update_base(self, attrs: Mapping) -> Optional["DiffSyncModel"]:
"""Base Update method to update the attributes of this instance, along with any platform-specific data updates.

This method is not meant to be subclassed, users should redefine update() instead.

Args:
attrs: Dictionary of attributes to update on the object

Returns:
DiffSyncModel: this instance.
"""
for attr, value in attrs.items():
# TODO: enforce that only attrs in self._attributes can be updated in this way?
setattr(self, attr, value)

self.set_status(DiffSyncStatus.SUCCESS, "Updated successfully")
return self

def update(self, attrs: Mapping) -> Optional["DiffSyncModel"]:
"""Update the attributes of this instance, along with any platform-specific data updates.

Subclasses must call `super().update()`; they may wish to then override the default status information
Subclasses must call `super().update()` or `self.update_base()`; they may wish to then override the default status information
by calling `set_status()` to provide more context (such as details of any interactions with underlying systems).

Args:
Expand All @@ -214,17 +248,23 @@ def update(self, attrs: Mapping) -> Optional["DiffSyncModel"]:
Raises:
ObjectNotUpdated: if an error occurred.
"""
for attr, value in attrs.items():
# TODO: enforce that only attrs in self._attributes can be updated in this way?
setattr(self, attr, value)
return self.update_base(attrs=attrs)

self.set_status(DiffSyncStatus.SUCCESS, "Updated successfully")
def delete_base(self) -> Optional["DiffSyncModel"]:
"""Base delete method Delete any platform-specific data corresponding to this instance.

This method is not meant to be subclassed, users should redefine delete() instead.

Returns:
DiffSyncModel: this instance.
"""
self.set_status(DiffSyncStatus.SUCCESS, "Deleted successfully")
return self

def delete(self) -> Optional["DiffSyncModel"]:
"""Delete any platform-specific data corresponding to this instance.

Subclasses must call `super().delete()`; they may wish to then override the default status information
Subclasses must call `super().delete()` or `self.delete_base()`; they may wish to then override the default status information
by calling `set_status()` to provide more context (such as details of any interactions with underlying systems).

Returns:
Expand All @@ -234,8 +274,7 @@ def delete(self) -> Optional["DiffSyncModel"]:
Raises:
ObjectNotDeleted: if an error occurred.
"""
self.set_status(DiffSyncStatus.SUCCESS, "Deleted successfully")
return self
return self.delete_base()

@classmethod
def get_type(cls) -> Text:
Expand Down
14 changes: 14 additions & 0 deletions diffsync/enum.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,20 @@ class DiffSyncModelFlags(enum.Flag):
Can be used for the case where deletion of a model results in the automatic deletion of all its children.
"""

SKIP_UNMATCHED_SRC = 0b100
"""Ignore the model if it only exists in the source/"from" DiffSync when determining diffs and syncing.

If this flag is set, no new model will be created in the target/"to" DiffSync.
"""

SKIP_UNMATCHED_DST = 0b1000
"""Ignore the model if it only exists in the target/"to" DiffSync when determining diffs and syncing.

If this flag is set, the model will not be deleted from the target/"to" DiffSync.
"""

SKIP_UNMATCHED_BOTH = SKIP_UNMATCHED_SRC | SKIP_UNMATCHED_DST


class DiffSyncFlags(enum.Flag):
"""Flags that can be passed to a sync_* or diff_* call to affect its behavior."""
Expand Down
80 changes: 49 additions & 31 deletions diffsync/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def validate_objects_for_diff(object_pairs: Iterable[Tuple[Optional["DiffSyncMod
if src_obj.get_identifiers() != dst_obj.get_identifiers():
raise ValueError(f"Keys mismatch: {src_obj.get_identifiers()} vs {dst_obj.get_identifiers()}")

def diff_object_pair(
def diff_object_pair( # pylint: disable=too-many-return-statements
self, src_obj: Optional["DiffSyncModel"], dst_obj: Optional["DiffSyncModel"]
) -> Optional[DiffElement]:
"""Diff the two provided DiffSyncModel objects and return a DiffElement or None.
Expand All @@ -180,11 +180,19 @@ def diff_object_pair(

log = self.logger.bind(model=model, unique_id=unique_id)
if self.flags & DiffSyncFlags.SKIP_UNMATCHED_SRC and not dst_obj:
log.debug("Skipping unmatched source object")
log.debug("Skipping due to SKIP_UNMATCHED_SRC flag on source adapter")
self.incr_models_processed()
return None
if self.flags & DiffSyncFlags.SKIP_UNMATCHED_DST and not src_obj:
log.debug("Skipping unmatched dest object")
log.debug("Skipping due to SKIP_UNMATCHED_DST flag on source adapter")
self.incr_models_processed()
return None
if src_obj and not dst_obj and src_obj.model_flags & DiffSyncModelFlags.SKIP_UNMATCHED_SRC:
log.debug("Skipping due to SKIP_UNMATCHED_SRC flag on model")
self.incr_models_processed()
return None
if dst_obj and not src_obj and dst_obj.model_flags & DiffSyncModelFlags.SKIP_UNMATCHED_DST:
log.debug("Skipping due to SKIP_UNMATCHED_DST flag on model")
self.incr_models_processed()
return None
if src_obj and src_obj.model_flags & DiffSyncModelFlags.IGNORE:
Expand Down Expand Up @@ -284,6 +292,7 @@ def __init__( # pylint: disable=too-many-arguments
):
"""Create a DiffSyncSyncer instance, ready to call `perform_sync()` against."""
self.diff = diff
self.src_diffsync = src_diffsync
self.dst_diffsync = dst_diffsync
self.flags = flags
self.callback = callback
Expand Down Expand Up @@ -339,42 +348,51 @@ def sync_diff_element(self, element: DiffElement, parent_model: "DiffSyncModel"
# We only actually need the "new" attrs to perform a create/update operation, and don't need any for a delete
attrs = diffs.get("+", {})

model: Optional["DiffSyncModel"]
# Retrieve Source Object to get its flags
src_model: Optional["DiffSyncModel"]
try:
model = self.dst_diffsync.get(self.model_class, ids)
model.set_status(DiffSyncStatus.UNKNOWN)
src_model = self.src_diffsync.get(self.model_class, ids)
except ObjectNotFound:
model = None
src_model = None

changed, modified_model = self.sync_model(model, ids, attrs)
model = modified_model or model
# Retrieve Dest (and primary) Object
dst_model: Optional["DiffSyncModel"]
try:
dst_model = self.dst_diffsync.get(self.model_class, ids)
dst_model.set_status(DiffSyncStatus.UNKNOWN)
except ObjectNotFound:
dst_model = None

changed, modified_model = self.sync_model(src_model=src_model, dst_model=dst_model, ids=ids, attrs=attrs)
dst_model = modified_model or dst_model

if not modified_model or not model:
if not modified_model or not dst_model:
self.logger.warning("No object resulted from sync, will not process child objects.")
return changed

if self.action == DiffSyncActions.CREATE:
if self.action == DiffSyncActions.CREATE: # type: ignore
if parent_model:
parent_model.add_child(model)
self.dst_diffsync.add(model)
parent_model.add_child(dst_model)
self.dst_diffsync.add(dst_model)
elif self.action == DiffSyncActions.DELETE:
if parent_model:
parent_model.remove_child(model)
if model.model_flags & DiffSyncModelFlags.SKIP_CHILDREN_ON_DELETE:
# We don't need to process the child objects, but we do need to discard them from the dst_diffsync
self.dst_diffsync.remove(model, remove_children=True)
parent_model.remove_child(dst_model)

skip_children = bool(dst_model.model_flags & DiffSyncModelFlags.SKIP_CHILDREN_ON_DELETE)
self.dst_diffsync.remove(dst_model, remove_children=skip_children)

if skip_children:
return changed
self.dst_diffsync.remove(model)

self.incr_elements_processed()

for child in element.get_children():
changed |= self.sync_diff_element(child, parent_model=model)
changed |= self.sync_diff_element(child, parent_model=dst_model)

return changed

def sync_model(
self, model: Optional["DiffSyncModel"], ids: Mapping, attrs: Mapping
def sync_model( # pylint: disable=too-many-branches, unused-argument
self, src_model: Optional["DiffSyncModel"], dst_model: Optional["DiffSyncModel"], ids: Mapping, attrs: Mapping
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like src_model is an unused variable now, so can we revert the changes to this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference would be to keep it in case we want to make a decision on a source flag in the future.
I think it was a miss in the design of this function

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point.

) -> Tuple[bool, Optional["DiffSyncModel"]]:
"""Create/update/delete the current DiffSyncModel with current ids/attrs, and update self.status and self.message.

Expand All @@ -387,27 +405,27 @@ def sync_model(
status = DiffSyncStatus.SUCCESS
message = "No changes to apply; no action needed"
self.log_sync_status(self.action, status, message)
return (False, model)
return (False, dst_model)

try:
self.logger.debug(f"Attempting model {self.action.value}")
if self.action == DiffSyncActions.CREATE:
if model is not None:
if dst_model is not None:
raise ObjectNotCreated(f"Failed to create {self.model_class.get_type()} {ids} - it already exists!")
model = self.model_class.create(diffsync=self.dst_diffsync, ids=ids, attrs=attrs)
dst_model = self.model_class.create(diffsync=self.dst_diffsync, ids=ids, attrs=attrs)
elif self.action == DiffSyncActions.UPDATE:
if model is None:
if dst_model is None:
raise ObjectNotUpdated(f"Failed to update {self.model_class.get_type()} {ids} - not found!")
model = model.update(attrs=attrs)
dst_model = dst_model.update(attrs=attrs)
elif self.action == DiffSyncActions.DELETE:
if model is None:
if dst_model is None:
raise ObjectNotDeleted(f"Failed to delete {self.model_class.get_type()} {ids} - not found!")
model = model.delete()
dst_model = dst_model.delete()
else:
raise ObjectCrudException(f'Unknown action "{self.action}"!')

if model is not None:
status, message = model.get_status()
if dst_model is not None:
status, message = dst_model.get_status()
else:
status = DiffSyncStatus.FAILURE
message = f"{self.model_class.get_type()} {self.action.value} did not return the model object."
Expand All @@ -422,7 +440,7 @@ def sync_model(

self.log_sync_status(self.action, status, message)

return (True, model)
return (True, dst_model)

def log_sync_status(self, action: Optional[DiffSyncActions], status: DiffSyncStatus, message: str):
"""Log the current sync status at the appropriate verbosity with appropriate context.
Expand Down
3 changes: 3 additions & 0 deletions docs/source/core_engine/01-flags.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ class MyAdapter(DiffSync):
|---|---|---|
| IGNORE | Do not render diffs containing this model; do not make any changes to this model when synchronizing. Can be used to indicate a model instance that exists but should not be changed by DiffSync. | 0b1 |
| SKIP_CHILDREN_ON_DELETE | When deleting this model, do not recursively delete its children. Can be used for the case where deletion of a model results in the automatic deletion of all its children. | 0b10 |
| SKIP_UNMATCHED_SRC | Ignore the model if it only exists in the source/"from" DiffSync when determining diffs and syncing. If this flag is set, no new model will be created in the target/"to" DiffSync. | 0b100 |
| SKIP_UNMATCHED_DST | Ignore the model if it only exists in the target/"to" DiffSync when determining diffs and syncing. If this flag is set, the model will not be deleted from the target/"to" DiffSync. | 0b1000 |
| SKIP_UNMATCHED_BOTH | Convenience value combining both SKIP_UNMATCHED_SRC and SKIP_UNMATCHED_DST into a single flag | 0b1100 |

## Working with flags

Expand Down
43 changes: 43 additions & 0 deletions tests/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,23 @@ def delete(self):
return super().delete() # type: ignore


class ExceptionModelMixin:
"""Test class that always throws exceptions when creating/updating/deleting instances."""

@classmethod
def create(cls, diffsync: DiffSync, ids: Mapping, attrs: Mapping):
"""As DiffSyncModel.create(), but always throw exceptions."""
raise NotImplementedError

def update(self, attrs: Mapping):
"""As DiffSyncModel.update(), but always throw exceptions."""
raise NotImplementedError

def delete(self):
"""As DiffSyncModel.delete(), but always throw exceptions."""
raise NotImplementedError


class Site(DiffSyncModel):
"""Concrete DiffSyncModel subclass representing a site or location that contains devices."""

Expand Down Expand Up @@ -300,6 +317,32 @@ def error_prone_backend_a():
return diffsync


class ExceptionSiteA(ExceptionModelMixin, SiteA): # pylint: disable=abstract-method
"""A Site that always throws exceptions."""


class ExceptionDeviceA(ExceptionModelMixin, DeviceA): # pylint: disable=abstract-method
"""A Device that always throws exceptions."""


class ExceptionInterface(ExceptionModelMixin, Interface): # pylint: disable=abstract-method
"""An Interface that always throws exceptions."""


class ExceptionDeviceBackendA(BackendA):
"""A variant of BackendA that always fails to create/update/delete Device objects."""

device = ExceptionDeviceA


@pytest.fixture
def exception_backend_a():
"""Provide an instance of ExceptionBackendA subclass of DiffSync."""
diffsync = ExceptionDeviceBackendA()
diffsync.load()
return diffsync


class SiteB(Site):
"""Extend Site with a `places` list."""

Expand Down
Loading