Ensure consistent JSON Serialization of DocumentKeys (#1596)

Stop relying on JSON.stringify() as part of generating canonicalId() since we haven't made any effort to implement sane toJSON() methods on our types.  We now use toString() instead, which required implementing it on SortedMap.

Author: schmidt-sebastian

packages/firestore/CHANGELOG.md

@@ -8,6 +8,9 @@
   While this feature is not yet available, all schema changes are included
   in this release. Once you upgrade, you will not be able to use an older version
   of the Firestore SDK with persistence enabled.
+- [fixed] Fixed an issue with IndexedDb persistence that triggered an internal
+  assert for Queries that use nested DocumentReferences in where() clauses
+  (#1524, #1596).
 
 # 1.0.4
 - [fixed] Fixed an uncaught promise error occurring when `enablePersistence()`

packages/firestore/src/core/sync_engine.ts

@@ -204,7 +204,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
     const queryView = this.queryViewsByQuery.get(query);
     if (queryView) {
-      // PORTING NOTE: With Mult-Tab Web, it is possible that a query view
+      // PORTING NOTE: With Multi-Tab Web, it is possible that a query view
       // already exists when EventManager calls us for the first time. This
       // happens when the primary tab is already listening to this query on
       // behalf of another tab and the user of the primary also starts listening

packages/firestore/src/model/field_value.ts

@@ -614,7 +614,7 @@ export class ObjectValue extends FieldValue {
   }
 
   toString(): string {
-    return JSON.stringify(this.value());
+    return this.internalValue.toString();
   }
 
   private child(childName: string): FieldValue | undefined {
@@ -688,6 +688,7 @@ export class ArrayValue extends FieldValue {
   }
 
   toString(): string {
-    return JSON.stringify(this.value());
+    const descriptions = this.internalValue.map(v => v.toString());
+    return `[${descriptions.join(',')}]`;
   }
 }

packages/firestore/src/model/path.ts

@@ -23,7 +23,7 @@ export const DOCUMENT_KEY_NAME = '__name__';
 /**
  * Path represents an ordered sequence of string segments.
  */
-export abstract class Path {
+abstract class Path {
   private segments: string[];
   private offset: number;
   private len: number;
@@ -32,6 +32,14 @@ export abstract class Path {
     this.init(segments, offset, length);
   }
 
+  /**
+   * Returns a String representation.
+   *
+   * Implementing classes are required to provide deterministic implementations as
+   * the String representation is used to obtain canonical Query IDs.
+   */
+  abstract toString(): string;
+
   /**
    * An initialization method that can be called from outside the constructor.
    * We need this so that we can have a non-static construct method that returns

packages/firestore/src/util/sorted_map.ts

@@ -143,6 +143,15 @@ export class SortedMap<K, V> {
     });
   }
 
+  toString(): string {
+    const descriptions: string[] = [];
+    this.inorderTraversal((k, v) => {
+      descriptions.push(`${k}:${v}`);
+      return false;
+    });
+    return `{${descriptions.join(', ')}}`;
+  }
+
   // Traverses the map in reverse key order and calls the specified action
   // function for each key/value pair. If action returns true, traversal is
   // aborted.

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

@@ -32,8 +32,10 @@ import {
   withTestDb
 } from '../util/helpers';
 
-const Timestamp = firebase.firestore!.Timestamp;
+const Blob = firebase.firestore!.Blob;
 const FieldPath = firebase.firestore!.FieldPath;
+const GeoPoint = firebase.firestore!.GeoPoint;
+const Timestamp = firebase.firestore!.Timestamp;
 
 // TODO(b/116617988): Use public API.
 interface FirestoreInternal extends firestore.FirebaseFirestore {
@@ -694,4 +696,41 @@ apiDescribe('Queries', persistence => {
       expect(querySnapshot.docs.map(d => d.id)).to.deep.equal(['cg-doc2']);
     });
   });
+
+  it('can query custom types', () => {
+    return withTestCollection(persistence, {}, async ref => {
+      const data = {
+        ref: ref.firestore.doc('f/c'),
+        geoPoint: new GeoPoint(0, 0),
+        buffer: Blob.fromBase64String('Zm9v'),
+        time: Timestamp.now(),
+        array: [
+          ref.firestore.doc('f/c'),
+          new GeoPoint(0, 0),
+          Blob.fromBase64String('Zm9v'),
+          Timestamp.now()
+        ]
+      };
+      await ref.add({ data });
+
+      // In https://github.com/firebase/firebase-js-sdk/issues/1524, a
+      // customer was not able to unlisten from a query that contained a
+      // nested object with a DocumentReference. The cause of it was that our
+      // serialization of nested references via JSON.stringify() was different
+      // for Queries created via the API layer versus Queries read from
+      // persistence. To simulate this issue, we have to listen and unlisten
+      // to the same query twice.
+      const query = ref.where('data', '==', data);
+
+      for (let i = 0; i < 2; ++i) {
+        const deferred = new Deferred();
+        const unsubscribe = query.onSnapshot(snapshot => {
+          expect(snapshot.size).to.equal(1);
+          deferred.resolve();
+        });
+        await deferred.promise;
+        unsubscribe();
+      }
+    });
+  });
 });

packages/firestore/test/unit/core/query.test.ts

@@ -383,6 +383,27 @@ describe('Query', () => {
     // .addOrderBy(orderBy(DOCUMENT_KEY_NAME, 'desc'))
     // .withUpperBound(lip3, 'exclusive');
 
+    const relativeReference = ref('project1/database1', 'col/doc');
+    const absoluteReference = ref(
+      'project1/database1',
+      'projects/project1/databases/database1/documents/col/doc',
+      5
+    );
+
+    const q16a = Query.atPath(path('foo')).addFilter(
+      filter('object', '==', { ref: relativeReference })
+    );
+    const q16b = Query.atPath(path('foo')).addFilter(
+      filter('object', '==', { ref: absoluteReference })
+    );
+
+    const q17a = Query.atPath(path('foo')).addFilter(
+      filter('array', '==', [relativeReference])
+    );
+    const q17b = Query.atPath(path('foo')).addFilter(
+      filter('array', '==', [absoluteReference])
+    );
+
     const queries = [
       [q1a, q1b],
       [q2a, q2b],
@@ -396,9 +417,11 @@ describe('Query', () => {
       [q10a],
       [q11a],
       [q12a],
-      [q13a]
+      [q13a],
       //[q14a],
       //[q15a],
+      [q16a, q16b],
+      [q17a, q17b]
     ];
 
     expectEqualitySets(queries, (q1, q2) => {

packages/firestore/test/util/helpers.ts

@@ -105,10 +105,14 @@ export function version(v: TestSnapshotVersion): SnapshotVersion {
   return SnapshotVersion.fromMicroseconds(v);
 }
 
-export function ref(dbIdStr: string, keyStr: string): DocumentKeyReference {
+export function ref(
+  dbIdStr: string,
+  keyStr: string,
+  offset?: number
+): DocumentKeyReference {
   const [project, database] = dbIdStr.split('/', 2);
   const dbId = new DatabaseId(project, database);
-  return new DocumentKeyReference(dbId, key(keyStr));
+  return new DocumentKeyReference(dbId, new DocumentKey(path(keyStr, offset)));
 }
 
 export function doc(