Remove unused baseDoc arg and iterate fowards in local serializer (#4367)

Author: Brian Chen

packages/firestore/src/local/local_documents_view.ts

@@ -268,7 +268,6 @@ export class LocalDocumentsView {
               const mutatedDoc = applyMutationToLocalView(
                 mutation,
                 baseDoc,
-                baseDoc,
                 batch.localWriteTime
               );
               if (mutatedDoc instanceof Document) {

packages/firestore/src/local/local_serializer.ts

@@ -201,19 +201,21 @@ export function fromDbMutationBatch(
   // on the SDK means that old `transform` mutations stored in IndexedDB need
   // to be updated to `update_transforms`.
   // TODO(b/174608374): Remove this code once we perform a schema migration.
-  for (let i = dbBatch.mutations.length - 1; i >= 0; --i) {
-    const mutationProto = dbBatch.mutations[i];
-    if (mutationProto?.transform !== undefined) {
+  for (let i = 0; i < dbBatch.mutations.length - 1; ++i) {
+    const currentMutation = dbBatch.mutations[i];
+    const hasTransform =
+      i + 1 < dbBatch.mutations.length &&
+      dbBatch.mutations[i + 1].transform !== undefined;
+    if (hasTransform) {
       debugAssert(
-        i >= 1 &&
-          dbBatch.mutations[i - 1].transform === undefined &&
-          dbBatch.mutations[i - 1].update !== undefined,
+        dbBatch.mutations[i].transform === undefined &&
+          dbBatch.mutations[i].update !== undefined,
         'TransformMutation should be preceded by a patch or set mutation'
       );
-      const mutationToJoin = dbBatch.mutations[i - 1];
-      mutationToJoin.updateTransforms = mutationProto.transform.fieldTransforms;
-      dbBatch.mutations.splice(i, 1);
-      --i;
+      const transformMutation = dbBatch.mutations[i + 1];
+      currentMutation.updateTransforms = transformMutation.transform!.fieldTransforms;
+      dbBatch.mutations.splice(i + 1, 1);
+      ++i;
     }
   }
 

packages/firestore/src/model/mutation.ts

@@ -270,9 +270,6 @@ export function applyMutationToRemoteDocument(
  * @param mutation - The mutation to apply.
  * @param maybeDoc - The document to mutate. The input document can be null if
  *     the client has no knowledge of the pre-mutation state of the document.
- * @param baseDoc - The state of the document prior to this mutation batch. The
- *     input document can be null if the client has no knowledge of the
- *     pre-mutation state of the document.
  * @param localWriteTime - A timestamp indicating the local write time of the
  *     batch this mutation is a part of.
  * @returns The mutated document. The returned document may be null, but only
@@ -281,25 +278,14 @@ export function applyMutationToRemoteDocument(
 export function applyMutationToLocalView(
   mutation: Mutation,
   maybeDoc: MaybeDocument | null,
-  baseDoc: MaybeDocument | null,
   localWriteTime: Timestamp
 ): MaybeDocument | null {
   verifyMutationKeyMatches(mutation, maybeDoc);
 
   if (mutation instanceof SetMutation) {
-    return applySetMutationToLocalView(
-      mutation,
-      maybeDoc,
-      localWriteTime,
-      baseDoc
-    );
+    return applySetMutationToLocalView(mutation, maybeDoc, localWriteTime);
   } else if (mutation instanceof PatchMutation) {
-    return applyPatchMutationToLocalView(
-      mutation,
-      maybeDoc,
-      localWriteTime,
-      baseDoc
-    );
+    return applyPatchMutationToLocalView(mutation, maybeDoc, localWriteTime);
   } else {
     debugAssert(
       mutation instanceof DeleteMutation,
@@ -469,8 +455,7 @@ function applySetMutationToRemoteDocument(
 function applySetMutationToLocalView(
   mutation: SetMutation,
   maybeDoc: MaybeDocument | null,
-  localWriteTime: Timestamp,
-  baseDoc: MaybeDocument | null
+  localWriteTime: Timestamp
 ): MaybeDocument | null {
   if (!preconditionIsValidForDocument(mutation.precondition, maybeDoc)) {
     return maybeDoc;
@@ -480,8 +465,7 @@ function applySetMutationToLocalView(
   const transformResults = localTransformResults(
     mutation.fieldTransforms,
     localWriteTime,
-    maybeDoc,
-    baseDoc
+    maybeDoc
   );
   newData = transformObject(
     mutation.fieldTransforms,
@@ -551,8 +535,7 @@ function applyPatchMutationToRemoteDocument(
 function applyPatchMutationToLocalView(
   mutation: PatchMutation,
   maybeDoc: MaybeDocument | null,
-  localWriteTime: Timestamp,
-  baseDoc: MaybeDocument | null
+  localWriteTime: Timestamp
 ): MaybeDocument | null {
   if (!preconditionIsValidForDocument(mutation.precondition, maybeDoc)) {
     return maybeDoc;
@@ -562,8 +545,7 @@ function applyPatchMutationToLocalView(
   const transformResults = localTransformResults(
     mutation.fieldTransforms,
     localWriteTime,
-    maybeDoc,
-    baseDoc
+    maybeDoc
   );
   const newData = patchDocument(mutation, maybeDoc, transformResults);
   return new Document(mutation.key, version, newData, {
@@ -613,13 +595,14 @@ function patchObject(mutation: PatchMutation, data: ObjectValue): ObjectValue {
  * containing transforms has been acknowledged by the server.
  *
  * @param fieldTransforms - The field transforms to apply the result to.
- * @param baseDoc - The document prior to applying this mutation batch.
+ * @param maybeDoc - The current state of the document after applying all
+ * previous mutations.
  * @param serverTransformResults - The transform results received by the server.
  * @returns The transform results list.
  */
 function serverTransformResults(
   fieldTransforms: FieldTransform[],
-  baseDoc: MaybeDocument | null,
+  maybeDoc: MaybeDocument | null,
   serverTransformResults: Array<ProtoValue | null>
 ): ProtoValue[] {
   const transformResults: ProtoValue[] = [];
@@ -633,8 +616,8 @@ function serverTransformResults(
     const fieldTransform = fieldTransforms[i];
     const transform = fieldTransform.transform;
     let previousValue: ProtoValue | null = null;
-    if (baseDoc instanceof Document) {
-      previousValue = baseDoc.field(fieldTransform.field);
+    if (maybeDoc instanceof Document) {
+      previousValue = maybeDoc.field(fieldTransform.field);
     }
     transformResults.push(
       applyTransformOperationToRemoteDocument(
@@ -657,14 +640,12 @@ function serverTransformResults(
  *     generate ServerTimestampValues).
  * @param maybeDoc - The current state of the document after applying all
  *     previous mutations.
- * @param baseDoc - The document prior to applying this mutation batch.
  * @returns The transform results list.
  */
 function localTransformResults(
   fieldTransforms: FieldTransform[],
   localWriteTime: Timestamp,
-  maybeDoc: MaybeDocument | null,
-  baseDoc: MaybeDocument | null
+  maybeDoc: MaybeDocument | null
 ): ProtoValue[] {
   const transformResults: ProtoValue[] = [];
   for (const fieldTransform of fieldTransforms) {

packages/firestore/src/model/mutation_batch.ts

@@ -132,21 +132,17 @@ export class MutationBatch {
         maybeDoc = applyMutationToLocalView(
           mutation,
           maybeDoc,
-          maybeDoc,
           this.localWriteTime
         );
       }
     }
 
-    const baseDoc = maybeDoc;
-
     // Second, apply all user-provided mutations.
     for (const mutation of this.mutations) {
       if (mutation.key.isEqual(docKey)) {
         maybeDoc = applyMutationToLocalView(
           mutation,
           maybeDoc,
-          baseDoc,
           this.localWriteTime
         );
       }

packages/firestore/test/unit/local/local_serializer.test.ts

@@ -129,21 +129,6 @@ describe('Local Serializer', () => {
     );
   });
 
-  it('TransformMutation (legacy) on its own throws assertion', () => {
-    const dbMutationBatch = new DbMutationBatch(
-      userId,
-      batchId,
-      1000,
-      [],
-      [transformMutationWrite]
-    );
-    expect(() =>
-      fromDbMutationBatch(localSerializer, dbMutationBatch)
-    ).to.throw(
-      'TransformMutation should be preceded by a patch or set mutation'
-    );
-  });
-
   it('DeleteMutation + TransformMutation (legacy) on its own throws assertion', () => {
     const dbMutationBatch = new DbMutationBatch(
       userId,

packages/firestore/test/unit/model/mutation.test.ts

@@ -61,7 +61,7 @@ describe('Mutation', () => {
     const baseDoc = doc('collection/key', 0, docData);
 
     const set = setMutation('collection/key', { bar: 'bar-value' });
-    const setDoc = applyMutationToLocalView(set, baseDoc, baseDoc, timestamp);
+    const setDoc = applyMutationToLocalView(set, baseDoc, timestamp);
     expect(setDoc).to.deep.equal(
       doc(
         'collection/key',
@@ -80,12 +80,7 @@ describe('Mutation', () => {
       'foo.bar': 'new-bar-value'
     });
 
-    const patchedDoc = applyMutationToLocalView(
-      patch,
-      baseDoc,
-      baseDoc,
-      timestamp
-    );
+    const patchedDoc = applyMutationToLocalView(patch, baseDoc, timestamp);
     expect(patchedDoc).to.deep.equal(
       doc(
         'collection/key',
@@ -106,12 +101,7 @@ describe('Mutation', () => {
       Precondition.none()
     );
 
-    const patchedDoc = applyMutationToLocalView(
-      patch,
-      baseDoc,
-      baseDoc,
-      timestamp
-    );
+    const patchedDoc = applyMutationToLocalView(patch, baseDoc, timestamp);
     expect(patchedDoc).to.deep.equal(
       doc(
         'collection/key',
@@ -132,12 +122,7 @@ describe('Mutation', () => {
       Precondition.none()
     );
 
-    const patchedDoc = applyMutationToLocalView(
-      patch,
-      baseDoc,
-      baseDoc,
-      timestamp
-    );
+    const patchedDoc = applyMutationToLocalView(patch, baseDoc, timestamp);
     expect(patchedDoc).to.deep.equal(
       doc(
         'collection/key',
@@ -156,12 +141,7 @@ describe('Mutation', () => {
       'foo.bar': FieldValue.delete()
     });
 
-    const patchedDoc = applyMutationToLocalView(
-      patch,
-      baseDoc,
-      baseDoc,
-      timestamp
-    );
+    const patchedDoc = applyMutationToLocalView(patch, baseDoc, timestamp);
     expect(patchedDoc).to.deep.equal(
       doc(
         'collection/key',
@@ -181,12 +161,7 @@ describe('Mutation', () => {
       'foo.bar': 'new-bar-value'
     });
 
-    const patchedDoc = applyMutationToLocalView(
-      patch,
-      baseDoc,
-      baseDoc,
-      timestamp
-    );
+    const patchedDoc = applyMutationToLocalView(patch, baseDoc, timestamp);
     expect(patchedDoc).to.deep.equal(
       doc(
         'collection/key',
@@ -200,12 +175,7 @@ describe('Mutation', () => {
   it('patching a NoDocument yields a NoDocument', () => {
     const baseDoc = deletedDoc('collection/key', 0);
     const patch = patchMutation('collection/key', { foo: 'bar' });
-    const patchedDoc = applyMutationToLocalView(
-      patch,
-      baseDoc,
-      baseDoc,
-      timestamp
-    );
+    const patchedDoc = applyMutationToLocalView(patch, baseDoc, timestamp);
     expect(patchedDoc).to.deep.equal(baseDoc);
   });
 
@@ -220,7 +190,6 @@ describe('Mutation', () => {
     const transformedDoc = applyMutationToLocalView(
       transform,
       baseDoc,
-      baseDoc,
       timestamp
     );
 
@@ -397,7 +366,6 @@ describe('Mutation', () => {
       transformedDoc = applyMutationToLocalView(
         transform,
         transformedDoc,
-        transformedDoc,
         timestamp
       );
     }
@@ -552,12 +520,7 @@ describe('Mutation', () => {
     const baseDoc = doc('collection/key', 0, { foo: 'bar' });
 
     const mutation = deleteMutation('collection/key');
-    const result = applyMutationToLocalView(
-      mutation,
-      baseDoc,
-      baseDoc,
-      Timestamp.now()
-    );
+    const result = applyMutationToLocalView(mutation, baseDoc, Timestamp.now());
     expect(result).to.deep.equal(deletedDoc('collection/key', 0));
   });
 
@@ -722,13 +685,11 @@ describe('Mutation', () => {
     let mutatedDoc = applyMutationToLocalView(
       transform,
       baseDoc,
-      baseDoc,
       Timestamp.now()
     );
     mutatedDoc = applyMutationToLocalView(
       transform,
       mutatedDoc,
-      baseDoc,
       Timestamp.now()
     );