Firestore: Fix FAILED_PRECONDITION when writing to a deleted document in a transaction (#6550)

Author: dconeybe

.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)

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();
     }

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<void> {
       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);

packages/firestore/test/lite/integration.test.ts

@@ -581,6 +581,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;