gemini-code-assist[bot] commented on code in PR #36048:
URL: https://github.com/apache/beam/pull/36048#discussion_r2321830786
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/MetadataSpannerConfigFactory.java:
##########
@@ -77,6 +77,26 @@ public static SpannerConfig create(
config =
config.withEmulatorHost(StaticValueProvider.of(emulatorHost.get()));
}
+ ValueProvider<String> experimentalHost =
primaryConfig.getExperimentalHost();
+ if (experimentalHost != null) {
+ config =
+ config.withExperimentalHost(
+ String.valueOf(StaticValueProvider.of(experimentalHost.get())));
+ }
+
+ ValueProvider<Boolean> plainText = primaryConfig.getPlainText();
+ if (plainText != null) {
+ config =
+ config.withPlainText(
+
Boolean.valueOf(String.valueOf(StaticValueProvider.of(plainText.get()))));
+ }
+
+ ValueProvider<String> clientCert = primaryConfig.getClientCertPath();
+ ValueProvider<String> clientKey = primaryConfig.getClientCertKeyPath();
+ if (clientCert != null && clientKey != null) {
+ config = config.withClientCert(String.valueOf(clientCert),
String.valueOf(clientKey));
+ }
Review Comment:
The logic for propagating experimental host, plaintext, and client
certificate settings is incorrect. It misuses `String.valueOf` and
`Boolean.valueOf` on `ValueProvider` objects, which will lead to incorrect
configuration values. The values should be extracted from the `ValueProvider`s
before being passed to the config builder methods.
```java
ValueProvider<String> experimentalHost =
primaryConfig.getExperimentalHost();
if (experimentalHost != null && experimentalHost.get() != null) {
config = config.withExperimentalHost(experimentalHost.get());
}
ValueProvider<Boolean> plainText = primaryConfig.getPlainText();
if (plainText != null && plainText.get() != null) {
config = config.withPlainText(plainText.get());
}
ValueProvider<String> clientCert = primaryConfig.getClientCertPath();
ValueProvider<String> clientKey = primaryConfig.getClientCertKeyPath();
if (clientCert != null
&& clientCert.get() != null
&& clientKey != null
&& clientKey.get() != null) {
config = config.withClientCert(clientCert.get(), clientKey.get());
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerConfig.java:
##########
@@ -345,4 +361,24 @@ public SpannerConfig withCredentials(Credentials
credentials) {
public SpannerConfig withCredentials(ValueProvider<Credentials> credentials)
{
return toBuilder().setCredentials(credentials).build();
}
+
+ /** Specifies the experimental host to set on SpannerOptions
(setExperimentalHost). */
+ public SpannerConfig withExperimentalHost(String experimentalHost) {
+ return toBuilder()
+
.setExperimentalHost((ValueProvider.StaticValueProvider.of(experimentalHost)))
+ .build();
+ }
+
+ /** Specifies whether to use plaintext channel. */
+ public SpannerConfig withPlainText(boolean plainText) {
+ return
toBuilder().setPlainText(ValueProvider.StaticValueProvider.of(plainText)).build();
+ }
+
+ /** Specifies certificate paths to use for mTLS channel. */
+ public SpannerConfig withClientCert(String certPath, String keyPath) {
+ return toBuilder()
+ .setClientCertPath(ValueProvider.StaticValueProvider.of(certPath))
+ .setClientCertKeyPath(ValueProvider.StaticValueProvider.of(keyPath))
+ .build();
+ }
Review Comment:
To correctly support `ValueProvider` arguments in `SpannerIO` transforms,
you should add overloaded `with...` methods that accept `ValueProvider`s,
similar to other configuration options like `withEmulatorHost`. The current
implementation in `SpannerIO` for these new options is incorrect because it
calls `String.valueOf()` on the `ValueProvider` itself.
The `with...` methods that take primitive types should then delegate to the
`ValueProvider` overloads. This also provides an opportunity to remove the
redundant cast in `withExperimentalHost`.
```java
/** Specifies the experimental host to set on SpannerOptions
(setExperimentalHost). */
public SpannerConfig withExperimentalHost(ValueProvider<String>
experimentalHost) {
return toBuilder().setExperimentalHost(experimentalHost).build();
}
/** Specifies the experimental host to set on SpannerOptions
(setExperimentalHost). */
public SpannerConfig withExperimentalHost(String experimentalHost) {
return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
}
/** Specifies whether to use plaintext channel. */
public SpannerConfig withPlainText(ValueProvider<Boolean> plainText) {
return toBuilder().setPlainText(plainText).build();
}
/** Specifies whether to use plaintext channel. */
public SpannerConfig withPlainText(boolean plainText) {
return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
}
/** Specifies certificate paths to use for mTLS channel. */
public SpannerConfig withClientCert(
ValueProvider<String> certPath, ValueProvider<String> keyPath) {
return toBuilder()
.setClientCertPath(certPath)
.setClientCertKeyPath(keyPath)
.build();
}
/** Specifies certificate paths to use for mTLS channel. */
public SpannerConfig withClientCert(String certPath, String keyPath) {
return withClientCert(
ValueProvider.StaticValueProvider.of(certPath),
ValueProvider.StaticValueProvider.of(keyPath));
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -1137,6 +1205,41 @@
return
withEmulatorHost(ValueProvider.StaticValueProvider.of(emulatorHost));
}
+ /** Specifies the SpannerOptions experimental host (setExperimentalHost).
*/
+ public CreateTransaction withExperimentalHost(ValueProvider<String>
experimentalHost) {
+ SpannerConfig config = getSpannerConfig();
+ return
withSpannerConfig(config.withExperimentalHost(String.valueOf(experimentalHost)));
+ }
+
+ public CreateTransaction withExperimentalHost(String experimentalHost) {
+ return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
+ }
+
+ /** Specifies whether to use plaintext channel. */
+ public CreateTransaction withPlainText(ValueProvider<Boolean> plainText) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+
config.withPlainText(Boolean.parseBoolean(String.valueOf(plainText))));
+ }
+
+ public CreateTransaction withPlainText(boolean plainText) {
+ return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
+ }
+
+ /** Specifies client certificate and key paths for mTLS. */
+ public CreateTransaction withClientCert(
+ ValueProvider<String> certPath, ValueProvider<String> keyPath) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+ config.withClientCert(String.valueOf(certPath),
String.valueOf(keyPath)));
+ }
Review Comment:
The `with...` methods that accept `ValueProvider` arguments are implemented
incorrectly. They use `String.valueOf()` or
`Boolean.parseBoolean(String.valueOf())` on the `ValueProvider` object itself,
which does not retrieve the contained value. This will lead to incorrect
behavior at runtime.
This pattern should be corrected for `withExperimentalHost`,
`withPlainText`, and `withClientCert` by calling the corresponding `with...`
methods on `SpannerConfig` that accept `ValueProvider`s (which need to be
added).
```java
public CreateTransaction withExperimentalHost(ValueProvider<String>
experimentalHost) {
SpannerConfig config = getSpannerConfig();
return
withSpannerConfig(config.withExperimentalHost(experimentalHost));
}
public CreateTransaction withExperimentalHost(String experimentalHost) {
return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
}
/** Specifies whether to use plaintext channel. */
public CreateTransaction withPlainText(ValueProvider<Boolean> plainText)
{
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withPlainText(plainText));
}
public CreateTransaction withPlainText(boolean plainText) {
return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
}
/** Specifies client certificate and key paths for mTLS. */
public CreateTransaction withClientCert(
ValueProvider<String> certPath, ValueProvider<String> keyPath) {
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withClientCert(certPath, keyPath));
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -841,6 +875,40 @@
return
withEmulatorHost(ValueProvider.StaticValueProvider.of(emulatorHost));
}
+ /** Specifies the SpannerOptions experimental host (setExperimentalHost).
*/
+ public Read withExperimentalHost(ValueProvider<String> experimentalHost) {
+ SpannerConfig config = getSpannerConfig();
+ return
withSpannerConfig(config.withExperimentalHost(String.valueOf(experimentalHost)));
+ }
+
+ public Read withExperimentalHost(String experimentalHost) {
+ return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
+ }
+
+ /** Specifies whether to use plaintext channel. */
+ public Read withPlainText(ValueProvider<Boolean> plainText) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+
config.withPlainText(Boolean.parseBoolean(String.valueOf(plainText))));
+ }
+
+ public Read withPlainText(boolean plainText) {
+ return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
+ }
+
+ /** Specifies client certificate and key paths for mTLS. */
+ public Read withClientCert(ValueProvider<String> certPath,
ValueProvider<String> keyPath) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+ config.withClientCert(String.valueOf(certPath),
String.valueOf(keyPath)));
+ }
Review Comment:
The `with...` methods that accept `ValueProvider` arguments are implemented
incorrectly. They use `String.valueOf()` or
`Boolean.parseBoolean(String.valueOf())` on the `ValueProvider` object itself,
which does not retrieve the contained value. This will lead to incorrect
behavior at runtime.
This pattern should be corrected for `withExperimentalHost`,
`withPlainText`, and `withClientCert` by calling the corresponding `with...`
methods on `SpannerConfig` that accept `ValueProvider`s (which need to be
added).
```java
public Read withExperimentalHost(ValueProvider<String> experimentalHost)
{
SpannerConfig config = getSpannerConfig();
return
withSpannerConfig(config.withExperimentalHost(experimentalHost));
}
public Read withExperimentalHost(String experimentalHost) {
return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
}
/** Specifies whether to use plaintext channel. */
public Read withPlainText(ValueProvider<Boolean> plainText) {
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withPlainText(plainText));
}
public Read withPlainText(boolean plainText) {
return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
}
/** Specifies client certificate and key paths for mTLS. */
public Read withClientCert(ValueProvider<String> certPath,
ValueProvider<String> keyPath) {
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withClientCert(certPath, keyPath));
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -1274,6 +1377,40 @@
return
withEmulatorHost(ValueProvider.StaticValueProvider.of(emulatorHost));
}
+ /** Specifies the SpannerOptions experimental host (setExperimentalHost).
*/
+ public Write withExperimentalHost(ValueProvider<String> experimentalHost) {
+ SpannerConfig config = getSpannerConfig();
+ return
withSpannerConfig(config.withExperimentalHost(String.valueOf(experimentalHost)));
+ }
+
+ public Write withExperimentalHost(String experimentalHost) {
+ return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
+ }
+
+ /** Specifies whether to use plaintext channel. */
+ public Write withPlainText(ValueProvider<Boolean> plainText) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+
config.withPlainText(Boolean.parseBoolean(String.valueOf(plainText))));
+ }
+
+ public Write withPlainText(boolean plainText) {
+ return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
+ }
+
+ /** Specifies client certificate and key paths for mTLS. */
+ public Write withClientCert(ValueProvider<String> certPath,
ValueProvider<String> keyPath) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+ config.withClientCert(String.valueOf(certPath),
String.valueOf(keyPath)));
+ }
Review Comment:
The `with...` methods that accept `ValueProvider` arguments are implemented
incorrectly. They use `String.valueOf()` or
`Boolean.parseBoolean(String.valueOf())` on the `ValueProvider` object itself,
which does not retrieve the contained value. This will lead to incorrect
behavior at runtime.
This pattern should be corrected for `withExperimentalHost`,
`withPlainText`, and `withClientCert` by calling the corresponding `with...`
methods on `SpannerConfig` that accept `ValueProvider`s (which need to be
added).
```java
public Write withExperimentalHost(ValueProvider<String>
experimentalHost) {
SpannerConfig config = getSpannerConfig();
return
withSpannerConfig(config.withExperimentalHost(experimentalHost));
}
public Write withExperimentalHost(String experimentalHost) {
return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
}
/** Specifies whether to use plaintext channel. */
public Write withPlainText(ValueProvider<Boolean> plainText) {
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withPlainText(plainText));
}
public Write withPlainText(boolean plainText) {
return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
}
/** Specifies client certificate and key paths for mTLS. */
public Write withClientCert(ValueProvider<String> certPath,
ValueProvider<String> keyPath) {
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withClientCert(certPath, keyPath));
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerAccessor.java:
##########
@@ -220,6 +220,29 @@ static SpannerOptions buildSpannerOptions(SpannerConfig
spannerConfig) {
builder.setServiceFactory(serviceFactory);
}
builder.setHost(spannerConfig.getHostValue());
+
+ ValueProvider<String> experimentalHost =
spannerConfig.getExperimentalHost();
+ if (experimentalHost != null
+ && experimentalHost.get() != null
+ && !experimentalHost.get().isEmpty()) {
+ builder.setExperimentalHost(experimentalHost.get());
+ }
+ ValueProvider<Boolean> plainText = spannerConfig.getPlainText();
+ if (plainText != null && Boolean.TRUE.equals(plainText.get())) {
+ builder.setChannelConfigurator(b -> b.usePlaintext());
+ builder.setCredentials(NoCredentials.getInstance());
+ }
+ ValueProvider<String> clientCert = spannerConfig.getClientCertPath();
+ ValueProvider<String> clientKey = spannerConfig.getClientCertKeyPath();
+ if (clientCert != null
+ && clientKey != null
+ && clientCert.get() != null
+ && clientKey.get() != null
+ && !clientCert.get().isEmpty()
+ && !clientKey.get().isEmpty()) {
Review Comment:
The checks for null and empty strings can be simplified by using
`com.google.common.base.Strings.isNullOrEmpty()`, which is already used
elsewhere in this file. This will improve readability and reduce verbosity.
```java
if (experimentalHost != null &&
!org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Strings.isNullOrEmpty(experimentalHost.get()))
{
builder.setExperimentalHost(experimentalHost.get());
}
ValueProvider<Boolean> plainText = spannerConfig.getPlainText();
if (plainText != null && Boolean.TRUE.equals(plainText.get())) {
builder.setChannelConfigurator(b -> b.usePlaintext());
builder.setCredentials(NoCredentials.getInstance());
}
ValueProvider<String> clientCert = spannerConfig.getClientCertPath();
ValueProvider<String> clientKey = spannerConfig.getClientCertKeyPath();
if (clientCert != null
&& clientKey != null
&&
!org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Strings.isNullOrEmpty(clientCert.get())
&&
!org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Strings.isNullOrEmpty(clientKey.get()))
{
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -612,6 +612,40 @@ public ReadAll withEmulatorHost(String emulatorHost) {
return
withEmulatorHost(ValueProvider.StaticValueProvider.of(emulatorHost));
}
+ /** Specifies the SpannerOptions experimental host (setExperimentalHost).
*/
+ public ReadAll withExperimentalHost(ValueProvider<String>
experimentalHost) {
+ SpannerConfig config = getSpannerConfig();
+ return
withSpannerConfig(config.withExperimentalHost(String.valueOf(experimentalHost)));
+ }
+
+ public ReadAll withExperimentalHost(String experimentalHost) {
+ return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
+ }
+
+ /** Specifies whether to use plaintext channel. */
+ public ReadAll withPlainText(ValueProvider<Boolean> plainText) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+
config.withPlainText(Boolean.parseBoolean(String.valueOf(plainText))));
+ }
+
+ public ReadAll withPlainText(boolean plainText) {
+ return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
+ }
+
+ /** Specifies client certificate and key paths for mTLS. */
+ public ReadAll withClientCert(ValueProvider<String> certPath,
ValueProvider<String> keyPath) {
+ SpannerConfig config = getSpannerConfig();
+ return withSpannerConfig(
+ config.withClientCert(String.valueOf(certPath),
String.valueOf(keyPath)));
+ }
Review Comment:
The `with...` methods that accept `ValueProvider` arguments are implemented
incorrectly. They use `String.valueOf()` or
`Boolean.parseBoolean(String.valueOf())` on the `ValueProvider` object itself,
which does not retrieve the contained value. This will lead to incorrect
behavior at runtime.
This pattern should be corrected for `withExperimentalHost`,
`withPlainText`, and `withClientCert` by calling the corresponding `with...`
methods on `SpannerConfig` that accept `ValueProvider`s (which need to be
added).
```java
public ReadAll withExperimentalHost(ValueProvider<String>
experimentalHost) {
SpannerConfig config = getSpannerConfig();
return
withSpannerConfig(config.withExperimentalHost(experimentalHost));
}
public ReadAll withExperimentalHost(String experimentalHost) {
return
withExperimentalHost(ValueProvider.StaticValueProvider.of(experimentalHost));
}
/** Specifies whether to use plaintext channel. */
public ReadAll withPlainText(ValueProvider<Boolean> plainText) {
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withPlainText(plainText));
}
public ReadAll withPlainText(boolean plainText) {
return withPlainText(ValueProvider.StaticValueProvider.of(plainText));
}
/** Specifies client certificate and key paths for mTLS. */
public ReadAll withClientCert(ValueProvider<String> certPath,
ValueProvider<String> keyPath) {
SpannerConfig config = getSpannerConfig();
return withSpannerConfig(config.withClientCert(certPath, keyPath));
}
```
--
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]
