allthingssecurity opened a new pull request, #28589: URL: https://github.com/apache/camel/pull/28589
# Description [CAMEL-26081](https://issues.apache.org/jira/browse/CAMEL-26081) With `fileExist=Move`, the default move strategies of ftp/ftps (`FtpDefaultMoveExistingFileStrategy`) and of sftp/mina-sftp (`SftpDefaultMoveExistingFileStrategy`, camel-ftp-common) evaluate `moveExisting` with `${file:name}` set to the name they are given, and complete a relative result with the directory of the file (`completePartialRelativePath`). With the default `stepwise=true`, the ftp/sftp producer has changed into the target directory and passes the bare name, so `moveExisting=backup/${file:name}` for `hello.txt` in `inbox/orders` moves the old file to `inbox/orders/backup/hello.txt`. In three cases the strategy gets the path of the file from the login directory instead, and the same expression gives `inbox/orders/backup/inbox/orders/hello.txt`: - `stepwise=false` (ftp, ftps, sftp); - mina-sftp, always (it has the `stepwise` option, but its producer never changes directory: `MinaSftpOperations.doStoreFile` passes the full name); - a `tempFileName`, also with `stepwise=true`: `GenericFileProducer` moves the existing target itself, with the full target name. So the backup lands in a nested copy of the endpoint path, and anything that looks for it at `inbox/orders/backup/hello.txt` (where `stepwise=true` puts it) does not find it. That includes the "there already exists a file" check of `eagerDeleteTargetFile=false`: it looks at the nested path, so a backup at `inbox/orders/backup/hello.txt` is not seen and does not stop the move. (A second backup at the nested path is found, and the producer then fails as intended.) `${file:name.noext}` and `${file:name.noext.single}` read the same `CamelFileName` header, so `backup/${file:name.noext}.bak` nests the same way. A route moved from `sftp:` to `mina-sftp:` (which is meant to replace JSch) with the documented `${file:name}` token is affected without any change to the route. Changes: - Both strategies set `${file:name}` to the name of the file in its directory, as with `stepwise=true` without a temp file. `${file:name.noext}` and `${file:name.noext.single}` read that header and follow it. `${file:onlyname}`, `${file:parent}`, `${file:ext}` and the completion with the directory of the file are unchanged, so for a relative endpoint directory the result is now the same as with `stepwise=true` (see "Not changed" for the exceptions), and `${file:parent}/renamed-${file:onlyname}` (the form the existing tests use) works as before. A `moveExisting` without `${...}` (`moveExisting=backup`) is turned into `${file:parent}/backup/${file:onlyname}` by the endpoint and is not affected. The rename still uses the full name of the file. - I did not make the name relative to the endpoint directory: a file written to `sub/hello.txt` would then get `${file:name}` = `sub/hello.txt`, completed with the directory of the file (`.../sub`), which nests `sub` again; with `stepwise=true` it is moved to `sub/backup/hello.txt`, and the tests check that. - Upgrade guide 4.24: `camel-ftp, camel-sftp, camel-ftps, camel-mina-sftp - ${file:name} in moveExisting is the name of the file`, after the ftps entry. It names the `name.noext` tokens and says that `${file:parent}`, `${file:onlyname}`, `${file:ext}` and `moveExisting=backup` are unchanged. - No option or generated file change. - Backport: mina-sftp exists since 4.19; camel-4.22.x has the same strategies (read, not run). This changes where backups land, so I would keep it on main. Not changed (follow-ups; I can open a JIRA for each): - camel-smb has the same strategy and the same nesting: `SmbDefaultMoveExistingFileStrategy:51` sets `CamelFileName` to the full name, and `SmbOperations:475` passes the full `name`. It is the same one-line change, but the camel-smb tests are ITs that need a Samba container, which I could not run here, so I left it for its own JIRA and PR. - The file component (`GenericFileDefaultMoveExistingFileStrategy`) also passes the path of the file and nests the same way; it is not changed here (local files, other semantics of `${file:name}` there). - An absolute endpoint directory (`mina-sftp://host//data/inbox`, or sftp/ftp with `stepwise=false`): both strategies strip the leading separator of the directory of the file, so the backup goes to `data/inbox/backup/hello.txt` below the login directory, while sftp with `stepwise=true` changes into `/data/inbox` and puts it in `/data/inbox/backup/hello.txt`. This is the same on main (which nests as well), and this PR does not change it. - The completion with the directory of the file is a plain string prefix check (`completePartialRelativePath`). With `stepwise=false`, `moveExisting=inbox-old/${file:name}` for a file in `inbox` is not completed and lands in `<login>/inbox-old/` instead of `inbox/inbox-old/`. Also the same on main. Tests (ITs with the module's embedded SFTP/FTP servers, as the existing `*ProducerMoveExistingIT`; JUnit assertions as in the modules): - New `MinaSftpProducerMoveExistingFileNameIT` (camel-mina-sftp), 7 tests: `backup/${file:name}`, `old-${file:name}`, `backup/${file:name.noext}.bak`, a file in a sub directory, a `tempFileName`, the `eagerDeleteTargetFile=false` check with a backup at `backup/hello.txt`, and `${file:parent}/renamed-${file:onlyname}`. The first 6 fail on main, for example `File .../move/backup/hello.txt should exist ==> expected: <true> but was: <false>` and `Expected org.apache.camel.CamelExecutionException to be thrown, but nothing was thrown.`; the `${file:parent}` one is a control that passes on main. - New `SftpProducerMoveExistingFileNameIT` and `FtpProducerMoveExistingFileNameIT` (camel-ftp), the first 6 scenarios each with `stepwise=true` and `stepwise=false` (12 runs per class). On main 7 of 12 fail in each: all 6 with `stepwise=false`, and the `tempFileName` one with `stepwise=true`. The other 5 `stepwise=true` runs pass on main: they show the result the fix gives in the other cases. - With the fix, the unit tests and the move-existing and stepwise ITs of camel-ftp (211 run, 0 failures, 9 skipped) and camel-mina-sftp (49 run, 0 failures, 1 skipped) pass, including the existing `SftpProducerMoveExistingIT`, `FtpProducerMoveExistingIT` and `FtpProducerToDMoveExistingIT`. I ran the ITs locally through surefire; CI runs them with failsafe. The formatter and import-sort plugins ran. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it): [CAMEL-26081](https://issues.apache.org/jira/browse/CAMEL-26081). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built camel-ftp-common, camel-ftp and camel-mina-sftp and ran their tests, including the formatter and import-sort plugins. No generated file changes. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
