From e7527a0f52a63643081e5f69b2b9bf75bfad81ce Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Thu, 6 Aug 2020 15:58:59 -0400 Subject: [PATCH 01/15] feat: add support for read-only transactions in TransactionOptions * Add new typesafe builders for read-only `TransactionOptions.readOnlyOptionsBuilder` and read-write `TransactionOptions.readWriteOptionsBuilder` transactions in TransactionOptions * These new builders ensure only those parameters relevant to the respective type of transaction are available * Deprecate existing `TransactionOptions.create(...)` methods in favor of the new builders * Deprecate existing and annotate @InternalApi `TransactionOptions.getNumberOfAttempts` * Update Transaction and TransactionRunner to use TransactionOptions directly --- .../google/cloud/firestore/FirestoreImpl.java | 10 +- .../google/cloud/firestore/Transaction.java | 21 +- .../cloud/firestore/TransactionOptions.java | 256 +++++++++++++++++- .../cloud/firestore/TransactionRunner.java | 36 ++- .../cloud/firestore/TransactionTest.java | 124 ++++++++- .../cloud/firestore/it/ITSystemTest.java | 96 +++++++ 6 files changed, 504 insertions(+), 39 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 717cf8fe4..1fa2f756b 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 @@ -30,7 +30,6 @@ import com.google.firestore.v1.BatchGetDocumentsResponse; import com.google.firestore.v1.DatabaseRootName; import com.google.protobuf.ByteString; -import io.grpc.Context; import io.opencensus.trace.AttributeValue; import io.opencensus.trace.Tracer; import io.opencensus.trace.Tracing; @@ -40,7 +39,6 @@ import java.util.List; import java.util.Map; import java.util.Random; -import java.util.concurrent.Executor; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -310,15 +308,9 @@ public ApiFuture runAsyncTransaction( public ApiFuture runAsyncTransaction( @Nonnull final Transaction.AsyncFunction updateFunction, @Nonnull TransactionOptions transactionOptions) { - final Executor userCallbackExecutor = - Context.currentContextExecutor( - transactionOptions.getExecutor() != null - ? transactionOptions.getExecutor() - : firestoreClient.getExecutor()); TransactionRunner transactionRunner = - new TransactionRunner<>( - this, updateFunction, userCallbackExecutor, transactionOptions.getNumberOfAttempts()); + new TransactionRunner<>(this, updateFunction, transactionOptions); return transactionRunner.run(); } 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 c543c3743..0ab6e9bf9 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 @@ -19,11 +19,14 @@ import com.google.api.core.ApiFunction; import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; +import com.google.cloud.firestore.TransactionOptions.EitherReadOnlyOrReadWrite; +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 java.util.List; @@ -61,12 +64,18 @@ public interface AsyncFunction { ApiFuture updateCallback(Transaction transaction); } + private final TransactionOptions originalTransactionOptions; + @Nullable private final ByteString previousTransactionId; private ByteString transactionId; - private @Nullable ByteString previousTransactionId; - Transaction(FirestoreImpl firestore, @Nullable Transaction previousTransaction) { + Transaction( + FirestoreImpl firestore, + TransactionOptions transactionOptions, + @Nullable Transaction previousTransaction) { super(firestore); - previousTransactionId = previousTransaction != null ? previousTransaction.transactionId : null; + this.originalTransactionOptions = transactionOptions; + this.previousTransactionId = + previousTransaction != null ? previousTransaction.transactionId : null; } Transaction wrapResult(ApiFuture result) { @@ -78,11 +87,15 @@ ApiFuture begin() { BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); beginTransaction.setDatabase(firestore.getDatabaseName()); - if (previousTransactionId != null) { + final EitherReadOnlyOrReadWrite options = originalTransactionOptions.getOptions(); + if (options.getType() == TransactionOptionsType.READ_WRITE && previousTransactionId != null) { beginTransaction .getOptionsBuilder() .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); + } else if (options.getType() == TransactionOptionsType.READ_ONLY) { + final ReadOnly.Builder builder = options.getReadOnly().toProtoBuilder(); + beginTransaction.getOptionsBuilder().setReadOnly(builder); } ApiFuture transactionBeginFuture = diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 9b54f7ac8..705606165 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -16,7 +16,12 @@ package com.google.cloud.firestore; +import com.google.api.core.InternalApi; import com.google.common.base.Preconditions; +import com.google.firestore.v1.TransactionOptions.ReadOnly; +import com.google.firestore.v1.TransactionOptions.ReadWrite; +import com.google.protobuf.Timestamp; +import com.google.protobuf.TimestampOrBuilder; import java.util.concurrent.Executor; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -24,34 +29,57 @@ /** Options specifying the behavior of Firestore Transactions. */ public final class TransactionOptions { + private static final TransactionOptions DEFAULT_READ_WRITE_TRANSACTION_OPTIONS = + readWriteOptionsBuilder().build(); + private static final int DEFAULT_NUM_ATTEMPTS = 5; - private final int numberOfAttempts; private final Executor executor; + private final EitherReadOnlyOrReadWrite options; - TransactionOptions(int maxAttempts, Executor executor) { - this.numberOfAttempts = maxAttempts; + TransactionOptions(Executor executor, EitherReadOnlyOrReadWrite options) { this.executor = executor; + this.options = options; } + /** + * @return int Representing the initial number of attempts a read-write transaction will be + * attempted + * @deprecated As of v2.0.0, only applicable to Read-Write transactions. Use {@link + * ReadWriteOptions#getNumberOfAttempts()} instead + */ + @Deprecated + @InternalApi public int getNumberOfAttempts() { - return numberOfAttempts; + if (options.getType() == TransactionOptionsType.READ_WRITE) { + return options.getReadWrite().numberOfAttempts; + } else { + return 1; + } } + /** @return Executor Executor to be used to run user callbacks on */ @Nullable public Executor getExecutor() { return executor; } + @Nonnull + @InternalApi + EitherReadOnlyOrReadWrite getOptions() { + return options; + } + /** - * Create a default set of options suitable for most use cases. Transactions will be attempted 5 - * times. + * Create a default set of ReadWrite options suitable for most use cases. Transactions will be + * attempted 5 times. * * @return The TransactionOptions object. + * @see #readWriteOptionsBuilder() */ @Nonnull public static TransactionOptions create() { - return new TransactionOptions(DEFAULT_NUM_ATTEMPTS, null); + return DEFAULT_READ_WRITE_TRANSACTION_OPTIONS; } /** @@ -59,11 +87,13 @@ public static TransactionOptions create() { * * @param numberOfAttempts The number of execution attempts. * @return The TransactionOptions object. + * @deprecated As of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} + * @see #readWriteOptionsBuilder() */ @Nonnull + @Deprecated public static TransactionOptions create(int numberOfAttempts) { - Preconditions.checkArgument(numberOfAttempts > 0, "You must allow at least one attempt"); - return new TransactionOptions(numberOfAttempts, null); + return readWriteOptionsBuilder().setNumberOfAttempts(numberOfAttempts).build(); } /** @@ -71,10 +101,13 @@ public static TransactionOptions create(int numberOfAttempts) { * * @param executor The executor to run the user callback code on. * @return The TransactionOptions object. + * @deprecated As of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} + * @see #readWriteOptionsBuilder() */ @Nonnull - public static TransactionOptions create(@Nonnull Executor executor) { - return new TransactionOptions(DEFAULT_NUM_ATTEMPTS, executor); + @Deprecated + public static TransactionOptions create(@Nullable Executor executor) { + return readWriteOptionsBuilder().setExecutor(executor).build(); } /** @@ -83,10 +116,205 @@ public static TransactionOptions create(@Nonnull Executor executor) { * @param executor The executor to run the user callback code on. * @param numberOfAttempts The number of execution attempts. * @return The TransactionOptions object. + * @deprecated As of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} and + * {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} + * @see #readWriteOptionsBuilder() */ @Nonnull - public static TransactionOptions create(@Nonnull Executor executor, int numberOfAttempts) { - Preconditions.checkArgument(numberOfAttempts > 0, "You must allow at least one attempt"); - return new TransactionOptions(numberOfAttempts, executor); + @Deprecated + public static TransactionOptions create(@Nullable Executor executor, int numberOfAttempts) { + return readWriteOptionsBuilder() + .setExecutor(executor) + .setNumberOfAttempts(numberOfAttempts) + .build(); + } + + @Nonnull + public static ReadWriteOptionsBuilder readWriteOptionsBuilder() { + return Builder.readWriteBuilder(); + } + + @Nonnull + public static ReadOnlyOptionsBuilder readOnlyOptionsBuilder() { + return Builder.readOnlyBuilder(); + } + + public abstract static class Builder> { + @Nullable protected Executor executor; + + protected Builder(@Nullable Executor executor) { + this.executor = executor; + } + + @Nullable + public Executor getExecutor() { + return executor; + } + + @Nonnull + @SuppressWarnings("unchecked") + public B setExecutor(@Nullable Executor executor) { + this.executor = executor; + return (B) this; + } + + @Nonnull + public abstract TransactionOptions build(); + + static ReadOnlyOptionsBuilder readOnlyBuilder() { + return new ReadOnlyOptionsBuilder(null, null); + } + + static ReadWriteOptionsBuilder readWriteBuilder() { + return new ReadWriteOptionsBuilder(null, DEFAULT_NUM_ATTEMPTS); + } + } + + public static final class ReadOnlyOptionsBuilder + extends Builder { + @Nullable private TimestampOrBuilder readTime; + + private ReadOnlyOptionsBuilder(@Nullable Executor executor, @Nullable Timestamp readTime) { + super(executor); + this.readTime = readTime; + } + + @Nullable + public TimestampOrBuilder getReadTime() { + return readTime; + } + + public ReadOnlyOptionsBuilder setReadTime(@Nullable TimestampOrBuilder readTime) { + this.readTime = readTime; + return this; + } + + @Nonnull + @Override + public TransactionOptions build() { + final Timestamp timestamp; + if (readTime != null && readTime instanceof Timestamp.Builder) { + timestamp = ((Timestamp.Builder) readTime).build(); + } else { + timestamp = (Timestamp) readTime; + } + return new TransactionOptions( + executor, EitherReadOnlyOrReadWrite.readOnly(new ReadOnlyOptions(timestamp))); + } + } + + public static final class ReadWriteOptionsBuilder + extends Builder { + private int numberOfAttempts; + + private ReadWriteOptionsBuilder(@Nullable Executor executor, int numberOfAttempts) { + super(executor); + this.numberOfAttempts = numberOfAttempts; + } + + public int getNumberOfAttempts() { + return numberOfAttempts; + } + + @Nonnull + public ReadWriteOptionsBuilder setNumberOfAttempts(int numberOfAttempts) { + Preconditions.checkArgument(numberOfAttempts > 0, "You must allow at least one attempt"); + this.numberOfAttempts = numberOfAttempts; + return this; + } + + @Nonnull + @Override + public TransactionOptions build() { + return new TransactionOptions( + executor, EitherReadOnlyOrReadWrite.readWrite(new ReadWriteOptions(numberOfAttempts))); + } + } + + // TODO: Refactor to use Optional when java7 support is dropped + static final class EitherReadOnlyOrReadWrite { + private final TransactionOptionsType type; + private final ReadOnlyOptions readOnly; + private final ReadWriteOptions readWrite; + + private EitherReadOnlyOrReadWrite( + TransactionOptionsType type, ReadOnlyOptions readOnly, ReadWriteOptions readWrite) { + this.type = type; + this.readOnly = readOnly; + this.readWrite = readWrite; + } + + public TransactionOptionsType getType() { + return type; + } + + ReadOnlyOptions getReadOnly() { + Preconditions.checkState( + readOnly != null && readWrite == null, "Unable to call getReadOnly for ReadWriteOptions"); + return readOnly; + } + + ReadWriteOptions getReadWrite() { + Preconditions.checkState( + readWrite != null && readOnly == null, "Unable to call getReadWrite for ReadOnlyOptions"); + return readWrite; + } + + static EitherReadOnlyOrReadWrite readOnly(ReadOnlyOptions readOnlyOptions) { + return new EitherReadOnlyOrReadWrite(TransactionOptionsType.READ_ONLY, readOnlyOptions, null); + } + + static EitherReadOnlyOrReadWrite readWrite(ReadWriteOptions readWriteOptions) { + return new EitherReadOnlyOrReadWrite( + TransactionOptionsType.READ_WRITE, null, readWriteOptions); + } + } + + enum TransactionOptionsType { + READ_ONLY, + READ_WRITE + } + + interface ToProtoBuilder { + ProtoBuilder toProtoBuilder(); + } + + static final class ReadOnlyOptions implements ToProtoBuilder { + @Nullable private final Timestamp readTime; + + private ReadOnlyOptions(@Nullable Timestamp readTime) { + this.readTime = readTime; + } + + @Override + public ReadOnly.Builder toProtoBuilder() { + if (readTime != null) { + return ReadOnly.getDefaultInstance().toBuilder().setReadTime(readTime); + } else { + return ReadOnly.getDefaultInstance().toBuilder(); + } + } + + @Nullable + public Timestamp getReadTime() { + return readTime; + } + } + + static final class ReadWriteOptions implements ToProtoBuilder { + private final int numberOfAttempts; + + private ReadWriteOptions(int numberOfAttempts) { + this.numberOfAttempts = numberOfAttempts; + } + + @Override + public ReadWrite.Builder toProtoBuilder() { + return ReadWrite.getDefaultInstance().toBuilder(); + } + + public int getNumberOfAttempts() { + return numberOfAttempts; + } } } 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 a6724c654..fe83ae785 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 @@ -26,8 +26,10 @@ 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.EitherReadOnlyOrReadWrite; import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.MoreExecutors; +import io.grpc.Context; import io.opencensus.trace.AttributeValue; import io.opencensus.trace.Span; import io.opencensus.trace.Tracer; @@ -58,10 +60,11 @@ class TransactionRunner { private final Transaction.AsyncFunction userCallback; private final Span span; - private final FirestoreImpl firestoreClient; + private final FirestoreImpl firestore; private final ScheduledExecutorService firestoreExecutor; private final Executor userCallbackExecutor; private final ExponentialRetryAlgorithm backoffAlgorithm; + private final TransactionOptions transactionOptions; private TimedAttemptSettings nextBackoffAttempt; private Transaction transaction; private int attemptsRemaining; @@ -69,20 +72,33 @@ class TransactionRunner { /** * @param firestore The active Firestore instance * @param userCallback The user provided transaction callback - * @param userCallbackExecutor The executor to run the user callback on - * @param numberOfAttempts The total number of attempts for this transaction + * @param transactionOptions The set options determining which executor the {@code userCallback} + * is run on, whether the transaction is read-write or read-only */ TransactionRunner( FirestoreImpl firestore, Transaction.AsyncFunction userCallback, - Executor userCallbackExecutor, - int numberOfAttempts) { + TransactionOptions transactionOptions) { + this.transactionOptions = transactionOptions; this.span = tracer.spanBuilder("CloudFirestore.Transaction").startSpan(); - this.firestoreClient = firestore; + this.firestore = firestore; this.firestoreExecutor = firestore.getClient().getExecutor(); this.userCallback = userCallback; - this.attemptsRemaining = numberOfAttempts; - this.userCallbackExecutor = userCallbackExecutor; + final EitherReadOnlyOrReadWrite options = transactionOptions.getOptions(); + switch (options.getType()) { + case READ_WRITE: + this.attemptsRemaining = options.getReadWrite().getNumberOfAttempts(); + break; + case READ_ONLY: + this.attemptsRemaining = 1; + break; + } + this.userCallbackExecutor = + Context.currentContextExecutor( + transactionOptions.getExecutor() != null + ? transactionOptions.getExecutor() + : this.firestore.getClient().getExecutor()); + this.backoffAlgorithm = new ExponentialRetryAlgorithm( firestore.getOptions().getRetrySettings(), CurrentMillisClock.getDefaultClock()); @@ -90,7 +106,7 @@ class TransactionRunner { } ApiFuture run() { - this.transaction = new Transaction(firestoreClient, this.transaction); + this.transaction = new Transaction(firestore, transactionOptions, this.transaction); --attemptsRemaining; @@ -186,7 +202,7 @@ public ApiFuture apply(T userFunctionResult) { /** The callback that is invoked after the Commit RPC returns. It returns the user result. */ private class CommitTransactionCallback implements ApiFunction, T> { - private T userFunctionResult; + private final T userFunctionResult; CommitTransactionCallback(T userFunctionResult) { this.userFunctionResult = userFunctionResult; 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 ac3767c76..2fb944ec1 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 @@ -34,12 +34,14 @@ import static com.google.cloud.firestore.LocalFirestoreHelper.rollbackResponse; import static com.google.cloud.firestore.LocalFirestoreHelper.set; import static com.google.cloud.firestore.LocalFirestoreHelper.update; +import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; @@ -51,9 +53,17 @@ import com.google.api.gax.rpc.UnaryCallable; import com.google.cloud.Timestamp; import com.google.cloud.firestore.LocalFirestoreHelper.ResponseStubber; +import com.google.cloud.firestore.TransactionOptions.EitherReadOnlyOrReadWrite; +import com.google.cloud.firestore.TransactionOptions.ReadOnlyOptions; +import com.google.cloud.firestore.TransactionOptions.ReadOnlyOptionsBuilder; +import com.google.cloud.firestore.TransactionOptions.ReadWriteOptions; +import com.google.cloud.firestore.TransactionOptions.ReadWriteOptionsBuilder; +import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; import com.google.cloud.firestore.spi.v1.FirestoreRpc; import com.google.firestore.v1.BatchGetDocumentsRequest; import com.google.firestore.v1.DocumentMask; +import com.google.firestore.v1.TransactionOptions.ReadOnly; +import com.google.firestore.v1.TransactionOptions.ReadWrite; import com.google.firestore.v1.Write; import com.google.protobuf.GeneratedMessageV3; import com.google.protobuf.Message; @@ -64,6 +74,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; import java.util.concurrent.Executors; import java.util.concurrent.ThreadFactory; import java.util.concurrent.atomic.AtomicInteger; @@ -74,10 +85,10 @@ import org.mockito.ArgumentCaptor; import org.mockito.Captor; import org.mockito.Matchers; -import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; +@SuppressWarnings("deprecation") @RunWith(MockitoJUnitRunner.class) public class TransactionTest { @@ -86,7 +97,7 @@ public class TransactionTest { new ApiException( new Exception("Test exception"), GrpcStatusCode.of(Status.Code.UNKNOWN), true)); - @Spy private FirestoreRpc firestoreRpc = Mockito.mock(FirestoreRpc.class); + @Spy private FirestoreRpc firestoreRpc = mock(FirestoreRpc.class); @Spy private FirestoreImpl firestoreMock = @@ -865,4 +876,113 @@ public String updateCallback(Transaction transaction) { assertEquals(begin(), requests.get(0)); assertEquals(commit(TRANSACTION_ID, writes.toArray(new Write[] {})), requests.get(1)); } + + @Test + public void readOnlyTransactionOptionsBuilder_setReadTime() { + Executor executor = mock(Executor.class); + final com.google.protobuf.Timestamp.Builder readTime = + com.google.protobuf.Timestamp.getDefaultInstance().toBuilder().setSeconds(1).setNanos(0); + final ReadOnlyOptionsBuilder builder = + TransactionOptions.readOnlyOptionsBuilder().setExecutor(executor).setReadTime(readTime); + final ReadOnly expectedReadOnly = ReadOnly.newBuilder().setReadTime(readTime).build(); + + final TransactionOptions transactionOptions = builder.build(); + + assertThat(builder.getExecutor()).isSameInstanceAs(executor); + assertThat(builder.getReadTime()).isSameInstanceAs(readTime); + + assertThat(transactionOptions.getExecutor()).isSameInstanceAs(executor); + final EitherReadOnlyOrReadWrite options = transactionOptions.getOptions(); + + assertThat(options.getType()).isEqualTo(TransactionOptionsType.READ_ONLY); + final ReadOnlyOptions readOnly = options.getReadOnly(); + // actually build the builder so we get a useful .equals method + assertThat(readOnly.toProtoBuilder().build()).isEqualTo(expectedReadOnly); + } + + @Test + public void readOnlyTransactionOptionsBuilder_defaults() { + final ReadOnlyOptionsBuilder builder = TransactionOptions.readOnlyOptionsBuilder(); + final ReadOnly expectedReadOnly = ReadOnly.newBuilder().build(); + + final TransactionOptions transactionOptions = builder.build(); + + assertThat(builder.getExecutor()).isNull(); + assertThat(builder.getReadTime()).isNull(); + + final ReadOnlyOptions readOnly = transactionOptions.getOptions().getReadOnly(); + assertThat(readOnly.getReadTime()).isNull(); + // actually build the builder so we get a useful .equals method + assertThat(readOnly.toProtoBuilder().build()).isEqualTo(expectedReadOnly); + } + + @Test + public void readOnlyTransactionOptionsBuilder_errorWhenGettingReadWrite() { + final ReadOnlyOptionsBuilder builder = TransactionOptions.readOnlyOptionsBuilder(); + try { + //noinspection ResultOfMethodCallIgnored + builder.build().getOptions().getReadWrite(); + fail("Error expected"); + } catch (IllegalStateException ignore) { + // expected + } + } + + @Test + public void readWriteTransactionOptionsBuilder_setNumberOfAttempts() { + Executor executor = mock(Executor.class); + final ReadWriteOptionsBuilder builder = + TransactionOptions.readWriteOptionsBuilder().setExecutor(executor).setNumberOfAttempts(2); + final ReadWrite expectedReadWrite = ReadWrite.newBuilder().build(); + + final TransactionOptions transactionOptions = builder.build(); + + assertThat(builder.getExecutor()).isSameInstanceAs(executor); + assertThat(builder.getNumberOfAttempts()).isEqualTo(2); + + assertThat(transactionOptions.getExecutor()).isSameInstanceAs(executor); + final EitherReadOnlyOrReadWrite options = transactionOptions.getOptions(); + + assertThat(options.getType()).isEqualTo(TransactionOptionsType.READ_WRITE); + final ReadWriteOptions readWrite = options.getReadWrite(); + // actually build the builder so we get a useful .equals method + assertThat(readWrite.toProtoBuilder().build()).isEqualTo(expectedReadWrite); + } + + @Test + public void readWriteTransactionOptionsBuilder_defaults() { + final ReadWrite expectedReadWrite = ReadWrite.newBuilder().build(); + + final TransactionOptions transactionOptions = + TransactionOptions.readWriteOptionsBuilder().build(); + final ReadWriteOptions readWrite = transactionOptions.getOptions().getReadWrite(); + + assertThat(transactionOptions.getExecutor()).isNull(); + assertThat(readWrite.getNumberOfAttempts()).isEqualTo(5); + + // actually build the builder so we get a useful .equals method + assertThat(readWrite.toProtoBuilder().build()).isEqualTo(expectedReadWrite); + } + + @Test + public void readWriteTransactionOptionsBuilder_errorWhenGettingReadWrite() { + final ReadWriteOptionsBuilder builder = TransactionOptions.readWriteOptionsBuilder(); + try { + //noinspection ResultOfMethodCallIgnored + builder.build().getOptions().getReadOnly(); + fail("Error expected"); + } catch (IllegalStateException ignore) { + // expected + } + } + + @Test + public void readWriteTransactionOptionsBuilder_errorAttemptingToSetNumAttemptsLessThanOne() { + try { + TransactionOptions.readWriteOptionsBuilder().setNumberOfAttempts(0); + fail("Error expected"); + } catch (IllegalArgumentException ignore) { + // expected + } + } } 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 d2a768181..a8ad08b8e 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 @@ -18,6 +18,7 @@ import static com.google.cloud.firestore.LocalFirestoreHelper.UPDATE_SINGLE_FIELD_OBJECT; import static com.google.cloud.firestore.LocalFirestoreHelper.map; +import static com.google.common.truth.Truth.assertThat; import static java.util.Arrays.asList; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; @@ -56,12 +57,16 @@ import com.google.cloud.firestore.SetOptions; import com.google.cloud.firestore.Transaction; import com.google.cloud.firestore.Transaction.Function; +import com.google.cloud.firestore.TransactionOptions; import com.google.cloud.firestore.WriteBatch; import com.google.cloud.firestore.WriteResult; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.firestore.v1.RunQueryRequest; +import io.grpc.Status; +import io.grpc.Status.Code; +import io.grpc.StatusRuntimeException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -72,8 +77,12 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -1414,6 +1423,93 @@ public void bulkWriterWritesInOrder() throws Exception { assertEquals(Collections.singletonMap("foo", "bar3"), result.get().getData()); } + @Test + public void readOnlyTransaction_successfulGet() + throws ExecutionException, InterruptedException, TimeoutException { + final DocumentReference documentReference = randomColl.add(SINGLE_FIELD_MAP).get(); + + final AtomicReference ref = new AtomicReference<>(); + + final ApiFuture runTransaction = + firestore.runTransaction( + new Function() { + @Override + public Void updateCallback(Transaction transaction) throws Exception { + final DocumentSnapshot snapshot = + transaction.get(documentReference).get(5, TimeUnit.SECONDS); + ref.compareAndSet(null, snapshot); + return null; + } + }, + TransactionOptions.readOnlyOptionsBuilder().build()); + + runTransaction.get(10, TimeUnit.SECONDS); + assertEquals("bar", ref.get().get("foo")); + } + + @Test + public void readOnlyTransaction_faliureWhenAttemptingWrite() + throws InterruptedException, TimeoutException { + + final DocumentReference documentReference = randomColl.document("tx/ro/writeShouldFail"); + final ApiFuture runTransaction = + firestore.runTransaction( + new Function() { + @Override + public Void updateCallback(Transaction transaction) { + transaction.set(documentReference, SINGLE_FIELD_MAP); + return null; + } + }, + TransactionOptions.readOnlyOptionsBuilder().build()); + + try { + 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"); + } + } + + @Test + public void readOnlyTransaction_failureWhenAttemptReadOlderThan60Seconds() + throws ExecutionException, InterruptedException, TimeoutException { + final DocumentReference documentReference = randomColl.add(SINGLE_FIELD_MAP).get(); + + final TransactionOptions options = + TransactionOptions.readOnlyOptionsBuilder() + .setReadTime(com.google.protobuf.Timestamp.newBuilder().setSeconds(1).setNanos(0)) + .build(); + + final ApiFuture runTransaction = + firestore.runTransaction( + new Function() { + @Override + public Void updateCallback(Transaction transaction) throws Exception { + transaction.get(documentReference).get(5, TimeUnit.SECONDS); + return null; + } + }, + options); + + try { + runTransaction.get(10, TimeUnit.SECONDS); + } catch (ExecutionException e) { + final Throwable rootCause = ExceptionUtils.getRootCause(e); + assertThat(rootCause).isInstanceOf(StatusRuntimeException.class); + final StatusRuntimeException invalidArgument = (StatusRuntimeException) rootCause; + final Status status = invalidArgument.getStatus(); + assertThat(status.getCode()).isEqualTo(Code.FAILED_PRECONDITION); + assertThat(status.getDescription()).contains("old"); + } + } + /** Wrapper around ApiStreamObserver that returns the results in a list. */ private static class StreamConsumer implements ApiStreamObserver { SettableApiFuture> done = SettableApiFuture.create(); From 0785374e78da1ca4014ee0357566afa80dd617bb Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Thu, 6 Aug 2020 18:19:16 -0400 Subject: [PATCH 02/15] fix deps declaration --- google-cloud-firestore/pom.xml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/google-cloud-firestore/pom.xml b/google-cloud-firestore/pom.xml index 5e0018903..175b4470a 100644 --- a/google-cloud-firestore/pom.xml +++ b/google-cloud-firestore/pom.xml @@ -152,6 +152,12 @@ 2.11.2 test + + org.apache.commons + commons-lang3 + 3.11 + test + From b6717fda0acfd7ed4db279e4f290b54ec1eb57a5 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Fri, 7 Aug 2020 18:33:46 -0400 Subject: [PATCH 03/15] docs: minor javadoc fixes after review --- .../cloud/firestore/TransactionOptions.java | 15 +++++++-------- .../google/cloud/firestore/TransactionRunner.java | 4 ++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 705606165..76392688e 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -43,9 +43,9 @@ public final class TransactionOptions { } /** - * @return int Representing the initial number of attempts a read-write transaction will be + * @return the initial number of attempts a read-write transaction will be * attempted - * @deprecated As of v2.0.0, only applicable to Read-Write transactions. Use {@link + * @deprecated as of v2.0.0, only applicable to Read-Write transactions. Use {@link * ReadWriteOptions#getNumberOfAttempts()} instead */ @Deprecated @@ -58,7 +58,7 @@ public int getNumberOfAttempts() { } } - /** @return Executor Executor to be used to run user callbacks on */ + /** @return Executor to be used to run user callbacks on */ @Nullable public Executor getExecutor() { return executor; @@ -71,8 +71,7 @@ EitherReadOnlyOrReadWrite getOptions() { } /** - * Create a default set of ReadWrite options suitable for most use cases. Transactions will be - * attempted 5 times. + * Create a default set of options suitable for most use cases. Transactions will be opened as ReadWrite transactions and attempted up to 5 times. * * @return The TransactionOptions object. * @see #readWriteOptionsBuilder() @@ -87,7 +86,7 @@ public static TransactionOptions create() { * * @param numberOfAttempts The number of execution attempts. * @return The TransactionOptions object. - * @deprecated As of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} + * @deprecated as of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} * @see #readWriteOptionsBuilder() */ @Nonnull @@ -101,7 +100,7 @@ public static TransactionOptions create(int numberOfAttempts) { * * @param executor The executor to run the user callback code on. * @return The TransactionOptions object. - * @deprecated As of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} + * @deprecated as of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} * @see #readWriteOptionsBuilder() */ @Nonnull @@ -116,7 +115,7 @@ public static TransactionOptions create(@Nullable Executor executor) { * @param executor The executor to run the user callback code on. * @param numberOfAttempts The number of execution attempts. * @return The TransactionOptions object. - * @deprecated As of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} and + * @deprecated as of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} and * {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} * @see #readWriteOptionsBuilder() */ 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 fe83ae785..87296a288 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 @@ -72,8 +72,8 @@ class TransactionRunner { /** * @param firestore The active Firestore instance * @param userCallback The user provided transaction callback - * @param transactionOptions The set options determining which executor the {@code userCallback} - * is run on, whether the transaction is read-write or read-only + * @param transactionOptions The options determining which executor the {@code userCallback} + * is run on and whether the transaction is read-write or read-only */ TransactionRunner( FirestoreImpl firestore, From 2492652f913d22426f8532928304182a3afff66c Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Fri, 7 Aug 2020 18:47:24 -0400 Subject: [PATCH 04/15] fix: remove superfluous EitherReadOnlyOrReadWrite class --- .../google/cloud/firestore/Transaction.java | 8 +- .../cloud/firestore/TransactionOptions.java | 82 ++++++++----------- .../cloud/firestore/TransactionRunner.java | 6 +- .../cloud/firestore/TransactionTest.java | 17 ++-- 4 files changed, 46 insertions(+), 67 deletions(-) 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 0ab6e9bf9..94d8acee3 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 @@ -19,7 +19,6 @@ import com.google.api.core.ApiFunction; import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; -import com.google.cloud.firestore.TransactionOptions.EitherReadOnlyOrReadWrite; import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; @@ -87,14 +86,13 @@ ApiFuture begin() { BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); beginTransaction.setDatabase(firestore.getDatabaseName()); - final EitherReadOnlyOrReadWrite options = originalTransactionOptions.getOptions(); - if (options.getType() == TransactionOptionsType.READ_WRITE && previousTransactionId != null) { + if (originalTransactionOptions.getType() == TransactionOptionsType.READ_WRITE && previousTransactionId != null) { beginTransaction .getOptionsBuilder() .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); - } else if (options.getType() == TransactionOptionsType.READ_ONLY) { - final ReadOnly.Builder builder = options.getReadOnly().toProtoBuilder(); + } else if (originalTransactionOptions.getType() == TransactionOptionsType.READ_ONLY) { + final ReadOnly.Builder builder = originalTransactionOptions.getReadOnly().toProtoBuilder(); beginTransaction.getOptionsBuilder().setReadOnly(builder); } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 76392688e..834424ed5 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -35,11 +35,18 @@ public final class TransactionOptions { private static final int DEFAULT_NUM_ATTEMPTS = 5; private final Executor executor; - private final EitherReadOnlyOrReadWrite options; - - TransactionOptions(Executor executor, EitherReadOnlyOrReadWrite options) { + private final TransactionOptionsType type; + private final ReadOnlyOptions readOnly; + private final ReadWriteOptions readWrite; + + TransactionOptions(Executor executor, + TransactionOptionsType type, + ReadOnlyOptions readOnly, + ReadWriteOptions readWrite) { this.executor = executor; - this.options = options; + this.type = type; + this.readOnly = readOnly; + this.readWrite = readWrite; } /** @@ -51,8 +58,8 @@ public final class TransactionOptions { @Deprecated @InternalApi public int getNumberOfAttempts() { - if (options.getType() == TransactionOptionsType.READ_WRITE) { - return options.getReadWrite().numberOfAttempts; + if (type == TransactionOptionsType.READ_WRITE) { + return readWrite.numberOfAttempts; } else { return 1; } @@ -66,8 +73,24 @@ public Executor getExecutor() { @Nonnull @InternalApi - EitherReadOnlyOrReadWrite getOptions() { - return options; + TransactionOptionsType getType() { + return type; + } + + @Nonnull + @InternalApi + ReadOnlyOptions getReadOnly() { + Preconditions.checkState( + readOnly != null && readWrite == null, "Unable to call getReadOnly for ReadWriteOptions"); + return readOnly; + } + + @Nonnull + @InternalApi + ReadWriteOptions getReadWrite() { + Preconditions.checkState( + readWrite != null && readOnly == null, "Unable to call getReadWrite for ReadOnlyOptions"); + return readWrite; } /** @@ -198,7 +221,7 @@ public TransactionOptions build() { timestamp = (Timestamp) readTime; } return new TransactionOptions( - executor, EitherReadOnlyOrReadWrite.readOnly(new ReadOnlyOptions(timestamp))); + executor, TransactionOptionsType.READ_ONLY, new ReadOnlyOptions(timestamp), null); } } @@ -226,46 +249,7 @@ public ReadWriteOptionsBuilder setNumberOfAttempts(int numberOfAttempts) { @Override public TransactionOptions build() { return new TransactionOptions( - executor, EitherReadOnlyOrReadWrite.readWrite(new ReadWriteOptions(numberOfAttempts))); - } - } - - // TODO: Refactor to use Optional when java7 support is dropped - static final class EitherReadOnlyOrReadWrite { - private final TransactionOptionsType type; - private final ReadOnlyOptions readOnly; - private final ReadWriteOptions readWrite; - - private EitherReadOnlyOrReadWrite( - TransactionOptionsType type, ReadOnlyOptions readOnly, ReadWriteOptions readWrite) { - this.type = type; - this.readOnly = readOnly; - this.readWrite = readWrite; - } - - public TransactionOptionsType getType() { - return type; - } - - ReadOnlyOptions getReadOnly() { - Preconditions.checkState( - readOnly != null && readWrite == null, "Unable to call getReadOnly for ReadWriteOptions"); - return readOnly; - } - - ReadWriteOptions getReadWrite() { - Preconditions.checkState( - readWrite != null && readOnly == null, "Unable to call getReadWrite for ReadOnlyOptions"); - return readWrite; - } - - static EitherReadOnlyOrReadWrite readOnly(ReadOnlyOptions readOnlyOptions) { - return new EitherReadOnlyOrReadWrite(TransactionOptionsType.READ_ONLY, readOnlyOptions, null); - } - - static EitherReadOnlyOrReadWrite readWrite(ReadWriteOptions readWriteOptions) { - return new EitherReadOnlyOrReadWrite( - TransactionOptionsType.READ_WRITE, null, readWriteOptions); + executor, TransactionOptionsType.READ_WRITE, null, new ReadWriteOptions(numberOfAttempts)); } } 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 87296a288..8c6c49939 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 @@ -26,7 +26,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.EitherReadOnlyOrReadWrite; import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.MoreExecutors; import io.grpc.Context; @@ -84,10 +83,9 @@ class TransactionRunner { this.firestore = firestore; this.firestoreExecutor = firestore.getClient().getExecutor(); this.userCallback = userCallback; - final EitherReadOnlyOrReadWrite options = transactionOptions.getOptions(); - switch (options.getType()) { + switch (transactionOptions.getType()) { case READ_WRITE: - this.attemptsRemaining = options.getReadWrite().getNumberOfAttempts(); + this.attemptsRemaining = transactionOptions.getReadWrite().getNumberOfAttempts(); break; case READ_ONLY: this.attemptsRemaining = 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 2fb944ec1..e33d08876 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 @@ -53,7 +53,6 @@ import com.google.api.gax.rpc.UnaryCallable; import com.google.cloud.Timestamp; import com.google.cloud.firestore.LocalFirestoreHelper.ResponseStubber; -import com.google.cloud.firestore.TransactionOptions.EitherReadOnlyOrReadWrite; import com.google.cloud.firestore.TransactionOptions.ReadOnlyOptions; import com.google.cloud.firestore.TransactionOptions.ReadOnlyOptionsBuilder; import com.google.cloud.firestore.TransactionOptions.ReadWriteOptions; @@ -71,6 +70,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; @@ -87,6 +87,7 @@ import org.mockito.Matchers; import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; +import org.mockito.stubbing.Stubber; @SuppressWarnings("deprecation") @RunWith(MockitoJUnitRunner.class) @@ -892,9 +893,8 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { assertThat(builder.getReadTime()).isSameInstanceAs(readTime); assertThat(transactionOptions.getExecutor()).isSameInstanceAs(executor); - final EitherReadOnlyOrReadWrite options = transactionOptions.getOptions(); - assertThat(options.getType()).isEqualTo(TransactionOptionsType.READ_ONLY); + assertThat(transactionOptions.getType()).isEqualTo(TransactionOptionsType.READ_ONLY); final ReadOnlyOptions readOnly = options.getReadOnly(); // actually build the builder so we get a useful .equals method assertThat(readOnly.toProtoBuilder().build()).isEqualTo(expectedReadOnly); @@ -910,7 +910,7 @@ public void readOnlyTransactionOptionsBuilder_defaults() { assertThat(builder.getExecutor()).isNull(); assertThat(builder.getReadTime()).isNull(); - final ReadOnlyOptions readOnly = transactionOptions.getOptions().getReadOnly(); + final ReadOnlyOptions readOnly = transactionOptions.getReadOnly(); assertThat(readOnly.getReadTime()).isNull(); // actually build the builder so we get a useful .equals method assertThat(readOnly.toProtoBuilder().build()).isEqualTo(expectedReadOnly); @@ -921,7 +921,7 @@ public void readOnlyTransactionOptionsBuilder_errorWhenGettingReadWrite() { final ReadOnlyOptionsBuilder builder = TransactionOptions.readOnlyOptionsBuilder(); try { //noinspection ResultOfMethodCallIgnored - builder.build().getOptions().getReadWrite(); + builder.build().getReadWrite(); fail("Error expected"); } catch (IllegalStateException ignore) { // expected @@ -941,9 +941,8 @@ public void readWriteTransactionOptionsBuilder_setNumberOfAttempts() { assertThat(builder.getNumberOfAttempts()).isEqualTo(2); assertThat(transactionOptions.getExecutor()).isSameInstanceAs(executor); - final EitherReadOnlyOrReadWrite options = transactionOptions.getOptions(); - assertThat(options.getType()).isEqualTo(TransactionOptionsType.READ_WRITE); + assertThat(transactionOptions.getType()).isEqualTo(TransactionOptionsType.READ_WRITE); final ReadWriteOptions readWrite = options.getReadWrite(); // actually build the builder so we get a useful .equals method assertThat(readWrite.toProtoBuilder().build()).isEqualTo(expectedReadWrite); @@ -955,7 +954,7 @@ public void readWriteTransactionOptionsBuilder_defaults() { final TransactionOptions transactionOptions = TransactionOptions.readWriteOptionsBuilder().build(); - final ReadWriteOptions readWrite = transactionOptions.getOptions().getReadWrite(); + final ReadWriteOptions readWrite = transactionOptions.getReadWrite(); assertThat(transactionOptions.getExecutor()).isNull(); assertThat(readWrite.getNumberOfAttempts()).isEqualTo(5); @@ -969,7 +968,7 @@ public void readWriteTransactionOptionsBuilder_errorWhenGettingReadWrite() { final ReadWriteOptionsBuilder builder = TransactionOptions.readWriteOptionsBuilder(); try { //noinspection ResultOfMethodCallIgnored - builder.build().getOptions().getReadOnly(); + builder.build().getReadOnly(); fail("Error expected"); } catch (IllegalStateException ignore) { // expected From 12d2f32afe63ec101c2a4ee1de795731b396a1f4 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Fri, 7 Aug 2020 18:54:40 -0400 Subject: [PATCH 05/15] chore: formatting --- .../com/google/cloud/firestore/Transaction.java | 3 ++- .../google/cloud/firestore/TransactionOptions.java | 14 +++++++++----- .../google/cloud/firestore/TransactionRunner.java | 4 ++-- 3 files changed, 13 insertions(+), 8 deletions(-) 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 94d8acee3..144810e33 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 @@ -86,7 +86,8 @@ ApiFuture begin() { BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); beginTransaction.setDatabase(firestore.getDatabaseName()); - if (originalTransactionOptions.getType() == TransactionOptionsType.READ_WRITE && previousTransactionId != null) { + if (originalTransactionOptions.getType() == TransactionOptionsType.READ_WRITE + && previousTransactionId != null) { beginTransaction .getOptionsBuilder() .getReadWriteBuilder() diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 834424ed5..c19e82f76 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -39,7 +39,8 @@ public final class TransactionOptions { private final ReadOnlyOptions readOnly; private final ReadWriteOptions readWrite; - TransactionOptions(Executor executor, + TransactionOptions( + Executor executor, TransactionOptionsType type, ReadOnlyOptions readOnly, ReadWriteOptions readWrite) { @@ -50,8 +51,7 @@ public final class TransactionOptions { } /** - * @return the initial number of attempts a read-write transaction will be - * attempted + * @return the initial number of attempts a read-write transaction will be attempted * @deprecated as of v2.0.0, only applicable to Read-Write transactions. Use {@link * ReadWriteOptions#getNumberOfAttempts()} instead */ @@ -94,7 +94,8 @@ ReadWriteOptions getReadWrite() { } /** - * Create a default set of options suitable for most use cases. Transactions will be opened as ReadWrite transactions and attempted up to 5 times. + * Create a default set of options suitable for most use cases. Transactions will be opened as + * ReadWrite transactions and attempted up to 5 times. * * @return The TransactionOptions object. * @see #readWriteOptionsBuilder() @@ -249,7 +250,10 @@ public ReadWriteOptionsBuilder setNumberOfAttempts(int numberOfAttempts) { @Override public TransactionOptions build() { return new TransactionOptions( - executor, TransactionOptionsType.READ_WRITE, null, new ReadWriteOptions(numberOfAttempts)); + executor, + TransactionOptionsType.READ_WRITE, + null, + new ReadWriteOptions(numberOfAttempts)); } } 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 8c6c49939..8a3a954aa 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 @@ -71,8 +71,8 @@ class TransactionRunner { /** * @param firestore The active Firestore instance * @param userCallback The user provided transaction callback - * @param transactionOptions The options determining which executor the {@code userCallback} - * is run on and whether the transaction is read-write or read-only + * @param transactionOptions The options determining which executor the {@code userCallback} is + * run on and whether the transaction is read-write or read-only */ TransactionRunner( FirestoreImpl firestore, From b383e4de3df1b2531b0fbb599516d2de0e3f7fa4 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Fri, 7 Aug 2020 19:05:44 -0400 Subject: [PATCH 06/15] fix: typo during refactor --- .../test/java/com/google/cloud/firestore/TransactionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e33d08876..e597582c5 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 @@ -895,7 +895,7 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { assertThat(transactionOptions.getExecutor()).isSameInstanceAs(executor); assertThat(transactionOptions.getType()).isEqualTo(TransactionOptionsType.READ_ONLY); - final ReadOnlyOptions readOnly = options.getReadOnly(); + final ReadOnlyOptions readOnly = transactionOptions.getReadOnly(); // actually build the builder so we get a useful .equals method assertThat(readOnly.toProtoBuilder().build()).isEqualTo(expectedReadOnly); } From 66be9a25aae1340dfb1a136b11e478b3992741bb Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Mon, 10 Aug 2020 17:58:35 -0400 Subject: [PATCH 07/15] fix some scoping to allow reading of options after built --- .../cloud/firestore/TransactionOptions.java | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index c19e82f76..6ec2c40d6 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -72,22 +72,19 @@ public Executor getExecutor() { } @Nonnull - @InternalApi - TransactionOptionsType getType() { + public TransactionOptionsType getType() { return type; } @Nonnull - @InternalApi - ReadOnlyOptions getReadOnly() { + public ReadOnlyOptions getReadOnly() { Preconditions.checkState( readOnly != null && readWrite == null, "Unable to call getReadOnly for ReadWriteOptions"); return readOnly; } @Nonnull - @InternalApi - ReadWriteOptions getReadWrite() { + public ReadWriteOptions getReadWrite() { Preconditions.checkState( readWrite != null && readOnly == null, "Unable to call getReadWrite for ReadOnlyOptions"); return readWrite; @@ -162,7 +159,7 @@ public static ReadOnlyOptionsBuilder readOnlyOptionsBuilder() { return Builder.readOnlyBuilder(); } - public abstract static class Builder> { + public abstract static class Builder> { @Nullable protected Executor executor; protected Builder(@Nullable Executor executor) { @@ -193,8 +190,7 @@ static ReadWriteOptionsBuilder readWriteBuilder() { } } - public static final class ReadOnlyOptionsBuilder - extends Builder { + public static final class ReadOnlyOptionsBuilder extends Builder { @Nullable private TimestampOrBuilder readTime; private ReadOnlyOptionsBuilder(@Nullable Executor executor, @Nullable Timestamp readTime) { @@ -226,8 +222,7 @@ public TransactionOptions build() { } } - public static final class ReadWriteOptionsBuilder - extends Builder { + public static final class ReadWriteOptionsBuilder extends Builder { private int numberOfAttempts; private ReadWriteOptionsBuilder(@Nullable Executor executor, int numberOfAttempts) { @@ -257,16 +252,12 @@ public TransactionOptions build() { } } - enum TransactionOptionsType { + public enum TransactionOptionsType { READ_ONLY, READ_WRITE } - interface ToProtoBuilder { - ProtoBuilder toProtoBuilder(); - } - - static final class ReadOnlyOptions implements ToProtoBuilder { + public static final class ReadOnlyOptions implements ToProtoBuilder { @Nullable private final Timestamp readTime; private ReadOnlyOptions(@Nullable Timestamp readTime) { @@ -288,7 +279,7 @@ public Timestamp getReadTime() { } } - static final class ReadWriteOptions implements ToProtoBuilder { + public static final class ReadWriteOptions implements ToProtoBuilder { private final int numberOfAttempts; private ReadWriteOptions(int numberOfAttempts) { @@ -304,4 +295,8 @@ public int getNumberOfAttempts() { return numberOfAttempts; } } + + interface ToProtoBuilder { + ProtoBuilder toProtoBuilder(); + } } From 6c507d18be9ce5b97f943523c0945aeed6d0bdbb Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Mon, 10 Aug 2020 17:55:17 -0400 Subject: [PATCH 08/15] fix: rename field --- .../java/com/google/cloud/firestore/Transaction.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 144810e33..241df4b46 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,7 +63,7 @@ public interface AsyncFunction { ApiFuture updateCallback(Transaction transaction); } - private final TransactionOptions originalTransactionOptions; + private final TransactionOptions transactionOptions; @Nullable private final ByteString previousTransactionId; private ByteString transactionId; @@ -72,7 +72,7 @@ public interface AsyncFunction { TransactionOptions transactionOptions, @Nullable Transaction previousTransaction) { super(firestore); - this.originalTransactionOptions = transactionOptions; + this.transactionOptions = transactionOptions; this.previousTransactionId = previousTransaction != null ? previousTransaction.transactionId : null; } @@ -86,14 +86,14 @@ ApiFuture begin() { BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); beginTransaction.setDatabase(firestore.getDatabaseName()); - if (originalTransactionOptions.getType() == TransactionOptionsType.READ_WRITE + if (transactionOptions.getType() == TransactionOptionsType.READ_WRITE && previousTransactionId != null) { beginTransaction .getOptionsBuilder() .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); - } else if (originalTransactionOptions.getType() == TransactionOptionsType.READ_ONLY) { - final ReadOnly.Builder builder = originalTransactionOptions.getReadOnly().toProtoBuilder(); + } else if (transactionOptions.getType() == TransactionOptionsType.READ_ONLY) { + final ReadOnly.Builder builder = transactionOptions.getReadOnly().toProtoBuilder(); beginTransaction.getOptionsBuilder().setReadOnly(builder); } From c0561665b8f3fd922a92abfd6c7ccc052cab189f Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Mon, 10 Aug 2020 17:56:54 -0400 Subject: [PATCH 09/15] fix: enum .equals --- .../src/main/java/com/google/cloud/firestore/Transaction.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 241df4b46..965fd9fdf 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 @@ -86,13 +86,13 @@ ApiFuture begin() { BeginTransactionRequest.Builder beginTransaction = BeginTransactionRequest.newBuilder(); beginTransaction.setDatabase(firestore.getDatabaseName()); - if (transactionOptions.getType() == TransactionOptionsType.READ_WRITE + if (TransactionOptionsType.READ_WRITE.equals(transactionOptions.getType()) && previousTransactionId != null) { beginTransaction .getOptionsBuilder() .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); - } else if (transactionOptions.getType() == TransactionOptionsType.READ_ONLY) { + } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType())) { final ReadOnly.Builder builder = transactionOptions.getReadOnly().toProtoBuilder(); beginTransaction.getOptionsBuilder().setReadOnly(builder); } From bf666665040bb0aa1b41f3db9bb6a4c7fc86fcc9 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Mon, 10 Aug 2020 18:00:04 -0400 Subject: [PATCH 10/15] fix: rename public builder factory methods --- .../cloud/firestore/TransactionOptions.java | 20 +++++++++---------- .../cloud/firestore/TransactionTest.java | 14 ++++++------- .../cloud/firestore/it/ITSystemTest.java | 6 +++--- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 6ec2c40d6..48dcef102 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -30,7 +30,7 @@ public final class TransactionOptions { private static final TransactionOptions DEFAULT_READ_WRITE_TRANSACTION_OPTIONS = - readWriteOptionsBuilder().build(); + createReadWriteOptionsBuilder().build(); private static final int DEFAULT_NUM_ATTEMPTS = 5; @@ -95,7 +95,7 @@ public ReadWriteOptions getReadWrite() { * ReadWrite transactions and attempted up to 5 times. * * @return The TransactionOptions object. - * @see #readWriteOptionsBuilder() + * @see #createReadWriteOptionsBuilder() */ @Nonnull public static TransactionOptions create() { @@ -108,12 +108,12 @@ public static TransactionOptions create() { * @param numberOfAttempts The number of execution attempts. * @return The TransactionOptions object. * @deprecated as of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} - * @see #readWriteOptionsBuilder() + * @see #createReadWriteOptionsBuilder() */ @Nonnull @Deprecated public static TransactionOptions create(int numberOfAttempts) { - return readWriteOptionsBuilder().setNumberOfAttempts(numberOfAttempts).build(); + return createReadWriteOptionsBuilder().setNumberOfAttempts(numberOfAttempts).build(); } /** @@ -122,12 +122,12 @@ public static TransactionOptions create(int numberOfAttempts) { * @param executor The executor to run the user callback code on. * @return The TransactionOptions object. * @deprecated as of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} - * @see #readWriteOptionsBuilder() + * @see #createReadWriteOptionsBuilder() */ @Nonnull @Deprecated public static TransactionOptions create(@Nullable Executor executor) { - return readWriteOptionsBuilder().setExecutor(executor).build(); + return createReadWriteOptionsBuilder().setExecutor(executor).build(); } /** @@ -138,24 +138,24 @@ public static TransactionOptions create(@Nullable Executor executor) { * @return The TransactionOptions object. * @deprecated as of 2.0.0, replaced by {@link ReadWriteOptionsBuilder#setExecutor(Executor)} and * {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} - * @see #readWriteOptionsBuilder() + * @see #createReadWriteOptionsBuilder() */ @Nonnull @Deprecated public static TransactionOptions create(@Nullable Executor executor, int numberOfAttempts) { - return readWriteOptionsBuilder() + return createReadWriteOptionsBuilder() .setExecutor(executor) .setNumberOfAttempts(numberOfAttempts) .build(); } @Nonnull - public static ReadWriteOptionsBuilder readWriteOptionsBuilder() { + public static ReadWriteOptionsBuilder createReadWriteOptionsBuilder() { return Builder.readWriteBuilder(); } @Nonnull - public static ReadOnlyOptionsBuilder readOnlyOptionsBuilder() { + public static ReadOnlyOptionsBuilder createReadOnlyOptionsBuilder() { return Builder.readOnlyBuilder(); } 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 e597582c5..83b85bb90 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 @@ -884,7 +884,7 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { final com.google.protobuf.Timestamp.Builder readTime = com.google.protobuf.Timestamp.getDefaultInstance().toBuilder().setSeconds(1).setNanos(0); final ReadOnlyOptionsBuilder builder = - TransactionOptions.readOnlyOptionsBuilder().setExecutor(executor).setReadTime(readTime); + TransactionOptions.createReadOnlyOptionsBuilder().setExecutor(executor).setReadTime(readTime); final ReadOnly expectedReadOnly = ReadOnly.newBuilder().setReadTime(readTime).build(); final TransactionOptions transactionOptions = builder.build(); @@ -902,7 +902,7 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { @Test public void readOnlyTransactionOptionsBuilder_defaults() { - final ReadOnlyOptionsBuilder builder = TransactionOptions.readOnlyOptionsBuilder(); + final ReadOnlyOptionsBuilder builder = TransactionOptions.createReadOnlyOptionsBuilder(); final ReadOnly expectedReadOnly = ReadOnly.newBuilder().build(); final TransactionOptions transactionOptions = builder.build(); @@ -918,7 +918,7 @@ public void readOnlyTransactionOptionsBuilder_defaults() { @Test public void readOnlyTransactionOptionsBuilder_errorWhenGettingReadWrite() { - final ReadOnlyOptionsBuilder builder = TransactionOptions.readOnlyOptionsBuilder(); + final ReadOnlyOptionsBuilder builder = TransactionOptions.createReadOnlyOptionsBuilder(); try { //noinspection ResultOfMethodCallIgnored builder.build().getReadWrite(); @@ -932,7 +932,7 @@ public void readOnlyTransactionOptionsBuilder_errorWhenGettingReadWrite() { public void readWriteTransactionOptionsBuilder_setNumberOfAttempts() { Executor executor = mock(Executor.class); final ReadWriteOptionsBuilder builder = - TransactionOptions.readWriteOptionsBuilder().setExecutor(executor).setNumberOfAttempts(2); + TransactionOptions.createReadWriteOptionsBuilder().setExecutor(executor).setNumberOfAttempts(2); final ReadWrite expectedReadWrite = ReadWrite.newBuilder().build(); final TransactionOptions transactionOptions = builder.build(); @@ -953,7 +953,7 @@ public void readWriteTransactionOptionsBuilder_defaults() { final ReadWrite expectedReadWrite = ReadWrite.newBuilder().build(); final TransactionOptions transactionOptions = - TransactionOptions.readWriteOptionsBuilder().build(); + TransactionOptions.createReadWriteOptionsBuilder().build(); final ReadWriteOptions readWrite = transactionOptions.getReadWrite(); assertThat(transactionOptions.getExecutor()).isNull(); @@ -965,7 +965,7 @@ public void readWriteTransactionOptionsBuilder_defaults() { @Test public void readWriteTransactionOptionsBuilder_errorWhenGettingReadWrite() { - final ReadWriteOptionsBuilder builder = TransactionOptions.readWriteOptionsBuilder(); + final ReadWriteOptionsBuilder builder = TransactionOptions.createReadWriteOptionsBuilder(); try { //noinspection ResultOfMethodCallIgnored builder.build().getReadOnly(); @@ -978,7 +978,7 @@ public void readWriteTransactionOptionsBuilder_errorWhenGettingReadWrite() { @Test public void readWriteTransactionOptionsBuilder_errorAttemptingToSetNumAttemptsLessThanOne() { try { - TransactionOptions.readWriteOptionsBuilder().setNumberOfAttempts(0); + TransactionOptions.createReadWriteOptionsBuilder().setNumberOfAttempts(0); fail("Error expected"); } catch (IllegalArgumentException ignore) { // expected 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 a8ad08b8e..56bdb9c01 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 @@ -1441,7 +1441,7 @@ public Void updateCallback(Transaction transaction) throws Exception { return null; } }, - TransactionOptions.readOnlyOptionsBuilder().build()); + TransactionOptions.createReadOnlyOptionsBuilder().build()); runTransaction.get(10, TimeUnit.SECONDS); assertEquals("bar", ref.get().get("foo")); @@ -1461,7 +1461,7 @@ public Void updateCallback(Transaction transaction) { return null; } }, - TransactionOptions.readOnlyOptionsBuilder().build()); + TransactionOptions.createReadOnlyOptionsBuilder().build()); try { runTransaction.get(10, TimeUnit.SECONDS); @@ -1483,7 +1483,7 @@ public void readOnlyTransaction_failureWhenAttemptReadOlderThan60Seconds() final DocumentReference documentReference = randomColl.add(SINGLE_FIELD_MAP).get(); final TransactionOptions options = - TransactionOptions.readOnlyOptionsBuilder() + TransactionOptions.createReadOnlyOptionsBuilder() .setReadTime(com.google.protobuf.Timestamp.newBuilder().setSeconds(1).setNanos(0)) .build(); From 2c142fc0bb4a5cd4a4c40e3de0425115b3ff1ac2 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Mon, 10 Aug 2020 19:10:02 -0400 Subject: [PATCH 11/15] add javadocs and cleanup some of the public surface --- .../google/cloud/firestore/Transaction.java | 2 +- .../cloud/firestore/TransactionOptions.java | 188 +++++++++++++++--- .../cloud/firestore/TransactionTest.java | 20 +- 3 files changed, 175 insertions(+), 35 deletions(-) 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 965fd9fdf..48ff6378f 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 @@ -93,7 +93,7 @@ ApiFuture begin() { .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType())) { - final ReadOnly.Builder builder = transactionOptions.getReadOnly().toProtoBuilder(); + final ReadOnly builder = transactionOptions.getReadOnly().toProto(); beginTransaction.getOptionsBuilder().setReadOnly(builder); } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 48dcef102..ba81f9366 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -17,16 +17,32 @@ package com.google.cloud.firestore; import com.google.api.core.InternalApi; +import com.google.api.core.InternalExtensionOnly; import com.google.common.base.Preconditions; import com.google.firestore.v1.TransactionOptions.ReadOnly; import com.google.firestore.v1.TransactionOptions.ReadWrite; import com.google.protobuf.Timestamp; import com.google.protobuf.TimestampOrBuilder; +import java.util.Objects; import java.util.concurrent.Executor; import javax.annotation.Nonnull; import javax.annotation.Nullable; -/** Options specifying the behavior of Firestore Transactions. */ +/** + * Options specifying the behavior of Firestore Transactions. + * + *

A transaction in Firestore can be either read-write or read-only. + * + *

The default set of options is a read-write transaction with a max attempt count of 5. This + * attempt count can be customized via the {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} + * method. A new instance of a builder can be created by calling {@link + * #createReadWriteOptionsBuilder()}. + * + *

A read-only transaction can be configured via the {@link ReadOnlyOptionsBuilder} class. A new + * instance can be created by calling {@link #createReadOnlyOptionsBuilder()}. + * + * @see com.google.firestore.v1.TransactionOptions + */ public final class TransactionOptions { private static final TransactionOptions DEFAULT_READ_WRITE_TRANSACTION_OPTIONS = @@ -71,11 +87,28 @@ public Executor getExecutor() { return executor; } + /** + * A type flag indicating the type of transaction represented. This method should be called before + * any call to {@link #getReadOnly()} or {@link #getReadWrite()} to determine with method to call. + * If a miss-matched {@code get} method is called an {@link IllegalStateException} will be thrown. + * + * @return The type of transaction this represents. Either read-only or read-write. + */ @Nonnull public TransactionOptionsType getType() { return type; } + /** + * Get the set of read-only options for this instance. Before calling this method {@link + * #getType()} should be called, and the return value should be {@link + * TransactionOptionsType#READ_ONLY}. + * + * @return The instance of {@link ReadOnlyOptions} held in this instance if this instance in fact + * read-only. + * @throws IllegalStateException if the type of this instance is {@link + * TransactionOptionsType#READ_WRITE} + */ @Nonnull public ReadOnlyOptions getReadOnly() { Preconditions.checkState( @@ -83,6 +116,16 @@ public ReadOnlyOptions getReadOnly() { return readOnly; } + /** + * Get the set of read-write options for this instance. Before calling this method {@link + * #getType()} should be called, and the return value should be {@link + * TransactionOptionsType#READ_WRITE}. + * + * @return The instance of {@link ReadWriteOptions} held in this instance if this instance in fact + * read-write. + * @throws IllegalStateException if the type of this instance is {@link + * TransactionOptionsType#READ_ONLY} + */ @Nonnull public ReadWriteOptions getReadWrite() { Preconditions.checkState( @@ -149,16 +192,25 @@ public static TransactionOptions create(@Nullable Executor executor, int numberO .build(); } + /** + * @return a new builder with default values applicable to configuring options for a read-write + * transaction. + */ @Nonnull public static ReadWriteOptionsBuilder createReadWriteOptionsBuilder() { - return Builder.readWriteBuilder(); + return new ReadWriteOptionsBuilder(null, DEFAULT_NUM_ATTEMPTS); } + /** + * @return a new builder with default values applicable to configuring options for a read-only + * transaction. + */ @Nonnull public static ReadOnlyOptionsBuilder createReadOnlyOptionsBuilder() { - return Builder.readOnlyBuilder(); + return new ReadOnlyOptionsBuilder(null, null); } + @InternalExtensionOnly public abstract static class Builder> { @Nullable protected Executor executor; @@ -166,11 +218,20 @@ protected Builder(@Nullable Executor executor) { this.executor = executor; } + /** + * @return The {@link Executor} user callbacks will execute on, If null, the default executor + * will be used. + */ @Nullable public Executor getExecutor() { return executor; } + /** + * @param executor The {@link Executor} user callbacks will executed on. If null, the default + * executor will be used. + * @return {@code this} builder + */ @Nonnull @SuppressWarnings("unchecked") public B setExecutor(@Nullable Executor executor) { @@ -178,18 +239,16 @@ public B setExecutor(@Nullable Executor executor) { return (B) this; } + /** @return an instance of {@link TransactionOptions} from the values passed to this builder */ @Nonnull public abstract TransactionOptions build(); - - static ReadOnlyOptionsBuilder readOnlyBuilder() { - return new ReadOnlyOptionsBuilder(null, null); - } - - static ReadWriteOptionsBuilder readWriteBuilder() { - return new ReadWriteOptionsBuilder(null, DEFAULT_NUM_ATTEMPTS); - } } + /** + * A typesafe builder class representing those options that are applicable when configuring a + * transaction to be read-only. All methods function as "set" rather than returning a new copy + * with a value set on it. + */ public static final class ReadOnlyOptionsBuilder extends Builder { @Nullable private TimestampOrBuilder readTime; @@ -198,11 +257,21 @@ private ReadOnlyOptionsBuilder(@Nullable Executor executor, @Nullable Timestamp this.readTime = readTime; } + /** @return the currently set value that will be used as the readTime. */ @Nullable public TimestampOrBuilder getReadTime() { return readTime; } + /** + * Specify to read documents at the given time. This may not be more than 60 in the past from + * when the request is processed by the server. + * + * @param readTime The specific time to read documents at. Must not be older than 60 seconds. A + * null value means read most up to date data. + * @return {@code this} builder + */ + @Nonnull public ReadOnlyOptionsBuilder setReadTime(@Nullable TimestampOrBuilder readTime) { this.readTime = readTime; return this; @@ -222,6 +291,12 @@ public TransactionOptions build() { } } + /** + * A typesafe builder class representing those options that are applicable when configuring a + * transaction to be read-write. All methods function as "set" rather than returning a new copy + * with a value set on it. By default, a read-write transaction will be attempted a max of 5 + * times. + */ public static final class ReadWriteOptionsBuilder extends Builder { private int numberOfAttempts; @@ -230,10 +305,24 @@ private ReadWriteOptionsBuilder(@Nullable Executor executor, int numberOfAttempt this.numberOfAttempts = numberOfAttempts; } + /** + * Specify the max number of attempts a transaction will be attempted before resulting in an + * error. + * + * @return The max number of attempts to try and commit the transaction. + */ public int getNumberOfAttempts() { return numberOfAttempts; } + /** + * Specify the max number of attempts a transaction will be attempted before resulting in an + * error. + * + * @param numberOfAttempts The max number of attempts to try and commit the transaction. + * @return {@code this} builder + * @throws IllegalArgumentException if numberOfAttempts is less than or equal to 0 + */ @Nonnull public ReadWriteOptionsBuilder setNumberOfAttempts(int numberOfAttempts) { Preconditions.checkArgument(numberOfAttempts > 0, "You must allow at least one attempt"); @@ -257,46 +346,97 @@ public enum TransactionOptionsType { READ_WRITE } - public static final class ReadOnlyOptions implements ToProtoBuilder { + /** + * A typesafe options class representing those options that are applicable when configuring a + * transaction to be read-only. + */ + public static final class ReadOnlyOptions { @Nullable private final Timestamp readTime; private ReadOnlyOptions(@Nullable Timestamp readTime) { this.readTime = readTime; } - @Override - public ReadOnly.Builder toProtoBuilder() { + ReadOnly toProto() { + final ReadOnly.Builder builder = ReadOnly.getDefaultInstance().toBuilder(); if (readTime != null) { - return ReadOnly.getDefaultInstance().toBuilder().setReadTime(readTime); - } else { - return ReadOnly.getDefaultInstance().toBuilder(); + builder.setReadTime(readTime); } + return builder.build(); } + /** + * Specify to read documents at the given time. This may not be more than 60 in the past from + * when the request is processed by the server. + * + * @return The specific time to read documents at. Must not be older than 60 seconds. A null + * value means read most up to date data. + */ @Nullable public Timestamp getReadTime() { return readTime; } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ReadOnlyOptions that = (ReadOnlyOptions) o; + return Objects.equals(readTime, that.readTime); + } + + @Override + public int hashCode() { + return Objects.hash(readTime); + } } - public static final class ReadWriteOptions implements ToProtoBuilder { + /** + * A typesafe options class representing those options that are applicable when configuring a + * transaction to be read-write. By default, a read-write transaction will be attempted a max of 5 + * times. + */ + public static final class ReadWriteOptions { + private static final ReadWrite PROTO = ReadWrite.getDefaultInstance().toBuilder().build(); private final int numberOfAttempts; private ReadWriteOptions(int numberOfAttempts) { this.numberOfAttempts = numberOfAttempts; } - @Override - public ReadWrite.Builder toProtoBuilder() { - return ReadWrite.getDefaultInstance().toBuilder(); + ReadWrite toProto() { + return PROTO; } + /** + * Specify the max number of attempts a transaction will be attempted before resulting in an + * error. + * + * @return The max number of attempts to try and commit the transaction. + */ public int getNumberOfAttempts() { return numberOfAttempts; } - } - interface ToProtoBuilder { - ProtoBuilder toProtoBuilder(); + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ReadWriteOptions that = (ReadWriteOptions) o; + return numberOfAttempts == that.numberOfAttempts; + } + + @Override + public int hashCode() { + return Objects.hash(numberOfAttempts); + } } } 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 83b85bb90..3930b490d 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 @@ -884,7 +884,9 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { final com.google.protobuf.Timestamp.Builder readTime = com.google.protobuf.Timestamp.getDefaultInstance().toBuilder().setSeconds(1).setNanos(0); final ReadOnlyOptionsBuilder builder = - TransactionOptions.createReadOnlyOptionsBuilder().setExecutor(executor).setReadTime(readTime); + TransactionOptions.createReadOnlyOptionsBuilder() + .setExecutor(executor) + .setReadTime(readTime); final ReadOnly expectedReadOnly = ReadOnly.newBuilder().setReadTime(readTime).build(); final TransactionOptions transactionOptions = builder.build(); @@ -896,8 +898,7 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { assertThat(transactionOptions.getType()).isEqualTo(TransactionOptionsType.READ_ONLY); final ReadOnlyOptions readOnly = transactionOptions.getReadOnly(); - // actually build the builder so we get a useful .equals method - assertThat(readOnly.toProtoBuilder().build()).isEqualTo(expectedReadOnly); + assertThat(readOnly.toProto()).isEqualTo(expectedReadOnly); } @Test @@ -912,8 +913,7 @@ public void readOnlyTransactionOptionsBuilder_defaults() { final ReadOnlyOptions readOnly = transactionOptions.getReadOnly(); assertThat(readOnly.getReadTime()).isNull(); - // actually build the builder so we get a useful .equals method - assertThat(readOnly.toProtoBuilder().build()).isEqualTo(expectedReadOnly); + assertThat(readOnly.toProto()).isEqualTo(expectedReadOnly); } @Test @@ -932,7 +932,9 @@ public void readOnlyTransactionOptionsBuilder_errorWhenGettingReadWrite() { public void readWriteTransactionOptionsBuilder_setNumberOfAttempts() { Executor executor = mock(Executor.class); final ReadWriteOptionsBuilder builder = - TransactionOptions.createReadWriteOptionsBuilder().setExecutor(executor).setNumberOfAttempts(2); + TransactionOptions.createReadWriteOptionsBuilder() + .setExecutor(executor) + .setNumberOfAttempts(2); final ReadWrite expectedReadWrite = ReadWrite.newBuilder().build(); final TransactionOptions transactionOptions = builder.build(); @@ -944,8 +946,7 @@ public void readWriteTransactionOptionsBuilder_setNumberOfAttempts() { assertThat(transactionOptions.getType()).isEqualTo(TransactionOptionsType.READ_WRITE); final ReadWriteOptions readWrite = options.getReadWrite(); - // actually build the builder so we get a useful .equals method - assertThat(readWrite.toProtoBuilder().build()).isEqualTo(expectedReadWrite); + assertThat(readWrite.toProto()).isEqualTo(expectedReadWrite); } @Test @@ -959,8 +960,7 @@ public void readWriteTransactionOptionsBuilder_defaults() { assertThat(transactionOptions.getExecutor()).isNull(); assertThat(readWrite.getNumberOfAttempts()).isEqualTo(5); - // actually build the builder so we get a useful .equals method - assertThat(readWrite.toProtoBuilder().build()).isEqualTo(expectedReadWrite); + assertThat(readWrite.toProto()).isEqualTo(expectedReadWrite); } @Test From d4127f9e890c43d6531f98bd3b9c4b8c3022884f Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Tue, 11 Aug 2020 17:07:52 -0400 Subject: [PATCH 12/15] chore: formatting --- .../test/java/com/google/cloud/firestore/TransactionTest.java | 2 -- 1 file changed, 2 deletions(-) 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 3930b490d..c153c5647 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 @@ -70,7 +70,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; @@ -87,7 +86,6 @@ import org.mockito.Matchers; import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; -import org.mockito.stubbing.Stubber; @SuppressWarnings("deprecation") @RunWith(MockitoJUnitRunner.class) From 2c112bc8577d3b000b3fa81712d56911151dcb0c Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Tue, 11 Aug 2020 17:49:49 -0400 Subject: [PATCH 13/15] fix: remove typesafe options --- .../google/cloud/firestore/Transaction.java | 6 +- .../cloud/firestore/TransactionOptions.java | 169 +++--------------- .../cloud/firestore/TransactionRunner.java | 9 +- .../cloud/firestore/TransactionTest.java | 52 +----- 4 files changed, 33 insertions(+), 203 deletions(-) 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 48ff6378f..4fe13b552 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 @@ -25,7 +25,6 @@ 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 java.util.List; @@ -92,9 +91,8 @@ ApiFuture begin() { .getOptionsBuilder() .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); - } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType())) { - final ReadOnly builder = transactionOptions.getReadOnly().toProto(); - beginTransaction.getOptionsBuilder().setReadOnly(builder); + } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType()) && transactionOptions.getReadTime()!= null) { + beginTransaction.getOptionsBuilder().getReadOnlyBuilder().setReadTime(transactionOptions.getReadTime()); } ApiFuture transactionBeginFuture = diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index ba81f9366..752d57179 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -16,14 +16,10 @@ package com.google.cloud.firestore; -import com.google.api.core.InternalApi; import com.google.api.core.InternalExtensionOnly; import com.google.common.base.Preconditions; -import com.google.firestore.v1.TransactionOptions.ReadOnly; -import com.google.firestore.v1.TransactionOptions.ReadWrite; import com.google.protobuf.Timestamp; import com.google.protobuf.TimestampOrBuilder; -import java.util.Objects; import java.util.concurrent.Executor; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -52,33 +48,26 @@ public final class TransactionOptions { private final Executor executor; private final TransactionOptionsType type; - private final ReadOnlyOptions readOnly; - private final ReadWriteOptions readWrite; + private final int numberOfAttempts; + @Nullable + private final Timestamp readTime; TransactionOptions( Executor executor, TransactionOptionsType type, - ReadOnlyOptions readOnly, - ReadWriteOptions readWrite) { + int numberOfAttempts, + @Nullable Timestamp readTime) { this.executor = executor; this.type = type; - this.readOnly = readOnly; - this.readWrite = readWrite; + this.numberOfAttempts = numberOfAttempts; + this.readTime = readTime; } /** * @return the initial number of attempts a read-write transaction will be attempted - * @deprecated as of v2.0.0, only applicable to Read-Write transactions. Use {@link - * ReadWriteOptions#getNumberOfAttempts()} instead */ - @Deprecated - @InternalApi public int getNumberOfAttempts() { - if (type == TransactionOptionsType.READ_WRITE) { - return readWrite.numberOfAttempts; - } else { - return 1; - } + return numberOfAttempts; } /** @return Executor to be used to run user callbacks on */ @@ -88,9 +77,7 @@ public Executor getExecutor() { } /** - * A type flag indicating the type of transaction represented. This method should be called before - * any call to {@link #getReadOnly()} or {@link #getReadWrite()} to determine with method to call. - * If a miss-matched {@code get} method is called an {@link IllegalStateException} will be thrown. + * A type flag indicating the type of transaction represented. * * @return The type of transaction this represents. Either read-only or read-write. */ @@ -100,37 +87,18 @@ public TransactionOptionsType getType() { } /** - * Get the set of read-only options for this instance. Before calling this method {@link - * #getType()} should be called, and the return value should be {@link - * TransactionOptionsType#READ_ONLY}. - * - * @return The instance of {@link ReadOnlyOptions} held in this instance if this instance in fact - * read-only. - * @throws IllegalStateException if the type of this instance is {@link - * TransactionOptionsType#READ_WRITE} - */ - @Nonnull - public ReadOnlyOptions getReadOnly() { - Preconditions.checkState( - readOnly != null && readWrite == null, "Unable to call getReadOnly for ReadWriteOptions"); - return readOnly; - } - - /** - * Get the set of read-write options for this instance. Before calling this method {@link - * #getType()} should be called, and the return value should be {@link - * TransactionOptionsType#READ_WRITE}. + * Specify to read documents at the given time. This may not be more than 60 in the past from + * when the request is processed by the server. * - * @return The instance of {@link ReadWriteOptions} held in this instance if this instance in fact - * read-write. - * @throws IllegalStateException if the type of this instance is {@link - * TransactionOptionsType#READ_ONLY} + * @return The specific time to read documents at. A null value means read most up to date data. */ - @Nonnull - public ReadWriteOptions getReadWrite() { - Preconditions.checkState( - readWrite != null && readOnly == null, "Unable to call getReadWrite for ReadOnlyOptions"); - return readWrite; + @Nullable + public Timestamp getReadTime() { + if (TransactionOptionsType.READ_ONLY.equals(type)) { + return readTime; + } else { + return null; + } } /** @@ -287,7 +255,7 @@ public TransactionOptions build() { timestamp = (Timestamp) readTime; } return new TransactionOptions( - executor, TransactionOptionsType.READ_ONLY, new ReadOnlyOptions(timestamp), null); + executor, TransactionOptionsType.READ_ONLY, 1, timestamp); } } @@ -336,8 +304,8 @@ public TransactionOptions build() { return new TransactionOptions( executor, TransactionOptionsType.READ_WRITE, - null, - new ReadWriteOptions(numberOfAttempts)); + numberOfAttempts, + null); } } @@ -346,97 +314,4 @@ public enum TransactionOptionsType { READ_WRITE } - /** - * A typesafe options class representing those options that are applicable when configuring a - * transaction to be read-only. - */ - public static final class ReadOnlyOptions { - @Nullable private final Timestamp readTime; - - private ReadOnlyOptions(@Nullable Timestamp readTime) { - this.readTime = readTime; - } - - ReadOnly toProto() { - final ReadOnly.Builder builder = ReadOnly.getDefaultInstance().toBuilder(); - if (readTime != null) { - builder.setReadTime(readTime); - } - return builder.build(); - } - - /** - * Specify to read documents at the given time. This may not be more than 60 in the past from - * when the request is processed by the server. - * - * @return The specific time to read documents at. Must not be older than 60 seconds. A null - * value means read most up to date data. - */ - @Nullable - public Timestamp getReadTime() { - return readTime; - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - ReadOnlyOptions that = (ReadOnlyOptions) o; - return Objects.equals(readTime, that.readTime); - } - - @Override - public int hashCode() { - return Objects.hash(readTime); - } - } - - /** - * A typesafe options class representing those options that are applicable when configuring a - * transaction to be read-write. By default, a read-write transaction will be attempted a max of 5 - * times. - */ - public static final class ReadWriteOptions { - private static final ReadWrite PROTO = ReadWrite.getDefaultInstance().toBuilder().build(); - private final int numberOfAttempts; - - private ReadWriteOptions(int numberOfAttempts) { - this.numberOfAttempts = numberOfAttempts; - } - - ReadWrite toProto() { - return PROTO; - } - - /** - * Specify the max number of attempts a transaction will be attempted before resulting in an - * error. - * - * @return The max number of attempts to try and commit the transaction. - */ - public int getNumberOfAttempts() { - return numberOfAttempts; - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - ReadWriteOptions that = (ReadWriteOptions) o; - return numberOfAttempts == that.numberOfAttempts; - } - - @Override - public int hashCode() { - return Objects.hash(numberOfAttempts); - } - } } 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 8a3a954aa..2c6a8c735 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 @@ -83,14 +83,7 @@ class TransactionRunner { this.firestore = firestore; this.firestoreExecutor = firestore.getClient().getExecutor(); this.userCallback = userCallback; - switch (transactionOptions.getType()) { - case READ_WRITE: - this.attemptsRemaining = transactionOptions.getReadWrite().getNumberOfAttempts(); - break; - case READ_ONLY: - this.attemptsRemaining = 1; - break; - } + this.attemptsRemaining = transactionOptions.getNumberOfAttempts(); this.userCallbackExecutor = Context.currentContextExecutor( transactionOptions.getExecutor() != null 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 c153c5647..25c60c5f4 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 @@ -53,16 +53,12 @@ import com.google.api.gax.rpc.UnaryCallable; import com.google.cloud.Timestamp; import com.google.cloud.firestore.LocalFirestoreHelper.ResponseStubber; -import com.google.cloud.firestore.TransactionOptions.ReadOnlyOptions; import com.google.cloud.firestore.TransactionOptions.ReadOnlyOptionsBuilder; -import com.google.cloud.firestore.TransactionOptions.ReadWriteOptions; import com.google.cloud.firestore.TransactionOptions.ReadWriteOptionsBuilder; import com.google.cloud.firestore.TransactionOptions.TransactionOptionsType; import com.google.cloud.firestore.spi.v1.FirestoreRpc; import com.google.firestore.v1.BatchGetDocumentsRequest; import com.google.firestore.v1.DocumentMask; -import com.google.firestore.v1.TransactionOptions.ReadOnly; -import com.google.firestore.v1.TransactionOptions.ReadWrite; import com.google.firestore.v1.Write; import com.google.protobuf.GeneratedMessageV3; import com.google.protobuf.Message; @@ -885,7 +881,6 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { TransactionOptions.createReadOnlyOptionsBuilder() .setExecutor(executor) .setReadTime(readTime); - final ReadOnly expectedReadOnly = ReadOnly.newBuilder().setReadTime(readTime).build(); final TransactionOptions transactionOptions = builder.build(); @@ -895,35 +890,21 @@ public void readOnlyTransactionOptionsBuilder_setReadTime() { assertThat(transactionOptions.getExecutor()).isSameInstanceAs(executor); assertThat(transactionOptions.getType()).isEqualTo(TransactionOptionsType.READ_ONLY); - final ReadOnlyOptions readOnly = transactionOptions.getReadOnly(); - assertThat(readOnly.toProto()).isEqualTo(expectedReadOnly); + assertThat(transactionOptions.getReadTime()).isEqualTo(readTime.build()); + assertThat(transactionOptions.getNumberOfAttempts()).isEqualTo(1); } @Test public void readOnlyTransactionOptionsBuilder_defaults() { final ReadOnlyOptionsBuilder builder = TransactionOptions.createReadOnlyOptionsBuilder(); - final ReadOnly expectedReadOnly = ReadOnly.newBuilder().build(); final TransactionOptions transactionOptions = builder.build(); assertThat(builder.getExecutor()).isNull(); assertThat(builder.getReadTime()).isNull(); - final ReadOnlyOptions readOnly = transactionOptions.getReadOnly(); - assertThat(readOnly.getReadTime()).isNull(); - assertThat(readOnly.toProto()).isEqualTo(expectedReadOnly); - } - - @Test - public void readOnlyTransactionOptionsBuilder_errorWhenGettingReadWrite() { - final ReadOnlyOptionsBuilder builder = TransactionOptions.createReadOnlyOptionsBuilder(); - try { - //noinspection ResultOfMethodCallIgnored - builder.build().getReadWrite(); - fail("Error expected"); - } catch (IllegalStateException ignore) { - // expected - } + assertThat(transactionOptions.getReadTime()).isNull(); + assertThat(transactionOptions.getNumberOfAttempts()).isEqualTo(1); } @Test @@ -933,7 +914,6 @@ public void readWriteTransactionOptionsBuilder_setNumberOfAttempts() { TransactionOptions.createReadWriteOptionsBuilder() .setExecutor(executor) .setNumberOfAttempts(2); - final ReadWrite expectedReadWrite = ReadWrite.newBuilder().build(); final TransactionOptions transactionOptions = builder.build(); @@ -943,34 +923,18 @@ public void readWriteTransactionOptionsBuilder_setNumberOfAttempts() { assertThat(transactionOptions.getExecutor()).isSameInstanceAs(executor); assertThat(transactionOptions.getType()).isEqualTo(TransactionOptionsType.READ_WRITE); - final ReadWriteOptions readWrite = options.getReadWrite(); - assertThat(readWrite.toProto()).isEqualTo(expectedReadWrite); + assertThat(transactionOptions.getNumberOfAttempts()).isEqualTo(2); + assertThat(transactionOptions.getReadTime()).isNull(); } @Test public void readWriteTransactionOptionsBuilder_defaults() { - final ReadWrite expectedReadWrite = ReadWrite.newBuilder().build(); - final TransactionOptions transactionOptions = TransactionOptions.createReadWriteOptionsBuilder().build(); - final ReadWriteOptions readWrite = transactionOptions.getReadWrite(); assertThat(transactionOptions.getExecutor()).isNull(); - assertThat(readWrite.getNumberOfAttempts()).isEqualTo(5); - - assertThat(readWrite.toProto()).isEqualTo(expectedReadWrite); - } - - @Test - public void readWriteTransactionOptionsBuilder_errorWhenGettingReadWrite() { - final ReadWriteOptionsBuilder builder = TransactionOptions.createReadWriteOptionsBuilder(); - try { - //noinspection ResultOfMethodCallIgnored - builder.build().getReadOnly(); - fail("Error expected"); - } catch (IllegalStateException ignore) { - // expected - } + assertThat(transactionOptions.getNumberOfAttempts()).isEqualTo(5); + assertThat(transactionOptions.getReadTime()).isNull(); } @Test From 4ecc861e060059ecaf412cd38d446be7ccbf3be1 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Tue, 11 Aug 2020 17:57:01 -0400 Subject: [PATCH 14/15] docs: javadoc fixes --- .../google/cloud/firestore/Transaction.java | 8 +++- .../cloud/firestore/TransactionOptions.java | 39 +++++++++---------- 2 files changed, 25 insertions(+), 22 deletions(-) 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 4fe13b552..ff5a40623 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 @@ -91,8 +91,12 @@ ApiFuture begin() { .getOptionsBuilder() .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); - } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType()) && transactionOptions.getReadTime()!= null) { - beginTransaction.getOptionsBuilder().getReadOnlyBuilder().setReadTime(transactionOptions.getReadTime()); + } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType()) + && transactionOptions.getReadTime() != null) { + beginTransaction + .getOptionsBuilder() + .getReadOnlyBuilder() + .setReadTime(transactionOptions.getReadTime()); } ApiFuture transactionBeginFuture = diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 752d57179..05f2a3fcb 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -29,10 +29,10 @@ * *

A transaction in Firestore can be either read-write or read-only. * - *

The default set of options is a read-write transaction with a max attempt count of 5. This - * attempt count can be customized via the {@link ReadWriteOptionsBuilder#setNumberOfAttempts(int)} - * method. A new instance of a builder can be created by calling {@link - * #createReadWriteOptionsBuilder()}. + *

The default set of options is a read-write transaction with a maximum number of 5 attempts. + * This attempt count can be customized via the {@link + * ReadWriteOptionsBuilder#setNumberOfAttempts(int)} method. A new instance of a builder can be + * created by calling {@link #createReadWriteOptionsBuilder()}. * *

A read-only transaction can be configured via the {@link ReadOnlyOptionsBuilder} class. A new * instance can be created by calling {@link #createReadOnlyOptionsBuilder()}. @@ -49,8 +49,7 @@ public final class TransactionOptions { private final Executor executor; private final TransactionOptionsType type; private final int numberOfAttempts; - @Nullable - private final Timestamp readTime; + @Nullable private final Timestamp readTime; TransactionOptions( Executor executor, @@ -64,7 +63,10 @@ public final class TransactionOptions { } /** - * @return the initial number of attempts a read-write transaction will be attempted + * Returns the maximum number of times a transaction will be attempted before resulting in an + * error. + * + * @return The max number of attempts to try and commit the transaction. */ public int getNumberOfAttempts() { return numberOfAttempts; @@ -87,8 +89,10 @@ public TransactionOptionsType getType() { } /** - * Specify to read documents at the given time. This may not be more than 60 in the past from - * when the request is processed by the server. + * A {@link Timestamp} specifying the time documents are to be read at. If null, the server will + * read documents at the most up to date available. If nonnull, the specified {@code Timestamp} + * may not be more than 60 seconds in the past (evaluated when the request is processed by the + * server). * * @return The specific time to read documents at. A null value means read most up to date data. */ @@ -161,7 +165,7 @@ public static TransactionOptions create(@Nullable Executor executor, int numberO } /** - * @return a new builder with default values applicable to configuring options for a read-write + * @return a new Builder with default values applicable to configuring options for a read-write * transaction. */ @Nonnull @@ -170,7 +174,7 @@ public static ReadWriteOptionsBuilder createReadWriteOptionsBuilder() { } /** - * @return a new builder with default values applicable to configuring options for a read-only + * @return a new Builder with default values applicable to configuring options for a read-only * transaction. */ @Nonnull @@ -232,8 +236,8 @@ public TimestampOrBuilder getReadTime() { } /** - * Specify to read documents at the given time. This may not be more than 60 in the past from - * when the request is processed by the server. + * Specify to read documents at the given time. This may not be more than 60 seconds in the past + * from when the request is processed by the server. * * @param readTime The specific time to read documents at. Must not be older than 60 seconds. A * null value means read most up to date data. @@ -254,8 +258,7 @@ public TransactionOptions build() { } else { timestamp = (Timestamp) readTime; } - return new TransactionOptions( - executor, TransactionOptionsType.READ_ONLY, 1, timestamp); + return new TransactionOptions(executor, TransactionOptionsType.READ_ONLY, 1, timestamp); } } @@ -302,10 +305,7 @@ public ReadWriteOptionsBuilder setNumberOfAttempts(int numberOfAttempts) { @Override public TransactionOptions build() { return new TransactionOptions( - executor, - TransactionOptionsType.READ_WRITE, - numberOfAttempts, - null); + executor, TransactionOptionsType.READ_WRITE, numberOfAttempts, null); } } @@ -313,5 +313,4 @@ public enum TransactionOptionsType { READ_ONLY, READ_WRITE } - } From 738e82cd2375843a98399cce43b47050ae8134e3 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Wed, 12 Aug 2020 14:45:47 -0400 Subject: [PATCH 15/15] final review fixes --- .../com/google/cloud/firestore/Transaction.java | 13 +++++++------ .../google/cloud/firestore/TransactionOptions.java | 7 +++++-- .../com/google/cloud/firestore/it/ITSystemTest.java | 2 +- 3 files changed, 13 insertions(+), 9 deletions(-) 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 ff5a40623..6a9fd9714 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 @@ -25,6 +25,7 @@ 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 java.util.List; @@ -91,12 +92,12 @@ ApiFuture begin() { .getOptionsBuilder() .getReadWriteBuilder() .setRetryTransaction(previousTransactionId); - } else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType()) - && transactionOptions.getReadTime() != null) { - beginTransaction - .getOptionsBuilder() - .getReadOnlyBuilder() - .setReadTime(transactionOptions.getReadTime()); + } 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/TransactionOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java index 05f2a3fcb..14dd40819 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionOptions.java @@ -90,14 +90,17 @@ public TransactionOptionsType getType() { /** * A {@link Timestamp} specifying the time documents are to be read at. If null, the server will - * read documents at the most up to date available. If nonnull, the specified {@code Timestamp} + * read documents at the most up to date available. If non-null, the specified {@code Timestamp} * may not be more than 60 seconds in the past (evaluated when the request is processed by the * server). * - * @return The specific time to read documents at. A null value means read most up to date data. + * @return The specific time to read documents at. A null value means reading the most up to date + * data. */ @Nullable public Timestamp getReadTime() { + // This if statement is not strictly necessary, however is kept here for clarity sake to show + // that readTime is only applicable to a read-only transaction type. if (TransactionOptionsType.READ_ONLY.equals(type)) { return readTime; } else { 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 56bdb9c01..1a1a3d4d1 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 @@ -1448,7 +1448,7 @@ public Void updateCallback(Transaction transaction) throws Exception { } @Test - public void readOnlyTransaction_faliureWhenAttemptingWrite() + public void readOnlyTransaction_failureWhenAttemptingWrite() throws InterruptedException, TimeoutException { final DocumentReference documentReference = randomColl.document("tx/ro/writeShouldFail");