Subscriptions, State machine for ReconcileAndLocalSaveOperation #149

palpatim · 2019-11-23T01:57:04Z

Notes V2

Removed handling of incoming mutations; we'll get those from our subscriptions. Derp.
PR feedback

Notes V1

Added a ReconcileAndLocalSaveOperation that will handle any incoming cloud sync events, whether from mutations or subscriptions.
Refactored OutgoingMutationQueue to be based on Operation/OperationQueue rather than Combine streams. Logic is much simpler now.
Added new StateMachine class that we can promote to an AWSPluginsCore support type if it looks useful in other contexts
Added a trivial DefaultLogger support protocol to provide a default log property.
- NOTE: This often breaks in tests right now because Logger is uninitialized during Amplify.reset. Tracked in Make Logging and Hub categories available at initialization #161
Updated handling of optional fields during Model updates
Hardcoded a _deleted conflict resolution field alongside the previously-existing _version field. We'll remove those once @drochetti finishes conflict resolution

Note to reviewers

Many of the integration tests added in this PR are broken due to errors in connections handling. Once codegen/CLI changes are pushed, and #164 is merged, I'll dive deeper and get those fixed.

@drochetti Could you please pay special attention to the changes I made to model & schema, especially the optional field handling in
AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/Model+SQLite.swift sqlValues(for fields:)? Unit tests are passing, but I haven't added additional tests to cover this case and I want to get another set of eyes on this to validate my thinking.

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

- Remove AWSDataStoreCategoryPlugin references that got reinstated after merge

- Initial subscriptions are now performed at startup

…t resolution handling

- Mutation queue is now an OperationQueue instead of a Combine stream - Add 'unknown' case to DataStoreError - Fix SQLite tests and disable init logging for SQLiteStorageEngineAdapter

* master: API Predicate to GraphQL filter variables (#150) model associations: refactor + missing use cases (#153) Basic unit tests for comprehend and translate (#160) CoreML logic to Detect faces (#155) API Category - Updates to APIError and GraphQLResponse (#133) Initial commit for unit test for translate service behavior (#158) bug fix change (#152)

* master: fix API model association (#163)

palpatim

Self review to set file comparison checkpoint

palpatim · 2019-11-26T17:27:42Z

Note to reviewers: I will move DefaultLogger to Amplify core in a subsequent PR.

…agated to our subscriptions

Amplify/Core/Configuration/Internal/Amplify+Reset.swift

wooj2 · 2019-11-26T17:24:04Z

.../DataStore/AWSDataStoreCategoryPlugin/AWSDataStoreCategoryPlugin+DataStoreBaseBehavior.swift

+                if models.count > 1 {
+                    completion(.failure(.nonUniqueResult(model: modelType.modelName, count: count)))
+                } else {
+                    completion(.success(models.first))
+                }


seems like we should reverse this in the event that models is empty.

if models.count == 1 { completion(.success(... } else { ... }

Upon reflection I think I'll change it to a guard to really capture the intent that this is an error case.

Upon even more reflection, I now remember why I did it this way: a 0-count model array is also valid!

switch to the rescue:

switch models.count { case 0, 1: completion(.success(models.first)) default: completion(.failure(.nonUniqueResult(model: modelType.modelName, count: models.count))) }

wooj2 · 2019-11-26T18:17:13Z

Amplify/Categories/DataStore/DataStoreCategory+HubPayloadEventName.swift

-    /// mutated data from the service.
-    static let subscriptionReceived = "DataStore.subscriptionReceived"
+    /// Dispatched when DataStore receives a sync response from the cloud via the API category. This event does not
+    /// define the source of the event--it could be in response to either a subscription or a locally-sourced mutation.


Currently this we seem to only be using the variable in the subscription flow (ReconcileAndLocalSaveOperation.swift) -- maybe it would be better to have two separate variables -- one so that customers can listen to subscription events, and one for when location mutations are made.

I thought about that, but the comment explains why I don't think it's a meaningful distinction: any incoming event could potentially overwrite the data in the local store, regardless of source. What would the customer use case be for ignoring one type over the other, given that either could result in a change?

Amplify/Categories/DataStore/Model/Model+Syncable.swift

.../DataStore/AWSDataStoreCategoryPlugin/AWSDataStoreCategoryPlugin+DataStoreBaseBehavior.swift

...ifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/IncomingEventReconciliationQueues.swift

wooj2 · 2019-11-26T21:27:19Z

...ore/AWSDataStoreCategoryPlugin/Sync/MutationSync/Outgoing/SyncMutationToCloudOperation.swift

+//
+// SPDX-License-Identifier: Apache-2.0
+//
+


Hmm, we have
OutgoingMutationQueue.swift
and
SyncMutationToCloudOperation.swift

Maybe it would be clearer to rename these to:
OutgoingMutationOperationQueue.swift
and
OutgoingMutationOperation.swift

Hmm, the "outgoing mutation" includes saving the local mutation event as well as creating & enqueing the cloud sync operations. The SyncMutationToCloudOperation only does that one thing.

Part of the problem is that the OutgoingMutationQueue is doing too much, at too many different levels:

loading initial mutations from disk (infrastructure @ global scope)

Creating a sync operation from a loaded mutation event (infrastructure at local scope)

accepting incoming mutation submissions, creating a sync operation from it (transactional at system scope)

The SyncMutationToCloudOperation on the other hand is a little bit more targeted:

send to cloud (

validate response

notify

Rather than just rename the operation, I'd rather refactor the queue to do less itself & delegate more, and enforce a better separation of levels of responsibility. Thoughts?

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/ReconcileAndLocalSaveOperation.swift

...re/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/IncomingSubscriptionEventPublisher.swift

...WSDataStoreCategoryPlugin/Sync/SubscriptionSync/AsyncSubscriptionEventToAnyModelMapper.swift

wooj2

My comments are minor.. I'm largely good with broad framework for both outgoing/ingoing mutations as well as subscriptions. We can iterate from here! thanks!!

AmplifyPlugins/API/AWSAPICategoryPluginFunctionalTests/APICategoryPluginConcurrencyTests.swift

...gins/DataStore/AWSDataStoreCategoryPluginIntegrationTests/SubscriptionIntegrationTests.swift

palpatim · 2019-11-26T22:51:13Z

Addressed PR feedback (less a few open questions for further discussion). Merging now to serve as a base for upcoming changes--notably #161 to stop integration tests from crashing :(

palpatim requested a review from wooj2 November 23, 2019 01:57

WIP: Subscriptions, State machine for ReconcileAndLocalSaveOperation

7618217

- Remove AWSDataStoreCategoryPlugin references that got reinstated after merge

palpatim force-pushed the palpatim/api-sync branch from e4ec5b0 to 7618217 Compare November 24, 2019 17:23

palpatim added 12 commits November 24, 2019 09:32

API category extensions for Subscriptions to model protocol

7bf86a0

API Plugin extensions for Subscriptions to model protocol

56549ee

Add untyped Model flavor of StorageAdapter methods

01e8682

- Initial subscriptions are now performed at startup

WIP

1ffb470

Fix optional Persistable handling

0ba0b38

Added _deleted metadata field to Post while we work on adding conflic…

074d0e0

…t resolution handling

Save incoming mutation responses

1834058

- Mutation queue is now an OperationQueue instead of a Combine stream - Add 'unknown' case to DataStoreError - Fix SQLite tests and disable init logging for SQLiteStorageEngineAdapter

Disable post creation in subscription test while we troubleshoot

97627a2

Merge branch 'master' into palpatim/api-sync

513d574

* master: fix API model association (#163)

Update SQLStatementTests to account for _deleted field

0546010

Minor cleanup; changed Hub notification payload for sync events

743587b

palpatim requested a review from drochetti November 26, 2019 16:06

palpatim changed the title ~~WIP: Subscriptions, State machine for ReconcileAndLocalSaveOperation~~ Subscriptions, State machine for ReconcileAndLocalSaveOperation Nov 26, 2019

Removed unnecessary property

2839e3b

palpatim commented Nov 26, 2019

View reviewed changes

Removed unnecessary workQueue property

7d026f3

palpatim mentioned this pull request Nov 26, 2019

include associations in the GraphQL query selected fields #164

Merged

palpatim added 2 commits November 26, 2019 12:39

PR feedback

661efb2

Removed unnecessary handlers for incoming events, since they are prop…

09921cc

…agated to our subscriptions

palpatim requested a review from lawmicha November 26, 2019 20:51

Merge branch 'master' into palpatim/api-sync

a8e88f1

wooj2 reviewed Nov 26, 2019

View reviewed changes

wooj2 approved these changes Nov 26, 2019

View reviewed changes

lawmicha reviewed Nov 26, 2019

View reviewed changes

AmplifyPlugins/API/AWSAPICategoryPluginFunctionalTests/APICategoryPluginConcurrencyTests.swift Outdated Show resolved Hide resolved

lawmicha reviewed Nov 26, 2019

View reviewed changes

...gins/DataStore/AWSDataStoreCategoryPluginIntegrationTests/SubscriptionIntegrationTests.swift Outdated Show resolved Hide resolved

PR feedback

d7b2540

palpatim merged commit 0d5b48f into master Nov 26, 2019

palpatim deleted the palpatim/api-sync branch November 27, 2019 03:03

waleedbutt pushed a commit to hassan31/amplify-swift that referenced this pull request Sep 18, 2024

chore: bump the amplify package version (aws-amplify#149)

54ac57c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions, State machine for ReconcileAndLocalSaveOperation #149

Subscriptions, State machine for ReconcileAndLocalSaveOperation #149

palpatim commented Nov 23, 2019 •

edited

Loading

palpatim left a comment

palpatim commented Nov 26, 2019

wooj2 Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

wooj2 Nov 26, 2019

palpatim Nov 26, 2019

wooj2 Nov 26, 2019

palpatim Nov 26, 2019

wooj2 left a comment

palpatim commented Nov 26, 2019

Subscriptions, State machine for ReconcileAndLocalSaveOperation #149

Subscriptions, State machine for ReconcileAndLocalSaveOperation #149

Conversation

palpatim commented Nov 23, 2019 • edited Loading

palpatim left a comment

Choose a reason for hiding this comment

palpatim commented Nov 26, 2019

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

wooj2 left a comment

Choose a reason for hiding this comment

palpatim commented Nov 26, 2019

palpatim commented Nov 23, 2019 •

edited

Loading