gemini-code-assist[bot] commented on code in PR #36418:
URL: https://github.com/apache/beam/pull/36418#discussion_r2426810806
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/util/construction/ValidateRunnerXlangTest.java:
##########
@@ -286,6 +303,120 @@ public void test() {
}
}
+ /**
+ * Motivation behind GroupByKeyWithGbekTest.
+ *
+ * <p>Target transform – GroupByKey
+ * (https://beam.apache.org/documentation/programming-guide/#groupbykey)
Test scenario – Grouping
+ * a collection of KV<K,V> to a collection of KV<K, Iterable<V>> by key
Boundary conditions
+ * checked – –> PCollection<KV<?, ?>> to external transforms –>
PCollection<KV<?, Iterable<?>>>
+ * from external transforms while using GroupByEncryptedKey overrides
+ */
+ @RunWith(JUnit4.class)
+ public static class GroupByKeyWithGbekTest extends
ValidateRunnerXlangTestBase {
+ @Rule public ExpectedException thrown = ExpectedException.none();
+ private static final String PROJECT_ID = "apache-beam-testing";
+ private static final String SECRET_ID = "gbek-test";
+ private static String gcpSecretVersionName;
+ private static String secretId;
+
+ @BeforeClass
+ public static void setUpClass() {
+ secretId = String.format("%s-%d", SECRET_ID, new
SecureRandom().nextInt(10000));
+ SecretManagerServiceClient client;
+ try {
+ client = SecretManagerServiceClient.create();
+ } catch (IOException e) {
+ gcpSecretVersionName = null;
+ return;
+ }
+ ProjectName projectName = ProjectName.of(PROJECT_ID);
+ SecretName secretName = SecretName.of(PROJECT_ID, secretId);
+
+ try {
+ client.getSecret(secretName);
+ } catch (Exception e) {
+ com.google.cloud.secretmanager.v1.Secret secret =
+ com.google.cloud.secretmanager.v1.Secret.newBuilder()
+ .setReplication(
+ com.google.cloud.secretmanager.v1.Replication.newBuilder()
+ .setAutomatic(
+
com.google.cloud.secretmanager.v1.Replication.Automatic.newBuilder()
+ .build())
+ .build())
+ .build();
+ client.createSecret(projectName, secretId, secret);
+ byte[] secretBytes = new byte[32];
+ new SecureRandom().nextBytes(secretBytes);
+ client.addSecretVersion(
+ secretName,
+ SecretPayload.newBuilder()
+
.setData(ByteString.copyFrom(java.util.Base64.getUrlEncoder().encode(secretBytes)))
+ .build());
+ }
+ gcpSecretVersionName = secretName.toString() + "/versions/latest";
+ expansionAddr =
+ String.format("localhost:%s",
Integer.valueOf(System.getProperty("expansionPort")));
+ }
+
+ @AfterClass
+ public static void tearDownClass() {
+ if (gcpSecretVersionName != null) {
+ try {
+ SecretManagerServiceClient client =
SecretManagerServiceClient.create();
+ SecretName secretName = SecretName.of(PROJECT_ID, secretId);
+ client.deleteSecret(secretName);
+ } catch (IOException e) {
+ // Do nothing.
+ }
+ }
+ }
Review Comment:
`SecretManagerServiceClient` is `AutoCloseable` and should be closed to
prevent resource leaks. Using a try-with-resources block is the recommended way
to ensure the client is always closed. Additionally, catching a broader
`Exception` in cleanup methods makes them more robust.
```java
public static void tearDownClass() {
if (gcpSecretVersionName != null) {
try (SecretManagerServiceClient client =
SecretManagerServiceClient.create()) {
SecretName secretName = SecretName.of(PROJECT_ID, secretId);
client.deleteSecret(secretName);
} catch (Exception e) {
// Suppress exceptions during cleanup.
}
}
}
```
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/util/construction/ValidateRunnerXlangTest.java:
##########
@@ -286,6 +303,120 @@ public void test() {
}
}
+ /**
+ * Motivation behind GroupByKeyWithGbekTest.
+ *
+ * <p>Target transform – GroupByKey
+ * (https://beam.apache.org/documentation/programming-guide/#groupbykey)
Test scenario – Grouping
+ * a collection of KV<K,V> to a collection of KV<K, Iterable<V>> by key
Boundary conditions
+ * checked – –> PCollection<KV<?, ?>> to external transforms –>
PCollection<KV<?, Iterable<?>>>
+ * from external transforms while using GroupByEncryptedKey overrides
+ */
+ @RunWith(JUnit4.class)
+ public static class GroupByKeyWithGbekTest extends
ValidateRunnerXlangTestBase {
+ @Rule public ExpectedException thrown = ExpectedException.none();
+ private static final String PROJECT_ID = "apache-beam-testing";
+ private static final String SECRET_ID = "gbek-test";
+ private static String gcpSecretVersionName;
+ private static String secretId;
+
+ @BeforeClass
+ public static void setUpClass() {
+ secretId = String.format("%s-%d", SECRET_ID, new
SecureRandom().nextInt(10000));
+ SecretManagerServiceClient client;
+ try {
+ client = SecretManagerServiceClient.create();
+ } catch (IOException e) {
+ gcpSecretVersionName = null;
+ return;
+ }
+ ProjectName projectName = ProjectName.of(PROJECT_ID);
+ SecretName secretName = SecretName.of(PROJECT_ID, secretId);
+
+ try {
+ client.getSecret(secretName);
+ } catch (Exception e) {
+ com.google.cloud.secretmanager.v1.Secret secret =
+ com.google.cloud.secretmanager.v1.Secret.newBuilder()
+ .setReplication(
+ com.google.cloud.secretmanager.v1.Replication.newBuilder()
+ .setAutomatic(
+
com.google.cloud.secretmanager.v1.Replication.Automatic.newBuilder()
+ .build())
+ .build())
+ .build();
+ client.createSecret(projectName, secretId, secret);
+ byte[] secretBytes = new byte[32];
+ new SecureRandom().nextBytes(secretBytes);
+ client.addSecretVersion(
+ secretName,
+ SecretPayload.newBuilder()
+
.setData(ByteString.copyFrom(java.util.Base64.getUrlEncoder().encode(secretBytes)))
+ .build());
+ }
+ gcpSecretVersionName = secretName.toString() + "/versions/latest";
+ expansionAddr =
+ String.format("localhost:%s",
Integer.valueOf(System.getProperty("expansionPort")));
+ }
Review Comment:
`SecretManagerServiceClient` is `AutoCloseable` and should be closed to
prevent resource leaks. Using a try-with-resources block is the recommended way
to ensure the client is always closed, even when exceptions occur.
```java
public static void setUpClass() {
secretId = String.format("%s-%d", SECRET_ID, new
SecureRandom().nextInt(10000));
try (SecretManagerServiceClient client =
SecretManagerServiceClient.create()) {
ProjectName projectName = ProjectName.of(PROJECT_ID);
SecretName secretName = SecretName.of(PROJECT_ID, secretId);
try {
client.getSecret(secretName);
} catch (Exception e) {
com.google.cloud.secretmanager.v1.Secret secret =
com.google.cloud.secretmanager.v1.Secret.newBuilder()
.setReplication(
com.google.cloud.secretmanager.v1.Replication.newBuilder()
.setAutomatic(
com.google.cloud.secretmanager.v1.Replication.Automatic.newBuilder()
.build())
.build())
.build();
client.createSecret(projectName, secretId, secret);
byte[] secretBytes = new byte[32];
new SecureRandom().nextBytes(secretBytes);
client.addSecretVersion(
secretName,
SecretPayload.newBuilder()
.setData(ByteString.copyFrom(java.util.Base64.getUrlEncoder().encode(secretBytes)))
.build());
}
gcpSecretVersionName = secretName.toString() + "/versions/latest";
expansionAddr =
String.format("localhost:%s",
Integer.valueOf(System.getProperty("expansionPort")));
} catch (IOException e) {
gcpSecretVersionName = null;
}
}
```
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/util/construction/ValidateRunnerXlangTest.java:
##########
@@ -286,6 +303,120 @@ public void test() {
}
}
+ /**
+ * Motivation behind GroupByKeyWithGbekTest.
+ *
+ * <p>Target transform – GroupByKey
+ * (https://beam.apache.org/documentation/programming-guide/#groupbykey)
Test scenario – Grouping
+ * a collection of KV<K,V> to a collection of KV<K, Iterable<V>> by key
Boundary conditions
+ * checked – –> PCollection<KV<?, ?>> to external transforms –>
PCollection<KV<?, Iterable<?>>>
+ * from external transforms while using GroupByEncryptedKey overrides
+ */
+ @RunWith(JUnit4.class)
+ public static class GroupByKeyWithGbekTest extends
ValidateRunnerXlangTestBase {
+ @Rule public ExpectedException thrown = ExpectedException.none();
+ private static final String PROJECT_ID = "apache-beam-testing";
+ private static final String SECRET_ID = "gbek-test";
+ private static String gcpSecretVersionName;
+ private static String secretId;
+
+ @BeforeClass
+ public static void setUpClass() {
+ secretId = String.format("%s-%d", SECRET_ID, new
SecureRandom().nextInt(10000));
+ SecretManagerServiceClient client;
+ try {
+ client = SecretManagerServiceClient.create();
+ } catch (IOException e) {
+ gcpSecretVersionName = null;
+ return;
+ }
+ ProjectName projectName = ProjectName.of(PROJECT_ID);
+ SecretName secretName = SecretName.of(PROJECT_ID, secretId);
+
+ try {
+ client.getSecret(secretName);
+ } catch (Exception e) {
+ com.google.cloud.secretmanager.v1.Secret secret =
+ com.google.cloud.secretmanager.v1.Secret.newBuilder()
+ .setReplication(
+ com.google.cloud.secretmanager.v1.Replication.newBuilder()
+ .setAutomatic(
+
com.google.cloud.secretmanager.v1.Replication.Automatic.newBuilder()
+ .build())
+ .build())
+ .build();
+ client.createSecret(projectName, secretId, secret);
+ byte[] secretBytes = new byte[32];
+ new SecureRandom().nextBytes(secretBytes);
+ client.addSecretVersion(
+ secretName,
+ SecretPayload.newBuilder()
+
.setData(ByteString.copyFrom(java.util.Base64.getUrlEncoder().encode(secretBytes)))
+ .build());
+ }
+ gcpSecretVersionName = secretName.toString() + "/versions/latest";
+ expansionAddr =
+ String.format("localhost:%s",
Integer.valueOf(System.getProperty("expansionPort")));
+ }
+
+ @AfterClass
+ public static void tearDownClass() {
+ if (gcpSecretVersionName != null) {
+ try {
+ SecretManagerServiceClient client =
SecretManagerServiceClient.create();
+ SecretName secretName = SecretName.of(PROJECT_ID, secretId);
+ client.deleteSecret(secretName);
+ } catch (IOException e) {
+ // Do nothing.
+ }
+ }
+ }
+
+ @After
+ @Override
+ public void tearDown() {
+ // Override tearDown since we're doing our own assertion instead of
relying on base class
+ // assertions
+ }
+
+ @Test
+ @Category({
+ ValidatesRunner.class,
+ UsesJavaExpansionService.class,
+ UsesPythonExpansionService.class
+ })
+ public void test() {
+ if (gcpSecretVersionName == null) {
+ // Skip test if we couldn't set up secret manager
+ return;
+ }
+ PipelineOptions options = TestPipeline.testingPipelineOptions();
+ options.setGbek(String.format("type:gcpsecret;version_name:%s",
gcpSecretVersionName));
+ Pipeline pipeline = Pipeline.create(options);
+ groupByKeyTest(pipeline);
+ PipelineResult pipelineResult = pipeline.run();
+ pipelineResult.waitUntilFinish();
+ assertThat(pipelineResult.getState(),
equalTo(PipelineResult.State.DONE));
+ }
+
+ @Test
+ @Category({
+ ValidatesRunner.class,
+ UsesJavaExpansionService.class,
+ UsesPythonExpansionService.class
+ })
+ public void testFailure() {
+ thrown.expect(Exception.class);
+ PipelineOptions options = TestPipeline.testingPipelineOptions();
+ options.setGbek("version_name:fake_secret");
+ Pipeline pipeline = Pipeline.create(options);
+ groupByKeyTest(pipeline);
+ PipelineResult pipelineResult = pipeline.run();
+ pipelineResult.waitUntilFinish();
+ assertThat(pipelineResult.getState(),
equalTo(PipelineResult.State.DONE));
+ }
Review Comment:
The `gbek` option `version_name:fake_secret` is syntactically incorrect
because it's missing the `type` field. This will cause the test to fail during
option parsing. To create a more robust failure test that checks the secret
fetching logic, consider using a syntactically correct option string with a
non-existent secret. Also, the `assertThat` call is unreachable if the test
passes as expected, so it can be removed for clarity.
```java
public void testFailure() {
thrown.expect(Exception.class);
PipelineOptions options = TestPipeline.testingPipelineOptions();
options.setGbek("type:gcpsecret;version_name:non-existent-secret");
Pipeline pipeline = Pipeline.create(options);
groupByKeyTest(pipeline);
pipeline.run().waitUntilFinish();
}
```
--
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]
