docstore: add DynamoDB item size validation with 400 KB limit

[Yaminik1996]

Summary

< Provide a brief description of the changes in this PR >

Some conventions to follow

1. add the module name as a prefix
   • for example: add a prefix: docstore: for document store module, blobstore for Blob Store module
2. for a test only PR, add test:
3. for a perf improvement only PR, add perf:
4. for a refactoring only PR, add "refactor:"

[iamabhilaksh]

Solid direction — centralizing the size check in newPut() covers PutItem and TransactWriteItems in one place, which is the right seam. Two things holding me back from approving, both cheap to fix.

CI is red: the Maven build job is failing on AwsDocstoreIT (7 failures). The three new tests — testCreateWithinSizeLimit, testCreateOversizedDocument, testPutOversizedDocument — have no WireMock fixtures committed under docstore-aws/src/test/resources/mappings/, so their requests fall through to live DynamoDB with placeholder creds ("security token is invalid"). Re-running with -Drecord against a real/LocalStack table and committing the generated mappings should clear it.

Separately, the byte-size formula undercounts vs. AWS's documented algorithm in two spots (inline). Not fatal — DynamoDB still rejects oversized writes server-side — but it undercuts the fail-fast client-side guarantee for number-heavy or nested payloads, and both are one-line fixes.

One coverage note while you're in here: the unit tests exercise the calculator directly, but no integration test pushes a nested map/list document through newPut() near the 400KB boundary — the case most exposed to the List/Map overhead gap below. One boundary IT with a nested payload would cover it.

[iamabhilaksh]

AWS's documented number size is (1 byte per 2 significant digits) + (1 byte) — there's a flat +1 this is missing, so every number undercounts by 1 byte (a 1-digit number should be 2 bytes, not 1). It compounds across Number Sets and lists of numbers. Suggest size + 1 here, and bump the floor/cap to Math.max(2, Math.min(39, ...)) to match.

[iamabhilaksh]

Same missing +1 in the fallback path — worth fixing in lockstep with L157 so both branches agree with the AWS formula.

[iamabhilaksh]

AWS charges List/Map a flat 3 bytes of container overhead plus 1 byte per element, on top of the element sizes — this sums elements only. It compounds with nesting depth, exactly the shape a document store hits. Suggest adding 3 + value.l().size() here (and 3 + value.m().size() in the Map branch at L93). Sets are correctly left overhead-free, so only these two need it.

[iamabhilaksh]

LGTM 👍 — all three earlier points check out: CI green, the number +1 and List/Map 3 + size() overhead are in on all four spots (verified against the test expectations), and the nested-boundary case is covered.

One non-blocking follow-up: testItemSizeValidation_nestedPayloadNearLimit is a mocked-client unit test rather than a true IT — fine by me since validation is fully client-side, just flagging in case you wanted a wire round-trip.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 63.01370% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.37%. Comparing base (b06ec13) to head (9ba2e05).

Files with missing lines	Patch %	Lines
...loudj/docstore/aws/DynamoDbItemSizeCalculator.java	59.09%	18 Missing and 9 partials ⚠️

❌ Your patch status has failed because the patch coverage (63.01%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #555      +/-   ##
============================================
- Coverage     83.47%   83.37%   -0.10%     
  Complexity      674      674              
============================================
  Files           215      216       +1     
  Lines         14957    15030      +73     
  Branches       2064     2081      +17     
============================================
+ Hits          12485    12531      +46     
- Misses         1648     1666      +18     
- Partials        824      833       +9     

Flag	Coverage Δ
unittests	83.37% <63.01%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sandeepvinayak]

what's the motivation of this change - it seems like an overkill/overengineeing? does any of the provider sdk do these prechecks at the client side? if there is an exception from remote - we anyway can throw the relevant exception in rare cases.

[Yaminik1996]

It was logged by one of the customers here in W-19998633.

Description as below:

DDB has a 400 KB limit for each row, but it is better for the customer fo find out earlier instead of when the program fails to write. It would be desirable to have a configurable option with a threshold value, an exception can be thrown if the threshold is exceeded.

[sandeepvinayak]

Even if it's desirable to have this feature from one customer’s experience but it comes with the maintenance cost and an overhead, the limits keep evolving. IMO adding size calculation before every write introduces measurable performance overhead and maintenance complexity that isn't provided by the native SDKs. it doesn't justify the addition I think.

[sandeepvinayak]

Also, given that this ask is from a long time ago it tells that it’s not a priority ask from them

[sandeepvinayak]

please see the comment

[LihaoLiuXs]

This test never calls the production code — it asserts a test-local copy of the algorithm against itself.

Every assertion here invokes calculateExpectedNumberSize (L143–149), a private helper defined in this test class, not DynamoDbItemSizeCalculator. That helper is a line-for-line duplicate of the production calculateNumberSize: same stripTrailingZeros().toPlainString(), same strip of - and ., same Math.max(2, Math.min(39, (n + 1) / 2 + 1)). So the test passes for any behavior of the production method — change the production formula and this test still goes green, which is the opposite of what the name formulaCoverage promises.

The same helper is used to derive the expected value in testCalculateItemSize_multipleAttributes (L31), so that test cannot catch a wrong number formula either. Codecov reports 59% patch coverage on the calculator with 18 lines missed, consistent with the number path being effectively untested.

Suggested fix: delete the helper and assert hard-coded byte counts directly against the production entry point, e.g. assertEquals(2, DynamoDbItemSizeCalculator.calculateItemSize(Map.of("k", AttributeValue.builder().n("5").build())) - 1), so expectations are independent of the implementation.

[LihaoLiuXs]

This counts positional digits, not significant digits, over-charging magnitude-heavy numbers by up to 37 bytes each — and since the caller hard-fails, over-counting rejects writes DynamoDB would accept.

toPlainString() expands the exponent before the digits are counted. 1.0E10 becomes "10000000000" → 11 digits → 7 bytes; 1.0E100 and 1.0E308 become 101 and 309 digits → the 39-byte cap. DynamoDB trims leading and trailing zeros, so all three are 1 significant digit → 2 bytes. Even 1000 is charged 3 instead of 2.

This is reachable from ordinary data, not just hand-built AttributeValues: AwsEncoder.encodeFloat emits Double.toString(value), so any double field holding a large- or small-magnitude value hits it. About 276 such attributes fabricate 10 KB of phantom size — enough for validateItemSize to reject an item that is comfortably under 400 KB, with no way for the caller to opt out. Plain decimals such as 1234.5 are unaffected, so this will not show up in the current tests.

Suggested fix: count significant digits from the unscaled value instead of the expanded string:

BigDecimal n = new BigDecimal(numberString).stripTrailingZeros();
int significantDigits = n.unscaledValue().abs().toString().length();

[LihaoLiuXs]

Throwing from newPut aborts an entire non-atomic ActionList before any write is submitted, turning one oversized document into zero writes performed.

runWrites (L334–338) builds every WriteOperation with .map(newWriteOperation).collect(...) before submitting any of them, so this exception escapes during the build phase and none of the sibling putItem calls are ever issued. ActionList.enableAtomicWrites() is documented as the mode where writes "all fail or succeed" — that distinction only means anything if the default list does not behave that way, and today it doesn't: DynamoDB rejects the oversized item on its own PutItem while the other writes in the list still succeed. After this change, one oversized document in a 25-write list drops all 25.

It also makes the failure mode estimator-dependent: the same document yields "whole batch aborted" when the client estimate crosses 409600 and "one item rejected, rest succeed" when it doesn't.

Suggested fix: validate all items up front in runWrites/runTxWrites and name the offending action, or keep the check inside runPut so it fails only that operation.