-
Notifications
You must be signed in to change notification settings - Fork 28
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
fix: correctly serialize enum parameters in URIs #1094
Conversation
Affected ArtifactsNo artifacts changed size |
when (memberTarget) { | ||
is CollectionShape -> renderCollectionShape(it, memberTarget, writer) | ||
is TimestampShape -> { | ||
when { | ||
memberTarget is CollectionShape -> renderCollectionShape(it, memberTarget, writer) | ||
memberTarget is TimestampShape -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this change in formatting for readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it actually hampers readability somewhat. But when
with a subject (e.g., when (memberTarget)
) cannot be used to predicate anything but type checks (e.g., is CollectionShape
) or equality checks (e.g. ,== someOtherValue
). My new branch on L84 accesses an extension property (memberTarget.isEnum
) which isn't possible when using a when
with a subject. I'd've avoided removing the subject if I could've.
This might get better when Kotlin 2.1 adds support for when
-with-subject guards.
@@ -125,15 +126,15 @@ class HttpStringValuesMapSerializer( | |||
|
|||
private fun renderCollectionShape(binding: HttpBindingDescriptor, memberTarget: CollectionShape, writer: KotlinWriter) { | |||
val collectionMemberTarget = model.expectShape(memberTarget.member.target) | |||
val mapFnContents = when (collectionMemberTarget.type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Was something wrong with .type
?
@@ -49,7 +48,7 @@ internal class SmokeTestOperationSerializer: HttpSerializer.NonStreaming<SmokeTe | |||
builder.url { | |||
path.encodedSegments { | |||
add(PercentEncoding.Path.encode("smoketest")) | |||
"$label1".split("/").mapTo(this) { PercentEncoding.SmithyLabel.encode(it) } | |||
input.label1.split("/").mapTo(this) { PercentEncoding.SmithyLabel.encode(it) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this is so much easier to read now
Issue #
Addresses awslabs/aws-sdk-kotlin#1314
Description of changes
We are currently serializing enums parameters with their name as opposed to their value in URI paths, leading to invalid requests. This change fixes several small codegen bugs related to path and parameter serialization in HTTP bindings.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.