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

fix: Correct serde for errors in event streams #585

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

jbelkins
Copy link
Contributor

Issue #

awslabs/aws-sdk-swift#1100

Description of changes

Fixes serializers / deserializers for errors that are embedded in event streams, by accounting for the custom error properties being namespaced under properties.

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

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

Copy link
Contributor

@dayaffe dayaffe left a comment

Choose a reason for hiding this comment

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

LGTM once CI is passing

Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

Left a couple questions regarding the way "properties." namespace is added to deserializers of error structs.

@@ -232,9 +233,10 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
val httpBodyMembers = members.filter { it.isInHttpBody() }
generateCodingKeysForMembers(ctx, writer, httpBodyMembers)
writer.write("")
renderStructEncode(ctx, shape, mapOf(), httpBodyMembers, writer, defaultTimestampFormat)
val path = "properties.".takeIf { shape.hasTrait<ErrorTrait>() } ?: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This would make every error struct's members to be name-spaced under "properties" and not just errors embedded in event streams, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the properties namespace will be used for both.

@@ -154,7 +155,7 @@ abstract class MemberShapeDecodeGenerator(
*/
open fun renderAssigningDecodedMember(topLevelMember: MemberShape, decodedMemberName: String) {
val topLevelMemberName = ctx.symbolProvider.toMemberName(topLevelMember)
writer.write("\$L = \$L", topLevelMemberName, decodedMemberName)
writer.write("\$L\$L = \$L", path, topLevelMemberName, decodedMemberName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to other comment I left - should there be some condition that checks that error is embedded in event streams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed here, this code is not called and this decoder is not generated unless the error is a member of an event stream.

Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

Lgtm!

@jbelkins jbelkins merged commit d0269d2 into main Sep 13, 2023
7 checks passed
@jbelkins jbelkins deleted the jbe/event_stream_error_fix branch September 13, 2023 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants