Merge 8321655 into e13b794

Author: Brian Chen

packages/firestore/src/model/mutation.ts

@@ -87,11 +87,11 @@ export class MutationResult {
      */
     readonly version: SnapshotVersion,
     /**
-     * The resulting fields returned from the backend after a
-     * TransformMutation has been committed. Contains one FieldValue for each
-     * FieldTransform that was in the mutation.
+     * The resulting fields returned from the backend after a mutation
+     * containing field transforms has been committed. Contains one FieldValue
+     * for each FieldTransform that was in the mutation.
      *
-     * Will be null if the mutation was not a TransformMutation.
+     * Will be null if the mutation did not contain any field transforms.
      */
     readonly transformResults: Array<ProtoValue | null> | null
   ) {}
@@ -178,8 +178,8 @@ export function preconditionIsValidForDocument(
  * Mutations not only act on the value of the document but also its version.
  *
  * For local mutations (mutations that haven't been committed yet), we preserve
- * the existing version for Set, Patch, and Transform mutations. For Delete
- * mutations, we reset the version to 0.
+ * the existing version for Set and Patch mutations. For Delete mutations, we
+ * reset the version to 0.
  *
  * Here's the expected transition table.
  *
@@ -191,28 +191,22 @@ export function preconditionIsValidForDocument(
  * PatchMutation      Document(v3)          Document(v3)
  * PatchMutation      NoDocument(v3)        NoDocument(v3)
  * PatchMutation      null                  null
- * TransformMutation  Document(v3)          Document(v3)
- * TransformMutation  NoDocument(v3)        NoDocument(v3)
- * TransformMutation  null                  null
  * DeleteMutation     Document(v3)          NoDocument(v0)
  * DeleteMutation     NoDocument(v3)        NoDocument(v0)
  * DeleteMutation     null                  NoDocument(v0)
  *
  * For acknowledged mutations, we use the updateTime of the WriteResponse as
- * the resulting version for Set, Patch, and Transform mutations. As deletes
- * have no explicit update time, we use the commitTime of the WriteResponse for
+ * the resulting version for Set and Patch mutations. As deletes have no
+ * explicit update time, we use the commitTime of the WriteResponse for
  * Delete mutations.
  *
  * If a mutation is acknowledged by the backend but fails the precondition check
  * locally, we return an `UnknownDocument` and rely on Watch to send us the
  * updated version.
  *
- * Note that TransformMutations don't create Documents (in the case of being
- * applied to a NoDocument), even though they would on the backend. This is
- * because the client always combines the TransformMutation with a SetMutation
- * or PatchMutation and we only want to apply the transform if the prior
- * mutation resulted in a Document (always true for a SetMutation, but not
- * necessarily for a PatchMutation).
+ * Field transforms are used only with Patch and Set Mutations. We use the
+ * `updateTransforms` message to store transforms, rather than the `transforms`s
+ * messages.
  *
  * ## Subclassing Notes
  *
@@ -335,13 +329,7 @@ export function extractMutationBaseValue(
   mutation: Mutation,
   maybeDoc: MaybeDocument | null
 ): ObjectValue | null {
-  if (mutation.fieldTransforms !== undefined) {
-    return extractTransformMutationBaseValue(
-      mutation.fieldTransforms,
-      maybeDoc
-    );
-  }
-  return null;
+  return extractTransformMutationBaseValue(mutation.fieldTransforms, maybeDoc);
 }
 
 function extractTransformMutationBaseValue(
@@ -489,19 +477,17 @@ function applySetMutationToLocalView(
   }
 
   let newData = mutation.value;
-  if (mutation.fieldTransforms) {
-    const transformResults = localTransformResults(
-      mutation.fieldTransforms,
-      localWriteTime,
-      maybeDoc,
-      baseDoc
-    );
-    newData = transformObject(
-      mutation.fieldTransforms,
-      newData,
-      transformResults
-    );
-  }
+  const transformResults = localTransformResults(
+    mutation.fieldTransforms,
+    localWriteTime,
+    maybeDoc,
+    baseDoc
+  );
+  newData = transformObject(
+    mutation.fieldTransforms,
+    newData,
+    transformResults
+  );
 
   const version = getPostMutationVersion(maybeDoc);
   return new Document(mutation.key, version, newData, {
@@ -623,8 +609,8 @@ function patchObject(mutation: PatchMutation, data: ObjectValue): ObjectValue {
 
 /**
  * Creates a list of "transform results" (a transform result is a field value
- * representing the result of applying a transform) for use after a
- * TransformMutation has been acknowledged by the server.
+ * representing the result of applying a transform) for use after a mutation
+ * 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.
@@ -664,10 +650,10 @@ function serverTransformResults(
 /**
  * Creates a list of "transform results" (a transform result is a field value
  * representing the result of applying a transform) for use when applying a
- * TransformMutation locally.
+ * transform locally.
  *
  * @param fieldTransforms - The field transforms to apply the result to.
- * @param localWriteTime - The local time of the transform mutation (used to
+ * @param localWriteTime - The local time of the mutation (used to
  *     generate ServerTimestampValues).
  * @param maybeDoc - The current state of the document after applying all
  *     previous mutations.
@@ -689,14 +675,6 @@ function localTransformResults(
       previousValue = maybeDoc.field(fieldTransform.field);
     }
 
-    if (previousValue === null && baseDoc instanceof Document) {
-      // If the current document does not contain a value for the mutated
-      // field, use the value that existed before applying this mutation
-      // batch. This solves an edge case where a PatchMutation clears the
-      // values in a nested map before the TransformMutation is applied.
-      previousValue = baseDoc.field(fieldTransform.field);
-    }
-
     transformResults.push(
       applyTransformOperationToLocalView(
         transform,

packages/firestore/src/model/server_timestamps.ts

@@ -33,9 +33,8 @@ import { normalizeTimestamp } from './normalize';
  *
  * Notes:
  * - ServerTimestampValue instances are created as the result of applying a
- *   TransformMutation (see TransformMutation.applyTo()). They can only exist in
- *   the local view of a document. Therefore they do not need to be parsed or
- *   serialized.
+ *   transform. They can only exist in the local view of a document. Therefore
+ *   they do not need to be parsed or serialized.
  * - When evaluated locally (e.g. for snapshot.data()), they by default
  *   evaluate to `null`. This behavior can be configured by passing custom
  *   FieldValueOptions to value().

packages/firestore/src/model/transform_operation.ts

@@ -25,7 +25,7 @@ import { normalizeNumber } from './normalize';
 import { serverTimestamp } from './server_timestamps';
 import { isArray, isInteger, isNumber, valueEquals } from './values';
 
-/** Represents a transform within a TransformMutation. */
+/** Used to represent a field transform on a mutation. */
 export class TransformOperation {
   // Make sure that the structural type of `TransformOperation` is unique.
   // See https://github.com/microsoft/TypeScript/issues/5451

packages/firestore/test/integration/api/server_timestamp.test.ts

@@ -104,25 +104,6 @@ apiDescribe('Server Timestamps', (persistence: boolean) => {
     );
   }
 
-  /**
-   * Verifies a snapshot containing setData but using the previous field value
-   * for the timestamps.
-   */
-  function verifyTimestampsUsePreviousValue(
-    current: firestore.DocumentSnapshot,
-    prev: firestore.DocumentSnapshot | null
-  ): void {
-    if (!prev) {
-      verifyTimestampsAreNull(current);
-    } else {
-      expect(current.exists).to.equal(true);
-      const when = prev.get('when');
-      expect(current.data({ serverTimestamps: 'previous' })).to.deep.equal(
-        expectedDataWithTimestamp(when)
-      );
-    }
-  }
-
   /**
    * Wraps a test, getting a docRef and event accumulator, and cleaning them
    * up when done.
@@ -207,34 +188,6 @@ apiDescribe('Server Timestamps', (persistence: boolean) => {
     });
   });
 
-  it('can return previous value', () => {
-    // The following test includes an update of the nested map "deep", which
-    // updates it to contain a single ServerTimestamp. This update is split
-    // into two mutations: One that sets "deep" to an empty map and overwrites
-    // the previous ServerTimestamp value and a second transform that writes
-    // the new ServerTimestamp. This step in the test verifies that we can
-    // still access the old ServerTimestamp value (from `previousSnapshot`) even
-    // though it was removed in an intermediate step.
-
-    let previousSnapshot: firestore.DocumentSnapshot;
-
-    return withTestSetup(() => {
-      return writeInitialData()
-        .then(() => docRef.update(updateData))
-        .then(() => accumulator.awaitLocalEvent())
-        .then(snapshot => verifyTimestampsUsePreviousValue(snapshot, null))
-        .then(() => accumulator.awaitRemoteEvent())
-        .then(snapshot => {
-          previousSnapshot = snapshot;
-        })
-        .then(() => docRef.update(updateData))
-        .then(() => accumulator.awaitLocalEvent())
-        .then(snapshot =>
-          verifyTimestampsUsePreviousValue(snapshot, previousSnapshot)
-        );
-    });
-  });
-
   it('can return previous value of different type', () => {
     return withTestSetup(() => {
       return writeInitialData()