Skip to content
This repository was archived by the owner on May 8, 2026. It is now read-only.

Commit 71eab7b

Browse files
authored
fix: enforce read-before-write in transaction execute() (#2358)
Ensures that Pipeline.execute() calls within a transaction follow the requirement that all reads must occur before any writes. Previously, this check was missing from the execute() methods in ServerSideTransaction.
1 parent 6acac4b commit 71eab7b

2 files changed

Lines changed: 23 additions & 0 deletions

File tree

google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,13 +271,15 @@ public ApiFuture<AggregateQuerySnapshot> get(@Nonnull AggregateQuery query) {
271271
@Nonnull
272272
@Override
273273
public ApiFuture<Pipeline.Snapshot> execute(@Nonnull Pipeline pipeline) {
274+
Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG);
274275
return execute(pipeline, new PipelineExecuteOptions());
275276
}
276277

277278
@Nonnull
278279
@Override
279280
public ApiFuture<Pipeline.Snapshot> execute(
280281
@Nonnull Pipeline pipeline, @Nonnull PipelineExecuteOptions options) {
282+
Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG);
281283
try (TraceUtil.Scope ignored = transactionTraceContext.makeCurrent()) {
282284
return pipeline.execute(new PipelineExecuteOptions(), transactionId, null);
283285
}

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -960,6 +960,27 @@ public void getShouldThrowWhenInvokedAfterAWriteWithAnAggregateQuery() throws Ex
960960
assertThat(executionException.getCause()).isInstanceOf(IllegalStateException.class);
961961
}
962962

963+
@Test
964+
public void executeShouldThrowWhenInvokedAfterAWrite() throws Exception {
965+
doReturn(beginResponse())
966+
.when(firestoreMock)
967+
.sendRequest(ArgumentMatchers.any(), ArgumentMatchers.any());
968+
969+
ApiFuture<Void> transaction =
970+
firestoreMock.runTransaction(
971+
t -> {
972+
t.set(documentReference, LocalFirestoreHelper.SINGLE_FIELD_MAP);
973+
t.execute(firestoreMock.pipeline().collection("test"));
974+
return null;
975+
});
976+
977+
ExecutionException executionException =
978+
assertThrows(ExecutionException.class, transaction::get);
979+
assertThat(executionException.getCause()).isInstanceOf(IllegalStateException.class);
980+
assertThat(executionException.getCause().getMessage())
981+
.contains("Firestore transactions require all reads to be executed before all writes");
982+
}
983+
963984
@Test
964985
public void givesProperErrorMessageForCommittedTransaction() throws Exception {
965986
doReturn(beginResponse())

0 commit comments

Comments
 (0)