Return sync result from create/update/delete methods instead of returning the

[jamesharr]

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 or None 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.

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) 2.0 Tracking #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?

[Kircheneer]

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?

[glennmatthews]

I haven't thought it through in depth, but conceptually it feels reasonable to me. :)

[Dav-C]

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.

[Kircheneer]

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