Skip to content

Port operations - #7250

Open
RanVaknin wants to merge 5 commits into
feature/master/DDB-mapperv2from
rvaknin/port-operations
Open

Port operations#7250
RanVaknin wants to merge 5 commits into
feature/master/DDB-mapperv2from
rvaknin/port-operations

Conversation

@RanVaknin

@RanVaknin RanVaknin commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR ports the DynamoDBMapper operations (load, save, query, scan, batchLoad, batchWrite, the transaction methods, and count) 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, *Response types, .x() accessors, the software.amazon.awssdk namespace, v2 enums, and hasX() guards). The mapper stays synchronous and wraps DynamoDbClient.

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 returned null for 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 the count pagination 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:

  • batchLoad accumulation. v1 built the per-table KeysAndAttributes up 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 immutable KeysAndAttributes at flush time via a new helper. Worth confirming the 100-key chunk boundary still flushes and clears the same way and that consistentReads is applied to every table.
  • Batch-get retry. v1's retry loop reused the same request object, calling 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.
  • Lazy pagination. The paginated lists and the parallel-scan helper used to advance a page by calling setExclusiveStartKey(...) on a shared request; v2 reassigns the field from a builder each page, which also means the request field is no longer final. fetchNextPage() stays synchronized, 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:

  • ShapeRequestTest drives a real DynamoDbClient, captures each mapper call's marshalled request through an afterMarshalling interceptor that aborts before transmission, and asserts the target and JSON body byte for byte against a fixture in src/test/resources/, roughly 60 cases covering the full serialization matrix, every SaveBehavior variant, version and condition expressions, projections, and transactions.
  • ShapeResponseTest feeds predefined attribute maps straight into marshallIntoObject, 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. ShapeResponseBehaviorTest fills that gap with mock tests that stub DynamoDbClient responses 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

  • Table administration (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.
  • S3Link: out of scope; its tests are excluded too.
  • The 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.

DDB mapper v2 roadmap

** BASE PACKAGE SETUP **
+ 1. Source verbatim port ✅
+ 2. Test verbatim port ✅
+ 3. Namespace swap (main + datamodeling tests) ✅ 
+ 4. Namespace swap (remaining test packages) 

** PORTING OPERATIONS **
+ 0. converters (AttributeValue seam) ✅ 
1. load() <---- current PR
2. save() <---- current PR
3. query() + scan() <---- current PR
4. deleteItem() <---- current PR
5. updateItem() <---- current PR
6. Batch operations <---- current PR
7. Transactions <---- current PR
8. S3Link
9. Table Admin (control plane convenience methods)

** PERFORMANCE IMPROVEMENTS **
1. getTableModel caching
2. Wire "fast" createX AV factory methods to convertors
3. ByteBuffer → SdkBytes copy
4. Others

** DEPENDENCY MODERNIZATION **
1. EasyMock -> Mockito <---- current PR
2. Log4j 1.x -> 2.x
3. commons-logging -> SLF4J

segmentScanRequest = segmentScanRequest.toBuilder().exclusiveStartKey(lastScanResult.lastEvaluatedKey()).build();
} else {
segmentScanRequest.setExclusiveStartKey(null);
segmentScanRequest = segmentScanRequest.toBuilder().exclusiveStartKey(null).build();

@RanVaknin RanVaknin Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RanVaknin RanVaknin changed the title Rvaknin/port operations Port operations Aug 11, 2026
pause(batchLoadStrategy.getDelayBeforeNextRetry(batchLoadContext));
batchGetItemRequest.setRequestItems(
batchGetItemResult.getUnprocessedKeys());
batchGetItemRequest = batchGetItemRequest.toBuilder()

@RanVaknin RanVaknin Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed because it was already non functional in v1. No coverage is lost

@RanVaknin
RanVaknin marked this pull request as ready for review August 13, 2026 20:27
@RanVaknin
RanVaknin requested a review from a team as a code owner August 13, 2026 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant