From 95aabb3b0399f1216d20196f358e99e5b816c8ba Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 18:25:30 -0500 Subject: [PATCH 01/10] Optimize ReadOnly transactions. --- .../cloud/firestore/AggregateQuery.java | 39 +-- .../google/cloud/firestore/FirestoreImpl.java | 26 +- .../com/google/cloud/firestore/Query.java | 6 +- .../cloud/firestore/ReadOnlyTransaction.java | 199 +++++++++++++++ .../cloud/firestore/ReadWriteTransaction.java | 202 ++++++++++++++++ .../google/cloud/firestore/Transaction.java | 226 ++++++------------ .../cloud/firestore/TransactionRunner.java | 10 +- .../cloud/firestore/TransactionTest.java | 2 +- 8 files changed, 518 insertions(+), 192 deletions(-) create mode 100644 google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java create mode 100644 google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java index 1613b74dd..921300b47 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java @@ -72,20 +72,22 @@ public Query getQuery() { */ @Nonnull public ApiFuture get() { - return get(null); + return get(null, null); } @Nonnull - ApiFuture get(@Nullable final ByteString transactionId) { + ApiFuture get( + @Nullable final ByteString transactionId, @Nullable com.google.protobuf.Timestamp readTime) { AggregateQueryResponseDeliverer responseDeliverer = new AggregateQueryResponseDeliverer( - transactionId, /* startTimeNanos= */ query.rpcContext.getClock().nanoTime()); + transactionId, readTime, /* startTimeNanos= */ query.rpcContext.getClock().nanoTime()); runQuery(responseDeliverer); return responseDeliverer.getFuture(); } private void runQuery(AggregateQueryResponseDeliverer responseDeliverer) { - RunAggregationQueryRequest request = toProto(responseDeliverer.getTransactionId()); + RunAggregationQueryRequest request = + toProto(responseDeliverer.transactionId, responseDeliverer.readTime); AggregateQueryResponseObserver responseObserver = new AggregateQueryResponseObserver(responseDeliverer); ServerStreamingCallable callable = @@ -96,12 +98,17 @@ private void runQuery(AggregateQueryResponseDeliverer responseDeliverer) { private final class AggregateQueryResponseDeliverer { @Nullable private final ByteString transactionId; + @Nullable private final com.google.protobuf.Timestamp readTime; private final long startTimeNanos; private final SettableApiFuture future = SettableApiFuture.create(); private final AtomicBoolean isFutureCompleted = new AtomicBoolean(false); - AggregateQueryResponseDeliverer(@Nullable ByteString transactionId, long startTimeNanos) { + AggregateQueryResponseDeliverer( + @Nullable ByteString transactionId, + @Nullable com.google.protobuf.Timestamp readTime, + long startTimeNanos) { this.transactionId = transactionId; + this.readTime = readTime; this.startTimeNanos = startTimeNanos; } @@ -109,15 +116,6 @@ ApiFuture getFuture() { return future; } - @Nullable - ByteString getTransactionId() { - return transactionId; - } - - long getStartTimeNanos() { - return startTimeNanos; - } - void deliverResult(@Nonnull Map data, Timestamp readTime) { if (isFutureCompleted.compareAndSet(false, true)) { Map mappedData = new HashMap<>(); @@ -176,8 +174,8 @@ private boolean shouldRetry(Throwable throwable) { FirestoreSettings.newBuilder().runAggregationQuerySettings().getRetryableCodes(); return query.shouldRetryQuery( throwable, - responseDeliverer.getTransactionId(), - responseDeliverer.getStartTimeNanos(), + responseDeliverer.transactionId, + responseDeliverer.startTimeNanos, retryableCodes); } @@ -193,11 +191,13 @@ public void onComplete() {} */ @Nonnull public RunAggregationQueryRequest toProto() { - return toProto(null); + return toProto(null, null); } @Nonnull - RunAggregationQueryRequest toProto(@Nullable final ByteString transactionId) { + RunAggregationQueryRequest toProto( + @Nullable final ByteString transactionId, + @Nullable final com.google.protobuf.Timestamp readTime) { RunQueryRequest runQueryRequest = query.toProto(); RunAggregationQueryRequest.Builder request = RunAggregationQueryRequest.newBuilder(); @@ -205,6 +205,9 @@ RunAggregationQueryRequest toProto(@Nullable final ByteString transactionId) { if (transactionId != null) { request.setTransaction(transactionId); } + if (readTime != null) { + request.setReadTime(readTime); + } StructuredAggregationQuery.Builder structuredAggregationQuery = request.getStructuredAggregationQueryBuilder(); diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index 1fab69e97..b8718723d 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -194,14 +194,14 @@ public Iterable listCollections() { @Override public ApiFuture> getAll( @Nonnull DocumentReference... documentReferences) { - return this.getAll(documentReferences, null, (ByteString) null); + return this.getAll(documentReferences, null, (ByteString) null, null); } @Nonnull @Override public ApiFuture> getAll( @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { - return this.getAll(documentReferences, fieldMask, (ByteString) null); + return this.getAll(documentReferences, fieldMask, (ByteString) null, null); } @Override @@ -209,13 +209,14 @@ public void getAll( final @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask, @Nonnull final ApiStreamObserver apiStreamObserver) { - this.getAll(documentReferences, fieldMask, null, apiStreamObserver); + this.getAll(documentReferences, fieldMask, null, null, apiStreamObserver); } void getAll( final @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask, @Nullable ByteString transactionId, + @Nullable com.google.protobuf.Timestamp readTime, final ApiStreamObserver apiStreamObserver) { ResponseObserver responseObserver = @@ -304,6 +305,10 @@ public void onComplete() { request.setTransaction(transactionId); } + if (readTime != null) { + request.setReadTime(readTime); + } + for (DocumentReference docRef : documentReferences) { request.addDocuments(docRef.getName()); } @@ -322,13 +327,15 @@ public void onComplete() { ApiFuture> getAll( final @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask, - @Nullable ByteString transactionId) { + @Nullable ByteString transactionId, + @Nullable com.google.protobuf.Timestamp readTime) { final SettableApiFuture> futureList = SettableApiFuture.create(); final Map documentSnapshotMap = new HashMap<>(); getAll( documentReferences, fieldMask, transactionId, + readTime, new ApiStreamObserver() { @Override public void onNext(DocumentSnapshot documentSnapshot) { @@ -390,9 +397,14 @@ public ApiFuture runAsyncTransaction( @Nonnull final Transaction.AsyncFunction updateFunction, @Nonnull TransactionOptions transactionOptions) { - TransactionRunner transactionRunner = - new TransactionRunner<>(this, updateFunction, transactionOptions); - return transactionRunner.run(); + switch (transactionOptions.getType()) { + case READ_ONLY: + return updateFunction.updateCallback( + new ReadOnlyTransaction(this, transactionOptions.getReadTime())); + case READ_WRITE: + default: + return new TransactionRunner<>(this, updateFunction, transactionOptions).run(); + } } @Nonnull diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java index 59ec6f960..865e522c9 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java @@ -1784,7 +1784,7 @@ boolean shouldRetry(DocumentSnapshot lastDocument, Throwable t) { */ @Nonnull public ApiFuture get() { - return get(null); + return get(null, null); } /** @@ -1811,7 +1811,7 @@ public ListenerRegistration addSnapshotListener( return Watch.forQuery(this).runWatch(executor, listener); } - ApiFuture get(@Nullable ByteString transactionId) { + ApiFuture get(@Nullable ByteString transactionId, @Nullable Timestamp readTime) { final SettableApiFuture result = SettableApiFuture.create(); internalStream( @@ -1843,7 +1843,7 @@ public void onCompleted() { }, /* startTimeNanos= */ rpcContext.getClock().nanoTime(), transactionId, - /* readTime= */ null); + readTime); return result; } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java new file mode 100644 index 000000000..723d0f4f4 --- /dev/null +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java @@ -0,0 +1,199 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.firestore; + +import com.google.api.core.ApiFuture; +import com.google.api.core.ApiFutures; +import com.google.common.util.concurrent.MoreExecutors; +import com.google.protobuf.ByteString; +import com.google.protobuf.Timestamp; +import io.opencensus.trace.Tracing; +import java.util.List; +import java.util.Map; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +final class ReadOnlyTransaction implements Transaction { + + public static final String WRITE_EXCEPTION_MSG = "Firestore transactions do not support writes"; + private final FirestoreImpl firestore; + private final Timestamp readTime; + + ReadOnlyTransaction(FirestoreImpl firestore, Timestamp readTime) { + this.firestore = firestore; + this.readTime = readTime; + } + + @Nonnull + @Override + public ApiFuture get(@Nonnull DocumentReference documentRef) { + Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_GETDOCUMENT); + return ApiFutures.transform( + firestore.getAll( + new DocumentReference[] {documentRef}, + /*fieldMask=*/ null, + /*transactionId=*/ (ByteString) null, + readTime), + snapshots -> snapshots.isEmpty() ? null : snapshots.get(0), + MoreExecutors.directExecutor()); + } + + @Nonnull + @Override + public ApiFuture> getAll( + @Nonnull DocumentReference... documentReferences) { + return firestore.getAll( + documentReferences, /*fieldMask=*/ null, /*transactionId=*/ (ByteString) null, readTime); + } + + @Nonnull + @Override + public ApiFuture> getAll( + @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { + return firestore.getAll( + documentReferences, /*fieldMask=*/ null, /*transactionId=*/ (ByteString) null, readTime); + } + + @Nonnull + @Override + public ApiFuture get(@Nonnull Query query) { + return query.get(null, com.google.cloud.Timestamp.fromProto(readTime)); + } + + @Nonnull + @Override + public ApiFuture get(@Nonnull AggregateQuery query) { + return query.get(null, readTime); + } + + @Nonnull + @Override + public Transaction create( + @Nonnull DocumentReference documentReference, @Nonnull Map fields) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction create( + @Nonnull DocumentReference documentReference, @Nonnull Object pojo) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction set( + @Nonnull DocumentReference documentReference, @Nonnull Map fields) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction set( + @Nonnull DocumentReference documentReference, + @Nonnull Map fields, + @Nonnull SetOptions options) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction set( + @Nonnull DocumentReference documentReference, @Nonnull Object pojo) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction set( + @Nonnull DocumentReference documentReference, + @Nonnull Object pojo, + @Nonnull SetOptions options) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction update( + @Nonnull DocumentReference documentReference, @Nonnull Map fields) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull Map fields, + Precondition precondition) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull String field, + @Nullable Object value, + Object... moreFieldsAndValues) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull FieldPath fieldPath, + @Nullable Object value, + Object... moreFieldsAndValues) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull Precondition precondition, + @Nonnull String field, + @Nullable Object value, + Object... moreFieldsAndValues) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull Precondition precondition, + @Nonnull FieldPath fieldPath, + @Nullable Object value, + Object... moreFieldsAndValues) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction delete( + @Nonnull DocumentReference documentReference, @Nonnull Precondition precondition) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } + + @Nonnull + @Override + public Transaction delete(@Nonnull DocumentReference documentReference) { + throw new IllegalStateException(WRITE_EXCEPTION_MSG); + } +} diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java new file mode 100644 index 000000000..75e72cc9c --- /dev/null +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java @@ -0,0 +1,202 @@ +/* + * Copyright 2017 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.firestore; + +import com.google.api.core.ApiFuture; +import com.google.api.core.ApiFutures; +import com.google.common.base.Preconditions; +import com.google.common.util.concurrent.MoreExecutors; +import com.google.firestore.v1.BeginTransactionRequest; +import com.google.firestore.v1.BeginTransactionResponse; +import com.google.firestore.v1.RollbackRequest; +import com.google.protobuf.ByteString; +import com.google.protobuf.Empty; +import io.opencensus.trace.Tracing; +import java.util.List; +import java.util.logging.Level; +import java.util.logging.Logger; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +/** + * A Transaction is passed to a Function to provide the methods to read and write data within the + * transaction context. + * + * @see Firestore#runTransaction(Function) + */ +public final class ReadWriteTransaction extends UpdateBuilder + implements Transaction { + + private static final Logger LOGGER = Logger.getLogger(ReadWriteTransaction.class.getName()); + private static final String READ_BEFORE_WRITE_ERROR_MSG = + "Firestore transactions require all reads to be executed before all writes"; + + private ByteString transactionId; + + ReadWriteTransaction( + FirestoreImpl firestore, @Nullable ReadWriteTransaction previousTransaction) { + super(firestore); + this.transactionId = previousTransaction != null ? previousTransaction.transactionId : null; + } + + public boolean hasTransactionId() { + return transactionId != null; + } + + @Override + ReadWriteTransaction wrapResult(int writeIndex) { + return this; + } + + /** Starts a transaction and obtains the transaction id. */ + ApiFuture begin() { + Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_BEGINTRANSACTION); + BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); + beginTransaction.setDatabase(firestore.getDatabaseName()); + + if (transactionId != null) { + beginTransaction.getOptionsBuilder().getReadWriteBuilder().setRetryTransaction(transactionId); + } + + ApiFuture transactionBeginFuture = + firestore.sendRequest( + beginTransaction.build(), firestore.getClient().beginTransactionCallable()); + + return ApiFutures.transform( + transactionBeginFuture, + beginTransactionResponse -> { + transactionId = beginTransactionResponse.getTransaction(); + return null; + }, + MoreExecutors.directExecutor()); + } + + /** Commits a transaction. */ + ApiFuture> commit() { + return super.commit(transactionId); + } + + /** Rolls a transaction back and releases all read locks. */ + ApiFuture rollback() { + Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_ROLLBACK); + RollbackRequest req = + RollbackRequest.newBuilder() + .setTransaction(transactionId) + .setDatabase(firestore.getDatabaseName()) + .build(); + + ApiFuture rollbackFuture = + firestore.sendRequest(req, firestore.getClient().rollbackCallable()); + + ApiFuture transform = + ApiFutures.transform(rollbackFuture, resp -> null, MoreExecutors.directExecutor()); + + return ApiFutures.catching( + transform, + Throwable.class, + (error) -> { + LOGGER.log( + Level.WARNING, + "Failed best effort to rollback of transaction " + transactionId, + error); + return null; + }, + MoreExecutors.directExecutor()); + } + + /** + * Reads the document referred to by the provided DocumentReference. Holds a pessimistic lock on + * the returned document. + * + * @return The contents of the Document at this DocumentReference. + */ + @Override + @Nonnull + public ApiFuture get(@Nonnull DocumentReference documentRef) { + Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_GETDOCUMENT); + return ApiFutures.transform( + firestore.getAll( + new DocumentReference[] {documentRef}, + /*fieldMask=*/ null, + transactionId, + /*readTime=*/ null), + snapshots -> snapshots.isEmpty() ? null : snapshots.get(0), + MoreExecutors.directExecutor()); + } + + /** + * Retrieves multiple documents from Firestore. Holds a pessimistic lock on all returned + * documents. + * + * @param documentReferences List of Document References to fetch. + */ + @Override + @Nonnull + public ApiFuture> getAll( + @Nonnull DocumentReference... documentReferences) { + Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + + return firestore.getAll( + documentReferences, /*fieldMask=*/ null, transactionId, /*readTime=*/ null); + } + + /** + * Retrieves multiple documents from Firestore, while optionally applying a field mask to reduce + * the amount of data transmitted from the backend. Holds a pessimistic lock on all returned + * documents. + * + * @param documentReferences Array with Document References to fetch. + * @param fieldMask If set, specifies the subset of fields to return. + */ + @Override + @Nonnull + public ApiFuture> getAll( + @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { + Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + + return firestore.getAll(documentReferences, fieldMask, transactionId, /*readTime=*/ null); + } + + /** + * Returns the result set from the provided query. Holds a pessimistic lock on all returned + * documents. + * + * @return The contents of the Document at this DocumentReference. + */ + @Override + @Nonnull + public ApiFuture get(@Nonnull Query query) { + Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + + return query.get(transactionId, /*readTime=*/ null); + } + + /** + * Returns the result from the provided aggregate query. Holds a pessimistic lock on all accessed + * documents. + * + * @return The result of the aggregation. + */ + @Override + @Nonnull + public ApiFuture get(@Nonnull AggregateQuery query) { + Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + + return query.get(transactionId, null); + } +} diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java index e76c2fdba..e5371b9e5 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java @@ -1,5 +1,5 @@ /* - * Copyright 2017 Google LLC + * Copyright 2024 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,34 +17,12 @@ package com.google.cloud.firestore; import com.google.api.core.ApiFuture; -import com.google.api.core.ApiFutures; -import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; -import com.google.common.base.Preconditions; -import com.google.common.util.concurrent.MoreExecutors; -import com.google.firestore.v1.BeginTransactionRequest; -import com.google.firestore.v1.BeginTransactionResponse; -import com.google.firestore.v1.RollbackRequest; -import com.google.firestore.v1.TransactionOptions.ReadOnly; -import com.google.protobuf.ByteString; -import com.google.protobuf.Empty; -import io.opencensus.trace.Tracing; import java.util.List; -import java.util.logging.Level; -import java.util.logging.Logger; +import java.util.Map; import javax.annotation.Nonnull; import javax.annotation.Nullable; -/** - * A Transaction is passed to a Function to provide the methods to read and write data within the - * transaction context. - * - * @see Firestore#runTransaction(Function) - */ -public final class Transaction extends UpdateBuilder { - - private static final Logger LOGGER = Logger.getLogger(Transaction.class.getName()); - private static final String READ_BEFORE_WRITE_ERROR_MSG = - "Firestore transactions require all reads to be executed before all writes"; +public interface Transaction { /** * User callback that takes a Firestore Transaction. @@ -66,158 +44,88 @@ public interface AsyncFunction { ApiFuture updateCallback(Transaction transaction); } - private final TransactionOptions transactionOptions; - private ByteString transactionId; + @Nonnull + ApiFuture get(@Nonnull DocumentReference documentRef); - Transaction( - FirestoreImpl firestore, - TransactionOptions transactionOptions, - @Nullable Transaction previousTransaction) { - super(firestore); - this.transactionOptions = transactionOptions; - this.transactionId = previousTransaction != null ? previousTransaction.transactionId : null; - } + @Nonnull + ApiFuture> getAll(@Nonnull DocumentReference... documentReferences); - public boolean hasTransactionId() { - return transactionId != null; - } + @Nonnull + ApiFuture> getAll( + @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask); - Transaction wrapResult(int writeIndex) { - return this; - } + @Nonnull + ApiFuture get(@Nonnull Query query); - /** Starts a transaction and obtains the transaction id. */ - ApiFuture begin() { - Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_BEGINTRANSACTION); - BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); - beginTransaction.setDatabase(firestore.getDatabaseName()); - - if (TransactionOptionsType.READ_WRITE.equals(transactionOptions.getType()) - && transactionId != null) { - beginTransaction.getOptionsBuilder().getReadWriteBuilder().setRetryTransaction(transactionId); - } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType())) { - final ReadOnly.Builder readOnlyBuilder = ReadOnly.newBuilder(); - if (transactionOptions.getReadTime() != null) { - readOnlyBuilder.setReadTime(transactionOptions.getReadTime()); - } - beginTransaction.getOptionsBuilder().setReadOnly(readOnlyBuilder); - } - - ApiFuture transactionBeginFuture = - firestore.sendRequest( - beginTransaction.build(), firestore.getClient().beginTransactionCallable()); - - return ApiFutures.transform( - transactionBeginFuture, - beginTransactionResponse -> { - transactionId = beginTransactionResponse.getTransaction(); - return null; - }, - MoreExecutors.directExecutor()); - } + @Nonnull + ApiFuture get(@Nonnull AggregateQuery query); - /** Commits a transaction. */ - ApiFuture> commit() { - return super.commit(transactionId); - } + @Nonnull + Transaction create(@Nonnull DocumentReference documentReference, @Nonnull Map fields); - /** Rolls a transaction back and releases all read locks. */ - ApiFuture rollback() { - Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_ROLLBACK); - RollbackRequest req = - RollbackRequest.newBuilder() - .setTransaction(transactionId) - .setDatabase(firestore.getDatabaseName()) - .build(); - - ApiFuture rollbackFuture = - firestore.sendRequest(req, firestore.getClient().rollbackCallable()); - - ApiFuture transform = - ApiFutures.transform(rollbackFuture, resp -> null, MoreExecutors.directExecutor()); - - return ApiFutures.catching( - transform, - Throwable.class, - (error) -> { - LOGGER.log( - Level.WARNING, - "Failed best effort to rollback of transaction " + transactionId, - error); - return null; - }, - MoreExecutors.directExecutor()); - } + @Nonnull + Transaction create(@Nonnull DocumentReference documentReference, @Nonnull Object pojo); - /** - * Reads the document referred to by the provided DocumentReference. Holds a pessimistic lock on - * the returned document. - * - * @return The contents of the Document at this DocumentReference. - */ @Nonnull - public ApiFuture get(@Nonnull DocumentReference documentRef) { - Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); - Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_GETDOCUMENT); - return ApiFutures.transform( - firestore.getAll(new DocumentReference[] {documentRef}, /*fieldMask=*/ null, transactionId), - snapshots -> snapshots.isEmpty() ? null : snapshots.get(0), - MoreExecutors.directExecutor()); - } + Transaction set(@Nonnull DocumentReference documentReference, @Nonnull Map fields); - /** - * Retrieves multiple documents from Firestore. Holds a pessimistic lock on all returned - * documents. - * - * @param documentReferences List of Document References to fetch. - */ @Nonnull - public ApiFuture> getAll( - @Nonnull DocumentReference... documentReferences) { - Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + Transaction set( + @Nonnull DocumentReference documentReference, + @Nonnull Map fields, + @Nonnull SetOptions options); - return firestore.getAll(documentReferences, /*fieldMask=*/ null, transactionId); - } + @Nonnull + Transaction set(@Nonnull DocumentReference documentReference, @Nonnull Object pojo); - /** - * Retrieves multiple documents from Firestore, while optionally applying a field mask to reduce - * the amount of data transmitted from the backend. Holds a pessimistic lock on all returned - * documents. - * - * @param documentReferences Array with Document References to fetch. - * @param fieldMask If set, specifies the subset of fields to return. - */ @Nonnull - public ApiFuture> getAll( - @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { - Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + Transaction set( + @Nonnull DocumentReference documentReference, + @Nonnull Object pojo, + @Nonnull SetOptions options); - return firestore.getAll(documentReferences, fieldMask, transactionId); - } + @Nonnull + Transaction update(@Nonnull DocumentReference documentReference, @Nonnull Map fields); - /** - * Returns the result set from the provided query. Holds a pessimistic lock on all returned - * documents. - * - * @return The contents of the Document at this DocumentReference. - */ @Nonnull - public ApiFuture get(@Nonnull Query query) { - Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull Map fields, + Precondition precondition); - return query.get(transactionId); - } + @Nonnull + Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull String field, + @Nullable Object value, + Object... moreFieldsAndValues); - /** - * Returns the result from the provided aggregate query. Holds a pessimistic lock on all accessed - * documents. - * - * @return The result of the aggregation. - */ @Nonnull - public ApiFuture get(@Nonnull AggregateQuery query) { - Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); + Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull FieldPath fieldPath, + @Nullable Object value, + Object... moreFieldsAndValues); - return query.get(transactionId); - } + @Nonnull + Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull Precondition precondition, + @Nonnull String field, + @Nullable Object value, + Object... moreFieldsAndValues); + + @Nonnull + Transaction update( + @Nonnull DocumentReference documentReference, + @Nonnull Precondition precondition, + @Nonnull FieldPath fieldPath, + @Nullable Object value, + Object... moreFieldsAndValues); + + @Nonnull + Transaction delete(@Nonnull DocumentReference documentReference, @Nonnull Precondition precondition); + + @Nonnull + Transaction delete(@Nonnull DocumentReference documentReference); } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java index 6ab4fecd2..480fb7c5b 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java @@ -24,6 +24,8 @@ import com.google.api.gax.retrying.ExponentialRetryAlgorithm; import com.google.api.gax.retrying.TimedAttemptSettings; import com.google.api.gax.rpc.ApiException; +import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.MoreExecutors; import io.grpc.Context; @@ -60,9 +62,8 @@ class TransactionRunner { private final ScheduledExecutorService firestoreExecutor; private final Executor userCallbackExecutor; private final ExponentialRetryAlgorithm backoffAlgorithm; - private final TransactionOptions transactionOptions; private TimedAttemptSettings nextBackoffAttempt; - private Transaction transaction; + private ReadWriteTransaction transaction; private int attemptsRemaining; /** @@ -75,7 +76,8 @@ class TransactionRunner { FirestoreImpl firestore, Transaction.AsyncFunction userCallback, TransactionOptions transactionOptions) { - this.transactionOptions = transactionOptions; + Preconditions.checkArgument( + TransactionOptionsType.READ_WRITE.equals(transactionOptions.getType())); this.span = tracer.spanBuilder("CloudFirestore.Transaction").startSpan(); this.firestore = firestore; this.firestoreExecutor = firestore.getClient().getExecutor(); @@ -94,7 +96,7 @@ class TransactionRunner { } ApiFuture run() { - this.transaction = new Transaction(firestore, transactionOptions, this.transaction); + this.transaction = new ReadWriteTransaction(firestore, this.transaction); --attemptsRemaining; 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 index a1fa02e29..89ca54567 100644 --- 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 @@ -969,7 +969,7 @@ public void givesProperErrorMessageForCommittedTransaction() throws Exception { .when(firestoreMock) .sendRequest( requestCapture.capture(), ArgumentMatchers.>any()); - String expectedErrorMessage = "Cannot modify a Transaction that has already been committed."; + String expectedErrorMessage = "Cannot modify a ReadWriteTransaction that has already been committed."; DocumentReference docRef = firestoreMock.collection("foo").document("bar"); From bf9921cd89e659eba15d6493cab25e0f274fc922 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 18:27:40 -0500 Subject: [PATCH 02/10] Pretty --- .../google/cloud/firestore/ReadOnlyTransaction.java | 6 ++---- .../java/com/google/cloud/firestore/Transaction.java | 12 ++++++++---- .../com/google/cloud/firestore/TransactionTest.java | 3 ++- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java index 723d0f4f4..bdb3703c2 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java @@ -89,8 +89,7 @@ public Transaction create( @Nonnull @Override - public Transaction create( - @Nonnull DocumentReference documentReference, @Nonnull Object pojo) { + public Transaction create(@Nonnull DocumentReference documentReference, @Nonnull Object pojo) { throw new IllegalStateException(WRITE_EXCEPTION_MSG); } @@ -112,8 +111,7 @@ public Transaction set( @Nonnull @Override - public Transaction set( - @Nonnull DocumentReference documentReference, @Nonnull Object pojo) { + public Transaction set(@Nonnull DocumentReference documentReference, @Nonnull Object pojo) { throw new IllegalStateException(WRITE_EXCEPTION_MSG); } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java index e5371b9e5..5d76d4409 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java @@ -61,13 +61,15 @@ ApiFuture> getAll( ApiFuture get(@Nonnull AggregateQuery query); @Nonnull - Transaction create(@Nonnull DocumentReference documentReference, @Nonnull Map fields); + Transaction create( + @Nonnull DocumentReference documentReference, @Nonnull Map fields); @Nonnull Transaction create(@Nonnull DocumentReference documentReference, @Nonnull Object pojo); @Nonnull - Transaction set(@Nonnull DocumentReference documentReference, @Nonnull Map fields); + Transaction set( + @Nonnull DocumentReference documentReference, @Nonnull Map fields); @Nonnull Transaction set( @@ -85,7 +87,8 @@ Transaction set( @Nonnull SetOptions options); @Nonnull - Transaction update(@Nonnull DocumentReference documentReference, @Nonnull Map fields); + Transaction update( + @Nonnull DocumentReference documentReference, @Nonnull Map fields); @Nonnull Transaction update( @@ -124,7 +127,8 @@ Transaction update( Object... moreFieldsAndValues); @Nonnull - Transaction delete(@Nonnull DocumentReference documentReference, @Nonnull Precondition precondition); + Transaction delete( + @Nonnull DocumentReference documentReference, @Nonnull Precondition precondition); @Nonnull Transaction delete(@Nonnull DocumentReference documentReference); 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 index 89ca54567..87ac6d9db 100644 --- 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 @@ -969,7 +969,8 @@ public void givesProperErrorMessageForCommittedTransaction() throws Exception { .when(firestoreMock) .sendRequest( requestCapture.capture(), ArgumentMatchers.>any()); - String expectedErrorMessage = "Cannot modify a ReadWriteTransaction that has already been committed."; + String expectedErrorMessage = + "Cannot modify a ReadWriteTransaction that has already been committed."; DocumentReference docRef = firestoreMock.collection("foo").document("bar"); From b69e4dde03b8d597f63364276b90428134e1752e Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 9 Feb 2024 10:17:26 -0500 Subject: [PATCH 03/10] Refactor --- .../cloud/firestore/ReadOnlyTransaction.java | 2 +- .../cloud/firestore/ReadWriteTransaction.java | 2 +- .../cloud/firestore/it/ITSystemTest.java | 21 +++++++++---------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java index bdb3703c2..8dc7f51a0 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java @@ -29,7 +29,7 @@ final class ReadOnlyTransaction implements Transaction { - public static final String WRITE_EXCEPTION_MSG = "Firestore transactions do not support writes"; + public static final String WRITE_EXCEPTION_MSG = "Firestore ready-only transactions do not support writes"; private final FirestoreImpl firestore; private final Timestamp readTime; diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java index 75e72cc9c..6225a28df 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java @@ -38,7 +38,7 @@ * * @see Firestore#runTransaction(Function) */ -public final class ReadWriteTransaction extends UpdateBuilder +final class ReadWriteTransaction extends UpdateBuilder implements Transaction { private static final Logger LOGGER = Logger.getLogger(ReadWriteTransaction.class.getName()); diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java index 2c092918b..fd0dd8419 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java @@ -1785,18 +1785,17 @@ public void readOnlyTransaction_failureWhenAttemptingWrite() }, TransactionOptions.createReadOnlyOptionsBuilder().build()); - try { + ExecutionException e = assertThrows(ExecutionException.class, () -> { runTransaction.get(10, TimeUnit.SECONDS); - } catch (ExecutionException e) { - final Throwable cause = e.getCause(); - assertThat(cause).isInstanceOf(FirestoreException.class); - final Throwable rootCause = ExceptionUtils.getRootCause(cause); - assertThat(rootCause).isInstanceOf(StatusRuntimeException.class); - final StatusRuntimeException invalidArgument = (StatusRuntimeException) rootCause; - final Status status = invalidArgument.getStatus(); - assertThat(status.getCode()).isEqualTo(Code.INVALID_ARGUMENT); - assertThat(status.getDescription()).contains("read-only"); - } + }); + final Throwable cause = e.getCause(); + assertThat(cause).isInstanceOf(FirestoreException.class); + final Throwable rootCause = ExceptionUtils.getRootCause(cause); + assertThat(rootCause).isInstanceOf(StatusRuntimeException.class); + final StatusRuntimeException invalidArgument = (StatusRuntimeException) rootCause; + final Status status = invalidArgument.getStatus(); + assertThat(status.getCode()).isEqualTo(Code.INVALID_ARGUMENT); + assertThat(status.getDescription()).contains("read-only"); } @Test From d3fc2c31390e9ffc1505e2ef9e856aa9834f0619 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 9 Feb 2024 10:53:22 -0500 Subject: [PATCH 04/10] Handle null readTime --- .../google/cloud/firestore/FirestoreImpl.java | 15 +++++------ ...nsaction.java => ReadTimeTransaction.java} | 6 +++-- ...action.java => ServerSideTransaction.java} | 25 ++++++++++++++----- .../cloud/firestore/TransactionRunner.java | 8 +++--- 4 files changed, 35 insertions(+), 19 deletions(-) rename google-cloud-firestore/src/main/java/com/google/cloud/firestore/{ReadOnlyTransaction.java => ReadTimeTransaction.java} (96%) rename google-cloud-firestore/src/main/java/com/google/cloud/firestore/{ReadWriteTransaction.java => ServerSideTransaction.java} (85%) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index b8718723d..7f99a3e30 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -29,6 +29,7 @@ import com.google.api.gax.rpc.StreamController; import com.google.api.gax.rpc.UnaryCallable; import com.google.cloud.Timestamp; +import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; import com.google.cloud.firestore.spi.v1.FirestoreRpc; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -397,13 +398,13 @@ public ApiFuture runAsyncTransaction( @Nonnull final Transaction.AsyncFunction updateFunction, @Nonnull TransactionOptions transactionOptions) { - switch (transactionOptions.getType()) { - case READ_ONLY: - return updateFunction.updateCallback( - new ReadOnlyTransaction(this, transactionOptions.getReadTime())); - case READ_WRITE: - default: - return new TransactionRunner<>(this, updateFunction, transactionOptions).run(); + if (transactionOptions.getReadTime() != null) { + return updateFunction.updateCallback( + new ReadTimeTransaction(this, transactionOptions.getReadTime())); + } else { + // For READ_ONLY transactions without readTime, there is still strong consistency applied, + // that cannot be tracked client side. + return new TransactionRunner<>(this, updateFunction, transactionOptions).run(); } } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java similarity index 96% rename from google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java rename to google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java index 8dc7f51a0..d209fac18 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadOnlyTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java @@ -18,6 +18,7 @@ import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; +import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; import com.google.protobuf.ByteString; import com.google.protobuf.Timestamp; @@ -27,13 +28,14 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; -final class ReadOnlyTransaction implements Transaction { +final class ReadTimeTransaction implements Transaction { public static final String WRITE_EXCEPTION_MSG = "Firestore ready-only transactions do not support writes"; private final FirestoreImpl firestore; private final Timestamp readTime; - ReadOnlyTransaction(FirestoreImpl firestore, Timestamp readTime) { + ReadTimeTransaction(FirestoreImpl firestore, Timestamp readTime) { + Preconditions.checkNotNull(readTime, "readTime cannot be null"); this.firestore = firestore; this.readTime = readTime; } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java similarity index 85% rename from google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java rename to google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java index 6225a28df..4dcb72bbf 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadWriteTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java @@ -18,11 +18,13 @@ import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; +import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; import com.google.firestore.v1.BeginTransactionRequest; import com.google.firestore.v1.BeginTransactionResponse; import com.google.firestore.v1.RollbackRequest; +import com.google.firestore.v1.TransactionOptions.ReadOnly; import com.google.protobuf.ByteString; import com.google.protobuf.Empty; import io.opencensus.trace.Tracing; @@ -38,18 +40,22 @@ * * @see Firestore#runTransaction(Function) */ -final class ReadWriteTransaction extends UpdateBuilder +final class ServerSideTransaction extends UpdateBuilder implements Transaction { - private static final Logger LOGGER = Logger.getLogger(ReadWriteTransaction.class.getName()); + private static final Logger LOGGER = Logger.getLogger(ServerSideTransaction.class.getName()); private static final String READ_BEFORE_WRITE_ERROR_MSG = "Firestore transactions require all reads to be executed before all writes"; + private final TransactionOptions transactionOptions; private ByteString transactionId; - ReadWriteTransaction( - FirestoreImpl firestore, @Nullable ReadWriteTransaction previousTransaction) { + ServerSideTransaction( + FirestoreImpl firestore, + TransactionOptions transactionOptions, + @Nullable ServerSideTransaction previousTransaction) { super(firestore); + this.transactionOptions = transactionOptions; this.transactionId = previousTransaction != null ? previousTransaction.transactionId : null; } @@ -58,7 +64,7 @@ public boolean hasTransactionId() { } @Override - ReadWriteTransaction wrapResult(int writeIndex) { + ServerSideTransaction wrapResult(int writeIndex) { return this; } @@ -68,8 +74,15 @@ ApiFuture begin() { BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); beginTransaction.setDatabase(firestore.getDatabaseName()); - if (transactionId != null) { + if (TransactionOptionsType.READ_WRITE.equals(transactionOptions.getType()) + && transactionId != null) { beginTransaction.getOptionsBuilder().getReadWriteBuilder().setRetryTransaction(transactionId); + } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType())) { + final ReadOnly.Builder readOnlyBuilder = ReadOnly.newBuilder(); + if (transactionOptions.getReadTime() != null) { + readOnlyBuilder.setReadTime(transactionOptions.getReadTime()); + } + beginTransaction.getOptionsBuilder().setReadOnly(readOnlyBuilder); } ApiFuture transactionBeginFuture = diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java index 480fb7c5b..430af9d0e 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java @@ -62,8 +62,9 @@ class TransactionRunner { private final ScheduledExecutorService firestoreExecutor; private final Executor userCallbackExecutor; private final ExponentialRetryAlgorithm backoffAlgorithm; + private final TransactionOptions transactionOptions; private TimedAttemptSettings nextBackoffAttempt; - private ReadWriteTransaction transaction; + private ServerSideTransaction transaction; private int attemptsRemaining; /** @@ -76,8 +77,7 @@ class TransactionRunner { FirestoreImpl firestore, Transaction.AsyncFunction userCallback, TransactionOptions transactionOptions) { - Preconditions.checkArgument( - TransactionOptionsType.READ_WRITE.equals(transactionOptions.getType())); + this.transactionOptions = transactionOptions; this.span = tracer.spanBuilder("CloudFirestore.Transaction").startSpan(); this.firestore = firestore; this.firestoreExecutor = firestore.getClient().getExecutor(); @@ -96,7 +96,7 @@ class TransactionRunner { } ApiFuture run() { - this.transaction = new ReadWriteTransaction(firestore, this.transaction); + this.transaction = new ServerSideTransaction(firestore, transactionOptions, this.transaction); --attemptsRemaining; From 32999105a9c2afd7926c9447fa8579def87bd2c6 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 9 Feb 2024 11:20:14 -0500 Subject: [PATCH 05/10] Consistent error messages --- .../com/google/cloud/firestore/ServerSideTransaction.java | 5 +++++ .../main/java/com/google/cloud/firestore/UpdateBuilder.java | 6 +++++- .../java/com/google/cloud/firestore/TransactionTest.java | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java index 4dcb72bbf..97a3e7803 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java @@ -59,6 +59,11 @@ final class ServerSideTransaction extends UpdateBuilder this.transactionId = previousTransaction != null ? previousTransaction.transactionId : null; } + @Override + protected String className() { + return "Transaction"; + } + public boolean hasTransactionId() { return transactionId != null; } 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 index 3b589fa9b..c81ee2a1e 100644 --- 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 @@ -298,13 +298,17 @@ private T addWrite(DocumentReference documentReference, Write.Builder write) { !committed, String.format( "Cannot modify a %s that has already been committed.", - this.getClass().getSimpleName())); + className())); writes.add(operation); writeIndex = writes.size() - 1; } return wrapResult(writeIndex); } + protected String className() { + return this.getClass().getSimpleName(); + } + /** Removes all values in 'fields' that are not specified in 'fieldMask'. */ private static Map applyFieldMask( Map fields, List fieldMask) { 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 index 87ac6d9db..bf4db87f1 100644 --- 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 @@ -970,7 +970,7 @@ public void givesProperErrorMessageForCommittedTransaction() throws Exception { .sendRequest( requestCapture.capture(), ArgumentMatchers.>any()); String expectedErrorMessage = - "Cannot modify a ReadWriteTransaction that has already been committed."; + "Cannot modify a Transaction that has already been committed."; DocumentReference docRef = firestoreMock.collection("foo").document("bar"); From 42c6541bf6932a33db56927bec8c8f27fd849ca2 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 9 Feb 2024 11:20:46 -0500 Subject: [PATCH 06/10] Pretty --- .../java/com/google/cloud/firestore/FirestoreImpl.java | 1 - .../com/google/cloud/firestore/ReadTimeTransaction.java | 3 ++- .../com/google/cloud/firestore/TransactionRunner.java | 2 -- .../java/com/google/cloud/firestore/UpdateBuilder.java | 4 +--- .../java/com/google/cloud/firestore/TransactionTest.java | 3 +-- .../java/com/google/cloud/firestore/it/ITSystemTest.java | 9 ++++++--- 6 files changed, 10 insertions(+), 12 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index 7f99a3e30..20d74226d 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -29,7 +29,6 @@ import com.google.api.gax.rpc.StreamController; import com.google.api.gax.rpc.UnaryCallable; import com.google.cloud.Timestamp; -import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; import com.google.cloud.firestore.spi.v1.FirestoreRpc; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java index d209fac18..1fac0f2b6 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java @@ -30,7 +30,8 @@ final class ReadTimeTransaction implements Transaction { - public static final String WRITE_EXCEPTION_MSG = "Firestore ready-only transactions do not support writes"; + public static final String WRITE_EXCEPTION_MSG = + "Firestore ready-only transactions do not support writes"; private final FirestoreImpl firestore; private final Timestamp readTime; diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java index 430af9d0e..acb650eec 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java @@ -24,8 +24,6 @@ import com.google.api.gax.retrying.ExponentialRetryAlgorithm; import com.google.api.gax.retrying.TimedAttemptSettings; import com.google.api.gax.rpc.ApiException; -import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.MoreExecutors; import io.grpc.Context; 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 index c81ee2a1e..27f5a497d 100644 --- 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 @@ -296,9 +296,7 @@ private T addWrite(DocumentReference documentReference, Write.Builder write) { synchronized (writes) { Preconditions.checkState( !committed, - String.format( - "Cannot modify a %s that has already been committed.", - className())); + String.format("Cannot modify a %s that has already been committed.", className())); writes.add(operation); writeIndex = writes.size() - 1; } 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 index bf4db87f1..a1fa02e29 100644 --- 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 @@ -969,8 +969,7 @@ public void givesProperErrorMessageForCommittedTransaction() throws Exception { .when(firestoreMock) .sendRequest( requestCapture.capture(), ArgumentMatchers.>any()); - String expectedErrorMessage = - "Cannot modify a Transaction that has already been committed."; + String expectedErrorMessage = "Cannot modify a Transaction that has already been committed."; DocumentReference docRef = firestoreMock.collection("foo").document("bar"); diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java index fd0dd8419..6c9f55a99 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java @@ -1785,9 +1785,12 @@ public void readOnlyTransaction_failureWhenAttemptingWrite() }, TransactionOptions.createReadOnlyOptionsBuilder().build()); - ExecutionException e = assertThrows(ExecutionException.class, () -> { - runTransaction.get(10, TimeUnit.SECONDS); - }); + ExecutionException e = + assertThrows( + ExecutionException.class, + () -> { + runTransaction.get(10, TimeUnit.SECONDS); + }); final Throwable cause = e.getCause(); assertThat(cause).isInstanceOf(FirestoreException.class); final Throwable rootCause = ExceptionUtils.getRootCause(cause); From 724eb17648ca30fdef258d5610a8fd396d9553d1 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Sat, 10 Feb 2024 14:12:33 -0500 Subject: [PATCH 07/10] Refactor --- .../google/cloud/firestore/FirestoreImpl.java | 2 +- .../cloud/firestore/ReadTimeTransaction.java | 10 +- .../firestore/ServerSideTransaction.java | 65 +++----- ....java => ServerSideTransactionRunner.java} | 27 ++-- .../google/cloud/firestore/Transaction.java | 144 ++++++++---------- 5 files changed, 111 insertions(+), 137 deletions(-) rename google-cloud-firestore/src/main/java/com/google/cloud/firestore/{TransactionRunner.java => ServerSideTransactionRunner.java} (92%) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index 20d74226d..9d25519d2 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -403,7 +403,7 @@ public ApiFuture runAsyncTransaction( } else { // For READ_ONLY transactions without readTime, there is still strong consistency applied, // that cannot be tracked client side. - return new TransactionRunner<>(this, updateFunction, transactionOptions).run(); + return new ServerSideTransactionRunner<>(this, updateFunction, transactionOptions).run(); } } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java index 1fac0f2b6..96f38ff72 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java @@ -28,16 +28,15 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; -final class ReadTimeTransaction implements Transaction { +final class ReadTimeTransaction extends Transaction { public static final String WRITE_EXCEPTION_MSG = "Firestore ready-only transactions do not support writes"; - private final FirestoreImpl firestore; private final Timestamp readTime; ReadTimeTransaction(FirestoreImpl firestore, Timestamp readTime) { + super(firestore); Preconditions.checkNotNull(readTime, "readTime cannot be null"); - this.firestore = firestore; this.readTime = readTime; } @@ -197,4 +196,9 @@ public Transaction delete( public Transaction delete(@Nonnull DocumentReference documentReference) { throw new IllegalStateException(WRITE_EXCEPTION_MSG); } + + @Override + public String toString() { + return String.format("%s{readTime=%s}", getClass().getSimpleName(), readTime); + } } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java index 97a3e7803..ebedc327c 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java @@ -34,54 +34,43 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; -/** - * A Transaction is passed to a Function to provide the methods to read and write data within the - * transaction context. - * - * @see Firestore#runTransaction(Function) - */ -final class ServerSideTransaction extends UpdateBuilder - implements Transaction { +final class ServerSideTransaction extends Transaction { private static final Logger LOGGER = Logger.getLogger(ServerSideTransaction.class.getName()); + private static final String READ_BEFORE_WRITE_ERROR_MSG = "Firestore transactions require all reads to be executed before all writes"; - private final TransactionOptions transactionOptions; - private ByteString transactionId; - - ServerSideTransaction( - FirestoreImpl firestore, - TransactionOptions transactionOptions, - @Nullable ServerSideTransaction previousTransaction) { - super(firestore); - this.transactionOptions = transactionOptions; - this.transactionId = previousTransaction != null ? previousTransaction.transactionId : null; - } + private final FirestoreImpl firestore; - @Override - protected String className() { - return "Transaction"; - } + final ByteString transactionId; - public boolean hasTransactionId() { - return transactionId != null; + private ServerSideTransaction(FirestoreImpl firestore, ByteString transactionId) { + super(firestore); + this.firestore = firestore; + this.transactionId = transactionId; } - @Override - ServerSideTransaction wrapResult(int writeIndex) { - return this; + public ByteString getTransactionId() { + return transactionId; } - /** Starts a transaction and obtains the transaction id. */ - ApiFuture begin() { + public static ApiFuture begin( + FirestoreImpl firestore, + TransactionOptions transactionOptions, + @Nullable ServerSideTransaction previousTransaction) { Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_BEGINTRANSACTION); BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); beginTransaction.setDatabase(firestore.getDatabaseName()); + ByteString previousTransactionId = + previousTransaction != null ? previousTransaction.transactionId : null; if (TransactionOptionsType.READ_WRITE.equals(transactionOptions.getType()) - && transactionId != null) { - beginTransaction.getOptionsBuilder().getReadWriteBuilder().setRetryTransaction(transactionId); + && previousTransactionId != null) { + beginTransaction + .getOptionsBuilder() + .getReadWriteBuilder() + .setRetryTransaction(previousTransactionId); } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType())) { final ReadOnly.Builder readOnlyBuilder = ReadOnly.newBuilder(); if (transactionOptions.getReadTime() != null) { @@ -96,10 +85,8 @@ ApiFuture begin() { return ApiFutures.transform( transactionBeginFuture, - beginTransactionResponse -> { - transactionId = beginTransactionResponse.getTransaction(); - return null; - }, + beginTransactionResponse -> + new ServerSideTransaction(firestore, beginTransactionResponse.getTransaction()), MoreExecutors.directExecutor()); } @@ -145,8 +132,8 @@ ApiFuture rollback() { @Override @Nonnull public ApiFuture get(@Nonnull DocumentReference documentRef) { - Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_GETDOCUMENT); + Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); return ApiFutures.transform( firestore.getAll( new DocumentReference[] {documentRef}, @@ -168,7 +155,6 @@ public ApiFuture get(@Nonnull DocumentReference documentRef) { public ApiFuture> getAll( @Nonnull DocumentReference... documentReferences) { Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); - return firestore.getAll( documentReferences, /*fieldMask=*/ null, transactionId, /*readTime=*/ null); } @@ -186,7 +172,6 @@ public ApiFuture> getAll( public ApiFuture> getAll( @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); - return firestore.getAll(documentReferences, fieldMask, transactionId, /*readTime=*/ null); } @@ -200,7 +185,6 @@ public ApiFuture> getAll( @Nonnull public ApiFuture get(@Nonnull Query query) { Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); - return query.get(transactionId, /*readTime=*/ null); } @@ -214,7 +198,6 @@ public ApiFuture get(@Nonnull Query query) { @Nonnull public ApiFuture get(@Nonnull AggregateQuery query) { Preconditions.checkState(isEmpty(), READ_BEFORE_WRITE_ERROR_MSG); - return query.get(transactionId, null); } } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java similarity index 92% rename from google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java rename to google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java index acb650eec..77a8e4b9a 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java @@ -46,7 +46,7 @@ *

TransactionRunner uses exponential backoff to increase the chance that retries succeed. To * customize the backoff settings, you can specify custom settings via {@link FirestoreOptions}. */ -class TransactionRunner { +class ServerSideTransactionRunner { private static final Tracer tracer = Tracing.getTracer(); private static final io.opencensus.trace.Status TOO_MANY_RETRIES_STATUS = @@ -71,7 +71,7 @@ class TransactionRunner { * @param transactionOptions The options determining which executor the {@code userCallback} is * run on and whether the transaction is read-write or read-only */ - TransactionRunner( + ServerSideTransactionRunner( FirestoreImpl firestore, Transaction.AsyncFunction userCallback, TransactionOptions transactionOptions) { @@ -94,8 +94,6 @@ class TransactionRunner { } ApiFuture run() { - this.transaction = new ServerSideTransaction(firestore, transactionOptions, this.transaction); - --attemptsRemaining; span.addAnnotation( @@ -110,10 +108,14 @@ ApiFuture run() { MoreExecutors.directExecutor()); } + ApiFuture begin() { + ServerSideTransaction previousTransaction = this.transaction; + this.transaction = null; + return ServerSideTransaction.begin(firestore, transactionOptions, previousTransaction); + } + private ApiFuture maybeRollback() { - return transaction.hasTransactionId() - ? transaction.rollback() - : ApiFutures.immediateFuture(null); + return transaction != null ? transaction.rollback() : ApiFutures.immediateFuture(null); } /** A callback that invokes the BeginTransaction callback. */ @@ -127,7 +129,7 @@ private ApiFuture rollbackCallback(Void input) { nextBackoffAttempt = backoffAlgorithm.createNextAttempt(nextBackoffAttempt); return ApiFutures.transformAsync( - backoff, TransactionRunner.this::backoffCallback, MoreExecutors.directExecutor()); + backoff, this::backoffCallback, MoreExecutors.directExecutor()); } /** @@ -165,14 +167,15 @@ public void onSuccess(T result) { /** A callback that invokes the BeginTransaction callback. */ private ApiFuture backoffCallback(Void input) { return ApiFutures.transformAsync( - transaction.begin(), this::beginTransactionCallback, MoreExecutors.directExecutor()); + begin(), this::beginTransactionCallback, MoreExecutors.directExecutor()); } /** * The callback for the BeginTransaction RPC, which invokes the user callback and handles all * errors thereafter. */ - private ApiFuture beginTransactionCallback(Void input) { + private ApiFuture beginTransactionCallback(ServerSideTransaction serverSideTransaction) { + this.transaction = serverSideTransaction; return ApiFutures.transformAsync( invokeUserCallback(), this::userFunctionCallback, MoreExecutors.directExecutor()); } @@ -202,7 +205,7 @@ private ApiFuture restartTransactionCallback(Throwable throwable) { } ApiException apiException = (ApiException) throwable; - if (transaction.hasTransactionId() && isRetryableTransactionError(apiException)) { + if (isRetryableTransactionError(apiException)) { if (attemptsRemaining > 0) { span.addAnnotation("retrying"); return run(); @@ -248,7 +251,7 @@ private static boolean isRetryableTransactionError(ApiException exception) { private ApiFuture rollbackAndReject(final Throwable throwable) { final SettableApiFuture failedTransaction = SettableApiFuture.create(); - if (transaction.hasTransactionId()) { + if (transaction != null) { // We use `addListener()` since we want to return the original exception regardless of // whether rollback() succeeds. transaction diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java index 5d76d4409..5bfc7e043 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java @@ -1,5 +1,5 @@ /* - * Copyright 2024 Google LLC + * Copyright 2017 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,11 +18,25 @@ import com.google.api.core.ApiFuture; import java.util.List; -import java.util.Map; +import java.util.logging.Logger; import javax.annotation.Nonnull; import javax.annotation.Nullable; -public interface Transaction { +/** + * A Transaction is passed to a Function to provide the methods to read and write data within the + * transaction context. + * + * @see Firestore#runTransaction(Function) + */ +public abstract class Transaction extends UpdateBuilder { + + private static final Logger LOGGER = Logger.getLogger(Transaction.class.getName()); + private static final String READ_BEFORE_WRITE_ERROR_MSG = + "Firestore transactions require all reads to be executed before all writes"; + + Transaction(FirestoreImpl firestore) { + super(firestore); + } /** * User callback that takes a Firestore Transaction. @@ -44,92 +58,62 @@ public interface AsyncFunction { ApiFuture updateCallback(Transaction transaction); } - @Nonnull - ApiFuture get(@Nonnull DocumentReference documentRef); - - @Nonnull - ApiFuture> getAll(@Nonnull DocumentReference... documentReferences); - - @Nonnull - ApiFuture> getAll( - @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask); - - @Nonnull - ApiFuture get(@Nonnull Query query); - - @Nonnull - ApiFuture get(@Nonnull AggregateQuery query); - - @Nonnull - Transaction create( - @Nonnull DocumentReference documentReference, @Nonnull Map fields); - - @Nonnull - Transaction create(@Nonnull DocumentReference documentReference, @Nonnull Object pojo); - - @Nonnull - Transaction set( - @Nonnull DocumentReference documentReference, @Nonnull Map fields); - - @Nonnull - Transaction set( - @Nonnull DocumentReference documentReference, - @Nonnull Map fields, - @Nonnull SetOptions options); - - @Nonnull - Transaction set(@Nonnull DocumentReference documentReference, @Nonnull Object pojo); - - @Nonnull - Transaction set( - @Nonnull DocumentReference documentReference, - @Nonnull Object pojo, - @Nonnull SetOptions options); - - @Nonnull - Transaction update( - @Nonnull DocumentReference documentReference, @Nonnull Map fields); - - @Nonnull - Transaction update( - @Nonnull DocumentReference documentReference, - @Nonnull Map fields, - Precondition precondition); + @Override + protected String className() { + return "Transaction"; + } - @Nonnull - Transaction update( - @Nonnull DocumentReference documentReference, - @Nonnull String field, - @Nullable Object value, - Object... moreFieldsAndValues); + @Override + Transaction wrapResult(int writeIndex) { + return this; + } + /** + * Reads the document referred to by the provided DocumentReference. Holds a pessimistic lock on + * the returned document. + * + * @return The contents of the Document at this DocumentReference. + */ @Nonnull - Transaction update( - @Nonnull DocumentReference documentReference, - @Nonnull FieldPath fieldPath, - @Nullable Object value, - Object... moreFieldsAndValues); + public abstract ApiFuture get(@Nonnull DocumentReference documentRef); + /** + * Retrieves multiple documents from Firestore. Holds a pessimistic lock on all returned + * documents. + * + * @param documentReferences List of Document References to fetch. + */ @Nonnull - Transaction update( - @Nonnull DocumentReference documentReference, - @Nonnull Precondition precondition, - @Nonnull String field, - @Nullable Object value, - Object... moreFieldsAndValues); + public abstract ApiFuture> getAll( + @Nonnull DocumentReference... documentReferences); + /** + * Retrieves multiple documents from Firestore, while optionally applying a field mask to reduce + * the amount of data transmitted from the backend. Holds a pessimistic lock on all returned + * documents. + * + * @param documentReferences Array with Document References to fetch. + * @param fieldMask If set, specifies the subset of fields to return. + */ @Nonnull - Transaction update( - @Nonnull DocumentReference documentReference, - @Nonnull Precondition precondition, - @Nonnull FieldPath fieldPath, - @Nullable Object value, - Object... moreFieldsAndValues); + public abstract ApiFuture> getAll( + @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask); + /** + * Returns the result set from the provided query. Holds a pessimistic lock on all returned + * documents. + * + * @return The contents of the Document at this DocumentReference. + */ @Nonnull - Transaction delete( - @Nonnull DocumentReference documentReference, @Nonnull Precondition precondition); + public abstract ApiFuture get(@Nonnull Query query); + /** + * Returns the result from the provided aggregate query. Holds a pessimistic lock on all accessed + * documents. + * + * @return The result of the aggregation. + */ @Nonnull - Transaction delete(@Nonnull DocumentReference documentReference); + public abstract ApiFuture get(@Nonnull AggregateQuery query); } From 3bb84b8005f3fb68bbeaf0265a6983564cc6525b Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Sat, 10 Feb 2024 14:20:47 -0500 Subject: [PATCH 08/10] Make more backward compatible --- .../java/com/google/cloud/firestore/ReadTimeTransaction.java | 5 +++++ .../com/google/cloud/firestore/ServerSideTransaction.java | 5 +++++ .../google/cloud/firestore/ServerSideTransactionRunner.java | 2 +- .../main/java/com/google/cloud/firestore/Transaction.java | 2 ++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java index 96f38ff72..c7d0eab69 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java @@ -40,6 +40,11 @@ final class ReadTimeTransaction extends Transaction { this.readTime = readTime; } + @Override + public boolean hasTransactionId() { + return false; + } + @Nonnull @Override public ApiFuture get(@Nonnull DocumentReference documentRef) { diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java index ebedc327c..b935dc3b4 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java @@ -123,6 +123,11 @@ ApiFuture rollback() { MoreExecutors.directExecutor()); } + @Override + public boolean hasTransactionId() { + return true; + } + /** * Reads the document referred to by the provided DocumentReference. Holds a pessimistic lock on * the returned document. diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java index 77a8e4b9a..c05ca76f1 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java @@ -46,7 +46,7 @@ *

TransactionRunner uses exponential backoff to increase the chance that retries succeed. To * customize the backoff settings, you can specify custom settings via {@link FirestoreOptions}. */ -class ServerSideTransactionRunner { +final class ServerSideTransactionRunner { private static final Tracer tracer = Tracing.getTracer(); private static final io.opencensus.trace.Status TOO_MANY_RETRIES_STATUS = diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java index 5bfc7e043..be242f8fb 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java @@ -63,6 +63,8 @@ protected String className() { return "Transaction"; } + public abstract boolean hasTransactionId(); + @Override Transaction wrapResult(int writeIndex) { return this; From e88d48a7dd6b76d326a7841872e4d050861b809b Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Sat, 10 Feb 2024 14:31:52 -0500 Subject: [PATCH 09/10] Clirr --- google-cloud-firestore/clirr-ignored-differences.xml | 6 ++++++ .../main/java/com/google/cloud/firestore/Transaction.java | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/google-cloud-firestore/clirr-ignored-differences.xml b/google-cloud-firestore/clirr-ignored-differences.xml index 5c9b4d6a0..7a4c83689 100644 --- a/google-cloud-firestore/clirr-ignored-differences.xml +++ b/google-cloud-firestore/clirr-ignored-differences.xml @@ -17,6 +17,12 @@ + + + 3005 + com/google/cloud/firestore/Transaction + + 7012 diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java index be242f8fb..22fa4e065 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java @@ -17,6 +17,7 @@ package com.google.cloud.firestore; import com.google.api.core.ApiFuture; +import com.google.api.core.InternalExtensionOnly; import java.util.List; import java.util.logging.Logger; import javax.annotation.Nonnull; @@ -28,13 +29,14 @@ * * @see Firestore#runTransaction(Function) */ +@InternalExtensionOnly public abstract class Transaction extends UpdateBuilder { private static final Logger LOGGER = Logger.getLogger(Transaction.class.getName()); private static final String READ_BEFORE_WRITE_ERROR_MSG = "Firestore transactions require all reads to be executed before all writes"; - Transaction(FirestoreImpl firestore) { + protected Transaction(FirestoreImpl firestore) { super(firestore); } From 8c851d8828760cc4cac8e0a71bf6c45d8e4a075e Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 12 Feb 2024 12:25:34 -0500 Subject: [PATCH 10/10] Feedback --- .../google/cloud/firestore/FirestoreImpl.java | 20 ++++++++++++++++-- .../cloud/firestore/ReadTimeTransaction.java | 21 ++++++++++--------- .../firestore/ServerSideTransaction.java | 9 ++++++++ .../ServerSideTransactionRunner.java | 8 +++++-- .../cloud/firestore/it/ITSystemTest.java | 6 +++++- 5 files changed, 49 insertions(+), 15 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index 9d25519d2..82dfc5176 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -194,14 +194,14 @@ public Iterable listCollections() { @Override public ApiFuture> getAll( @Nonnull DocumentReference... documentReferences) { - return this.getAll(documentReferences, null, (ByteString) null, null); + return this.getAll(documentReferences, null, (ByteString) null); } @Nonnull @Override public ApiFuture> getAll( @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { - return this.getAll(documentReferences, fieldMask, (ByteString) null, null); + return this.getAll(documentReferences, fieldMask, (ByteString) null); } @Override @@ -323,6 +323,20 @@ public void onComplete() { streamRequest(request.build(), responseObserver, firestoreClient.batchGetDocumentsCallable()); } + final ApiFuture> getAll( + final @Nonnull DocumentReference[] documentReferences, + @Nullable FieldMask fieldMask, + @Nullable com.google.protobuf.Timestamp readTime) { + return getAll(documentReferences, fieldMask, null, readTime); + } + + private ApiFuture> getAll( + final @Nonnull DocumentReference[] documentReferences, + @Nullable FieldMask fieldMask, + @Nullable ByteString transactionId) { + return getAll(documentReferences, fieldMask, transactionId, null); + } + /** Internal getAll() method that accepts an optional transaction id. */ ApiFuture> getAll( final @Nonnull DocumentReference[] documentReferences, @@ -398,6 +412,8 @@ public ApiFuture runAsyncTransaction( @Nonnull TransactionOptions transactionOptions) { if (transactionOptions.getReadTime() != null) { + // READ_ONLY transactions with readTime have no retry, nor transaction state, so we don't need + // a runner. return updateFunction.updateCallback( new ReadTimeTransaction(this, transactionOptions.getReadTime())); } else { diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java index c7d0eab69..36f86f4d1 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java @@ -20,7 +20,6 @@ import com.google.api.core.ApiFutures; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; -import com.google.protobuf.ByteString; import com.google.protobuf.Timestamp; import io.opencensus.trace.Tracing; import java.util.List; @@ -28,6 +27,14 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +/** + * The ReadTimeTransaction is a ready-only Transaction that specifies a ReadTime. Unlike a + * `ServerSideTransaction`, we do not need a `transactionId` since we provide a `readTime` on all + * requests. No concurrency control is required, since data in the past is immutable. As with all + * `read-only` transactions, we do not allow any write request. + * + * @see Transaction + */ final class ReadTimeTransaction extends Transaction { public static final String WRITE_EXCEPTION_MSG = @@ -50,11 +57,7 @@ public boolean hasTransactionId() { public ApiFuture get(@Nonnull DocumentReference documentRef) { Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_GETDOCUMENT); return ApiFutures.transform( - firestore.getAll( - new DocumentReference[] {documentRef}, - /*fieldMask=*/ null, - /*transactionId=*/ (ByteString) null, - readTime), + firestore.getAll(new DocumentReference[] {documentRef}, /*fieldMask=*/ null, readTime), snapshots -> snapshots.isEmpty() ? null : snapshots.get(0), MoreExecutors.directExecutor()); } @@ -63,16 +66,14 @@ public ApiFuture get(@Nonnull DocumentReference documentRef) { @Override public ApiFuture> getAll( @Nonnull DocumentReference... documentReferences) { - return firestore.getAll( - documentReferences, /*fieldMask=*/ null, /*transactionId=*/ (ByteString) null, readTime); + return firestore.getAll(documentReferences, /*fieldMask=*/ null, readTime); } @Nonnull @Override public ApiFuture> getAll( @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { - return firestore.getAll( - documentReferences, /*fieldMask=*/ null, /*transactionId=*/ (ByteString) null, readTime); + return firestore.getAll(documentReferences, /*fieldMask=*/ null, readTime); } @Nonnull diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java index b935dc3b4..0227c2c4c 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java @@ -34,6 +34,15 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +/** + * A `ServerSideTransaction` is a `Transaction` that uses server generated `transactionId` on + * requests. The implementation starts with a `beginTransaction` request that receives a + * `transactionId` from server. The `ServerSideTransactionRunner` must either `commit()` or + * `rollback()` when done. + * + * @see Transaction + * @see ServerSideTransactionRunner + */ final class ServerSideTransaction extends Transaction { private static final Logger LOGGER = Logger.getLogger(ServerSideTransaction.class.getName()); diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java index c05ca76f1..5cf4c1f79 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java @@ -115,7 +115,11 @@ ApiFuture begin() { } private ApiFuture maybeRollback() { - return transaction != null ? transaction.rollback() : ApiFutures.immediateFuture(null); + return hasTransaction() ? transaction.rollback() : ApiFutures.immediateFuture(null); + } + + private boolean hasTransaction() { + return transaction != null; } /** A callback that invokes the BeginTransaction callback. */ @@ -251,7 +255,7 @@ private static boolean isRetryableTransactionError(ApiException exception) { private ApiFuture rollbackAndReject(final Throwable throwable) { final SettableApiFuture failedTransaction = SettableApiFuture.create(); - if (transaction != null) { + if (hasTransaction()) { // We use `addListener()` since we want to return the original exception regardless of // whether rollback() succeeds. transaction diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java index 6c9f55a99..3766800a5 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java @@ -1841,7 +1841,11 @@ public void readOnlyTransaction_failureWhenAttemptReadOlderThan60Seconds() // To ensure we exceed this, we use 120 minutes. // If this test fails, we should likely be update documentation to reflect new value. See all // usages of "Read Time" on proto, and within SDK. - final long twoHours = System.currentTimeMillis() / 1000 - 7200; + // + // If Point-in-Time Recovery is enabled, can additionally be a whole minute timestamp within the + // past 7 days. For that reason `twoHours` is calculated to whole minute to more accurately + // catch this situation. + final long twoHours = (System.currentTimeMillis() / 60_000 - 120) * 60; final TransactionOptions options = TransactionOptions.createReadOnlyOptionsBuilder() .setReadTime(