fix(dynamodb): enforce primary key size limits - #3354
Conversation
|
🎉 Thanks for your first pull request to Floci! Your CI checks need a maintainer to approve them before they run. That is GitHub's standard gate on first-time contributors, not a problem with your PR — so if the checks look like they are doing nothing, that is why. Once a maintainer approves, CI and the compatibility suite start automatically. Nothing is needed from you in the meantime. While you wait, a couple of things that make review faster:
Come join us in Slack — it is the fastest way to reach maintainers if you get stuck, or want feedback on an approach before investing more time in it. |
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/dynamodb/DynamoDbItemSize.java | Exposes the existing package-level attribute-size calculation for reuse by key validation. |
| src/main/java/io/github/hectorvent/floci/services/dynamodb/DynamoDbService.java | Applies byte-size limits to string and binary table partition and sort keys in the shared validation path. |
| src/test/java/io/github/hectorvent/floci/services/dynamodb/DynamoDbKeySizeIntegrationTest.java | Verifies wire-level boundary handling, AWS-shaped errors, and absence of writes after rejection. |
| src/test/java/io/github/hectorvent/floci/services/dynamodb/DynamoDbKeySizeServiceTest.java | Covers exact limits, UTF-8 and binary sizing, keyed operations, non-key values, and atomic batch and transaction rejection. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Request[Keyed DynamoDB request] --> Schema[Validate key presence and type]
Schema --> Partition[Measure partition key]
Partition -->|Over 2048 bytes| Reject[HTTP 400 ValidationException]
Partition -->|Within limit| Sort{Table has sort key?}
Sort -->|No| Execute[Execute operation]
Sort -->|Yes| MeasureSort[Measure sort key]
MeasureSort -->|Over 1024 bytes| Reject
MeasureSort -->|Within limit| Execute
Reviews (2): Last reviewed commit: "test(dynamodb): align key size test name..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ((attr.has("S") || attr.has("B")) && DynamoDbItemSize.attributeValueSize(attr) > limit) { | ||
| throw new AwsException("ValidationException", | ||
| "One or more parameter values were invalid: Size of " | ||
| + (partitionKey ? "hashkey" : "rangekey") | ||
| + " has exceeded the maximum size limit of " + limit + " bytes", 400); | ||
| } |
There was a problem hiding this comment.
The newly added if statement has an unbraced body. This violates the repository directive to always use braces in conditionals and must be corrected before merging.
| if ((attr.has("S") || attr.has("B")) && DynamoDbItemSize.attributeValueSize(attr) > limit) { | |
| throw new AwsException("ValidationException", | |
| "One or more parameter values were invalid: Size of " | |
| + (partitionKey ? "hashkey" : "rangekey") | |
| + " has exceeded the maximum size limit of " + limit + " bytes", 400); | |
| } | |
| if ((attr.has("S") || attr.has("B")) | |
| && DynamoDbItemSize.attributeValueSize(attr) > limit) { | |
| throw new AwsException("ValidationException", | |
| "One or more parameter values were invalid: Size of " | |
| + (partitionKey ? "hashkey" : "rangekey") | |
| + " has exceeded the maximum size limit of " + limit + " bytes", 400); | |
| } |
Context Used: AGENTS.md (source)
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
Checked the 2048/1024-byte limits and the UTF-8/decoded-byte encoding rules against AWS's constraints docs and they match exactly, including the subtlety of measuring strings in UTF-8 bytes rather than String.length(), which is exactly the bug class this closes. Good that Number keys are left out of the check too, they can never realistically approach these limits given DynamoDB's 38-digit precision cap, so that's not a gap.
The fix goes through the one shared key-resolution path every operation uses, and the batch/transaction tests genuinely prove atomicity rather than just asserting it, a valid sibling item in the same request never gets written when another item's key is oversized, which is the right behavior for a request-shape validation failure.
One small thing: the exact ValidationException message text isn't literally documented on AWS's own constraints page, so I couldn't verify that string against a citable reference the way I could the limits themselves. It matches what real DynamoDB is known to return, just flagging that it rests on prior knowledge rather than a doc citation.
Nice work, thanks for the thorough test coverage.
3 participants
Summary
Enforce the 2048-byte partition key and 1024-byte sort key limits in the shared table primary-key validation path. Reuse the existing attribute size calculation so strings are measured in UTF-8 bytes and binary values in decoded bytes.
Fixes #3323. Requests that previously stored an oversized table primary key now fail with HTTP 400 ValidationException. Tests cover exact boundaries, one byte over, multibyte strings, binary values, key arguments, non-key values, and batch/transaction rejection without partial writes.
Type of change
fix:)AWS Compatibility
The limits and encoding rules are documented in DynamoDB constraints. This change covers table primary keys; it does not add validation for secondary-index key sizes or query expressions. No live AWS service was used during validation.
Validation
The new service regression ran 10 cases before the fix: 9 failed because oversized keys were accepted. After the fix, all 186 cases in DynamoDbKeySizeServiceTest and DynamoDbServiceTest passed on JDK 25:
./mvnw -B -ntp test -Dtest=DynamoDbKeySizeServiceTest,DynamoDbServiceTestAll 81 HTTP integration cases passed, including four new boundary/rejection cases and 77 existing DynamoDB integration tests:
./mvnw -B -ntp test -Dtest=DynamoDbKeySizeIntegrationTest,DynamoDbIntegrationTestThe new wire-level tests verify HTTP status, error type/message and Scan count after the rejected write.
Checklist
./mvnw testpasses locally (not run)