fix: Better error message when Transaction/WriteBatch is modified aft…

…er commit. (#1503) * fix: Better error message when Transaction/WriteBatch is modified after commit. * Address feedback. * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
googleapis · Dec 15, 2023 · 9693c7b · 9693c7b
1 parent 7557d7c
commit 9693c7b
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 1 deletion.
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java
@@ -161,7 +161,10 @@ private T performCreate(
 
   private void verifyNotCommitted() {
     Preconditions.checkState(
-        !isCommitted(), "Cannot modify a WriteBatch that has already been committed.");
+        !isCommitted(),
+        String.format(
+            "Cannot modify a %s that has already been committed.",
+            this.getClass().getSimpleName()));
   }
 
   /**

diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java
@@ -1242,4 +1242,20 @@ public static List<String> bundleToElementList(ByteBuffer bundle) {
 
     return result;
   }
+
+  @FunctionalInterface
+  interface VoidFunction {
+    void apply();
+  }
+
+  static void assertException(VoidFunction voidFunction, String expectedErrorMessage) {
+    String errorMessage = "";
+    try {
+      voidFunction.apply();
+    } catch (Exception e) {
+      errorMessage = e.getMessage();
+    } finally {
+      assertEquals(errorMessage, expectedErrorMessage);
+    }
+  }
 }
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java
@@ -36,6 +36,7 @@
 import static com.google.cloud.firestore.LocalFirestoreHelper.rollbackResponse;
 import static com.google.cloud.firestore.LocalFirestoreHelper.set;
 import static com.google.cloud.firestore.LocalFirestoreHelper.update;
+import static com.google.cloud.firestore.it.ITQueryTest.map;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
@@ -959,6 +960,43 @@ public void getShouldThrowWhenInvokedAfterAWriteWithAnAggregateQuery() throws Ex
     assertThat(executionException.getCause()).isInstanceOf(IllegalStateException.class);
   }
 
+  @Test
+  public void givesProperErrorMessageForCommittedTransaction() throws Exception {
+    doReturn(beginResponse())
+        .doReturn(commitResponse(0, 0))
+        .when(firestoreMock)
+        .sendRequest(
+            requestCapture.capture(), ArgumentMatchers.<UnaryCallable<Message, Message>>any());
+    String expectedErrorMessage = "Cannot modify a Transaction that has already been committed.";
+
+    DocumentReference docRef = firestoreMock.collection("foo").document("bar");
+
+    // Commit a transaction.
+    Transaction t = firestoreMock.runTransaction(transaction -> transaction).get();
+
+    // Then run other operations in the same transaction.
+    LocalFirestoreHelper.assertException(
+        () -> {
+          t.set(docRef, map("foo", "bar"));
+        },
+        expectedErrorMessage);
+    LocalFirestoreHelper.assertException(
+        () -> {
+          t.update(docRef, map("foo", "bar"));
+        },
+        expectedErrorMessage);
+    LocalFirestoreHelper.assertException(
+        () -> {
+          t.create(docRef, map("foo", "bar"));
+        },
+        expectedErrorMessage);
+    LocalFirestoreHelper.assertException(
+        () -> {
+          t.delete(docRef);
+        },
+        expectedErrorMessage);
+  }
+
   private ApiException exception(Status.Code code, boolean shouldRetry) {
     return exception(code, "Test exception", shouldRetry);
   }

diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/WriteBatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/WriteBatchTest.java
@@ -341,4 +341,43 @@ public void deleteDocument() throws Exception {
     CommitRequest commitRequest = commitCapture.getValue();
     assertEquals(commit(writes.toArray(new Write[] {})), commitRequest);
   }
+
+  @Test
+  public void throwsWhenModifyingACommittedWriteBatch() throws Exception {
+    doReturn(commitResponse(0, 0))
+        .when(firestoreMock)
+        .sendRequest(
+            commitCapture.capture(),
+            ArgumentMatchers.<UnaryCallable<CommitRequest, CommitResponse>>any());
+
+    String expectedErrorMessage = "Cannot modify a WriteBatch that has already been committed.";
+
+    DocumentReference docRef = firestoreMock.collection("foo").document("bar");
+
+    // Commit a batch.
+    WriteBatch batch = firestoreMock.batch();
+    batch.commit().get();
+
+    // Then run other operations in the same batch.
+    LocalFirestoreHelper.assertException(
+        () -> {
+          batch.set(docRef, map("foo", "bar"));
+        },
+        expectedErrorMessage);
+    LocalFirestoreHelper.assertException(
+        () -> {
+          batch.update(docRef, map("foo", "bar"));
+        },
+        expectedErrorMessage);
+    LocalFirestoreHelper.assertException(
+        () -> {
+          batch.create(docRef, map("foo", "bar"));
+        },
+        expectedErrorMessage);
+    LocalFirestoreHelper.assertException(
+        () -> {
+          batch.delete(docRef);
+        },
+        expectedErrorMessage);
+  }
 }