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.

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

refactor: xml content type header renderer to be overridable #977

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

0marperez
Copy link
Contributor

Issue #

Description of changes

-The part of the code that renders the xml content type header is made overridable

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@0marperez 0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Oct 11, 2023
@0marperez 0marperez marked this pull request as ready for review October 11, 2023 19:20
@0marperez 0marperez requested a review from a team as a code owner October 11, 2023 19:20
Comment on lines 269 to 273
renderContentTypeHeader(ctx, op, writer)
}

protected open fun renderContentTypeHeader(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) {
val resolver = getProtocolHttpBindingResolver(ctx.model, ctx.service)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Pass resolver through to renderContentTypeHeader.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't do that elsewhere FWIW (e.g.renderUri below)

Comment on lines 269 to 273
renderContentTypeHeader(ctx, op, writer)
}

protected open fun renderContentTypeHeader(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) {
val resolver = getProtocolHttpBindingResolver(ctx.model, ctx.service)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't do that elsewhere FWIW (e.g.renderUri below)

@0marperez 0marperez force-pushed the missing-content-type branch from b21fee1 to cae26c5 Compare November 10, 2023 09:25
*/
fun OperationShape.inputIsUnionShape(model: Model): Boolean {
val reqShape = model.expectShape<StructureShape>(input.get())
return reqShape.isUnionShape
Copy link
Contributor

Choose a reason for hiding this comment

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

fix: This is always false. (1) operation inputs MUST be a structure shape and (2) you've already pulled out a StructureShape so it can't be a UnionShape at this point.

I don't think you even need this function though as the other function introduced appears to do what you want/need.

@@ -193,6 +192,15 @@ val Shape.isSparse: Boolean
val Shape.isStreaming: Boolean
get() = hasTrait<StreamingTrait>()

/**
* Returns boolean indicating if operation input is union shaped
Copy link
Contributor

Choose a reason for hiding this comment

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

fix: It returns true if the operation input has an explicit HTTP payload member that is a union shape.

The input shape itself can never be a union in smithy.

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@0marperez 0marperez merged commit 7abd49a into main Nov 14, 2023
13 checks passed
@0marperez 0marperez deleted the missing-content-type branch November 14, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants