Fix the ordering of the index-based lookup in getAll(keys) (#6128)

Author: schmidt-sebastian

.changeset/nasty-hairs-knock.md

@@ -0,0 +1,5 @@
+---
+"@firebase/firestore": patch
+---
+
+Fixes an issue during multi-document lookup that resulted in the IndexedDB error "The parameter is less than or equal to this cursor's".

packages/firestore/src/local/indexeddb_remote_document_cache.ts

@@ -611,12 +611,36 @@ function dbCollectionGroupKey(
     /* document id */ path.length > 0 ? path[path.length - 1] : ''
   ];
 }
+
 /**
  * Comparator that compares document keys according to the primary key sorting
- * used by the `DbRemoteDocumentDocument` store (by collection path and then
- * document ID).
+ * used by the `DbRemoteDocumentDocument` store (by prefix path, collection id
+ * and then document ID).
+ *
+ * Visible for testing.
  */
-function dbKeyComparator(l: DocumentKey, r: DocumentKey): number {
-  const cmp = l.path.length - r.path.length;
-  return cmp !== 0 ? cmp : DocumentKey.comparator(l, r);
+export function dbKeyComparator(l: DocumentKey, r: DocumentKey): number {
+  const left = l.path.toArray();
+  const right = r.path.toArray();
+
+  // The ordering is based on https://chromium.googlesource.com/chromium/blink/+/fe5c21fef94dae71c1c3344775b8d8a7f7e6d9ec/Source/modules/indexeddb/IDBKey.cpp#74
+  let cmp = 0;
+  for (let i = 0; i < left.length - 2 && i < right.length - 2; ++i) {
+    cmp = primitiveComparator(left[i], right[i]);
+    if (cmp) {
+      return cmp;
+    }
+  }
+
+  cmp = primitiveComparator(left.length, right.length);
+  if (cmp) {
+    return cmp;
+  }
+
+  cmp = primitiveComparator(left[left.length - 2], right[right.length - 2]);
+  if (cmp) {
+    return cmp;
+  }
+
+  return primitiveComparator(left[left.length - 1], right[right.length - 1]);
 }

packages/firestore/src/local/simple_db.ts

@@ -656,6 +656,8 @@ export class SimpleDbStore<
   loadAll(): PersistencePromise<ValueType[]>;
   /** Loads all elements for the index range from the object store. */
   loadAll(range: IDBKeyRange): PersistencePromise<ValueType[]>;
+  /** Loads all elements ordered by the given index. */
+  loadAll(index: string): PersistencePromise<ValueType[]>;
   /**
    * Loads all elements from the object store that fall into the provided in the
    * index range for the given index.
@@ -845,9 +847,7 @@ export class SimpleDbStore<
           cursor.continue(controller.skipToKey);
         }
       };
-    }).next(() => {
-      return PersistencePromise.waitFor(results);
-    });
+    }).next(() => PersistencePromise.waitFor(results));
   }
 
   private options(

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

@@ -406,6 +406,19 @@ function genericRemoteDocumentCacheTests(
       });
   });
 
+  it('can set and read several documents with deeply nested keys', () => {
+    // This test verifies that the sorting works correctly in IndexedDB,
+    // which sorts by prefix path first.
+    // Repro of https://github.com/firebase/firebase-js-sdk/issues/6110
+    const keys = ['a/a/a/a/a/a/a/a', 'b/b/b/b/a/a', 'c/c/a/a', 'd/d'];
+    return cache
+      .addEntries(keys.map(k => doc(k, VERSION, DOC_DATA)))
+      .then(() => cache.getEntries(documentKeySet(...keys.map(k => key(k)))))
+      .then(read => {
+        expect(read.size).to.equal(keys.length);
+      });
+  });
+
   it('can set and read several documents including missing document', () => {
     const docs = [
       doc(DOC_PATH, VERSION, DOC_DATA),

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

@@ -19,13 +19,15 @@ import { expect, use } from 'chai';
 import chaiAsPromised from 'chai-as-promised';
 import { Context } from 'mocha';
 
+import { dbKeyComparator } from '../../../src/local/indexeddb_remote_document_cache';
 import { PersistencePromise } from '../../../src/local/persistence_promise';
 import {
   SimpleDb,
   SimpleDbSchemaConverter,
   SimpleDbStore,
   SimpleDbTransaction
 } from '../../../src/local/simple_db';
+import { DocumentKey } from '../../../src/model/document_key';
 import { fail } from '../../../src/util/assert';
 import { Code, FirestoreError } from '../../../src/util/error';
 
@@ -66,13 +68,16 @@ class TestSchemaConverter implements SimpleDbSchemaConverter {
     fromVersion: number,
     toVersion: number
   ): PersistencePromise<void> {
-    const objectStore = db.createObjectStore('users', { keyPath: 'id' });
-    objectStore.createIndex('age-name', ['age', 'name'], {
+    const userStore = db.createObjectStore('users', { keyPath: 'id' });
+    userStore.createIndex('age-name', ['age', 'name'], {
       unique: false
     });
 
     // A store that uses arrays as keys.
-    db.createObjectStore('docs');
+    const docStore = db.createObjectStore('docs');
+    docStore.createIndex('path', ['prefixPath', 'collectionId', 'documentId'], {
+      unique: false
+    });
     return PersistencePromise.resolve();
   }
 }
@@ -526,6 +531,68 @@ describe('SimpleDb', () => {
     );
   });
 
+  it('correctly sorts keys with nested arrays', async function (this: Context) {
+    // This test verifies that the sorting in IndexedDb matches
+    // `dbKeyComparator()`
+
+    const keys = [
+      'a/a/a/a/a/a/a/a/a/a',
+      'a/b/a/a/a/a/a/a/a/b',
+      'b/a/a/a/a/a/a/a/a/a',
+      'b/b/a/a/a/a/a/a/a/b',
+      'b/b/a/a/a/a/a/a',
+      'b/b/b/a/a/a/a/b',
+      'c/c/a/a/a/a',
+      'd/d/a/a',
+      'e/e'
+    ].map(k => DocumentKey.fromPath(k));
+
+    interface ValueType {
+      prefixPath: string[];
+      collectionId: string;
+      documentId: string;
+    }
+
+    const expectedOrder = [...keys];
+    expectedOrder.sort(dbKeyComparator);
+
+    const actualOrder = await db.runTransaction(
+      this.test!.fullTitle(),
+      'readwrite',
+      ['docs'],
+      txn => {
+        const store = txn.store<string[], ValueType>('docs');
+
+        const writes = keys.map(k => {
+          const path = k.path.toArray();
+          return store.put(k.path.toArray(), {
+            prefixPath: path.slice(0, path.length - 2),
+            collectionId: path[path.length - 2],
+            documentId: path[path.length - 1]
+          });
+        });
+
+        return PersistencePromise.waitFor(writes).next(() =>
+          store
+            .loadAll('path')
+            .next(keys =>
+              keys.map(k =>
+                DocumentKey.fromSegments([
+                  ...k.prefixPath,
+                  k.collectionId,
+                  k.documentId
+                ])
+              )
+            )
+        );
+      }
+    );
+
+    expect(actualOrder.map(k => k.toString())).to.deep.equal(
+      expectedOrder.map(k => k.toString())
+    );
+  });
+
   it('retries transactions', async function (this: Context) {
     let attemptCount = 0;
 

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

@@ -17,7 +17,6 @@
 
 import { SnapshotVersion } from '../../../src/core/snapshot_version';
 import { IndexManager } from '../../../src/local/index_manager';
-import { remoteDocumentCacheGetNewDocumentChanges } from '../../../src/local/indexeddb_remote_document_cache';
 import { Persistence } from '../../../src/local/persistence';
 import { PersistencePromise } from '../../../src/local/persistence_promise';
 import { RemoteDocumentCache } from '../../../src/local/remote_document_cache';
@@ -139,23 +138,6 @@ export class TestRemoteDocumentCache {
     );
   }
 
-  getNewDocumentChanges(sinceReadTime: SnapshotVersion): Promise<{
-    changedDocs: MutableDocumentMap;
-    readTime: SnapshotVersion;
-  }> {
-    return this.persistence.runTransaction(
-      'getNewDocumentChanges',
-      'readonly',
-      txn => {
-        return remoteDocumentCacheGetNewDocumentChanges(
-          this.cache,
-          txn,
-          sinceReadTime
-        );
-      }
-    );
-  }
-
   getSize(): Promise<number> {
     return this.persistence.runTransaction('get size', 'readonly', txn =>
       this.cache.getSize(txn)