CAMEL-24121: camel-smb - Add connectIfNecessary to existsFile and storeFileDirectly#24787
Conversation
…reFileDirectly 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.
Correct fix for a real NPE bug — existsFile() and storeFileDirectly() were the only two share-accessing methods that didn't call connectIfNecessary() first.
The bug: When tempPrefix or tempFileName is configured, GenericFileProducer.processExchange() calls existsFile() before storeFile(). Since SmbEndpoint.isSingletonProducer() returns false, every exchange gets a fresh, unconnected SmbOperations — so share is null, causing NPE.
The fix follows the established pattern exactly:
existsFile()— mirrorsexistsFolder():connectIfNecessary()+try/catch(SMBRuntimeException)withsafeDisconnect(smbre)✅storeFileDirectly()— addsconnectIfNecessary()(already has its owntry/catch(IOException)) ✅
Test is well-structured:
SmbProducerTempPrefixITfollows the same pattern as the 15+ existingSmbProducer*ITtests (extendsSmbServerTestSupport, usessendFile(), Awaitility with explicit timeout)- Verifies the exact scenario that would NPE: sending a file with
tempPrefix=.uploading
Static analysis: ast-grep ran — no new findings introduced (all findings are pre-existing in the file).
Checklist:
- ✅ Fix matches the
connectIfNecessary()+safeDisconnect()pattern used bydeleteFile(),existsFolder(),renameFile(),buildDirectory(),retrieveFile(),storeFile() - ✅ New IT covers the
tempPrefixscenario - ✅ Commit convention followed
- ✅ CI pending (builds running)
Reviewed with Claude Code (claude-code/1.0) on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
Summary
SmbOperations.existsFile()accesses thesharefield without callingconnectIfNecessary()first. WhentempPrefixortempFileNameis configured on the producer,GenericFileProducer.processExchange()callsexistsFile()before anystoreFile(), and sinceSmbEndpoint.isSingletonProducer()returnsfalse, every exchange gets a fresh, unconnectedSmbOperations— soshareis alwaysnull, causing an NPE on every message.Changes
SmbOperations.existsFile()— addedconnectIfNecessary()andsafeDisconnecterror handling, matching the pattern used bydeleteFile(),existsFolder(),renameFile(), etc.SmbOperations.storeFileDirectly()— addedconnectIfNecessary()(used for checksum file writing, had the same missing guard).SmbProducerTempPrefixIT— verifies thattempPrefix=.uploadingworks on the SMB producer.Test plan
SmbProducerTempPrefixITpasses (sends a file withtempPrefix, verifies it arrives at the expected path)Claude Code on behalf of @davsclaus
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com