Split RuntimeError and RequestRejection by protocol
#2517
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| use strum_macros::Display; |
There was a problem hiding this comment.
Do we want to derive Display like this? If we get around to instrumenting the server in key areas we might want to tracing::error!(%err) and get a human readable error out?
There was a problem hiding this comment.
Should we use thiserror::Error instead?
There was a problem hiding this comment.
Yeah let's derive thiserror::Error. We should also sprinkle tracing::debug!(%err) all throughout the codebase whenever returning a rejection type; debug though, since these are not errors.
I'd like to refactor some things to remove some of these variants first.
There was a problem hiding this comment.
Yeah, tracing::debug! would be better.
rust-runtime/aws-smithy-http-server/src/routing/request_spec.rs
Outdated
Show resolved
Hide resolved
| #[derive(Debug, Display)] | ||
| pub enum ResponseRejection { | ||
| InvalidHttpStatusCode, | ||
| Serialization(crate::Error), |
There was a problem hiding this comment.
Given that we're now protocol specific presumably we don't need to use crate::Error (~ Box<dyn Error>)?
A task for another PR?
There was a problem hiding this comment.
Yeah I'm going to remove some of these first.
There was a problem hiding this comment.
Starting with e.g. #2522.
| interface ServerProtocol : Protocol { | ||
| /** The path such that `aws_smithy_http_server::proto::$path` points to the protocol's module. */ | ||
| val protocolModulePath: String |
There was a problem hiding this comment.
Even though this drys things up, I'm not sure how I feel about this. Might a constructor on RuntimeType, for requestRejection, responseRejection, and runtimeError, taking protocolModulePath: String be sufficient?
There was a problem hiding this comment.
Hmm the ServerProtocol should know where these types lie, we shouldn't invert the responsibility on the caller in ServerRuntimeType. protocolModulePath is just to make it so that most implementers can rely on the default implementations, but AWS JSON needs to switch based on version.
There was a problem hiding this comment.
protocolModulePathis just to make it so that most implementers can rely on the default implementations
Right, in Rust this would be akin to adding a const DEFAULT: &'static str; to a trait in order to get default function implementations to work? In the case where functions don't follow the default implementation this field becomes useless?
There was a problem hiding this comment.
Maybe I'm just misunderstanding Kotlin here, is the function implementation overridable?
fun requestRejection(runtimeConfig: RuntimeConfig): RuntimeType =
ServerCargoDependency.smithyHttpServer(runtimeConfig)
.toType().resolve("proto::$protocolModulePath::rejection::RequestRejection")If not then I actually have no problem with this - we're tightening the ServerProtocol contract but there's no redundancy.
There was a problem hiding this comment.
It is overridable. ServerAwsJsonProtocol overrides it.
There was a problem hiding this comment.
In the case where functions don't follow the default implementation this field becomes useless?
Not useless, the field can be used as implementations see fit. See e.g. the implementation of markerStruct in ServerRestJsonProtocol.
There was a problem hiding this comment.
In the case where it's not being used by the default function impls then it can live as just a val in the implementing class?
This is a style nit, I won't block on this.
There was a problem hiding this comment.
It is used by the default function impls.
There was a problem hiding this comment.
I mean in the case we are not using the default function impls, instead we're rolling our own*, then the val can live in the class implementing the interface.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
As outlined in the [Protocol Specific Errors] of the [Service Builder Improvements RFC], `RuntimeError` should be split up into smaller, protocol specific, errors which accurately model the failure cases of each protocol. The same goes for `RequestRejection`. Closes #1703. [Protocol Specific Errors]: https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0020_service_builder.md#protocol-specific-errors [Service Builder Improvements RFC]: https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0020_service_builder.md
david-perez
force-pushed
the
davidpz/split-runtimeerror-and-requestrejection-into-protocol-specific-structs
branch
from
c271761 to
fad1764
Compare
|
I've rebased into a single commit to avoid GitHub's merge queue using an ugly squashed commit message. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
david-perez
deleted the
davidpz/split-runtimeerror-and-requestrejection-into-protocol-specific-structs
branch
Rejection types used to be loose, but now that they are protocol-specific (see #2517), they can be tightened up to be more useful. Where possible, variants now no longer take in the dynamic `crate::Error`, instead taking in concrete error types arising from particular fallible invocations in the generated SDKs.This makes it easier to see how errors arise, and allows for more specific and helpful error messages that can be logged. We `#[derive(thiserror::Error)]` on rejection types to cut down on boilerplate code. This commit removes the dependency on `strum_macros`.
Rejection types used to be loose, but now that they are protocol-specific (see #2517), they can be tightened up to be more useful. Where possible, variants now no longer take in the dynamic `crate::Error`, instead taking in concrete error types arising from particular fallible invocations in the generated SDKs.This makes it easier to see how errors arise, and allows for more specific and helpful error messages that can be logged. We `#[derive(thiserror::Error)]` on rejection types to cut down on boilerplate code. This commit removes the dependency on `strum_macros`.
Rejection types used to be loose, but now that they are protocol-specific (see #2517), they can be tightened up to be more useful. Where possible, variants now no longer take in the dynamic `crate::Error`, instead taking in concrete error types arising from particular fallible invocations in the generated SDKs.This makes it easier to see how errors arise, and allows for more specific and helpful error messages that can be logged. We `#[derive(thiserror::Error)]` on rejection types to cut down on boilerplate code. This commit removes the dependency on `strum_macros`.
As outlined in the [Protocol Specific Errors] of the [Service Builder Improvements RFC], `RuntimeError` should be split up into smaller, protocol specific, errors which accurately model the failure cases of each protocol. The same goes for `RequestRejection`. Closes #1703. [Protocol Specific Errors]: https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0020_service_builder.md#protocol-specific-errors [Service Builder Improvements RFC]: https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0020_service_builder.md
Rejection types used to be loose, but now that they are protocol-specific (see #2517), they can be tightened up to be more useful. Where possible, variants now no longer take in the dynamic `crate::Error`, instead taking in concrete error types arising from particular fallible invocations in the generated SDKs.This makes it easier to see how errors arise, and allows for more specific and helpful error messages that can be logged. We `#[derive(thiserror::Error)]` on rejection types to cut down on boilerplate code. This commit removes the dependency on `strum_macros`.
As outlined in the [Protocol Specific Errors] of the [Service Builder Improvements RFC], `RuntimeError` should be split up into smaller, protocol specific, errors which accurately model the failure cases of each protocol. The same goes for `RequestRejection`. Closes #1703. [Protocol Specific Errors]: https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0020_service_builder.md#protocol-specific-errors [Service Builder Improvements RFC]: https://github.com/awslabs/smithy-rs/blob/main/design/src/rfcs/rfc0020_service_builder.md
Rejection types used to be loose, but now that they are protocol-specific (see #2517), they can be tightened up to be more useful. Where possible, variants now no longer take in the dynamic `crate::Error`, instead taking in concrete error types arising from particular fallible invocations in the generated SDKs.This makes it easier to see how errors arise, and allows for more specific and helpful error messages that can be logged. We `#[derive(thiserror::Error)]` on rejection types to cut down on boilerplate code. This commit removes the dependency on `strum_macros`.
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._
As outlined in the Protocol Specific Errors of the Service Builder
Improvements RFC,
RuntimeErrorshould be split up into smaller,protocol specific, errors which accurately model the failure cases of
each protocol.
The same goes for
RequestRejection.Closes #1703.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.