fix(datastore): save metadata when merging even if mutation outbox has pending item (#1319)

Author: richardmcclellan

aws-api-appsync/src/main/java/com/amplifyframework/datastore/appsync/SerializedModel.java

@@ -151,7 +151,7 @@ public String toString() {
         return "SerializedModel{" +
             "id='" + modelId + '\'' +
             ", serializedData=" + serializedData +
-            ", modelSchema=" + modelSchema +
+            ", modelSchema=" + modelSchema.getName() +
             '}';
     }
 

aws-api/src/androidTest/java/com/amplifyframework/api/aws/CodeGenerationInstrumentationTest.java

@@ -32,7 +32,7 @@
 import com.amplifyframework.testmodels.ratingsblog.User;
 import com.amplifyframework.testmodels.teamproject.Projectfields;
 import com.amplifyframework.testmodels.teamproject.Team;
-import com.amplifyframework.testutils.FieldValue;
+import com.amplifyframework.testutils.ModelAssert;
 import com.amplifyframework.testutils.sync.SynchronousApi;
 
 import org.junit.BeforeClass;
@@ -88,19 +88,11 @@ public void queryMatchesMutationResult() throws ApiException, ParseException {
             .relationship(MaritalStatus.married)
             .build();
         Person createdPerson = api.create(PERSON_API_NAME, david);
-        try {
-            // API creates model with "createdAt" and "updatedAt" fields, which local models don't have.
-            // Manually write these in.
-            FieldValue.set(david, "createdAt", createdPerson.getCreatedAt());
-            FieldValue.set(david, "updatedAt", createdPerson.getUpdatedAt());
-        } catch (NoSuchFieldException | IllegalAccessException error) {
-            // Failure to override these fields will fail the assertion anyways
-        }
-        assertEquals(david, createdPerson);
+        ModelAssert.assertEqualsIgnoringTimestamps(david, createdPerson);
 
         // Query for that created person, expect him to be there
         Person queriedPerson = api.get(PERSON_API_NAME, Person.class, createdPerson.getId());
-        // Do NOT override createdAt/updatedAt fields here to confirm that synced items have same values.
+        // Do NOT ignore createdAt/updatedAt fields here to confirm that synced items have same values.
         assertEquals(createdPerson, queriedPerson);
     }
 

aws-datastore/src/androidTest/java/com/amplifyframework/datastore/AppSyncClientInstrumentationTest.java

@@ -42,7 +42,7 @@
 import com.amplifyframework.testmodels.commentsblog.Post;
 import com.amplifyframework.testmodels.commentsblog.PostStatus;
 import com.amplifyframework.testutils.Await;
-import com.amplifyframework.testutils.FieldValue;
+import com.amplifyframework.testutils.ModelAssert;
 import com.amplifyframework.testutils.Resources;
 
 import org.junit.BeforeClass;
@@ -89,12 +89,10 @@ public static void onceBeforeTests() throws AmplifyException {
      * Tests the operations in AppSyncClient.
      * @throws DataStoreException If any call to AppSync endpoint fails to return a response
      * @throws AmplifyException On failure to obtain ModelSchema
-     * @throws NoSuchFieldException On failure to null out createdAt, updatedAt fields in response
-     * @throws IllegalAccessException On failure to null out createdAt, updatedAt fields in response
      */
     @Test
     @SuppressWarnings("MethodLength")
-    public void testAllOperations() throws AmplifyException, NoSuchFieldException, IllegalAccessException {
+    public void testAllOperations() throws AmplifyException {
         ModelSchema blogOwnerSchema = ModelSchema.fromModelClass(BlogOwner.class);
         ModelSchema postSchema = ModelSchema.fromModelClass(Post.class);
         ModelSchema blogSchema = ModelSchema.fromModelClass(Blog.class);
@@ -108,12 +106,7 @@ public void testAllOperations() throws AmplifyException, NoSuchFieldException, I
         ModelWithMetadata<BlogOwner> blogOwnerCreateResult = create(owner, blogOwnerSchema);
         BlogOwner actual = blogOwnerCreateResult.getModel();
 
-        // The response from AppSync has createdAt and updatedAt fields.  We can't actually know what values to expect
-        // beforehand, so updated the "expected" object with the "actual" values.
-        FieldValue.set(owner, "createdAt", actual.getCreatedAt());
-        FieldValue.set(owner, "updatedAt", actual.getUpdatedAt());
-
-        assertEquals(owner, actual);
+        ModelAssert.assertEqualsIgnoringTimestamps(owner, actual);
         assertEquals(new Integer(1), blogOwnerCreateResult.getSyncMetadata().getVersion());
         // TODO: BE AWARE THAT THE DELETED PROPERTY RETURNS NULL INSTEAD OF FALSE
         assertNull(blogOwnerCreateResult.getSyncMetadata().isDeleted());
@@ -163,18 +156,16 @@ public void testAllOperations() throws AmplifyException, NoSuchFieldException, I
             .blog(blog)
             .build();
         Post post1ModelResult = create(post1, postSchema).getModel();
-        FieldValue.set(post1, "createdAt", post1ModelResult.getCreatedAt());
         Post post2ModelResult = create(post2, postSchema).getModel();
-        FieldValue.set(post2, "createdAt", post2ModelResult.getCreatedAt());
 
         // Results only have blog ID so strip out other information from the original post blog
-        assertEquals(
+        ModelAssert.assertEqualsIgnoringTimestamps(
             post1.copyOfBuilder()
                 .blog(Blog.justId(blog.getId()))
                 .build(),
             post1ModelResult
         );
-        assertEquals(
+        ModelAssert.assertEqualsIgnoringTimestamps(
             post2.copyOfBuilder()
                 .blog(Blog.justId(blog.getId()))
                 .build(),
@@ -201,7 +192,7 @@ public void testAllOperations() throws AmplifyException, NoSuchFieldException, I
 
         // Delete one of the posts
         ModelWithMetadata<Post> post1DeleteResult = delete(post1, postSchema, 1);
-        assertEquals(
+        ModelAssert.assertEqualsIgnoringTimestamps(
             post1.copyOfBuilder()
                 .blog(Blog.justId(blog.getId()))
                 .build(),

aws-datastore/src/androidTest/java/com/amplifyframework/datastore/BasicCloudSyncInstrumentationTest.java

@@ -44,6 +44,7 @@
 import com.amplifyframework.testmodels.commentsblog.Post;
 import com.amplifyframework.testmodels.commentsblog.PostAuthorJoin;
 import com.amplifyframework.testutils.HubAccumulator;
+import com.amplifyframework.testutils.ModelAssert;
 import com.amplifyframework.testutils.Resources;
 import com.amplifyframework.testutils.sync.SynchronousApi;
 import com.amplifyframework.testutils.sync.SynchronousDataStore;
@@ -199,4 +200,42 @@ public void syncDownFromCloudIsWorking() throws AmplifyException {
         assertEquals("Jameson Williams", owner.getName());
         assertEquals(jameson.getId(), owner.getId());
     }
+
+    /**
+     * Verify that updating an item shortly after creating it succeeds.  This can be tricky because the _version
+     * returned in the response from the create request must be included in the input for the subsequent update request.
+     * @throws DataStoreException On failure to save or query items from DataStore.
+     * @throws ApiException On failure to query the API.
+     */
+    @Test
+    public void updateAfterCreate() throws DataStoreException, ApiException {
+        // Setup
+        BlogOwner richard = BlogOwner.builder()
+                .name("Richard")
+                .build();
+        BlogOwner updatedRichard = richard.copyOfBuilder()
+                .name("Richard McClellan")
+                .build();
+        String modelName = BlogOwner.class.getSimpleName();
+
+        // Expect two mutations to be published to AppSync.
+        HubAccumulator richardAccumulator =
+            HubAccumulator.create(HubChannel.DATASTORE, publicationOf(modelName, richard.getId()), 2)
+                .start();
+
+        // Create an item, then update it and save it again.
+        dataStore.save(richard);
+        dataStore.save(updatedRichard);
+
+        // Verify that 2 mutations were published.
+        richardAccumulator.await(30, TimeUnit.SECONDS);
+
+        // Verify that the updatedRichard is saved in the DataStore.
+        BlogOwner localRichard = dataStore.get(BlogOwner.class, richard.getId());
+        ModelAssert.assertEqualsIgnoringTimestamps(updatedRichard, localRichard);
+
+        // Verify that the updatedRichard is saved on the backend.
+        BlogOwner remoteRichard = api.get(BlogOwner.class, richard.getId());
+        ModelAssert.assertEqualsIgnoringTimestamps(updatedRichard, remoteRichard);
+    }
 }

aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/Merger.java

@@ -90,12 +90,6 @@ <T extends Model> Completable merge(
             int incomingVersion = metadata.getVersion() == null ? -1 : metadata.getVersion();
             T model = modelWithMetadata.getModel();
 
-            // Check if there is a pending mutation for this model, in the outbox.
-            if (mutationOutbox.hasPendingMutation(model.getId())) {
-                LOG.info("Mutation outbox has pending mutation for " + model.getId() + ", refusing to merge.");
-                return Completable.complete();
-            }
-
             return versionRepository.findModelVersion(model)
                 .onErrorReturnItem(-1)
                 // If the incoming version is strictly less than the current version, it's "out of date,"
@@ -105,10 +99,17 @@ <T extends Model> Completable merge(
                 // and the version would get bumped up.
                 .filter(currentVersion -> currentVersion == -1 || incomingVersion > currentVersion)
                 // If we should merge, then do so now, starting with the model data.
-                .flatMapCompletable(shouldMerge ->
-                        (isDelete ? delete(model, changeTypeConsumer) : save(model, changeTypeConsumer))
-                                .andThen(save(metadata, NoOpConsumer.create()))
-                )
+                .flatMapCompletable(shouldMerge -> {
+                    Completable firstStep;
+                    if (mutationOutbox.hasPendingMutation(model.getId())) {
+                        LOG.info("Mutation outbox has pending mutation for " + model.getId()
+                            + ". Saving the metadata, but not model itself.");
+                        firstStep = Completable.complete();
+                    } else {
+                        firstStep = (isDelete ? delete(model, changeTypeConsumer) : save(model, changeTypeConsumer));
+                    }
+                    return firstStep.andThen(save(metadata, NoOpConsumer.create()));
+                })
                 // Let the world know that we've done a good thing.
                 .doOnComplete(() -> {
                     announceSuccessfulMerge(modelWithMetadata);
@@ -132,7 +133,6 @@ <T extends Model> Completable merge(
             long duration = System.currentTimeMillis() - startTime.get();
             LOG.verbose("Merged a single item in " + duration + " ms.");
         });
-
     }
 
     /**

aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/PendingMutation.java

@@ -246,7 +246,6 @@ public int hashCode() {
     public String toString() {
         return "PendingMutation{" +
             "mutatedItem=" + mutatedItem +
-            ", modelSchema=" + modelSchema +
             ", mutationType=" + mutationType +
             ", mutationId=" + mutationId +
             ", predicate=" + predicate +

aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/PersistentMutationOutbox.java

@@ -283,6 +283,9 @@ private IncomingMutationConflictHandler(@NonNull PendingMutation<T> existing,
          * @return A completable with the actions to resolve the conflict.
          */
         Completable resolve() {
+            LOG.debug("IncomingMutationConflict - "
+                + " existing " + existing.getMutationType()
+                + " incoming " + incoming.getMutationType());
             switch (incoming.getMutationType()) {
                 case CREATE:
                     return handleIncomingCreate();

testutils/src/main/java/com/amplifyframework/testutils/FieldValue.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package com.amplifyframework.testutils;
+
+import com.amplifyframework.core.model.Model;
+
+import org.junit.Assert;
+
+import java.lang.reflect.Field;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A utility for comparing {@class Model} objects.
+ */
+public final class ModelAssert {
+    private ModelAssert() {}
+
+    /**
+     * Asserts whether two models are equal, ignoring the createdAt and updatedAt values.  This method modifies the
+     * timestamp fields (createdAt and updatedAt) on the expected object, with the values, from the actual, does the
+     * assert, and then sets them back to the original values at the end, so that this method doesn't have side effects.
+     * @param expected expected value
+     * @param actual the value to check against <code>expected</code>
+     * @param <T> type of model
+     */
+    public static <T extends Model> void assertEqualsIgnoringTimestamps(T expected, T actual) {
+        Map<String, Object> originalValues = new HashMap<>();
+        for (String fieldName : Arrays.asList("createdAt", "updatedAt")) {
+            try {
+                Field privateField = actual.getClass().getDeclaredField(fieldName); // can throw NoSuchFieldException
+                privateField.setAccessible(true);
+                Object actualValue = privateField.get(actual); // can throw IllegalAccessException
+                Object expectedValue = privateField.get(expected); // can throw IllegalAccessException
+                privateField.set(expected, actualValue); // can throw IllegalAccessException
+                // if we got here, we successfully updated the expected object, so we should remember the original
+                // expected value, so we can restore it at the end.
+                originalValues.put(fieldName, expectedValue);
+            } catch (NoSuchFieldException exception) {
+                // Field doesn't exist, just proceed with assertEquals
+            } catch (IllegalAccessException exception) {
+                // Field was not accessible, just proceed by calling assertEquals anyway.
+            }
+        }
+
+        // Do the assert!
+        Assert.assertEquals(expected, actual);
+
+        // Reset values back to original, so that this method has no side effects.
+        for (String fieldName : originalValues.keySet()) {
+            try {
+                Field privateField = actual.getClass().getDeclaredField(fieldName); // can throw NoSuchFieldException
+                privateField.setAccessible(true);
+                privateField.set(expected, originalValues.get(fieldName)); // can throw IllegalAccessException
+            } catch (NoSuchFieldException exception) {
+                // Field doesn't exist.  Shouldn't happen, since it didn't happen above.
+            } catch (IllegalAccessException exception) {
+                // Field was not accessible.  Shouldn't happen, since it didn't happen above.
+            }
+        }
+    }
+}