From 48642adbf1a5bc3c1f7699777c14e4308eb846d7 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 9 Oct 2019 14:31:26 -0700 Subject: [PATCH 1/7] Add runTransaction parameter --- .../src/local/indexeddb_persistence.ts | 93 ++- .../unit/local/encoded_resource_path.test.ts | 11 +- .../unit/local/indexeddb_persistence.test.ts | 771 ++++++++++-------- .../test/unit/local/simple_db.test.ts | 53 +- .../test/unit/specs/spec_test_runner.ts | 17 +- 5 files changed, 534 insertions(+), 411 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_persistence.ts b/packages/firestore/src/local/indexeddb_persistence.ts index 5a889e4a3c9..1b163ab1cbd 100644 --- a/packages/firestore/src/local/indexeddb_persistence.ts +++ b/packages/firestore/src/local/indexeddb_persistence.ts @@ -321,6 +321,7 @@ export class IndexedDbPersistence implements Persistence { .then(() => { return this.simpleDb.runTransaction( 'readonly', + /* idempotent= */ false, [DbTargetGlobal.store], txn => { return getHighestListenSequenceNumber(txn).next( @@ -344,8 +345,11 @@ export class IndexedDbPersistence implements Persistence { } private startRemoteDocumentCache(): Promise { - return this.simpleDb.runTransaction('readonly', ALL_STORES, txn => - this.remoteDocumentCache.start(txn) + return this.simpleDb.runTransaction( + 'readonly', + /* idempotent= */ false, + ALL_STORES, + txn => this.remoteDocumentCache.start(txn) ); } @@ -391,47 +395,52 @@ export class IndexedDbPersistence implements Persistence { * primary lease. */ private updateClientMetadataAndTryBecomePrimary(): Promise { - return this.simpleDb.runTransaction('readwrite', ALL_STORES, txn => { - const metadataStore = clientMetadataStore(txn); - return metadataStore - .put( - new DbClientMetadata( - this.clientId, - Date.now(), - this.networkEnabled, - this.inForeground + return this.simpleDb.runTransaction( + 'readwrite', + /* idempotent= */ false, + ALL_STORES, + txn => { + const metadataStore = clientMetadataStore(txn); + return metadataStore + .put( + new DbClientMetadata( + this.clientId, + Date.now(), + this.networkEnabled, + this.inForeground + ) ) - ) - .next(() => { - if (this.isPrimary) { - return this.verifyPrimaryLease(txn).next(success => { - if (!success) { - this.isPrimary = false; - this.queue.enqueueAndForget(() => - this.primaryStateListener(false) - ); - } - }); - } - }) - .next(() => this.canActAsPrimary(txn)) - .next(canActAsPrimary => { - const wasPrimary = this.isPrimary; - this.isPrimary = canActAsPrimary; - - if (wasPrimary !== this.isPrimary) { - this.queue.enqueueAndForget(() => - this.primaryStateListener(this.isPrimary) - ); - } + .next(() => { + if (this.isPrimary) { + return this.verifyPrimaryLease(txn).next(success => { + if (!success) { + this.isPrimary = false; + this.queue.enqueueAndForget(() => + this.primaryStateListener(false) + ); + } + }); + } + }) + .next(() => this.canActAsPrimary(txn)) + .next(canActAsPrimary => { + const wasPrimary = this.isPrimary; + this.isPrimary = canActAsPrimary; + + if (wasPrimary !== this.isPrimary) { + this.queue.enqueueAndForget(() => + this.primaryStateListener(this.isPrimary) + ); + } - if (wasPrimary && !this.isPrimary) { - return this.releasePrimaryLeaseIfHeld(txn); - } else if (this.isPrimary) { - return this.acquireOrExtendPrimaryLease(txn); - } - }); - }); + if (wasPrimary && !this.isPrimary) { + return this.releasePrimaryLeaseIfHeld(txn); + } else if (this.isPrimary) { + return this.acquireOrExtendPrimaryLease(txn); + } + }); + } + ); } private verifyPrimaryLease( @@ -646,6 +655,7 @@ export class IndexedDbPersistence implements Persistence { this.detachWindowUnloadHook(); await this.simpleDb.runTransaction( 'readwrite', + /* idempotent= */ false, [DbPrimaryClient.store, DbClientMetadata.store], txn => { return this.releasePrimaryLeaseIfHeld(txn).next(() => @@ -678,6 +688,7 @@ export class IndexedDbPersistence implements Persistence { getActiveClients(): Promise { return this.simpleDb.runTransaction( 'readonly', + /* idempotent= */ false, [DbClientMetadata.store], txn => { return clientMetadataStore(txn) diff --git a/packages/firestore/test/unit/local/encoded_resource_path.test.ts b/packages/firestore/test/unit/local/encoded_resource_path.test.ts index 544392f7872..8ea54d6c06a 100644 --- a/packages/firestore/test/unit/local/encoded_resource_path.test.ts +++ b/packages/firestore/test/unit/local/encoded_resource_path.test.ts @@ -216,7 +216,12 @@ function runTransaction( transaction: SimpleDbTransaction ) => PersistencePromise ): Promise { - return db.runTransaction('readwrite', ['test'], txn => { - return fn(txn.store('test'), txn); - }); + return db.runTransaction( + 'readwrite', + /* idempotent= */ false, + ['test'], + txn => { + return fn(txn.store('test'), txn); + } + ); } diff --git a/packages/firestore/test/unit/local/indexeddb_persistence.test.ts b/packages/firestore/test/unit/local/indexeddb_persistence.test.ts index fd8585be851..8fd7d5f2a44 100644 --- a/packages/firestore/test/unit/local/indexeddb_persistence.test.ts +++ b/packages/firestore/test/unit/local/indexeddb_persistence.test.ts @@ -224,6 +224,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { const sdb = new SimpleDb(db); return sdb.runTransaction( 'readwrite', + /* idempotent= */ false, [DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store], txn => { const targets = txn.store(DbTarget.store); @@ -253,6 +254,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { const sdb = new SimpleDb(db); return sdb.runTransaction( 'readwrite', + /* idempotent= */ false, [DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store], txn => { const targets = txn.store(DbTarget.store); @@ -317,39 +319,49 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { return withDb(3, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => { - const store = txn.store(DbMutationBatch.store); - return PersistencePromise.forEach( - testMutations, - (testMutation: DbMutationBatch) => store.put(testMutation) - ); - }); + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + [DbMutationBatch.store], + txn => { + const store = txn.store(DbMutationBatch.store); + return PersistencePromise.forEach( + testMutations, + (testMutation: DbMutationBatch) => store.put(testMutation) + ); + } + ); }).then(() => withDb(4, db => { expect(db.version).to.be.equal(4); expect(getAllObjectStores(db)).to.have.members(V4_STORES); const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => { - const store = txn.store( - DbMutationBatch.store - ); - let p = PersistencePromise.forEach( - testMutations, - (testMutation: DbMutationBatch) => - store.get(testMutation.batchId).next(mutationBatch => { - expect(mutationBatch).to.deep.equal(testMutation); - }) - ); - p = p.next(() => { - store - .add({} as any) // eslint-disable-line @typescript-eslint/no-explicit-any - .next(batchId => { - expect(batchId).to.equal(43); - }); - }); - return p; - }); + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + [DbMutationBatch.store], + txn => { + const store = txn.store( + DbMutationBatch.store + ); + let p = PersistencePromise.forEach( + testMutations, + (testMutation: DbMutationBatch) => + store.get(testMutation.batchId).next(mutationBatch => { + expect(mutationBatch).to.deep.equal(testMutation); + }) + ); + p = p.next(() => { + store + .add({} as any) // eslint-disable-line @typescript-eslint/no-explicit-any + .next(batchId => { + expect(batchId).to.equal(43); + }); + }); + return p; + } + ); }) ); }); @@ -422,56 +434,11 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { return withDb(4, db => { const sdb = new SimpleDb(db); // We can only use the V4 stores here, since that's as far as we've upgraded. - return sdb.runTransaction('readwrite', V4_STORES, txn => { - const mutationBatchStore = txn.store< - DbMutationBatchKey, - DbMutationBatch - >(DbMutationBatch.store); - const documentMutationStore = txn.store< - DbDocumentMutationKey, - DbDocumentMutation - >(DbDocumentMutation.store); - const mutationQueuesStore = txn.store< - DbMutationQueueKey, - DbMutationQueue - >(DbMutationQueue.store); - // Manually populate the mutation queue and create all indicies. - return PersistencePromise.forEach( - testMutations, - (testMutation: DbMutationBatch) => { - return mutationBatchStore.put(testMutation).next(() => { - return PersistencePromise.forEach( - testMutation.mutations, - (write: firestoreV1ApiClientInterfaces.Write) => { - const indexKey = DbDocumentMutation.key( - testMutation.userId, - path(write.update!.name!, 5), - testMutation.batchId - ); - return documentMutationStore.put( - indexKey, - DbDocumentMutation.PLACEHOLDER - ); - } - ); - }); - } - ).next(() => - // Populate the mutation queues' metadata - PersistencePromise.waitFor([ - mutationQueuesStore.put(new DbMutationQueue('foo', 2, '')), - mutationQueuesStore.put(new DbMutationQueue('bar', 3, '')), - mutationQueuesStore.put(new DbMutationQueue('empty', -1, '')) - ]) - ); - }); - }).then(() => - withDb(5, db => { - expect(db.version).to.be.equal(5); - - const sdb = new SimpleDb(db); - // There is no V5_STORES, continue using V4. - return sdb.runTransaction('readwrite', V4_STORES, txn => { + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V4_STORES, + txn => { const mutationBatchStore = txn.store< DbMutationBatchKey, DbMutationBatch @@ -484,27 +451,82 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { DbMutationQueueKey, DbMutationQueue >(DbMutationQueue.store); - - // Verify that all but the two pending mutations have been cleared - // by the migration. - let p = mutationBatchStore.count().next(count => { - expect(count).to.deep.equal(2); - }); - p = p.next(() => - documentMutationStore.count().next(count => { - expect(count).to.equal(2); - }) + // Manually populate the mutation queue and create all indicies. + return PersistencePromise.forEach( + testMutations, + (testMutation: DbMutationBatch) => { + return mutationBatchStore.put(testMutation).next(() => { + return PersistencePromise.forEach( + testMutation.mutations, + (write: firestoreV1ApiClientInterfaces.Write) => { + const indexKey = DbDocumentMutation.key( + testMutation.userId, + path(write.update!.name!, 5), + testMutation.batchId + ); + return documentMutationStore.put( + indexKey, + DbDocumentMutation.PLACEHOLDER + ); + } + ); + }); + } + ).next(() => + // Populate the mutation queues' metadata + PersistencePromise.waitFor([ + mutationQueuesStore.put(new DbMutationQueue('foo', 2, '')), + mutationQueuesStore.put(new DbMutationQueue('bar', 3, '')), + mutationQueuesStore.put(new DbMutationQueue('empty', -1, '')) + ]) ); + } + ); + }).then(() => + withDb(5, db => { + expect(db.version).to.be.equal(5); - // Verify that we still have one metadata entry for each existing - // queue - p = p.next(() => - mutationQueuesStore.count().next(count => { - expect(count).to.equal(3); - }) - ); - return p; - }); + const sdb = new SimpleDb(db); + // There is no V5_STORES, continue using V4. + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V4_STORES, + txn => { + const mutationBatchStore = txn.store< + DbMutationBatchKey, + DbMutationBatch + >(DbMutationBatch.store); + const documentMutationStore = txn.store< + DbDocumentMutationKey, + DbDocumentMutation + >(DbDocumentMutation.store); + const mutationQueuesStore = txn.store< + DbMutationQueueKey, + DbMutationQueue + >(DbMutationQueue.store); + + // Verify that all but the two pending mutations have been cleared + // by the migration. + let p = mutationBatchStore.count().next(count => { + expect(count).to.deep.equal(2); + }); + p = p.next(() => + documentMutationStore.count().next(count => { + expect(count).to.equal(2); + }) + ); + + // Verify that we still have one metadata entry for each existing + // queue + p = p.next(() => + mutationQueuesStore.count().next(count => { + expect(count).to.equal(3); + }) + ); + return p; + } + ); }) ); }); @@ -523,30 +545,40 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { })); // V5 stores doesn't exist const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V4_STORES, txn => { - const store = txn.store( - DbRemoteDocument.store - ); - return PersistencePromise.forEach( - dbRemoteDocs, - ({ dbKey, dbDoc }: { dbKey: string[]; dbDoc: DbRemoteDocument }) => - store.put(dbKey, dbDoc) - ); - }); + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V4_STORES, + txn => { + const store = txn.store( + DbRemoteDocument.store + ); + return PersistencePromise.forEach( + dbRemoteDocs, + ({ dbKey, dbDoc }: { dbKey: string[]; dbDoc: DbRemoteDocument }) => + store.put(dbKey, dbDoc) + ); + } + ); }); await withDb(6, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readonly', V6_STORES, txn => { - const store = txn.store< - DbRemoteDocumentGlobalKey, - DbRemoteDocumentGlobal - >(DbRemoteDocumentGlobal.store); - return store.get(DbRemoteDocumentGlobal.key).next(metadata => { - // We don't really care what the size is, just that it's greater than 0. - // Our sizing algorithm may change at some point. - expect(metadata!.byteSize).to.be.greaterThan(0); - }); - }); + return sdb.runTransaction( + 'readonly', + /* idempotent= */ false, + V6_STORES, + txn => { + const store = txn.store< + DbRemoteDocumentGlobalKey, + DbRemoteDocumentGlobal + >(DbRemoteDocumentGlobal.store); + return store.get(DbRemoteDocumentGlobal.key).next(metadata => { + // We don't really care what the size is, just that it's greater than 0. + // Our sizing algorithm may change at some point. + expect(metadata!.byteSize).to.be.greaterThan(0); + }); + } + ); }); }); @@ -559,80 +591,91 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { const serializer = TEST_SERIALIZER; const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V6_STORES, txn => { - const targetGlobalStore = txn.store( - DbTargetGlobal.store - ); - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - const targetDocumentStore = txn.store< - DbTargetDocumentKey, - DbTargetDocument - >(DbTargetDocument.store); - return targetGlobalStore - .get(DbTargetGlobal.key) - .next(metadata => { - expect(metadata).to.not.be.null; - metadata!.highestListenSequenceNumber = newSequenceNumber; - return targetGlobalStore.put(DbTargetGlobal.key, metadata!); - }) - .next(() => { - // Set up some documents (we only need the keys) - // For the odd ones, add sentinel rows. - const promises: Array> = []; - for (let i = 0; i < 10; i++) { - const document = doc('docs/doc_' + i, 1, { foo: 'bar' }); - promises.push( - remoteDocumentStore.put( - document.key.path.toArray(), - serializer.toDbRemoteDocument(document, document.version) - ) - ); - if (i % 2 === 1) { + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V6_STORES, + txn => { + const targetGlobalStore = txn.store< + DbTargetGlobalKey, + DbTargetGlobal + >(DbTargetGlobal.store); + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + const targetDocumentStore = txn.store< + DbTargetDocumentKey, + DbTargetDocument + >(DbTargetDocument.store); + return targetGlobalStore + .get(DbTargetGlobal.key) + .next(metadata => { + expect(metadata).to.not.be.null; + metadata!.highestListenSequenceNumber = newSequenceNumber; + return targetGlobalStore.put(DbTargetGlobal.key, metadata!); + }) + .next(() => { + // Set up some documents (we only need the keys) + // For the odd ones, add sentinel rows. + const promises: Array> = []; + for (let i = 0; i < 10; i++) { + const document = doc('docs/doc_' + i, 1, { foo: 'bar' }); promises.push( - targetDocumentStore.put( - new DbTargetDocument( - 0, - encode(document.key.path), - oldSequenceNumber - ) + remoteDocumentStore.put( + document.key.path.toArray(), + serializer.toDbRemoteDocument(document, document.version) ) ); + if (i % 2 === 1) { + promises.push( + targetDocumentStore.put( + new DbTargetDocument( + 0, + encode(document.key.path), + oldSequenceNumber + ) + ) + ); + } } - } - return PersistencePromise.waitFor(promises); - }); - }); + return PersistencePromise.waitFor(promises); + }); + } + ); }); // Now run the migration and verify await withDb(7, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readonly', V6_STORES, txn => { - const targetDocumentStore = txn.store< - DbTargetDocumentKey, - DbTargetDocument - >(DbTargetDocument.store); - const range = IDBKeyRange.bound( - [0], - [1], - /*lowerOpen=*/ false, - /*upperOpen=*/ true - ); - return targetDocumentStore.iterate( - { range }, - ([_, path], targetDocument) => { - const decoded = decode(path); - const lastSegment = decoded.lastSegment(); - const docNum = +lastSegment.split('_')[1]; - const expected = - docNum % 2 === 1 ? oldSequenceNumber : newSequenceNumber; - expect(targetDocument.sequenceNumber).to.equal(expected); - } - ); - }); + return sdb.runTransaction( + 'readonly', + /* idempotent= */ false, + V6_STORES, + txn => { + const targetDocumentStore = txn.store< + DbTargetDocumentKey, + DbTargetDocument + >(DbTargetDocument.store); + const range = IDBKeyRange.bound( + [0], + [1], + /*lowerOpen=*/ false, + /*upperOpen=*/ true + ); + return targetDocumentStore.iterate( + { range }, + ([_, path], targetDocument) => { + const decoded = decode(path); + const lastSegment = decoded.lastSegment(); + const docNum = +lastSegment.split('_')[1]; + const expected = + docNum % 2 === 1 ? oldSequenceNumber : newSequenceNumber; + expect(targetDocument.sequenceNumber).to.equal(expected); + } + ); + } + ); }); }); @@ -661,62 +704,78 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(7, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V6_STORES, txn => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - const documentMutationStore = txn.store< - DbDocumentMutationKey, - DbDocumentMutation - >(DbDocumentMutation.store); - // We "cheat" and only write the DbDocumentMutation index entries, since that's - // all the migration uses. - return PersistencePromise.forEach(writePaths, (writePath: string) => { - const indexKey = DbDocumentMutation.key( - 'dummy-uid', - path(writePath), - /*dummy batchId=*/ 123 - ); - return documentMutationStore.put( - indexKey, - DbDocumentMutation.PLACEHOLDER - ); - }).next(() => { - // Write the remote document entries. - return PersistencePromise.forEach(remoteDocPaths, (path: string) => { - const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); - return remoteDocumentStore.put( - remoteDoc.key.path.toArray(), - TEST_SERIALIZER.toDbRemoteDocument(remoteDoc, remoteDoc.version) + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V6_STORES, + txn => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + const documentMutationStore = txn.store< + DbDocumentMutationKey, + DbDocumentMutation + >(DbDocumentMutation.store); + // We "cheat" and only write the DbDocumentMutation index entries, since that's + // all the migration uses. + return PersistencePromise.forEach(writePaths, (writePath: string) => { + const indexKey = DbDocumentMutation.key( + 'dummy-uid', + path(writePath), + /*dummy batchId=*/ 123 + ); + return documentMutationStore.put( + indexKey, + DbDocumentMutation.PLACEHOLDER + ); + }).next(() => { + // Write the remote document entries. + return PersistencePromise.forEach( + remoteDocPaths, + (path: string) => { + const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); + return remoteDocumentStore.put( + remoteDoc.key.path.toArray(), + TEST_SERIALIZER.toDbRemoteDocument( + remoteDoc, + remoteDoc.version + ) + ); + } ); }); - }); - }); + } + ); }); // Migrate to v8 and verify index entries. await withDb(8, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V8_STORES, txn => { - const collectionParentsStore = txn.store< - DbCollectionParentKey, - DbCollectionParent - >(DbCollectionParent.store); - return collectionParentsStore.loadAll().next(indexEntries => { - const actualParents: { [key: string]: string[] } = {}; - for (const { collectionId, parent } of indexEntries) { - let parents = actualParents[collectionId]; - if (!parents) { - parents = []; - actualParents[collectionId] = parents; + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V8_STORES, + txn => { + const collectionParentsStore = txn.store< + DbCollectionParentKey, + DbCollectionParent + >(DbCollectionParent.store); + return collectionParentsStore.loadAll().next(indexEntries => { + const actualParents: { [key: string]: string[] } = {}; + for (const { collectionId, parent } of indexEntries) { + let parents = actualParents[collectionId]; + if (!parents) { + parents = []; + actualParents[collectionId] = parents; + } + parents.push(decode(parent).toString()); } - parents.push(decode(parent).toString()); - } - expect(actualParents).to.deep.equal(expectedParents); - }); - }); + expect(actualParents).to.deep.equal(expectedParents); + }); + } + ); }); }); @@ -740,73 +799,86 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(8, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V8_STORES, txn => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - // Write the remote document entries. - return PersistencePromise.forEach(existingDocPaths, (path: string) => { - const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); - - const dbRemoteDoc = TEST_SERIALIZER.toDbRemoteDocument( - remoteDoc, - remoteDoc.version - ); - // Mimic the old serializer and delete previously unset values - delete dbRemoteDoc.readTime; - delete dbRemoteDoc.parentPath; + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V8_STORES, + txn => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); - return remoteDocumentStore.put( - remoteDoc.key.path.toArray(), - dbRemoteDoc + // Write the remote document entries. + return PersistencePromise.forEach( + existingDocPaths, + (path: string) => { + const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); + + const dbRemoteDoc = TEST_SERIALIZER.toDbRemoteDocument( + remoteDoc, + remoteDoc.version + ); + // Mimic the old serializer and delete previously unset values + delete dbRemoteDoc.readTime; + delete dbRemoteDoc.parentPath; + + return remoteDocumentStore.put( + remoteDoc.key.path.toArray(), + dbRemoteDoc + ); + } ); - }); - }); + } + ); }); // Migrate to v9 and verify that new documents are indexed. await withDb(9, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V8_STORES, txn => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - // Verify the existing remote document entries. - return remoteDocumentStore - .loadAll() - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll1/doc1', - 'projects/test-project/databases/(default)/documents/coll1/doc2', - 'projects/test-project/databases/(default)/documents/coll2/doc1', - 'projects/test-project/databases/(default)/documents/coll2/doc2' - ]); - }) - .next(() => addDocs(txn, newDocPaths, /* version= */ 2)) - .next(() => { - // Verify that we can get recent changes in a collection filtered by - // read time. - const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); - const range = IDBKeyRange.lowerBound( - [['coll2'], lastReadTime], - true - ); - return remoteDocumentStore - .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll2/doc3', - 'projects/test-project/databases/(default)/documents/coll2/doc4' - ]); - }); - }); - }); + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V8_STORES, + txn => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + + // Verify the existing remote document entries. + return remoteDocumentStore + .loadAll() + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll1/doc1', + 'projects/test-project/databases/(default)/documents/coll1/doc2', + 'projects/test-project/databases/(default)/documents/coll2/doc1', + 'projects/test-project/databases/(default)/documents/coll2/doc2' + ]); + }) + .next(() => addDocs(txn, newDocPaths, /* version= */ 2)) + .next(() => { + // Verify that we can get recent changes in a collection filtered by + // read time. + const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); + const range = IDBKeyRange.lowerBound( + [['coll2'], lastReadTime], + true + ); + return remoteDocumentStore + .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll2/doc3', + 'projects/test-project/databases/(default)/documents/coll2/doc4' + ]); + }); + }); + } + ); }); }); @@ -816,31 +888,36 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(9, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V8_STORES, txn => { - return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => - addDocs(txn, newDocPaths, /* version= */ 2).next(() => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); - const range = IDBKeyRange.lowerBound( - [['coll'], lastReadTime], - true - ); - return remoteDocumentStore - .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll/doc3', - 'projects/test-project/databases/(default)/documents/coll/doc4' - ]); - }); - }) - ); - }); + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V8_STORES, + txn => { + return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => + addDocs(txn, newDocPaths, /* version= */ 2).next(() => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + + const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); + const range = IDBKeyRange.lowerBound( + [['coll'], lastReadTime], + true + ); + return remoteDocumentStore + .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll/doc3', + 'projects/test-project/databases/(default)/documents/coll/doc4' + ]); + }); + }) + ); + } + ); }); }); @@ -850,28 +927,33 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(9, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction('readwrite', V8_STORES, txn => { - return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => - addDocs(txn, newDocPaths, /* version= */ 2).next(() => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); - const range = IDBKeyRange.lowerBound(lastReadTime, true); - return remoteDocumentStore - .loadAll(DbRemoteDocument.readTimeIndex, range) - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll1/new', - 'projects/test-project/databases/(default)/documents/coll2/new' - ]); - }); - }) - ); - }); + return sdb.runTransaction( + 'readwrite', + /* idempotent= */ false, + V8_STORES, + txn => { + return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => + addDocs(txn, newDocPaths, /* version= */ 2).next(() => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + + const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); + const range = IDBKeyRange.lowerBound(lastReadTime, true); + return remoteDocumentStore + .loadAll(DbRemoteDocument.readTimeIndex, range) + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll1/new', + 'projects/test-project/databases/(default)/documents/coll2/new' + ]); + }); + }) + ); + } + ); }); }); @@ -912,12 +994,17 @@ describe('IndexedDb: canActAsPrimary', () => { SCHEMA_VERSION, new SchemaConverter(TEST_SERIALIZER) ); - await simpleDb.runTransaction('readwrite', [DbPrimaryClient.store], txn => { - const primaryStore = txn.store( - DbPrimaryClient.store - ); - return primaryStore.delete(DbPrimaryClient.key); - }); + await simpleDb.runTransaction( + 'readwrite', + /* idempotent= */ false, + [DbPrimaryClient.store], + txn => { + const primaryStore = txn.store( + DbPrimaryClient.store + ); + return primaryStore.delete(DbPrimaryClient.key); + } + ); simpleDb.close(); } diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index 57df764657a..4aac3b3f083 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -93,9 +93,14 @@ describe('SimpleDb', () => { transaction: SimpleDbTransaction ) => PersistencePromise ): Promise { - return db.runTransaction('readwrite', ['users'], txn => { - return fn(txn.store('users'), txn); - }); + return db.runTransaction( + 'readwrite', + /* idempotent= */ false, + ['users'], + txn => { + return fn(txn.store('users'), txn); + } + ); } function writeTestData(): Promise { @@ -481,23 +486,33 @@ describe('SimpleDb', () => { ['foo', 'd'], ['foob'] ]; - await db.runTransaction('readwrite', ['users', 'docs'], txn => { - const docsStore = txn.store('docs'); - return PersistencePromise.waitFor( - keys.map(key => { - const value = 'doc ' + key.join('/'); - return docsStore.put(key, value); - }) - ); - }); + await db.runTransaction( + 'readwrite', + /* idempotent= */ false, + ['users', 'docs'], + txn => { + const docsStore = txn.store('docs'); + return PersistencePromise.waitFor( + keys.map(key => { + const value = 'doc ' + key.join('/'); + return docsStore.put(key, value); + }) + ); + } + ); - await db.runTransaction('readonly', ['docs'], txn => { - const store = txn.store('docs'); - const range = IDBKeyRange.bound(['foo'], ['foo', 'c']); - return store.loadAll(range).next(results => { - expect(results).to.deep.equal(['doc foo', 'doc foo/bar/baz']); - }); - }); + await db.runTransaction( + 'readonly', + /* idempotent= */ false, + ['docs'], + txn => { + const store = txn.store('docs'); + const range = IDBKeyRange.bound(['foo'], ['foo', 'c']); + return store.loadAll(range).next(results => { + expect(results).to.deep.equal(['doc foo', 'doc foo/bar/baz']); + }); + } + ); }); // A little perf test for convenient benchmarking diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 1b195e94b8d..00f418208e0 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -1650,11 +1650,16 @@ async function clearCurrentPrimaryLease(): Promise { SCHEMA_VERSION, new SchemaConverter(TEST_SERIALIZER) ); - await db.runTransaction('readwrite', [DbPrimaryClient.store], txn => { - const primaryClientStore = txn.store( - DbPrimaryClient.store - ); - return primaryClientStore.delete(DbPrimaryClient.key); - }); + await db.runTransaction( + 'readwrite', + /* idempotent= */ false, + [DbPrimaryClient.store], + txn => { + const primaryClientStore = txn.store( + DbPrimaryClient.store + ); + return primaryClientStore.delete(DbPrimaryClient.key); + } + ); db.close(); } From e4cdfe0abe0ba9f5aa400dd0c81e3aea5bfbfcad Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 9 Oct 2019 14:34:16 -0700 Subject: [PATCH 2/7] Add transaction retries --- .../src/local/indexeddb_persistence.ts | 22 ++++- packages/firestore/src/local/simple_db.ts | 88 ++++++++++++++----- .../test/unit/local/simple_db.test.ts | 80 +++++++++++++++++ 3 files changed, 166 insertions(+), 24 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_persistence.ts b/packages/firestore/src/local/indexeddb_persistence.ts index 1b163ab1cbd..1148cea2394 100644 --- a/packages/firestore/src/local/indexeddb_persistence.ts +++ b/packages/firestore/src/local/indexeddb_persistence.ts @@ -121,6 +121,15 @@ export class IndexedDbTransaction extends PersistenceTransaction { } } +// The different modes supported by `IndexedDbPersistence.runTransaction()` +type TransactionMode = + | 'readonly' + | 'readwrite' + | 'readwrite-primary' + | 'readonly-idempotent' + | 'readwrite-idempotent' + | 'readwrite-primary-idempotent'; + /** * An IndexedDB-backed instance of Persistence. Data is stored persistently * across sessions. @@ -753,17 +762,26 @@ export class IndexedDbPersistence implements Persistence { runTransaction( action: string, - mode: 'readonly' | 'readwrite' | 'readwrite-primary', + mode: TransactionMode, transactionOperation: ( transaction: PersistenceTransaction ) => PersistencePromise ): Promise { log.debug(LOG_TAG, 'Starting transaction:', action); + const idempotent = + [ + 'readonly-idempotent', + 'readwrite-idempotent', + 'readwrite-primary-idempotent' + ].indexOf(mode) !== -1; + const readonly = ['readonly', 'readonly-idempotent'].indexOf(mode) !== -1; + // Do all transactions as readwrite against all object stores, since we // are the only reader/writer. return this.simpleDb.runTransaction( - mode === 'readonly' ? 'readonly' : 'readwrite', + readonly ? 'readonly' : 'readwrite', + idempotent, ALL_STORES, simpleDbTxn => { if (mode === 'readwrite-primary') { diff --git a/packages/firestore/src/local/simple_db.ts b/packages/firestore/src/local/simple_db.ts index 70cfdc784bc..9401d8a308d 100644 --- a/packages/firestore/src/local/simple_db.ts +++ b/packages/firestore/src/local/simple_db.ts @@ -25,6 +25,12 @@ import { PersistencePromise } from './persistence_promise'; const LOG_TAG = 'SimpleDb'; +/** + * The maximum number of retry attempts for an IndexedDb transaction that fails + * with a DOMException. + */ +const TRANSACTION_RETRY_COUNT = 3; + export interface SimpleDbSchemaConverter { createOrUpgrade( db: IDBDatabase, @@ -242,32 +248,58 @@ export class SimpleDb { }; } - runTransaction( + async runTransaction( mode: 'readonly' | 'readwrite', + idempotent: boolean, objectStores: string[], transactionFn: (transaction: SimpleDbTransaction) => PersistencePromise ): Promise { - const transaction = SimpleDbTransaction.open(this.db, mode, objectStores); - const transactionFnResult = transactionFn(transaction) - .catch(error => { - // Abort the transaction if there was an error. - transaction.abort(error); - // We cannot actually recover, and calling `abort()` will cause the transaction's - // completion promise to be rejected. This in turn means that we won't use - // `transactionFnResult` below. We return a rejection here so that we don't add the - // possibility of returning `void` to the type of `transactionFnResult`. - return PersistencePromise.reject(error); - }) - .toPromise(); - - // As noted above, errors are propagated by aborting the transaction. So - // we swallow any error here to avoid the browser logging it as unhandled. - transactionFnResult.catch(() => {}); - - // Wait for the transaction to complete (i.e. IndexedDb's onsuccess event to - // fire), but still return the original transactionFnResult back to the - // caller. - return transaction.completionPromise.then(() => transactionFnResult); + let attemptNumber = 0; + + while (true) { + ++attemptNumber; + + const transaction = SimpleDbTransaction.open(this.db, mode, objectStores); + try { + const transactionFnResult = transactionFn(transaction) + .catch(error => { + // Abort the transaction if there was an error. + transaction.abort(error); + // We cannot actually recover, and calling `abort()` will cause the transaction's + // completion promise to be rejected. This in turn means that we won't use + // `transactionFnResult` below. We return a rejection here so that we don't add the + // possibility of returning `void` to the type of `transactionFnResult`. + return PersistencePromise.reject(error); + }) + .toPromise(); + + // As noted above, errors are propagated by aborting the transaction. So + // we swallow any error here to avoid the browser logging it as unhandled. + transactionFnResult.catch(() => {}); + + // Wait for the transaction to complete (i.e. IndexedDb's onsuccess event to + // fire), but still return the original transactionFnResult back to the + // caller. + await transaction.completionPromise; + return transactionFnResult; + } catch (e) { + // TODO: We could probably be smarter about this an not retry exceptions + // that are likely unrecoverable (such as quota exceeded errors). + const retryable = + idempotent && + isDomException(e) && + attemptNumber < TRANSACTION_RETRY_COUNT; + debug( + 'Transaction failed with error: %s. Retrying: %s.', + e.message, + retryable + ); + + if (!retryable) { + return Promise.reject(e); + } + } + } } close(): void { @@ -755,3 +787,15 @@ function checkForAndReportiOSError(error: DOMException): Error { } return error; } + +/** + * Checks whether an error is a DOMException (e.g. as thrown by IndexedDb). + * + * Supports both browsers and Node with persistence. + */ +function isDomException(e: Error): boolean { + return ( + (typeof DOMException !== 'undefined' && e instanceof DOMException) || + e.constructor.name === 'DOMException' + ); +} diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index 4aac3b3f083..0b75f3d53f7 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -515,6 +515,86 @@ describe('SimpleDb', () => { ); }); + it('retries idempotent transactions', async () => { + let attemptCount = 0; + + const result = await db.runTransaction( + 'readwrite', + /* idempotent= */ true, + ['users'], + txn => { + ++attemptCount; + if (attemptCount === 1) { + const store = txn.store('users'); + return store + .add(dummyUser) + .next(() => { + return store.add(dummyUser); + }) + .next(() => 'Uncaught unique key violation'); + } else { + console.log('retry'); + return PersistencePromise.resolve('success'); + } + } + ); + + expect(result).to.equal('success'); + expect(attemptCount).to.equal(2); + }); + + it('retries transaction only three times', async () => { + let attemptCount = 0; + + await expect( + db.runTransaction('readwrite', /* idempotent= */ true, ['users'], txn => { + ++attemptCount; + const store = txn.store('users'); + return store + .add(dummyUser) + .next(() => { + return store.add(dummyUser); + }) + .next(() => 'Uncaught unique key violation'); + }) + ).to.eventually.be.rejected; + + expect(attemptCount).to.equal(3); + }); + + it('does not retry explicitly aborted transactions', async () => { + let attemptCount = 0; + + await expect( + db.runTransaction('readonly', /* idempotent= */ true, ['users'], txn => { + ++attemptCount; + txn.abort(); + return PersistencePromise.reject(new Error('Aborted')); + }) + ).to.eventually.be.rejected; + + expect(attemptCount).to.equal(1); + }); + + it('does not retry non-idempotent transactions', async () => { + let attemptCount = 0; + + await expect( + db.runTransaction('readonly', /* idempotent= */ false, ['users'], txn => { + ++attemptCount; + const store = txn.store('users'); + return store + .add(dummyUser) + .next(() => { + return store.add(dummyUser); + }) + .next(() => 'Uncaught unique key violation'); + }) + ).to.eventually.be.rejectedWith('Uncaught unique key violation'); + + expect(attemptCount).to.equal(1); + }); + // A little perf test for convenient benchmarking // eslint-disable-next-line no-restricted-properties it.skip('Perf', () => { From d3669b6d8565c40317d5553f81500d62d545c16d Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 9 Oct 2019 15:02:45 -0700 Subject: [PATCH 3/7] Unit test fix --- packages/firestore/test/unit/local/simple_db.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index 0b75f3d53f7..f22e02fe521 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -566,7 +566,7 @@ describe('SimpleDb', () => { let attemptCount = 0; await expect( - db.runTransaction('readonly', /* idempotent= */ true, ['users'], txn => { + db.runTransaction('readwrite', /* idempotent= */ true, ['users'], txn => { ++attemptCount; txn.abort(); return PersistencePromise.reject(new Error('Aborted')); @@ -580,7 +580,7 @@ describe('SimpleDb', () => { let attemptCount = 0; await expect( - db.runTransaction('readonly', /* idempotent= */ false, ['users'], txn => { + db.runTransaction('readwrite', /* idempotent= */ false, ['users'], txn => { ++attemptCount; const store = txn.store('users'); return store From 10c2f8ec4f00a89baa44d5eabfa856e3c40c1299 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 9 Oct 2019 15:02:51 -0700 Subject: [PATCH 4/7] [AUTOMATED]: Prettier Code Styling --- .../test/unit/local/simple_db.test.ts | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index f22e02fe521..302a6dde818 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -580,16 +580,21 @@ describe('SimpleDb', () => { let attemptCount = 0; await expect( - db.runTransaction('readwrite', /* idempotent= */ false, ['users'], txn => { - ++attemptCount; - const store = txn.store('users'); - return store - .add(dummyUser) - .next(() => { - return store.add(dummyUser); - }) - .next(() => 'Uncaught unique key violation'); - }) + db.runTransaction( + 'readwrite', + /* idempotent= */ false, + ['users'], + txn => { + ++attemptCount; + const store = txn.store('users'); + return store + .add(dummyUser) + .next(() => { + return store.add(dummyUser); + }) + .next(() => 'Uncaught unique key violation'); + } + ) ).to.eventually.be.rejectedWith('Uncaught unique key violation'); expect(attemptCount).to.equal(1); From a1e5a00238241cde38ccf4675e482fc69559ee28 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 9 Oct 2019 15:30:16 -0700 Subject: [PATCH 5/7] First round of feedback --- .../src/local/indexeddb_persistence.ts | 117 ++- packages/firestore/src/local/simple_db.ts | 37 +- .../unit/local/encoded_resource_path.test.ts | 11 +- .../unit/local/indexeddb_persistence.test.ts | 771 ++++++++---------- .../test/unit/local/simple_db.test.ts | 98 +-- .../test/unit/specs/spec_test_runner.ts | 17 +- 6 files changed, 467 insertions(+), 584 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_persistence.ts b/packages/firestore/src/local/indexeddb_persistence.ts index 1148cea2394..f6aabeba18f 100644 --- a/packages/firestore/src/local/indexeddb_persistence.ts +++ b/packages/firestore/src/local/indexeddb_persistence.ts @@ -122,7 +122,7 @@ export class IndexedDbTransaction extends PersistenceTransaction { } // The different modes supported by `IndexedDbPersistence.runTransaction()` -type TransactionMode = +type IndexedDbTransactionMode = | 'readonly' | 'readwrite' | 'readwrite-primary' @@ -330,7 +330,6 @@ export class IndexedDbPersistence implements Persistence { .then(() => { return this.simpleDb.runTransaction( 'readonly', - /* idempotent= */ false, [DbTargetGlobal.store], txn => { return getHighestListenSequenceNumber(txn).next( @@ -354,11 +353,8 @@ export class IndexedDbPersistence implements Persistence { } private startRemoteDocumentCache(): Promise { - return this.simpleDb.runTransaction( - 'readonly', - /* idempotent= */ false, - ALL_STORES, - txn => this.remoteDocumentCache.start(txn) + return this.simpleDb.runTransaction('readonly', ALL_STORES, txn => + this.remoteDocumentCache.start(txn) ); } @@ -404,52 +400,47 @@ export class IndexedDbPersistence implements Persistence { * primary lease. */ private updateClientMetadataAndTryBecomePrimary(): Promise { - return this.simpleDb.runTransaction( - 'readwrite', - /* idempotent= */ false, - ALL_STORES, - txn => { - const metadataStore = clientMetadataStore(txn); - return metadataStore - .put( - new DbClientMetadata( - this.clientId, - Date.now(), - this.networkEnabled, - this.inForeground - ) + return this.simpleDb.runTransaction('readwrite', ALL_STORES, txn => { + const metadataStore = clientMetadataStore(txn); + return metadataStore + .put( + new DbClientMetadata( + this.clientId, + Date.now(), + this.networkEnabled, + this.inForeground ) - .next(() => { - if (this.isPrimary) { - return this.verifyPrimaryLease(txn).next(success => { - if (!success) { - this.isPrimary = false; - this.queue.enqueueAndForget(() => - this.primaryStateListener(false) - ); - } - }); - } - }) - .next(() => this.canActAsPrimary(txn)) - .next(canActAsPrimary => { - const wasPrimary = this.isPrimary; - this.isPrimary = canActAsPrimary; - - if (wasPrimary !== this.isPrimary) { - this.queue.enqueueAndForget(() => - this.primaryStateListener(this.isPrimary) - ); - } + ) + .next(() => { + if (this.isPrimary) { + return this.verifyPrimaryLease(txn).next(success => { + if (!success) { + this.isPrimary = false; + this.queue.enqueueAndForget(() => + this.primaryStateListener(false) + ); + } + }); + } + }) + .next(() => this.canActAsPrimary(txn)) + .next(canActAsPrimary => { + const wasPrimary = this.isPrimary; + this.isPrimary = canActAsPrimary; + + if (wasPrimary !== this.isPrimary) { + this.queue.enqueueAndForget(() => + this.primaryStateListener(this.isPrimary) + ); + } - if (wasPrimary && !this.isPrimary) { - return this.releasePrimaryLeaseIfHeld(txn); - } else if (this.isPrimary) { - return this.acquireOrExtendPrimaryLease(txn); - } - }); - } - ); + if (wasPrimary && !this.isPrimary) { + return this.releasePrimaryLeaseIfHeld(txn); + } else if (this.isPrimary) { + return this.acquireOrExtendPrimaryLease(txn); + } + }); + }); } private verifyPrimaryLease( @@ -664,7 +655,6 @@ export class IndexedDbPersistence implements Persistence { this.detachWindowUnloadHook(); await this.simpleDb.runTransaction( 'readwrite', - /* idempotent= */ false, [DbPrimaryClient.store, DbClientMetadata.store], txn => { return this.releasePrimaryLeaseIfHeld(txn).next(() => @@ -697,7 +687,6 @@ export class IndexedDbPersistence implements Persistence { getActiveClients(): Promise { return this.simpleDb.runTransaction( 'readonly', - /* idempotent= */ false, [DbClientMetadata.store], txn => { return clientMetadataStore(txn) @@ -762,26 +751,28 @@ export class IndexedDbPersistence implements Persistence { runTransaction( action: string, - mode: TransactionMode, + mode: IndexedDbTransactionMode, transactionOperation: ( transaction: PersistenceTransaction ) => PersistencePromise ): Promise { log.debug(LOG_TAG, 'Starting transaction:', action); - const idempotent = - [ - 'readonly-idempotent', - 'readwrite-idempotent', - 'readwrite-primary-idempotent' - ].indexOf(mode) !== -1; - const readonly = ['readonly', 'readonly-idempotent'].indexOf(mode) !== -1; + // TODO(schmidt-sebastian): Simplify once all transactions are idempotent. + const idempotent = mode.endsWith('idempotent'); + const readonly = mode.startsWith('readonly'); + const simpleDbMode = readonly + ? idempotent + ? 'readonly-idempotent' + : 'readonly' + : idempotent + ? 'readwrite-idempotent' + : 'readwrite'; // Do all transactions as readwrite against all object stores, since we // are the only reader/writer. return this.simpleDb.runTransaction( - readonly ? 'readonly' : 'readwrite', - idempotent, + simpleDbMode, ALL_STORES, simpleDbTxn => { if (mode === 'readwrite-primary') { diff --git a/packages/firestore/src/local/simple_db.ts b/packages/firestore/src/local/simple_db.ts index 9401d8a308d..3e88cae6245 100644 --- a/packages/firestore/src/local/simple_db.ts +++ b/packages/firestore/src/local/simple_db.ts @@ -31,6 +31,13 @@ const LOG_TAG = 'SimpleDb'; */ const TRANSACTION_RETRY_COUNT = 3; +// The different modes supported by `SimpleDb.runTransaction()` +type SimpleDbTransactionMode = + | 'readonly' + | 'readwrite' + | 'readonly-idempotent' + | 'readwrite-idempotent'; + export interface SimpleDbSchemaConverter { createOrUpgrade( db: IDBDatabase, @@ -249,17 +256,22 @@ export class SimpleDb { } async runTransaction( - mode: 'readonly' | 'readwrite', - idempotent: boolean, + mode: SimpleDbTransactionMode, objectStores: string[], transactionFn: (transaction: SimpleDbTransaction) => PersistencePromise ): Promise { + let readonly = mode.startsWith('idempotent'); + let idempotent = mode.endsWith('idempotent'); let attemptNumber = 0; while (true) { ++attemptNumber; - const transaction = SimpleDbTransaction.open(this.db, mode, objectStores); + const transaction = SimpleDbTransaction.open( + this.db, + readonly ? 'readonly' : 'readwrite', + objectStores + ); try { const transactionFnResult = transactionFn(transaction) .catch(error => { @@ -283,8 +295,9 @@ export class SimpleDb { await transaction.completionPromise; return transactionFnResult; } catch (e) { - // TODO: We could probably be smarter about this an not retry exceptions - // that are likely unrecoverable (such as quota exceeded errors). + // TODO(schmidt-sebastian): We could probably be smarter about this and + // not retry exceptions that are likely unrecoverable (such as quota + // exceeded errors). const retryable = idempotent && isDomException(e) && @@ -788,14 +801,12 @@ function checkForAndReportiOSError(error: DOMException): Error { return error; } -/** - * Checks whether an error is a DOMException (e.g. as thrown by IndexedDb). - * - * Supports both browsers and Node with persistence. - */ -function isDomException(e: Error): boolean { +/** Checks whether an error is a DOMException (e.g. as thrown by IndexedDb). */ +function isDomException(error: Error): boolean { + // DOMException is not a global type in Node with persistence, and hence we + // check the constructor name if the type in unknown. return ( - (typeof DOMException !== 'undefined' && e instanceof DOMException) || - e.constructor.name === 'DOMException' + (typeof DOMException !== 'undefined' && error instanceof DOMException) || + error.constructor.name === 'DOMException' ); } diff --git a/packages/firestore/test/unit/local/encoded_resource_path.test.ts b/packages/firestore/test/unit/local/encoded_resource_path.test.ts index 8ea54d6c06a..544392f7872 100644 --- a/packages/firestore/test/unit/local/encoded_resource_path.test.ts +++ b/packages/firestore/test/unit/local/encoded_resource_path.test.ts @@ -216,12 +216,7 @@ function runTransaction( transaction: SimpleDbTransaction ) => PersistencePromise ): Promise { - return db.runTransaction( - 'readwrite', - /* idempotent= */ false, - ['test'], - txn => { - return fn(txn.store('test'), txn); - } - ); + return db.runTransaction('readwrite', ['test'], txn => { + return fn(txn.store('test'), txn); + }); } diff --git a/packages/firestore/test/unit/local/indexeddb_persistence.test.ts b/packages/firestore/test/unit/local/indexeddb_persistence.test.ts index 8fd7d5f2a44..fd8585be851 100644 --- a/packages/firestore/test/unit/local/indexeddb_persistence.test.ts +++ b/packages/firestore/test/unit/local/indexeddb_persistence.test.ts @@ -224,7 +224,6 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { const sdb = new SimpleDb(db); return sdb.runTransaction( 'readwrite', - /* idempotent= */ false, [DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store], txn => { const targets = txn.store(DbTarget.store); @@ -254,7 +253,6 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { const sdb = new SimpleDb(db); return sdb.runTransaction( 'readwrite', - /* idempotent= */ false, [DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store], txn => { const targets = txn.store(DbTarget.store); @@ -319,49 +317,39 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { return withDb(3, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - [DbMutationBatch.store], - txn => { - const store = txn.store(DbMutationBatch.store); - return PersistencePromise.forEach( - testMutations, - (testMutation: DbMutationBatch) => store.put(testMutation) - ); - } - ); + return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => { + const store = txn.store(DbMutationBatch.store); + return PersistencePromise.forEach( + testMutations, + (testMutation: DbMutationBatch) => store.put(testMutation) + ); + }); }).then(() => withDb(4, db => { expect(db.version).to.be.equal(4); expect(getAllObjectStores(db)).to.have.members(V4_STORES); const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - [DbMutationBatch.store], - txn => { - const store = txn.store( - DbMutationBatch.store - ); - let p = PersistencePromise.forEach( - testMutations, - (testMutation: DbMutationBatch) => - store.get(testMutation.batchId).next(mutationBatch => { - expect(mutationBatch).to.deep.equal(testMutation); - }) - ); - p = p.next(() => { - store - .add({} as any) // eslint-disable-line @typescript-eslint/no-explicit-any - .next(batchId => { - expect(batchId).to.equal(43); - }); - }); - return p; - } - ); + return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => { + const store = txn.store( + DbMutationBatch.store + ); + let p = PersistencePromise.forEach( + testMutations, + (testMutation: DbMutationBatch) => + store.get(testMutation.batchId).next(mutationBatch => { + expect(mutationBatch).to.deep.equal(testMutation); + }) + ); + p = p.next(() => { + store + .add({} as any) // eslint-disable-line @typescript-eslint/no-explicit-any + .next(batchId => { + expect(batchId).to.equal(43); + }); + }); + return p; + }); }) ); }); @@ -434,11 +422,56 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { return withDb(4, db => { const sdb = new SimpleDb(db); // We can only use the V4 stores here, since that's as far as we've upgraded. - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V4_STORES, - txn => { + return sdb.runTransaction('readwrite', V4_STORES, txn => { + const mutationBatchStore = txn.store< + DbMutationBatchKey, + DbMutationBatch + >(DbMutationBatch.store); + const documentMutationStore = txn.store< + DbDocumentMutationKey, + DbDocumentMutation + >(DbDocumentMutation.store); + const mutationQueuesStore = txn.store< + DbMutationQueueKey, + DbMutationQueue + >(DbMutationQueue.store); + // Manually populate the mutation queue and create all indicies. + return PersistencePromise.forEach( + testMutations, + (testMutation: DbMutationBatch) => { + return mutationBatchStore.put(testMutation).next(() => { + return PersistencePromise.forEach( + testMutation.mutations, + (write: firestoreV1ApiClientInterfaces.Write) => { + const indexKey = DbDocumentMutation.key( + testMutation.userId, + path(write.update!.name!, 5), + testMutation.batchId + ); + return documentMutationStore.put( + indexKey, + DbDocumentMutation.PLACEHOLDER + ); + } + ); + }); + } + ).next(() => + // Populate the mutation queues' metadata + PersistencePromise.waitFor([ + mutationQueuesStore.put(new DbMutationQueue('foo', 2, '')), + mutationQueuesStore.put(new DbMutationQueue('bar', 3, '')), + mutationQueuesStore.put(new DbMutationQueue('empty', -1, '')) + ]) + ); + }); + }).then(() => + withDb(5, db => { + expect(db.version).to.be.equal(5); + + const sdb = new SimpleDb(db); + // There is no V5_STORES, continue using V4. + return sdb.runTransaction('readwrite', V4_STORES, txn => { const mutationBatchStore = txn.store< DbMutationBatchKey, DbMutationBatch @@ -451,82 +484,27 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { DbMutationQueueKey, DbMutationQueue >(DbMutationQueue.store); - // Manually populate the mutation queue and create all indicies. - return PersistencePromise.forEach( - testMutations, - (testMutation: DbMutationBatch) => { - return mutationBatchStore.put(testMutation).next(() => { - return PersistencePromise.forEach( - testMutation.mutations, - (write: firestoreV1ApiClientInterfaces.Write) => { - const indexKey = DbDocumentMutation.key( - testMutation.userId, - path(write.update!.name!, 5), - testMutation.batchId - ); - return documentMutationStore.put( - indexKey, - DbDocumentMutation.PLACEHOLDER - ); - } - ); - }); - } - ).next(() => - // Populate the mutation queues' metadata - PersistencePromise.waitFor([ - mutationQueuesStore.put(new DbMutationQueue('foo', 2, '')), - mutationQueuesStore.put(new DbMutationQueue('bar', 3, '')), - mutationQueuesStore.put(new DbMutationQueue('empty', -1, '')) - ]) - ); - } - ); - }).then(() => - withDb(5, db => { - expect(db.version).to.be.equal(5); - const sdb = new SimpleDb(db); - // There is no V5_STORES, continue using V4. - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V4_STORES, - txn => { - const mutationBatchStore = txn.store< - DbMutationBatchKey, - DbMutationBatch - >(DbMutationBatch.store); - const documentMutationStore = txn.store< - DbDocumentMutationKey, - DbDocumentMutation - >(DbDocumentMutation.store); - const mutationQueuesStore = txn.store< - DbMutationQueueKey, - DbMutationQueue - >(DbMutationQueue.store); - - // Verify that all but the two pending mutations have been cleared - // by the migration. - let p = mutationBatchStore.count().next(count => { - expect(count).to.deep.equal(2); - }); - p = p.next(() => - documentMutationStore.count().next(count => { - expect(count).to.equal(2); - }) - ); + // Verify that all but the two pending mutations have been cleared + // by the migration. + let p = mutationBatchStore.count().next(count => { + expect(count).to.deep.equal(2); + }); + p = p.next(() => + documentMutationStore.count().next(count => { + expect(count).to.equal(2); + }) + ); - // Verify that we still have one metadata entry for each existing - // queue - p = p.next(() => - mutationQueuesStore.count().next(count => { - expect(count).to.equal(3); - }) - ); - return p; - } - ); + // Verify that we still have one metadata entry for each existing + // queue + p = p.next(() => + mutationQueuesStore.count().next(count => { + expect(count).to.equal(3); + }) + ); + return p; + }); }) ); }); @@ -545,40 +523,30 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { })); // V5 stores doesn't exist const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V4_STORES, - txn => { - const store = txn.store( - DbRemoteDocument.store - ); - return PersistencePromise.forEach( - dbRemoteDocs, - ({ dbKey, dbDoc }: { dbKey: string[]; dbDoc: DbRemoteDocument }) => - store.put(dbKey, dbDoc) - ); - } - ); + return sdb.runTransaction('readwrite', V4_STORES, txn => { + const store = txn.store( + DbRemoteDocument.store + ); + return PersistencePromise.forEach( + dbRemoteDocs, + ({ dbKey, dbDoc }: { dbKey: string[]; dbDoc: DbRemoteDocument }) => + store.put(dbKey, dbDoc) + ); + }); }); await withDb(6, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readonly', - /* idempotent= */ false, - V6_STORES, - txn => { - const store = txn.store< - DbRemoteDocumentGlobalKey, - DbRemoteDocumentGlobal - >(DbRemoteDocumentGlobal.store); - return store.get(DbRemoteDocumentGlobal.key).next(metadata => { - // We don't really care what the size is, just that it's greater than 0. - // Our sizing algorithm may change at some point. - expect(metadata!.byteSize).to.be.greaterThan(0); - }); - } - ); + return sdb.runTransaction('readonly', V6_STORES, txn => { + const store = txn.store< + DbRemoteDocumentGlobalKey, + DbRemoteDocumentGlobal + >(DbRemoteDocumentGlobal.store); + return store.get(DbRemoteDocumentGlobal.key).next(metadata => { + // We don't really care what the size is, just that it's greater than 0. + // Our sizing algorithm may change at some point. + expect(metadata!.byteSize).to.be.greaterThan(0); + }); + }); }); }); @@ -591,91 +559,80 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { const serializer = TEST_SERIALIZER; const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V6_STORES, - txn => { - const targetGlobalStore = txn.store< - DbTargetGlobalKey, - DbTargetGlobal - >(DbTargetGlobal.store); - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - const targetDocumentStore = txn.store< - DbTargetDocumentKey, - DbTargetDocument - >(DbTargetDocument.store); - return targetGlobalStore - .get(DbTargetGlobal.key) - .next(metadata => { - expect(metadata).to.not.be.null; - metadata!.highestListenSequenceNumber = newSequenceNumber; - return targetGlobalStore.put(DbTargetGlobal.key, metadata!); - }) - .next(() => { - // Set up some documents (we only need the keys) - // For the odd ones, add sentinel rows. - const promises: Array> = []; - for (let i = 0; i < 10; i++) { - const document = doc('docs/doc_' + i, 1, { foo: 'bar' }); + return sdb.runTransaction('readwrite', V6_STORES, txn => { + const targetGlobalStore = txn.store( + DbTargetGlobal.store + ); + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + const targetDocumentStore = txn.store< + DbTargetDocumentKey, + DbTargetDocument + >(DbTargetDocument.store); + return targetGlobalStore + .get(DbTargetGlobal.key) + .next(metadata => { + expect(metadata).to.not.be.null; + metadata!.highestListenSequenceNumber = newSequenceNumber; + return targetGlobalStore.put(DbTargetGlobal.key, metadata!); + }) + .next(() => { + // Set up some documents (we only need the keys) + // For the odd ones, add sentinel rows. + const promises: Array> = []; + for (let i = 0; i < 10; i++) { + const document = doc('docs/doc_' + i, 1, { foo: 'bar' }); + promises.push( + remoteDocumentStore.put( + document.key.path.toArray(), + serializer.toDbRemoteDocument(document, document.version) + ) + ); + if (i % 2 === 1) { promises.push( - remoteDocumentStore.put( - document.key.path.toArray(), - serializer.toDbRemoteDocument(document, document.version) + targetDocumentStore.put( + new DbTargetDocument( + 0, + encode(document.key.path), + oldSequenceNumber + ) ) ); - if (i % 2 === 1) { - promises.push( - targetDocumentStore.put( - new DbTargetDocument( - 0, - encode(document.key.path), - oldSequenceNumber - ) - ) - ); - } } - return PersistencePromise.waitFor(promises); - }); - } - ); + } + return PersistencePromise.waitFor(promises); + }); + }); }); // Now run the migration and verify await withDb(7, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readonly', - /* idempotent= */ false, - V6_STORES, - txn => { - const targetDocumentStore = txn.store< - DbTargetDocumentKey, - DbTargetDocument - >(DbTargetDocument.store); - const range = IDBKeyRange.bound( - [0], - [1], - /*lowerOpen=*/ false, - /*upperOpen=*/ true - ); - return targetDocumentStore.iterate( - { range }, - ([_, path], targetDocument) => { - const decoded = decode(path); - const lastSegment = decoded.lastSegment(); - const docNum = +lastSegment.split('_')[1]; - const expected = - docNum % 2 === 1 ? oldSequenceNumber : newSequenceNumber; - expect(targetDocument.sequenceNumber).to.equal(expected); - } - ); - } - ); + return sdb.runTransaction('readonly', V6_STORES, txn => { + const targetDocumentStore = txn.store< + DbTargetDocumentKey, + DbTargetDocument + >(DbTargetDocument.store); + const range = IDBKeyRange.bound( + [0], + [1], + /*lowerOpen=*/ false, + /*upperOpen=*/ true + ); + return targetDocumentStore.iterate( + { range }, + ([_, path], targetDocument) => { + const decoded = decode(path); + const lastSegment = decoded.lastSegment(); + const docNum = +lastSegment.split('_')[1]; + const expected = + docNum % 2 === 1 ? oldSequenceNumber : newSequenceNumber; + expect(targetDocument.sequenceNumber).to.equal(expected); + } + ); + }); }); }); @@ -704,78 +661,62 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(7, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V6_STORES, - txn => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - const documentMutationStore = txn.store< - DbDocumentMutationKey, - DbDocumentMutation - >(DbDocumentMutation.store); - // We "cheat" and only write the DbDocumentMutation index entries, since that's - // all the migration uses. - return PersistencePromise.forEach(writePaths, (writePath: string) => { - const indexKey = DbDocumentMutation.key( - 'dummy-uid', - path(writePath), - /*dummy batchId=*/ 123 - ); - return documentMutationStore.put( - indexKey, - DbDocumentMutation.PLACEHOLDER - ); - }).next(() => { - // Write the remote document entries. - return PersistencePromise.forEach( - remoteDocPaths, - (path: string) => { - const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); - return remoteDocumentStore.put( - remoteDoc.key.path.toArray(), - TEST_SERIALIZER.toDbRemoteDocument( - remoteDoc, - remoteDoc.version - ) - ); - } + return sdb.runTransaction('readwrite', V6_STORES, txn => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + const documentMutationStore = txn.store< + DbDocumentMutationKey, + DbDocumentMutation + >(DbDocumentMutation.store); + // We "cheat" and only write the DbDocumentMutation index entries, since that's + // all the migration uses. + return PersistencePromise.forEach(writePaths, (writePath: string) => { + const indexKey = DbDocumentMutation.key( + 'dummy-uid', + path(writePath), + /*dummy batchId=*/ 123 + ); + return documentMutationStore.put( + indexKey, + DbDocumentMutation.PLACEHOLDER + ); + }).next(() => { + // Write the remote document entries. + return PersistencePromise.forEach(remoteDocPaths, (path: string) => { + const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); + return remoteDocumentStore.put( + remoteDoc.key.path.toArray(), + TEST_SERIALIZER.toDbRemoteDocument(remoteDoc, remoteDoc.version) ); }); - } - ); + }); + }); }); // Migrate to v8 and verify index entries. await withDb(8, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V8_STORES, - txn => { - const collectionParentsStore = txn.store< - DbCollectionParentKey, - DbCollectionParent - >(DbCollectionParent.store); - return collectionParentsStore.loadAll().next(indexEntries => { - const actualParents: { [key: string]: string[] } = {}; - for (const { collectionId, parent } of indexEntries) { - let parents = actualParents[collectionId]; - if (!parents) { - parents = []; - actualParents[collectionId] = parents; - } - parents.push(decode(parent).toString()); + return sdb.runTransaction('readwrite', V8_STORES, txn => { + const collectionParentsStore = txn.store< + DbCollectionParentKey, + DbCollectionParent + >(DbCollectionParent.store); + return collectionParentsStore.loadAll().next(indexEntries => { + const actualParents: { [key: string]: string[] } = {}; + for (const { collectionId, parent } of indexEntries) { + let parents = actualParents[collectionId]; + if (!parents) { + parents = []; + actualParents[collectionId] = parents; } + parents.push(decode(parent).toString()); + } - expect(actualParents).to.deep.equal(expectedParents); - }); - } - ); + expect(actualParents).to.deep.equal(expectedParents); + }); + }); }); }); @@ -799,86 +740,73 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(8, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V8_STORES, - txn => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - // Write the remote document entries. - return PersistencePromise.forEach( - existingDocPaths, - (path: string) => { - const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); - - const dbRemoteDoc = TEST_SERIALIZER.toDbRemoteDocument( - remoteDoc, - remoteDoc.version - ); - // Mimic the old serializer and delete previously unset values - delete dbRemoteDoc.readTime; - delete dbRemoteDoc.parentPath; + return sdb.runTransaction('readwrite', V8_STORES, txn => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + + // Write the remote document entries. + return PersistencePromise.forEach(existingDocPaths, (path: string) => { + const remoteDoc = doc(path, /*version=*/ 1, { data: 1 }); + + const dbRemoteDoc = TEST_SERIALIZER.toDbRemoteDocument( + remoteDoc, + remoteDoc.version + ); + // Mimic the old serializer and delete previously unset values + delete dbRemoteDoc.readTime; + delete dbRemoteDoc.parentPath; - return remoteDocumentStore.put( - remoteDoc.key.path.toArray(), - dbRemoteDoc - ); - } + return remoteDocumentStore.put( + remoteDoc.key.path.toArray(), + dbRemoteDoc ); - } - ); + }); + }); }); // Migrate to v9 and verify that new documents are indexed. await withDb(9, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V8_STORES, - txn => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - // Verify the existing remote document entries. - return remoteDocumentStore - .loadAll() - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll1/doc1', - 'projects/test-project/databases/(default)/documents/coll1/doc2', - 'projects/test-project/databases/(default)/documents/coll2/doc1', - 'projects/test-project/databases/(default)/documents/coll2/doc2' - ]); - }) - .next(() => addDocs(txn, newDocPaths, /* version= */ 2)) - .next(() => { - // Verify that we can get recent changes in a collection filtered by - // read time. - const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); - const range = IDBKeyRange.lowerBound( - [['coll2'], lastReadTime], - true - ); - return remoteDocumentStore - .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll2/doc3', - 'projects/test-project/databases/(default)/documents/coll2/doc4' - ]); - }); - }); - } - ); + return sdb.runTransaction('readwrite', V8_STORES, txn => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + + // Verify the existing remote document entries. + return remoteDocumentStore + .loadAll() + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll1/doc1', + 'projects/test-project/databases/(default)/documents/coll1/doc2', + 'projects/test-project/databases/(default)/documents/coll2/doc1', + 'projects/test-project/databases/(default)/documents/coll2/doc2' + ]); + }) + .next(() => addDocs(txn, newDocPaths, /* version= */ 2)) + .next(() => { + // Verify that we can get recent changes in a collection filtered by + // read time. + const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); + const range = IDBKeyRange.lowerBound( + [['coll2'], lastReadTime], + true + ); + return remoteDocumentStore + .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll2/doc3', + 'projects/test-project/databases/(default)/documents/coll2/doc4' + ]); + }); + }); + }); }); }); @@ -888,36 +816,31 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(9, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V8_STORES, - txn => { - return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => - addDocs(txn, newDocPaths, /* version= */ 2).next(() => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); - const range = IDBKeyRange.lowerBound( - [['coll'], lastReadTime], - true - ); - return remoteDocumentStore - .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll/doc3', - 'projects/test-project/databases/(default)/documents/coll/doc4' - ]); - }); - }) - ); - } - ); + return sdb.runTransaction('readwrite', V8_STORES, txn => { + return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => + addDocs(txn, newDocPaths, /* version= */ 2).next(() => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + + const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); + const range = IDBKeyRange.lowerBound( + [['coll'], lastReadTime], + true + ); + return remoteDocumentStore + .loadAll(DbRemoteDocument.collectionReadTimeIndex, range) + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll/doc3', + 'projects/test-project/databases/(default)/documents/coll/doc4' + ]); + }); + }) + ); + }); }); }); @@ -927,33 +850,28 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => { await withDb(9, db => { const sdb = new SimpleDb(db); - return sdb.runTransaction( - 'readwrite', - /* idempotent= */ false, - V8_STORES, - txn => { - return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => - addDocs(txn, newDocPaths, /* version= */ 2).next(() => { - const remoteDocumentStore = txn.store< - DbRemoteDocumentKey, - DbRemoteDocument - >(DbRemoteDocument.store); - - const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); - const range = IDBKeyRange.lowerBound(lastReadTime, true); - return remoteDocumentStore - .loadAll(DbRemoteDocument.readTimeIndex, range) - .next(docsRead => { - const keys = docsRead.map(dbDoc => dbDoc.document!.name); - expect(keys).to.have.members([ - 'projects/test-project/databases/(default)/documents/coll1/new', - 'projects/test-project/databases/(default)/documents/coll2/new' - ]); - }); - }) - ); - } - ); + return sdb.runTransaction('readwrite', V8_STORES, txn => { + return addDocs(txn, oldDocPaths, /* version= */ 1).next(() => + addDocs(txn, newDocPaths, /* version= */ 2).next(() => { + const remoteDocumentStore = txn.store< + DbRemoteDocumentKey, + DbRemoteDocument + >(DbRemoteDocument.store); + + const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1)); + const range = IDBKeyRange.lowerBound(lastReadTime, true); + return remoteDocumentStore + .loadAll(DbRemoteDocument.readTimeIndex, range) + .next(docsRead => { + const keys = docsRead.map(dbDoc => dbDoc.document!.name); + expect(keys).to.have.members([ + 'projects/test-project/databases/(default)/documents/coll1/new', + 'projects/test-project/databases/(default)/documents/coll2/new' + ]); + }); + }) + ); + }); }); }); @@ -994,17 +912,12 @@ describe('IndexedDb: canActAsPrimary', () => { SCHEMA_VERSION, new SchemaConverter(TEST_SERIALIZER) ); - await simpleDb.runTransaction( - 'readwrite', - /* idempotent= */ false, - [DbPrimaryClient.store], - txn => { - const primaryStore = txn.store( - DbPrimaryClient.store - ); - return primaryStore.delete(DbPrimaryClient.key); - } - ); + await simpleDb.runTransaction('readwrite', [DbPrimaryClient.store], txn => { + const primaryStore = txn.store( + DbPrimaryClient.store + ); + return primaryStore.delete(DbPrimaryClient.key); + }); simpleDb.close(); } diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index 302a6dde818..11ed5d19451 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -93,14 +93,9 @@ describe('SimpleDb', () => { transaction: SimpleDbTransaction ) => PersistencePromise ): Promise { - return db.runTransaction( - 'readwrite', - /* idempotent= */ false, - ['users'], - txn => { - return fn(txn.store('users'), txn); - } - ); + return db.runTransaction('readwrite', ['users'], txn => { + return fn(txn.store('users'), txn); + }); } function writeTestData(): Promise { @@ -486,41 +481,30 @@ describe('SimpleDb', () => { ['foo', 'd'], ['foob'] ]; - await db.runTransaction( - 'readwrite', - /* idempotent= */ false, - ['users', 'docs'], - txn => { - const docsStore = txn.store('docs'); - return PersistencePromise.waitFor( - keys.map(key => { - const value = 'doc ' + key.join('/'); - return docsStore.put(key, value); - }) - ); - } - ); + await db.runTransaction('readwrite', ['users', 'docs'], txn => { + const docsStore = txn.store('docs'); + return PersistencePromise.waitFor( + keys.map(key => { + const value = 'doc ' + key.join('/'); + return docsStore.put(key, value); + }) + ); + }); - await db.runTransaction( - 'readonly', - /* idempotent= */ false, - ['docs'], - txn => { - const store = txn.store('docs'); - const range = IDBKeyRange.bound(['foo'], ['foo', 'c']); - return store.loadAll(range).next(results => { - expect(results).to.deep.equal(['doc foo', 'doc foo/bar/baz']); - }); - } - ); + await db.runTransaction('readonly', ['docs'], txn => { + const store = txn.store('docs'); + const range = IDBKeyRange.bound(['foo'], ['foo', 'c']); + return store.loadAll(range).next(results => { + expect(results).to.deep.equal(['doc foo', 'doc foo/bar/baz']); + }); + }); }); it('retries idempotent transactions', async () => { let attemptCount = 0; const result = await db.runTransaction( - 'readwrite', - /* idempotent= */ true, + 'readwrite-idempotent', ['users'], txn => { ++attemptCount; @@ -529,11 +513,10 @@ describe('SimpleDb', () => { return store .add(dummyUser) .next(() => { - return store.add(dummyUser); + return store.add(dummyUser); // Fails with a unique key violation }) - .next(() => 'Uncaught unique key violation'); + .next(() => 'Aborted'); } else { - console.log('retry'); return PersistencePromise.resolve('success'); } } @@ -543,19 +526,19 @@ describe('SimpleDb', () => { expect(attemptCount).to.equal(2); }); - it('retries transaction only three times', async () => { + it('retries transactions only three times', async () => { let attemptCount = 0; await expect( - db.runTransaction('readwrite', /* idempotent= */ true, ['users'], txn => { + db.runTransaction('readwrite-idempotent', ['users'], txn => { ++attemptCount; const store = txn.store('users'); return store .add(dummyUser) .next(() => { - return store.add(dummyUser); + return store.add(dummyUser); // Fails with a unique key violation }) - .next(() => 'Uncaught unique key violation'); + .next(() => 'Aborted'); }) ).to.eventually.be.rejected; @@ -566,7 +549,7 @@ describe('SimpleDb', () => { let attemptCount = 0; await expect( - db.runTransaction('readwrite', /* idempotent= */ true, ['users'], txn => { + db.runTransaction('readwrite-idempotent', ['users'], txn => { ++attemptCount; txn.abort(); return PersistencePromise.reject(new Error('Aborted')); @@ -580,22 +563,17 @@ describe('SimpleDb', () => { let attemptCount = 0; await expect( - db.runTransaction( - 'readwrite', - /* idempotent= */ false, - ['users'], - txn => { - ++attemptCount; - const store = txn.store('users'); - return store - .add(dummyUser) - .next(() => { - return store.add(dummyUser); - }) - .next(() => 'Uncaught unique key violation'); - } - ) - ).to.eventually.be.rejectedWith('Uncaught unique key violation'); + db.runTransaction('readwrite', ['users'], txn => { + ++attemptCount; + const store = txn.store('users'); + return store + .add(dummyUser) + .next(() => { + return store.add(dummyUser); // Fails with a unique key violation + }) + .next(() => 'Aborted'); + }) + ).to.eventually.be.rejected; expect(attemptCount).to.equal(1); }); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 00f418208e0..1b195e94b8d 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -1650,16 +1650,11 @@ async function clearCurrentPrimaryLease(): Promise { SCHEMA_VERSION, new SchemaConverter(TEST_SERIALIZER) ); - await db.runTransaction( - 'readwrite', - /* idempotent= */ false, - [DbPrimaryClient.store], - txn => { - const primaryClientStore = txn.store( - DbPrimaryClient.store - ); - return primaryClientStore.delete(DbPrimaryClient.key); - } - ); + await db.runTransaction('readwrite', [DbPrimaryClient.store], txn => { + const primaryClientStore = txn.store( + DbPrimaryClient.store + ); + return primaryClientStore.delete(DbPrimaryClient.key); + }); db.close(); } From aee20aa67718ff7a2753029729b0be7a7c4e05ca Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 9 Oct 2019 15:49:31 -0700 Subject: [PATCH 6/7] Lint --- packages/firestore/src/local/simple_db.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/local/simple_db.ts b/packages/firestore/src/local/simple_db.ts index 3e88cae6245..5899dffade6 100644 --- a/packages/firestore/src/local/simple_db.ts +++ b/packages/firestore/src/local/simple_db.ts @@ -260,8 +260,8 @@ export class SimpleDb { objectStores: string[], transactionFn: (transaction: SimpleDbTransaction) => PersistencePromise ): Promise { - let readonly = mode.startsWith('idempotent'); - let idempotent = mode.endsWith('idempotent'); + const readonly = mode.startsWith('readonly'); + const idempotent = mode.endsWith('idempotent'); let attemptNumber = 0; while (true) { From bde88fbb7c63079f36d33ce5537bf73b6cc7da32 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 10 Oct 2019 11:43:38 -0700 Subject: [PATCH 7/7] Update test name --- 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 11ed5d19451..53d7421d36f 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -500,7 +500,7 @@ describe('SimpleDb', () => { }); }); - it('retries idempotent transactions', async () => { + it('retries transactions marked as idempotent', async () => { let attemptCount = 0; const result = await db.runTransaction(