Rest Xml marshaller/unmarshaller refactor #758

varunnvs92 · 2018-10-15T17:22:17Z

Refactored xml marshaller/unmarshaller similar to json/query
Addressed most of the old comments
Perf tests results are similar to json (a bit slower than v1). But it is ~10 micro seconds depending on the file size which is minute compared to other latency like network calls

TODO

Will run integ tests

codegen/src/main/resources/templates/rest-xml/request-marshaller.ftl

shorea · 2018-10-15T17:51:48Z

codegen/src/test/resources/software/amazon/awssdk/codegen/poet/model/alltypesrequest.java

@@ -74,7 +74,7 @@
                    .memberFieldInfo(
                        SdkField.<String> builder(MarshallingType.STRING)
                            .traits(LocationTrait.builder().location(MarshallLocation.PAYLOAD)
-                                                 .locationName("member").build()).build()).build()).build();
+                                                 .locationName("member").build()).build()).flattened(false).build()).build();


In my branch I only add flattened when it's not false. Helps cut the noise down a bit.

shorea · 2018-10-15T17:53:21Z

...ws-core/src/main/java/software/amazon/awssdk/awscore/protocol/xml/AwsXmlProtocolFactory.java

+
+    public <T extends AwsRequest> ProtocolMarshaller<Request<T>> createProtocolMarshaller(OperationInfo operationInfo,
+                                                                                          T origRequest,
+                                                                                          String xmlNameSpaceUri) {


Might be worth having some mechanism to communicate protocol specific metadata in OperationInfo. Like some attribute map similar to the execution attributes. Or allowing protocols to extend OperationInfo to add additional properties.

shorea · 2018-10-15T17:56:14Z

...re/src/main/java/software/amazon/awssdk/core/internal/protocol/restxml/HeaderMarshaller.java

+
+    // TODO Is it required? No shape uses this. Fix casting
+    // See line 8 in MarshalHeaderMembersMacro.ftl
+    public static final XmlMarshaller<List> LiST = new SimpleHeaderMarshaller<List>(null) {


I don't think we need to support this and should remove it until we need it. It's not clear how we should marshall lists in headers as it's not yet defined.

shorea · 2018-10-15T17:57:29Z

...re/src/main/java/software/amazon/awssdk/core/internal/protocol/restxml/HeaderMarshaller.java

+                return;
+            }
+
+            for (Map.Entry<String, String> entry : ((Map<String, String>) map).entrySet()) {


Do we need to assume it's a string or can we lookup in the marshaller registry and delegate?

shorea · 2018-10-15T18:25:28Z

...n/java/software/amazon/awssdk/core/internal/protocol/restxml/unmarshall/XmlUnmarshaller.java

+@SdkInternalApi
+public interface XmlUnmarshaller<T> {
+
+    T unmarshall(XmlUnmarshallerContext context,


NP one liner

shorea · 2018-10-15T18:27:11Z

pom.xml

@@ -549,7 +549,7 @@
                    </execution>
                </executions>
                <configuration>
-                    <failOnWarning>true</failOnWarning>
+                    <failOnWarning>false</failOnWarning>


What's this?

shorea · 2018-10-15T18:27:22Z

services/budgets/pom.xml

@@ -29,4 +29,14 @@
        AWS Budgets Service
    </description>
    <url>https://aws.amazon.com/sdkforjava</url>
+
+    <dependencies>


shorea · 2018-10-15T18:27:47Z

services/cloudfront/src/main/resources/codegen-resources/log4j.properties

@@ -0,0 +1,29 @@
+
+#


Why's this in codegen-resources?

shorea · 2018-10-15T18:27:55Z

services/s3/pom.xml

@@ -63,5 +63,9 @@
            <version>${awsjavasdk.version}</version>
            <scope>test</scope>
        </dependency>
+        <dependency>


shorea · 2018-10-25T18:30:08Z

...c/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlPayloadMarshaller.java

+        @Override
+        public void marshall(SdkBytes val, XmlMarshallerContext context, String paramName, SdkField<SdkBytes> sdkField,
+                             ValueToStringConverter.ValueToString<SdkBytes> converter) {
+            context.xmlGenerator().xmlWriter().value(val.asByteBuffer());


Is this base64 encoded?

Is it worth just moving that up to this level? We've got a ValueToString for SDK_BYTES that base64 encodes already.

Makes sense. I will use it

shorea · 2018-10-25T18:35:45Z

...main/java/software/amazon/awssdk/protocols/xml/internal/unmarshall/XmlOperationMetadata.java

+    }
+
+    /**
+     * This value indicates if the root element in the response xml document should be ignored


Do we need this or can we just use the PayloadTrait?

As we discussed yesterday, we are setting a customization to enable this value for operation like S3 GetBucketLocationOutput. So we need it

I was thinking that we would use that customization to just apply the payload trait to LocationConstraint. Wouldn't that have the same behavior?

shorea · 2018-10-25T18:36:41Z

services/rds/src/test/java/software/amazon/awssdk/services/rds/PresignRequestHandlerTest.java

@@ -55,7 +55,8 @@
    private static final Region DESTINATION_REGION = Region.of("us-west-2");

    private static RdsPresignInterceptor<CopyDbSnapshotRequest> presignInterceptor = new CopyDbSnapshotPresignInterceptor();
-    private final CopyDbSnapshotRequestMarshaller marshaller = new CopyDbSnapshotRequestMarshaller(new AwsQueryProtocolFactory());
+    private final CopyDbSnapshotRequestMarshaller marshaller =
+        new CopyDbSnapshotRequestMarshaller(AwsQueryProtocolFactory.builder().build());


Should we just have a create method?

We need to add overload create methods if we add state to this class, so i avoided it. We don't use it in many places, so it should be fine. We can always add it later.

Sounds good.

shorea · 2018-10-25T18:38:08Z

...rc/main/java/software/amazon/awssdk/services/s3/handlers/AddContentMd5HeaderInterceptor.java

+    @Override
+    public SdkHttpFullRequest modifyHttpRequest(Context.ModifyHttpRequest context, ExecutionAttributes executionAttributes) {
+        SdkHttpFullRequest request = context.httpRequest();
+        if (request.contentStreamProvider().isPresent() && !request.firstMatchingHeader(CONTENT_MD5).isPresent()) {


Does this add MD5 for all operations? what about PutObject?

Ignoring PutObject and UploadPart as discussed offline

…a5323a23 Pull request: release <- staging/194ffc2d-3f15-4f2c-95ce-60e7a5323a23

shorea reviewed Oct 15, 2018

View reviewed changes

varunnvs92 force-pushed the xmlRefactor branch from 940be6d to 44e23c7 Compare October 24, 2018 18:43

varunnvs92 changed the title ~~Rest Xml marshaller refactor~~ [Not ready for review] Rest Xml marshaller refactor Oct 24, 2018

varunnvs92 force-pushed the xmlRefactor branch 5 times, most recently from 5db4222 to fb81b6b Compare October 25, 2018 03:46

varunnvs92 changed the title ~~[Not ready for review] Rest Xml marshaller refactor~~ Rest Xml marshaller refactor Oct 25, 2018

varunnvs92 force-pushed the xmlRefactor branch from fb81b6b to 358df4d Compare October 25, 2018 03:51

Refact xml marshallers/unmarshallers

af5c829

varunnvs92 force-pushed the xmlRefactor branch from 358df4d to af5c829 Compare October 25, 2018 03:55

varunnvs92 changed the title ~~Rest Xml marshaller refactor~~ Rest Xml marshaller/unmarshaller refactor Oct 25, 2018

shorea reviewed Oct 25, 2018

View reviewed changes

shorea approved these changes Oct 25, 2018

View reviewed changes

Address review comments

297087e

varunnvs92 force-pushed the xmlRefactor branch from 491e2ca to 297087e Compare October 25, 2018 19:35

varunnvs92 merged commit 07b72ee into master Oct 25, 2018

zoewangg deleted the xmlRefactor branch November 29, 2018 19:31

aws-sdk-java-automation added a commit that referenced this pull request Mar 17, 2020

Merge pull request #758 from aws/staging/194ffc2d-3f15-4f2c-95ce-60e7…

be068f4

…a5323a23 Pull request: release <- staging/194ffc2d-3f15-4f2c-95ce-60e7a5323a23

Rest Xml marshaller/unmarshaller refactor #758

Rest Xml marshaller/unmarshaller refactor #758

Uh oh!

Conversation

varunnvs92 commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

varunnvs92 commented Oct 15, 2018 •

edited

Loading