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

Add EC2 Query protocol support #475

Merged
merged 3 commits into from
Jun 10, 2021
Merged

Add EC2 Query protocol support #475

merged 3 commits into from
Jun 10, 2021

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Jun 9, 2021

Fixes #221.

This PR copies most of the AWS Query support, and then makes the following tweaks to bring it in line with EC2 Query:

  • Adds support for the @ec2QueryName trait and its specified name fallback chain: @ec2QueryName -> @xmlName capitalized -> "key"/"value" (for map key/values only) -> member name capitalized
  • Flattens all lists and maps
  • Doesn't expect a nested <Result> tag inside of responses

Also forked the RestXml inline wrapped error parser since EC2 query wraps the <Error> tag in an <Errors> tag.

I think there's a discussion to be had about whether we want to attempt to share the code between AwsQueryParserGenerator and Ec2QueryParserGenerator given the small diff between the two:

-class AwsQuerySerializerGenerator(protocolConfig: ProtocolConfig) : StructuredDataSerializerGenerator {
+class Ec2QuerySerializerGenerator(protocolConfig: ProtocolConfig) : StructuredDataSerializerGenerator {
     private data class Context<T : Shape>(
         /** Expression that yields a QueryValueWriter */
         val writerExpression: String,
         /** Expression representing the value to write to the QueryValueWriter */
         val valueExpression: ValueExpression,
@@ -219,28 +220,33 @@

     private fun determineTimestampFormat(shape: MemberShape): TimestampFormatTrait.Format =
         shape.getMemberTrait(model, TimestampFormatTrait::class.java).orNull()?.format
             ?: TimestampFormatTrait.Format.DATE_TIME

+    private fun MemberShape.ec2QueryKeyName(prioritizedFallback: String? = null): String =
+        getTrait<Ec2QueryNameTrait>()?.value
+            ?: getTrait<XmlNameTrait>()?.value?.let { StringUtils.capitalize(it) }
+            ?: prioritizedFallback
+            ?: StringUtils.capitalize(memberName)
+
     private fun RustWriter.structWriter(context: MemberContext, inner: RustWriter.(String) -> Unit) {
-        val prefix = context.shape.getTrait<XmlNameTrait>()?.value ?: context.shape.memberName
+        val prefix = context.shape.ec2QueryKeyName()
         safeName("scope").also { scopeName ->
             Attribute.AllowUnusedMut.render(this)
             rust("let mut $scopeName = ${context.writerExpression}.prefix(${prefix.dq()});")
             inner(scopeName)
         }
     }

     private fun RustWriter.serializeCollection(memberContext: MemberContext, context: Context<CollectionShape>) {
-        val flat = memberContext.shape.getTrait<XmlFlattenedTrait>() != null
         val memberOverride = when (val override = context.shape.member.getTrait<XmlNameTrait>()?.value) {
             null -> "None"
             else -> "Some(${override.dq()})"
         }
         val itemName = safeName("item")
         safeName("list").also { listName ->
-            rust("let mut $listName = ${context.writerExpression}.start_list($flat, $memberOverride);")
+            rust("let mut $listName = ${context.writerExpression}.start_list(true, $memberOverride);")
             rustBlock("for $itemName in ${context.valueExpression.asRef()}") {
                 val entryName = safeName("entry")
                 Attribute.AllowUnusedMut.render(this)
                 rust("let mut $entryName = $listName.entry();")
                 val targetShape = model.expectShape(context.shape.member.target)
@@ -252,17 +258,16 @@
             rust("$listName.finish();")
         }
     }

     private fun RustWriter.serializeMap(memberContext: MemberContext, context: Context<MapShape>) {
-        val flat = memberContext.shape.getTrait<XmlFlattenedTrait>() != null
-        val entryKeyName = (context.shape.key.getTrait<XmlNameTrait>()?.value ?: "key").dq()
-        val entryValueName = (context.shape.value.getTrait<XmlNameTrait>()?.value ?: "value").dq()
+        val entryKeyName = context.shape.key.ec2QueryKeyName("key").dq()
+        val entryValueName = context.shape.value.ec2QueryKeyName("value").dq()
         safeName("map").also { mapName ->
             val keyName = safeName("key")
             val valueName = safeName("value")
-            rust("let mut $mapName = ${context.writerExpression}.start_map($flat, $entryKeyName, $entryValueName);")
+            rust("let mut $mapName = ${context.writerExpression}.start_map(true, $entryKeyName, $entryValueName);")
             rustBlock("for ($keyName, $valueName) in ${context.valueExpression.asRef()}") {
                 val keyTarget = model.expectShape(context.shape.key.target)
                 val keyExpression = when (keyTarget.hasTrait<EnumTrait>()) {
                     true -> "$keyName.as_str()"
                     else -> keyName

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jdisanti
Copy link
Collaborator Author

jdisanti commented Jun 9, 2021

While this passes the protocol tests, I'm not sure if it's actually correct given some ambiguity in the spec. The spec calls for unflattened collections by default, but all of the protocol tests have flattened collections without using @xmlFlattened, and the tests even comment that all lists are flattened in EC2 Query.

Likewise for maps, I need to figure out if the "key"/"value" name overriding is actually needed since they are flattened. Will dig into this more.

@jdisanti jdisanti requested a review from rcoh June 9, 2021 00:57
Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM. I'm fine with keeping them separate but we need to be extremely careful to fix bugs in both places. Would also be fine to make a QueryProtocolSerializer abstract class that exposed abstract functions for the things that needed to vary

@jdisanti
Copy link
Collaborator Author

jdisanti commented Jun 9, 2021

Opened a PR on Smithy to make the spec doc consistent with the protocol tests: smithy-lang/smithy#823

@jdisanti jdisanti merged commit 1469090 into smithy-lang:main Jun 10, 2021
@jdisanti jdisanti deleted the ec2-query branch June 11, 2021 20:38
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.

EC2 query protocol
2 participants