Support auto construct lists #497

dagnir · 2018-05-23T18:10:50Z

Description

Support auto construct lists. Only contains JSON marshaller changes ATM.

Motivation and Context

Support auto construct lists

Testing

Added new unit tests, reference files, and updated some existing ref files as well.

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

License

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

millems · 2018-05-23T18:16:12Z

...main/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java

@@ -123,6 +123,8 @@

    private Map<String, String> modelMarshallerDefaultValueSupplier;

+    private boolean useAutoConstructList = false;


I've gotten into an existential crisis now. What do auto-construct lists mean for the customer and the service in this context?

Customers shouldn't have to worry about auto constructed lists, which is why I kept the return types as List<T>.

For services, the only one I'm aware of currently is CloudFormation where we explicitly distinguish between a normal empty list and an auto constructed one and only send the normal list variety.

cfnRequestBuilder.withListMember(new ArrayList<>())

vs

cfnRequestBuilder.withListMember(null)

and internally we instantiate an auto constructed list.

@shorea probably has more insight on this.

ELB definitely has some weirdness too. We should test that. I want to say KMS had something weird with Maps but I can't exactly remember.

An auto constructed list is essentially null for marshalling purposes. So for a JSON service we would omit the field entirely from the document if it's an auto constructed list.

Do you think we could get away without having CloudFormation being a special case and just default to empty lists for responses? It's annoying for a customer to distinguish between empty list and null for responses, but having so much code for just CloudFormation feels like a lot of work.

Do we know of any services where there is a semantic difference between null and empty lists in responses?

If the customer doesn't set anything on the request then we treat that as auto constructed list and send null on the wire (whatever that means for the protocol)
If the customer explicitly sets an empty list then we treat that as non-null and will send an empty list on the wire.

Inbound both null and empty lists will appear as empty lists to the customer with no real way to distinguish between them. If a service has a semantic difference between this (which they probably shouldn't) then we'd have to not auto construct for that service or perhaps just operation.

shorea · 2018-05-23T19:13:43Z

...software/amazon/awssdk/codegen/poet/model/autoconstructcontainers/listofblobstypecopier.java

+@Generated("software.amazon.awssdk:codegen")
+final class ListOfBlobsTypeCopier {
+    static List<ByteBuffer> copy(Collection<ByteBuffer> listOfBlobsTypeParam) {
+        if (listOfBlobsTypeParam == null) {


Do we need to check auto construct here to? Can we add a test to make sure that a POJO with an auto constructed list is preserved when we go from toBuilder and back?

Hmm, we shouldn't have to. We use the model copiers on all builder setters (unless the param is something immutable like String) and the builder constructor from the model class, so it should always be type of AutonConstructAwareList, if that's what the customization config says.

Would making the field type AutoConstructAwareList suffice?

I just realized that we wrap in unmodifiableList before returning which will mess with instanceof checks we might do later in the marshallers. I'll need to make a few changes.

Updated to do an instance check. Also added tests to verify this behavior

shorea · 2018-05-23T19:14:06Z

...main/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java

@@ -123,6 +123,8 @@

    private Map<String, String> modelMarshallerDefaultValueSupplier;

+    private boolean useAutoConstructList = false;


This should default to true.

Any reason why? It's default false in v1: https://github.com/aws/aws-sdk-java/blob/7bf266202554a33d1f1e89d1691a5a389d36966c/aws-java-sdk-code-generator/src/main/java/com/amazonaws/codegen/model/config/customization/CustomizationConfig.java#L41

We want to go all in for auto construct unless we absolutely can't do it for whatever reason right? If we make it opt in via customization we'll forget to do it for a service and become inconsistent.

Cool, wasn't aware of that but cool with it! Will update

Updated to make auto construct lists the default

shorea · 2018-05-23T19:14:48Z

core/src/main/java/software/amazon/awssdk/core/util/SdkAutoConstructAwareList.java

+    /**
+     * @return {@code true} if this list was auto constructed.
+     */
+    boolean isAutoConstructed();


Is this needed?

Not sure what you mean. Do you mean this should just be a marker interface?

Removed. Now using it basically as a marker interface.

shorea · 2018-05-23T19:15:34Z

core/src/main/java/software/amazon/awssdk/core/util/DefaultSdkAutoConstructAwareList.java

+        isAutoConstructed = true;
+    }
+
+    public DefaultSdkAutoConstructAwareList(Collection<? extends T> other) {


Do we need these other constructors or can we just use the list implementation they pass in?

I'd prefer to keep this since we make use of the copy constructor in in the model copier classes.

Removed per the above comment.

shorea

This looks good to me. Can you test with ELB and that cloudformation use case you mentioned?

shorea · 2018-05-31T17:45:54Z

...erated-classes-test/src/main/resources/codegen-resources/autoconstructedlists/codegen.config

@@ -0,0 +1,5 @@
+{
+    "serviceInterfaceName" : "AmazonCodeGenerationJsonRpcCustomized",
+    "packageSuffix" : "protdofjafoobarbazocol.jsonrpc.customized",


Lol what a name.

LOL, I was trying to see if packageSuffix was supported but it doesn't seem to be atm.

…4f1e2565 Pull request: release <- staging/9e1b3fb5-5971-4be8-9f8b-0f434f1e2565

millems reviewed May 23, 2018

View reviewed changes

shorea reviewed May 23, 2018

View reviewed changes

shorea reviewed May 31, 2018

View reviewed changes

shorea approved these changes May 31, 2018

View reviewed changes

dagnir force-pushed the autoconstruct-list-json branch 3 times, most recently from 56b71b6 to 2d9076a Compare May 31, 2018 20:26

Support auto construct lists

a61c5fb

dagnir force-pushed the autoconstruct-list-json branch from 2d9076a to a61c5fb Compare May 31, 2018 20:56

dagnir merged commit cd2662a into aws:master May 31, 2018

dagnir deleted the autoconstruct-list-json branch May 31, 2018 21:11

dagnir mentioned this pull request Jun 7, 2018

Handle autoconstruct lists in non-JSON protocols #529

Merged

12 tasks

aws-sdk-java-automation added a commit that referenced this pull request May 14, 2019

Merge pull request #497 from aws/staging/9e1b3fb5-5971-4be8-9f8b-0f43…

4021b7d

…4f1e2565 Pull request: release <- staging/9e1b3fb5-5971-4be8-9f8b-0f434f1e2565

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support auto construct lists #497

Support auto construct lists #497

dagnir commented May 23, 2018 •

edited

Loading

millems May 23, 2018

dagnir May 23, 2018

shorea May 23, 2018

millems May 23, 2018

shorea May 23, 2018

shorea May 23, 2018

dagnir May 23, 2018

dagnir May 23, 2018

dagnir May 24, 2018

shorea May 23, 2018

dagnir May 23, 2018

shorea May 30, 2018

dagnir May 30, 2018

dagnir May 30, 2018

shorea May 23, 2018

dagnir May 23, 2018

dagnir May 24, 2018

shorea May 23, 2018

dagnir May 23, 2018

dagnir May 24, 2018

shorea left a comment

shorea May 31, 2018

dagnir May 31, 2018

		@@ -123,6 +123,8 @@

		private Map<String, String> modelMarshallerDefaultValueSupplier;

		private boolean useAutoConstructList = false;

Support auto construct lists #497

Support auto construct lists #497

Conversation

dagnir commented May 23, 2018 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shorea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagnir commented May 23, 2018 •

edited

Loading