Refactoring for JSON marshalling/unmarshalling, support for JSON valu… #727

shorea · 2018-09-24T19:07:13Z

…e trait

varunnvs92

Are the codegen generation tests updated?

varunnvs92 · 2018-09-24T20:37:03Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/intermediate/IntermediateModel.java

@@ -64,7 +69,7 @@ public IntermediateModel(
        @JsonProperty("customizationConfig") CustomizationConfig customizationConfig,
        @JsonProperty("serviceExamples") ServiceExamples examples) {

-        this(metadata, operations, shapes, customizationConfig, examples, Collections.emptyMap(), Collections.emptyMap());
+        this(metadata, operations, shapes, customizationConfig, examples, Collections.emptyMap(), Collections.emptyMap(), null);


You can pass in DefaultNamingStrategy instead of null

DefaultNamingStrategy requires the c2j models and we only have intermediate at this point.

varunnvs92 · 2018-09-24T22:54:39Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/http/NewJsonResponseHandler.java

+
+
+        try {
+            T result = unmarshaller.unmarshall(pojoSupplier, response);


In which case this will be null?

If the payload is empty, I need to find a cleaner way to handle this.

This has been fixed. Can't remember how but it has lol

varunnvs92 · 2018-09-24T22:56:33Z

...dk-core/src/main/java/software/amazon/awssdk/core/internal/protocol/json/JsonMarshaller.java

@@ -38,6 +39,6 @@ public void marshall(Void val, JsonMarshallerContext context, String paramName)
     * @param context   Dependencies needed for marshalling.
     * @param paramName Optional param/field name. May be null in certain situations.
     */


Nit: add doc sdkField param

varunnvs92 · 2018-09-24T23:10:37Z

...src/main/java/software/amazon/awssdk/core/internal/protocol/json/JsonProtocolMarshaller.java

-    @Override
-    public Request<OrigRequestT> finishMarshalling() {
+    private Object resolveValue(Object val, SdkField<?> marshallingInfo) {
+        IdempotencyTrait trait = marshallingInfo.getTrait(IdempotencyTrait.class);


Didn't understand why we are using IdempotencyTrait here>

Discussed offline. The idempotency trait auto fills a value when none is provided.

FYI this has changed to the DefaultValueTrait to accomodate Glacier.

varunnvs92 · 2018-09-24T23:51:40Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/protocol/SdkField.java

+import software.amazon.awssdk.core.protocol.traits.LocationTrait;
+import software.amazon.awssdk.core.protocol.traits.Trait;
+
+public class SdkField<TypeT> {


Add some javadocs as its used widely

Added javadocs to all the relevant classes

varunnvs92 · 2018-09-25T17:19:36Z

...src/main/java/software/amazon/awssdk/core/internal/protocol/json/JsonProtocolMarshaller.java

-    private <V> V resolveValue(V val, MarshallingInfo<V> marshallingInfo) {
-        return val == null && marshallingInfo.defaultValueSupplier() != null ? marshallingInfo.defaultValueSupplier().get() : val;
+    private boolean isBinary(SdkField<?> field, Object val) {
+        return isExplicitPayloadMember(field) && val instanceof SdkBytes;


What about InputStream type? Previously we considered both SdkBytes and InputStream as binary.

Streaming is no longer part of marshalling/unmarshalling. It's handled separately as it differs for sync vs async.

…port for Timestamp format trait

varunnvs92

Overall lgtm. Minor feedback and looks like last commit broke the travis build

varunnvs92 · 2018-10-09T23:42:25Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/AsyncClientClass.java

-        List<String> eventNames = EventStreamUtils.getEvents(eventStreamShape)
-                                                  .map(shape -> shape.getShapeName())
+        List<String> eventNames = EventStreamUtils.getEventMembers(eventStreamShape)
+                                                  .map(MemberModel::getC2jName)


Shouldn't we use shape name instead of c2j name? (incase we change the shape name in customizations)

This should be the member name as defined in the model since we use it for matching the event-type to the right unmarshaller.

varunnvs92 · 2018-10-10T00:50:48Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/ShapeModelSpec.java

+
+    private CodeBlock createLocationTrait(MemberModel m) {
+        return CodeBlock.builder()
+                        // TODO will marshall and unmarshall location name ever differ?


Should we use the build pattern instead of create() here? So in case marshall and unmarshall location differ, we can add new methods without needing to deprecate create.

This should be the member name as defined in the model since we use it for matching the event-type to the right unmarshaller.

Replied to the wrong comment, +1 to the builder.

varunnvs92 · 2018-10-10T01:39:51Z

...ain/java/software/amazon/awssdk/awscore/internal/protocol/json/AwsStructuredCborFactory.java

@@ -26,14 +27,21 @@
 @SdkInternalApi
 public final class AwsStructuredCborFactory extends SdkStructuredCborFactory {

+    private static final ObjectMapper MAPPER = new ObjectMapper(CBOR_FACTORY);


We were talking about getting rid of object mapper to make sdk initialization faster. Is it possible to use jackson-jr classes instead of object mapper?

Yeah I have an action item to replace this with something else. I was playing around with Jackson-JR and it doesn't seem to have the same capabilities as JsonNode so I'll likely have to write something custom.

Got it, not a blocker for this PR

varunnvs92 · 2018-10-10T02:15:39Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/http/NewJsonResponseHandler.java

+ */
+@SdkProtectedApi
+@ReviewBeforeRelease("Metadata in base result has been broken. Fix this and deal with AwsResponseHandlerAdapter")
+public final class NewJsonResponseHandler<T extends SdkPojo> implements HttpResponseHandler<T> {


Name feels little ambiguous. Are we going to replace the current JsonResponseHandler with this class?

Yeah will do, meant to replace it.

varunnvs92 · 2018-10-11T00:40:25Z

...c/main/java/software/amazon/awssdk/core/internal/protocol/json/SimpleTypeJsonMarshaller.java

+                jsonGenerator.writeFieldName(paramName);
+            }
+            TimestampFormatTrait trait = sdkField.getTrait(TimestampFormatTrait.class);
+            if (trait != null) {


Similar logic is present in ValueToStringConverter.FROM_INSTANT. Can you use it here?

They differ subtly in such a way that makes it hard to combine. For example we are always producing a String in ValueToString but we need to know how to write it to the JSON generator (i.e. as a string or as a string number). And we also have the fallback which is needed for CBOR.

…91bcdb2d Pull request: release <- staging/031a365b-dfef-4050-8bd9-a55a91bcdb2d

shorea force-pushed the marshaller-refactor branch from 40ca93f to 0309bb7 Compare September 24, 2018 19:40

Refactoring for JSON marshalling/unmarshalling, support for JSON valu…

59a4edb

…e trait

shorea force-pushed the marshaller-refactor branch from 0309bb7 to 59a4edb Compare September 24, 2018 20:00

varunnvs92 reviewed Sep 25, 2018

View reviewed changes

shorea added 4 commits October 8, 2018 11:29

Various refactoring for JSON (un)marshallers and fixes. Tests and sup…

f67a509

…port for Timestamp format trait

Fixing glacier marshalling

08cbf80

Merge branch 'master' into marshaller-refactor

a91396d

Fixing findbugs in protocol tests.

0569a80

shorea force-pushed the marshaller-refactor branch from 25da4f9 to 0569a80 Compare October 9, 2018 02:19

Fixing Lambda integ tests and adding protocol tests for status code

1a271f9

shorea force-pushed the marshaller-refactor branch from bd9c8d7 to 34a78c6 Compare October 9, 2018 23:53

Cleaning up protocol factory and introducing builder

339daa8

shorea force-pushed the marshaller-refactor branch from 34a78c6 to 339daa8 Compare October 10, 2018 00:03

varunnvs92 reviewed Oct 10, 2018

View reviewed changes

Fixes and removing some unused classes

6e848bd

varunnvs92 reviewed Oct 11, 2018

View reviewed changes

varunnvs92 approved these changes Oct 11, 2018

View reviewed changes

Merge branch 'master' into marshaller-refactor

dc44552

shorea merged commit 8dccb12 into master Oct 11, 2018

shorea deleted the marshaller-refactor branch November 23, 2018 03:55

aws-sdk-java-automation added a commit that referenced this pull request Feb 6, 2020

Merge pull request #727 from aws/staging/031a365b-dfef-4050-8bd9-a55a…

3f586fa

…91bcdb2d Pull request: release <- staging/031a365b-dfef-4050-8bd9-a55a91bcdb2d



		try {
		T result = unmarshaller.unmarshall(pojoSupplier, response);

Refactoring for JSON marshalling/unmarshalling, support for JSON valu… #727

Refactoring for JSON marshalling/unmarshalling, support for JSON valu… #727

Uh oh!

Conversation

shorea commented Sep 24, 2018

Uh oh!

varunnvs92 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!

varunnvs92 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!

Uh oh!