From 20152fe76671a24385fedfa00b160a64e929cd9c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 1 Nov 2018 15:25:24 -0700 Subject: [PATCH 01/16] Adding FieldValue.numericAdd() --- .../firestore/NumericTransformsTest.java | 154 +++++++++++++++++ .../testutil/IntegrationTestUtil.java | 60 +++++-- .../google/firebase/firestore/FieldValue.java | 57 +++++- .../firebase/firestore/UserDataConverter.java | 9 + .../firestore/local/LocalSerializer.java | 17 +- .../firebase/firestore/local/LocalStore.java | 49 +++++- .../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 | 61 ++++++- .../NumericAddTransformOperation.java | 109 ++++++++++++ .../model/mutation/PatchMutation.java | 11 ++ .../mutation/ServerTimestampOperation.java | 5 + .../firestore/model/mutation/SetMutation.java | 11 ++ .../model/mutation/TransformMutation.java | 19 ++ .../model/mutation/TransformOperation.java | 3 + .../firestore/remote/RemoteSerializer.java | 20 +++ .../firebase/firestore/proto/mutation.proto | 6 + .../google/firestore/v1beta1/write.proto | 12 ++ .../firestore/local/LocalSerializerTest.java | 22 ++- .../firestore/local/LocalStoreTestCase.java | 162 +++++++++++++++++- .../local/LruGarbageCollectorTestCase.java | 3 +- .../local/MutationQueueTestCase.java | 23 ++- .../firestore/model/MutationTest.java | 102 ++++++++++- 29 files changed, 942 insertions(+), 48 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/NumericAddTransformOperation.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..5d78c2cd3f3 --- /dev/null +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java @@ -0,0 +1,154 @@ +// 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.Ignore; +import org.junit.Test; + +@Ignore("Not yet available in production") +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.numericAdd(1337)))); + expectLocalAndRemoteValue(1337L); + } + + @Test + public void integerIncrementExistingInteger() { + writeInitialData(map("sum", 1337L)); + waitFor(docRef.update("sum", FieldValue.numericAdd(1))); + expectLocalAndRemoteValue(1338L); + } + + @Test + public void doubleIncrementWithExistingDouble() { + writeInitialData(map("sum", 13.37D)); + waitFor(docRef.update("sum", FieldValue.numericAdd(0.1))); + expectLocalAndRemoteValue(13.47D); + } + + @Test + public void integerIncrementExistingDouble() { + writeInitialData(map("sum", 13.37D)); + waitFor(docRef.update("sum", FieldValue.numericAdd(1))); + expectLocalAndRemoteValue(14.37D); + } + + @Test + public void doubleIncrementExistingInteger() { + writeInitialData(map("sum", 1337L)); + waitFor(docRef.update("sum", FieldValue.numericAdd(0.1))); + expectLocalAndRemoteValue(1337.1D); + } + + @Test + public void integerIncrementExistingString() { + writeInitialData(map("sum", "overwrite")); + waitFor(docRef.update("sum", FieldValue.numericAdd(1337))); + expectLocalAndRemoteValue(1337L); + } + + @Test + public void doubleIncrementExistingString() { + writeInitialData(map("sum", "overwrite")); + waitFor(docRef.update("sum", FieldValue.numericAdd(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.numericAdd(0.1D)); + docRef.update("sum", FieldValue.numericAdd(0.01D)); + docRef.update("sum", FieldValue.numericAdd(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 c629b676b72..c148a5daec6 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,13 @@ /** 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. + 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 +90,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() { @@ -93,11 +111,33 @@ public static FirebaseFirestoreSettings newTestSettings() { 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)); + + // Disable SSL and hostname verification + 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..a7bc8a3a3cd 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 numericAdd() transforms. */ + static class NumericAddFieldValue extends FieldValue { + private final Number operand; + + NumericAddFieldValue(Number operand) { + this.operand = operand; + } + + @Override + String getMethodName() { + return "FieldValue.numericAdd"; + } + + 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 add + * the given value to the field's current value. + * + *

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

If 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 numericAdd(long l) { + return new NumericAddFieldValue(l); + } + + /** + * Returns a special value that can be used with set() or update() that tells the server to add + * the given value to the field's current 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 numericAdd(double l) { + return new NumericAddFieldValue(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..72c2e0b2531 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.NumericAddTransformOperation; 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,13 @@ private void parseSentinelFieldValue( ArrayTransformOperation arrayRemove = new ArrayTransformOperation.Remove(parsedElements); context.addToFieldTransforms(context.getPath(), arrayRemove); + } else if (value instanceof com.google.firebase.firestore.FieldValue.NumericAddFieldValue) { + com.google.firebase.firestore.FieldValue.NumericAddFieldValue numericAddFieldValue = + (com.google.firebase.firestore.FieldValue.NumericAddFieldValue) value; + NumberValue operand = (NumberValue) parseQueryValue(numericAddFieldValue.getOperand()); + NumericAddTransformOperation numericAdd = new NumericAddTransformOperation(operand); + context.addToFieldTransforms(context.getPath(), numericAdd); + } 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 55c8bd146ab..abe2d65369d 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 @@ -160,6 +160,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)); } @@ -171,13 +174,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 6c203edd3ad..80c467ce3ea 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.HashSet; import java.util.List; import java.util.Map; @@ -182,16 +187,46 @@ 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 = + Set keys = new HashSet<>(); + for (Mutation mutation : mutations) { + keys.add(mutation.getKey()); + } + + // 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); - return new LocalWriteResult(batch.getBatchId(), changedDocuments); + + return persistence.runTransaction( + "Locally write mutations", + () -> { + + // For non-idempotent mutations (such as `FieldValue.numericAdd()`), 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 (maybeDocument instanceof Document && !mutation.isIdempotent()) { + FieldMask fieldMask = mutation.getFieldMask(); + if (fieldMask != null) { + ObjectValue baseValues = fieldMask.applyTo(((Document) maybeDocument).getData()); + baseMutations.add( + new PatchMutation(mutation.getKey(), baseValues, fieldMask, Precondition.NONE)); + } + } + } + + MutationBatch batch = + mutationQueue.addMutationBatch(localWriteTime, baseMutations, mutations); + ImmutableSortedMap changedDocuments = + batch.applyToLocalView(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 9af1b1faaba..842e73e4aca 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 @@ -138,7 +138,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; @@ -151,7 +152,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 da67269cbea..728eb1abd99 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 @@ -47,8 +47,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 ccf5bb70490..485332cdf3d 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 @@ -192,11 +192,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 89e10ec3364..73f2056ab8c 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 @@ -87,6 +87,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 0cbef886ccd..ffb65f3c4ae 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.Collection; /** @@ -64,6 +66,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 bc3ab7ec111..68e2ff8ae74 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.Collections; @@ -40,13 +41,33 @@ 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) { 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 certain 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,19 @@ 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 applyToLocalView( + ImmutableSortedMap maybeDocumentMap) { + ImmutableSortedMap changedDocuments = maybeDocumentMap; + for (DocumentKey key : getKeys()) { + MaybeDocument maybeDocument = applyToLocalView(key, changedDocuments.get(key)); + if (maybeDocument != null) { + changedDocuments = changedDocuments.insert(maybeDocument.getKey(), maybeDocument); + } + } + return changedDocuments; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -121,6 +165,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 +173,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 +184,8 @@ public String toString() { + batchId + ", localWriteTime=" + localWriteTime + + ", baseMutations=" + + baseMutations + ", mutations=" + mutations + ')'; @@ -177,10 +225,19 @@ public boolean isTombstone() { /** Converts this batch to a tombstone. */ public MutationBatch toTombstone() { - return new MutationBatch(batchId, localWriteTime, Collections.emptyList()); + return new MutationBatch(batchId, localWriteTime, baseMutations, Collections.emptyList()); } + /** @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/NumericAddTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java new file mode 100644 index 00000000000..f6f67749927 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java @@ -0,0 +1,109 @@ +// 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 transforms. Converts all field + * values to longs or doubles and resolves overflows to Long.MAX_VALUE/Long.MIN_VALUE. + */ +public class NumericAddTransformOperation implements TransformOperation { + private NumberValue operand; + + public NumericAddTransformOperation(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 = safeAdd(((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 `safeAdd()` that resolves positive and negative numeric overflows to + * Long.MAX_VALUE or Long.MIN_VALUE respectively (instead of throwing an ArithmeticException). + */ + private long safeAdd(long x, long y) { + long r = x + y; + + 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..12a90088e72 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 @@ -121,6 +121,25 @@ public MaybeDocument applyToLocalView( getKey(), doc.getVersion(), newData, Document.DocumentState.LOCAL_MUTATIONS); } + @Override + public FieldMask getFieldMask() { + List fieldMask = new ArrayList<>(); + for (FieldTransform transform : fieldTransforms) { + fieldMask.add(transform.getFieldPath()); + } + return FieldMask.fromCollection(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 fc4eb36bd30..b403e75d4e0 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.NumericAddTransformOperation; 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; @@ -558,6 +560,12 @@ private DocumentTransform.FieldTransform encodeFieldTransform(FieldTransform fie .setFieldPath(fieldTransform.getFieldPath().canonicalString()) .setRemoveAllFromArray(encodeArrayTransformElements(remove.getElements())) .build(); + } else if (transform instanceof NumericAddTransformOperation) { + NumericAddTransformOperation numericAdd = (NumericAddTransformOperation) transform; + return DocumentTransform.FieldTransform.newBuilder() + .setFieldPath(fieldTransform.getFieldPath().canonicalString()) + .setNumericAdd(encodeValue(numericAdd.getOperand())) + .build(); } else { throw fail("Unknown transform: %s", transform); } @@ -594,6 +602,18 @@ private FieldTransform decodeFieldTransform(DocumentTransform.FieldTransform fie FieldPath.fromServerFormat(fieldTransform.getFieldPath()), new ArrayTransformOperation.Remove( decodeArrayTransformElements(fieldTransform.getRemoveAllFromArray()))); + case NUMERIC_ADD: + { + FieldValue operand = decodeValue(fieldTransform.getNumericAdd()); + 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 NumericAddTransformOperation( + (NumberValue) decodeValue(fieldTransform.getNumericAdd()))); + } 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 ea448c89a64..11482112bf0 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,10 @@ message WriteBatch { // The local time at which the write batch was initiated. google.protobuf.Timestamp local_write_time = 3; + + // A list of writes that are used to populate a base state when this mutation + // is applied locally. These writes are used to locally overwrite values that + // are persisted in the remote document cache. These writes are never sent to + // the backend. + repeated google.firestore.v1beta1.Write base_writes = 4; } diff --git a/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto b/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto index 4f86df37ede..c50868573e7 100644 --- a/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto +++ b/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto @@ -103,6 +103,18 @@ message DocumentTransform { // Sets the field to the given server value. ServerValue set_to_server_value = 2; + // Adds the given value to the field's current value. + // + // This must be an integer or a double value. + // If the 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. + // If either of the given value or the current field value are doubles, + // both values will be interpreted as doubles. Double arithmetic and + // representation of double values follow IEEE 754 semantics. + // If there is positive/negative integer overflow, the field is resolved + // to the largest magnitude positive/negative integer. + Value numeric_add = 3; + // Append the given elements in order if they are not already present in // the current field value. // If the field is not an array, or if the field does not yet exist, it is 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 db743396b29..978a9238fe3 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 @@ -44,6 +44,7 @@ import com.google.firestore.v1beta1.Value; import com.google.firestore.v1beta1.Write; import com.google.protobuf.ByteString; +import java.util.Collections; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -66,6 +67,12 @@ public void setUp() { @Test public void testEncodesMutationBatch() { + Mutation baseWrite = + new PatchMutation( + key("foo/bar"), + TestUtil.wrapObject(map("a", "b")), + FieldMask.fromCollection(asList(field("a"))), + com.google.firebase.firestore.model.mutation.Precondition.NONE); Mutation set = setMutation("foo/bar", map("a", "b", "num", 1)); Mutation patch = new PatchMutation( @@ -75,7 +82,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.v1beta1.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() @@ -109,6 +127,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(); @@ -118,6 +137,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..1032b02f5e7 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,143 @@ public void testRemoteDocumentKeysForTarget() { keys = localStore.getRemoteDocumentKeys(2); assertSetEquals(asList(key("foo/bar"), key("foo/baz")), keys); } + + @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.numericAdd(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.numericAdd(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()) { + 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.numericAdd(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.numericAdd(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 testHoldsBackNonIdempotentTransforms() { + 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.numericAdd(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", 1, map("sum", 2, "array_union", Collections.singletonList("bar"))), + asList(2), + emptyList())); + assertChanged( + doc( + "foo/bar", + 1, + map("sum", 1, "array_union", Arrays.asList("bar", "foo")), + 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.numericAdd(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.numericAdd(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)); + + 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)); + } } 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 11cbc6a3493..50f74781dcb 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; @@ -359,7 +360,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 c0f2d45b793..d741e979d2c 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 @@ -41,6 +41,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; @@ -211,7 +212,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))); } }); @@ -239,7 +242,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))); } }); @@ -268,7 +273,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))); } }); @@ -304,7 +311,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))); } }); @@ -328,10 +337,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)))); }); @@ -423,7 +434,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 d686c4829ef..e2cf5d98599 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 @@ -40,12 +40,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; @@ -166,6 +168,69 @@ public void testAppliesLocalServerTimestampTransformsToDocuments() { assertEquals(expectedDoc, transformedDoc); } + @Test + public void testAppliesNumericAddTransformToDocument() { + Map baseDoc = + map("longPlusLong", 1, "longPlusDouble", 2, "doublePlusLong", 3.3, "doublePlusDouble", 4.0); + Map transform = + map( + "longPlusLong", + FieldValue.numericAdd(1), + "longPlusDouble", + FieldValue.numericAdd(2.2), + "doublePlusLong", + FieldValue.numericAdd(3), + "doublePlusDouble", + FieldValue.numericAdd(4.4)); + Map expected = + map( + "longPlusLong", + 2L, + "longPlusDouble", + 4.2D, + "doublePlusLong", + 6.3D, + "doublePlusDouble", + 8.4D); + + verifyTransform(baseDoc, transform, expected); + } + + @Test + public void testAppliesNumericAddTransformToUnexpectedType() { + Map baseDoc = map("string", "zero"); + Map transform = map("string", FieldValue.numericAdd(1)); + Map expected = map("string", 1); + verifyTransform(baseDoc, transform, expected); + } + + @Test + public void testAppliesNumericAddTransformToMissingField() { + Map baseDoc = map(); + Map transform = map("missing", FieldValue.numericAdd(1)); + Map expected = map("missing", 1); + verifyTransform(baseDoc, transform, expected); + } + + @Test + public void testAppliesNumericAddTransformsConsecutively() { + Map baseDoc = map("number", 1); + Map transform1 = map("number", FieldValue.numericAdd(2)); + Map transform2 = map("number", FieldValue.numericAdd(3)); + Map transform3 = map("number", FieldValue.numericAdd(4)); + Map expected = map("number", 10); + verifyTransform(baseDoc, Arrays.asList(transform1, transform2, transform3), expected); + } + + @Test + public void testAppliesNumericAddWithoutOverflow() { + Map baseDoc = map("max", Long.MAX_VALUE - 1, "min", Long.MIN_VALUE + 1); + Map transform = + map("max", FieldValue.numericAdd(2), "min", FieldValue.numericAdd(-2)); + Map expected = map("max", Long.MAX_VALUE, "min", 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 @@ -324,15 +389,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 testAppliesServerAckedNumericAddTransformToDocuments() { + Map data = map("sum", 1); + Document baseDoc = doc("collection/key", 0, data); + + Mutation transform = transformMutation("collection/key", map("sum", FieldValue.numericAdd(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 From 602d03f5546ee7c806e7693a9a1b4916a931aead Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 2 Nov 2018 12:34:59 -0700 Subject: [PATCH 02/16] Adding NaN and Infinity test --- .../firestore/model/MutationTest.java | 38 +++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) 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 e2cf5d98599..c835959fe03 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 @@ -171,7 +171,23 @@ public void testAppliesLocalServerTimestampTransformsToDocuments() { @Test public void testAppliesNumericAddTransformToDocument() { Map baseDoc = - map("longPlusLong", 1, "longPlusDouble", 2, "doublePlusLong", 3.3, "doublePlusDouble", 4.0); + 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", @@ -181,7 +197,15 @@ public void testAppliesNumericAddTransformToDocument() { "doublePlusLong", FieldValue.numericAdd(3), "doublePlusDouble", - FieldValue.numericAdd(4.4)); + FieldValue.numericAdd(4.4), + "longPlusNan", + FieldValue.numericAdd(Double.NaN), + "doublePlusNan", + FieldValue.numericAdd(Double.NaN), + "longPlusInfinity", + FieldValue.numericAdd(Double.POSITIVE_INFINITY), + "doublePlusInfinity", + FieldValue.numericAdd(Double.POSITIVE_INFINITY)); Map expected = map( "longPlusLong", @@ -191,7 +215,15 @@ public void testAppliesNumericAddTransformToDocument() { "doublePlusLong", 6.3D, "doublePlusDouble", - 8.4D); + 8.4D, + "longPlusNan", + Double.NaN, + "doublePlusNan", + Double.NaN, + "longPlusInfinity", + Double.POSITIVE_INFINITY, + "doublePlusInfinity", + Double.POSITIVE_INFINITY); verifyTransform(baseDoc, transform, expected); } From ef1862b7c4503fdd9f21c25cfc303e040960e159 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Sat, 3 Nov 2018 13:49:38 -0700 Subject: [PATCH 03/16] Use addExact in comment --- .../model/mutation/NumericAddTransformOperation.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java index f6f67749927..d1a292908b9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java @@ -52,8 +52,8 @@ public FieldValue applyToLocalView(FieldValue previousValue, Timestamp localWrit } /** - * Implementation of Java 8's `safeAdd()` that resolves positive and negative numeric overflows to - * Long.MAX_VALUE or Long.MIN_VALUE respectively (instead of throwing an ArithmeticException). + * 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 safeAdd(long x, long y) { long r = x + y; From 6e386c4f45af115cc1c1587f0b01e6705134ce15 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Sat, 3 Nov 2018 18:50:25 -0700 Subject: [PATCH 04/16] Adding more Edge case testing --- .../NumericAddTransformOperation.java | 2 +- .../firestore/model/MutationTest.java | 43 +++++++++++++++++-- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java index d1a292908b9..d4710b1391b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java @@ -62,7 +62,7 @@ private long safeAdd(long x, long y) { return r; } - if (r > 0L) { + if (r >= 0L) { return Long.MIN_VALUE; } else { return Long.MAX_VALUE; 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 c835959fe03..bfb94f6e3f6 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 @@ -256,10 +256,47 @@ public void testAppliesNumericAddTransformsConsecutively() { @Test public void testAppliesNumericAddWithoutOverflow() { - Map baseDoc = map("max", Long.MAX_VALUE - 1, "min", Long.MIN_VALUE + 1); + Map baseDoc = + map( + "a", + Long.MAX_VALUE - 1, + "b", + Long.MAX_VALUE - 1, + "c", + Long.MAX_VALUE, + "d", + Long.MAX_VALUE); Map transform = - map("max", FieldValue.numericAdd(2), "min", FieldValue.numericAdd(-2)); - Map expected = map("max", Long.MAX_VALUE, "min", Long.MIN_VALUE); + map( + "a", FieldValue.numericAdd(1), + "b", FieldValue.numericAdd(Long.MAX_VALUE), + "c", FieldValue.numericAdd(1), + "d", FieldValue.numericAdd(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 testAppliesNumericAddWithoutUnderflow() { + 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.numericAdd(-1), + "b", FieldValue.numericAdd(Long.MIN_VALUE), + "c", FieldValue.numericAdd(-1), + "d", FieldValue.numericAdd(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); } From 0df4ee81b8ff7d8b25a1ac3b61d9f8d873ba43b2 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 5 Nov 2018 13:27:54 -0800 Subject: [PATCH 05/16] Review feedback --- .../firestore/NumericTransformsTest.java | 17 ++++++++++----- .../testutil/IntegrationTestUtil.java | 1 + .../google/firebase/firestore/FieldValue.java | 4 ++-- .../firebase/firestore/local/LocalStore.java | 18 ++++++++++++---- .../model/mutation/MutationBatch.java | 21 ++++++++++++------- .../NumericAddTransformOperation.java | 1 + .../model/mutation/TransformMutation.java | 4 ++++ .../firebase/firestore/proto/mutation.proto | 14 +++++++++---- .../firestore/local/LocalStoreTestCase.java | 21 +++++++++++++++++++ 9 files changed, 78 insertions(+), 23 deletions(-) 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 index 5d78c2cd3f3..8bf304c0d0a 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java @@ -30,6 +30,7 @@ import org.junit.Ignore; import org.junit.Test; +// TODO(b/119035894): Enable these tests once available in Production @Ignore("Not yet available in production") public class NumericTransformsTest { private static final double DOUBLE_EPSILON = 0.000001; @@ -88,7 +89,13 @@ public void createDocumentWithIncrement() { } @Test - public void integerIncrementExistingInteger() { + public void mergeOnNonExistingDocumentWithIncrement() { + waitFor(docRef.set(map("sum", FieldValue.numericAdd(1337)), SetOptions.merge())); + expectLocalAndRemoteValue(1337L); + } + + @Test + public void integerIncrementWithExistingInteger() { writeInitialData(map("sum", 1337L)); waitFor(docRef.update("sum", FieldValue.numericAdd(1))); expectLocalAndRemoteValue(1338L); @@ -102,28 +109,28 @@ public void doubleIncrementWithExistingDouble() { } @Test - public void integerIncrementExistingDouble() { + public void integerIncrementWithExistingDouble() { writeInitialData(map("sum", 13.37D)); waitFor(docRef.update("sum", FieldValue.numericAdd(1))); expectLocalAndRemoteValue(14.37D); } @Test - public void doubleIncrementExistingInteger() { + public void doubleIncrementWithExistingInteger() { writeInitialData(map("sum", 1337L)); waitFor(docRef.update("sum", FieldValue.numericAdd(0.1))); expectLocalAndRemoteValue(1337.1D); } @Test - public void integerIncrementExistingString() { + public void integerIncrementWithExistingString() { writeInitialData(map("sum", "overwrite")); waitFor(docRef.update("sum", FieldValue.numericAdd(1337))); expectLocalAndRemoteValue(1337L); } @Test - public void doubleIncrementExistingString() { + public void doubleIncrementWithExistingString() { writeInitialData(map("sum", "overwrite")); waitFor(docRef.update("sum", FieldValue.numericAdd(13.37))); expectLocalAndRemoteValue(13.37D); 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 c148a5daec6..bdfadf18a25 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 @@ -61,6 +61,7 @@ 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"; 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 a7bc8a3a3cd..089a34ceced 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 @@ -158,8 +158,8 @@ public static FieldValue arrayRemove(@NonNull Object... elements) { * the given value to the field's current value. * *

If the current field value is an integer, possible integer overflows are resolved to - * Long.MAX_VALUE or Long.MIN_VALUE respectively. If the current field value is a double, both - * values will be interpreted as doubles and the arithmetic will follow IEEE 754 semantics. + * 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 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. 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 80c467ce3ea..b1f4267687a 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 @@ -211,12 +211,22 @@ public LocalWriteResult writeLocally(List mutations) { List baseMutations = new ArrayList<>(); for (Mutation mutation : mutations) { MaybeDocument maybeDocument = existingDocuments.get(mutation.getKey()); - if (maybeDocument instanceof Document && !mutation.isIdempotent()) { + if (!mutation.isIdempotent()) { FieldMask fieldMask = mutation.getFieldMask(); if (fieldMask != null) { - ObjectValue baseValues = fieldMask.applyTo(((Document) maybeDocument).getData()); - baseMutations.add( - new PatchMutation(mutation.getKey(), baseValues, fieldMask, Precondition.NONE)); + if (maybeDocument instanceof Document) { + ObjectValue baseValues = fieldMask.applyTo(((Document) maybeDocument).getData()); + baseMutations.add( + new PatchMutation( + mutation.getKey(), baseValues, fieldMask, Precondition.NONE)); + } else { + baseMutations.add( + new PatchMutation( + mutation.getKey(), + ObjectValue.emptyObject(), + fieldMask, + Precondition.NONE)); + } } } } 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 68e2ff8ae74..d05253723a6 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 @@ -119,8 +119,8 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo maybeDoc.getKey()); } - // First, apply the base state. This allows us to apply certain non-idempotent transform against - // a consistent set of values. + // 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)) { @@ -143,14 +143,18 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo /** Computes the local view for all provided documents given the mutations in this batch. */ public ImmutableSortedMap applyToLocalView( ImmutableSortedMap maybeDocumentMap) { - ImmutableSortedMap changedDocuments = 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 maybeDocument = applyToLocalView(key, changedDocuments.get(key)); - if (maybeDocument != null) { - changedDocuments = changedDocuments.insert(maybeDocument.getKey(), maybeDocument); + MaybeDocument mutatedDocument = applyToLocalView(key, mutatedDocuments.get(key)); + if (mutatedDocument != null) { + mutatedDocuments = mutatedDocuments.insert(mutatedDocument.getKey(), mutatedDocument); } } - return changedDocuments; + return mutatedDocuments; } @Override @@ -225,7 +229,8 @@ public boolean isTombstone() { /** Converts this batch to a tombstone. */ public MutationBatch toTombstone() { - return new MutationBatch(batchId, localWriteTime, baseMutations, Collections.emptyList()); + return new MutationBatch( + batchId, localWriteTime, Collections.emptyList(), Collections.emptyList()); } /** @return The user-provided mutations in this mutation batch. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java index d4710b1391b..712d0c2fb85 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java @@ -58,6 +58,7 @@ public FieldValue applyToLocalView(FieldValue previousValue, Timestamp localWrit private long safeAdd(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; } 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 12a90088e72..e128582ed7b 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 @@ -123,6 +123,10 @@ public MaybeDocument applyToLocalView( @Override public FieldMask getFieldMask() { + // Note: 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. List fieldMask = new ArrayList<>(); for (FieldTransform transform : fieldTransforms) { fieldMask.add(transform.getFieldPath()); 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 11482112bf0..8f9607d56d5 100644 --- a/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto +++ b/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto @@ -56,9 +56,15 @@ message WriteBatch { // The local time at which the write batch was initiated. google.protobuf.Timestamp local_write_time = 3; - // A list of writes that are used to populate a base state when this mutation - // is applied locally. These writes are used to locally overwrite values that - // are persisted in the remote document cache. These writes are never sent to - // the backend. + // A list of "writes" that represent a partial base state from when this + // write batch was initially created. During local application of the write + // 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. numericAdd + // 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.v1beta1.Write base_writes = 4; } 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 1032b02f5e7..0a4e176b918 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 @@ -968,6 +968,9 @@ public void testHandlesSetMutationThenTransformMutationThenTransformMutation() { @Test public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransformMutation() { if (garbageCollectorIsEager()) { + // Since this test doesn't open a Query, 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; } @@ -1088,4 +1091,22 @@ public void testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransf 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 testHandlesMergeMutationThenRemoteEvent() { + 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.numericAdd(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)); + } } From e74e03c3e2c2e7f7532431d6983a745f08400ea7 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 6 Nov 2018 10:36:16 -0800 Subject: [PATCH 06/16] More Feedback --- .../testutil/IntegrationTestUtil.java | 6 +++- .../firestore/local/LocalSerializer.java | 12 ++++---- .../firebase/firestore/local/LocalStore.java | 17 ++++++----- .../firestore/local/MemoryMutationQueue.java | 4 +-- .../firestore/local/MutationQueue.java | 8 ++--- .../firestore/local/SQLiteMutationQueue.java | 4 +-- .../model/mutation/MutationBatch.java | 22 +++++++------- .../model/mutation/TransformMutation.java | 4 --- .../firestore/local/LocalSerializerTest.java | 2 +- .../firestore/local/LocalStoreTestCase.java | 29 ++++++++++++++++--- 10 files changed, 66 insertions(+), 42 deletions(-) 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 bdfadf18a25..b31764071a0 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 @@ -117,7 +117,11 @@ public static FirebaseFirestoreSettings newTestSettingsWithSnapshotTimestampsEna if (CONNECT_TO_EMULATOR) { settings.setHost(String.format("%s:%d", EMULATOR_HOST, EMULATOR_PORT)); - // Disable SSL and hostname verification + // 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 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 abe2d65369d..58fe23e7a7b 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 @@ -160,7 +160,7 @@ com.google.firebase.firestore.proto.WriteBatch encodeMutationBatch(MutationBatch result.setBatchId(batch.getBatchId()); result.setLocalWriteTime(rpcSerializer.encodeTimestamp(batch.getLocalWriteTime())); - for (Mutation mutation : batch.getBaseMutations()) { + for (Mutation mutation : batch.getBaseStateMutations()) { result.addBaseWrites(rpcSerializer.encodeMutation(mutation)); } for (Mutation mutation : batch.getMutations()) { @@ -174,17 +174,17 @@ MutationBatch decodeMutationBatch(com.google.firebase.firestore.proto.WriteBatch int batchId = batch.getBatchId(); Timestamp localWriteTime = rpcSerializer.decodeTimestamp(batch.getLocalWriteTime()); - int baseMutationsCount = batch.getBaseWritesCount(); - List baseMutations = new ArrayList<>(baseMutationsCount); - for (int i = 0; i < baseMutationsCount; i++) { - baseMutations.add(rpcSerializer.decodeMutation(batch.getBaseWrites(i))); + int baseStateMutationsCount = batch.getBaseWritesCount(); + List baseStateMutations = new ArrayList<>(baseStateMutationsCount); + for (int i = 0; i < baseStateMutationsCount; i++) { + baseStateMutations.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, baseMutations, mutations); + return new MutationBatch(batchId, localWriteTime, baseStateMutations, 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 b1f4267687a..1b3216160ee 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 @@ -33,7 +33,6 @@ 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; @@ -208,31 +207,35 @@ public LocalWriteResult writeLocally(List mutations) { // 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<>(); + List baseStateMutations = 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) { if (maybeDocument instanceof Document) { ObjectValue baseValues = fieldMask.applyTo(((Document) maybeDocument).getData()); - baseMutations.add( + baseStateMutations.add( new PatchMutation( - mutation.getKey(), baseValues, fieldMask, Precondition.NONE)); + mutation.getKey(), baseValues, fieldMask, mutation.getPrecondition())); } else { - baseMutations.add( + baseStateMutations.add( new PatchMutation( mutation.getKey(), ObjectValue.emptyObject(), fieldMask, - Precondition.NONE)); + mutation.getPrecondition())); } } } } MutationBatch batch = - mutationQueue.addMutationBatch(localWriteTime, baseMutations, mutations); + mutationQueue.addMutationBatch(localWriteTime, baseStateMutations, mutations); ImmutableSortedMap changedDocuments = batch.applyToLocalView(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 842e73e4aca..da6fc13f9ee 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 @@ -139,7 +139,7 @@ public void setLastStreamToken(ByteString streamToken) { @Override public MutationBatch addMutationBatch( - Timestamp localWriteTime, List baseMutations, List mutations) { + Timestamp localWriteTime, List baseStateMutations, List mutations) { hardAssert(!mutations.isEmpty(), "Mutation batches should not be empty"); int batchId = nextBatchId; @@ -152,7 +152,7 @@ public MutationBatch addMutationBatch( prior.getBatchId() < batchId, "Mutation batchIds must be monotonically increasing order"); } - MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseMutations, mutations); + MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseStateMutations, 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 728eb1abd99..ed451fbea7f 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 @@ -51,13 +51,13 @@ interface MutationQueue { * 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 baseStateMutations 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); + Timestamp localWriteTime, List baseStateMutations, 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 485332cdf3d..3203dca75e9 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 @@ -193,11 +193,11 @@ private void writeMutationQueueMetadata() { @Override public MutationBatch addMutationBatch( - Timestamp localWriteTime, List baseMutations, List mutations) { + Timestamp localWriteTime, List baseStateMutations, List mutations) { int batchId = nextBatchId; nextBatchId += 1; - MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseMutations, mutations); + MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseStateMutations, mutations); MessageLite proto = serializer.encodeMutationBatch(batch); db.execute( 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 d05253723a6..c6b99e3c6f5 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 @@ -52,7 +52,7 @@ public final class MutationBatch { * 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; + private final List baseStateMutations; /** * The user-provided mutations in this mutation batch. User-provided mutations are applied both @@ -63,11 +63,11 @@ public final class MutationBatch { public MutationBatch( int batchId, Timestamp localWriteTime, - List baseMutations, + List baseStateMutations, List mutations) { this.batchId = batchId; this.localWriteTime = localWriteTime; - this.baseMutations = baseMutations; + this.baseStateMutations = baseStateMutations; this.mutations = mutations; } @@ -121,8 +121,8 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo // 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); + for (int i = 0; i < baseStateMutations.size(); i++) { + Mutation mutation = baseStateMutations.get(i); if (mutation.getKey().equals(documentKey)) { maybeDoc = mutation.applyToLocalView(maybeDoc, maybeDoc, localWriteTime); } @@ -169,7 +169,7 @@ public boolean equals(Object o) { MutationBatch that = (MutationBatch) o; return batchId == that.batchId && localWriteTime.equals(that.localWriteTime) - && baseMutations.equals(that.baseMutations) + && baseStateMutations.equals(that.baseStateMutations) && mutations.equals(that.mutations); } @@ -177,7 +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 + baseStateMutations.hashCode(); result = 31 * result + mutations.hashCode(); return result; } @@ -188,8 +188,8 @@ public String toString() { + batchId + ", localWriteTime=" + localWriteTime - + ", baseMutations=" - + baseMutations + + ", baseStateMutations=" + + baseStateMutations + ", mutations=" + mutations + ')'; @@ -242,7 +242,7 @@ public List getMutations() { * @return The mutations that are used to populate the base values when this mutation batch is * applied locally. */ - public List getBaseMutations() { - return baseMutations; + public List getBaseStateMutations() { + return baseStateMutations; } } 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 e128582ed7b..12a90088e72 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 @@ -123,10 +123,6 @@ public MaybeDocument applyToLocalView( @Override public FieldMask getFieldMask() { - // Note: 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. List fieldMask = new ArrayList<>(); for (FieldTransform transform : fieldTransforms) { fieldMask.add(transform.getFieldPath()); 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 978a9238fe3..33d7d9a1201 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 @@ -137,7 +137,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.getBaseStateMutations(), decoded.getBaseStateMutations()); 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 0a4e176b918..1b06f78633d 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 @@ -950,6 +950,9 @@ public void testRemoteDocumentKeysForTarget() { assertSetEquals(asList(key("foo/bar"), key("foo/baz")), keys); } + // TODO(mrschmidt): The FieldValue.numericAdd() 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))); @@ -968,9 +971,9 @@ public void testHandlesSetMutationThenTransformMutationThenTransformMutation() { @Test public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransformMutation() { if (garbageCollectorIsEager()) { - // Since this test doesn't open a Query, 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. + // 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; } @@ -1093,7 +1096,7 @@ public void testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransf } @Test - public void testHandlesMergeMutationThenRemoteEvent() { + public void testHandlesMergeMutationWithTransformThenRemoteEvent() { Query query = Query.atPath(ResourcePath.fromString("foo")); allocateQuery(query); assertTargetId(2); @@ -1109,4 +1112,22 @@ public void testHandlesMergeMutationThenRemoteEvent() { 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.numericAdd(1))))); + assertChanged(deletedDoc("foo/bar", 0)); + assertNotContains("foo/bar"); + + 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)); + } } From 1f98e0a9b9d158da2dcecc439f34fd8754541d36 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 7 Nov 2018 10:43:31 -0800 Subject: [PATCH 07/16] Porting changes back from Web --- .../firebase/firestore/local/LocalStore.java | 11 ++- .../model/mutation/MutationBatch.java | 2 +- .../firestore/local/LocalStoreTestCase.java | 81 ++++++++++--------- 3 files changed, 47 insertions(+), 47 deletions(-) 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 1b3216160ee..f0bac957655 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 @@ -194,14 +194,13 @@ public LocalWriteResult writeLocally(List mutations) { keys.add(mutation.getKey()); } - // 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); - 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.numericAdd()`), we record the base // state in a separate patch mutation. This is later used to guarantee consistent values @@ -237,7 +236,7 @@ public LocalWriteResult writeLocally(List mutations) { MutationBatch batch = mutationQueue.addMutationBatch(localWriteTime, baseStateMutations, mutations); ImmutableSortedMap changedDocuments = - batch.applyToLocalView(existingDocuments); + batch.applyToLocalDocumentSet(existingDocuments); return new LocalWriteResult(batch.getBatchId(), changedDocuments); }); } 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 c6b99e3c6f5..76125f494d7 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 @@ -141,7 +141,7 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo } /** Computes the local view for all provided documents given the mutations in this batch. */ - public ImmutableSortedMap applyToLocalView( + 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 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 1b06f78633d..50b961d26fa 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 @@ -999,7 +999,45 @@ public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransfo } @Test - public void testHoldsBackNonIdempotentTransforms() { + 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.numericAdd(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.numericAdd(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); @@ -1047,54 +1085,17 @@ public void testHoldsBackNonIdempotentTransforms() { // ArrayUnion transform is recomputed and includes the backend value. applyRemoteEvent( addedRemoteEvent( - doc("foo/bar", 1, map("sum", 2, "array_union", Collections.singletonList("bar"))), + doc("foo/bar", 2, map("sum", 1337, "array_union", Collections.singletonList("bar"))), asList(2), emptyList())); assertChanged( doc( "foo/bar", - 1, + 2, map("sum", 1, "array_union", Arrays.asList("bar", "foo")), 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.numericAdd(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.numericAdd(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)); - - 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 testHandlesMergeMutationWithTransformThenRemoteEvent() { Query query = Query.atPath(ResourcePath.fromString("foo")); From 720d7082eb4585beed058d3d28041554970e83f8 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 7 Nov 2018 15:49:08 -0800 Subject: [PATCH 08/16] Last round of feedback, mostly reverting rename --- .../firestore/local/LocalSerializer.java | 12 ++++----- .../firebase/firestore/local/LocalStore.java | 27 +++++++++---------- .../firestore/local/MemoryMutationQueue.java | 4 +-- .../firestore/local/MutationQueue.java | 8 +++--- .../firestore/local/SQLiteMutationQueue.java | 4 +-- .../model/mutation/MutationBatch.java | 22 +++++++-------- .../firestore/local/LocalSerializerTest.java | 2 +- 7 files changed, 38 insertions(+), 41 deletions(-) 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 58fe23e7a7b..abe2d65369d 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 @@ -160,7 +160,7 @@ com.google.firebase.firestore.proto.WriteBatch encodeMutationBatch(MutationBatch result.setBatchId(batch.getBatchId()); result.setLocalWriteTime(rpcSerializer.encodeTimestamp(batch.getLocalWriteTime())); - for (Mutation mutation : batch.getBaseStateMutations()) { + for (Mutation mutation : batch.getBaseMutations()) { result.addBaseWrites(rpcSerializer.encodeMutation(mutation)); } for (Mutation mutation : batch.getMutations()) { @@ -174,17 +174,17 @@ MutationBatch decodeMutationBatch(com.google.firebase.firestore.proto.WriteBatch int batchId = batch.getBatchId(); Timestamp localWriteTime = rpcSerializer.decodeTimestamp(batch.getLocalWriteTime()); - int baseStateMutationsCount = batch.getBaseWritesCount(); - List baseStateMutations = new ArrayList<>(baseStateMutationsCount); - for (int i = 0; i < baseStateMutationsCount; i++) { - baseStateMutations.add(rpcSerializer.decodeMutation(batch.getBaseWrites(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, baseStateMutations, 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 f0bac957655..5b28b993b2c 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 @@ -33,6 +33,7 @@ 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; @@ -206,7 +207,7 @@ public LocalWriteResult writeLocally(List mutations) { // 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 baseStateMutations = new ArrayList<>(); + List baseMutations = new ArrayList<>(); for (Mutation mutation : mutations) { MaybeDocument maybeDocument = existingDocuments.get(mutation.getKey()); if (!mutation.isIdempotent()) { @@ -216,25 +217,21 @@ public LocalWriteResult writeLocally(List mutations) { // (such as `arrayUnion()`) when any non-idempotent transforms are present. FieldMask fieldMask = mutation.getFieldMask(); if (fieldMask != null) { - if (maybeDocument instanceof Document) { - ObjectValue baseValues = fieldMask.applyTo(((Document) maybeDocument).getData()); - baseStateMutations.add( - new PatchMutation( - mutation.getKey(), baseValues, fieldMask, mutation.getPrecondition())); - } else { - baseStateMutations.add( - new PatchMutation( - mutation.getKey(), - ObjectValue.emptyObject(), - fieldMask, - mutation.getPrecondition())); - } + 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, baseStateMutations, mutations); + 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 da6fc13f9ee..842e73e4aca 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 @@ -139,7 +139,7 @@ public void setLastStreamToken(ByteString streamToken) { @Override public MutationBatch addMutationBatch( - Timestamp localWriteTime, List baseStateMutations, List mutations) { + Timestamp localWriteTime, List baseMutations, List mutations) { hardAssert(!mutations.isEmpty(), "Mutation batches should not be empty"); int batchId = nextBatchId; @@ -152,7 +152,7 @@ public MutationBatch addMutationBatch( prior.getBatchId() < batchId, "Mutation batchIds must be monotonically increasing order"); } - MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseStateMutations, 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 ed451fbea7f..728eb1abd99 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 @@ -51,13 +51,13 @@ interface MutationQueue { * Creates a new mutation batch and adds it to this mutation queue. * * @param localWriteTime The original write time of this mutation. - * @param baseStateMutations 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 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 baseStateMutations, List mutations); + 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 3203dca75e9..485332cdf3d 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 @@ -193,11 +193,11 @@ private void writeMutationQueueMetadata() { @Override public MutationBatch addMutationBatch( - Timestamp localWriteTime, List baseStateMutations, List mutations) { + Timestamp localWriteTime, List baseMutations, List mutations) { int batchId = nextBatchId; nextBatchId += 1; - MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseStateMutations, 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/MutationBatch.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java index 76125f494d7..8657c8d13a4 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 @@ -52,7 +52,7 @@ public final class MutationBatch { * 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 baseStateMutations; + private final List baseMutations; /** * The user-provided mutations in this mutation batch. User-provided mutations are applied both @@ -63,11 +63,11 @@ public final class MutationBatch { public MutationBatch( int batchId, Timestamp localWriteTime, - List baseStateMutations, + List baseMutations, List mutations) { this.batchId = batchId; this.localWriteTime = localWriteTime; - this.baseStateMutations = baseStateMutations; + this.baseMutations = baseMutations; this.mutations = mutations; } @@ -121,8 +121,8 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo // First, apply the base state. This allows us to apply non-idempotent transform against a // consistent set of values. - for (int i = 0; i < baseStateMutations.size(); i++) { - Mutation mutation = baseStateMutations.get(i); + for (int i = 0; i < baseMutations.size(); i++) { + Mutation mutation = baseMutations.get(i); if (mutation.getKey().equals(documentKey)) { maybeDoc = mutation.applyToLocalView(maybeDoc, maybeDoc, localWriteTime); } @@ -169,7 +169,7 @@ public boolean equals(Object o) { MutationBatch that = (MutationBatch) o; return batchId == that.batchId && localWriteTime.equals(that.localWriteTime) - && baseStateMutations.equals(that.baseStateMutations) + && baseMutations.equals(that.baseMutations) && mutations.equals(that.mutations); } @@ -177,7 +177,7 @@ public boolean equals(Object o) { public int hashCode() { int result = batchId; result = 31 * result + localWriteTime.hashCode(); - result = 31 * result + baseStateMutations.hashCode(); + result = 31 * result + baseMutations.hashCode(); result = 31 * result + mutations.hashCode(); return result; } @@ -188,8 +188,8 @@ public String toString() { + batchId + ", localWriteTime=" + localWriteTime - + ", baseStateMutations=" - + baseStateMutations + + ", baseMutations=" + + baseMutations + ", mutations=" + mutations + ')'; @@ -242,7 +242,7 @@ public List getMutations() { * @return The mutations that are used to populate the base values when this mutation batch is * applied locally. */ - public List getBaseStateMutations() { - return baseStateMutations; + public List getBaseMutations() { + return baseMutations; } } 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 33d7d9a1201..978a9238fe3 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 @@ -137,7 +137,7 @@ public void testEncodesMutationBatch() { assertEquals(model.getBatchId(), decoded.getBatchId()); assertEquals(model.getLocalWriteTime(), decoded.getLocalWriteTime()); assertEquals(model.getMutations(), decoded.getMutations()); - assertEquals(model.getBaseStateMutations(), decoded.getBaseStateMutations()); + assertEquals(model.getBaseMutations(), decoded.getBaseMutations()); assertEquals(model.getKeys(), decoded.getKeys()); } From 2a4807f4e9097821da5e52345deba9ada55819d8 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 7 Nov 2018 15:57:00 -0800 Subject: [PATCH 09/16] Adding comment --- .../google/firebase/firestore/local/LocalStoreTestCase.java | 3 +++ 1 file changed, 3 insertions(+) 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 50b961d26fa..66f6e91861c 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 @@ -1127,6 +1127,9 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() { 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)); From f4cf0397242a196dd416092b6089b0773fc3c512 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 8 Nov 2018 13:42:23 -0800 Subject: [PATCH 10/16] Enable tests --- .../com/google/firebase/firestore/NumericTransformsTest.java | 2 -- 1 file changed, 2 deletions(-) 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 index 8bf304c0d0a..38633f7c40d 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java @@ -30,8 +30,6 @@ import org.junit.Ignore; import org.junit.Test; -// TODO(b/119035894): Enable these tests once available in Production -@Ignore("Not yet available in production") public class NumericTransformsTest { private static final double DOUBLE_EPSILON = 0.000001; From 3f2e09e86f1ab5043c7fc3f44cefedc4cab754db Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 8 Nov 2018 14:14:38 -0800 Subject: [PATCH 11/16] Running Google Java Format --- .../com/google/firebase/firestore/NumericTransformsTest.java | 1 - .../com/google/firebase/firestore/local/LocalStoreTestCase.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) 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 index 38633f7c40d..0e27cade73a 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java @@ -27,7 +27,6 @@ import java.util.concurrent.ExecutionException; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; public class NumericTransformsTest { 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 66f6e91861c..62bbe0d7aaf 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 @@ -1129,7 +1129,7 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() { // 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)); From 261c7c8b61163c22e23c1b735a9a946549e67aa9 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 12 Nov 2018 16:24:44 -0800 Subject: [PATCH 12/16] Update Proto with wording from iOS --- .../proto/google/firebase/firestore/proto/mutation.proto | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 8f9607d56d5..9de9a6bdaf5 100644 --- a/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto +++ b/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto @@ -56,9 +56,9 @@ message WriteBatch { // The local time at which the write batch was initiated. google.protobuf.Timestamp local_write_time = 3; - // A list of "writes" that represent a partial base state from when this - // write batch was initially created. During local application of the write - // batch, these base_writes are applied prior to the real writes in order to + // 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. numericAdd // transforms) to make sure that the local view of the modified documents From bb10ecb4070814e2dc3c522d073c1611ca3a3e22 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 13 Nov 2018 12:02:44 -0800 Subject: [PATCH 13/16] Rename numericAdd to increment --- .../firestore/NumericTransformsTest.java | 22 +++---- .../google/firebase/firestore/FieldValue.java | 24 ++++---- .../firebase/firestore/UserDataConverter.java | 15 +++-- .../firebase/firestore/local/LocalStore.java | 2 +- ...> NumericIncrementTransformOperation.java} | 13 +++-- .../firestore/remote/RemoteSerializer.java | 9 +-- .../firebase/firestore/proto/mutation.proto | 2 +- .../firestore/local/LocalStoreTestCase.java | 20 +++---- .../firestore/model/MutationTest.java | 58 +++++++++---------- 9 files changed, 85 insertions(+), 80 deletions(-) rename firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/{NumericAddTransformOperation.java => NumericIncrementTransformOperation.java} (89%) 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 index 0e27cade73a..83f41e8109e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java @@ -81,55 +81,55 @@ private void expectLocalAndRemoteValue(long expectedSum) { @Test public void createDocumentWithIncrement() { - waitFor(docRef.set(map("sum", FieldValue.numericAdd(1337)))); + waitFor(docRef.set(map("sum", FieldValue.increment(1337)))); expectLocalAndRemoteValue(1337L); } @Test public void mergeOnNonExistingDocumentWithIncrement() { - waitFor(docRef.set(map("sum", FieldValue.numericAdd(1337)), SetOptions.merge())); + 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.numericAdd(1))); + waitFor(docRef.update("sum", FieldValue.increment(1))); expectLocalAndRemoteValue(1338L); } @Test public void doubleIncrementWithExistingDouble() { writeInitialData(map("sum", 13.37D)); - waitFor(docRef.update("sum", FieldValue.numericAdd(0.1))); + 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.numericAdd(1))); + waitFor(docRef.update("sum", FieldValue.increment(1))); expectLocalAndRemoteValue(14.37D); } @Test public void doubleIncrementWithExistingInteger() { writeInitialData(map("sum", 1337L)); - waitFor(docRef.update("sum", FieldValue.numericAdd(0.1))); + waitFor(docRef.update("sum", FieldValue.increment(0.1))); expectLocalAndRemoteValue(1337.1D); } @Test public void integerIncrementWithExistingString() { writeInitialData(map("sum", "overwrite")); - waitFor(docRef.update("sum", FieldValue.numericAdd(1337))); + waitFor(docRef.update("sum", FieldValue.increment(1337))); expectLocalAndRemoteValue(1337L); } @Test public void doubleIncrementWithExistingString() { writeInitialData(map("sum", "overwrite")); - waitFor(docRef.update("sum", FieldValue.numericAdd(13.37))); + waitFor(docRef.update("sum", FieldValue.increment(13.37))); expectLocalAndRemoteValue(13.37D); } @@ -139,9 +139,9 @@ public void multipleDoubleIncrements() throws ExecutionException, InterruptedExc Tasks.await(docRef.getFirestore().disableNetwork()); - docRef.update("sum", FieldValue.numericAdd(0.1D)); - docRef.update("sum", FieldValue.numericAdd(0.01D)); - docRef.update("sum", FieldValue.numericAdd(0.001D)); + 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); 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 089a34ceced..c8c53834810 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 @@ -83,17 +83,17 @@ List getElements() { } } - /* FieldValue class for numericAdd() transforms. */ - static class NumericAddFieldValue extends FieldValue { + /* FieldValue class for increment() transforms. */ + static class NumericIncrementFieldValue extends FieldValue { private final Number operand; - NumericAddFieldValue(Number operand) { + NumericIncrementFieldValue(Number operand) { this.operand = operand; } @Override String getMethodName() { - return "FieldValue.numericAdd"; + return "FieldValue.increment"; } Number getOperand() { @@ -154,8 +154,8 @@ public static FieldValue arrayRemove(@NonNull Object... elements) { } /** - * Returns a special value that can be used with set() or update() that tells the server to add - * the given value to the field's current value. + * 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 @@ -168,13 +168,13 @@ public static FieldValue arrayRemove(@NonNull Object... elements) { */ @NonNull @PublicApi - public static FieldValue numericAdd(long l) { - return new NumericAddFieldValue(l); + 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 add - * the given value to the field's current value. + * 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 @@ -184,7 +184,7 @@ public static FieldValue numericAdd(long l) { */ @NonNull @PublicApi - public static FieldValue numericAdd(double l) { - return new NumericAddFieldValue(l); + 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 72c2e0b2531..d7347ec22a0 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,7 +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.NumericAddTransformOperation; +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; @@ -351,11 +351,14 @@ private void parseSentinelFieldValue( ArrayTransformOperation arrayRemove = new ArrayTransformOperation.Remove(parsedElements); context.addToFieldTransforms(context.getPath(), arrayRemove); - } else if (value instanceof com.google.firebase.firestore.FieldValue.NumericAddFieldValue) { - com.google.firebase.firestore.FieldValue.NumericAddFieldValue numericAddFieldValue = - (com.google.firebase.firestore.FieldValue.NumericAddFieldValue) value; - NumberValue operand = (NumberValue) parseQueryValue(numericAddFieldValue.getOperand()); - NumericAddTransformOperation numericAdd = new NumericAddTransformOperation(operand); + } 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 numericAdd = + new NumericIncrementTransformOperation(operand); context.addToFieldTransforms(context.getPath(), numericAdd); } else { 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 5b28b993b2c..9750cdf7c93 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 @@ -203,7 +203,7 @@ public LocalWriteResult writeLocally(List mutations) { ImmutableSortedMap existingDocuments = localDocuments.getDocuments(keys); - // For non-idempotent mutations (such as `FieldValue.numericAdd()`), we record the base + // 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. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java similarity index 89% rename from firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java rename to firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java index 712d0c2fb85..baf92294558 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericAddTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java @@ -23,13 +23,14 @@ import com.google.firebase.firestore.model.value.NumberValue; /** - * Implements the backend semantics for locally computed NUMERIC_ADD transforms. Converts all field - * values to longs or doubles and resolves overflows to Long.MAX_VALUE/Long.MIN_VALUE. + * 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 NumericAddTransformOperation implements TransformOperation { +public class NumericIncrementTransformOperation implements TransformOperation { private NumberValue operand; - public NumericAddTransformOperation(NumberValue operand) { + public NumericIncrementTransformOperation(NumberValue operand) { this.operand = operand; } @@ -37,7 +38,7 @@ public NumericAddTransformOperation(NumberValue operand) { 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 = safeAdd(((IntegerValue) previousValue).getInternalValue(), operandAsLong()); + long sum = safeIncrement(((IntegerValue) previousValue).getInternalValue(), operandAsLong()); return IntegerValue.valueOf(sum); } else if (previousValue instanceof IntegerValue) { double sum = ((IntegerValue) previousValue).getInternalValue() + operandAsDouble(); @@ -55,7 +56,7 @@ public FieldValue applyToLocalView(FieldValue previousValue, Timestamp localWrit * 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 safeAdd(long x, long y) { + 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 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 b403e75d4e0..777aed9b47d 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,7 +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.NumericAddTransformOperation; +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; @@ -560,8 +560,9 @@ private DocumentTransform.FieldTransform encodeFieldTransform(FieldTransform fie .setFieldPath(fieldTransform.getFieldPath().canonicalString()) .setRemoveAllFromArray(encodeArrayTransformElements(remove.getElements())) .build(); - } else if (transform instanceof NumericAddTransformOperation) { - NumericAddTransformOperation numericAdd = (NumericAddTransformOperation) transform; + } else if (transform instanceof NumericIncrementTransformOperation) { + NumericIncrementTransformOperation numericAdd = + (NumericIncrementTransformOperation) transform; return DocumentTransform.FieldTransform.newBuilder() .setFieldPath(fieldTransform.getFieldPath().canonicalString()) .setNumericAdd(encodeValue(numericAdd.getOperand())) @@ -611,7 +612,7 @@ private FieldTransform decodeFieldTransform(DocumentTransform.FieldTransform fie operand.getClass().getCanonicalName()); return new FieldTransform( FieldPath.fromServerFormat(fieldTransform.getFieldPath()), - new NumericAddTransformOperation( + new NumericIncrementTransformOperation( (NumberValue) decodeValue(fieldTransform.getNumericAdd()))); } default: 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 9de9a6bdaf5..96fae103804 100644 --- a/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto +++ b/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto @@ -60,7 +60,7 @@ message WriteBatch { // 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. numericAdd + // 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. 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 62bbe0d7aaf..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 @@ -950,7 +950,7 @@ public void testRemoteDocumentKeysForTarget() { assertSetEquals(asList(key("foo/bar"), key("foo/baz")), keys); } - // TODO(mrschmidt): The FieldValue.numericAdd() field transform tests below would probably be + // 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 @@ -959,11 +959,11 @@ public void testHandlesSetMutationThenTransformMutationThenTransformMutation() { 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.numericAdd(1)))); + 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.numericAdd(2)))); + 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)); } @@ -985,7 +985,7 @@ public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransfo 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.numericAdd(1)))); + 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)); @@ -993,7 +993,7 @@ public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransfo 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.numericAdd(2)))); + 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)); } @@ -1013,7 +1013,7 @@ public void testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransf 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.numericAdd(1)))); + 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)); @@ -1023,7 +1023,7 @@ public void testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransf 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.numericAdd(2)))); + 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)); @@ -1072,7 +1072,7 @@ public void testHoldsBackOnlyNonIdempotentTransforms() { writeMutations( Arrays.asList( - transformMutation("foo/bar", map("sum", FieldValue.numericAdd(1))), + transformMutation("foo/bar", map("sum", FieldValue.increment(1))), transformMutation("foo/bar", map("array_union", FieldValue.arrayUnion("foo"))))); assertChanged( doc( @@ -1105,7 +1105,7 @@ public void testHandlesMergeMutationWithTransformThenRemoteEvent() { writeMutations( asList( patchMutation("foo/bar", map(), Collections.emptyList()), - transformMutation("foo/bar", map("sum", FieldValue.numericAdd(1))))); + 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)); @@ -1123,7 +1123,7 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() { writeMutations( asList( patchMutation("foo/bar", map()), - transformMutation("foo/bar", map("sum", FieldValue.numericAdd(1))))); + transformMutation("foo/bar", map("sum", FieldValue.increment(1))))); assertChanged(deletedDoc("foo/bar", 0)); assertNotContains("foo/bar"); 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 bfb94f6e3f6..c9688cfe467 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 @@ -169,7 +169,7 @@ public void testAppliesLocalServerTimestampTransformsToDocuments() { } @Test - public void testAppliesNumericAddTransformToDocument() { + public void testAppliesIncrementTransformToDocument() { Map baseDoc = map( "longPlusLong", @@ -191,21 +191,21 @@ public void testAppliesNumericAddTransformToDocument() { Map transform = map( "longPlusLong", - FieldValue.numericAdd(1), + FieldValue.increment(1), "longPlusDouble", - FieldValue.numericAdd(2.2), + FieldValue.increment(2.2), "doublePlusLong", - FieldValue.numericAdd(3), + FieldValue.increment(3), "doublePlusDouble", - FieldValue.numericAdd(4.4), + FieldValue.increment(4.4), "longPlusNan", - FieldValue.numericAdd(Double.NaN), + FieldValue.increment(Double.NaN), "doublePlusNan", - FieldValue.numericAdd(Double.NaN), + FieldValue.increment(Double.NaN), "longPlusInfinity", - FieldValue.numericAdd(Double.POSITIVE_INFINITY), + FieldValue.increment(Double.POSITIVE_INFINITY), "doublePlusInfinity", - FieldValue.numericAdd(Double.POSITIVE_INFINITY)); + FieldValue.increment(Double.POSITIVE_INFINITY)); Map expected = map( "longPlusLong", @@ -229,33 +229,33 @@ public void testAppliesNumericAddTransformToDocument() { } @Test - public void testAppliesNumericAddTransformToUnexpectedType() { + public void testAppliesIncrementTransformToUnexpectedType() { Map baseDoc = map("string", "zero"); - Map transform = map("string", FieldValue.numericAdd(1)); + Map transform = map("string", FieldValue.increment(1)); Map expected = map("string", 1); verifyTransform(baseDoc, transform, expected); } @Test - public void testAppliesNumericAddTransformToMissingField() { + public void testAppliesIncrementTransformToMissingField() { Map baseDoc = map(); - Map transform = map("missing", FieldValue.numericAdd(1)); + Map transform = map("missing", FieldValue.increment(1)); Map expected = map("missing", 1); verifyTransform(baseDoc, transform, expected); } @Test - public void testAppliesNumericAddTransformsConsecutively() { + public void testAppliesIncrementTransformsConsecutively() { Map baseDoc = map("number", 1); - Map transform1 = map("number", FieldValue.numericAdd(2)); - Map transform2 = map("number", FieldValue.numericAdd(3)); - Map transform3 = map("number", FieldValue.numericAdd(4)); + 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 testAppliesNumericAddWithoutOverflow() { + public void testAppliesIncrementWithoutOverflow() { Map baseDoc = map( "a", @@ -268,17 +268,17 @@ public void testAppliesNumericAddWithoutOverflow() { Long.MAX_VALUE); Map transform = map( - "a", FieldValue.numericAdd(1), - "b", FieldValue.numericAdd(Long.MAX_VALUE), - "c", FieldValue.numericAdd(1), - "d", FieldValue.numericAdd(Long.MAX_VALUE)); + "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 testAppliesNumericAddWithoutUnderflow() { + public void testAppliesIncrementWithoutUnderflow() { Map baseDoc = map( "a", @@ -291,10 +291,10 @@ public void testAppliesNumericAddWithoutUnderflow() { Long.MIN_VALUE); Map transform = map( - "a", FieldValue.numericAdd(-1), - "b", FieldValue.numericAdd(Long.MIN_VALUE), - "c", FieldValue.numericAdd(-1), - "d", FieldValue.numericAdd(Long.MIN_VALUE)); + "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); @@ -480,11 +480,11 @@ private void verifyTransform( } @Test - public void testAppliesServerAckedNumericAddTransformToDocuments() { + public void testAppliesServerAckedIncrementTransformToDocuments() { Map data = map("sum", 1); Document baseDoc = doc("collection/key", 0, data); - Mutation transform = transformMutation("collection/key", map("sum", FieldValue.numericAdd(2))); + Mutation transform = transformMutation("collection/key", map("sum", FieldValue.increment(2))); MutationResult mutationResult = new MutationResult(version(1), Collections.singletonList(IntegerValue.valueOf(3L))); From 5b4f0a29fd77d1a68cefd05516993293315391ee Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Sun, 18 Nov 2018 18:25:35 -0800 Subject: [PATCH 14/16] Fix wording --- .../main/java/com/google/firebase/firestore/FieldValue.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 c8c53834810..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 @@ -161,8 +161,8 @@ public static FieldValue arrayRemove(@NonNull Object... elements) { * 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 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. + *

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(). */ From 991fb7c752e7abc6f929aa381ac86e0f4c97f148 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 26 Dec 2018 09:15:17 -0800 Subject: [PATCH 15/16] Update Protobuf to use increment() --- .../google/firebase/firestore/UserDataConverter.java | 4 ++-- .../google/firebase/firestore/local/LocalStore.java | 1 - .../firebase/firestore/model/mutation/FieldMask.java | 1 - .../firestore/model/mutation/TransformMutation.java | 6 ++++-- .../firebase/firestore/remote/RemoteSerializer.java | 10 +++++----- .../src/proto/google/firestore/v1beta1/write.proto | 2 +- .../firebase/firestore/local/LocalSerializerTest.java | 4 +++- 7 files changed, 15 insertions(+), 13 deletions(-) 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 a854de99e37..a5741879398 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 @@ -350,9 +350,9 @@ private void parseSentinelFieldValue( numericIncrementFieldValue = (com.google.firebase.firestore.FieldValue.NumericIncrementFieldValue) value; NumberValue operand = (NumberValue) parseQueryValue(numericIncrementFieldValue.getOperand()); - NumericIncrementTransformOperation numericAdd = + NumericIncrementTransformOperation incrementOperation = new NumericIncrementTransformOperation(operand); - context.addToFieldTransforms(context.getPath(), numericAdd); + 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/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 2e5e5f6ff99..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 @@ -39,7 +39,6 @@ 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; 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 7b48459275c..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 @@ -17,7 +17,6 @@ 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; /** 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 12a90088e72..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; /** @@ -123,11 +125,11 @@ public MaybeDocument applyToLocalView( @Override public FieldMask getFieldMask() { - List fieldMask = new ArrayList<>(); + Set fieldMask = new HashSet<>(); for (FieldTransform transform : fieldTransforms) { fieldMask.add(transform.getFieldPath()); } - return FieldMask.fromCollection(fieldMask); + return FieldMask.fromSet(fieldMask); } @Override 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 d905496d51e..44a2fa115cb 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 @@ -563,11 +563,11 @@ private DocumentTransform.FieldTransform encodeFieldTransform(FieldTransform fie .setRemoveAllFromArray(encodeArrayTransformElements(remove.getElements())) .build(); } else if (transform instanceof NumericIncrementTransformOperation) { - NumericIncrementTransformOperation numericAdd = + NumericIncrementTransformOperation incrementOperation = (NumericIncrementTransformOperation) transform; return DocumentTransform.FieldTransform.newBuilder() .setFieldPath(fieldTransform.getFieldPath().canonicalString()) - .setNumericAdd(encodeValue(numericAdd.getOperand())) + .setIncrement(encodeValue(incrementOperation.getOperand())) .build(); } else { throw fail("Unknown transform: %s", transform); @@ -605,9 +605,9 @@ private FieldTransform decodeFieldTransform(DocumentTransform.FieldTransform fie FieldPath.fromServerFormat(fieldTransform.getFieldPath()), new ArrayTransformOperation.Remove( decodeArrayTransformElements(fieldTransform.getRemoveAllFromArray()))); - case NUMERIC_ADD: + case INCREMENT: { - FieldValue operand = decodeValue(fieldTransform.getNumericAdd()); + FieldValue operand = decodeValue(fieldTransform.getIncrement()); hardAssert( operand instanceof NumberValue, "Expected NUMERIC_ADD transform to be of number type, but was %s", @@ -615,7 +615,7 @@ private FieldTransform decodeFieldTransform(DocumentTransform.FieldTransform fie return new FieldTransform( FieldPath.fromServerFormat(fieldTransform.getFieldPath()), new NumericIncrementTransformOperation( - (NumberValue) decodeValue(fieldTransform.getNumericAdd()))); + (NumberValue) decodeValue(fieldTransform.getIncrement()))); } default: throw fail("Unknown FieldTransform proto: %s", fieldTransform); diff --git a/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto b/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto index c50868573e7..b7250a65c98 100644 --- a/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto +++ b/firebase-firestore/src/proto/google/firestore/v1beta1/write.proto @@ -113,7 +113,7 @@ message DocumentTransform { // representation of double values follow IEEE 754 semantics. // If there is positive/negative integer overflow, the field is resolved // to the largest magnitude positive/negative integer. - Value numeric_add = 3; + Value increment = 3; // Append the given elements in order if they are not already present in // the current field value. 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 4a7bb69f7ad..52abbc51c0d 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; @@ -70,7 +72,7 @@ public void testEncodesMutationBatch() { new PatchMutation( key("foo/bar"), TestUtil.wrapObject(map("a", "b")), - FieldMask.fromCollection(asList(field("a"))), + 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 = From 468c282d7bfa61c800841f21f9a51c6bbb028ad4 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 6 Mar 2019 11:12:30 -0800 Subject: [PATCH 16/16] v1 Update --- .../src/proto/google/firebase/firestore/proto/mutation.proto | 2 +- .../google/firebase/firestore/local/LocalSerializerTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 a2a957c0979..23d4b66b661 100644 --- a/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto +++ b/firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto @@ -66,5 +66,5 @@ message WriteBatch { // non-idempotent write before the write is removed from the queue. // // These writes are never sent to the backend. - repeated google.firestore.v1beta1.Write base_writes = 4; + 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 9d32a21d363..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 @@ -90,7 +90,7 @@ public void testEncodesMutationBatch() { Write baseWriteProto = Write.newBuilder() .setUpdate( - com.google.firestore.v1beta1.Document.newBuilder() + 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"))