gemini-code-assist[bot] commented on code in PR #38928:
URL: https://github.com/apache/beam/pull/38928#discussion_r3400784852
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerAccessor.java:
##########
@@ -249,6 +249,14 @@ static SpannerOptions buildSpannerOptions(SpannerConfig
spannerConfig) {
builder.setChannelConfigurator(b -> b.usePlaintext());
builder.setCredentials(NoCredentials.getInstance());
}
+ ValueProvider<String> clientCert = spannerConfig.getClientCertPath();
+ ValueProvider<String> clientKey = spannerConfig.getClientCertKeyPath();
+ if (clientCert != null
+ && clientKey != null
+ && !Strings.isNullOrEmpty(clientCert.get())
+ && !Strings.isNullOrEmpty(clientKey.get())) {
+ builder.useClientCert(clientCert.get(), clientKey.get());
+ }
Review Comment:
Since clientCert and clientKey are ValueProviders, they could be
RuntimeValueProviders. Calling .get() on them without checking isAccessible()
can throw an IllegalStateException during pipeline construction or validation
if they are not yet available. Adding isAccessible() checks prevents this issue.
```java
if (clientCert != null
&& clientKey != null
&& clientCert.isAccessible()
&& clientKey.isAccessible()
&& !Strings.isNullOrEmpty(clientCert.get())
&& !Strings.isNullOrEmpty(clientKey.get())) {
builder.useClientCert(clientCert.get(), clientKey.get());
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerConfig.java:
##########
@@ -367,6 +376,7 @@ public SpannerConfig
withExperimentalHost(ValueProvider<String> experimentalHost
return toBuilder()
.setInstanceId(EXPERIMENTAL_HOST_INSTANCE_ID)
.setExperimentalHost(experimentalHost)
+
.setCredentials(ValueProvider.StaticValueProvider.of(NoCredentials.getInstance()))
.build();
Review Comment:
Unconditionally setting NoCredentials in withExperimentalHost makes the
fluent API order-dependent. If a user calls withCredentials(...) before
withExperimentalHost(...), their credentials will be silently overwritten with
NoCredentials. It is safer to remove this default from SpannerConfig and
instead default to NoCredentials in SpannerAccessor.buildSpannerOptions when
experimentalHost is present and no credentials are set.
```java
return toBuilder()
.setInstanceId(EXPERIMENTAL_HOST_INSTANCE_ID)
.setExperimentalHost(experimentalHost)
.build();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
