Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Comment thread
lqiu96 marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.cloud.grpc.GrpcTransportOptions;
import com.google.cloud.http.HttpTransportOptions;
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import java.lang.reflect.Method;
Expand Down Expand Up @@ -129,7 +130,10 @@ public static class Builder extends ServiceOptions.Builder<Datastore, DatastoreO
private String databaseId;
private TransportChannelProvider channelProvider = null;
private String host;
private TransportOptions transportOptions;

@Nonnull
private TransportOptions transportOptions =
new DatastoreDefaults().getDefaultTransportOptions();

@Nullable private DatastoreOpenTelemetryOptions openTelemetryOptions = null;

Expand All @@ -141,13 +145,35 @@ private Builder(DatastoreOptions options) {
this.databaseId = options.databaseId;
this.openTelemetryOptions = options.openTelemetryOptions;
this.channelProvider = validateChannelProvider(options.channelProvider);
this.host = options.getHost();
this.transportOptions = options.getTransportOptions();
Comment on lines 147 to +149
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

These assignments are redundant and potentially introduce a bug. The super(options) call already copies the host and transportOptions fields from the provided options object. By calling options.getHost() and options.getTransportOptions(), you are resolving the defaults and storing them as explicit values in the builder. This breaks the toBuilder() pattern where a user might want to change the transport and have the host automatically switch to its corresponding default. Per repository guidelines, reuse pre-configured objects from options classes directly instead of creating new ones and copying settings.

Suggested change
this.channelProvider = validateChannelProvider(options.channelProvider);
this.host = options.getHost();
this.transportOptions = options.getTransportOptions();
this.channelProvider = validateChannelProvider(options.channelProvider);
References
  1. Reuse pre-configured objects from options classes directly instead of creating new ones and copying settings.

}

private TransportChannelProvider validateChannelProvider(
TransportChannelProvider channelProvider) {
Preconditions.checkNotNull(channelProvider, "TransportChannelProvider cannot be null");
if (!(channelProvider instanceof InstantiatingGrpcChannelProvider)) {
throw new IllegalArgumentException(
"Only GRPC channels are allowed for " + API_SHORT_NAME + ".");
}
return channelProvider;
}

/**
* Sets the transport options.
*
* @param transportOptions the transport options to set, must be {@link HttpTransportOptions} or
* {@link GrpcTransportOptions}
* @return the builder
* @throws IllegalArgumentException if the transport options are not supported
*/
@Override
public Builder setTransportOptions(TransportOptions transportOptions) {
if (!(transportOptions instanceof HttpTransportOptions)) {
public Builder setTransportOptions(@Nonnull TransportOptions transportOptions) {
Preconditions.checkNotNull(transportOptions, "TransportOptions cannot be null");
if (!(transportOptions instanceof HttpTransportOptions)
&& !(transportOptions instanceof GrpcTransportOptions)) {
throw new IllegalArgumentException(
"Only http transport is allowed for " + API_SHORT_NAME + ".");
"Only http and grpc transport are allowed for " + API_SHORT_NAME + ".");
}
this.transportOptions = transportOptions;
return super.setTransportOptions(transportOptions);
Comment on lines 178 to 179
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The assignment this.transportOptions = transportOptions; is redundant because super.setTransportOptions(transportOptions) already performs this assignment in the base class.

Suggested change
this.transportOptions = transportOptions;
return super.setTransportOptions(transportOptions);
return super.setTransportOptions(transportOptions);

Expand Down Expand Up @@ -185,10 +211,28 @@ public Builder setChannelProvider(TransportChannelProvider channelProvider) {
return this;
}

/**
* Builds the {@link DatastoreOptions} instance.
*
* <p>If the host is not explicitly set, it defaults to the transport-specific default host:
*
* <ul>
* <li>gRPC: {@code datastore.googleapis.com:443}
* <li>HTTP: {@code https://datastore.googleapis.com}
* </ul>
*
* @return the {@link DatastoreOptions} instance
*/
@Override
public DatastoreOptions build() {
if (this.host == null && this.transportOptions instanceof GrpcTransportOptions) {
this.setHost(DatastoreSettings.getDefaultEndpoint());
if (this.host == null) {
// Use whatever host value the user passes in, otherwise use the transport specific default
// host values
if (this.transportOptions instanceof GrpcTransportOptions) {
this.setHost(DatastoreSettings.getDefaultEndpoint());
} else if (this.transportOptions instanceof HttpTransportOptions) {
this.setHost(com.google.datastore.v1.client.DatastoreFactory.DEFAULT_HOST);
}
}
return new DatastoreOptions(this);
}
Expand Down Expand Up @@ -218,15 +262,6 @@ public Builder setOpenTelemetryOptions(
}
}

private static TransportChannelProvider validateChannelProvider(
TransportChannelProvider channelProvider) {
if (channelProvider != null && !(channelProvider instanceof InstantiatingGrpcChannelProvider)) {
throw new IllegalArgumentException(
"Only GRPC channels are allowed for " + API_SHORT_NAME + ".");
}
return channelProvider;
}

private DatastoreOptions(Builder builder) {
super(DatastoreFactory.class, DatastoreRpcFactory.class, builder, new DatastoreDefaults());

Expand Down Expand Up @@ -310,8 +345,8 @@ public TransportOptions getDefaultTransportOptions() {
return TRANSPORT_OPTIONS;
}

public static HttpTransportOptions.Builder getDefaultTransportOptionsBuilder() {
return HttpTransportOptions.newBuilder();
public static GrpcTransportOptions.Builder getDefaultTransportOptionsBuilder() {
return GrpcTransportOptions.newBuilder();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,17 @@ public void testInvalidConfigForHttp() {

@Test
public void testTransport() {
// default http transport
assertThat(options.build().getTransportOptions()).isInstanceOf(HttpTransportOptions.class);
// default grpc transport
assertThat(options.build().getTransportOptions()).isInstanceOf(GrpcTransportOptions.class);

// custom http transport
DatastoreOptions httpTransportOptions =
DatastoreOptions.newBuilder()
.setTransportOptions(HttpTransportOptions.newBuilder().build())
.setProjectId(PROJECT_ID)
.setCredentials(NoCredentials.getInstance())
.build();
assertThat(httpTransportOptions.getTransportOptions()).isInstanceOf(HttpTransportOptions.class);

// custom grpc transport
DatastoreOptions grpcTransportOptions =
Expand Down Expand Up @@ -317,15 +326,24 @@ public void testHostWithGrpcAndHttp() {
.build();
assertThat(grpcTransportOptionsCustomHost.getHost()).isEqualTo(customHost);

DatastoreOptions defaultTransportOptions =
DatastoreOptions.newBuilder()
.setProjectId(PROJECT_ID)
.setCredentials(NoCredentials.getInstance())
.build();
assertThat(defaultTransportOptions.getHost()).isEqualTo(DatastoreSettings.getDefaultEndpoint());

DatastoreOptions httpTransportOptions =
DatastoreOptions.newBuilder()
.setTransportOptions(HttpTransportOptions.newBuilder().build())
.setProjectId(PROJECT_ID)
.setCredentials(NoCredentials.getInstance())
.build();
assertThat(httpTransportOptions.getHost()).isEqualTo(DatastoreFactory.DEFAULT_HOST);

DatastoreOptions httpTransportOptionsCustomHost =
DatastoreOptions.newBuilder()
.setTransportOptions(HttpTransportOptions.newBuilder().build())
.setHost(customHost)
.setProjectId(PROJECT_ID)
.setCredentials(NoCredentials.getInstance())
Expand Down
Loading