Initial sync startup/3-way merge #238

palpatim · 2019-12-06T00:49:22Z

Initial implementation of InitialSyncOrchestrator, integrated with
ReconciliationQueue to apply incoming sync query results
Added a buffer queue for incoming remote events to ReconciliationQueue, so
remote API sync can be turned off independently of applying mutations from
sync queries
Made CancelAwareBlockOperation a standalone class
Model extensions
- Change ModelSchema+Syncable to ModelSchema+Attributes
- Added 'ModelSchema.isSystem' convenience getter
- Change 'hasSyncableModels' to look for the existence of any non-system
  models; all models are considered syncable by default (no local model support
  at launch)
- Added ModelGraphs to track Model connection dependencies for initial sync
Made AtomicValue public so it can be used in plugins; added convenience
methods for RangeReplaceableCollection
Removed isSyncable schema attribute in favor of designating all non-system
models as syncable
Added buffer queue for incoming subscription event
submission to reconciliation queue
Added 'DataStoreError.internalOperation' to contain errors about nil weak
references being unavailable
Added convenience DataStoreError constructor for missing reconciliation queue
Fix some linting warnings
Test cleanup throughout
Added responders to MockAPICategoryPlugin to let us invoke callback of methods
Convert ModelRegistry.register to take protocol argument rather than concrete type

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- Initial implementation of InitialSyncOrchestrator, integrated with ReconciliationQueue to apply incoming sync query results - Added a buffer queue for incoming remote events to ReconciliationQueue, so remote API sync can be turned off indpendently of applying mutations from sync queries - Made CancelAwareBlockOperation a standalone class - Model extensions - Change ModelSchema+Syncable to ModelSchema+Attributes - Added 'ModelSchema.isSystem' convenience getter - Change 'hasSyncableModels' to look for the existence of any non-system models; all models are considered syncable by default (no local model support at launch) - Added ModelGraphs to track Model connection dependencies for initial sync - Made AtomicValue public so it can be used in plugins; added convenience methods for RangeReplaceableCollection - Removed isSyncable schema attribute in favor of designating all non-system models as syncable; added buffer queue for incoming subscription event submission to reconciliation queue; refactoring & renaming - Added 'DataStore.internalOperation' to contain errors about nil weak references being unavailable - Added convenience DataStoreError constructor for missing reconciliation queue

- Fix some linting warnings - Minor test cleanup - Added responders to MockAPICategoryPlugin to let us invoke callback of methods - Added unit tests for InitialSyncOperation happy path

…concrete type - InitialSyncOrchestrator unit tests

palpatim

Self review to set checkpoint

lawmicha

leaving some initial comments and opening issue for API category's Model based GraphQL APIs dependency on ModelRegistry's isSyncable method #239

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOperation.swift

lawmicha · 2019-12-06T23:18:25Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/StorageEngine.swift

        if #available(iOS 13, *) {
            let syncEngine = isSyncEnabled ? try? RemoteSyncEngine(storageAdapter: storageAdapter) : nil
-            self.init(storageAdapter: storageAdapter, syncEngine: syncEngine, isSyncEnabled: isSyncEnabled)
+            self.init(storageAdapter: storageAdapter, syncEngine: syncEngine)
        } else {
-            self.init(storageAdapter: storageAdapter, syncEngine: nil, isSyncEnabled: isSyncEnabled)
+            self.init(storageAdapter: storageAdapter, syncEngine: nil)
        }


is there a way to check if Amplify.API is configured here? if it's iOS13 && Amplify.API.isConfigured then create a RemoveSyncEngine with storageAdapter and Amplify.API. I would have imagined RemoteSyncEngine to always need an API when being constructed.

I don't have a real good use case to expose isConfigured at the Category level except that whenever pluginKey's do not match the plugin's key, then the plugin that was added to Amplify doesn't get called on the plugin's configure() method, which developer will then find out it is not configured at plugin's API calls. And also in this case where plugins are seeing if another plugin is configured. DataStore checking if API for sync, API checking for DataStore for Model based GraphQL calls,

is there a way to check if Amplify.API is configured here?

Not yet, but it's something we need to figure out. Currently I'm of two minds:

Expose an explicit isConfigured check on the category, as you discuss, and note it as internal in the docs. I also can't think of a good customer-facing use case, but I don't know of a way to expose an internal property across modules without resorting to Objective-C.

Do away with isConfigured checks altogether except if the customer adds a plugin. Other categories would be fulfilled by no-op operations that invoke errors on their callbacks as soon as they're created. That adds more runtime complexity to the Amplify core, though.

We're going to have to answer this question once and for all, and soon, since our cross-category use cases are going to grow: Predictions->Storage (upload an image to Storage so Predictions can identify entities in it); or DataStore->Storage (complex object support); or Analytics as a "middleware" plugin that intercepts Hub events and auto-logs messages.

Amplify.xcodeproj/project.pbxproj

Amplify/Core/Support/AsychronousOperation.swift

Amplify/Categories/DataStore/Model/ModelRegistry.swift

...tore/AWSDataStoreCategoryPlugin/Sync/MutationSync/MutationEvent/MutationEventPublisher.swift

...oreCategoryPlugin/Sync/MutationSync/OutgoingMutationQueue/SyncMutationToCloudOperation.swift

AmplifyPlugins/DataStore/AWSDataStoreCategoryPluginTests/Core/ModelGraphsTests.swift

AmplifyPlugins/DataStore/AWSDataStoreCategoryPluginTests/TestSupport/SyncEngineTestBase.swift

wooj2 · 2019-12-07T00:16:52Z

Pods/Pods.xcodeproj/project.pbxproj

@@ -28,290 +28,296 @@
 /* End PBXAggregateTarget section */

 /* Begin PBXBuildFile section */
-		004DE85F3AE8FFE0816925652FFFBB75 /* AWSCognitoCredentialsProvider+Extension.h in Headers */ = {isa = PBXBuildFile; fileRef = 953E533AB3626686BC0DCFEE4DCDBCC9 /* AWSCognitoCredentialsProvider+Extension.h */; settings = {ATTRIBUTES = (Public, ); }; };


I've typically avoided commiting this file, as well as the .lock files... do we need to commit these files?

We should have a team discussion about this. Guidance from CocoaPods (and my personal preference) is that we should commit Pods, manifest & lock to the repo, as they are necessary for non-CocoaPods systems (e.g., Carthage) to build. However, we haven't propagated that pattern into the plugin folders, and I'd like to understand other points of view.

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift

...CategoryPlugin/Sync/SubscriptionSync/ReconcileAndLocalSave/AWSModelReconciliationQueue.swift

palpatim

Responding to PR comments

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOperation.swift

palpatim · 2019-12-09T15:07:52Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/StorageEngine.swift

        if #available(iOS 13, *) {
            let syncEngine = isSyncEnabled ? try? RemoteSyncEngine(storageAdapter: storageAdapter) : nil
-            self.init(storageAdapter: storageAdapter, syncEngine: syncEngine, isSyncEnabled: isSyncEnabled)
+            self.init(storageAdapter: storageAdapter, syncEngine: syncEngine)
        } else {
-            self.init(storageAdapter: storageAdapter, syncEngine: nil, isSyncEnabled: isSyncEnabled)
+            self.init(storageAdapter: storageAdapter, syncEngine: nil)
        }


is there a way to check if Amplify.API is configured here?

Not yet, but it's something we need to figure out. Currently I'm of two minds:

Expose an explicit isConfigured check on the category, as you discuss, and note it as internal in the docs. I also can't think of a good customer-facing use case, but I don't know of a way to expose an internal property across modules without resorting to Objective-C.

Do away with isConfigured checks altogether except if the customer adds a plugin. Other categories would be fulfilled by no-op operations that invoke errors on their callbacks as soon as they're created. That adds more runtime complexity to the Amplify core, though.

We're going to have to answer this question once and for all, and soon, since our cross-category use cases are going to grow: Predictions->Storage (upload an image to Storage so Predictions can identify entities in it); or DataStore->Storage (complex object support); or Analytics as a "middleware" plugin that intercepts Hub events and auto-logs messages.

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOperation.swift

...tore/AWSDataStoreCategoryPlugin/Sync/MutationSync/MutationEvent/MutationEventPublisher.swift

...oreCategoryPlugin/Sync/MutationSync/OutgoingMutationQueue/SyncMutationToCloudOperation.swift

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift

AmplifyPlugins/DataStore/AWSDataStoreCategoryPluginTests/TestSupport/SyncEngineTestBase.swift

palpatim added 3 commits December 5, 2019 11:14

Use code-generated models in AmplifyTestCommon

1c06828

- Fix some linting warnings - Minor test cleanup - Added responders to MockAPICategoryPlugin to let us invoke callback of methods - Added unit tests for InitialSyncOperation happy path

Convert ModelRegistry.register to take protocol argument rather than …

b4e9602

…concrete type - InitialSyncOrchestrator unit tests

palpatim added the datastore Issues related to the DataStore category label Dec 6, 2019

palpatim requested review from drochetti, lawmicha and wooj2 December 6, 2019 00:49

palpatim commented Dec 6, 2019

View reviewed changes

lawmicha reviewed Dec 6, 2019

View reviewed changes

wooj2 reviewed Dec 7, 2019

View reviewed changes

palpatim commented Dec 9, 2019

View reviewed changes

lawmicha mentioned this pull request Dec 9, 2019

Model based APIs for API Category check against existence of DataStore #239

Closed

lawmicha approved these changes Dec 9, 2019

View reviewed changes

palpatim merged commit f4b0423 into master Dec 9, 2019

palpatim deleted the palpatim/3-way-merge branch December 9, 2019 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial sync startup/3-way merge #238

Initial sync startup/3-way merge #238

palpatim commented Dec 6, 2019

palpatim left a comment

lawmicha left a comment •

edited

Loading

lawmicha Dec 6, 2019 •

edited

Loading

palpatim Dec 9, 2019

wooj2 Dec 7, 2019

palpatim Dec 9, 2019

palpatim left a comment

palpatim Dec 9, 2019

Initial sync startup/3-way merge #238

Initial sync startup/3-way merge #238

Conversation

palpatim commented Dec 6, 2019

palpatim left a comment

Choose a reason for hiding this comment

lawmicha left a comment • edited Loading

Choose a reason for hiding this comment

lawmicha Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

palpatim Dec 9, 2019

Choose a reason for hiding this comment

wooj2 Dec 7, 2019

Choose a reason for hiding this comment

palpatim Dec 9, 2019

Choose a reason for hiding this comment

palpatim left a comment

Choose a reason for hiding this comment

palpatim Dec 9, 2019

Choose a reason for hiding this comment

lawmicha left a comment •

edited

Loading

lawmicha Dec 6, 2019 •

edited

Loading