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 support for operationContextParams Endpoints trait #3755

Merged
merged 5 commits into from
Jul 13, 2024

Conversation

landonxjames
Copy link
Contributor

Motivation and Context

We have to support the new operationContextParams trait for endpoint resolution. This trait specifies JMESPath expressions for selecting parameter data from the operation's input type.

Description

  • Add codegen support for the JMESPath keys function (required by the trait spec)
  • Add codegen support for the trait itself. This is achieved by generating get_param_name functions for each param specified in operationContextParams. These functions pull the data out of the input object and it is added to the endpoint params in the ${operationName}EndpointParamsInterceptor

Testing

Updated the existing test suite for JMESPath codegen to test the keys function. Updated the existing EndpointsDecoratorTest with an operationContextParams trait specifying one param of each supported type to test the codegen.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

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

@landonxjames landonxjames requested review from a team as code owners July 11, 2024 21:33
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -194,6 +216,8 @@ class EndpointsDecoratorTest {
.map(ToString::to_string)
.collect::<Vec<_>>()
)
.jmes_path_param_string_array(vec!["key2".to_string(), "key1".to_string()])
Copy link
Contributor

Choose a reason for hiding this comment

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

question: are these jmes_path_param_* functions internal or public?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably pub since they're methods on service-specific endpoint params builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, they are pub since they are part of the param builder. Normally they won't have the jmes_path_param_ prefix, thats just because I named them like that in the model:

@operationContextParams(
            JmesPathParamString: {
                path: "nested.field",
            }
            JmesPathParamBoolean: {
                path: "nested.boolField",
            }
            JmesPathParamStringArray: {
                path: "keys(nested.mapField)",
            }
        )

We could make them private with the assumption that these should only be set by the generated JMESPath getters, but we allow overriding other values set in the model (mostly those on staticContextParams) so it seemed most consistent to keep that behavior.

And fix a test that was flaky because of non-deterministic hashmap key
ordering
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@landonxjames landonxjames added this pull request to the merge queue Jul 13, 2024
Merged via the queue into main with commit e4a58c3 Jul 13, 2024
44 checks passed
@landonxjames landonxjames deleted the landonxjames/ep-ocp branch July 13, 2024 00:57
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.

4 participants