CAMEL-24159: camel-azure-storage-queue - Fix medium-severity bugs from code review#24870
Conversation
…m code review 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.
Claude Code on behalf of gnodet
The credential override guard is correct (same pattern as the other azure PRs). The createIfNotExists fix addresses a real usability issue.
Credential override guard (QueueComponent.java)
Consistent with servicebus/eventhubs/blob PRs. if (credentialType == null) prevents overriding explicit user configuration. ✅
Idempotent queue creation (QueueOperations + QueueClientWrapper)
The switch from create() (throws 409 on existing queue) to createIfNotExists() is the right fix for the createQueue=true use case — producers shouldn't fail on repeated sends.
However, the wrapper method silently drops its parameters:
public void createIfNotExists(Map<String, String> metadata, Duration timeout) {
client.createIfNotExists(); // ← metadata and timeout are ignored!
}The calling code in QueueOperations extracts metadata and timeout from the exchange and passes them in, but they're never forwarded. This is a regression compared to the old create(metadata, timeout) call — users who set queue metadata via exchange headers would silently lose that metadata.
The Azure SDK's QueueClient has createIfNotExistsWithResponse(Map<String, String> metadata, Duration timeout, Context context) — the implementation should be:
public void createIfNotExists(Map<String, String> metadata, Duration timeout) {
client.createIfNotExistsWithResponse(metadata, timeout, Context.NONE);
}This should be a small fix. Everything else LGTM.
|
🧪 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).
|
…eateIfNotExists Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
Claude Code on behalf of davsclaus @gnodet Good catch — fixed in |
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of gnodet
Re-review — commit 1aacea0
The `createIfNotExists` parameter-dropping bug I flagged is now fixed:
// Before: ignored metadata and timeout
client.createIfNotExists();
// After: forwards both parameters
client.createIfNotExistsWithResponse(metadata, timeout, Context.NONE);This was the last outstanding item. LGTM.
Summary
Claude Code on behalf of davsclaus
Fixes 2 medium-severity findings in
camel-azure-storage-queuefrom CAMEL-24159 code review:QueueComponent): wrap the else branch withif (credentialType == null)so user-explicit credential type is not overridden when aStorageSharedKeyCredentialbean is found in the registry.QueueOperations+QueueClientWrapper): whencreateQueue=trueonsendMessage, usecreateIfNotExistsWithResponse()instead ofcreate()so repeated sends don't throw HTTP 409 Conflict when the queue already exists with different metadata.Review feedback addressed
createIfNotExistswrapper to forwardmetadataandtimeoutviacreateIfNotExistsWithResponse(metadata, timeout, Context.NONE)instead of the no-argcreateIfNotExists()that silently dropped both parameters.Test plan
cd components/camel-azure/camel-azure-storage-queue && mvn clean installcredentialType🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com