Add new model flags #87

dgarros · 2021-12-16T19:03:59Z

Fixes #85 & #49

This PR adds 6 new model flags

SKIP_UNMATCHED_SRC : Ignore the model if it only exists in the source/"from" DiffSync when determining diffs and syncing.
SKIP_UNMATCHED_DST : Ignore the model if it only exists in the target/"to" DiffSync when determining diffs and syncing.
SKIP_UNMATCHED_BOTH: SKIP_UNMATCHED_SRC | SKIP_UNMATCHED_DST
CRUD_NO_UPDATE: Do not call update() on the DiffSyncModel during sync(), the model and the changes will still be visible in the diff.
CRUD_NO_DELETE: Do not call delete() on the DiffSyncModel during sync(), the model and the changes will still be visible in the diff.
CRUD_NO_UPDATE_DELETE = CRUD_NO_UPDATE | CRUD_NO_DELETE

The new model flags SKIP_UNMATCHED_* provide the same behavior as the global flags with the same name.

The CRUD_* model flags provide a way to disable some CURD method selectively.
Since, in the current design the create/update/delete method must execute the base methods with super() I had to move the logic in the base methods into a new method create|update|delete_base() in order to not execute the user code but still keep the same behavior internally. Not sure if we should recommend the user to call new base methods now or if it's best to keep using super()

Also one of the unit test file got too long (1000 lines) so I had to split it into 2 files

TODO

code
unit tests
update documentation

glennmatthews · 2021-12-17T21:27:21Z

The _base methods feel clunky to me. I think in general we should (possibly as a breaking "2.0" change) refine the create/update/delete methods to accept **kwargs so that we can add additional passthrough parameters here and the future without it being a breaking change again.

diffsync/helpers.py

tests/unit/test_diffsync_model_flags.py

glennmatthews · 2021-12-22T22:12:43Z

diffsync/enum.py

+
+    SKIP_UNMATCHED_BOTH = SKIP_UNMATCHED_SRC | SKIP_UNMATCHED_DST
+
+    CRUD_NO_UPDATE = 0b10000


Shouldn't we have a CRUD_NO_CREATE flag (to be set on the "source" side) as well?

Relatedly, should we allow for CRUD_NO_UPDATE to be set on either the "source" or "target" model instances, and prevent changes in either case?

I thought about it too but I'm not sure if this is possible to have flags defined on the source side currently because we are only passing the dest model to the sync_model method.

I think it would require us to retrieve the src_model from the src_diffsync and pass it to sync_model(). I can give it a try but if it turns out to be too complicated I might change my mind

I've made the changes mentioned previously, I think it's cleaner overall

glennmatthews · 2021-12-22T22:15:40Z

tests/unit/test_diffsync_model_flags.py

+
+    exception_backend_a.sync_from(backend_a)
+    assert "nyc-spine1" in exception_backend_a.get(exception_backend_a.site, "nyc").devices
+    assert "nyc-spine1__eth0" not in exception_backend_a.get(exception_backend_a.device, "nyc-spine1").interfaces


It's a little surprising to me that CRUD_NO_DELETE by itself doesn't prevent deletion of child objects. I can sort of understand requiring to also set SKIP_CHILDREN_ON_DELETE but it would be good to document this really clearly.

Doc update

| CRUD_NO_DELETE | Do not delete and do not call delete() on the DiffSyncModel during sync(), the model and the changes will still be visible in the diff. This flag do not impact the children of this object, it's recommended to either set the flag on all objects individually or set the flag SKIP_CHILDREN_ON_DELETE | 0b1000000 |

diffsync/__init__.py

dgarros · 2021-12-23T20:52:57Z

@glennmatthews I've made all the suggested changes and I've updated the doc, should be good for a final review, thanks

tests/unit/test_diffsync_model_flags.py

glennmatthews · 2021-12-23T21:06:33Z

tests/unit/test_diffsync_model_flags.py

+    diff = backend_a.diff_from(backend_a_with_extra_models)
+    assert diff.summary() == {"create": 1, "update": 0, "delete": 0, "no-change": 23}
+
+    # Check that only the parent object is affected by the flag not the children


Don't we need some diffs on the child objects for this to actually test what the comment is saying? Also, this statement seems at odds with the behavior of SKIP_UNMATCHED_DST (which does result in child objects being skipped as well).

Good point, the comment wasn't clear nor useful, I removed it

tests/unit/test_diffsync_model_flags.py

glennmatthews · 2021-12-23T21:11:12Z

docs/source/core_engine/01-flags.md

+| SKIP_UNMATCHED_BOTH | Convenience value combining both SKIP_UNMATCHED_SRC and SKIP_UNMATCHED_DST into a single flag | 0b1100 |
+| CRUD_NO_CREATE | Do not create and do not call create() on the DiffSyncModel during sync(), the model and the changes will still be visible in the diff. Because the destination model doesn't exist before calling create(), the flag need to be positioned on the source object. | 0b10000 |
+| CRUD_NO_UPDATE | Do not update and do not call update() on the DiffSyncModel during sync(), the model and the changes will still be visible in the diff. This flag can be positioned on both source and destination objects | 0b100000 |
+| CRUD_NO_DELETE | Do not delete and do not call delete() on the DiffSyncModel during sync(), the model and the changes will still be visible in the diff. This flag do not impact the children of this object, it's recommended to either set the flag on all objects individually or set the flag SKIP_CHILDREN_ON_DELETE | 0b1000000 |


Looking at these flags side by side, it seems like there's some overlap. Is the only difference between SKIP_UNMATCHED_DST and CRUD_NO_DELETE that the former applies to both diff and sync while the latter applies only to sync? (If so, that would probably be helpful to state explicitly.) Same question for SKIP_UNMATCHED_SRC versus CRUD_NO_CREATE?

Yes that's correct, good point
it might be best to rename these flags

CRUD_NO_DELETE > SKIP_SYNC_UNMATCHED_DST

CRUD_NO_CREATE > SKIP_SYNC_UNMATCHED_SRC

CRUD_NO_UPDATE > SKIP_SYNC_UPDATE

CRUD_NO_UPDATE_DELETE change to SKIP_SYNC_ALL and add SKIP_SYNC_UNMATCHED_SRC to
what do you think ? happy to use different names if you have something better

After more discussion we decided to remove all CRUD_* flags for now.
the same behavior can be achieve by setting different flags during a diff or a sync

glennmatthews · 2022-01-21T19:43:00Z

diffsync/__init__.py

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


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?

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

DiffSyncModel

NetworkImporterModel

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

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. :-)

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

glennmatthews · 2022-01-21T19:46:03Z

diffsync/helpers.py

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


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

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

Fair point.

docs/source/core_engine/01-flags.md

dgarros requested a review from glennmatthews December 16, 2021 19:03

dgarros changed the title ~~Add model new flags~~ Add new model flags Dec 16, 2021

glennmatthews reviewed Dec 17, 2021

View reviewed changes

diffsync/helpers.py Outdated Show resolved Hide resolved

glennmatthews reviewed Dec 22, 2021

View reviewed changes

glennmatthews reviewed Dec 23, 2021

View reviewed changes

diffsync/__init__.py Outdated Show resolved Hide resolved

glennmatthews reviewed Dec 23, 2021

View reviewed changes

glennmatthews mentioned this pull request Jan 4, 2022

Create a new release on Pypi to include new changes #88

Closed

glennmatthews reviewed Jan 21, 2022

View reviewed changes

glennmatthews previously approved these changes Jan 21, 2022

View reviewed changes

Add SKIP model flags

4466bbe

dgarros dismissed glennmatthews’s stale review via 4466bbe January 24, 2022 16:27

dgarros force-pushed the dga-model-flags branch from 152c2a6 to 4466bbe Compare January 24, 2022 16:27

Fix log message

51877c9

dgarros requested a review from glennmatthews January 24, 2022 16:58

glennmatthews approved these changes Jan 24, 2022

View reviewed changes

dgarros merged commit 6a412de into main Jan 24, 2022

dgarros mentioned this pull request Jan 25, 2022

Option to skip deletion of records on a case-by-case basis #49

Closed

jdrew82 deleted the dga-model-flags branch March 27, 2025 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new model flags #87

Add new model flags #87

dgarros commented Dec 16, 2021 •

edited

Loading

glennmatthews commented Dec 17, 2021

glennmatthews Dec 22, 2021

dgarros Dec 23, 2021

dgarros Dec 23, 2021

glennmatthews Dec 22, 2021

dgarros Dec 23, 2021

dgarros commented Dec 23, 2021

glennmatthews Dec 23, 2021

dgarros Jan 21, 2022

glennmatthews Dec 23, 2021

dgarros Jan 7, 2022

dgarros Jan 21, 2022

glennmatthews Jan 21, 2022

dgarros Jan 21, 2022

glennmatthews Jan 21, 2022

dgarros Jan 24, 2022

glennmatthews Jan 21, 2022

dgarros Jan 21, 2022

glennmatthews Jan 21, 2022


		SKIP_UNMATCHED_BOTH = SKIP_UNMATCHED_SRC \| SKIP_UNMATCHED_DST

		CRUD_NO_UPDATE = 0b10000

Add new model flags #87

Add new model flags #87

Conversation

dgarros commented Dec 16, 2021 • edited Loading

TODO

glennmatthews commented Dec 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarros commented Dec 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarros commented Dec 16, 2021 •

edited

Loading