Abort the synchronization task if no data source exists #209
Conversation
No. We cannot reliably guarantee it. |
| // If no data source is specified, we should abort the synchronization task. | ||
| guard let latest = source ?? latestDataSource(forKey: key) else { | ||
| Self.logKeySyncStatus(key, source: .local, syncStatus: .abort, value: nil) | ||
| return |
There was a problem hiding this comment.
Should this be continue instead? Do we really want to cancel all sync tasks just because the first key does not have a timestamp?
There was a problem hiding this comment.
Yeah, you're absolutely right, many thanks for catching this 🙏 .
Also add a test for it.
| return .local | ||
| case (nil, .some(_)): | ||
| .local | ||
| case let (.some(localTimestamp), .some(remoteTimestamp)): |
There was a problem hiding this comment.
This is inverted.
Make sure to add a test for this case that would have failed.
There was a problem hiding this comment.
My fault, thought it was already covered by the tests.
Added a test for latestDataSource.
The code coverage for Defaults.iCloud has increased to 90% now.
| let latest = source ?? latestDataSource(forKey: key) | ||
| // If no data source is specified, we should abort the synchronization task. | ||
| guard let latest = source ?? latestDataSource(forKey: key) else { | ||
| Self.logKeySyncStatus(key, source: .local, syncStatus: .abort, value: nil) |
There was a problem hiding this comment.
| Self.logKeySyncStatus(key, source: .local, syncStatus: .abort, value: nil) | |
| Self.logKeySyncStatus(key, source: nil, syncStatus: .abort, value: nil) |
?
| @@ -193,6 +193,7 @@ extension Defaults.iCloud { | |||
| } | |||
|
|
|||
| private enum SyncStatus { | |||
| case abort | |||
There was a problem hiding this comment.
| case abort | |
| case aborted |
?
And should be the last case.
hank121314
force-pushed
the
fix/abort-nil-dat-source
branch
2 times, most recently
from
5d5513e to
12bfc4b
Compare
hank121314
force-pushed
the
fix/abort-nil-dat-source
branch
from
12bfc4b to
5fc5dd8
Compare
Description
This PR fixes #206.
Previously, when a user newly installed the app, we would create some synchronization tasks.
Since the app was newly installed, both the remote and local data sources did not have a timestamp record, so we would sync the local data source to the remote one, which might override values in iCloud.
With this PR, we will abort the synchronization task if both the remote and local data sources do not have a timestamp record.
This prevents unintended data overrides and allows developers to wait for iCloud synchronization instead (call
synchronizeon app launch and wait fordidChangeExternallyNotification).Implementation
Defaults.iCloud.latestDataSourcereturn an optional. If it returnsnil, we should abort the synchronization task.SyncStatus.abortin the logger to indicate that the key synchronization has been aborted.Discussion
Should we provide a function that ensures iCloud is synced?
ex.