From 71df7080cd0cc87869c41a22503dc28d554aa96e Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 7 Apr 2022 09:18:15 -0600 Subject: [PATCH 1/5] Fix the ordering of the index-based lookup in getAll(keys) --- .../local/indexeddb_remote_document_cache.ts | 27 +++++-- packages/firestore/src/local/simple_db.ts | 6 +- .../unit/local/remote_document_cache.test.ts | 12 +++ .../test/unit/local/simple_db.test.ts | 73 ++++++++++++++++++- .../unit/local/test_remote_document_cache.ts | 18 ----- 5 files changed, 107 insertions(+), 29 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 6a22c28bd80..f0505af2e41 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -611,12 +611,29 @@ 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(); + + 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; + } + } + + return ( + primitiveComparator(left.length, right.length) || + primitiveComparator(left[left.length - 2], right[right.length - 2]) || + primitiveComparator(left[left.length - 1], right[right.length - 1]) + ); } diff --git a/packages/firestore/src/local/simple_db.ts b/packages/firestore/src/local/simple_db.ts index 103c96ed819..c9a0ddacf97 100644 --- a/packages/firestore/src/local/simple_db.ts +++ b/packages/firestore/src/local/simple_db.ts @@ -656,6 +656,8 @@ export class SimpleDbStore< loadAll(): PersistencePromise; /** Loads all elements for the index range from the object store. */ loadAll(range: IDBKeyRange): PersistencePromise; + /** Loads all elements ordered by the given index. */ + loadAll(index: string): PersistencePromise; /** * 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( diff --git a/packages/firestore/test/unit/local/remote_document_cache.test.ts b/packages/firestore/test/unit/local/remote_document_cache.test.ts index d22a4eab7dc..2dfedc79ca6 100644 --- a/packages/firestore/test/unit/local/remote_document_cache.test.ts +++ b/packages/firestore/test/unit/local/remote_document_cache.test.ts @@ -405,6 +405,18 @@ function genericRemoteDocumentCacheTests( expect(read.size).to.equal(keys.length); }); }); + 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 = [ diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index b07bac1a73d..51d754dd018 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -19,6 +19,7 @@ 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, @@ -26,6 +27,7 @@ import { 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 { - 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.only('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('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; diff --git a/packages/firestore/test/unit/local/test_remote_document_cache.ts b/packages/firestore/test/unit/local/test_remote_document_cache.ts index 7e6d523d822..62344dd4b41 100644 --- a/packages/firestore/test/unit/local/test_remote_document_cache.ts +++ b/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 { return this.persistence.runTransaction('get size', 'readonly', txn => this.cache.getSize(txn) From 05475a379115f4385e60dbaee3d7a2171ac5cd11 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 7 Apr 2022 09:54:50 -0600 Subject: [PATCH 2/5] Lint --- packages/firestore/test/unit/local/simple_db.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index 51d754dd018..1b789623961 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -531,7 +531,7 @@ describe('SimpleDb', () => { ); }); - it.only('correctly sorts keys with nested arrays', async function (this: Context) { + it('correctly sorts keys with nested arrays', async function (this: Context) { // This test verifies that the sorting in IndexedDb matches // `dbKeyComparator()` From 3d875d22e4d83a66f0ac26fd6f05fb04ede74193 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 7 Apr 2022 10:49:12 -0600 Subject: [PATCH 3/5] Empty line --- packages/firestore/test/unit/local/remote_document_cache.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/firestore/test/unit/local/remote_document_cache.test.ts b/packages/firestore/test/unit/local/remote_document_cache.test.ts index 2dfedc79ca6..0bed7c4dade 100644 --- a/packages/firestore/test/unit/local/remote_document_cache.test.ts +++ b/packages/firestore/test/unit/local/remote_document_cache.test.ts @@ -405,6 +405,7 @@ function genericRemoteDocumentCacheTests( expect(read.size).to.equal(keys.length); }); }); + 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. From 0a4d45cfc2bce73936dc00afc29c0a95f63999cb Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 7 Apr 2022 10:52:05 -0600 Subject: [PATCH 4/5] Create nasty-hairs-knock.md --- .changeset/nasty-hairs-knock.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/nasty-hairs-knock.md diff --git a/.changeset/nasty-hairs-knock.md b/.changeset/nasty-hairs-knock.md new file mode 100644 index 00000000000..796044b730e --- /dev/null +++ b/.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". From 7c28b1653ac262291d3b9e562b687e6d91b47ae0 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 12 Apr 2022 10:32:37 -0600 Subject: [PATCH 5/5] Feedback --- .../local/indexeddb_remote_document_cache.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index f0505af2e41..19b7c566322 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -623,6 +623,7 @@ 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]); @@ -631,9 +632,15 @@ export function dbKeyComparator(l: DocumentKey, r: DocumentKey): number { } } - return ( - primitiveComparator(left.length, right.length) || - primitiveComparator(left[left.length - 2], right[right.length - 2]) || - primitiveComparator(left[left.length - 1], right[right.length - 1]) - ); + 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]); }