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) diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts index d604ee059a5..be83305993b 100644 --- a/packages/firestore/src/core/transaction.ts +++ b/packages/firestore/src/core/transaction.ts @@ -120,7 +120,7 @@ export class Transaction { if (doc.isFoundDocument()) { docVersion = doc.version; } else if (doc.isNoDocument()) { - // For deleted docs, we must use baseVersion 0 when we overwrite them. + // Represent a deleted doc using SnapshotVersion.min(). docVersion = SnapshotVersion.min(); } else { throw fail('Document in a transaction was a ' + doc.constructor.name); @@ -147,7 +147,11 @@ 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 (version.isEqual(SnapshotVersion.min())) { + return Precondition.exists(false); + } else { + return Precondition.updateTime(version); + } } else { return Precondition.none(); } diff --git a/packages/firestore/test/integration/api/transactions.test.ts b/packages/firestore/test/integration/api/transactions.test.ts index f0c2128f289..3c26f085042 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,19 @@ 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; + default: + throw new Error(`invalid fromDocumentType: ${this.fromDocumentType}`); } } @@ -289,6 +316,47 @@ 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); + + 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); diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index deaa9a0236f..f853602dcec 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -524,6 +524,24 @@ 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); + 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;