Migrate AttributeValue - #7199
Open
RanVaknin wants to merge 6 commits into
Open
Conversation
… v2 converter port
RanVaknin
marked this pull request as ready for review
July 28, 2026 18:41
dagnir
reviewed
Jul 28, 2026
| import software.amazon.awssdk.mapper.dynamodb.pojos.AutoKeyAndVal; | ||
| import software.amazon.awssdk.mapper.dynamodb.pojos.TestClass; | ||
|
|
||
| public class StandardModelFactoriesEdgeCasesTest { |
Contributor
There was a problem hiding this comment.
Can you add a javadoc for this class? What specific edge cases are we testing? why?
| * compatibility with dates written by the v1 mapper. | ||
| */ | ||
| @SdkInternalApi | ||
| public final class MapperDateUtils { |
Comment on lines
+21
to
+25
| import org.joda.time.DateTimeZone; | ||
| import org.joda.time.format.DateTimeFormat; | ||
| import org.joda.time.format.DateTimeFormatter; | ||
| import org.joda.time.format.ISODateTimeFormat; | ||
| import org.joda.time.tz.FixedDateTimeZone; |
Contributor
There was a problem hiding this comment.
Do we need to keep using Joda time for this internal class?
| final List<ByteBuffer> result = new ArrayList<ByteBuffer>(value.bs().size()); | ||
| for (SdkBytes sb : value.bs()) { | ||
| // Writable copy for v1 parity; SdkBytes.asByteBuffer() would be read-only. | ||
| result.add(ByteBuffer.wrap(sb.asByteArray())); |
Contributor
There was a problem hiding this comment.
Creating a writable ByteBuffer from SdkBytes seems to occur often enough that it might make sense to create a util method
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
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.
Scope
This PR ports only the converter/marshalling layer of the v1
DynamoDBMapperto the v2AttributeValuetype (software.amazon.awssdk.services.dynamodb.model.AttributeValue). Every operation onDynamoDBMapper(load, save, query, scan, batch, transaction, tableAdmin) stays on v1 and is ported in later PRs.The PR does not compile - The files that stay v1 (the two operation files, the paginated list classes that feed v1 results into v2
toParameters, andDynamoDBMapperFieldModel's query condition methods) reference v1 types that no longer line up with the v2 converter surface, so those files are red. This keeps the diff scoped to the converter layer, and the subsequent operation PRs turn it green.This is intentional in order to make each PR review boundary conceptually separate and limits the diff.
Testing approach
Baseline on v1 - Run existing + new test against the v1 source before porting the types to v2
StandardModelFactoriesEdgeCasesTest) to widen coverage of the boundaries this migration touches (empty vs unset collections, binary roundtrips, boolean encodings, date formats)Migrate the test bodies, not the assertions. For each ported test we changed only the mechanical v1→v2 surface in the test body (
new AttributeValue().withS(x)→AttributeValue.builder().s(x).build(),getS()→s(), and so on) The assertion logic stayed the same (aside from migrating the new types needed for assertion). A green run therefore means the v2 source produces the same result the v1 source did.Running them despite the red module. The
maven-compiler-plugintestIncludesallowlist inpom.xmlcompiles only the converter tests. To execute them we temporarily stubbed the operation files (and their closure), compiled, and ran the suite: 287 passed, 0 failed. The stub was then reverted, restoring the red state.Changes
Mechanical swap
1.
set()/with()→builder().build()AttributeValueis mutable (new AttributeValue().withS(x));AttributeValue.builder().s(x).build()).Question should we use the fast
createX()factory methods added in #7039 here in this PR?I decided to not port these here so we can measure performance against a baseline port vs performance improvement branch later.
2. null vs empty —
hasguardsv2
.l()/.m()/.ss()/.ns()/.bs()return empty collections (not null) when unset. v2 addedhasL()/hasM()/hasSs()/hasNs()/hasBs()to distinguish "explicitly empty" from "unset". Guards were added in the parenttypeCheckmethods (LUnmarshaller,MUnmarshaller,SSUnmarshaller, etc.), which run beforeunmarshallviaConversionSchemasdispatch. Pattern:value.hasSs() ? value.ss() : null.3. Boolean legacy
"1"/"0"encodingOld DynamoDB stored booleans as Number
"1"/"0"before nativeBOOLexisted. The reader accepts both and produces the sameBoolean; the writer produces the number form.V2CompatibleBool.get:if (o.bool() != null) return o.bool() ? "1" : "0"; return o.n();Covered by
StandardModelFactoriesV2UnconvertTest—n("0")/n("1"),bool(false)/bool(true), and boolean set fromns("1")/ns("0")/ns("0","1").Needs Reviewer Feedback:
3. SdkBytes boundary
v1 binary is
java.nio.ByteBuffer; v2 binary issoftware.amazon.awssdk.core.SdkBytes. On write,SdkBytes.fromByteBuffer(bb)copies the buffer and leaves the caller's position untouched, matching v1.On read, POJO fields typed
ByteBuffermust stay mutable to match v1'sgetB().SdkByteswhich AttributeValue uses internally, is immutable and exposes its bytes only as a readonly buffer (viaasByteBuffer()) so everyByteBufferread site returnsByteBuffer.wrap(sdkBytes.asByteArray()). The copy costs one O(n) pass per read, so this path is slower than v1Returning
asByteBuffer()is arguably the better, more v2 idiomatic choice, but if a v1 customer tries to mutate a buffer between reads and writes, the immutable implementation would throwReadOnlyBufferExceptionat runtime.As part of future work I plan to add an opt-in feature (field annotation or mapper config flag) that returns the readonly zero copy buffer instead of the mutable copy, for callers who never mutate and want to beat v1's read performance. Same
ByteBufferfield type, behavior selected by the flag.Question - Should we pursue backwards compatibility? Or limit potential performance regression? Or add support for it via future opt in flag? (supporting both cases)
5.
MapperDateUtils.java(new file)The date converters format and parse ISO-8601 strings through two methods on v1's
com.amazonaws.util.DateUtils. A v2 module can't depend on that class, and v2'ssoftware.amazon.awssdk.utils.DateUtilsisn't equivalent: it works injava.time.Instantrather thanDate, and its formatter omits zero milliseconds (...T00:00:00Zvs v1's...T00:00:00.000Z), so switching to it would change the stored string form of dates and break round-trip compatibility with v1 data.MapperDateUtilsports only the two methods the converters use, on v1's Joda-Time formatters, to keep that format identical. A partial port is enough since the converter layer touches nothing else inDateUtils.Question - The v1 mapper Date types are tightly coupled to Joda-Time (a library that the v2 SDK doesnt use at all). For the sake of backwards compatibility, I ported the APIs the mapper relies on as a thin utils layer. Should we push for a bigger refactor to completely decouple the mapper from Joda time?
6. MapperExceptions.java (new file)
Both marshallers wrapped errors with v1's
Throwables.failure, which a v2 module can't depend on. Instead we ported thefailurefunction helper into a new classMapperExceptions.javaBreaking change: checked exceptions now wrap in
SdkClientExceptioninstead of v1'sAmazonClientException. The breaking change is unavoidable, so users who are explicitly error handling the v1 exception would have to update their code beyond the promised "import swap".Question - Is the name for this new class acceptable? Should we port this method as a standalone and throw it in some other util package we have in the SDK?