Empty beans are now preserved through serialization and deserialization

[reed53]

Description

Previously when a bean that had all null attributes, but itself was not null, was serialized and deserialized, the bean would become null. With this change, the bean is not lost through serialization and deserialization.

Motivation and Context

This pull request aims to fix the following issue: #2280
My team and I were trying to move to SDK 2, however this issue caused a loss in preservation through Dynamo, so this aims to fix that.

Testing

Previously existing tests were modified so that if given an empty bean, it expects an empty bean in return.

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Checklist

• I have read the CONTRIBUTING document
• Local run of mvn install succeeds
• My code follows the code style of this project
• My change requires a change to the Javadoc documentation
• I have updated the Javadoc documentation accordingly
• I have read the README document
• I have added tests to cover my changes
• All new and existing tests passed
• A short description of the change has been added to the CHANGELOG
• My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

• I confirm that this pull request can be released under the Apache 2 license

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[zoewangg]

Thank you for your PR and apologies for the delayed response. We will take a look shortly

[zoewangg]

I understand the use case, but this change might be backwards-incompatible. I'll discuss with the team.

[zoewangg]

Thank you for your patience! We can't merge this change for backwards-compatibility reasons. We think customizing it at the attribute level might be a better way to support this. It would be something like DynamoDbUpdateBehavior, say DynamoDbPreserveEmptyBean (name subject to change) and you can put it on the attribute where you want to preserve the empty beans.

https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/annotations/DynamoDbUpdateBehavior.java

Would you be able to make the change? If not, we will add this task to our backlog, but we don't have a timeline for this change at the moment.

[reed53]

Thank you for your patience! We can't merge this change for backwards-compatibility reasons. We think customizing it at the attribute level might be a better way to support this. It would be something like DynamoDbUpdateBehavior, say DynamoDbPreserveEmptyBean (name subject to change) and you can put it on the attribute where you want to preserve the empty beans.

https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/annotations/DynamoDbUpdateBehavior.java

Would you be able to make the change? If not, we will add this task to our backlog, but we don't have a timeline for this change at the moment.

That sounds good! I will start working on that now, and let you know when I finish.

[codecov-io]

Codecov Report

Merging #2334 (588ff7d) into master (7e9b6f2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #2334      +/-   ##
============================================
+ Coverage     77.66%   77.67%   +0.01%     
  Complexity      366      366              
============================================
  Files          1244     1244              
  Lines         39272    39270       -2     
  Branches       3106     3104       -2     
============================================
+ Hits          30499    30503       +4     
+ Misses         7285     7283       -2     
+ Partials       1488     1484       -4     

Flag	Coverage Δ	Complexity Δ
unittests	77.67% <100.00%> (+0.01%)	0.00 <0.00> (ø)

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

Impacted Files	Coverage Δ	Complexity Δ
...ed/dynamodb/mapper/StaticImmutableTableSchema.java	88.41% <100.00%> (-0.14%)	0.00 <0.00> (ø)
...ssdk/core/internal/async/FileAsyncRequestBody.java	83.96% <0.00%> (+2.83%)	0.00% <0.00%> (ø%)
...ty/internal/IdleConnectionCountingChannelPool.java	89.02% <0.00%> (+3.65%)	0.00% <0.00%> (ø%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e9b6f2...588ff7d. Read the comment docs.

[reed53]

Hi @zoewangg, I've been working on updating the pull request but I've been running into issues as to how to pass information into the inner bean. At first, I tried to implement an annotation on a method, however since I need the attribute key to access tags, it would be impossible to get the tag from the inner bean, since I don't have access to the attribute key the outer bean uses. I then thought to instead implement an annotation on the class and set a flag on the table schema for the inner bean upon its creation in order to tell whether the bean should be preserved when empty or not. The issue with this is that the only way to pass down this information would be to either modify the parameters of public methods, or to modify how the schemas are recursively created to make it avoid the public methods. Is there a better approach to this that you can think of?

[zoewangg]

Yeah, this is bit tricky. I looked into it yesterday, and I can't think of other approaches. @bmaizels, can you think of anything?

The issue with this is that the only way to pass down this information would be to either modify the parameters of public methods, or to modify how the schemas are recursively created to make it avoid the public methods

Could you elaborate on this? More specifically, what public methods need to be modified? We can update the public APIs in the internal classes (classes with @SdkInternalApi annotation)

[reed53]

Yeah, this is bit tricky. I looked into it yesterday, and I can't think of other approaches. @bmaizels, can you think of anything?

The issue with this is that the only way to pass down this information would be to either modify the parameters of public methods, or to modify how the schemas are recursively created to make it avoid the public methods

Could you elaborate on this? More specifically, what public methods need to be modified? We can update the public APIs in the internal classes (classes with @SdkInternalApi annotation)

The public method create for BeanTableSchema is referenced by recursiveCreate, and BeanTableSchema is unfortunately labeled as public api. The reason I wanted to use this method specifically is because that is what is used when the inner bean schemas are created, which would be the ideal spot to include the preserve empty behavior. Since the create method receives the class, it may be possible to check for the annotation within that method if the annotation is on the class. If that seems reasonable, please let me know.

[zoewangg]

I talked with @bmaizels offline. It seems there are some other approaches we can look into:

• create a client-level configuration for this behavior, although it means all beans will have this behavior.
• introduce a new mapToItem that takes a boolean flag to preserve empty bean, similar to what we have in Map<String, AttributeValue> itemToMap(T item, boolean ignoreNulls)

@reed53 WDUT? Would either of those work for your use-case? I'll also look into it a bit more to see which approach is better and get back to you later.

[reed53]

I talked with @bmaizels offline. It seems there are some other approaches we can look into:

• create a client-level configuration for this behavior, although it means all beans will have this behavior.
• introduce a new mapToItem that takes a boolean flag to preserve empty bean, similar to what we have in Map<String, AttributeValue> itemToMap(T item, boolean ignoreNulls)

@reed53 WDUT? Would either of those work for your use-case? I'll also look into it a bit more to see which approach is better and get back to you later.

The behavior for every bean is ideal, so the client sounds good, however it will still likely be pretty messy to implement, since we need to pass around a flag everywhere. I recently sent an email about the issue and got a response asking to schedule a call with a member of the SDK team. I will wait until after this call to continue working on this issue, as the call may help to clear some things up.

[zoewangg]

Hi @reed53 I investigated a bit more and the #2 option that I mentioned above (introduce a new mapToItem that takes a boolean flag to preserve empty bean) would not work because there's no way to pass the flag to DocumentAttributeConverter#transformTo with current implementation.

I'm not a big fan of client-level configuration as well because it's not very flexible, so I went ahead and implemented the class level annotation. Here is the PR #2364. Feedback is welcome! 🙂

[reed53]

Hi @reed53 I investigated a bit more and the #2 option that I mentioned above (introduce a new mapToItem that takes a boolean flag to preserve empty bean) would not work because there's no way to pass the flag to DocumentAttributeConverter#transformTo with current implementation.

I'm not a big fan of client-level configuration as well because it's not very flexible, so I went ahead and implemented the class level annotation. Here is the PR #2364. Feedback is welcome! 🙂

I have no comments beyond what is already mentioned there, but thank you very much for doing this!

[zoewangg]

Closing in favor of #2364