From 06d859e4b9e91a9e51c0b1214fce1288b60bb4d7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 14:31:34 -0400 Subject: [PATCH 1/8] Add a test case to set a deleted doc --- .../test/integration/api/transactions.test.ts | 71 +++++++++++++++++-- 1 file changed, 66 insertions(+), 5 deletions(-) diff --git a/packages/firestore/test/integration/api/transactions.test.ts b/packages/firestore/test/integration/api/transactions.test.ts index f0c2128f289..1941c506f1c 100644 --- a/packages/firestore/test/integration/api/transactions.test.ts +++ b/packages/firestore/test/integration/api/transactions.test.ts @@ -23,6 +23,7 @@ import { QueryDocumentSnapshot, Transaction, collection, + deleteDoc, doc, DocumentReference, DocumentSnapshot, @@ -87,6 +88,16 @@ apiDescribe('Database transactions', (persistence: boolean) => { await transaction.get(docRef); } + enum FromDocumentType { + // The operation will be performed on a document that exists. + EXISTING = 'existing', + // The operation will be performed on a document that has never existed. + NON_EXISTENT = 'non_existent', + // The operation will be performed on a document that existed, but was + // deleted. + DELETED = 'deleted' + } + /** * Used for testing that all possible combinations of executing transactions * result in the desired document value or error. @@ -101,16 +112,21 @@ apiDescribe('Database transactions', (persistence: boolean) => { constructor(readonly db: Firestore) {} private docRef!: DocumentReference; - private fromExistingDoc: boolean = false; + private fromDocumentType: FromDocumentType = FromDocumentType.NON_EXISTENT; private stages: TransactionStage[] = []; withExistingDoc(): this { - this.fromExistingDoc = true; + this.fromDocumentType = FromDocumentType.EXISTING; return this; } withNonexistentDoc(): this { - this.fromExistingDoc = false; + this.fromDocumentType = FromDocumentType.NON_EXISTENT; + return this; + } + + withDeletedDoc(): this { + this.fromDocumentType = FromDocumentType.DELETED; return this; } @@ -176,8 +192,17 @@ apiDescribe('Database transactions', (persistence: boolean) => { private async prepareDoc(): Promise { this.docRef = doc(collection(this.db, 'tester-docref')); - if (this.fromExistingDoc) { - await setDoc(this.docRef, { foo: 'bar0' }); + switch (this.fromDocumentType) { + case FromDocumentType.EXISTING: + await setDoc(this.docRef, { foo: 'bar0' }); + break; + case FromDocumentType.NON_EXISTENT: + // Nothing to do; document does not exist. + break; + case FromDocumentType.DELETED: + await setDoc(this.docRef, { foo: 'bar0' }); + await deleteDoc(this.docRef); + break; } } @@ -289,6 +314,42 @@ apiDescribe('Database transactions', (persistence: boolean) => { }); }); + it('runs transactions after getting a deleted document', async () => { + return withTestDb(persistence, async db => { + const tt = new TransactionTester(db); + + await tt.withDeletedDoc().run(get, delete1, delete1).expectNoDoc(); + await tt + .withDeletedDoc() + .run(get, delete1, update2) + .expectError('invalid-argument'); + await tt + .withDeletedDoc() + .run(get, delete1, set2) + .expectDoc({ foo: 'bar2' }); + + await tt + .withDeletedDoc() + .run(get, update1, delete1) + .expectError('invalid-argument'); + await tt + .withDeletedDoc() + .run(get, update1, update2) + .expectError('invalid-argument'); + await tt + .withDeletedDoc() + .run(get, update1, set1) + .expectError('invalid-argument'); + + await tt.withDeletedDoc().run(get, set1, delete1).expectNoDoc(); + await tt + .withDeletedDoc() + .run(get, set1, update2) + .expectDoc({ foo: 'bar2' }); + await tt.withDeletedDoc().run(get, set1, set2).expectDoc({ foo: 'bar2' }); + }); + }); + it('runs transactions on existing document', async () => { return withTestDb(persistence, async db => { const tt = new TransactionTester(db); From dc583be7bd9e75f217fe5cf06449a00230ecfd88 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 15:04:29 -0400 Subject: [PATCH 2/8] transaction.ts: fix precondition for deleted documents --- packages/firestore/src/core/transaction.ts | 28 ++++++++++++++-------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts index d604ee059a5..beb7fb9d398 100644 --- a/packages/firestore/src/core/transaction.ts +++ b/packages/firestore/src/core/transaction.ts @@ -40,7 +40,7 @@ import { SnapshotVersion } from './snapshot_version'; */ export class Transaction { // The version of each document that was read during this transaction. - private readVersions = new Map(); + private readVersions = new Map(); private mutations: Mutation[] = []; private committed = false; @@ -115,20 +115,24 @@ export class Transaction { } private recordVersion(doc: Document): void { - let docVersion: SnapshotVersion; + let docVersion: SnapshotVersion | null; if (doc.isFoundDocument()) { docVersion = doc.version; } else if (doc.isNoDocument()) { - // For deleted docs, we must use baseVersion 0 when we overwrite them. - docVersion = SnapshotVersion.min(); + // For deleted docs, we must use {exists: false} when we overwrite them. + docVersion = null; } else { throw fail('Document in a transaction was a ' + doc.constructor.name); } const existingVersion = this.readVersions.get(doc.key.toString()); - if (existingVersion) { - if (!docVersion.isEqual(existingVersion)) { + if (existingVersion !== undefined && existingVersion !== docVersion) { + if ( + docVersion == null || + existingVersion == null || + !docVersion.isEqual(existingVersion) + ) { // This transaction will fail no matter what. throw new FirestoreError( Code.ABORTED, @@ -146,8 +150,12 @@ export class Transaction { */ private precondition(key: DocumentKey): Precondition { const version = this.readVersions.get(key.toString()); - if (!this.writtenDocs.has(key.toString()) && version) { - return Precondition.updateTime(version); + if (!this.writtenDocs.has(key.toString()) && version !== undefined) { + if (version == null) { + return Precondition.exists(false); + } else { + return Precondition.updateTime(version); + } } else { return Precondition.none(); } @@ -160,8 +168,8 @@ export class Transaction { const version = this.readVersions.get(key.toString()); // The first time a document is written, we want to take into account the // read time and existence - if (!this.writtenDocs.has(key.toString()) && version) { - if (version.isEqual(SnapshotVersion.min())) { + if (!this.writtenDocs.has(key.toString()) && version !== undefined) { + if (version === null) { // The document doesn't exist, so fail the transaction. // This has to be validated locally because you can't send a From c8df3be9787c409669b5b86eb9d44913e48521ae Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 15:21:28 -0400 Subject: [PATCH 3/8] add test to the lite sdk test suite --- packages/firestore/test/lite/integration.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index deaa9a0236f..a2a8655cd16 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -524,6 +524,19 @@ describe('Transaction', () => { }); }); + it('can read deleted doc then write', () => { + return withTestDocAndInitialData({ counter: 1 }, async doc => { + await deleteDoc(doc); + await runTransaction(doc.firestore, async transaction => { + const snap = await transaction.get(doc); + expect(snap.exists()).to.be.false; + transaction.set(doc, { counter: 1 }); + }); + const result = await getDoc(doc); + expect(result.get('counter')).to.equal(1); + }); + }); + it('retries when document is modified', () => { return withTestDoc(async doc => { let retryCounter = 0; From 1fe7fde37de2eb7c51452c6d761f0e5aaf0f1034 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 15:35:22 -0400 Subject: [PATCH 4/8] add changeset --- .changeset/smart-plants-sparkle.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/smart-plants-sparkle.md diff --git a/.changeset/smart-plants-sparkle.md b/.changeset/smart-plants-sparkle.md new file mode 100644 index 00000000000..135c6110cc1 --- /dev/null +++ b/.changeset/smart-plants-sparkle.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Fix FAILED_PRECONDITION when writing to a deleted document in a transaction (#5871) From 9a21ad8e680d75a0998e1c7f8dd12895e09a4209 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 15:45:01 -0400 Subject: [PATCH 5/8] transaction.ts: refactor snapshot version comparison logic --- packages/firestore/src/core/transaction.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts index beb7fb9d398..9bba5d3f74e 100644 --- a/packages/firestore/src/core/transaction.ts +++ b/packages/firestore/src/core/transaction.ts @@ -34,6 +34,19 @@ import { Code, FirestoreError } from '../util/error'; import { SnapshotVersion } from './snapshot_version'; +function nullableSnapshotVersionsEqual( + v1: SnapshotVersion | null, + v2: SnapshotVersion | null +): boolean { + if (v1 === v2) { + return true; + } else if (v1 === null || v2 === null) { + return false; + } else { + return v1.isEqual(v2); + } +} + /** * Internal transaction object responsible for accumulating the mutations to * perform and the base versions for any documents read. @@ -127,12 +140,8 @@ export class Transaction { } const existingVersion = this.readVersions.get(doc.key.toString()); - if (existingVersion !== undefined && existingVersion !== docVersion) { - if ( - docVersion == null || - existingVersion == null || - !docVersion.isEqual(existingVersion) - ) { + if (existingVersion !== undefined) { + if (!nullableSnapshotVersionsEqual(existingVersion, docVersion)) { // This transaction will fail no matter what. throw new FirestoreError( Code.ABORTED, From f76ffb7c9e6fb16a95be3554d025c23111c52636 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 15:53:23 -0400 Subject: [PATCH 6/8] transactions.test.ts: add default branch to switch statement to make lint happy --- packages/firestore/test/integration/api/transactions.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/firestore/test/integration/api/transactions.test.ts b/packages/firestore/test/integration/api/transactions.test.ts index 1941c506f1c..b1789fa5d5a 100644 --- a/packages/firestore/test/integration/api/transactions.test.ts +++ b/packages/firestore/test/integration/api/transactions.test.ts @@ -203,6 +203,8 @@ apiDescribe('Database transactions', (persistence: boolean) => { await setDoc(this.docRef, { foo: 'bar0' }); await deleteDoc(this.docRef); break; + default: + throw new Error(`invalid fromDocumentType: ${this.fromDocumentType}`); } } From 2847792bb01a9016d6650179a13ad0d2cd96a836 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 21:51:36 -0400 Subject: [PATCH 7/8] transaction.ts: simplify the logic by going back to using SnapshotVersion.min() to represent a deleted document rather than null --- packages/firestore/src/core/transaction.ts | 33 +++++++--------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts index 9bba5d3f74e..be83305993b 100644 --- a/packages/firestore/src/core/transaction.ts +++ b/packages/firestore/src/core/transaction.ts @@ -34,26 +34,13 @@ import { Code, FirestoreError } from '../util/error'; import { SnapshotVersion } from './snapshot_version'; -function nullableSnapshotVersionsEqual( - v1: SnapshotVersion | null, - v2: SnapshotVersion | null -): boolean { - if (v1 === v2) { - return true; - } else if (v1 === null || v2 === null) { - return false; - } else { - return v1.isEqual(v2); - } -} - /** * Internal transaction object responsible for accumulating the mutations to * perform and the base versions for any documents read. */ export class Transaction { // The version of each document that was read during this transaction. - private readVersions = new Map(); + private readVersions = new Map(); private mutations: Mutation[] = []; private committed = false; @@ -128,20 +115,20 @@ export class Transaction { } private recordVersion(doc: Document): void { - let docVersion: SnapshotVersion | null; + let docVersion: SnapshotVersion; if (doc.isFoundDocument()) { docVersion = doc.version; } else if (doc.isNoDocument()) { - // For deleted docs, we must use {exists: false} when we overwrite them. - docVersion = null; + // Represent a deleted doc using SnapshotVersion.min(). + docVersion = SnapshotVersion.min(); } else { throw fail('Document in a transaction was a ' + doc.constructor.name); } const existingVersion = this.readVersions.get(doc.key.toString()); - if (existingVersion !== undefined) { - if (!nullableSnapshotVersionsEqual(existingVersion, docVersion)) { + if (existingVersion) { + if (!docVersion.isEqual(existingVersion)) { // This transaction will fail no matter what. throw new FirestoreError( Code.ABORTED, @@ -159,8 +146,8 @@ export class Transaction { */ private precondition(key: DocumentKey): Precondition { const version = this.readVersions.get(key.toString()); - if (!this.writtenDocs.has(key.toString()) && version !== undefined) { - if (version == null) { + if (!this.writtenDocs.has(key.toString()) && version) { + if (version.isEqual(SnapshotVersion.min())) { return Precondition.exists(false); } else { return Precondition.updateTime(version); @@ -177,8 +164,8 @@ export class Transaction { const version = this.readVersions.get(key.toString()); // The first time a document is written, we want to take into account the // read time and existence - if (!this.writtenDocs.has(key.toString()) && version !== undefined) { - if (version === null) { + if (!this.writtenDocs.has(key.toString()) && version) { + if (version.isEqual(SnapshotVersion.min())) { // The document doesn't exist, so fail the transaction. // This has to be validated locally because you can't send a From 2b9f7d35459164272ff5f498819802646b61269d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Aug 2022 21:52:41 -0400 Subject: [PATCH 8/8] add comments to the tests --- packages/firestore/test/integration/api/transactions.test.ts | 5 +++++ packages/firestore/test/lite/integration.test.ts | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/packages/firestore/test/integration/api/transactions.test.ts b/packages/firestore/test/integration/api/transactions.test.ts index b1789fa5d5a..3c26f085042 100644 --- a/packages/firestore/test/integration/api/transactions.test.ts +++ b/packages/firestore/test/integration/api/transactions.test.ts @@ -316,6 +316,11 @@ apiDescribe('Database transactions', (persistence: boolean) => { }); }); + // This test is identical to the test above, except that withNonexistentDoc() + // is replaced by withDeletedDoc(), to guard against regression of + // https://github.com/firebase/firebase-js-sdk/issues/5871, where transactions + // would incorrectly fail with FAILED_PRECONDITION when operations were + // performed on a deleted document (rather than a non-existent document). it('runs transactions after getting a deleted document', async () => { return withTestDb(persistence, async db => { const tt = new TransactionTester(db); diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index a2a8655cd16..f853602dcec 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -524,6 +524,11 @@ describe('Transaction', () => { }); }); + // This test is identical to the test above, except that a non-existent + // document is replaced by a deleted document, to guard against regression of + // https://github.com/firebase/firebase-js-sdk/issues/5871, where transactions + // would incorrectly fail with FAILED_PRECONDITION when operations were + // performed on a deleted document (rather than a non-existent document). it('can read deleted doc then write', () => { return withTestDocAndInitialData({ counter: 1 }, async doc => { await deleteDoc(doc);