fix: DynamoDB expression evaluation, UpdateExpression paths, and ConsumedCapacity#197
Conversation
|
Hi @diopolgg, Thanks for taking the time to contribute! We've seen your PR and will take a look soon. To ensure this implementation stays robust, could you also open a PR in our compatibility suite at https://github.com/hectorvent/floci-compatibility-tests? This helps us validate that your changes keep working perfectly as the project evolves. Also check conflicts with main branch, The Floci team |
|
Hi @diopolgg, tests project embedded directly in this project please sync with main, solve conflicts and get the test implemented. Test directory is Thanks |
hectorvent
left a comment
There was a problem hiding this comment.
Conflicts changes required
6b615d5 to
d019253
Compare
|
Resolved merge conflicts with latest main. All unit tests (1725) pass, and Java compatibility tests (DynamoDB expression, scan condition, and all other services) pass against a live Floci instance. Ready for re-review. |
|
Hello @diopolgg, Thanks for the PR! Could you please update the commit history to remove the Co-authored-by: Claude <...> line from the commit message? We prefer to keep the attribution focused on human contributors for this project. Once that's cleaned up, I'm happy to take another look! |
3dbe55a to
22003c1
Compare
|
@hectorvent Rebased again, removed claude code attributions.. seems conflicts appear pretty quickly here :). Sometimes quicker then we can make yet another review roundtrip. Thus hard to squash/merge. Hope it will be merged eventually though. We have large codebase with integration tests for DDB. Most of the fixes i did were just to make |
The old regex/string-splitting approach for filter, condition, and key condition expressions broke on several valid DynamoDB patterns: - <> comparisons on BOOL attributes (missing attribute handling) - IN operator (fell through to pass-all) - OR / NOT logical operators (only AND was implemented) - Parenthesized BETWEEN: pk = :pk AND (sk BETWEEN :start AND :end) - Compact format: (#f0 = :v0)AND(#f1 BETWEEN :v1 AND :v2) Adds ExpressionEvaluator with a proper tokenizer → recursive-descent parser → AST evaluator, and integrates it into DynamoDbService replacing matchesFilterExpression, evaluateCondition, and queryWithExpression's key-condition splitting and SK evaluation. Also fixes GSI sparse-index semantics: items where any GSI key attribute is null/missing are now excluded from GSI query results, matching real DynamoDB behavior. Bonus: correctly handles numeric ExpressionAttributeNames (#0, floci-io#1) that the old tokenizer missed (relates to floci-io#127).
…se boundary parsing Two bugs in applyUpdateExpression: 1. Dotted paths (e.g. `#details.subkey = :val`) were not resolved segment-by-segment — the entire `#details.subkey` was looked up as a single ExpressionAttributeName, failing silently. Now each segment is resolved independently and nested DynamoDB Map nodes are navigated/created as needed. 2. SET clause parsing used findNextComma before findNextClauseKeyword, so when the last SET value was followed by `REMOVE a,b,c`, the comma in the REMOVE list was treated as a SET assignment separator. This caused the last SET assignment to absorb part of the REMOVE list and silently fail. Now clause keywords are checked before commas.
…n DescribeTable - Return ConsumedCapacity in GetItem, PutItem, DeleteItem, UpdateItem, Query, Scan, BatchGetItem, BatchWriteItem responses when ReturnConsumedCapacity is TOTAL or INDEXES. - For INDEXES level, include per-table and per-GSI capacity breakdown. - Add ProvisionedThroughput to GSI descriptions in DescribeTable responses (was missing, causing clients that read GSI throughput to fail). - Simple capacity estimates: 0.5 RCU per item read, 1.0 WCU per write.
…licts - Add DynamoDbExpressionTests covering filter expressions (BOOL, IN, OR, NOT, nested parens), dotted UpdateExpression paths, ConsumedCapacity, and parenthesized BETWEEN in KeyConditionExpression - Add DynamoDbFilterExpressionIntegrationTest for HTTP-level validation - Fix stale trimBalancedOuterParens references left after rebase conflict resolution (method was replaced by ExpressionEvaluator AST parser) - Fix surefire include pattern to also run *Tests.java files that were silently skipped (DynamoDbScanConditionTests, EcsTests, Ec2Tests, etc.)
22003c1 to
0253336
Compare
|
Hi @diopolgg, Thank you for your patient and for keeping this PR aligned with latest changes on main. I see some tests errors, but they are not related to your PR. I will merged it and resolve those issues in another PR. Thank you again! |
Summary
This PR fixes several DynamoDB compatibility issues and adds ConsumedCapacity support:
1. Replace regex-based expression evaluation with AST parser (
fix)The old regex/string-splitting approach for filter, condition, and key condition expressions broke on several valid DynamoDB patterns:
<>comparisons on BOOL attributes (missing attribute handling)INoperator (fell through to pass-all)OR/NOTlogical operators (onlyANDwas implemented)pk = :pk AND (sk BETWEEN :start AND :end)(#f0 = :v0)AND(#f1 BETWEEN :v1 AND :v2)#0,#1) — relates to [BUG] DynamoDB: numeric ExpressionAttributeNames (#0, #1) not resolved — affects Query KeyConditionExpression and TransactWriteItems ConditionExpression #127Adds
ExpressionEvaluatorwith a proper tokenizer → recursive-descent parser → AST evaluator. Also fixes GSI sparse-index semantics: items where any GSI key attribute is null/missing are now excluded from GSI query results, matching real DynamoDB behavior.2. Support dotted paths in UpdateExpression and fix clause boundary parsing (
fix)Two bugs in
applyUpdateExpression:Dotted paths (e.g.
#details.subkey = :val) were not resolved segment-by-segment — the entire string was looked up as a single ExpressionAttributeName, failing silently. Now each segment is resolved independently and nested Map nodes are navigated/created as needed.SET/REMOVE clause boundary —
findNextCommawas checked beforefindNextClauseKeyword, soSET #a = :v1 REMOVE #b,#cwould treat the comma in the REMOVE list as a SET separator, corrupting both clauses.3. Add ConsumedCapacity in DynamoDB responses (
feat)ConsumedCapacityin GetItem, PutItem, DeleteItem, UpdateItem, Query, Scan, BatchGetItem, BatchWriteItem whenReturnConsumedCapacityisTOTALorINDEXES.INDEXESlevel, includes per-table and per-GSI capacity breakdown.ProvisionedThroughputto GSI descriptions in DescribeTable (was missing, causing clients that read GSI throughput to fail).Tests
Unit & integration tests (main project)
<>/=, IN operator, OR/NOT operators, nested parentheses, parenthesized BETWEEN, and compact-format key conditionsAll 1468 tests pass.
Compatibility tests (SDK against live Floci)
**/*Tests.javapattern tocompatibility-tests/sdk-test-java/pom.xml— several existing test classes (DynamoDbScanConditionTests,EcsTests,Ec2Tests, etc.) were silently skipped because surefire only matched**/*Test.javaAll 38 DynamoDB compatibility tests pass (18 existing + 6 previously skipped + 14 new).