Json Marshaller Migration To Poet #402

zoewangg · 2018-01-29T18:17:12Z

Description

Related to #30
To favor our new practices of making PR smaller and getting reviewed faster. This task is broken into the the following parts:

Marshaller Refactor
a. Json, rest-json marshaller
b. query xml, rest-xml, ec2 marshaller
c. api gateway marshaller
Unmarshaller Refacctor
a. Json, rest-json marshaller
b. query xml, rest-xml, ec2 marshaller
c. api gateway marshaller

This PR is for 1.a Json, rest-json marshaller

Motivation and Context

Replacing freemarker with poet
Removing unused files.

Testing

mvn clean install and integration test

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

shorea

No major concerns. Are the protocol tests passing?

shorea · 2018-01-31T19:49:50Z

codegen/src/main/java/software/amazon/awssdk/codegen/internal/Freemarker.java

    public Template getModelMarshallerTemplate() throws IOException {
        return getTemplate(templateConfig.getModelMarshaller());
    }

-    public Template getRequestMarshallerTemplate() throws IOException {


Would this still be used by other protocols?

Nope, this is only used by json protocol.

shorea · 2018-01-31T19:51:16Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/transform/JsonModelMarshallerSpec.java

+    private final ClassName requestClassName;
+
+    public JsonModelMarshallerSpec(IntermediateModel intermediateModel, ShapeModel shapeModel, String className) {
+        final PoetExtensions poetExtensions = new PoetExtensions(intermediateModel);


redundant finals, we don't usually do that for locals, especially in Java8

Intellij auto created it. Will update the setting and this.

shorea · 2018-01-31T19:52:05Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/transform/JsonModelMarshallerSpec.java

+                                               .addAnnotation(PoetUtils.GENERATED)
+                                               .addAnnotation(SdkInternalApi.class)
+                                               .addMethods(marshalMethods())
+                                               .addFields(memberVariables())


I know it doesn't matter since poet orders correctly but order is opposite of what i'd expect.

I'm not sure that poet does reorder annotations - I'm fairly sure they'll go on in the order that they're called on the builder....

Or did you mean fields/methods (that it does do correctly)

yeah fields and methods, here the member variables are added after the method but poet rearranges it. just looked weird when reading it

shorea · 2018-01-31T19:54:41Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/transform/JsonModelMarshallerSpec.java

+
+        methodSpecs.add(constructor());
+        methodSpecs.add(getInstanceMethod());
+        methodSpecs.add(marshalMethod());


I'm a two 'l' guy myself but apparently both spellings are valid. ¯_(ツ)_/¯

We should definitely choose one and be consistent!

two 'l' it is!

shorea · 2018-01-31T19:57:58Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/transform/JsonModelMarshallerSpec.java

+                initializationCodeBlockBuilder.add(".marshallLocationName($S)", memberModel.getHttp().getMarshallLocationName());
+            }
+
+            if (memberModel.getIsBinary()) {


Should we just always set this explicitly? If not can we inline true rather than consult getIsBinary again.

shorea · 2018-01-31T20:07:59Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/transform/MarshallerSpec.java

+import software.amazon.awssdk.core.runtime.transform.Marshaller;
+import software.amazon.awssdk.utils.Validate;
+
+public class MarshallerSpec implements ClassSpec {


Not exactly sure what this is for.

This is the high level class used inMarshallerGeneratorTasks to create the marshaller class and it uses different implementations of MarshallerProtocolSpec to add the marshaller methods based on the protocol. This PR only contains JsonMarshallerSpec. This class is similar to AsyncClientClass

shorea · 2018-01-31T20:15:58Z

...amazon/awssdk/codegen/poet/transform/operationwithnoinputoroutputrequestmodelmarshaller.java

+                         ProtocolMarshaller protocolMarshaller) {
+        Validate.paramNotNull(operationWithNoInputOrOutputRequest, "operationWithNoInputOrOutputRequest");
+        Validate.paramNotNull(protocolMarshaller, "protocolMarshaller");
+        try {


Be nice to get rid of this if it's easy to do.

shorea · 2018-01-31T20:19:45Z

...ware/amazon/awssdk/codegen/poet/transform/streaminginputoperationrequestmodelmarshaller.java

+    public void marshall(StreamingInputOperationRequest streamingInputOperationRequest, ProtocolMarshaller protocolMarshaller) {
+        Validate.paramNotNull(streamingInputOperationRequest, "streamingInputOperationRequest");
+        Validate.paramNotNull(protocolMarshaller, "protocolMarshaller");
+        try {


This doesn't seem right, shouldn't it be marshalling the streaming member?

don't we use StreamingRequestMarshaller to marshall all streaming members in v2?

…47cefc16 Pull request: release <- staging/9e20f076-9332-4cde-9ef8-c28b47cefc16

zoewangg changed the title ~~Json Marshaller Refactor~~ Json Marshaller Migration To Poet Jan 31, 2018

shorea reviewed Jan 31, 2018

View reviewed changes

zoewangg force-pushed the MarshallerRefactor branch 2 times, most recently from e42a39d to 61af631 Compare February 2, 2018 21:48

shorea approved these changes Feb 3, 2018

View reviewed changes

Json Marshaller Refactor

8f76ce3

zoewangg force-pushed the MarshallerRefactor branch from 61af631 to 8f76ce3 Compare February 3, 2018 00:14

zoewangg merged commit 713066c into aws:master Feb 3, 2018

zoewangg deleted the MarshallerRefactor branch February 3, 2018 00:33

aws-sdk-java-automation pushed a commit that referenced this pull request Feb 5, 2019

Merge pull request #402 from aws/staging/9e20f076-9332-4cde-9ef8-c28b…

9e36de6

…47cefc16 Pull request: release <- staging/9e20f076-9332-4cde-9ef8-c28b47cefc16

Json Marshaller Migration To Poet #402

Json Marshaller Migration To Poet #402

Uh oh!

Conversation

zoewangg commented Jan 29, 2018

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

shorea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!