Fixed DynamoDbEnhancedClient TableSchema::itemToMap to handle flatten… #5832
Conversation
|
Please update the motivation with more details like why are we fixing this now ? |
@joviegas Updated the PR with more details according to the template. |
CHANGELOG.md
Outdated
| @@ -1,4 +1,9 @@ | |||
| #### 👋 _Looking for changelogs for older versions? You can find them in the [changelogs](./changelogs) directory._ | |||
| # __2.30.10__ __2025-01-30__ | |||
There was a problem hiding this comment.
Please remove this change , since this are auto generated by release.
| @@ -1386,6 +1386,11 @@ public void buildAbstractWithFlatten() { | |||
|
|
|||
| assertThat(tableSchema.itemToMap(item, true), | |||
| is(singletonMap("documentString", AttributeValue.builder().s("test-string").build()))); | |||
|
|
|||
| Map<String, AttributeValue> attributeMapWithNulls = tableSchema.itemToMap(item, false); | |||
There was a problem hiding this comment.
Can we add new test case for this with separate assertions ?
There was a problem hiding this comment.
New test added.
| Map<String, AttributeValue> attributeMapWithNulls = tableSchema.itemToMap(item, false); | ||
| assertThat(attributeMapWithNulls.size(), is(2)); | ||
| assertThat(attributeMapWithNulls, hasEntry("documentString", AttributeValue.builder().s("test-string").build())); | ||
| assertThat(attributeMapWithNulls, hasEntry("documentInteger", AttributeValue.fromNul(true))); |
There was a problem hiding this comment.
Can we add a test case to check what happens when there is Inner Nested classe and self refering nested class
@DynamoDbBean
public class FlattenedImmutableBean {
private String id;
private String attribute1;
@DynamoDbFlatten
private AbstractImmutable abstractImmutable;
}
@DynamoDbImmutable(builder = AbstractImmutable.Builder.class)
public class AbstractImmutable {
private final String attribute2;
private final AbstractImmutable abstractImmutableOne;
}We have some existing tests classes like src/test/java/software/amazon/awssdk/enhanced/dynamodb/mapper/testbeans/AbstractImmutable.java
can we add more cases here ?
There was a problem hiding this comment.
New tests added.
There was a problem hiding this comment.
Thanks 👍
Will run the codebuilds , and then approve and merge once code build passes
| Map<String, AttributeValue> attributeMapWithNulls = tableSchema.itemToMap(item, false); | ||
| assertThat(attributeMapWithNulls.size(), is(2)); | ||
| assertThat(attributeMapWithNulls, hasEntry("documentString", AttributeValue.builder().s("test-string").build())); | ||
| assertThat(attributeMapWithNulls, hasEntry("documentInteger", AttributeValue.fromNul(true))); | ||
| } |
There was a problem hiding this comment.
Could you please add a test case for UpdateItem where we update DDB entry similar as #5380 , to make sure it works for nested classes
There was a problem hiding this comment.
@joviegas I checked the UpdateBehaviorTest class and it contains tests which covers updating DDB entry with nested and flatten members, with both IgnoreNullsMode.SCALAR_ONLY and IgnoreNullsMode.DEFAULT modes. Do you have specific scenario in mind which requires more coverage?
There was a problem hiding this comment.
will have look at it today and update a sample test
There was a problem hiding this comment.
Yeap I saw it now , then we are good.
iulianbudau
force-pushed
the
bugfix/table-schema-item-to-map-handle-null-flattened-members
branch
from
d8b81f4 to
e74034a
Compare
…ed members when ignoreNulls is false
iulianbudau
force-pushed
the
bugfix/table-schema-item-to-map-handle-null-flattened-members
branch
from
e74034a to
51eb1e8
Compare
…l-flattened-members
…ed members when ignoreNulls is false (#5832) Co-authored-by: John Viegas <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…l-flattened-members
|
Fixed [DynamoDbEnhancedClient]
TableSchema::itemToMapdoesn't respect ignoreNulls with null@DynamoDbFlattenedmembers #2540.Description
Current behavior:
TableSchema.fromBean(Bean.class).itemToMap(bean, false)returns a map without members of any @DynamoDbFlatten'd members if the flattened member is null even though ignoreNulls is set to false.Expected behavior:
TableSchema::itemToMapshould return a map that contains a consistent representation of null top-level (non-flattened) attributes and flattened attributes when their "parent" member is null and ignoreNulls is set to false.Motivation and Context
#2540
Modifications
Updated the
StaticImmutableTableSchema -> Map<String, AttributeValue> itemToMap(T item, boolean ignoreNulls)logic to properly account for the ignoreNulls check and generateAttributeValue.fromNul(true)values for attributes of@DynamoDbFlattenedmembers that are null.Testing
Unit tests coverage added.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License