Port operations - #7250
Conversation
| segmentScanRequest = segmentScanRequest.toBuilder().exclusiveStartKey(lastScanResult.lastEvaluatedKey()).build(); | ||
| } else { | ||
| segmentScanRequest.setExclusiveStartKey(null); | ||
| segmentScanRequest = segmentScanRequest.toBuilder().exclusiveStartKey(null).build(); |
There was a problem hiding this comment.
unavoidable performance hit. v1's ScanResult was mutable and exposed direct setter. v2's ScanResponse is immutable and we are required to use .toBuilder() to reconstruct and mutate it.
| pause(batchLoadStrategy.getDelayBeforeNextRetry(batchLoadContext)); | ||
| batchGetItemRequest.setRequestItems( | ||
| batchGetItemResult.getUnprocessedKeys()); | ||
| batchGetItemRequest = batchGetItemRequest.toBuilder() |
There was a problem hiding this comment.
minor performance hit. BatchGetItemRequest gets reconstructed at every iteration because its immutable, and requires a .toBuilder()...build().
This codepath only fires when a batch request returns a response with unprocessedKeys: [a,b,c] for the mapper to retry. So it's at least not the hot path of every batch request.
| return resultSet; | ||
| } | ||
|
|
||
| private static Map<String, KeysAndAttributes> buildKeysAndAttributes( |
There was a problem hiding this comment.
v1 grew each table's key list in place via requestItems.get(tableName).getKeys().add(x) because v1's KeysAndAttributes was mutable.
In v2 it's immutable, so the loop accumulates raw keys in a plain Map<String, List<Map<String, AttributeValue>>> and this helper builds each table's KeysAndAttributes once, at each processBatchGetRequest call (the 100 key boundary and the final partial batch)
|
|
||
| AttributeValueUpdate update = updateValues.get(entry.getKey()); | ||
| if (update != null) { | ||
| update.getValue() |
There was a problem hiding this comment.
AttributeValueUpdate is immutable in v2, so instead of mutating the existing update's value in place we rebuild it with the transformed value via toBuilder().value(...).build()
| public void disabled() { | ||
| } | ||
|
|
||
| // This record written by the .NET mapper no longer exists, so this test |
There was a problem hiding this comment.
This is removed because it was already non functional in v1. No coverage is lost
Summary
This PR ports the
DynamoDBMapperoperations (load,save,query,scan,batchLoad,batchWrite, the transaction methods, andcount) to v2, and lands the test suite that proves the port. The goal was a faithful port with the same behavior, same public surface, with changes limited to what v2 forces (immutable builders,*Responsetypes,.x()accessors, thesoftware.amazon.awssdknamespace, v2 enums, andhasX()guards). The mapper stays synchronous and wrapsDynamoDbClient.It ships as one PR because the operations all funnel through the same shared scaffolding where nearly all the mechanical v2 changes land, so slicing it up would mean stubbing operations that then break the shared path tests, paying a stub and refix tax on every PR.
Before touching the mapper I wrote fixture tests capturing both the request wire format and how the mapper reconstructs objects from a response, setting the source of truth for the migration. These ran green against real v1, so I then converted the mapper and re ran the same fixtures unchanged against v2, which is the backwards compatibility proof. Most of the diff is mechanical idiom conversion.
Source changes that need review (non mechanical)
Almost all the real risk is in
DynamoDBMapper.java, and almost all of it traces to one v2 behavior: where v1 returnednullfor a miss, v2 returns an empty but non-null map. A few guards had to change from a null check to a null or empty check to preserve behavior:load(not-found), the update-to-put fallback, and thecountpagination loop. Tiny edits, but the ones that change runtime behavior if they're wrong.Everywhere else, the risk comes from v1 mutating request objects in place, which v2's immutable requests forbid. Three spots had to be restructured, each in a slightly different way:
batchLoadaccumulation. v1 built the per-tableKeysAndAttributesup front and mutated each entry's key list in place as it walked the items. v2 can't do that, so it accumulates plain keys in a side map and materializes the immutableKeysAndAttributesat flush time via a new helper. Worth confirming the 100-key chunk boundary still flushes and clears the same way and thatconsistentReadsis applied to every table.setRequestItems(unprocessedKeys)on it. v2 rebuilds the request per retry and writes it back into the batch-load context so the retry strategy reads the new one, not the stale copy.setExclusiveStartKey(...)on a shared request; v2 reassigns the field from a builder each page, which also means the request field is no longerfinal.fetchNextPage()stayssynchronized, so what to verify is that the reassignment happens inside it and the loop reads the updated field.Testing
The heart of it is the shape suite under
src/test/.../shape/, split across the two paths:ShapeRequestTestdrives a realDynamoDbClient, captures each mapper call's marshalled request through anafterMarshallinginterceptor that aborts before transmission, and asserts the target and JSON body byte for byte against a fixture insrc/test/resources/, roughly 60 cases covering the full serialization matrix, every SaveBehavior variant, version and condition expressions, projections, and transactions.ShapeResponseTestfeeds predefined attribute maps straight intomarshallIntoObject, serializes the reconstructed POJO, and asserts that deserialization against a fixture.The shape suite can't reach the mapper's response handling control flow, which is exactly where the "null or empty" guards live.
ShapeResponseBehaviorTestfills that gap with mock tests that stubDynamoDbClientresponses and pin the branch behavior directly. These paths had no other running coverage.The rest is migration mechanics. The mocks moved from EasyMock to Mockito with the same assertions.
Deleted tests. Five files were removed with no loss of running mapper coverage. Four never exercised the mapper at all. The fifth used the mapper but was already dead on the base branch.
Deferred work / follow-ups
createTable/deleteTable/generate*Request): stubbed or still emitting v1 types. The v1-coupled tests that depend on it are excluded from the build and come back with that port.pom.xml<testExcludes>block lists every excluded file. Each was confirmed blocked on one of the deferred surfaces above, not hiding a regression in an already-ported path.