From 3823f1d3535b66353edafe4664fac8102ae8063f Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 6 Mar 2019 11:47:27 -0800 Subject: [PATCH] Adding FieldValue.numericAdd() (#105) This PR adds support for the Numeric Add transform and follows the semantics of the backend implementation. --- .../firestore/NumericTransformsTest.java | 158 +++++++++++++ .../testutil/IntegrationTestUtil.java | 65 +++++- .../google/firebase/firestore/FieldValue.java | 57 ++++- .../firebase/firestore/UserDataConverter.java | 12 + .../firestore/local/LocalSerializer.java | 17 +- .../firebase/firestore/local/LocalStore.java | 62 +++++- .../firestore/local/MemoryMutationQueue.java | 5 +- .../firestore/local/MutationQueue.java | 13 +- .../firestore/local/SQLiteMutationQueue.java | 5 +- .../mutation/ArrayTransformOperation.java | 5 + .../model/mutation/DeleteMutation.java | 11 + .../firestore/model/mutation/FieldMask.java | 21 ++ .../model/mutation/FieldTransform.java | 4 + .../firestore/model/mutation/Mutation.java | 11 + .../model/mutation/MutationBatch.java | 63 +++++- .../NumericIncrementTransformOperation.java | 111 ++++++++++ .../model/mutation/PatchMutation.java | 11 + .../mutation/ServerTimestampOperation.java | 5 + .../firestore/model/mutation/SetMutation.java | 11 + .../model/mutation/TransformMutation.java | 21 ++ .../model/mutation/TransformOperation.java | 3 + .../firestore/remote/RemoteSerializer.java | 21 ++ .../firebase/firestore/proto/mutation.proto | 12 + .../firestore/local/LocalSerializerTest.java | 24 +- .../firestore/local/LocalStoreTestCase.java | 208 +++++++++++++++++- .../local/LruGarbageCollectorTestCase.java | 3 +- .../local/MutationQueueTestCase.java | 23 +- .../firestore/model/MutationTest.java | 171 +++++++++++++- 28 files changed, 1084 insertions(+), 49 deletions(-) create mode 100644 firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java new file mode 100644 index 00000000000..83f41e8109e --- /dev/null +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java @@ -0,0 +1,158 @@ +// Copyright 2018 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License 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.google.firebase.firestore; + +import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testDocument; +import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitFor; +import static com.google.firebase.firestore.testutil.TestUtil.map; +import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertFalse; + +import com.google.android.gms.tasks.Tasks; +import com.google.firebase.firestore.testutil.EventAccumulator; +import com.google.firebase.firestore.testutil.IntegrationTestUtil; +import java.util.Map; +import java.util.concurrent.ExecutionException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +public class NumericTransformsTest { + private static final double DOUBLE_EPSILON = 0.000001; + + // A document reference to read and write to. + private DocumentReference docRef; + + // Accumulator used to capture events during the test. + private EventAccumulator accumulator; + + // Listener registration for a listener maintained during the course of the test. + private ListenerRegistration listenerRegistration; + + @Before + public void setUp() { + docRef = testDocument(); + accumulator = new EventAccumulator<>(); + listenerRegistration = + docRef.addSnapshotListener(MetadataChanges.INCLUDE, accumulator.listener()); + + // Wait for initial null snapshot to avoid potential races. + DocumentSnapshot initialSnapshot = accumulator.await(); + assertFalse(initialSnapshot.exists()); + } + + @After + public void tearDown() { + listenerRegistration.remove(); + IntegrationTestUtil.tearDown(); + } + + /** Writes some initialData and consumes the events generated. */ + private void writeInitialData(Map initialData) { + waitFor(docRef.set(initialData)); + accumulator.awaitRemoteEvent(); + } + + private void expectLocalAndRemoteValue(double expectedSum) { + DocumentSnapshot snap = accumulator.awaitLocalEvent(); + assertEquals(expectedSum, snap.getDouble("sum"), DOUBLE_EPSILON); + snap = accumulator.awaitRemoteEvent(); + assertEquals(expectedSum, snap.getDouble("sum"), DOUBLE_EPSILON); + } + + private void expectLocalAndRemoteValue(long expectedSum) { + DocumentSnapshot snap = accumulator.awaitLocalEvent(); + assertEquals(expectedSum, (long) snap.getLong("sum")); + snap = accumulator.awaitRemoteEvent(); + assertEquals(expectedSum, (long) snap.getLong("sum")); + } + + @Test + public void createDocumentWithIncrement() { + waitFor(docRef.set(map("sum", FieldValue.increment(1337)))); + expectLocalAndRemoteValue(1337L); + } + + @Test + public void mergeOnNonExistingDocumentWithIncrement() { + waitFor(docRef.set(map("sum", FieldValue.increment(1337)), SetOptions.merge())); + expectLocalAndRemoteValue(1337L); + } + + @Test + public void integerIncrementWithExistingInteger() { + writeInitialData(map("sum", 1337L)); + waitFor(docRef.update("sum", FieldValue.increment(1))); + expectLocalAndRemoteValue(1338L); + } + + @Test + public void doubleIncrementWithExistingDouble() { + writeInitialData(map("sum", 13.37D)); + waitFor(docRef.update("sum", FieldValue.increment(0.1))); + expectLocalAndRemoteValue(13.47D); + } + + @Test + public void integerIncrementWithExistingDouble() { + writeInitialData(map("sum", 13.37D)); + waitFor(docRef.update("sum", FieldValue.increment(1))); + expectLocalAndRemoteValue(14.37D); + } + + @Test + public void doubleIncrementWithExistingInteger() { + writeInitialData(map("sum", 1337L)); + waitFor(docRef.update("sum", FieldValue.increment(0.1))); + expectLocalAndRemoteValue(1337.1D); + } + + @Test + public void integerIncrementWithExistingString() { + writeInitialData(map("sum", "overwrite")); + waitFor(docRef.update("sum", FieldValue.increment(1337))); + expectLocalAndRemoteValue(1337L); + } + + @Test + public void doubleIncrementWithExistingString() { + writeInitialData(map("sum", "overwrite")); + waitFor(docRef.update("sum", FieldValue.increment(13.37))); + expectLocalAndRemoteValue(13.37D); + } + + @Test + public void multipleDoubleIncrements() throws ExecutionException, InterruptedException { + writeInitialData(map("sum", 0.0D)); + + Tasks.await(docRef.getFirestore().disableNetwork()); + + docRef.update("sum", FieldValue.increment(0.1D)); + docRef.update("sum", FieldValue.increment(0.01D)); + docRef.update("sum", FieldValue.increment(0.001D)); + + DocumentSnapshot snap = accumulator.awaitLocalEvent(); + assertEquals(0.1D, snap.getDouble("sum"), DOUBLE_EPSILON); + snap = accumulator.awaitLocalEvent(); + assertEquals(0.11D, snap.getDouble("sum"), DOUBLE_EPSILON); + snap = accumulator.awaitLocalEvent(); + assertEquals(0.111D, snap.getDouble("sum"), DOUBLE_EPSILON); + + Tasks.await(docRef.getFirestore().enableNetwork()); + + snap = accumulator.awaitRemoteEvent(); + assertEquals(0.111D, snap.getDouble("sum"), DOUBLE_EPSILON); + } +} diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 8bc407a78ad..cf7c2be307d 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -20,6 +20,7 @@ import static junit.framework.Assert.fail; import android.content.Context; +import android.net.SSLCertificateSocketFactory; import android.support.test.InstrumentationRegistry; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; @@ -38,10 +39,12 @@ import com.google.firebase.firestore.local.Persistence; import com.google.firebase.firestore.local.SQLitePersistence; import com.google.firebase.firestore.model.DatabaseId; +import com.google.firebase.firestore.remote.Datastore; import com.google.firebase.firestore.testutil.provider.FirestoreProvider; import com.google.firebase.firestore.util.AsyncQueue; import com.google.firebase.firestore.util.Logger; import com.google.firebase.firestore.util.Logger.Level; +import io.grpc.okhttp.OkHttpChannelBuilder; import java.io.File; import java.util.ArrayList; import java.util.HashMap; @@ -56,6 +59,14 @@ /** A set of helper methods for tests */ public class IntegrationTestUtil { + // Whether the integration tests should run against a local Firestore emulator instead of the + // Production environment. Note that the Android Emulator treats "10.0.2.2" as its host machine. + // TODO(mrschmidt): Support multiple envrionments (Emulator, QA, Nightly, Production) + private static final boolean CONNECT_TO_EMULATOR = false; + + private static final String EMULATOR_HOST = "10.0.2.2"; + private static final int EMULATOR_PORT = 8081; + // Alternate project ID for creating "bad" references. Doesn't actually need to work. public static final String BAD_PROJECT_ID = "test-project-2"; @@ -80,11 +91,19 @@ public static FirestoreProvider provider() { } public static DatabaseInfo testEnvDatabaseInfo() { - return new DatabaseInfo( - DatabaseId.forProject(provider.projectId()), - "test-persistenceKey", - provider.firestoreHost(), - /*sslEnabled=*/ true); + if (CONNECT_TO_EMULATOR) { + return new DatabaseInfo( + DatabaseId.forProject(provider.projectId()), + "test-persistenceKey", + String.format("%s:%d", EMULATOR_HOST, EMULATOR_PORT), + /*sslEnabled=*/ true); + } else { + return new DatabaseInfo( + DatabaseId.forProject(provider.projectId()), + "test-persistenceKey", + provider.firestoreHost(), + /*sslEnabled=*/ true); + } } public static FirebaseFirestoreSettings newTestSettings() { @@ -94,11 +113,37 @@ public static FirebaseFirestoreSettings newTestSettings() { @SuppressWarnings("deprecation") // for setTimestampsInSnapshotsEnabled() public static FirebaseFirestoreSettings newTestSettingsWithSnapshotTimestampsEnabled( boolean enabled) { - return new FirebaseFirestoreSettings.Builder() - .setHost(provider.firestoreHost()) - .setPersistenceEnabled(true) - .setTimestampsInSnapshotsEnabled(enabled) - .build(); + FirebaseFirestoreSettings.Builder settings = new FirebaseFirestoreSettings.Builder(); + + if (CONNECT_TO_EMULATOR) { + settings.setHost(String.format("%s:%d", EMULATOR_HOST, EMULATOR_PORT)); + + // The `sslEnabled` flag in DatabaseInfo currently does not in fact disable all SSL checks. + // Instead, we manually disable the SSL certificate check and the hostname verification for + // connections to the emulator. + // TODO(mrschmidt): Update the client to respect the `sslEnabled` flag and remove these + // channel overrides. + OkHttpChannelBuilder channelBuilder = + new OkHttpChannelBuilder(EMULATOR_HOST, EMULATOR_PORT) { + @Override + protected String checkAuthority(String authority) { + return authority; + } + }; + channelBuilder.hostnameVerifier((hostname, session) -> true); + SSLCertificateSocketFactory insecureFactory = + (SSLCertificateSocketFactory) SSLCertificateSocketFactory.getInsecure(0, null); + channelBuilder.sslSocketFactory(insecureFactory); + + Datastore.overrideChannelBuilder(() -> channelBuilder); + } else { + settings.setHost(provider.firestoreHost()); + } + + settings.setPersistenceEnabled(true); + settings.setTimestampsInSnapshotsEnabled(enabled); + + return settings.build(); } /** Initializes a new Firestore instance that uses the default project. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FieldValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FieldValue.java index 0eb7780efba..d65788ecb66 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FieldValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FieldValue.java @@ -61,7 +61,7 @@ String getMethodName() { } List getElements() { - return this.elements; + return elements; } } @@ -79,7 +79,25 @@ String getMethodName() { } List getElements() { - return this.elements; + return elements; + } + } + + /* FieldValue class for increment() transforms. */ + static class NumericIncrementFieldValue extends FieldValue { + private final Number operand; + + NumericIncrementFieldValue(Number operand) { + this.operand = operand; + } + + @Override + String getMethodName() { + return "FieldValue.increment"; + } + + Number getOperand() { + return operand; } } @@ -134,4 +152,39 @@ public static FieldValue arrayUnion(@NonNull Object... elements) { public static FieldValue arrayRemove(@NonNull Object... elements) { return new ArrayRemoveFieldValue(Arrays.asList(elements)); } + + /** + * Returns a special value that can be used with set() or update() that tells the server to + * increment the field's current value by the given value. + * + *

If the current field value is an integer, possible integer overflows are resolved to + * Long.MAX_VALUE or Long.MIN_VALUE. If the current field value is a double, both values will be + * interpreted as doubles and the arithmetic will follow IEEE 754 semantics. + * + *

If the current field is not an integer or double, or if the field does not yet exist, the + * transformation will set the field to the given value. + * + * @return The FieldValue sentinel for use in a call to set() or update(). + */ + @NonNull + @PublicApi + public static FieldValue increment(long l) { + return new NumericIncrementFieldValue(l); + } + + /** + * Returns a special value that can be used with set() or update() that tells the server to + * increment the field's current value by the given value. + * + *

If the current value is an integer or a double, both the current and the given value will be + * interpreted as doubles and all arithmetic will follow IEEE 754 semantics. Otherwise, the + * transformation will set the field to the given value. + * + * @return The FieldValue sentinel for use in a call to set() or update(). + */ + @NonNull + @PublicApi + public static FieldValue increment(double l) { + return new NumericIncrementFieldValue(l); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java index 898a189fa3a..b3b77dd757f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java @@ -31,6 +31,7 @@ import com.google.firebase.firestore.model.FieldPath; import com.google.firebase.firestore.model.mutation.ArrayTransformOperation; import com.google.firebase.firestore.model.mutation.FieldMask; +import com.google.firebase.firestore.model.mutation.NumericIncrementTransformOperation; import com.google.firebase.firestore.model.mutation.ServerTimestampOperation; import com.google.firebase.firestore.model.value.ArrayValue; import com.google.firebase.firestore.model.value.BlobValue; @@ -40,6 +41,7 @@ import com.google.firebase.firestore.model.value.GeoPointValue; import com.google.firebase.firestore.model.value.IntegerValue; import com.google.firebase.firestore.model.value.NullValue; +import com.google.firebase.firestore.model.value.NumberValue; import com.google.firebase.firestore.model.value.ObjectValue; import com.google.firebase.firestore.model.value.ReferenceValue; import com.google.firebase.firestore.model.value.StringValue; @@ -349,6 +351,16 @@ private void parseSentinelFieldValue( ArrayTransformOperation arrayRemove = new ArrayTransformOperation.Remove(parsedElements); context.addToFieldTransforms(context.getPath(), arrayRemove); + } else if (value + instanceof com.google.firebase.firestore.FieldValue.NumericIncrementFieldValue) { + com.google.firebase.firestore.FieldValue.NumericIncrementFieldValue + numericIncrementFieldValue = + (com.google.firebase.firestore.FieldValue.NumericIncrementFieldValue) value; + NumberValue operand = (NumberValue) parseQueryValue(numericIncrementFieldValue.getOperand()); + NumericIncrementTransformOperation incrementOperation = + new NumericIncrementTransformOperation(operand); + context.addToFieldTransforms(context.getPath(), incrementOperation); + } else { throw Assert.fail("Unknown FieldValue type: %s", Util.typeName(value)); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java index 77880f007b9..5a07249ac52 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java @@ -165,6 +165,9 @@ com.google.firebase.firestore.proto.WriteBatch encodeMutationBatch(MutationBatch result.setBatchId(batch.getBatchId()); result.setLocalWriteTime(rpcSerializer.encodeTimestamp(batch.getLocalWriteTime())); + for (Mutation mutation : batch.getBaseMutations()) { + result.addBaseWrites(rpcSerializer.encodeMutation(mutation)); + } for (Mutation mutation : batch.getMutations()) { result.addWrites(rpcSerializer.encodeMutation(mutation)); } @@ -176,13 +179,17 @@ MutationBatch decodeMutationBatch(com.google.firebase.firestore.proto.WriteBatch int batchId = batch.getBatchId(); Timestamp localWriteTime = rpcSerializer.decodeTimestamp(batch.getLocalWriteTime()); - int count = batch.getWritesCount(); - List mutations = new ArrayList<>(count); - for (int i = 0; i < count; i++) { + int baseMutationsCount = batch.getBaseWritesCount(); + List baseMutations = new ArrayList<>(baseMutationsCount); + for (int i = 0; i < baseMutationsCount; i++) { + baseMutations.add(rpcSerializer.decodeMutation(batch.getBaseWrites(i))); + } + int mutationsCount = batch.getWritesCount(); + List mutations = new ArrayList<>(mutationsCount); + for (int i = 0; i < mutationsCount; i++) { mutations.add(rpcSerializer.decodeMutation(batch.getWrites(i))); } - - return new MutationBatch(batchId, localWriteTime, mutations); + return new MutationBatch(batchId, localWriteTime, baseMutations, mutations); } com.google.firebase.firestore.proto.Target encodeQueryData(QueryData queryData) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index dae56aafe29..5e5236e5f96 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -28,13 +28,18 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; import com.google.firebase.firestore.model.SnapshotVersion; +import com.google.firebase.firestore.model.mutation.FieldMask; import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationBatch; import com.google.firebase.firestore.model.mutation.MutationBatchResult; +import com.google.firebase.firestore.model.mutation.PatchMutation; +import com.google.firebase.firestore.model.mutation.Precondition; +import com.google.firebase.firestore.model.value.ObjectValue; import com.google.firebase.firestore.remote.RemoteEvent; import com.google.firebase.firestore.remote.TargetChange; import com.google.firebase.firestore.util.Logger; import com.google.protobuf.ByteString; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -183,16 +188,55 @@ public ImmutableSortedMap handleUserChange(User user /** Accepts locally generated Mutations and commits them to storage. */ public LocalWriteResult writeLocally(List mutations) { Timestamp localWriteTime = Timestamp.now(); + // TODO: Call queryEngine.handleDocumentChange() appropriately. - MutationBatch batch = - persistence.runTransaction( - "Locally write mutations", - () -> mutationQueue.addMutationBatch(localWriteTime, mutations)); - - Set keys = batch.getKeys(); - ImmutableSortedMap changedDocuments = - localDocuments.getDocuments(keys); - return new LocalWriteResult(batch.getBatchId(), changedDocuments); + + Set keys = new HashSet<>(); + for (Mutation mutation : mutations) { + keys.add(mutation.getKey()); + } + + return persistence.runTransaction( + "Locally write mutations", + () -> { + // Load and apply all existing mutations. This lets us compute the current base state for + // all non-idempotent transforms before applying any additional user-provided writes. + ImmutableSortedMap existingDocuments = + localDocuments.getDocuments(keys); + + // For non-idempotent mutations (such as `FieldValue.increment()`), we record the base + // state in a separate patch mutation. This is later used to guarantee consistent values + // and prevents flicker even if the backend sends us an update that already includes our + // transform. + List baseMutations = new ArrayList<>(); + for (Mutation mutation : mutations) { + MaybeDocument maybeDocument = existingDocuments.get(mutation.getKey()); + if (!mutation.isIdempotent()) { + // Theoretically, we should only include non-idempotent fields in this field mask as + // this mask is used to populate the base state for all DocumentTransforms. By + // including all fields, we incorrectly prevent rebasing of idempotent transforms + // (such as `arrayUnion()`) when any non-idempotent transforms are present. + FieldMask fieldMask = mutation.getFieldMask(); + if (fieldMask != null) { + ObjectValue baseValues = + (maybeDocument instanceof Document) + ? fieldMask.applyTo(((Document) maybeDocument).getData()) + : ObjectValue.emptyObject(); + // NOTE: The base state should only be applied if there's some existing + // document to override, so use a Precondition of exists=true + baseMutations.add( + new PatchMutation( + mutation.getKey(), baseValues, fieldMask, Precondition.exists(true))); + } + } + } + + MutationBatch batch = + mutationQueue.addMutationBatch(localWriteTime, baseMutations, mutations); + ImmutableSortedMap changedDocuments = + batch.applyToLocalDocumentSet(existingDocuments); + return new LocalWriteResult(batch.getBatchId(), changedDocuments); + }); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index f4b549a08fb..cc26fd450fe 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -127,7 +127,8 @@ public void setLastStreamToken(ByteString streamToken) { } @Override - public MutationBatch addMutationBatch(Timestamp localWriteTime, List mutations) { + public MutationBatch addMutationBatch( + Timestamp localWriteTime, List baseMutations, List mutations) { hardAssert(!mutations.isEmpty(), "Mutation batches should not be empty"); int batchId = nextBatchId; @@ -140,7 +141,7 @@ public MutationBatch addMutationBatch(Timestamp localWriteTime, List m prior.getBatchId() < batchId, "Mutation batchIds must be monotonically increasing order"); } - MutationBatch batch = new MutationBatch(batchId, localWriteTime, mutations); + MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseMutations, mutations); queue.add(batch); // Track references by document key. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java index c9fbbcdebf7..0dbab22b162 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java @@ -43,8 +43,17 @@ interface MutationQueue { /** Sets the stream token for this mutation queue. */ void setLastStreamToken(ByteString streamToken); - /** Creates a new mutation batch and adds it to this mutation queue. */ - MutationBatch addMutationBatch(Timestamp localWriteTime, List mutations); + /** + * Creates a new mutation batch and adds it to this mutation queue. + * + * @param localWriteTime The original write time of this mutation. + * @param baseMutations Mutations that are used to populate the base values when this mutation is + * applied locally. These mutations are used to locally overwrite values that are persisted in + * the remote document cache. + * @param mutations The user-provided mutations in this mutation batch. + */ + MutationBatch addMutationBatch( + Timestamp localWriteTime, List baseMutations, List mutations); /** Loads the mutation batch with the given batchId. */ @Nullable diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java index 9c41c101ab8..4f459550ef1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java @@ -179,11 +179,12 @@ private void writeMutationQueueMetadata() { } @Override - public MutationBatch addMutationBatch(Timestamp localWriteTime, List mutations) { + public MutationBatch addMutationBatch( + Timestamp localWriteTime, List baseMutations, List mutations) { int batchId = nextBatchId; nextBatchId += 1; - MutationBatch batch = new MutationBatch(batchId, localWriteTime, mutations); + MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseMutations, mutations); MessageLite proto = serializer.encodeMutationBatch(batch); db.execute( diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java index f8f1c7abd8c..c781129e99d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java @@ -88,6 +88,11 @@ static ArrayList coercedFieldValuesArray(@Nullable FieldValue value) } } + @Override + public boolean isIdempotent() { + return true; + } + /** An array union transform operation. */ public static class Union extends ArrayTransformOperation { public Union(List elements) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java index fb46cb8d6fb..b7a00c2571a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java @@ -82,4 +82,15 @@ public MaybeDocument applyToLocalView( return new NoDocument(getKey(), SnapshotVersion.NONE, /*hasCommittedMutations=*/ false); } + + @Nullable + @Override + public FieldMask getFieldMask() { + return null; + } + + @Override + public boolean isIdempotent() { + return true; + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java index 7b0f8ee9fb2..16573090fb5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java @@ -15,6 +15,8 @@ package com.google.firebase.firestore.model.mutation; import com.google.firebase.firestore.model.FieldPath; +import com.google.firebase.firestore.model.value.FieldValue; +import com.google.firebase.firestore.model.value.ObjectValue; import java.util.Set; /** @@ -69,6 +71,25 @@ public boolean covers(FieldPath fieldPath) { return false; } + /** + * Applies this field mask to the provided object value and returns an object that only contains + * fields that are specified in both the input object and this field mask. + */ + public ObjectValue applyTo(ObjectValue data) { + ObjectValue filteredObject = ObjectValue.emptyObject(); + for (FieldPath path : mask) { + if (path.isEmpty()) { + return data; + } else { + FieldValue newValue = data.get(path); + if (newValue != null) { + filteredObject = filteredObject.set(path, newValue); + } + } + } + return filteredObject; + } + @Override public int hashCode() { return mask.hashCode(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldTransform.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldTransform.java index d6739a9f8b8..831f79e0739 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldTransform.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldTransform.java @@ -58,4 +58,8 @@ public int hashCode() { result = 31 * result + operation.hashCode(); return result; } + + public boolean isIdempotent() { + return operation.isIdempotent(); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java index 93fbaac2ebc..fb56493a85b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java @@ -148,4 +148,15 @@ static SnapshotVersion getPostMutationVersion(@Nullable MaybeDocument maybeDoc) return SnapshotVersion.NONE; } } + + /** + * If applicable, returns the field mask for this mutation. Fields that are not included in this + * field mask are not modified when this mutation is applied. Mutations that replace all document + * values return 'null'. + */ + @Nullable + public abstract FieldMask getFieldMask(); + + /** Returns whether all operations in the mutation are idempotent. */ + public abstract boolean isIdempotent(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java index a2b7d35671c..0c68de215ac 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.Timestamp; +import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; import java.util.HashSet; @@ -39,14 +40,34 @@ public final class MutationBatch { */ public static final int UNKNOWN = -1; + /** The unique ID of this mutation batch. */ private final int batchId; + + /** The original write time of this mutation. */ private final Timestamp localWriteTime; + + /** + * Mutations that are used to populate the base values when this mutation is applied locally. This + * can be used to locally overwrite values that are persisted in the remote document cache. Base + * mutations are never sent to the backend. + */ + private final List baseMutations; + + /** + * The user-provided mutations in this mutation batch. User-provided mutations are applied both + * locally and remotely on the backend. + */ private final List mutations; - public MutationBatch(int batchId, Timestamp localWriteTime, List mutations) { + public MutationBatch( + int batchId, + Timestamp localWriteTime, + List baseMutations, + List mutations) { hardAssert(!mutations.isEmpty(), "Cannot create an empty mutation batch"); this.batchId = batchId; this.localWriteTime = localWriteTime; + this.baseMutations = baseMutations; this.mutations = mutations; } @@ -98,8 +119,18 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo maybeDoc.getKey()); } + // First, apply the base state. This allows us to apply non-idempotent transform against a + // consistent set of values. + for (int i = 0; i < baseMutations.size(); i++) { + Mutation mutation = baseMutations.get(i); + if (mutation.getKey().equals(documentKey)) { + maybeDoc = mutation.applyToLocalView(maybeDoc, maybeDoc, localWriteTime); + } + } + MaybeDocument baseDoc = maybeDoc; + // Second, apply all user-provided mutations. for (int i = 0; i < mutations.size(); i++) { Mutation mutation = mutations.get(i); if (mutation.getKey().equals(documentKey)) { @@ -109,6 +140,23 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo return maybeDoc; } + /** Computes the local view for all provided documents given the mutations in this batch. */ + public ImmutableSortedMap applyToLocalDocumentSet( + ImmutableSortedMap maybeDocumentMap) { + // TODO(mrschmidt): This implementation is O(n^2). If we iterate through the mutations first + // (as done in `applyToLocalView(DocumentKey k, MaybeDoc d)`), we can reduce the complexity to + // O(n). + + ImmutableSortedMap mutatedDocuments = maybeDocumentMap; + for (DocumentKey key : getKeys()) { + MaybeDocument mutatedDocument = applyToLocalView(key, mutatedDocuments.get(key)); + if (mutatedDocument != null) { + mutatedDocuments = mutatedDocuments.insert(mutatedDocument.getKey(), mutatedDocument); + } + } + return mutatedDocuments; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -121,6 +169,7 @@ public boolean equals(Object o) { MutationBatch that = (MutationBatch) o; return batchId == that.batchId && localWriteTime.equals(that.localWriteTime) + && baseMutations.equals(that.baseMutations) && mutations.equals(that.mutations); } @@ -128,6 +177,7 @@ public boolean equals(Object o) { public int hashCode() { int result = batchId; result = 31 * result + localWriteTime.hashCode(); + result = 31 * result + baseMutations.hashCode(); result = 31 * result + mutations.hashCode(); return result; } @@ -138,6 +188,8 @@ public String toString() { + batchId + ", localWriteTime=" + localWriteTime + + ", baseMutations=" + + baseMutations + ", mutations=" + mutations + ')'; @@ -164,7 +216,16 @@ public Timestamp getLocalWriteTime() { return localWriteTime; } + /** @return The user-provided mutations in this mutation batch. */ public List getMutations() { return mutations; } + + /** + * @return The mutations that are used to populate the base values when this mutation batch is + * applied locally. + */ + public List getBaseMutations() { + return baseMutations; + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java new file mode 100644 index 00000000000..baf92294558 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java @@ -0,0 +1,111 @@ +// Copyright 2018 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License 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.google.firebase.firestore.model.mutation; + +import static com.google.firebase.firestore.util.Assert.fail; + +import com.google.firebase.Timestamp; +import com.google.firebase.firestore.model.value.DoubleValue; +import com.google.firebase.firestore.model.value.FieldValue; +import com.google.firebase.firestore.model.value.IntegerValue; +import com.google.firebase.firestore.model.value.NumberValue; + +/** + * Implements the backend semantics for locally computed NUMERIC_ADD (increment) transforms. + * Converts all field values to longs or doubles and resolves overflows to + * Long.MAX_VALUE/Long.MIN_VALUE. + */ +public class NumericIncrementTransformOperation implements TransformOperation { + private NumberValue operand; + + public NumericIncrementTransformOperation(NumberValue operand) { + this.operand = operand; + } + + @Override + public FieldValue applyToLocalView(FieldValue previousValue, Timestamp localWriteTime) { + // Return an integer value only if the previous value and the operand is an integer. + if (previousValue instanceof IntegerValue && operand instanceof IntegerValue) { + long sum = safeIncrement(((IntegerValue) previousValue).getInternalValue(), operandAsLong()); + return IntegerValue.valueOf(sum); + } else if (previousValue instanceof IntegerValue) { + double sum = ((IntegerValue) previousValue).getInternalValue() + operandAsDouble(); + return DoubleValue.valueOf(sum); + } else if (previousValue instanceof DoubleValue) { + double sum = ((DoubleValue) previousValue).getInternalValue() + operandAsDouble(); + return DoubleValue.valueOf(sum); + } + + // If the existing value is not a number, use the value of the transform as the new base value. + return operand; + } + + /** + * Implementation of Java 8's `addExact()` that resolves positive and negative numeric overflows + * to Long.MAX_VALUE or Long.MIN_VALUE respectively (instead of throwing an ArithmeticException). + */ + private long safeIncrement(long x, long y) { + long r = x + y; + + // See "Hacker's Delight" 2-12: Overflow if both arguments have the opposite sign of the result + if (((x ^ r) & (y ^ r)) >= 0) { + return r; + } + + if (r >= 0L) { + return Long.MIN_VALUE; + } else { + return Long.MAX_VALUE; + } + } + + private double operandAsDouble() { + if (operand instanceof DoubleValue) { + return ((DoubleValue) operand).getInternalValue(); + } else if (operand instanceof IntegerValue) { + return ((IntegerValue) operand).getInternalValue(); + } else { + throw fail( + "Expected 'operand' to be of Number type, but was " + + operand.getClass().getCanonicalName()); + } + } + + private long operandAsLong() { + if (operand instanceof DoubleValue) { + return (long) ((DoubleValue) operand).getInternalValue(); + } else if (operand instanceof IntegerValue) { + return ((IntegerValue) operand).getInternalValue(); + } else { + throw fail( + "Expected 'operand' to be of Number type, but was " + + operand.getClass().getCanonicalName()); + } + } + + @Override + public FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue transformResult) { + return transformResult; + } + + public FieldValue getOperand() { + return operand; + } + + @Override + public boolean isIdempotent() { + return false; + } +} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/PatchMutation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/PatchMutation.java index 3f119ea663e..ade731ba1c2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/PatchMutation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/PatchMutation.java @@ -130,6 +130,12 @@ public MaybeDocument applyToLocalView( return new Document(getKey(), version, newData, Document.DocumentState.LOCAL_MUTATIONS); } + @Nullable + @Override + public FieldMask getFieldMask() { + return mask; + } + /** * Patches the data of document if available or creates a new document. Note that this does not * check whether or not the precondition of this patch holds. @@ -157,4 +163,9 @@ private ObjectValue patchObject(ObjectValue obj) { } return obj; } + + @Override + public boolean isIdempotent() { + return true; + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ServerTimestampOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ServerTimestampOperation.java index 4d0eabd12f9..0f75a9430b8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ServerTimestampOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ServerTimestampOperation.java @@ -38,6 +38,11 @@ public FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue tra return transformResult; } + @Override + public boolean isIdempotent() { + return true; + } + // NOTE: Since we've guaranteed a singleton instance, we can rely on Object's default // implementation of equals() / hashCode(). } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/SetMutation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/SetMutation.java index dc0fa550e8f..f4fd44fe19e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/SetMutation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/SetMutation.java @@ -89,8 +89,19 @@ public MaybeDocument applyToLocalView( return new Document(getKey(), version, value, Document.DocumentState.LOCAL_MUTATIONS); } + @Nullable + @Override + public FieldMask getFieldMask() { + return null; + } + /** Returns the object value to use when setting the document. */ public ObjectValue getValue() { return value; } + + @Override + public boolean isIdempotent() { + return true; + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformMutation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformMutation.java index f925017fa50..abe82a999f2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformMutation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformMutation.java @@ -25,7 +25,9 @@ import com.google.firebase.firestore.model.value.FieldValue; import com.google.firebase.firestore.model.value.ObjectValue; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import javax.annotation.Nullable; /** @@ -121,6 +123,25 @@ public MaybeDocument applyToLocalView( getKey(), doc.getVersion(), newData, Document.DocumentState.LOCAL_MUTATIONS); } + @Override + public FieldMask getFieldMask() { + Set fieldMask = new HashSet<>(); + for (FieldTransform transform : fieldTransforms) { + fieldMask.add(transform.getFieldPath()); + } + return FieldMask.fromSet(fieldMask); + } + + @Override + public boolean isIdempotent() { + for (FieldTransform transform : fieldTransforms) { + if (!transform.isIdempotent()) { + return false; + } + } + return true; + } + /** * Asserts that the given MaybeDocument is actually a Document and verifies that it matches the * key for this mutation. Since we only support transformations with precondition exists this diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformOperation.java index 7e9bcf50610..22bda140c28 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformOperation.java @@ -30,4 +30,7 @@ public interface TransformOperation { * potentially using the server-provided transformResult. */ FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue transformResult); + + /** Returns whether this field transform is idempotent. */ + boolean isIdempotent(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java index 11b5500660c..beb223b8f24 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java @@ -45,6 +45,7 @@ import com.google.firebase.firestore.model.mutation.FieldTransform; import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationResult; +import com.google.firebase.firestore.model.mutation.NumericIncrementTransformOperation; import com.google.firebase.firestore.model.mutation.PatchMutation; import com.google.firebase.firestore.model.mutation.Precondition; import com.google.firebase.firestore.model.mutation.ServerTimestampOperation; @@ -59,6 +60,7 @@ import com.google.firebase.firestore.model.value.GeoPointValue; import com.google.firebase.firestore.model.value.IntegerValue; import com.google.firebase.firestore.model.value.NullValue; +import com.google.firebase.firestore.model.value.NumberValue; import com.google.firebase.firestore.model.value.ObjectValue; import com.google.firebase.firestore.model.value.ReferenceValue; import com.google.firebase.firestore.model.value.StringValue; @@ -556,6 +558,13 @@ private DocumentTransform.FieldTransform encodeFieldTransform(FieldTransform fie .setFieldPath(fieldTransform.getFieldPath().canonicalString()) .setRemoveAllFromArray(encodeArrayTransformElements(remove.getElements())) .build(); + } else if (transform instanceof NumericIncrementTransformOperation) { + NumericIncrementTransformOperation incrementOperation = + (NumericIncrementTransformOperation) transform; + return DocumentTransform.FieldTransform.newBuilder() + .setFieldPath(fieldTransform.getFieldPath().canonicalString()) + .setIncrement(encodeValue(incrementOperation.getOperand())) + .build(); } else { throw fail("Unknown transform: %s", transform); } @@ -592,6 +601,18 @@ private FieldTransform decodeFieldTransform(DocumentTransform.FieldTransform fie FieldPath.fromServerFormat(fieldTransform.getFieldPath()), new ArrayTransformOperation.Remove( decodeArrayTransformElements(fieldTransform.getRemoveAllFromArray()))); + case INCREMENT: + { + FieldValue operand = decodeValue(fieldTransform.getIncrement()); + hardAssert( + operand instanceof NumberValue, + "Expected NUMERIC_ADD transform to be of number type, but was %s", + operand.getClass().getCanonicalName()); + return new FieldTransform( + FieldPath.fromServerFormat(fieldTransform.getFieldPath()), + new NumericIncrementTransformOperation( + (NumberValue) decodeValue(fieldTransform.getIncrement()))); + } default: throw fail("Unknown FieldTransform proto: %s", fieldTransform); } diff --git a/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto b/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto index 8f3d6e7ef45..23d4b66b661 100644 --- a/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto +++ b/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto @@ -55,4 +55,16 @@ message WriteBatch { // The local time at which the write batch was initiated. google.protobuf.Timestamp local_write_time = 3; + + // A list of pseudo-writes that represent a partial base state from when this + // write batch was initially created. When computing the local view batch, + // these base_writes are applied prior to the real writes in order to + // override certain document fields from the remote document cache. This is + // necessary in the case of non-idempotent writes (e.g. increment + // transforms) to make sure that the local view of the modified documents + // doesn't flicker if the remote document cache receives the result of the + // non-idempotent write before the write is removed from the queue. + // + // These writes are never sent to the backend. + repeated google.firestore.v1.Write base_writes = 4; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java index 41d5f745e69..c97f3637fbc 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.deleteMutation; import static com.google.firebase.firestore.testutil.TestUtil.deletedDoc; import static com.google.firebase.firestore.testutil.TestUtil.doc; +import static com.google.firebase.firestore.testutil.TestUtil.field; import static com.google.firebase.firestore.testutil.TestUtil.fieldMask; import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; @@ -33,6 +34,7 @@ import com.google.firebase.firestore.model.NoDocument; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.model.UnknownDocument; +import com.google.firebase.firestore.model.mutation.FieldMask; import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationBatch; import com.google.firebase.firestore.model.mutation.PatchMutation; @@ -43,6 +45,7 @@ import com.google.firestore.v1.Value; import com.google.firestore.v1.Write; import com.google.protobuf.ByteString; +import java.util.Collections; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -65,6 +68,12 @@ public void setUp() { @Test public void testEncodesMutationBatch() { + Mutation baseWrite = + new PatchMutation( + key("foo/bar"), + TestUtil.wrapObject(map("a", "b")), + FieldMask.fromSet(Collections.singleton(field("a"))), + com.google.firebase.firestore.model.mutation.Precondition.NONE); Mutation set = setMutation("foo/bar", map("a", "b", "num", 1)); Mutation patch = new PatchMutation( @@ -74,7 +83,18 @@ public void testEncodesMutationBatch() { com.google.firebase.firestore.model.mutation.Precondition.exists(true)); Mutation del = deleteMutation("baz/quux"); Timestamp writeTime = Timestamp.now(); - MutationBatch model = new MutationBatch(42, writeTime, asList(set, patch, del)); + MutationBatch model = + new MutationBatch( + 42, writeTime, Collections.singletonList(baseWrite), asList(set, patch, del)); + + Write baseWriteProto = + Write.newBuilder() + .setUpdate( + com.google.firestore.v1.Document.newBuilder() + .setName("projects/p/databases/d/documents/foo/bar") + .putFields("a", Value.newBuilder().setStringValue("b").build())) + .setUpdateMask(DocumentMask.newBuilder().addFieldPaths("a")) + .build(); Write setProto = Write.newBuilder() @@ -108,6 +128,7 @@ public void testEncodesMutationBatch() { com.google.firebase.firestore.proto.WriteBatch batchProto = com.google.firebase.firestore.proto.WriteBatch.newBuilder() .setBatchId(42) + .addBaseWrites(baseWriteProto) .addAllWrites(asList(setProto, patchProto, delProto)) .setLocalWriteTime(writeTimeProto) .build(); @@ -117,6 +138,7 @@ public void testEncodesMutationBatch() { assertEquals(model.getBatchId(), decoded.getBatchId()); assertEquals(model.getLocalWriteTime(), decoded.getLocalWriteTime()); assertEquals(model.getMutations(), decoded.getMutations()); + assertEquals(model.getBaseMutations(), decoded.getBaseMutations()); assertEquals(model.getKeys(), decoded.getKeys()); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index 5144a91e62e..0d7617c1793 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -25,11 +25,13 @@ import static com.google.firebase.firestore.testutil.TestUtil.query; import static com.google.firebase.firestore.testutil.TestUtil.resumeToken; import static com.google.firebase.firestore.testutil.TestUtil.setMutation; +import static com.google.firebase.firestore.testutil.TestUtil.transformMutation; import static com.google.firebase.firestore.testutil.TestUtil.unknownDoc; import static com.google.firebase.firestore.testutil.TestUtil.updateRemoteEvent; import static com.google.firebase.firestore.testutil.TestUtil.values; import static com.google.firebase.firestore.testutil.TestUtil.version; import static com.google.firebase.firestore.testutil.TestUtil.viewChanges; +import static com.google.firebase.firestore.testutil.TestUtil.wrap; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; @@ -43,6 +45,7 @@ import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.database.collection.ImmutableSortedSet; +import com.google.firebase.firestore.FieldValue; import com.google.firebase.firestore.TestUtil.TestTargetMetadataProvider; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.Query; @@ -66,6 +69,8 @@ import com.google.firebase.firestore.testutil.TestUtil; import com.google.protobuf.ByteString; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map.Entry; import java.util.Set; @@ -118,7 +123,9 @@ private void writeMutation(Mutation mutation) { private void writeMutations(List mutations) { LocalWriteResult result = localStore.writeLocally(mutations); - batches.add(new MutationBatch(result.getBatchId(), Timestamp.now(), mutations)); + batches.add( + new MutationBatch( + result.getBatchId(), Timestamp.now(), Collections.emptyList(), mutations)); lastChanges = result.getChanges(); } @@ -130,16 +137,23 @@ private void notifyLocalViewChanges(LocalViewChanges changes) { localStore.notifyLocalViewChanges(asList(changes)); } - private void acknowledgeMutation(long documentVersion) { + private void acknowledgeMutation(long documentVersion, @Nullable Object transformResult) { MutationBatch batch = batches.remove(0); SnapshotVersion version = version(documentVersion); - MutationResult mutationResult = new MutationResult(version, /*transformResults=*/ null); + MutationResult mutationResult = + new MutationResult( + version, + transformResult != null ? Collections.singletonList(wrap(transformResult)) : null); MutationBatchResult result = MutationBatchResult.create( batch, version, singletonList(mutationResult), WriteStream.EMPTY_STREAM_TOKEN); lastChanges = localStore.acknowledgeBatch(result); } + private void acknowledgeMutation(long documentVersion) { + acknowledgeMutation(documentVersion, null); + } + private void rejectMutation() { MutationBatch batch = batches.get(0); batches.remove(0); @@ -203,7 +217,8 @@ private void assertNotContains(String keyPathString) { public void testMutationBatchKeys() { SetMutation set1 = setMutation("foo/bar", map("foo", "bar")); SetMutation set2 = setMutation("foo/baz", map("foo", "baz")); - MutationBatch batch = new MutationBatch(1, Timestamp.now(), asList(set1, set2)); + MutationBatch batch = + new MutationBatch(1, Timestamp.now(), Collections.emptyList(), asList(set1, set2)); Set keys = batch.getKeys(); assertEquals(2, keys.size()); } @@ -934,4 +949,189 @@ public void testRemoteDocumentKeysForTarget() { keys = localStore.getRemoteDocumentKeys(2); assertSetEquals(asList(key("foo/bar"), key("foo/baz")), keys); } + + // TODO(mrschmidt): The FieldValue.increment() field transform tests below would probably be + // better implemented as spec tests but currently they don't support transforms. + + @Test + public void testHandlesSetMutationThenTransformMutationThenTransformMutation() { + writeMutation(setMutation("foo/bar", map("sum", 0))); + assertContains(doc("foo/bar", 0, map("sum", 0), Document.DocumentState.LOCAL_MUTATIONS)); + assertChanged(doc("foo/bar", 0, map("sum", 0), Document.DocumentState.LOCAL_MUTATIONS)); + + writeMutation(transformMutation("foo/bar", map("sum", FieldValue.increment(1)))); + assertContains(doc("foo/bar", 0, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + assertChanged(doc("foo/bar", 0, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + + writeMutation(transformMutation("foo/bar", map("sum", FieldValue.increment(2)))); + assertContains(doc("foo/bar", 0, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + assertChanged(doc("foo/bar", 0, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + } + + @Test + public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransformMutation() { + if (garbageCollectorIsEager()) { + // Since this test doesn't start a listen, Eager GC removes the documents from the cache as + // soon as the mutation is applied. This creates a lot of special casing in this unit test but + // does not expand its test coverage. + return; + } + + writeMutation(setMutation("foo/bar", map("sum", 0))); + assertContains(doc("foo/bar", 0, map("sum", 0), Document.DocumentState.LOCAL_MUTATIONS)); + assertChanged(doc("foo/bar", 0, map("sum", 0), Document.DocumentState.LOCAL_MUTATIONS)); + + acknowledgeMutation(1); + assertChanged(doc("foo/bar", 1, map("sum", 0), Document.DocumentState.COMMITTED_MUTATIONS)); + assertContains(doc("foo/bar", 1, map("sum", 0), Document.DocumentState.COMMITTED_MUTATIONS)); + + writeMutation(transformMutation("foo/bar", map("sum", FieldValue.increment(1)))); + assertContains(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + + acknowledgeMutation(2, 1); + assertChanged(doc("foo/bar", 2, map("sum", 1), Document.DocumentState.COMMITTED_MUTATIONS)); + assertContains(doc("foo/bar", 2, map("sum", 1), Document.DocumentState.COMMITTED_MUTATIONS)); + + writeMutation(transformMutation("foo/bar", map("sum", FieldValue.increment(2)))); + assertContains(doc("foo/bar", 2, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + assertChanged(doc("foo/bar", 2, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + } + + @Test + public void testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransformMutation() { + Query query = Query.atPath(ResourcePath.fromString("foo")); + allocateQuery(query); + assertTargetId(2); + + writeMutation(setMutation("foo/bar", map("sum", 0))); + assertChanged(doc("foo/bar", 0, map("sum", 0), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 0, map("sum", 0), Document.DocumentState.LOCAL_MUTATIONS)); + + applyRemoteEvent(addedRemoteEvent(doc("foo/bar", 1, map("sum", 0)), asList(2), emptyList())); + acknowledgeMutation(1); + assertChanged(doc("foo/bar", 1, map("sum", 0), Document.DocumentState.SYNCED)); + assertContains(doc("foo/bar", 1, map("sum", 0), Document.DocumentState.SYNCED)); + + writeMutation(transformMutation("foo/bar", map("sum", FieldValue.increment(1)))); + assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + + // The value in this remote event gets ignored since we still have a pending transform mutation. + applyRemoteEvent(addedRemoteEvent(doc("foo/bar", 2, map("sum", 1337)), asList(2), emptyList())); + assertChanged(doc("foo/bar", 2, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 2, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + + // Add another increment. Note that we still compute the increment based on the local value. + writeMutation(transformMutation("foo/bar", map("sum", FieldValue.increment(2)))); + assertChanged(doc("foo/bar", 2, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 2, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + + acknowledgeMutation(3, 1); + assertChanged(doc("foo/bar", 3, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 3, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); + + acknowledgeMutation(4, 1339); + assertChanged(doc("foo/bar", 4, map("sum", 1339), Document.DocumentState.COMMITTED_MUTATIONS)); + assertContains(doc("foo/bar", 4, map("sum", 1339), Document.DocumentState.COMMITTED_MUTATIONS)); + } + + @Test + public void testHoldsBackOnlyNonIdempotentTransforms() { + Query query = Query.atPath(ResourcePath.fromString("foo")); + allocateQuery(query); + assertTargetId(2); + + writeMutation(setMutation("foo/bar", map("sum", 0, "array_union", new ArrayList<>()))); + assertChanged( + doc( + "foo/bar", + 0, + map("sum", 0, "array_union", new ArrayList<>()), + Document.DocumentState.LOCAL_MUTATIONS)); + + acknowledgeMutation(1); + assertChanged( + doc( + "foo/bar", + 1, + map("sum", 0, "array_union", new ArrayList<>()), + Document.DocumentState.COMMITTED_MUTATIONS)); + + applyRemoteEvent( + addedRemoteEvent( + doc("foo/bar", 1, map("sum", 0, "array_union", new ArrayList<>())), + asList(2), + emptyList())); + assertChanged( + doc( + "foo/bar", + 1, + map("sum", 0, "array_union", new ArrayList<>()), + Document.DocumentState.SYNCED)); + + writeMutations( + Arrays.asList( + transformMutation("foo/bar", map("sum", FieldValue.increment(1))), + transformMutation("foo/bar", map("array_union", FieldValue.arrayUnion("foo"))))); + assertChanged( + doc( + "foo/bar", + 1, + map("sum", 1, "array_union", Collections.singletonList("foo")), + Document.DocumentState.LOCAL_MUTATIONS)); + + // The sum transform is not idempotent and the backend's updated value is ignored. The + // ArrayUnion transform is recomputed and includes the backend value. + applyRemoteEvent( + addedRemoteEvent( + doc("foo/bar", 2, map("sum", 1337, "array_union", Collections.singletonList("bar"))), + asList(2), + emptyList())); + assertChanged( + doc( + "foo/bar", + 2, + map("sum", 1, "array_union", Arrays.asList("bar", "foo")), + Document.DocumentState.LOCAL_MUTATIONS)); + } + + @Test + public void testHandlesMergeMutationWithTransformThenRemoteEvent() { + Query query = Query.atPath(ResourcePath.fromString("foo")); + allocateQuery(query); + assertTargetId(2); + + writeMutations( + asList( + patchMutation("foo/bar", map(), Collections.emptyList()), + transformMutation("foo/bar", map("sum", FieldValue.increment(1))))); + assertChanged(doc("foo/bar", 0, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 0, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + + applyRemoteEvent(addedRemoteEvent(doc("foo/bar", 1, map("sum", 1337)), asList(2), emptyList())); + assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + } + + @Test + public void testHandlesPatchMutationWithTransformThenRemoteEvent() { + Query query = Query.atPath(ResourcePath.fromString("foo")); + allocateQuery(query); + assertTargetId(2); + + writeMutations( + asList( + patchMutation("foo/bar", map()), + transformMutation("foo/bar", map("sum", FieldValue.increment(1))))); + assertChanged(deletedDoc("foo/bar", 0)); + assertNotContains("foo/bar"); + + // Note: This test reflects the current behavior, but it may be preferable to replay the + // mutation once we receive the first value from the remote event. + + applyRemoteEvent(addedRemoteEvent(doc("foo/bar", 1, map("sum", 1337)), asList(2), emptyList())); + assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); + } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java index e53a1389f43..63889dfb6ce 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java @@ -40,6 +40,7 @@ import com.google.firebase.firestore.model.value.ObjectValue; import com.google.protobuf.ByteString; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -365,7 +366,7 @@ public void testRemoveOrphanedDocuments() { "actually register the mutations", () -> { Timestamp writeTime = Timestamp.now(); - mutationQueue.addMutationBatch(writeTime, mutations); + mutationQueue.addMutationBatch(writeTime, Collections.emptyList(), mutations); }); // Mark 5 documents eligible for GC. This simulates documents that were mutated then ack'd. diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java index ccbbc7f0e76..77e8d58b980 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java @@ -40,6 +40,7 @@ import com.google.firebase.firestore.remote.WriteStream; import com.google.protobuf.ByteString; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import org.junit.After; @@ -190,7 +191,9 @@ public void testAllMutationBatchesAffectingDocumentKey() { "New mutation batch", () -> { for (Mutation mutation : mutations) { - batches.add(mutationQueue.addMutationBatch(Timestamp.now(), asList(mutation))); + batches.add( + mutationQueue.addMutationBatch( + Timestamp.now(), Collections.emptyList(), asList(mutation))); } }); @@ -218,7 +221,9 @@ public void testAllMutationBatchesAffectingDocumentKeys() { "New mutation batch", () -> { for (Mutation mutation : mutations) { - batches.add(mutationQueue.addMutationBatch(Timestamp.now(), asList(mutation))); + batches.add( + mutationQueue.addMutationBatch( + Timestamp.now(), Collections.emptyList(), asList(mutation))); } }); @@ -247,7 +252,9 @@ public void testAllMutationBatchesAffectingDocumentLotsOfDocumentKeys() { "New mutation batch", () -> { for (Mutation mutation : mutations) { - batches.add(mutationQueue.addMutationBatch(Timestamp.now(), asList(mutation))); + batches.add( + mutationQueue.addMutationBatch( + Timestamp.now(), Collections.emptyList(), asList(mutation))); } }); @@ -283,7 +290,9 @@ public void testAllMutationBatchesAffectingQuery() { "New mutation batch", () -> { for (Mutation mutation : mutations) { - batches.add(mutationQueue.addMutationBatch(Timestamp.now(), asList(mutation))); + batches.add( + mutationQueue.addMutationBatch( + Timestamp.now(), Collections.emptyList(), asList(mutation))); } }); @@ -307,10 +316,12 @@ public void testAllMutationBatchesAffectingQuery_withCompoundBatches() { batches.add( mutationQueue.addMutationBatch( Timestamp.now(), + Collections.emptyList(), asList(setMutation("foo/bar", value), setMutation("foo/bar/baz/quux", value)))); batches.add( mutationQueue.addMutationBatch( Timestamp.now(), + Collections.emptyList(), asList(setMutation("foo/bar", value), setMutation("foo/baz", value)))); }); @@ -401,7 +412,9 @@ private MutationBatch addMutationBatch(String key) { return persistence.runTransaction( "New mutation batch", - () -> mutationQueue.addMutationBatch(Timestamp.now(), asList(mutation))); + () -> + mutationQueue.addMutationBatch( + Timestamp.now(), Collections.emptyList(), asList(mutation))); } /** diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java index 67c1fba0ac4..03733eed4b8 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java @@ -41,12 +41,14 @@ import com.google.firebase.firestore.model.mutation.PatchMutation; import com.google.firebase.firestore.model.mutation.Precondition; import com.google.firebase.firestore.model.mutation.TransformMutation; +import com.google.firebase.firestore.model.value.IntegerValue; import com.google.firebase.firestore.model.value.ObjectValue; import com.google.firebase.firestore.model.value.ServerTimestampValue; import com.google.firebase.firestore.model.value.StringValue; import com.google.firebase.firestore.model.value.TimestampValue; import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; @@ -167,6 +169,138 @@ public void testAppliesLocalServerTimestampTransformsToDocuments() { assertEquals(expectedDoc, transformedDoc); } + @Test + public void testAppliesIncrementTransformToDocument() { + Map baseDoc = + map( + "longPlusLong", + 1, + "longPlusDouble", + 2, + "doublePlusLong", + 3.3, + "doublePlusDouble", + 4.0, + "longPlusNan", + 5, + "doublePlusNan", + 6.6, + "longPlusInfinity", + 7, + "doublePlusInfinity", + 8.8); + Map transform = + map( + "longPlusLong", + FieldValue.increment(1), + "longPlusDouble", + FieldValue.increment(2.2), + "doublePlusLong", + FieldValue.increment(3), + "doublePlusDouble", + FieldValue.increment(4.4), + "longPlusNan", + FieldValue.increment(Double.NaN), + "doublePlusNan", + FieldValue.increment(Double.NaN), + "longPlusInfinity", + FieldValue.increment(Double.POSITIVE_INFINITY), + "doublePlusInfinity", + FieldValue.increment(Double.POSITIVE_INFINITY)); + Map expected = + map( + "longPlusLong", + 2L, + "longPlusDouble", + 4.2D, + "doublePlusLong", + 6.3D, + "doublePlusDouble", + 8.4D, + "longPlusNan", + Double.NaN, + "doublePlusNan", + Double.NaN, + "longPlusInfinity", + Double.POSITIVE_INFINITY, + "doublePlusInfinity", + Double.POSITIVE_INFINITY); + + verifyTransform(baseDoc, transform, expected); + } + + @Test + public void testAppliesIncrementTransformToUnexpectedType() { + Map baseDoc = map("string", "zero"); + Map transform = map("string", FieldValue.increment(1)); + Map expected = map("string", 1); + verifyTransform(baseDoc, transform, expected); + } + + @Test + public void testAppliesIncrementTransformToMissingField() { + Map baseDoc = map(); + Map transform = map("missing", FieldValue.increment(1)); + Map expected = map("missing", 1); + verifyTransform(baseDoc, transform, expected); + } + + @Test + public void testAppliesIncrementTransformsConsecutively() { + Map baseDoc = map("number", 1); + Map transform1 = map("number", FieldValue.increment(2)); + Map transform2 = map("number", FieldValue.increment(3)); + Map transform3 = map("number", FieldValue.increment(4)); + Map expected = map("number", 10); + verifyTransform(baseDoc, Arrays.asList(transform1, transform2, transform3), expected); + } + + @Test + public void testAppliesIncrementWithoutOverflow() { + Map baseDoc = + map( + "a", + Long.MAX_VALUE - 1, + "b", + Long.MAX_VALUE - 1, + "c", + Long.MAX_VALUE, + "d", + Long.MAX_VALUE); + Map transform = + map( + "a", FieldValue.increment(1), + "b", FieldValue.increment(Long.MAX_VALUE), + "c", FieldValue.increment(1), + "d", FieldValue.increment(Long.MAX_VALUE)); + Map expected = + map("a", Long.MAX_VALUE, "b", Long.MAX_VALUE, "c", Long.MAX_VALUE, "d", Long.MAX_VALUE); + verifyTransform(baseDoc, transform, expected); + } + + @Test + public void testAppliesIncrementWithoutUnderflow() { + Map baseDoc = + map( + "a", + Long.MIN_VALUE + 1, + "b", + Long.MIN_VALUE + 1, + "c", + Long.MIN_VALUE, + "d", + Long.MIN_VALUE); + Map transform = + map( + "a", FieldValue.increment(-1), + "b", FieldValue.increment(Long.MIN_VALUE), + "c", FieldValue.increment(-1), + "d", FieldValue.increment(Long.MIN_VALUE)); + Map expected = + map("a", Long.MIN_VALUE, "b", Long.MIN_VALUE, "c", Long.MIN_VALUE, "d", Long.MIN_VALUE); + verifyTransform(baseDoc, transform, expected); + } + // NOTE: This is more a test of UserDataConverter code than Mutation code but we don't have unit // tests for it currently. We could consider removing this test once we have integration tests. @Test @@ -325,15 +459,42 @@ public void testAppliesLocalArrayRemoveTransformWithNonPrimitiveElements() { private void verifyTransform( Map baseData, - Map transformData, + List> transforms, Map expectedData) { - Document baseDoc = doc("collection/key", 0, baseData); - TransformMutation transform = transformMutation("collection/key", transformData); - MaybeDocument transformedDoc = transform.applyToLocalView(baseDoc, baseDoc, Timestamp.now()); + MaybeDocument currentDoc = doc("collection/key", 0, baseData); + + for (Map transformData : transforms) { + TransformMutation transform = transformMutation("collection/key", transformData); + currentDoc = transform.applyToLocalView(currentDoc, currentDoc, Timestamp.now()); + } Document expectedDoc = doc("collection/key", 0, expectedData, Document.DocumentState.LOCAL_MUTATIONS); - assertEquals(expectedDoc, transformedDoc); + assertEquals(expectedDoc, currentDoc); + } + + private void verifyTransform( + Map baseData, + Map transformData, + Map expectedData) { + verifyTransform(baseData, Collections.singletonList(transformData), expectedData); + } + + @Test + public void testAppliesServerAckedIncrementTransformToDocuments() { + Map data = map("sum", 1); + Document baseDoc = doc("collection/key", 0, data); + + Mutation transform = transformMutation("collection/key", map("sum", FieldValue.increment(2))); + MutationResult mutationResult = + new MutationResult(version(1), Collections.singletonList(IntegerValue.valueOf(3L))); + + MaybeDocument transformedDoc = transform.applyToRemoteDocument(baseDoc, mutationResult); + + Map expectedData = map("sum", 3L); + assertEquals( + doc("collection/key", 1, expectedData, Document.DocumentState.COMMITTED_MUTATIONS), + transformedDoc); } @Test