Support customization of contentTypeOverride for Header ContentType #2519
Conversation
| @@ -242,6 +242,18 @@ public final SubclassT protocolVersion(String protocolVersion) { | |||
| return getSubclass(); | |||
| } | |||
|
|
|||
| /** | |||
| * ContentType of the client (By default it is application/x-amz-json-<JSON version>). | |||
There was a problem hiding this comment.
This default will change soon in the future (like, the next few months - there's a SEP on the way), and we'll probably forget to update this comment. Should we link to where the default is defined instead?
There was a problem hiding this comment.
Added link to code where we set the default value AWS_JSON = new DefaultJsonContentTypeResolver("application/x-amz-json-");
| return protocolMetadata.contentTypeOverride() != null ? protocolMetadata.contentTypeOverride() | ||
| : prefix + protocolMetadata.protocolVersion(); |
There was a problem hiding this comment.
I don't like "overrides" being bound so late. It means we have to pipe a lot information through from the code generator, which adds a lot more cross-module API surface area. Can we just have this line be return protocolMetadata.contentType(); and have the code generator define the content-type that's used?
We can have the default value be "prefix + protocolMetadata.protocolVersion()", but do that upstream in the protocol factory. I'd honestly prefer it to be defaulted in the code generator, but that would break people who just upgrade the JSON protocol module without updating their service clients (which we try not to do outside of minor version bumps).
There was a problem hiding this comment.
Moved the logic where the check for customized ContentType is present is moved to protocol factory.
| MetadataConfig customServiceMetadata = model.getCustomizationConfig().getCustomServiceMetadata(); | ||
| // Preference is given to CustomizationConfig | ||
| if (customServiceMetadata != null && customServiceMetadata.getContentType() != null) { | ||
| methodSpec.addCode(".contentTypeOverride($S)", customServiceMetadata.getContentType()); | ||
| } else if (metadata.getContentType() != null) { | ||
| methodSpec.addCode(".contentTypeOverride($S)", metadata.getContentType()); | ||
| } |
There was a problem hiding this comment.
Not a blocker, but this is easier for me to read:
String contentType = Optional.ofNullable(model.getCustomizationConfig().getCustomServiceMetadata())
.map(MetadataConfig::getContentType)
.orElse(metadata.getContentType()));
methodSpec.addCode(".contentTypeOverride($S)", contentType);
| public final SubclassT contentTypeOverride(String contentTypeOverride) { | ||
| protocolMetadata.contentTypeOverride(contentTypeOverride); | ||
| return getSubclass(); | ||
| } |
There was a problem hiding this comment.
I think in the future we will move contentType to be always controlled from the codegen, because we'll need to treat each protocol differently. Should we just name this "contentType" since in the future it won't really be an "override"?
There was a problem hiding this comment.
Changed all occurrences of contentTypeOverride to contentType.
|
Kudos, SonarCloud Quality Gate passed! |
Description
Motivation and Context
application/x-amz-json-1.1for json protocol.application/jsonContentType Header.application/jsonContentType header.Why is this change required? What problem does it solve?
application/jsonContentType header. Else the API calls to Services which acceptapplication/jsonContentType header will not be successful.If it fixes an open issue, please link to the issue here : PR #2510
File Modification
JsonProtocolSpec.java :
contentType attribute is populated only if CustomizationConfig is set. If not then we look for metadata.getContentType() else we donot add any ContentType, in this case it uses from DefaultJsonContentTypeResolver.
AwsJsonProtocolMetadata
Added contentType which is used while we resolve the ContentType.
BaseAwsJsonProtocolFactory
The Factory method that creates Builders for AwsJsonProtocolFactory. ContentType is added in the attributes which will be set by codegen. Here we select from ContentType if the ContentType is set else we select the default one as we used to do before.
services/wellarchitected/src/main/resources/codegen-resources/customization.config
The Config file to set wellarchitected services ContentType as application/json
codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/test-customservicemetadata-sync.java
codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/test-customservicemetadata-async.java
Expected generated files when ContentType Customization is set.
Rest of the changes are for testing.
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense