-
Notifications
You must be signed in to change notification settings - Fork 915
Allow object serialization on DefaultSdkAutoConstructList and Default… #976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…SdkAutoConstructMap
@zoewangg Could you please take a look at this PR? Thanks! |
@@ -158,4 +162,8 @@ public int lastIndexOf(Object o) { | |||
return impl.subList(fromIndex, toIndex); | |||
} | |||
|
|||
// For deserialization | |||
private Object readResolve() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give us more context of why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a serialization trick to ensure that we use the singleton instance (INSTANCE on line 37) when we deserialize this class's object. (Usually, deserialization constructs a new object without readResolve())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zoewangg ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay. Hmm, I'm not sure if we want to add this. This seems a bit hacky to me.
Codecov Report
@@ Coverage Diff @@
## master #976 +/- ##
============================================
+ Coverage 55.39% 55.39% +<.01%
- Complexity 4492 4494 +2
============================================
Files 799 799
Lines 27487 27489 +2
Branches 2224 2224
============================================
+ Hits 15226 15228 +2
Misses 11543 11543
Partials 718 718
Continue to review full report at Codecov.
|
@@ -158,4 +162,8 @@ public int lastIndexOf(Object o) { | |||
return impl.subList(fromIndex, toIndex); | |||
} | |||
|
|||
// For deserialization | |||
private Object readResolve() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay. Hmm, I'm not sure if we want to add this. This seems a bit hacky to me.
@@ -0,0 +1,29 @@ | |||
package software.amazon.awssdk.core.util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test files are missing copyright header. You can copy paste the copyright header from other file in this repo
Closing this PR for now as it seems it hasn't been update since @zoewangg made some comments. Please feel free to reopen once it's updated and ready for another look. |
…a702002f Pull request: release <- staging/f887abb7-3b40-41bf-8e0f-467fa702002f
Description
Allow Object serialization on DefaultSdkAutoConstructList and DefaultSdkAutoConstructMap.
Motivation and Context
software.amazon.awssdk.services.ec2.model.InstanceStatus isn't serializable because DefaultSdkAutoConstructList instance doesn't implement Serializable.
Testing
Wrote simple tests to check if DefaultSdkAutoConstructList & DefaultSdkAutoConstructMap are serializable and deserializable. They fail before the fix and pass after the fix.
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsLicense