PHOENIX-7900: Return HTTP 400 for missing ExpressionAttributeValues#9
Open
jinggou wants to merge 6 commits into
Open
PHOENIX-7900: Return HTTP 400 for missing ExpressionAttributeValues#9jinggou wants to merge 6 commits into
jinggou wants to merge 6 commits into
Conversation
Query API was throwing NullPointerException (HTTP 500) when required ExpressionAttributeValues placeholders were missing. Now validates that all referenced placeholders exist and throws ValidationException (HTTP 400) with clear error messages. Changes: - QueryService: Add validation before accessing ExpressionAttributeValues - Add QueryMissingExpressionAttributeValueIT: Integration tests - Add QueryServiceValidationTest: Unit tests All existing tests pass with no regressions.
palashc
requested changes
Jun 15, 2026
Contributor
There was a problem hiding this comment.
We can widen the scope of this JIRA to look at other APIs and code paths which might need this fix. Also there are test failures
Error: Failures:
Error: QueryMissingExpressionAttributeValueIT.testQueryWithMissingBetweenValue:228 Expected ValidationException in: ExpressionAttributeValues missing required placeholder ':v2' for sort key BETWEEN condition (Service: DynamoDb, Status Code: 400, Request ID: null)
Error: QueryMissingExpressionAttributeValueIT.testQueryWithMissingPartitionKeyValue:139 Expected ValidationException in: ExpressionAttributeValues missing required placeholder ':v0' for partition key (Service: DynamoDb, Status Code: 400, Request ID: null)
Error: QueryMissingExpressionAttributeValueIT.testQueryWithMissingSortKeyValue:184 Expected ValidationException in: ExpressionAttributeValues missing required placeholder ':v1' for sort key condition (Service: DynamoDb, Status Code: 400, Request ID: null)
palashc
reviewed
Jun 15, 2026
| String beginsWithPlaceholder = keyConditions.getBeginsWithSortKeyVal(); | ||
| if (!exprAttrVals.containsKey(beginsWithPlaceholder)) { | ||
| throw new ValidationException(String.format( | ||
| "ExpressionAttributeValues missing required placeholder '%s' for sort key begins_with condition", |
Contributor
There was a problem hiding this comment.
Exception message should mirror what localddb throws for this scenario
Changes based on PR review comments: - Move validation logic to ValidationUtil.requireExpressionAttributeValue() for reusability - Match AWS DynamoDB error message format exactly - Remove QueryServiceValidationTest (didn't exercise production code) - Add comprehensive test coverage: null values, malformed AttributeValues, begins_with - Fix test assertions to check awsErrorDetails().errorCode() for ValidationException All 180+ Query integration tests pass with no regressions.
Simplified the fix:
- Moved null check directly to DQLUtils.setKeyValueOnStatement() where NPE occurred
- Removed ValidationUtil.requireExpressionAttributeValue() utility method
- Reverted QueryService to simple .get() calls
- Added placeholderName parameter to show actual missing placeholder in error
- Integrated 3 tests into existing QueryIT.java (no new IT class)
- Deleted separate QueryMissingExpressionAttributeValueIT.java
Error message now includes actual placeholder name:
"Value provided in ExpressionAttributeValues unused in expressions: keys: {:status}"
All services calling setKeyValueOnStatement updated:
- QueryService: passes ExpressionAttributeValues placeholders (:v0, :v1, etc.)
- ScanService, GetItemService, BatchGetItemService: pass column names
virajjasani
reviewed
Jun 22, 2026
Comment on lines
+189
to
+194
| Map<String, Object> attrVal, String placeholderName, boolean isBeginsWith) throws SQLException { | ||
| if (attrVal == null) { | ||
| throw new ValidationException( | ||
| "Value provided in ExpressionAttributeValues unused in expressions: keys: {" | ||
| + placeholderName + "}"); | ||
| } |
Contributor
There was a problem hiding this comment.
To keep the changes simpler, let's not even use placeholderName in exception message so that method signature remains same and all service implementations don't need to change
palashc
approved these changes
Jun 23, 2026
palashc
left a comment
palashc
left a comment
Contributor
There was a problem hiding this comment.
+1, pending one nit addressed and build results
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: PHOENIX-7900
Query API was throwing NullPointerException (HTTP 500) when required ExpressionAttributeValues placeholders were missing. Now validates that all referenced placeholders exist and throws ValidationException (HTTP 400) with clear error messages.
Changes:
All existing tests pass with no regressions.