Eliaaazzz commented on code in PR #39363:
URL: https://github.com/apache/beam/pull/39363#discussion_r3622149461
##########
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:
nit: consider an allowlist such as `[^A-Za-z0-9-_.]` rather than a denylist.
It would match the shape of the splitter just above and of
`SdkComponents.java:204`, and it also covers ASCII control characters
`0x00-0x1F`, which are invalid on Windows as well. Probably can't occur in an
environment id, so feel free to ignore.
##########
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("[<>:\"/\\\\|?*]", "_");
return clip(
- String.format("%s-%s-%s", idGenerator.getId(), clip(environment,
25), base), 100);
+ String.format("%s-%s-%s", idGenerator.getId(),
clip(sanitizedEnvironment, 25), base),
Review Comment:
nit: a trailing `.` or space is invalid on Windows too, and can still
survive here, either from `base` or from `clip(..., 100)` landing on one. Low
impact, since Windows strips them on both write and read.
--
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]
