Croway commented on PR #29250:
URL: https://github.com/apache/camel/pull/29250#issuecomment-6659494409
Thanks for looking into this. I checked the PR on macOS, against the PR head
and against `main` with the fix reverted. I don't think this change fixes
CAMEL-26639 on Windows.
**On Windows the change has no effect**
- `java.io.File` on Windows already turns `/` into `\`
(`WinNTFileSystem.normalize`). So `new File("C:/Users/x/target")` and `new
File("C:\\Users\\x\\target")` are the same path, and `remaining` isn't used
after that line.
- `GenericFileConfiguration.setDirectory` then calls
`FileUtil.normalizePath`, which on Windows also turns `/` into `\`. The `File`
and the configured directory are the same before and after the PR.
- `remaining` is already decoded by the time it reaches `buildFileEndpoint`,
because `URISupport.extractRemainderPath` uses `getSchemeSpecificPart()`. It
holds `C:\Users\...`, not `%5C`.
- The `%5C` shown in the ticket is in the endpoint URI, and the PR doesn't
change that URI. With the fix, `endpoint.getEndpointUri()` is still
`file://C:%5CUsers%5Ctest%5Ctarget?fileName=test.txt`.
**The tests don't cover the bug**
- They pass on `main` with the `FileComponent` change reverted (3/3 on
macOS). On non-Windows, `setDirectory` already turns `\` into `/`.
- On Windows I expect all three to fail, because
`getConfiguration().getDirectory()` is always `\`-separated there. That
includes `shouldPreserveForwardSlashPaths`, where `/home/user/target` becomes
`C:\home\user\target`. I couldn't run them on a Windows JVM. This comes from
the code above, plus forcing `FileUtil` into Windows mode with `-Dos.name`,
where `setDirectory("C:/Users/test/target")` returns `C:\Users\test\target`. CI
won't show the failures, because the nightly Windows job builds with
`-Dquickly`.
- They call the protected `buildFileEndpoint` directly with an
already-decoded path. So the URI encoding and the consumer poll, the parts the
ticket says fail, never run.
**What the change does do: it fixes a real inconsistency on Linux/macOS**
A backslash is a legal filename character there. On `main`, `getFile()`
keeps the backslash but the configured directory doesn't:
| `file:<tmp>/out\sub` and `file:<tmp>/in\sub` | `main` | this PR |
|---|---|---|
| `getFile()` | `.../out\sub` | `.../out/sub` |
| `getConfiguration().getDirectory()` | `.../out/sub` | `.../out/sub` |
| producer | writes `out/sub/a.txt` and also creates an empty `out\sub`
directory | writes `out/sub/a.txt` only |
| consumer (files in both `in\sub` and `in/sub`) | polls nothing | consumes
`in/sub/nested.txt` |
Nothing that works today breaks. But this isn't the problem the ticket
describes.
**Suggestions**
1. Reproduce the Windows symptom first, with a route-based test:
`@EnabledOnOs(OS.WINDOWS)`, drop a file, assert with a `MockEndpoint`. That
shows where it really goes wrong. The ticket's workaround list includes
`FileWatchITCase`, and `file-watch:` doesn't use `FileComponent` at all, which
suggests the cause is somewhere else.
2. If you'd rather keep this as a Linux/macOS consistency fix:
- Use `FileUtil.normalizePath(remaining)` instead of the hand-written
`replace`, so `getFile()` and the directory follow the same per-OS rule. Or
normalize in `FileEndpoint.setFile`, which is public and also sets the
directory.
- Retitle the PR and commit to match.
- Drop the "URI compatibility" comment.
- In the test, check `getFile().getPath()` with `assertEquals` against
the expected path.
_Claude Code on behalf of Croway_
--
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]
