gemini-code-assist[bot] commented on code in PR #39363:
URL: https://github.com/apache/beam/pull/39363#discussion_r3601466480
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingService.java:
##########
@@ -510,8 +510,10 @@ private String createFilename(String environment,
RunnerApi.ArtifactInformation
// all path separators.
List<String> components =
Splitter.onPattern("[^A-Za-z-_.]]").splitToList(path);
String base = components.get(components.size() - 1);
+ String sanitizedEnvironment =
environment.replaceAll("[<>:\"/\\\\|?*]", "_");
Review Comment:
To avoid compiling the regular expression pattern on every invocation of
`createFilename`, it is highly recommended to precompile the pattern as a
`private static final Pattern` constant at the class level.
For example:
```java
private static final Pattern INVALID_WINDOWS_CHARS =
Pattern.compile("[<>:\"/\\\\|?*]");
```
```suggestion
String sanitizedEnvironment =
INVALID_WINDOWS_CHARS.matcher(environment).replaceAll("_");
```
##########
runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingServiceTest.java:
##########
@@ -166,6 +166,27 @@ public void testStageArtifacts() throws
InterruptedException, ExecutionException
checkArtifacts(contentsList, staged.get("env2"));
}
+ @Test
+ public void testStageArtifactsWithInvalidFilenameCharacters()
+ throws InterruptedException, ExecutionException {
+ String environment = "0:ref_Environment_default";
+ List<String> contentsList = ImmutableList.of("artifact-content");
+
+ stagingService.registerJob(
+ "stagingToken",
+ ImmutableMap.of(
+ environment,
+ Lists.transform(contentsList,
FakeArtifactRetrievalService::resolvedArtifact)));
+
+ ArtifactStagingService.offer(new FakeArtifactRetrievalService(),
stagingStub, "stagingToken");
+
+ Map<String, List<RunnerApi.ArtifactInformation>> staged =
+ stagingService.getStagedArtifacts("stagingToken");
+
+ assertEquals(1, staged.size());
+ checkArtifacts(contentsList, staged.get(environment));
Review Comment:
Since `:` is a valid filename character on Unix-like operating systems (such
as Linux and macOS), this test will pass on those platforms even without the
sanitization fix. To ensure the test actually verifies the sanitization
behavior across all platforms (including CI environments running on Linux),
consider asserting that the physical file created in the staging directory
contains the sanitized environment string (`0_ref_Environment_default`) and
does not contain the colon (`:`).
--
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]
