Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move Aws specific responseHandler related classes to aws-core #484

Merged
merged 1 commit into from
May 16, 2018
Merged

Move Aws specific responseHandler related classes to aws-core #484

merged 1 commit into from
May 16, 2018

Conversation

zoewangg
Copy link
Contributor

Description

Part2 of #27

Moving xml protocol and AWS JSON protocol related classes(response handler, unmarshaller/marshaller) to aws-core.

Testing

mvn clean install

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
shorea reviewed May 15, 2018
View reviewed changes
...ore/src/main/java/software/amazon/awssdk/awscore/protocol/json/CompositeErrorCodeParser.java
@@ -13,7 +13,7 @@
* permissions and limitations under the License.
*/

package software.amazon.awssdk.core.internal.http;
package software.amazon.awssdk.awscore.protocol.json;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename CompositeErrorCodeParser and ErrorCodeParser to include Json in the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have JsonErrorCodeParser though.
ErrorCodeParser is an interface and JsonErrorCodeParser and IonErrorCodeParser, CompositeErrorCodeParser are the implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe StructuredErrorCodeParser if we are sticking with the naming elsewhere?

aws-core/src/main/java/software/amazon/awssdk/awscore/protocol/json/IonErrorCodeParser.java
@@ -13,7 +13,7 @@
* permissions and limitations under the License.
*/

package software.amazon.awssdk.core.internal.http;
package software.amazon.awssdk.awscore.protocol.json;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eww this ION parser is gross. Can we add a review before release to revisit ION?

...sources/software/amazon/awssdk/codegen/poet/transform/nestedcontainersrequestmarshaller.java Outdated
@@ -22,9 +22,9 @@
private static final OperationInfo SDK_OPERATION_BINDING = OperationInfo.builder().protocol(Protocol.REST_JSON)
.requestUri("/").httpMethodName(HttpMethodName.POST).hasExplicitPayloadMember(false).hasPayloadMembers(true).build();

private final SdkJsonProtocolFactory protocolFactory;
private final AwsJsonProtocolFactory protocolFactory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to use the Aws specific one? Does that mean we have to branch marshallers for APIG?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AwsJsonProtocolFactory can have different JSON marshallers(ION, JSON, CBOR) and SdkJsonProtocolFactory only supports Json now.

No, they are still sharing the same marshallers/unmarshallers defined in SdkStructuredPlainJsonFactory.

core/src/main/java/software/amazon/awssdk/core/http/HttpResponseHandler.java
@@ -31,7 +31,6 @@
@FunctionalInterface
public interface HttpResponseHandler<T> {

//TODO: move this to aws-core module once HttpResponse is replaced with SdkHttpResponse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, found out APIG Api returns the AWS request id header as well.

core/src/main/java/software/amazon/awssdk/core/protocol/StructuredJsonFactory.java Outdated
* permissions and limitations under the License.
*/

package software.amazon.awssdk.core.protocol;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be protocol.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, good catch

core/src/main/java/software/amazon/awssdk/core/protocol/json/JsonContentTypeResolver.java
/**
* Content type resolver implementation for Ion-enabled services.
*/
JsonContentTypeResolver ION_BINARY = new JsonContentTypeResolverImpl("application/x-amz-ion-");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For stuff like this I don't necessarily care where it lives. It can be in core if that means we can hide the impl from module boundaries. Don't care either way though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. kindof weird to make impl public.

core/src/main/java/software/amazon/awssdk/core/protocol/json/SdkStructuredCborFactory.java Outdated
private static final JsonFactory CBOR_FACTORY = new CBORFactory();
protected static final JsonFactory CBOR_FACTORY = new CBORFactory();

protected static final BiFunction<JsonFactory, String, StructuredJsonGenerator> GENERATOR_SUPPLIER =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be advantageous to have a subinterface of BiFunction that gives the params better names. Didn't immediately grok what String could be without looking at the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

@zoewangg zoewangg force-pushed the AwsSepecificResponseHandler branch from 347ae51 to 0925f4b Compare May 15, 2018 23:20
@zoewangg zoewangg force-pushed the AwsSepecificResponseHandler branch from 0925f4b to 838d098 Compare May 16, 2018 00:06
@zoewangg zoewangg merged commit d867703 into master May 16, 2018
@zoewangg zoewangg deleted the AwsSepecificResponseHandler branch May 16, 2018 19:49
@zoewangg zoewangg mentioned this pull request Jun 11, 2018
Closed
aws-sdk-java-automation added a commit that referenced this pull request Apr 29, 2019
@aws-sdk-java-automation
Merge pull request #484 from aws/staging/7f9c1113-3bb2-4faf-999d-9f68…
b56a336 
…498f53e0

Pull request: release <- staging/7f9c1113-3bb2-4faf-999d-9f68498f53e0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shorea shorea shorea approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@zoewangg @shorea