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

Code-generate constraints.smithy #2101

Open
Tracked by #1401
david-perez opened this issue Dec 14, 2022 · 2 comments
Open
Tracked by #1401

Code-generate constraints.smithy #2101

david-perez opened this issue Dec 14, 2022 · 2 comments
Labels
high-priority High priority issue server Rust server SDK

Comments

@david-perez
Copy link
Contributor

constraints.smithy is the Smithy model file we've been using to test our implementation of constraint traits. This model is handwritten.

The file is growing bigger as time goes on and is becoming increasingly difficult for implementers to remember to test all possible (shape, constraint trait) combinations, as well as constrained shapes bound to the different parts of the HTTP message. Not to mention that some of the code paths we exercise are different depending on whether shapes are transitively constrained vs directly but not transitively constrained.

A Kotlin unit test that built this model by a more orderly exploration of the possible combinations would get as a long way. It would also serve as a stepping stone for more general fuzz-testing of Smithy models.

@david-perez david-perez added the server Rust server SDK label Dec 14, 2022
@david-perez
Copy link
Contributor Author

See e.g. #2103 (which is a follow-up fix to the bugfixes in #2085) for an example of how exhaustively testing combinations is desirable and how subtle the bugs / handwriting these can be if we don't.

david-perez added a commit that referenced this issue Jan 13, 2023
Turns out we've never supported them, neither directly constrained nor
with constrained members, because of a lack of tests. Yet another data
point to prioritize working on code-generating `constraints.smithy` (see
#2101).

The implementation is simple: we just need to call the symbol provider
on the member symbols instead of on the target symbols so we get
`Option<T>` list members / map values if applicable, and handle the
wrapper when converting between unconstrained and constrained types with
help from `match` and `Option<T>::map`.
david-perez added a commit that referenced this issue Jan 17, 2023
Turns out we've never supported them, neither directly constrained nor
with constrained members, because of a lack of tests. Yet another data
point to prioritize working on code-generating `constraints.smithy` (see
#2101).

The implementation is simple: we just need to call the symbol provider
on the member symbols instead of on the target symbols so we get
`Option<T>` list members / map values if applicable, and handle the
wrapper when converting between unconstrained and constrained types with
help from `match` and `Option<T>::map`.
aws-sdk-rust-ci pushed a commit to awslabs/aws-sdk-rust that referenced this issue Jan 26, 2023
…(#2213)

Turns out we've never supported them, neither directly constrained nor
with constrained members, because of a lack of tests. Yet another data
point to prioritize working on code-generating `constraints.smithy` (see
smithy-lang/smithy-rs#2101).

The implementation is simple: we just need to call the symbol provider
on the member symbols instead of on the target symbols so we get
`Option<T>` list members / map values if applicable, and handle the
wrapper when converting between unconstrained and constrained types with
help from `match` and `Option<T>::map`.
@david-perez david-perez added the high-priority High priority issue label Apr 17, 2023
david-perez added a commit that referenced this issue Apr 17, 2023
The issue is we're not changing the return type of the payload
deserializing function to be the unconstrained type (e.g. the builder in
case of an `@httpPayload`-bound structure shape) when the shape is
constrained.

Yet another example of why code-generating `constraints.smithy` (see #2101)
is important.

Closes #2583.
david-perez added a commit that referenced this issue Apr 19, 2023
…#2584)

The issue is we're not changing the return type of the payload
deserializing function to be the unconstrained type (e.g. the builder in
case of an `@httpPayload`-bound structure shape) when the shape is
constrained.

Yet another example of why code-generating `constraints.smithy` (see
#2101)
is important.

Closes #2583.

## Testing

The added integration test operation fails to compile without this
patch.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] 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._
unexge pushed a commit that referenced this issue Apr 24, 2023
…#2584)

The issue is we're not changing the return type of the payload
deserializing function to be the unconstrained type (e.g. the builder in
case of an `@httpPayload`-bound structure shape) when the shape is
constrained.

Yet another example of why code-generating `constraints.smithy` (see
#2101)
is important.

Closes #2583.

## Testing

The added integration test operation fails to compile without this
patch.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] 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._
rcoh pushed a commit that referenced this issue Apr 24, 2023
…#2584)

The issue is we're not changing the return type of the payload
deserializing function to be the unconstrained type (e.g. the builder in
case of an `@httpPayload`-bound structure shape) when the shape is
constrained.

Yet another example of why code-generating `constraints.smithy` (see
#2101)
is important.

Closes #2583.

## Testing

The added integration test operation fails to compile without this
patch.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] 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._
@david-perez
Copy link
Contributor Author

To inspect the code-generated models, we can use SmithyIdlModelSerializer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority High priority issue server Rust server SDK
Projects
None yet
Development

No branches or pull requests

1 participant