Return sync result from create/update/delete methods instead of returning the
jamesharr opened this issue · 4 comments
Environment
- DiffSync version: 1.9.0
Proposed Functionality
In 1.9.0, the results of a sync are stored as a field in DiffSyncModel, and for most users this status is automatically set when calling the super() of the create()/update()/delete() methods.
There are a few things that are awkward about this API:
- Providing a custom success message requires setting the status twice, once by using super().create/update/delete, and then again by using self.set_status() to override what was put there by default.
- create/update/delete are required to return
self
for success orNone
for an error (and to skip children).- In the success case, it's unclear as to why
self
needs to be returned. IE: why is this just not a bool? - In the failure case, it's unclear how to provide a failure message.
- In the success case, it's unclear as to why
At a conceptual level, it's also a bit weird to have the status stored on an object. In particular, how do you report the result of a delete()? If that object is no longer in the destination backend, then how can I read the result of that after a sync process? I think this is ultimately because the result is not a property of an object, it's the result of a process.
This Feature Request proposes:
- That the sync status is stored in the Diff (or part of the diff tree) instead of DiffSyncModel
- That the status be an object instead of two distinct values (status enum and message)
- That the status can be sub-classed by diffsync users to add additional context
- That the create/update/delete signature in DiffSyncModel be altered to return the status instead of the object
- That the Diff tree provides a public API to read the status & message
Misc notes
- It's likely this will be an appropriate issue for (2.0) #232
- The only storage engine I use is the Local (in-memory), it's unclear to me if this has an impact on the Redis storage engine.
Use Case
# DiffSync code
class SyncResult(pydantic.BaseModel):
status: DiffSyncStatus
message: str
skip_children: bool = False # Set to True to skip updating children
# User code example 1 - providing custom values
class MyModelA(DiffSyncModel):
@classmethod
def create(cls, diffsync:DiffSync, ids:Dict[str, any], attrs:Dict[str, any]) -> Tuple[Self, SyncResult]:
return cls(**ids, **atts), SyncResult(DiffSyncStatus.SUCCESS, "Sync successful")
def update(self, attrs:Dict[str,any]) -> SyncResult:
... # perform update
super().update(attrs) # Returns a successful SyncResult, but we can craft our own
return SyncResult(DiffSyncStatus.SUCCESS, "with immense difficulty")
def delete(self) -> SyncResult:
return SyncResult(DiffSyncStatus.ERROR, "Delete is not implemented")
# User code example 2 - simple user experience
class MyModelB(DiffSyncModel):
@classmethod
def create(cls, diffsync:DiffSync, ids:Dict[str, any], attrs:Dict[str, any]) -> Tuple[Self, SyncResult]:
return cls(**ids, **atts)
# I'm struggling to figure out an elegant way to provide the user experience that exists today,
# which is to make it easy to report success. This isn't ideal, but one thought is:
# - If a single-value is returned, the returned value is the object created and success is assumed
# - If a 2-tuple is returned: interpret the first value as the object created, and the second value is the SyncResult
def update(self, attrs:Dict[str,any]) -> SyncResult:
...
return super().update(**attrs) # DiffSyncModel.update() returns a success object by default
def delete(self) -> SyncResult:
return super().delete() # DiffSyncModel.delete() returns a success object by default.
# User code example 3 - augmented result to add additional fields
class CustomSyncResult(diffsync.SyncResult):
nso_dry_run_result: str
class MyModelC(DiffSyncModel):
@classmethod
def create(cls, diffsync:DiffSync, ids:Dict[str, any], attrs:Dict[str, any]) -> Tuple[Self, SyncResult]:
dry_run_text = nso.dry_run(...)
status = CustomSyncResult(DiffSyncStatus.SUCCESS, "Sync successful", nso_dry_run_result=dry_run_text)
obj = cls(**ids, **atts)
return obj, status
def update(self, attrs:Dict[str,any]) -> SyncResult:
dry_run_text = nso.dry_run(...)
super().update(attrs)
return CustomSyncResult(DiffSyncStatus.SUCCESS, "Sync successful", nso_dry_run_result=dry_run_text)
def delete(self) -> SyncResult:
return CustomSyncResult(DiffSyncStatus.SUCCESS, "Sync successful", nso_dry_run_result=dry_run_text)
The most straight forward way for a user to read the result of a status is to embed that status object into the DiffElement.
class DiffElement:
status: SyncStatus # possibly | None
- After a typical sync_to/sync_from, this should be an actual SyncStatus.
- Point of discussion: After a diff_to/diff_from, I'm not sure if this field should be left as None or if the diff should just report success. I lean slightly towards reporting a SUCCESS/UNKNOWN value here, since that would produce a consistent API
- Point of discussion: If children is skipped because the parent took care of deleting the child objects, what should this report? The Parent's status? No status? Unknown status? A special other status?
I support the idea of returning a sync result rather then the object itself, I think this would make error handling more straight-forward. @glennmatthews/@Dav-C - any opinion here?
I haven't thought it through in depth, but conceptually it feels reasonable to me. :)
Is anything lost by not returning the object? I think the current implementation loosely follows the REST paradigm where the object is returned for immediate use and the lack of a returned object is an error that should be handled. In any case, I think the return from all the methods should be consistent to avoid confusion.
When calling update
or delete
, you are calling on the class instance itself, so you have the instance available in 100% of cases. The proposal for create
returns a tuple with the class and the SyncResult
, so that's covered as well