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

Use aws_smithy_types::primitive::Parse in the server #2528

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

david-perez
Copy link
Contributor

This further reduces the number of request rejection variants.
https://docs.rs/aws-smithy-types/latest/aws_smithy_types/primitive/trait.Parse.html

Closes #1232.


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 david-perez added the server Rust server SDK label Apr 3, 2023
@david-perez david-perez requested a review from a team as a code owner April 3, 2023 09:26
@david-perez david-perez requested review from drganjoo and 82marbag and removed request for a team April 3, 2023 09:26
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

Copy link
Contributor

@hlbarber hlbarber left a comment

Choose a reason for hiding this comment

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

Great stuff

@david-perez david-perez force-pushed the davidpz/split-runtimeerror-and-requestrejection-into-protocol-specific-structs branch from c271761 to fad1764 Compare April 3, 2023 12:39
Base automatically changed from davidpz/split-runtimeerror-and-requestrejection-into-protocol-specific-structs to main April 3, 2023 13:31
@david-perez david-perez force-pushed the davidpz/use-primitive-parse-in-the-server branch from 15b7574 to dbea286 Compare April 3, 2023 13:56
@david-perez
Copy link
Contributor Author

I've rebased into a single commit to prevent GitHub's merge queue using an ugly squashed commit message.

@david-perez david-perez enabled auto-merge April 3, 2023 13:57
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@david-perez david-perez force-pushed the davidpz/use-primitive-parse-in-the-server branch from dbea286 to 433b377 Compare April 3, 2023 14:19
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@david-perez david-perez added this pull request to the merge queue Apr 3, 2023
Merged via the queue into main with commit f036223 Apr 3, 2023
@david-perez david-perez deleted the davidpz/use-primitive-parse-in-the-server branch April 3, 2023 15:12
@david-perez david-perez mentioned this pull request Apr 4, 2023
7 tasks
unexge pushed a commit that referenced this pull request Apr 24, 2023
rcoh pushed a commit that referenced this pull request Apr 24, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jul 17, 2024
RPC v2 CBOR is a new protocol that ~is being added~ has [recently been
added](https://smithy.io/2.0/additional-specs/protocols/smithy-rpc-v2.html)
to the Smithy
specification.

_(I'll add more details here as the patchset evolves)_

Credit goes to @jjant for initial implementation of the router, which I
built on top of from his
[`jjant/smithy-rpc-v2-exploration`](https://github.com/awslabs/smithy-rs/tree/jjant/smithy-rpc-v2-exploration)
branch.

Tracking issue: #3573.

## Caveats

`TODO`s are currently exhaustively sprinkled throughout the patch
documenting what remains to be done. Most of these need to be addressed
before this can be merged in; some can be punted on to not make this PR
bigger.

However, I'd like to call out the major caveats and blockers here. I'll
keep updating this list as the patchset evolves.

- [x] RPC v2 has still not been added to the Smithy specification. It is
currently being worked on over in the
[`smithy-rpc-v2`](https://github.com/awslabs/smithy/tree/smithy-rpc-v2)
branch. The following are prerrequisites for this PR to be merged;
**until they are done CI on this PR will fail**:
    - [x] Smithy merges in RPC v2 support.
    - [x] Smithy releases a new version incorporating RPC v2 support.
- Released in [Smithy
v1.47](https://github.com/smithy-lang/smithy/releases/tag/1.47.0)
    - [x] smithy-rs updates to the new version.
        - Updated in #3552
- [x] ~Protocol tests for the protocol do not currently exist in Smithy.
Until those get written~, this PR resorts to Rust unit tests and
integration tests that use `serde` to round-trip messages and compare
`serde`'s encoders and decoders with ours for correctness.
- Protocol tests are under the
[`smithy-protocol-tests`](https://github.com/smithy-lang/smithy/tree/main/smithy-protocol-tests/model/rpcv2Cbor)
directory in Smithy.
- We're keeping the `serde_cbor` round-trip tests for defense in depth.
- [ ] #3709 - Currently
only server-side support has been implemented, because that's what I'm
most familiar. However, we're almost all the way there to add
client-side support.
- ~[ ] [Smithy `document`
shapes](https://smithy.io/2.0/spec/simple-types.html#document) are not
supported. RPC v2's specification currently doesn't indicate how to
implement them.~
- [The
spec](https://smithy.io/2.0/additional-specs/protocols/smithy-rpc-v2.html#shape-serialization)
ended up leaving them as unsupported: "Document types are not currently
supported in this protocol."

## Prerequisite PRs

This section lists prerequisite PRs and issues that would make the diff
for this one lighter or easier to understand. It's preferable that these
PRs be merged prior to this one; some are hard prerequisites. They
mostly relate to parts of the codebase I've had to touch or ~pilfer~
inspect in this PR where I've made necessary changes, refactors and
"drive-by improvements" that are mostly unrelated, although some
directly unlock things I've needed in this patchset. It makes sense to
pull them out to ease reviewability and make this patch more
semantically self-contained.

- #2516
- #2517
- #2522
- #2524
- #2528
- #2536
- #2537
- #2531
- #2538
- #2539
- #2542
- #3684
- #3678
- #3690
- #3713
- #3726
- #3752

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

~RPC v2 has still not been added to the Smithy specification. It is
currently being worked on over in the
[`smithy-rpc-v2`](https://github.com/awslabs/smithy/tree/smithy-rpc-v2)
branch.~

This can only be tested _locally_ following these steps:

~1. Clone [the Smithy
repository](https://github.com/smithy-lang/smithy/tree/smithy-rpc-v2)
and checkout the `smithy-rpc-v2` branch.
2. Inside your local checkout of smithy-rs pointing to this PR's branch,
make sure you've added `mavenLocal()` as a repository in the
`build.gradle.kts` files.
[Example](8df82fd).
4. Inside your local checkout of Smithy's `smithy-rpc-v2` branch:
1. Set `VERSION` to the current Smithy version used in smithy-rs (1.28.1
as of writing, but [check
here](https://github.com/awslabs/smithy-rs/blob/main/gradle.properties#L21)).
    2. Run `./gradlew clean build pTML`.~
~6.~ 1. In your local checkout of the smithy-rs's `smithy-rpc-v2`
branch, run `./gradlew codegen-server-test:build -P
modules='rpcv2Cbor'`.

~You can troubleshoot whether you have Smithy correctly set up locally
by inspecting
`~/.m2/repository/software/amazon/smithy/smithy-protocols-traits`.~

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] 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._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Rust server SDK
Projects
None yet
2 participants