CAMEL-24088: camel-ftp - streamDownload treats refused RETR as success#24807
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
LGTM — important fix for silent data loss in FTP streamDownload mode.
retrieveFileToStreamInBody: FTPClient.retrieveFileStream() returns null on server refusal
(550/450/425), but the old code unconditionally set result = true (line 487 on main). This
caused the consumer to route an exchange with a null body, and the process strategy would then
commit — moving or deleting the remote file that was never actually downloaded. The fix
(result = is != null + guarded body/header) is clean and correct.
releaseRetrievedFileResources: The discarded completePendingCommand() return value meant a
truncated transfer (426/451) was silently accepted. The new check-and-throw is correct —
GenericFileOperationFailedException extends RuntimeCamelException (a RuntimeException),
so it won't be caught by the existing IOException catch clause in the same try block.
Side benefit: ignoreFileNotFoundOrPermissionError was effectively dead code for streamDownload
mode since retrieved was always true — this fix re-enables it.
Tests cover all five paths well (null stream, success stream, failed completion, success completion,
no-stream release).
Claude Code on behalf of gnodet
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 12 tested, 29 compile-only — current: 12 all testedMaveniverse Scalpel detected 41 affected modules (current approach: 12).
|
Summary
Claude Code on behalf of davsclaus
Fixes two related bugs in
FtpOperationswhere thestreamDownload=truepath never checks the FTP client's success indicators, causing silent data loss:retrieveFileToStreamInBody:FTPClient.retrieveFileStream()returnsnullwhen the server refuses RETR (550/450/425), but the code unconditionally setresult = true. The consumer then routes the exchange with a null body and the process strategy commits — moving or deleting the remote file that was never downloaded. Fixed by settingresult = is != nulland only populating the body/header when the stream is available.releaseRetrievedFileResources:FTPClient.completePendingCommand()returnsfalsewhen the transfer did not complete (server sent 426/451), but the return value was discarded. A truncated stream would be silently accepted and the commit would still run. Fixed by checking the return value and throwingGenericFileOperationFailedExceptionon failure.These fixes also re-enable
ignoreFileNotFoundOrPermissionErrorfor streamDownload mode — it was previously dead code becauseretrievedwas alwaystrue.Test plan
FtpOperationsStreamDownloadTestwith 5 test cases:testRetrieveFileStreamReturnsNullOnServerError— verifiesretrieveFilereturnsfalsewhen server refuses RETRtestRetrieveFileStreamSuccessReturnsTrue— verifies normal success path still workstestReleaseResourcesThrowsOnFailedCompletion— verifies exception on failedcompletePendingCommandtestReleaseResourcesSucceedsOnCompletedTransfer— verifies normal release pathtestReleaseResourcesSkipsWhenNoStream— verifies no-op when no stream header present🤖 Generated with Claude Code