Don't normalize Proto data during deepClone (#5147)

schmidt-sebastian · web-flow · commit 2cd9d7c394dd · 2021-07-15T10:11:43.000-06:00
diff --git a/.changeset/thirty-suits-switch.md b/.changeset/thirty-suits-switch.md
@@ -0,0 +1,6 @@
+---
+"firebase": patch
+"@firebase/firestore": patch
+---
+
+Fixed an issue that prevented Timestamps from being used via `update()` when connected to the Emulator
diff --git a/packages/firestore/src/exp/database.ts b/packages/firestore/src/exp/database.ts
@@ -15,12 +15,12 @@
  * limitations under the License.
  */
 
+// eslint-disable-next-line import/no-extraneous-dependencies
 import {
   _getProvider,
   _removeServiceInstance,
   FirebaseApp,
   getApp
-  // eslint-disable-next-line import/no-extraneous-dependencies
 } from '@firebase/app-exp';
 import { FirebaseAuthInternalName } from '@firebase/auth-interop-types';
 import { Provider } from '@firebase/component';
diff --git a/packages/firestore/src/lite/database.ts b/packages/firestore/src/lite/database.ts
@@ -15,12 +15,12 @@
  * limitations under the License.
  */
 
+// eslint-disable-next-line import/no-extraneous-dependencies
 import {
   _getProvider,
   _removeServiceInstance,
   FirebaseApp,
   getApp
-  // eslint-disable-next-line import/no-extraneous-dependencies
 } from '@firebase/app-exp';
 import { FirebaseAuthInternalName } from '@firebase/auth-interop-types';
 import { Provider } from '@firebase/component';
diff --git a/packages/firestore/src/model/values.ts b/packages/firestore/src/model/values.ts
@@ -559,8 +559,11 @@ export function isMapValue(
 export function deepClone(source: Value): Value {
   if (source.geoPointValue) {
     return { geoPointValue: { ...source.geoPointValue } };
-  } else if (source.timestampValue) {
-    return { timestampValue: { ...normalizeTimestamp(source.timestampValue) } };
+  } else if (
+    source.timestampValue &&
+    typeof source.timestampValue === 'object'
+  ) {
+    return { timestampValue: { ...source.timestampValue } };
   } else if (source.mapValue) {
     const target: Value = { mapValue: { fields: {} } };
     forEach(
diff --git a/packages/firestore/test/integration/api/type.test.ts b/packages/firestore/test/integration/api/type.test.ts
@@ -33,49 +33,59 @@ apiDescribe('Firestore', (persistence: boolean) => {
   async function expectRoundtrip(
     db: firestore.FirebaseFirestore,
     data: {},
-    validateSnapshots = true
-  ): Promise<void> {
+    validateSnapshots = true,
+    expectedData?: {}
+  ): Promise<firestore.DocumentSnapshot> {
+    expectedData = expectedData ?? data;
+
     const collection = db.collection(db.collection('a').doc().id);
     const doc = collection.doc();
-    await doc.set(data);
 
+    await doc.set(data);
     let docSnapshot = await doc.get();
-    expect(docSnapshot.data()).to.deep.equal(data);
+    expect(docSnapshot.data()).to.deep.equal(expectedData);
+
+    await doc.update(data);
+    docSnapshot = await doc.get();
+    expect(docSnapshot.data()).to.deep.equal(expectedData);
 
     // Validate that the transaction API returns the same types
     await db.runTransaction(async transaction => {
       docSnapshot = await transaction.get(doc);
-      expect(docSnapshot.data()).to.deep.equal(data);
+      expect(docSnapshot.data()).to.deep.equal(expectedData);
     });
 
     if (validateSnapshots) {
       let querySnapshot = await collection.get();
       docSnapshot = querySnapshot.docs[0];
-      expect(docSnapshot.data()).to.deep.equal(data);
+      expect(docSnapshot.data()).to.deep.equal(expectedData);
 
-      const eventsAccumulator = new EventsAccumulator<firestore.QuerySnapshot>();
+      const eventsAccumulator =
+        new EventsAccumulator<firestore.QuerySnapshot>();
       const unlisten = collection.onSnapshot(eventsAccumulator.storeEvent);
       querySnapshot = await eventsAccumulator.awaitEvent();
       docSnapshot = querySnapshot.docs[0];
-      expect(docSnapshot.data()).to.deep.equal(data);
+      expect(docSnapshot.data()).to.deep.equal(expectedData);
 
       unlisten();
     }
+
+    return docSnapshot;
   }
 
   it('can read and write null fields', () => {
-    return withTestDb(persistence, db => {
-      return expectRoundtrip(db, { a: 1, b: null });
+    return withTestDb(persistence, async db => {
+      await expectRoundtrip(db, { a: 1, b: null });
     });
   });
 
   it('can read and write number fields', () => {
-    return withTestDb(persistence, db => {
+    return withTestDb(persistence, async db => {
       // TODO(b/174486484): If we build ViewSnapshots from IndexedDb, this test
       // fails since we first store the backend proto in IndexedDb, which turns
       // -0.0 into 0.0.
       const validateSnapshots = !persistence;
-      return expectRoundtrip(
+      await expectRoundtrip(
         db,
         { a: 1, b: NaN, c: Infinity, d: -0.0 },
         validateSnapshots
@@ -84,91 +94,78 @@ apiDescribe('Firestore', (persistence: boolean) => {
   });
 
   it('can read and write array fields', () => {
-    return withTestDb(persistence, db => {
-      return expectRoundtrip(db, { array: [1, 'foo', { deep: true }, null] });
+    return withTestDb(persistence, async db => {
+      await expectRoundtrip(db, { array: [1, 'foo', { deep: true }, null] });
     });
   });
 
   it('can read and write geo point fields', () => {
-    return withTestDoc(persistence, doc => {
-      return doc
-        .set({
-          geopoint1: new GeoPoint(1.23, 4.56),
-          geopoint2: new GeoPoint(0, 0)
-        })
-        .then(() => {
-          return doc.get();
-        })
-        .then(docSnapshot => {
-          const latLong = docSnapshot.data()!['geopoint1'];
-          expect(latLong instanceof GeoPoint).to.equal(true);
-          expect(latLong.latitude).to.equal(1.23);
-          expect(latLong.longitude).to.equal(4.56);
-
-          const zeroLatLong = docSnapshot.data()!['geopoint2'];
-          expect(zeroLatLong instanceof GeoPoint).to.equal(true);
-          expect(zeroLatLong.latitude).to.equal(0);
-          expect(zeroLatLong.longitude).to.equal(0);
-        });
+    return withTestDb(persistence, async db => {
+      const docSnapshot = await expectRoundtrip(db, {
+        geopoint1: new GeoPoint(1.23, 4.56),
+        geopoint2: new GeoPoint(0, 0)
+      });
+
+      const latLong = docSnapshot.data()!['geopoint1'];
+      expect(latLong instanceof GeoPoint).to.equal(true);
+      expect(latLong.latitude).to.equal(1.23);
+      expect(latLong.longitude).to.equal(4.56);
+
+      const zeroLatLong = docSnapshot.data()!['geopoint2'];
+      expect(zeroLatLong instanceof GeoPoint).to.equal(true);
+      expect(zeroLatLong.latitude).to.equal(0);
+      expect(zeroLatLong.longitude).to.equal(0);
     });
   });
 
   it('can read and write bytes fields', () => {
-    return withTestDoc(persistence, doc => {
-      return doc
-        .set({
-          bytes: Blob.fromUint8Array(new Uint8Array([0, 1, 255]))
-        })
-        .then(() => {
-          return doc.get();
-        })
-        .then(docSnapshot => {
-          const blob = docSnapshot.data()!['bytes'];
-          // TODO(firestorexp): As part of the Compat migration, the SDK
-          // should re-wrap the firestore-exp types into the Compat API.
-          // Comment this change back in once this is complete (note that this
-          // check passes in the legacy API).
-          // expect(blob instanceof Blob).to.equal(true);
-          expect(blob.toUint8Array()).to.deep.equal(
-            new Uint8Array([0, 1, 255])
-          );
-        });
+    return withTestDb(persistence, async db => {
+      const docSnapshot = await expectRoundtrip(db, {
+        bytes: Blob.fromUint8Array(new Uint8Array([0, 1, 255]))
+      });
+
+      const blob = docSnapshot.data()!['bytes'];
+      // TODO(firestorexp): As part of the Compat migration, the SDK
+      // should re-wrap the firestore-exp types into the Compat API.
+      // Comment this change back in once this is complete (note that this
+      // check passes in the legacy API).
+      // expect(blob instanceof Blob).to.equal(true);
+      expect(blob.toUint8Array()).to.deep.equal(new Uint8Array([0, 1, 255]));
     });
   });
 
   it('can read and write date fields', () => {
-    return withTestDb(persistence, db => {
-      const dateValue = new Date('2017-04-10T09:10:11.123Z');
-      // Dates are returned as Timestamps, so expectRoundtrip can't be used
-      // here.
-      const doc = db.collection('rooms').doc();
-      return doc
-        .set({ date: dateValue })
-        .then(() => doc.get())
-        .then(snapshot => {
-          expect(snapshot.data()).to.deep.equal({
-            date: Timestamp.fromDate(dateValue)
-          });
-        });
+    return withTestDb(persistence, async db => {
+      const date = new Date('2017-04-10T09:10:11.123Z');
+      // Dates are returned as Timestamps
+      const data = { date };
+      const expectedData = { date: Timestamp.fromDate(date) };
+
+      await expectRoundtrip(
+        db,
+        data,
+        /* validateSnapshot= */ true,
+        expectedData
+      );
     });
   });
 
   it('can read and write timestamp fields', () => {
-    return withTestDb(persistence, db => {
+    return withTestDb(persistence, async db => {
       const timestampValue = Timestamp.now();
-      return expectRoundtrip(db, { timestamp: timestampValue });
+      await expectRoundtrip(db, { timestamp: timestampValue });
     });
   });
 
   it('can read and write document references', () => {
-    return withTestDoc(persistence, doc => {
-      return expectRoundtrip(doc.firestore, { a: 42, ref: doc });
+    return withTestDoc(persistence, async doc => {
+      await expectRoundtrip(doc.firestore, { a: 42, ref: doc });
     });
   });
 
   it('can read and write document references in an array', () => {
-    return withTestDoc(persistence, doc => {
-      return expectRoundtrip(doc.firestore, { a: 42, refs: [doc] });
+    return withTestDoc(persistence, async doc => {
+      await expectRoundtrip(doc.firestore, { a: 42, refs: [doc] });
     });
   });
 });
diff --git a/packages/firestore/test/unit/model/values.test.ts b/packages/firestore/test/unit/model/values.test.ts
@@ -25,7 +25,8 @@ import {
   valueCompare,
   valueEquals,
   estimateByteSize,
-  refValue
+  refValue,
+  deepClone
 } from '../../../src/model/values';
 import * as api from '../../../src/protos/firestore_proto_api';
 import { primitiveComparator } from '../../../src/util/misc';
@@ -405,4 +406,41 @@ describe('Values', () => {
       )
     ).to.equal('{a:1,b:2,c:3}');
   });
+
+  it('clones properties without normalization', () => {
+    const values = [
+      { integerValue: '1' },
+      { integerValue: 1 },
+      { doubleValue: '2' },
+      { doubleValue: 2 },
+      { timestampValue: '2007-04-05T14:30:01Z' },
+      { timestampValue: { seconds: 1175783401 } },
+      { timestampValue: '2007-04-05T14:30:01.999Z' },
+      {
+        timestampValue: { seconds: 1175783401, nanos: 999000000 }
+      },
+      { timestampValue: '2007-04-05T14:30:02Z' },
+      { timestampValue: { seconds: 1175783402 } },
+      { timestampValue: '2007-04-05T14:30:02.100Z' },
+      {
+        timestampValue: { seconds: 1175783402, nanos: 100000000 }
+      },
+      { timestampValue: '2007-04-05T14:30:02.100001Z' },
+      {
+        timestampValue: { seconds: 1175783402, nanos: 100001000 }
+      },
+      { bytesValue: new Uint8Array([0, 1, 2]) },
+      { bytesValue: 'AAEC' },
+      { bytesValue: new Uint8Array([0, 1, 3]) },
+      { bytesValue: 'AAED' }
+    ];
+
+    for (const value of values) {
+      expect(deepClone(value)).to.deep.equal(value);
+      const mapValue = { mapValue: { fields: { foo: value } } };
+      expect(deepClone(mapValue)).to.deep.equal(mapValue);
+      const arrayValue = { arrayValue: { values: [value] } };
+      expect(deepClone(arrayValue)).to.deep.equal(arrayValue);
+    }
+  });
 });
diff --git a/packages/firestore/test/unit/remote/rest_connection.test.ts b/packages/firestore/test/unit/remote/rest_connection.test.ts
@@ -47,7 +47,7 @@ export class TestRestConnection extends RestConnection {
     body: Req
   ): Promise<Resp> {
     this.lastUrl = url;
-    this.lastRequestBody = (body as unknown) as Indexable;
+    this.lastRequestBody = body as unknown as Indexable;
     this.lastHeaders = headers;
     const response = this.nextResponse;
     this.nextResponse = Promise.resolve<unknown>({});
@@ -63,7 +63,8 @@ describe('RestConnection', () => {
     'example.com',
     /*ssl=*/ false,
     /*forceLongPolling=*/ false,
-    /*autoDetectLongPolling=*/ false
+    /*autoDetectLongPolling=*/ false,
+    /*useFetchStreams=*/ false
   );
   const connection = new TestRestConnection(testDatabaseInfo);
 
diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts
@@ -256,7 +256,8 @@ abstract class TestRunner {
       'host',
       /*ssl=*/ false,
       /*forceLongPolling=*/ false,
-      /*autoDetectLongPolling=*/ false
+      /*autoDetectLongPolling=*/ false,
+      /*useFetchStreams=*/ false
     );
 
     // TODO(mrschmidt): During client startup in `firestore_client`, we block
@@ -297,11 +298,12 @@ abstract class TestRunner {
     const onlineComponentProvider = new MockOnlineComponentProvider(
       this.connection
     );
-    const offlineComponentProvider = await this.initializeOfflineComponentProvider(
-      onlineComponentProvider,
-      configuration,
-      this.useGarbageCollection
-    );
+    const offlineComponentProvider =
+      await this.initializeOfflineComponentProvider(
+        onlineComponentProvider,
+        configuration,
+        this.useGarbageCollection
+      );
     await onlineComponentProvider.initialize(
       offlineComponentProvider,
       configuration
@@ -912,9 +914,8 @@ abstract class TestRunner {
         this.expectedActiveLimboDocs = expectedState.activeLimboDocs!.map(key);
       }
       if ('enqueuedLimboDocs' in expectedState) {
-        this.expectedEnqueuedLimboDocs = expectedState.enqueuedLimboDocs!.map(
-          key
-        );
+        this.expectedEnqueuedLimboDocs =
+          expectedState.enqueuedLimboDocs!.map(key);
       }
       if ('activeTargets' in expectedState) {
         this.expectedActiveTargets.clear();
