fix: route53 custom error unmarshalling #961
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "id": "7f80de87-a59a-420e-a175-c21a89f93fe9", | ||
| "type": "bugfix", | ||
| "description": "Correctly handle and throw `InvalidChangeBatch` responses from Route53", | ||
| "issues": [ | ||
| "awslabs/aws-sdk-kotlin#242" | ||
| ] | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package aws.sdk.kotlin.codegen.customization.route53 | ||
|
|
||
| import aws.sdk.kotlin.codegen.protocols.core.AwsHttpBindingProtocolGenerator | ||
| import aws.sdk.kotlin.codegen.sdkId | ||
| import software.amazon.smithy.kotlin.codegen.KotlinSettings | ||
| import software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration | ||
| import software.amazon.smithy.kotlin.codegen.integration.SectionWriterBinding | ||
| import software.amazon.smithy.kotlin.codegen.model.expectShape | ||
| import software.amazon.smithy.model.Model | ||
| import software.amazon.smithy.model.shapes.OperationShape | ||
| import software.amazon.smithy.model.shapes.ServiceShape | ||
| import software.amazon.smithy.model.transform.ModelTransformer | ||
|
|
||
| class ChangeResourceRecordSetsUnmarshallingIntegration : KotlinIntegration { | ||
| override val sectionWriters: List<SectionWriterBinding> = listOf( | ||
| SectionWriterBinding(AwsHttpBindingProtocolGenerator.ProtocolErrorDeserialization) { writer, originalValue -> | ||
| val lines = originalValue?.split("\n") | ||
| writer.write("#L\naws.sdk.kotlin.services.route53.internal.parseCustomXmlErrorResponse(payload) ?: #L", lines?.get(0) ?: "", lines?.get(1) ?: "") | ||
| }, | ||
| ) | ||
|
|
||
| override fun preprocessModel(model: Model, settings: KotlinSettings): Model { | ||
| val transformer = ModelTransformer.create() | ||
| return transformer.mapShapes(model) { shape -> | ||
| if (shape.id.name == "ChangeResourceRecordsSet" && shape is OperationShape) { | ||
| shape.errors.removeIf { error -> error.name == "InvalidChangeBatch" } | ||
| } | ||
| shape | ||
| } | ||
| } | ||
|
|
||
| override fun enabledForService(model: Model, settings: KotlinSettings) = | ||
| model.expectShape<ServiceShape>(settings.service).sdkId.equals("route 53", ignoreCase = true) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package aws.sdk.kotlin.codegen.customization.route53 | ||
|
|
||
| import org.junit.jupiter.api.Test | ||
| import software.amazon.smithy.kotlin.codegen.test.defaultSettings | ||
| import software.amazon.smithy.kotlin.codegen.test.prependNamespaceAndService | ||
| import software.amazon.smithy.kotlin.codegen.test.toSmithyModel | ||
| import software.amazon.smithy.model.Model | ||
| import kotlin.test.assertFalse | ||
| import kotlin.test.assertTrue | ||
|
|
||
| class ChangeResourceRecordSetsUnmarshallingIntegrationTest { | ||
| @Test | ||
| fun nonRoute53ModelIntegration() { | ||
| val model = sampleModel("not route 53") | ||
| val isEnabledForModel = ChangeResourceRecordSetsUnmarshallingIntegration().enabledForService(model, model.defaultSettings()) | ||
| assertFalse(isEnabledForModel) | ||
| } | ||
|
|
||
| @Test | ||
| fun route53ModelIntegration() { | ||
| val model = sampleModel("route 53") | ||
| val isEnabledForModel = ChangeResourceRecordSetsUnmarshallingIntegration().enabledForService(model, model.defaultSettings()) | ||
| assertTrue(isEnabledForModel) | ||
| } | ||
| } | ||
|
|
||
| private fun sampleModel(serviceName: String): Model = | ||
| """ | ||
| @http(method: "PUT", uri: "/foo") | ||
| operation Foo { } | ||
|
|
||
| @http(method: "POST", uri: "/bar") | ||
| operation Bar { } | ||
| """ | ||
| .prependNamespaceAndService(operations = listOf("Foo", "Bar"), serviceName = serviceName) | ||
| .toSmithyModel() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package aws.sdk.kotlin.services.route53.internal | ||
|
|
||
| import aws.smithy.kotlin.runtime.InternalApi | ||
| import aws.smithy.kotlin.runtime.awsprotocol.ErrorDetails | ||
| import aws.smithy.kotlin.runtime.serde.* | ||
| import aws.smithy.kotlin.runtime.serde.xml.XmlDeserializer | ||
| import aws.smithy.kotlin.runtime.serde.xml.XmlSerialName | ||
|
|
||
| @InternalApi | ||
| public suspend fun parseCustomXmlErrorResponse(payload: ByteArray): ErrorDetails? { | ||
0marperez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| val details = InvalidChangeBatchDeserializer.deserialize(XmlDeserializer(payload, true)) | ||
| ?: InvalidChangeBatchMessageDeserializer.deserialize(XmlDeserializer(payload, true)) | ||
| ?: return null | ||
| return ErrorDetails(details.code, details.message, details.requestId) | ||
| } | ||
|
|
||
| internal object InvalidChangeBatchDeserializer { | ||
|
There was a problem hiding this comment. Nit: Couldn't this and basically everything else in this file be |
||
| private val MESSAGES_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Struct, XmlSerialName("Messages")) | ||
| private val REQUESTID_DESCRIPTOR = SdkFieldDescriptor(SerialKind.String, XmlSerialName("RequestId")) | ||
| private val OBJ_DESCRIPTOR = SdkObjectDescriptor.build { | ||
| trait(XmlSerialName("InvalidChangeBatch")) | ||
| field(MESSAGES_DESCRIPTOR) | ||
| field(REQUESTID_DESCRIPTOR) | ||
| } | ||
|
|
||
| suspend fun deserialize(deserializer: Deserializer): XmlErrorResponse? { | ||
| var requestId: String? = null | ||
| var messages: XmlError? = null | ||
|
|
||
| return try { | ||
| deserializer.deserializeStruct(OBJ_DESCRIPTOR) { | ||
| loop@ while (true) { | ||
| when (findNextFieldIndex()) { | ||
| MESSAGES_DESCRIPTOR.index -> messages = InvalidChangeBatchMessageDeserializer.deserialize(deserializer) | ||
| REQUESTID_DESCRIPTOR.index -> requestId = deserializeString() | ||
| null -> break@loop | ||
| else -> skipValue() | ||
| } | ||
| } | ||
| } | ||
| XmlErrorResponse(messages, requestId ?: messages?.requestId) | ||
| } catch (e: DeserializationException) { | ||
| null // return so an appropriate exception type can be instantiated above here. | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal object InvalidChangeBatchMessageDeserializer { | ||
| private val MESSAGE_DESCRIPTOR = SdkFieldDescriptor(SerialKind.String, XmlSerialName("Message")) | ||
| private val CODE_DESCRIPTOR = SdkFieldDescriptor(SerialKind.String, XmlSerialName("Code")) | ||
| private val REQUESTID_DESCRIPTOR = SdkFieldDescriptor(SerialKind.String, XmlSerialName("RequestId")) | ||
| private val OBJ_DESCRIPTOR = SdkObjectDescriptor.build { | ||
| trait(XmlSerialName("Messages")) | ||
| field(MESSAGE_DESCRIPTOR) | ||
| field(CODE_DESCRIPTOR) | ||
| field(REQUESTID_DESCRIPTOR) | ||
| } | ||
|
|
||
| suspend fun deserialize(deserializer: Deserializer): XmlError? { | ||
| var message: String? = null | ||
| var code: String? = null | ||
| var requestId: String? = null | ||
|
|
||
| return try { | ||
| deserializer.deserializeStruct(OBJ_DESCRIPTOR) { | ||
| loop@ while (true) { | ||
| when (findNextFieldIndex()) { | ||
| MESSAGE_DESCRIPTOR.index -> message = deserializeString() | ||
| CODE_DESCRIPTOR.index -> code = deserializeString() | ||
| REQUESTID_DESCRIPTOR.index -> requestId = deserializeString() | ||
| null -> break@loop | ||
| else -> skipValue() | ||
| } | ||
| } | ||
| } | ||
| XmlError(requestId, code, message) | ||
| } catch (e: DeserializationException) { | ||
| null // return so an appropriate exception type can be instantiated above here. | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // XML Error response parser from RestXMLErrorDeserializer | ||
| internal interface RestXmlErrorDetails { | ||
| val requestId: String? | ||
| val code: String? | ||
| val message: String? | ||
| } | ||
|
|
||
| // Models "ErrorResponse" type in https://awslabs.github.io/smithy/1.0/spec/aws/aws-restxml-protocol.html#operation-error-serialization | ||
| internal data class XmlErrorResponse( | ||
| val error: XmlError?, | ||
| override val requestId: String? = error?.requestId, | ||
| ) : RestXmlErrorDetails { | ||
| override val code: String? = error?.code | ||
| override val message: String? = error?.message | ||
| } | ||
|
|
||
| // Models "Error" type in https://awslabs.github.io/smithy/1.0/spec/aws/aws-restxml-protocol.html#operation-error-serialization | ||
| internal data class XmlError( | ||
| override val requestId: String?, | ||
| override val code: String?, | ||
| override val message: String?, | ||
| ) : RestXmlErrorDetails | ||
|
There was a problem hiding this comment. Question: Do we need these classes? Couldn't the deserializers return There was a problem hiding this comment. One of the deserializers can, the other can't. I can have the one that can return There was a problem hiding this comment. Doing that resulted in some weird errors with other parts of the code, I think I'll just leave it as is |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package aws.sdk.kotlin.services.route53.internal | ||
|
|
||
| import aws.sdk.kotlin.services.route53.model.Route53Exception | ||
| import aws.sdk.kotlin.services.route53.transform.ChangeResourceRecordSetsOperationDeserializer | ||
| import aws.smithy.kotlin.runtime.http.Headers | ||
| import aws.smithy.kotlin.runtime.http.HttpBody | ||
| import aws.smithy.kotlin.runtime.http.HttpStatusCode | ||
| import aws.smithy.kotlin.runtime.http.response.HttpResponse | ||
| import aws.smithy.kotlin.runtime.operation.ExecutionContext | ||
| import kotlinx.coroutines.runBlocking | ||
| import org.junit.jupiter.api.Test | ||
| import org.junit.jupiter.api.assertThrows | ||
| import kotlin.test.assertEquals | ||
|
|
||
| class ChangeResourceRecordSetsUnmarshallingTest { | ||
| @Test | ||
| fun unmarshallChangeResourceRecordSetsInvaildChangeBatchResponse() { | ||
| val bodyText = """ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <InvalidChangeBatch xmlns="https://route53.amazonaws.com/doc/2013-04-01/"> | ||
| <Messages> | ||
| <Message>This is a ChangeResourceRecordSets InvaildChangeBatchResponse message</Message> | ||
| </Messages> | ||
| <RequestId>b25f48e8-84fd-11e6-80d9-574e0c4664cb</RequestId> | ||
| </InvalidChangeBatch> | ||
| """.trimIndent() | ||
|
|
||
| val response: HttpResponse = HttpResponse( | ||
| HttpStatusCode(400, "Bad Request"), | ||
| Headers.invoke { }, | ||
| HttpBody.fromBytes(bodyText.encodeToByteArray()), | ||
| ) | ||
|
|
||
| val exception = assertThrows<Route53Exception> { | ||
| runBlocking { | ||
| ChangeResourceRecordSetsOperationDeserializer().deserialize(ExecutionContext(), response) | ||
| } | ||
| } | ||
| assertEquals("This is a ChangeResourceRecordSets InvaildChangeBatchResponse message", exception.message) | ||
| } | ||
|
|
||
| @Test | ||
| fun unmarshallChangeResourceRecordSetsGenericErrorResponse() { | ||
| val bodyText = """ | ||
| <?xml version="1.0"?> | ||
| <ErrorResponse xmlns="http://route53.amazonaws.com/doc/2016-09-07/"> | ||
| <Error> | ||
| <Type>Sender</Type> | ||
| <Code>MalformedXML</Code> | ||
| <Message>This is a ChangeResourceRecordSets generic error message</Message> | ||
| </Error> | ||
| <RequestId>b25f48e8-84fd-11e6-80d9-574e0c4664cb</RequestId> | ||
| </ErrorResponse> | ||
| """.trimIndent() | ||
|
|
||
| val response: HttpResponse = HttpResponse( | ||
| HttpStatusCode(400, "Bad Request"), | ||
| Headers.invoke { }, | ||
| HttpBody.fromBytes(bodyText.encodeToByteArray()), | ||
| ) | ||
|
|
||
| val exception = assertThrows<Route53Exception> { | ||
| runBlocking { | ||
| ChangeResourceRecordSetsOperationDeserializer().deserialize(ExecutionContext(), response) | ||
| } | ||
| } | ||
| assertEquals("This is a ChangeResourceRecordSets generic error message", exception.message) | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Nit: Solely referring to the original value with this lines list is pretty inscrutable. I suggest giving each line a clearer name so that it's easier to understand what's going on here. We can also clean up the null checking by just requiring a non-null value up front:
requireNotNull(originalValue) { "Unexpectedly empty original value" } val (nullCheckLine, parseLine) = originalValue.split("\n", limit = 2) writer.write("#L", nullCheckLine) writer.write("aws.sdk.kotlin.services.route53.internal.parseCustomXmlErrorResponse(payload) ?: #L", parseLine)