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

Commit 4fc2e27

Browse files
aseeringaseering
committed
Address code-review comments
1 parent 4556e81 commit 4fc2e27

7 files changed

Lines changed: 149 additions & 128 deletions

File tree

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -684,20 +684,20 @@ QueryOptions buildQueryOptions(QueryOptions requestOptions) {
684684
}
685685

686686
RequestOptions buildRequestOptions(Options options) {
687-
RequestOptions requestOptions = options.toRequestOptionsProto(false);
687+
RequestOptions.Builder builder = options.toRequestOptionsProto(false).toBuilder();
688688
RequestOptions.ClientContext defaultClientContext =
689689
session.getSpanner().getOptions().getClientContext();
690690
if (defaultClientContext != null) {
691-
RequestOptions.ClientContext.Builder builder = defaultClientContext.toBuilder();
692-
if (requestOptions.hasClientContext()) {
693-
builder.mergeFrom(requestOptions.getClientContext());
691+
RequestOptions.ClientContext.Builder clientContextBuilder = defaultClientContext.toBuilder();
692+
if (builder.hasClientContext()) {
693+
clientContextBuilder.mergeFrom(builder.getClientContext());
694694
}
695-
requestOptions = requestOptions.toBuilder().setClientContext(builder.build()).build();
695+
builder.setClientContext(clientContextBuilder.build());
696696
}
697697
if (getTransactionTag() != null) {
698-
return requestOptions.toBuilder().setTransactionTag(getTransactionTag()).build();
698+
builder.setTransactionTag(getTransactionTag());
699699
}
700-
return requestOptions;
700+
return builder.build();
701701
}
702702

703703
ExecuteSqlRequest.Builder getExecuteSqlRequestBuilder(

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -489,18 +489,19 @@ ApiFuture<Transaction> beginTransactionAsync(
489489
if (sessionReference.getIsMultiplexed() && mutation != null) {
490490
requestBuilder.setMutationKey(mutation);
491491
}
492-
RequestOptions requestOptions = transactionOptions.toRequestOptionsProto(true);
492+
RequestOptions.Builder optionsBuilder = transactionOptions.toRequestOptionsProto(true).toBuilder();
493493
RequestOptions.ClientContext defaultClientContext = spanner.getOptions().getClientContext();
494494
if (defaultClientContext != null) {
495495
RequestOptions.ClientContext.Builder builder = defaultClientContext.toBuilder();
496-
if (requestOptions.hasClientContext()) {
497-
builder.mergeFrom(requestOptions.getClientContext());
496+
if (optionsBuilder.hasClientContext()) {
497+
builder.mergeFrom(optionsBuilder.getClientContext());
498498
}
499-
requestOptions = requestOptions.toBuilder().setClientContext(builder.build()).build();
499+
optionsBuilder.setClientContext(builder.build());
500500
}
501501
if (!sessionReference.getIsMultiplexed()) {
502-
requestOptions = requestOptions.toBuilder().clearTransactionTag().build();
502+
optionsBuilder.clearTransactionTag();
503503
}
504+
RequestOptions requestOptions = optionsBuilder.build();
504505
if (!requestOptions.equals(RequestOptions.getDefaultInstance())) {
505506
requestBuilder.setRequestOptions(requestOptions);
506507
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1988,7 +1988,7 @@ public Builder setDefaultTransactionOptions(
19881988
}
19891989

19901990
/** Sets the default {@link RequestOptions.ClientContext} for all requests. */
1991-
public Builder setClientContext(RequestOptions.ClientContext clientContext) {
1991+
public Builder setDefaultClientContext(RequestOptions.ClientContext clientContext) {
19921992
this.clientContext = clientContext;
19931993
return this;
19941994
}

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,14 @@
4646
/** Unit tests for {@link Options}. */
4747
@RunWith(JUnit4.class)
4848
public class OptionsTest {
49+
private static final DirectedReadOptions DIRECTED_READ_OPTIONS =
50+
DirectedReadOptions.newBuilder()
51+
.setIncludeReplicas(
52+
IncludeReplicas.newBuilder()
53+
.addReplicaSelections(
54+
ReplicaSelection.newBuilder().setLocation("us-west1").build()))
55+
.build();
56+
4957
@Test
5058
public void testToRequestOptionsProto() {
5159
RequestOptions.ClientContext clientContext =
@@ -72,14 +80,6 @@ public void testToRequestOptionsProto() {
7280
assertEquals(clientContext, protoForTransaction.getClientContext());
7381
}
7482

75-
private static final DirectedReadOptions DIRECTED_READ_OPTIONS =
76-
DirectedReadOptions.newBuilder()
77-
.setIncludeReplicas(
78-
IncludeReplicas.newBuilder()
79-
.addReplicaSelections(
80-
ReplicaSelection.newBuilder().setLocation("us-west1").build()))
81-
.build();
82-
8383
@Test
8484
public void negativeLimitsNotAllowed() {
8585
IllegalArgumentException e =

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionImplTest.java‎

Lines changed: 46 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,6 @@
1616

1717
package com.google.cloud.spanner;
1818

19-
import static com.google.common.truth.Truth.assertThat;
20-
import static org.junit.Assert.assertEquals;
21-
import static org.junit.Assert.assertNotNull;
22-
import static org.junit.Assert.assertThrows;
23-
import static org.junit.Assert.fail;
24-
import static org.mockito.ArgumentMatchers.any;
25-
import static org.mockito.ArgumentMatchers.anyMap;
26-
import static org.mockito.ArgumentMatchers.eq;
27-
import static org.mockito.Mockito.mock;
28-
import static org.mockito.Mockito.when;
29-
3019
import com.google.api.core.ApiFutures;
3120
import com.google.api.core.NanoClock;
3221
import com.google.api.gax.grpc.GrpcCallContext;
@@ -38,6 +27,7 @@
3827
import com.google.cloud.spanner.XGoogSpannerRequestId.NoopRequestIdCreator;
3928
import com.google.cloud.spanner.spi.v1.SpannerRpc;
4029
import com.google.cloud.spanner.v1.stub.SpannerStubSettings;
30+
import static com.google.common.truth.Truth.assertThat;
4131
import com.google.protobuf.ByteString;
4232
import com.google.protobuf.Empty;
4333
import com.google.protobuf.ListValue;
@@ -64,56 +54,29 @@
6454
import java.util.TimeZone;
6555
import java.util.concurrent.TimeUnit;
6656
import javax.annotation.Nullable;
57+
import static org.junit.Assert.assertEquals;
58+
import static org.junit.Assert.assertNotNull;
59+
import static org.junit.Assert.assertThrows;
60+
import static org.junit.Assert.fail;
6761
import org.junit.Before;
6862
import org.junit.BeforeClass;
6963
import org.junit.Test;
7064
import org.junit.runner.RunWith;
7165
import org.junit.runners.JUnit4;
7266
import org.mockito.ArgumentCaptor;
67+
import static org.mockito.ArgumentMatchers.any;
68+
import static org.mockito.ArgumentMatchers.anyMap;
69+
import static org.mockito.ArgumentMatchers.eq;
7370
import org.mockito.Captor;
7471
import org.mockito.Mock;
7572
import org.mockito.Mockito;
73+
import static org.mockito.Mockito.mock;
74+
import static org.mockito.Mockito.when;
7675
import org.mockito.MockitoAnnotations;
7776

7877
/** Unit tests for {@link com.google.cloud.spanner.SessionImpl}. */
7978
@RunWith(JUnit4.class)
8079
public class SessionImplTest {
81-
@Test
82-
public void testBeginTransactionWithClientContext() {
83-
RequestOptions.ClientContext clientContext =
84-
RequestOptions.ClientContext.newBuilder()
85-
.putSecureContext(
86-
"key", com.google.protobuf.Value.newBuilder().setStringValue("value").build())
87-
.build();
88-
Mockito.when(
89-
rpc.beginTransactionAsync(
90-
Mockito.any(BeginTransactionRequest.class), anyMap(), eq(true)))
91-
.thenReturn(
92-
ApiFutures.immediateFuture(
93-
Transaction.newBuilder().setId(ByteString.copyFromUtf8("tx")).build()));
94-
95-
((SessionImpl) session)
96-
.beginTransactionAsync(
97-
Options.fromTransactionOptions(
98-
Options.priority(Options.RpcPriority.HIGH),
99-
Options.tag("tag"),
100-
Options.clientContext(clientContext)),
101-
true,
102-
Collections.emptyMap(),
103-
null,
104-
null);
105-
106-
ArgumentCaptor<BeginTransactionRequest> requestCaptor =
107-
ArgumentCaptor.forClass(BeginTransactionRequest.class);
108-
Mockito.verify(rpc).beginTransactionAsync(requestCaptor.capture(), anyMap(), eq(true));
109-
BeginTransactionRequest request = requestCaptor.getValue();
110-
RequestOptions requestOptions = request.getRequestOptions();
111-
assertEquals(RequestOptions.Priority.PRIORITY_HIGH, requestOptions.getPriority());
112-
// TransactionTag should NOT be set because session is not multiplexed.
113-
assertEquals("", requestOptions.getTransactionTag());
114-
assertEquals(clientContext, requestOptions.getClientContext());
115-
}
116-
11780
@Mock private SpannerRpc rpc;
11881
@Mock private SpannerOptions spannerOptions;
11982
private com.google.cloud.spanner.Session session;
@@ -204,6 +167,42 @@ private void doNestedRwTransaction() {
204167
});
205168
}
206169

170+
@Test
171+
public void testBeginTransactionWithClientContext() {
172+
RequestOptions.ClientContext clientContext =
173+
RequestOptions.ClientContext.newBuilder()
174+
.putSecureContext(
175+
"key", com.google.protobuf.Value.newBuilder().setStringValue("value").build())
176+
.build();
177+
Mockito.when(
178+
rpc.beginTransactionAsync(
179+
Mockito.any(BeginTransactionRequest.class), anyMap(), eq(true)))
180+
.thenReturn(
181+
ApiFutures.immediateFuture(
182+
Transaction.newBuilder().setId(ByteString.copyFromUtf8("tx")).build()));
183+
184+
((SessionImpl) session)
185+
.beginTransactionAsync(
186+
Options.fromTransactionOptions(
187+
Options.priority(Options.RpcPriority.HIGH),
188+
Options.tag("tag"),
189+
Options.clientContext(clientContext)),
190+
true,
191+
Collections.emptyMap(),
192+
null,
193+
null);
194+
195+
ArgumentCaptor<BeginTransactionRequest> requestCaptor =
196+
ArgumentCaptor.forClass(BeginTransactionRequest.class);
197+
Mockito.verify(rpc).beginTransactionAsync(requestCaptor.capture(), anyMap(), eq(true));
198+
BeginTransactionRequest request = requestCaptor.getValue();
199+
RequestOptions requestOptions = request.getRequestOptions();
200+
assertEquals(RequestOptions.Priority.PRIORITY_HIGH, requestOptions.getPriority());
201+
// TransactionTag should NOT be set because session is not multiplexed.
202+
assertEquals("", requestOptions.getTransactionTag());
203+
assertEquals(clientContext, requestOptions.getClientContext());
204+
}
205+
207206
@Test
208207
public void nestedReadWriteTxnThrows() {
209208
SpannerException e = assertThrows(SpannerException.class, () -> doNestedRwTransaction());

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java‎

Lines changed: 43 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,6 @@
1616

1717
package com.google.cloud.spanner;
1818

19-
import static com.google.common.truth.Truth.assertThat;
20-
import static org.junit.Assert.assertArrayEquals;
21-
import static org.junit.Assert.assertEquals;
22-
import static org.junit.Assert.assertThrows;
23-
import static org.mockito.ArgumentMatchers.any;
24-
import static org.mockito.ArgumentMatchers.eq;
25-
import static org.mockito.Mockito.doThrow;
26-
import static org.mockito.Mockito.eq;
27-
import static org.mockito.Mockito.mock;
28-
import static org.mockito.Mockito.never;
29-
import static org.mockito.Mockito.times;
30-
import static org.mockito.Mockito.verify;
31-
import static org.mockito.Mockito.when;
32-
3319
import com.google.api.core.ApiFutures;
3420
import com.google.cloud.grpc.GrpcTransportOptions;
3521
import com.google.cloud.grpc.GrpcTransportOptions.ExecutorFactory;
@@ -40,6 +26,7 @@
4026
import com.google.cloud.spanner.spi.v1.SpannerRpc;
4127
import com.google.cloud.spanner.v1.stub.SpannerStubSettings;
4228
import com.google.common.base.Preconditions;
29+
import static com.google.common.truth.Truth.assertThat;
4330
import com.google.protobuf.ByteString;
4431
import com.google.protobuf.Duration;
4532
import com.google.protobuf.Empty;
@@ -75,14 +62,25 @@
7562
import java.util.concurrent.Executors;
7663
import java.util.concurrent.ScheduledExecutorService;
7764
import java.util.concurrent.atomic.AtomicInteger;
65+
import static org.junit.Assert.assertArrayEquals;
66+
import static org.junit.Assert.assertEquals;
67+
import static org.junit.Assert.assertThrows;
7868
import org.junit.Before;
7969
import org.junit.BeforeClass;
8070
import org.junit.Test;
8171
import org.junit.runner.RunWith;
8272
import org.junit.runners.JUnit4;
8373
import org.mockito.ArgumentCaptor;
74+
import static org.mockito.ArgumentMatchers.any;
75+
import static org.mockito.ArgumentMatchers.eq;
8476
import org.mockito.Mock;
8577
import org.mockito.Mockito;
78+
import static org.mockito.Mockito.doThrow;
79+
import static org.mockito.Mockito.mock;
80+
import static org.mockito.Mockito.never;
81+
import static org.mockito.Mockito.times;
82+
import static org.mockito.Mockito.verify;
83+
import static org.mockito.Mockito.when;
8684
import org.mockito.MockitoAnnotations;
8785

8886
/** Unit test for {@link com.google.cloud.spanner.TransactionRunnerImpl} */
@@ -101,37 +99,6 @@ public void release(ScheduledExecutorService exec) {
10199
}
102100
}
103101

104-
@Test
105-
public void testCommitWithClientContext() {
106-
RequestOptions.ClientContext clientContext =
107-
RequestOptions.ClientContext.newBuilder()
108-
.putSecureContext(
109-
"key", com.google.protobuf.Value.newBuilder().setStringValue("value").build())
110-
.build();
111-
when(session.getName()).thenReturn("projects/p/instances/i/databases/d/sessions/s");
112-
when(session.newTransaction(any(Options.class), any())).thenReturn(txn);
113-
Mockito.clearInvocations(session);
114-
transactionRunner =
115-
new TransactionRunnerImpl(
116-
session,
117-
Options.priority(Options.RpcPriority.HIGH),
118-
Options.tag("tag"),
119-
Options.clientContext(clientContext));
120-
transactionRunner.setSpan(span);
121-
122-
transactionRunner.run(
123-
transaction -> {
124-
return null;
125-
});
126-
127-
ArgumentCaptor<Options> optionsCaptor = ArgumentCaptor.forClass(Options.class);
128-
verify(session).newTransaction(optionsCaptor.capture(), any());
129-
Options capturedOptions = optionsCaptor.getValue();
130-
assertEquals(RequestOptions.Priority.PRIORITY_HIGH, capturedOptions.priority());
131-
assertEquals("tag", capturedOptions.tag());
132-
assertEquals(clientContext, capturedOptions.clientContext());
133-
}
134-
135102
@Mock private SpannerRpc rpc;
136103
@Mock private SessionImpl session;
137104
@Mock private TransactionRunnerImpl.TransactionContextImpl txn;
@@ -196,6 +163,37 @@ public void setUp() {
196163
transactionRunner.setSpan(span);
197164
}
198165

166+
@Test
167+
public void testCommitWithClientContext() {
168+
RequestOptions.ClientContext clientContext =
169+
RequestOptions.ClientContext.newBuilder()
170+
.putSecureContext(
171+
"key", com.google.protobuf.Value.newBuilder().setStringValue("value").build())
172+
.build();
173+
when(session.getName()).thenReturn("projects/p/instances/i/databases/d/sessions/s");
174+
when(session.newTransaction(any(Options.class), any())).thenReturn(txn);
175+
Mockito.clearInvocations(session);
176+
transactionRunner =
177+
new TransactionRunnerImpl(
178+
session,
179+
Options.priority(Options.RpcPriority.HIGH),
180+
Options.tag("tag"),
181+
Options.clientContext(clientContext));
182+
transactionRunner.setSpan(span);
183+
184+
transactionRunner.run(
185+
transaction -> {
186+
return null;
187+
});
188+
189+
ArgumentCaptor<Options> optionsCaptor = ArgumentCaptor.forClass(Options.class);
190+
verify(session).newTransaction(optionsCaptor.capture(), any());
191+
Options capturedOptions = optionsCaptor.getValue();
192+
assertEquals(RequestOptions.Priority.PRIORITY_HIGH, capturedOptions.priority());
193+
assertEquals("tag", capturedOptions.tag());
194+
assertEquals(clientContext, capturedOptions.clientContext());
195+
}
196+
199197
@SuppressWarnings("unchecked")
200198
@Test
201199
public void usesPreparedTransaction() {

0 commit comments

Comments
 (0)