Merge bf9b50c into 068edfa

Author: schmidt-sebastian

.changeset/calm-shrimps-press.md

@@ -0,0 +1,5 @@
+---
+"@firebase/firestore": patch
+---
+
+Fixes an issue that returned invalid `DocumentReference`s in `QuerySnapshot`s.

packages/firestore/src/api/database.ts

@@ -967,8 +967,11 @@ export class QueryDocumentSnapshot<T = PublicDocumentData>
 export class Query<T = PublicDocumentData>
   extends Compat<ExpQuery<T>>
   implements PublicQuery<T> {
+  private readonly _userDataWriter: UserDataWriter;
+
   constructor(readonly firestore: Firestore, delegate: ExpQuery<T>) {
     super(delegate);
+    this._userDataWriter = new UserDataWriter(firestore);
   }
 
   where(
@@ -1076,7 +1079,18 @@ export class Query<T = PublicDocumentData>
     } else {
       query = getDocs(this._delegate);
     }
-    return query.then(result => new QuerySnapshot(this.firestore, result));
+    return query.then(
+      result =>
+        new QuerySnapshot(
+          this.firestore,
+          new ExpQuerySnapshot<T>(
+            this.firestore._delegate,
+            this._userDataWriter,
+            this._delegate,
+            result._snapshot
+          )
+        )
+    );
   }
 
   onSnapshot(observer: PartialObserver<PublicQuerySnapshot<T>>): Unsubscribe;
@@ -1100,7 +1114,16 @@ export class Query<T = PublicDocumentData>
     const options = extractSnapshotOptions(args);
     const observer = wrapObserver<QuerySnapshot<T>, ExpQuerySnapshot<T>>(
       args,
-      snap => new QuerySnapshot(this.firestore, snap)
+      snap =>
+        new QuerySnapshot(
+          this.firestore,
+          new ExpQuerySnapshot<T>(
+            this.firestore._delegate,
+            this._userDataWriter,
+            this._delegate,
+            snap._snapshot
+          )
+        )
     );
     return onSnapshot(this._delegate, options, observer);
   }

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

@@ -20,6 +20,7 @@ import { expect } from 'chai';
 import { addEqualityMatcher } from '../../util/equality_matcher';
 import * as firebaseExport from '../util/firebase_export';
 import { apiDescribe, withTestDb, withTestDoc } from '../util/helpers';
+import { EventsAccumulator } from '../util/events_accumulator';
 
 const Blob = firebaseExport.Blob;
 const GeoPoint = firebaseExport.GeoPoint;
@@ -28,17 +29,33 @@ const Timestamp = firebaseExport.Timestamp;
 apiDescribe('Firestore', (persistence: boolean) => {
   addEqualityMatcher();
 
-  function expectRoundtrip(
+  async function expectRoundtrip(
     db: firestore.FirebaseFirestore,
-    data: {}
+    data: {},
+    validateSnapshots = true
   ): Promise<void> {
-    const doc = db.collection('rooms').doc();
-    return doc
-      .set(data)
-      .then(() => doc.get())
-      .then(snapshot => {
-        expect(snapshot.data()).to.deep.equal(data);
-      });
+    const collection = db.collection(db.collection('a').doc().id);
+    const doc = collection.doc();
+    await doc.set(data);
+
+    let docSnapshot = await doc.get();
+    expect(docSnapshot.data()).to.deep.equal(data);
+
+    if (validateSnapshots) {
+      let querySnapshot = await collection.get();
+      docSnapshot = querySnapshot.docs[0];
+      expect(docSnapshot.data()).to.deep.equal(data);
+
+      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);
+
+      unlisten();
+    }
   }
 
   it('can read and write null fields', () => {
@@ -49,7 +66,15 @@ apiDescribe('Firestore', (persistence: boolean) => {
 
   it('can read and write number fields', () => {
     return withTestDb(persistence, db => {
-      return expectRoundtrip(db, { a: 1, b: NaN, c: Infinity, d: -0.0 });
+      // 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(
+        db,
+        { a: 1, b: NaN, c: Infinity, d: -0.0 },
+        validateSnapshots
+      );
     });
   });
 

packages/firestore/test/util/equality_matcher.ts

@@ -57,6 +57,20 @@ function customDeepEqual(
       return customMatcher.equalsFn(left, right);
     }
   }
+  if (left && typeof left === 'object' && right && typeof right === 'object') {
+    // The `isEqual` check below returns true if firestore-exp types are
+    // compared with API types from Firestore classic. We do want to
+    // differentiate between these types in our tests to ensure that the we do
+    // not return firestore-exp types in the classic SDK.
+    const leftObj = left as Record<string, unknown>;
+    const rightObj = right as Record<string, unknown>;
+    if (
+      leftObj.constructor.name === rightObj.constructor.name &&
+      leftObj.constructor !== rightObj.constructor
+    ) {
+      return false;
+    }
+  }
   if (typeof left === 'object' && left && 'isEqual' in left) {
     return (left as Equatable<unknown>).isEqual(right);
   }