CAMEL-23229 - Camel-AWS2-DDB: Add PartiQL, transactional, and batch w…#22188
CAMEL-23229 - Camel-AWS2-DDB: Add PartiQL, transactional, and batch w…#22188
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Review — Claude Code on behalf of Guillaume Nodet
Good PR — well-structured, follows existing patterns, has solid test coverage (unit + integration), and proper documentation. A few items worth addressing before merge:
1. Naming inconsistency: BatchWriteItem vs BatchGetItems (Medium)
The existing enum value is BatchGetItems (plural), but the new one is BatchWriteItem (singular). The DynamoDB SDK uses singular for both (batchWriteItem, batchGetItem), so the new name is arguably more accurate — but it's inconsistent within Camel's own enum. Worth deciding on a convention before shipping this public API.
Ddb2Operations.java
2. @Metadata labels include BatchExecuteStatement incorrectly (Low)
In Ddb2Constants.java, the STATEMENT and STATEMENT_PARAMETERS constants have label "ExecuteStatement BatchExecuteStatement", but BatchExecuteStatementCommand does not use either of these headers — it uses BATCH_STATEMENTS (which contains BatchStatementRequest objects with their own parameters). The labels should be just "ExecuteStatement".
3. Input/output token asymmetry in ExecuteStatementCommand (Medium)
The command reads the input pagination token from Ddb2Constants.NEXT_TOKEN ("CamelAwsDdbNextToken") but writes the output next token to Ddb2Constants.EXECUTE_STATEMENT_NEXT_TOKEN ("CamelAwsDdbExecuteStatementNextToken"). This means users doing pagination need to manually copy the output header to the input header between iterations. Consider using the same header for both, or at minimum documenting the asymmetry clearly.
4. TransactWriteItemsCommand discards response entirely (Low)
The execute() method calls transactWriteItems() but discards the response without setting any output headers. While the response is sparse (no items), it does contain consumedCapacity() and itemCollectionMetrics() which could be useful. Not a blocker, but worth considering.
5. No tests for missing required headers (Low)
There are no tests covering the case where required headers (e.g., STATEMENT for ExecuteStatement) are missing. Currently this would pass null to the SDK, producing unhelpful NullPointerExceptions. This is consistent with existing commands (pre-existing pattern), so not a blocker for this PR.
What looks good
- Clean separation of command classes following the established pattern
- Thorough documentation with examples for all 5 operations
- PartiQL examples correctly show parameterized queries (mitigating injection risk)
- Integration tests against LocalStack, plus the comprehensive
AWS2NewOperationsRuleIT - Generated catalog/DSL files properly updated
…rite operations Add 5 new operations to the AWS DynamoDB component: - ExecuteStatement: PartiQL single statement with pagination support - BatchExecuteStatement: PartiQL batch statement execution - TransactWriteItems: ACID transactional writes with idempotency token - TransactGetItems: ACID transactional reads - BatchWriteItem: Batch put/delete across tables Added 14 new header constants, 5 command classes, 5 unit test classes (7 tests), 5 LocalStack integration tests, and documentation with examples for all new operations. Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…rite operations Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
@gnodet this is sorted out |
|
🧪 CI tested the following changed modules:
All tested modules (72 modules)
|
3 participants
…rite operations
Add 5 new operations to the AWS DynamoDB component:
Added 14 new header constants, 5 command classes, 5 unit test classes (7 tests), 5 LocalStack integration tests, and documentation with examples for all new operations.
Description
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.