DataStore.save() with condition #355

lawmicha · 2020-03-12T15:39:22Z

Description of changes:

This adds the condition argument to the DataStore.save() for local storage.

Scenario 1: the local store does not contain model, and there is a condition pased in, fail with conditional save failed.

Scenario 2: the local store contains the model, but the condition does not match the existing instance, fail with conditional save failed

Scenario 3: the local storage contains the model, and the condition matches the existing instance, update the model with the provided fields with the condition in the where clause, preventing any race condition between checkiing that the condition matches existing instance and actually updating it.

Details

Add Codable conformance to QueryPredicate to allow encodinig/decoding to JSON, stored in MutationEvent. This is accomplished with AnyQueryPredicate for encoding array of predicates, and custom Codable for QueryOperator.
Storing queryPredicateJson in MutationEvent to allow decoding to QueryPredicate for 1. sending API requests with condition input on create/update mutations, and 2. handle conditional mutations in the AWSMutationDatabaseAdapter: MutationEventIngester.
AWSMutationDatabaseAdapter resolves conflicts for incoming candidate mutation events with conditions by saving the candidate. (does not drop/replace local with candidate, etc)
Added a ProcessMutationErrorFromCloudOperation to check errors for "conditional failed event", query for latest, and storage it back to local.

TODO:

emit failed events to the hub

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

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/StorageEngine.swift

drochetti

Did an first pass and left a couple of comments.

I believe the missing part here is since the condition needs to be sent to the Sync Engine later on, we will need to change the MutationEvent to save the condition as well. This will require a bit of work since now the conditions will need to be Codable and converted to/from JSON when interacting with the MutationEvent. See the existing model in the MutationEvent for reference.

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/StorageEngine.swift

AmplifyTests/CategoryTests/DataStore/DataStoreCategoryConfigurationTests.swift

...Plugins/DataStore/AWSDataStoreCategoryPluginTests/Core/SQLiteStorageEngineAdapterTests.swift

…uccessful responnse with graphql errors. If the graphQL error is conditional failed, then retrieve the latest remote model, and apply it to local storage.

…after possibly exeuction of ProcessMutationErrorFromCloudOperation. - Added unit test for ProcessMutationErrorFromCloudOperation

lawmicha · 2020-03-25T21:27:21Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Support/Utils/GraphQLResponseDecoder.swift

+                if data.count == 1, let first = data.first, case .null = first.value {
+                    let responseErrors = try decodeErrors(graphQLErrors: errors)
+                    return GraphQLResponse<R>.failure(.error(responseErrors))
+                }


this was added due to graphql response that looks like

{ "data": { "updatePost": null }, "errors": { ... } }

not too happy with this class, needs another pass on refactoring and better unit tests

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/StorageEngine.swift

- AnyModelTester class rename - unit test parallelization fix - add correct error from StorageEngine for save()

…version of QueryPredicate into JSON string

drochetti · 2020-03-25T21:09:37Z

Amplify/Categories/DataStore/Subscribe/MutationEvent.swift

    }

    public init<M: Model>(model: M,
                          mutationType: MutationType,
-                          version: Int? = nil) throws {
+                          version: Int? = nil,
+                          queryPredicate: QueryPredicate? = nil) throws {


I believe the model should stay a pure and simple value holding data structure. Therefore I don't think we need this initializer nor any initializer in models should throw. The encoding/decoding logic should happen outside, where the model is initialized (just like with the json property)

this code looks a bit outdated, it should be taking in queryPredicateJson: String? = nil

it is now updated to graphQLFilterJSON

Amplify/Categories/DataStore/DataStoreError.swift

Amplify/Categories/DataStore/Subscribe/MutationEvent+Schema.swift

drochetti · 2020-03-30T21:40:00Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/QueryPredicate+GraphQL.swift

+extension String {
+
+    /// Deserialize the JSON string converted with `QueryPredicate.toGraphQLFilterString()` to `[String: Any]` JSON
+    public func toGraphQLFilterJson() throws -> [String: Any] {


This should not be a public extension.

I believe we've been using JSON instead of Json

This should not be a public extension.

isn't this public for DataStore to turn it back into GraphQLFilter? that is unless we change the GraphQLRequest extension methods to take in graphQLFilterJSON String and then internally do graphQLFilterJSON.toGraphQLFiilter()

My point is that this is a generic String -> JSON function, added as an extension to String. Also the semantics are not entire correct. For example: "{a:1}". toGraphQLFilterJson() would return a valid JSON as [String: Any] but it's not a valid GraphQL filter.

Therefore, if the extension is toJSON(), that could be valid. Then the discussion is if we should expose that extension to customers or not (it seems outside of the scope of Amplify to add such extension).

taking a look at this to see if we can reduce the access level or at least move it under a static function of a different class rather than extension on String

i agree, the string extension is incorrect in this case, and is prone to errors, and don't think we want to take on the toJSON() implementation. Updated to GraphQLFilterConverter with static methods for turning into a GraphQLFilter JSON and back to a GraphQLFilter

drochetti · 2020-03-30T21:43:26Z

AmplifyPlugins/Core/AWSPluginsCoreTests/Model/Support/QueryPredicateGraphQLTests.swift

-    func toGraphQLFilterJSON() throws -> String {
+    func toGraphQLFilterPrettyPrintedJson() throws -> String {


minor: if you have only one toJSON function, I think if it's pretty printed or not can be omitted from the function name

this was to differentiate between the code's toGraphQLFilterJSON and test code's toGraphQLFilterJSON which does pretty print for test case assertion, i think what we can do here is in actual code, have optional options and allow test to leverage the parameter by passing in options = [.prettyPrinted]

not sure I understand in actual code, have optional options. If in tests it's always pretty printed, then no options should be needed. Do the extensions clash? Is that why you need another toJSON function?

yes the reason for the different naming in test code is the clashing of method names. i wanted toGraphQLFilterJSON with pretty print options set because our test relies on asserting against a hand crafted pretty printed version of the graphql filter json. "actual code" i meant the production code

drochetti · 2020-03-30T21:46:31Z

Amplify/Categories/DataStore/DataStoreCategoryBehavior.swift

@@ -15,6 +15,7 @@ public protocol DataStoreBaseBehavior {

    /// Saves the model to storage. If sync is enabled, also initiates a sync of the mutation to the remote API


this method just got a new layer of complexity, not need to address it right now but I believe we need to add proper API documentation to DataStore in general. I'll create an issue to track it.

AmplifyPlugins/Core/AWSPluginsCore/Model/Decorator/PredicateDecorator.swift

… error naming, removed QueryPredicateInput, updated MutationEvent's field as graphQLFiilterJSON

AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLRequest/GraphQLRequest+AnyModelWithSync.swift

lawmicha · 2020-04-02T20:11:48Z

Tracking issue regarding dead code: #370

drochetti

great job man, thanks for the thoughtful work and follow up :)

        /\/\

                        /\/\
         O  O  O
                 O
                __|__
                || ||_____
                || ||    |
     --------------------------
      \   O   O   O   O      /
 ~ ~ ~ ~ ~ ~ ~ ~  ~ ~ ~ ~ ~ ~ ~ ~
  ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~

lawmicha requested review from drochetti and wooj2 March 12, 2020 15:39

lawmicha force-pushed the feature/datastore-conditional-update branch from 8d38c08 to ae7e816 Compare March 12, 2020 15:40

lawmicha mentioned this pull request Mar 17, 2020

DataStore.save() with condition for remote storage #362

Merged

lawmicha commented Mar 18, 2020

View reviewed changes

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/StorageEngine.swift Outdated Show resolved Hide resolved

lawmicha self-assigned this Mar 23, 2020

lawmicha force-pushed the feature/datastore-conditional-update branch from ae7e816 to 723ae70 Compare March 23, 2020 14:55

Adding condition to save()

d631c28

lawmicha force-pushed the feature/datastore-conditional-update branch from 723ae70 to d631c28 Compare March 23, 2020 16:26

lawmicha changed the title ~~DataStore.save() with condition~~ DataStore.save() with condition for local storage Mar 23, 2020

lawmicha added 2 commits March 23, 2020 09:36

Adding condition to save()

1ff9951

Add predicate to mutation event

72381d1

drochetti suggested changes Mar 23, 2020

View reviewed changes

lawmicha added 2 commits March 24, 2020 09:03

QueryPredicate Codable conformance

06aabe3

add ingestor disposition logic

91f45de

lawmicha changed the title ~~DataStore.save() with condition for local storage~~ DataStore.save() with condition Mar 24, 2020

lawmicha added 5 commits March 24, 2020 10:55

test for conditional request failed

10f1b84

Add ProcessMutationErrorFromCloudOperation to check when there is a s…

6d7db20

…uccessful responnse with graphql errors. If the graphQL error is conditional failed, then retrieve the latest remote model, and apply it to local storage.

Adding simple AnyQueryPredicate test

7b60681

- Moved delete mutation event out of SyncMutationToCloudOperation to …

298d7d8

…after possibly exeuction of ProcessMutationErrorFromCloudOperation. - Added unit test for ProcessMutationErrorFromCloudOperation

Add E2E test for conditional save failed

3d263bc

lawmicha commented Mar 25, 2020

View reviewed changes

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/StorageEngine.swift Outdated Show resolved Hide resolved

lawmicha added 5 commits March 25, 2020 14:38

minor changes

4a5d409

- AnyModelTester class rename - unit test parallelization fix - add correct error from StorageEngine for save()

Removed Codable conformance for QueryPredicate and serialize graphQL …

4f8fab9

…version of QueryPredicate into JSON string

remove fetch and save when processing conditional save failed

8a38635

clean up, api tests, plugin core tests

f76ec94

minor comment update

95013e7

drochetti suggested changes Mar 30, 2020

View reviewed changes

lawmicha force-pushed the feature/datastore-conditional-update branch from 0357ad7 to 27a75e8 Compare March 31, 2020 18:40

lawmicha force-pushed the feature/datastore-conditional-update branch from 27a75e8 to 8977e88 Compare March 31, 2020 18:42

Code clean up - use GraphQLFilter typealias, 'JSON' casing, DataStore…

f09e606

… error naming, removed QueryPredicateInput, updated MutationEvent's field as graphQLFiilterJSON

lawmicha force-pushed the feature/datastore-conditional-update branch from 8977e88 to f09e606 Compare April 1, 2020 16:16

drochetti reviewed Apr 2, 2020

View reviewed changes

AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLRequest/GraphQLRequest+AnyModelWithSync.swift Outdated Show resolved Hide resolved

AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLRequest/GraphQLRequest+AnyModelWithSync.swift Outdated Show resolved Hide resolved

rename parameter to filter

01c3979

Move GraphQLFilter serialization under GraphQLFilterConverter

60c71c3

drochetti approved these changes Apr 2, 2020

View reviewed changes

lawmicha merged commit 5bd3e65 into master Apr 3, 2020

lawmicha deleted the feature/datastore-conditional-update branch April 6, 2020 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataStore.save() with condition #355

DataStore.save() with condition #355

lawmicha commented Mar 12, 2020 •

edited

Loading

drochetti left a comment

lawmicha Mar 25, 2020

drochetti Mar 25, 2020

lawmicha Mar 31, 2020

lawmicha Mar 31, 2020

drochetti Mar 30, 2020

drochetti Mar 30, 2020

lawmicha Mar 31, 2020

drochetti Apr 2, 2020

lawmicha Apr 2, 2020

lawmicha Apr 2, 2020

drochetti Mar 30, 2020

lawmicha Mar 31, 2020

drochetti Apr 2, 2020

lawmicha Apr 2, 2020

drochetti Mar 30, 2020

lawmicha commented Apr 2, 2020

drochetti left a comment

		func toGraphQLFilterJSON() throws -> String {
		func toGraphQLFilterPrettyPrintedJson() throws -> String {

		@@ -15,6 +15,7 @@ public protocol DataStoreBaseBehavior {

		/// Saves the model to storage. If sync is enabled, also initiates a sync of the mutation to the remote API

DataStore.save() with condition #355

DataStore.save() with condition #355

Conversation

lawmicha commented Mar 12, 2020 • edited Loading

drochetti left a 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

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

Choose a reason for hiding this comment

lawmicha commented Apr 2, 2020

drochetti left a comment

Choose a reason for hiding this comment

lawmicha commented Mar 12, 2020 •

edited

Loading