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

feat(smithy): awsQuery protocol #2807

Merged
merged 12 commits into from
May 1, 2023
Merged

feat(smithy): awsQuery protocol #2807

merged 12 commits into from
May 1, 2023

Conversation

dnys1
Copy link
Contributor

@dnys1 dnys1 commented Apr 1, 2023

Adds support for the awsQuery protocol which includes services such as CloudFormation, IAM, and STS.

@dnys1 dnys1 changed the title feat(smithy): awsQuery protocol support feat(smithy): awsQuery protocol Apr 1, 2023
@dnys1 dnys1 force-pushed the feat/smithy/aws-query branch 2 times, most recently from d642a00 to 422cee1 Compare April 3, 2023 16:53
@dnys1 dnys1 marked this pull request as ready for review April 3, 2023 16:56
@dnys1 dnys1 requested a review from a team as a code owner April 3, 2023 16:56
Base automatically changed from chore/repo/mar-deps to next April 11, 2023 20:44
dnys1 and others added 12 commits April 11, 2023 13:45
Previously, structure members were generated in alphabetical order, but this causes issues for some protocols like `awsQuery` that require members to be serialized in a specific order. This commit changes the codegen to generate members in the order they are defined in the Smithy model.
Updates service client generation for `awsQuery` which does not explicitly set an `@httpTrait`.
Tests the `awsQuery` protocol via the STS service.
Blindly appending `Operation` to the end of an operation's name can create conflicts, like with CloudFormation where both `DescribeStacks` and `DescribeStacksOperation` are valid operations. This change makes it so that the operation name is only appended if doing so wouldn't create a conflict.
Comment on lines 76 to 88
var isUnderspecified =
specifiedType.isUnspecified || specifiedType.parameters.isEmpty;

var keyType = specifiedType.parameters.isEmpty
? FullType.unspecified
: specifiedType.parameters[0];
var valueType = FullType(BuiltList, [
specifiedType.parameters.isEmpty
? FullType.unspecified
: specifiedType.parameters[1]
]);

var result = isUnderspecified
Copy link
Member

Choose a reason for hiding this comment

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

These can all be final, no?

Copy link
Member

Choose a reason for hiding this comment

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

Should we update this lib to use library.yaml instead of library_core.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.. eventually. It's a bigger task. I was optimizing for speed and correctness, not cleanliness for this package.

@@ -0,0 +1,61 @@
$version: "2.0"
Copy link
Member

Choose a reason for hiding this comment

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

This isn't generated, is it? Should we update .gitattributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not technically generated, but it's pulled from https://github.com/awslabs/smithy. I'll see if there's a better qualifier for gitattributes.

@@ -20,6 +20,7 @@ const skipProtocols = [
const awsProtocols = [
'awsJson1_0',
'awsJson1_1',
'awsQuery',
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment as above. This is not generated, correct? .gitattributes has everything in goldens marked as generated.

.result;

// Get credentials for the user
final credentials = await client
Copy link
Member

Choose a reason for hiding this comment

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

nit: Would name this result or response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. These tests were all mostly Copilot generated 😆

@dnys1 dnys1 merged commit 7197b3d into main May 1, 2023
@dnys1 dnys1 deleted the feat/smithy/aws-query branch May 1, 2023 18:49
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