You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "AtharvUrunkar (via GitHub)" <gi...@apache.org> on 2026/07/17 08:06:53 UTC

[PR] Fix artifact staging filenames on Windows [beam]

AtharvUrunkar opened a new pull request, #39363:
URL: https://github.com/apache/beam/pull/39363

   Fixes #39336
   
   ## What is the issue?
   
   Artifact staging can fail on Windows when an environment ID contains characters that are invalid in Windows filenames, such as `:`. The environment ID was previously embedded directly into the generated staging filename, causing an `InvalidPathException` before the pipeline could run.
   
   ## What does this PR do?
   
   * Sanitizes Windows-invalid filename characters in environment IDs before using them in staged artifact filenames.
   * Adds a regression test using an environment ID containing `:` to verify that artifact staging succeeds on Windows.
   
   ## Testing
   
   * `./gradlew :runners:java-fn-execution:test --tests "org.apache.beam.runners.fnexecution.artifact.ArtifactStagingServiceTest"`
   * `./gradlew :runners:java-fn-execution:spotlessCheck`
   * `./gradlew :runners:java-fn-execution:check`
   
   All tests and checks pass successfully.
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "AtharvUrunkar (via GitHub)" <gi...@apache.org>.
AtharvUrunkar commented on code in PR #39363:
URL: https://github.com/apache/beam/pull/39363#discussion_r3627902655


##########
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:
   Thanks for tracing this. Good catch on the splitter pattern and `base` potentially carrying invalid filename characters as well.
   
   I agree that an ASCII-only allowlist would be unnecessarily restrictive for valid Unicode filenames. I'll update the fix to preserve Unicode while sanitizing Windows-invalid characters, and I'll also take another look at the splitter issue so that both the environment-derived and artifact-derived parts of the generated filename are safe.
   
   I'll add/update the regression tests accordingly.
   



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Eliaaazzz (via GitHub)" <gi...@apache.org>.
Eliaaazzz commented on code in PR #39363:
URL: https://github.com/apache/beam/pull/39363#discussion_r3627434562


##########
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:
   Yes, you're right. Windows accepts Unicode filenames, so the ASCII-only allowlist would unnecessarily replace valid non-ASCII characters and is probably too restrictive for this fix.
   
   The narrower denylist makes more sense here. If covering control characters is worthwhile, they could instead be added explicitly, for example [\x00-\x1F<>:"/\\|?*], while preserving Unicode. Thanks for pointing that out.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5003830735

   Assigning reviewers:
   
   R: @chamikaramj added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #39363:
URL: https://github.com/apache/beam/pull/39363#discussion_r3623792748


##########
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:
   I understand non-ascii unicode file names are acceptible?



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5590724617

   Merged as part of #39916


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5004373289

   Assigning reviewers:
   
   R: @chamikaramj added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5004855449

   Assigning reviewers:
   
   R: @chamikaramj added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5456583700

   Tested working on https://github.com/Abacn/beam/commit/2ca7cabced837ec9d66f42606d4ac4391d33e1f2
   
   PVR tests can run on Windows:
   
   Command on power shell: `./gradlew :runners:spark:3:job-server:validatesPortableRunnerBatch --no-daemon --tests '*ParDoTest$BasicTests' -PdisableSpotlessCheck=true`
   
   Result:
   
   ```
   Class org.apache.beam.sdk.transforms.ParDoTest$BasicTests
   6 tests
   0 failures
   0 ignored
   2m46.88s duration
   100% successful
   
   Tests
   Standard error
   Test	Duration	Result
   testParDo	26.527s	passed
   testParDoEmpty	26.006s	passed
   testParDoEmptyOutputs	26.256s	passed
   testParDoInCustomTransform	26.254s	passed
   testPipelineOptionsParameter	26.127s	passed
   testSetupParameter	35.706s	passed
   ```


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5022169411

   Assigning reviewers:
   
   R: @damccorm added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5000498333

   ## Summary of Changes
   
   Hello, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
   
   This pull request addresses an issue where artifact staging would fail on Windows due to invalid characters in environment IDs. By sanitizing these IDs before they are used in file paths, the system now avoids InvalidPathException errors, ensuring consistent behavior across different operating systems.
   
   ### Highlights
   
   * **Filename Sanitization**: Introduced character replacement for environment IDs to ensure compatibility with Windows filesystem constraints.
   * **Regression Testing**: Added a new test case to verify that artifact staging handles environment IDs containing invalid characters like colons correctly.
   
   <details>
   <summary><b>New Features</b></summary>
   
   🧠 You can now enable **Memory** (public preview) to help **Gemini Code Assist** learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. **Click [here](https://codeassist.google/code-review/login) to enable Memory in your admin console.**
   </details>
   
   <details>
   <summary><b>Using Gemini Code Assist</b></summary>
   <br>
   
   The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips.
   
   <b>Invoking Gemini</b>
   
   You can request assistance from Gemini at any point by creating a comment using either `/gemini <command>` or `@gemini-code-assist <command>`. Below is a summary of the supported commands on the current page.
   
   Feature | Command | Description
   --- | --- | ---
   Code Review | `/gemini review` | Performs a code review for the current pull request in its current state.
   Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state.
   Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments.
   Help | `/gemini help` | Displays a list of available commands.
   
   <b>Customization</b>
   
   To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a `.gemini/` folder in the base of the repository. Detailed instructions can be found [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github).
   
   <b>Limitations & Feedback</b>
   
   Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up [here](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG).
   </details>
   
   
   [^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487).
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
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:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   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:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5002237379

   Assigning reviewers:
   
   R: @chamikaramj added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Eliaaazzz (via GitHub)" <gi...@apache.org>.
Eliaaazzz commented on code in PR #39363:
URL: https://github.com/apache/beam/pull/39363#discussion_r3622083711


##########
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:
   `base` on the line above isn't sanitized, and I think it can carry the same characters this is fixing.
   
   The splitter pattern has a stray `]` outside the character class, so it only splits on an invalid char followed by a literal `]`, and `base` ends up being the whole `path`:
   
   ```
   path=C:\Users\me\artifact.jar  ->  base=C:\Users\me\artifact.jar
   ```
   
   That branch is taken when `roleUrn != STAGING_TO_ARTIFACT_URN` and `typeUrn == FILE_ARTIFACT_URN`, which is the case for Go pipelines (`graphx/translate.go:147-152`) and for Python `pypi_requirements` (`stager.py:131-137`). Linux doesn't notice because `LocalFileSystem.create` mkdirs the parents, so it just makes nested directories.
   
   Would it make sense to fix the pattern to `[^A-Za-z-_.]`, or to sanitize the composed name instead of just `environment`? I traced this statically and haven't reproduced it, so worth a second look.
   



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5002806235

   Assigning reviewers:
   
   R: @chamikaramj added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5001596174

   Assigning reviewers:
   
   R: @chamikaramj added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5003307790

   Assigning reviewers:
   
   R: @chamikaramj added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn closed pull request #39363: Fix artifact staging filenames on Windows
URL: https://github.com/apache/beam/pull/39363


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #39363:
URL: https://github.com/apache/beam/pull/39363#issuecomment-5025311060

   Thanks for the fix. Both bot review comments sounds reasonable. Please check them.


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix artifact staging filenames on Windows [beam]

Posted by "Eliaaazzz (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org