fix: route53 custom error unmarshalling #961

0marperez · 2023-06-23T19:09:49Z

Issue #

Github issue #242

Description of changes

Implements route 53 custom error unmarshalling for InvaildChangeBatchResponse. Tests included.

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

...ices/route53/common/src/aws/sdk/kotlin/services/route53/internal/CustomErrorUnmarshalling.kt

...route53/common/test/aws/sdk/kotlin/services/route53/internal/CustomErrorUnmarshallingTest.kt

.changes/7f80de87-a59a-420e-a175-c21a89f93fe9.json

...n/kotlin/aws/sdk/kotlin/codegen/customization/route53/CustomErrorUnmarshallingIntegration.kt

...ices/route53/common/src/aws/sdk/kotlin/services/route53/internal/CustomErrorUnmarshalling.kt

...n/kotlin/aws/sdk/kotlin/codegen/customization/route53/CustomErrorUnmarshallingIntegration.kt

… route53-custom-error-unmarshalling

ianbotsf

Looks pretty good. A few minor points left...fix and ship!

ianbotsf · 2023-06-28T19:56:56Z

...common/src/aws/sdk/kotlin/services/route53/internal/ChangeResourceRecordSetsUnmarshalling.kt

+    return ErrorDetails(details.code, details.message, details.requestId)
+}
+
+internal object InvalidChangeBatchDeserializer {


Nit: Couldn't this and basically everything else in this file be private (except for parseCustomXmlErrorResponse)?

ianbotsf · 2023-06-28T20:00:52Z

...common/src/aws/sdk/kotlin/services/route53/internal/ChangeResourceRecordSetsUnmarshalling.kt

+// 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


Question: Do we need these classes? Couldn't the deserializers return ErrorDetails? directly?

One of the deserializers can, the other can't. I can have the one that can return ErrorDetails? do that but I would have to leave the other data class in there for the one that can't.

Doing that resulted in some weird errors with other parts of the code, I think I'll just leave it as is

ianbotsf · 2023-06-28T20:08:45Z

...sdk/kotlin/codegen/customization/route53/ChangeResourceRecordSetsUnmarshallingIntegration.kt

+            val lines = originalValue?.split("\n")
+            writer.write("#L\naws.sdk.kotlin.services.route53.internal.parseCustomXmlErrorResponse(payload) ?: #L", lines?.get(0) ?: "", lines?.get(1) ?: "")


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)

lauzadis

Great! No comments outside of what Ian already had.

...sdk/kotlin/codegen/customization/route53/ChangeResourceRecordSetsUnmarshallingIntegration.kt

… route53-custom-error-unmarshalling

This reverts commit 24e7975.

This reverts commit 9cb4e0c.

… route53-custom-error-unmarshalling

...common/src/aws/sdk/kotlin/services/route53/internal/ChangeResourceRecordSetsUnmarshalling.kt

aajtodd · 2023-06-29T16:31:07Z

...common/src/aws/sdk/kotlin/services/route53/internal/ChangeResourceRecordSetsUnmarshalling.kt

+}
+
+// XML Error response parser from RestXMLErrorDeserializer
+private interface RestXmlErrorDetails {


question: what purpose does this serve?

It was there from the copy and pasting I did and was necessary for that code to work. I modified things and it's no longer there anymore

...sdk/kotlin/codegen/customization/route53/ChangeResourceRecordSetsUnmarshallingIntegration.kt

aajtodd · 2023-06-30T18:48:57Z

...common/src/aws/sdk/kotlin/services/route53/internal/ChangeResourceRecordSetsUnmarshalling.kt

+    }
+}
+
+private object InvalidChangeBatchMessageDeserializer {


question: This is a nested type right? Can we not just re-use the generated deserializer for this then?

I modified things and it's not exactly the same so no

Why is the generated deserializer not usable as is?

Because of this line I believe. It should be Messages instead of Error. But I could be wrong

I see now it's not a nested type it's just a list of string. Ok I have further questions now, new comments incoming.

aajtodd · 2023-07-06T18:01:41Z

...common/src/aws/sdk/kotlin/services/route53/internal/ChangeResourceRecordSetsUnmarshalling.kt

+                loop@ while (true) {
+                    when (findNextFieldIndex()) {
+                        MESSAGE_DESCRIPTOR.index ->
+                            if (messages == null) messages = deserializeString() else messages += "\n" + deserializeString()


correctness: This is modeled as a list field why are we joining it to a string with \n?

I see how it looks like the Messages field here because of the variable but it's Message which is a String. I am assuming a response that looks like this here:

<?xml version="1.0" encoding="UTF-8"?> <InvalidChangeBatch xmlns="https://route53.amazonaws.com/doc/2013-04-01/"> <Messages> <Message>Tried to create resource record set duplicate.example.com. type A, but it already exists</Message> <Message>Tried to create resource record set duplicate.example.com. type B, but it already exists</Message> </Messages> <RequestId>b25f48e8-84fd-11e6-80d9-574e0c4664cb</RequestId> </InvalidChangeBatch>

Is this even right/possible?

aajtodd · 2023-07-06T18:05:07Z

...common/src/aws/sdk/kotlin/services/route53/internal/ChangeResourceRecordSetsUnmarshalling.kt

+private fun buildInvalidChangeBatchException(messages: String?): InvalidChangeBatch {
+    messages ?: throw DeserializationException("Missing message in InvalidChangeBatch XML response")
+    val builder = InvalidChangeBatch.Builder()
+    builder.message = messages


correctness: Why are we setting message to the result of parsing the list field? Also what if it populates both Message and Messages?

e.g.

<?xml version="1.0" encoding="UTF-8"?> <InvalidChangeBatch xmlns="https://route53.amazonaws.com/doc/2013-04-01/"> <Messages> <Message>Message from list</Message> </Messages> <Message>Some other message</Message> <RequestId>b25f48e8-84fd-11e6-80d9-574e0c4664cb</RequestId> </InvalidChangeBatch>

Why are we setting message to the result of parsing the list field? :
I'm setting the Message value here which is a String not a String list. That variable name is pretty confusing and makes it seem like the Messages field. I'll change it

What if it populates both Message and Messages? :
So I don't populate Messages right now I only extract the Message. I also asked Monday if I had to worry about seeing a Message not inside a Messages tag and got told that it shouldn't happen. So that's something we have to look out for as well?

… route53-custom-error-unmarshalling

sonarcloud · 2023-07-07T01:57:52Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
17.5% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

aajtodd · 2023-07-07T13:48:41Z

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/core/AwsHttpBindingProtocolGenerator.kt

@@ -108,6 +111,7 @@ abstract class AwsHttpBindingProtocolGenerator : HttpBindingProtocolGenerator()
        writer.write("val payload = response.body.#T()", RuntimeTypes.Http.readAll)
            .write("val wrappedResponse = response.#T(payload)", RuntimeTypes.AwsProtocolCore.withPayload)
            .write("")
+            .declareSection(ProtocolErrorDeserialization) { }


nit: you don't need the empty { } it's already the default

0marperez added 4 commits June 21, 2023 16:40

Added custom unmarshaller & wrote one test

6de9a9a

Fixed unmarshalling & added tests

68d1cf4

Run ktlint

6983ee2

Added change log

a5545c9

0marperez added the customization Service Customization label Jun 23, 2023

0marperez requested a review from a team as a code owner June 23, 2023 19:09

ianbotsf reviewed Jun 23, 2023

View reviewed changes

0marperez changed the title ~~feat: route53 custom error unmarshalling~~ fix: route53 custom error unmarshalling Jun 23, 2023

lauzadis reviewed Jun 23, 2023

View reviewed changes

...n/kotlin/aws/sdk/kotlin/codegen/customization/route53/CustomErrorUnmarshallingIntegration.kt Outdated Show resolved Hide resolved

0marperez added 5 commits June 27, 2023 14:22

Renamed files and modified change log

87a497d

Removed code duplication in favor of hooks and preprocessing

78a50ee

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

4283403

… route53-custom-error-unmarshalling

Bug fixes, ktlint & integration tests

dd81188

Added copyright header missing

f3ee41e

ianbotsf approved these changes Jun 28, 2023

View reviewed changes

lauzadis approved these changes Jun 28, 2023

View reviewed changes

Polishing: visibility modifiers and legibility

bc422b3

aajtodd requested changes Jun 29, 2023

View reviewed changes

...sdk/kotlin/codegen/customization/route53/ChangeResourceRecordSetsUnmarshallingIntegration.kt Outdated Show resolved Hide resolved

...sdk/kotlin/codegen/customization/route53/ChangeResourceRecordSetsUnmarshallingIntegration.kt Outdated Show resolved Hide resolved

0marperez added 10 commits June 29, 2023 09:51

Made changes to integration

24e7975

Got rid of unnecessary data class and interface

9cb4e0c

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

dc8ab39

… route53-custom-error-unmarshalling

Revert "Made changes to integration"

648d6ca

This reverts commit 24e7975.

Revert "Got rid of unnecessary data class and interface"

f19a940

This reverts commit 9cb4e0c.

fix section writer assumption

c79e34e

Added back invalid change batch deserializer

5ec392a

Renamed unused parameter

f381c29

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

c3be1e9

… route53-custom-error-unmarshalling

Changed Route53Exception to InvalidChangeBatch type

6fc5cf4

ianbotsf approved these changes Jun 30, 2023

View reviewed changes

0marperez requested a review from aajtodd June 30, 2023 17:43

aajtodd requested changes Jun 30, 2023

View reviewed changes

0marperez added 2 commits July 3, 2023 14:36

Added multiple message support and some clean up

9fd40be

Changed separator for multiple messages

b7d2597

aajtodd reviewed Jul 6, 2023

View reviewed changes

0marperez added 2 commits July 6, 2023 21:55

Refactoring and correctnes addressed based on PR feedback

678c896

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

212fb85

… route53-custom-error-unmarshalling

aajtodd approved these changes Jul 7, 2023

View reviewed changes

nit fix

a21df0b

0marperez merged commit 74fb92d into main Jul 7, 2023
9 of 10 checks passed

0marperez deleted the route53-custom-error-unmarshalling branch July 7, 2023 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: route53 custom error unmarshalling #961

fix: route53 custom error unmarshalling #961

0marperez commented Jun 23, 2023

ianbotsf left a comment

ianbotsf Jun 28, 2023

ianbotsf Jun 28, 2023

0marperez Jun 29, 2023

0marperez Jun 29, 2023

ianbotsf Jun 28, 2023

lauzadis left a comment

aajtodd Jun 29, 2023

0marperez Jul 6, 2023

aajtodd Jun 30, 2023

0marperez Jul 6, 2023

aajtodd Jul 6, 2023

0marperez Jul 6, 2023

aajtodd Jul 6, 2023

aajtodd Jul 6, 2023

0marperez Jul 6, 2023

aajtodd Jul 6, 2023

0marperez Jul 6, 2023

sonarcloud bot commented Jul 7, 2023

aajtodd Jul 7, 2023

		val lines = originalValue?.split("\n")
		writer.write("#L\naws.sdk.kotlin.services.route53.internal.parseCustomXmlErrorResponse(payload) ?: #L", lines?.get(0) ?: "", lines?.get(1) ?: "")

fix: route53 custom error unmarshalling #961

fix: route53 custom error unmarshalling #961

Conversation

0marperez commented Jun 23, 2023

Issue #

Description of changes

ianbotsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauzadis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 7, 2023

Choose a reason for hiding this comment