Skip to content

Commit c7c8285

Browse files
authored
Tighten server rejection types (#2542)
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`.
1 parent 035c99e commit c7c8285

File tree

6 files changed

+102
-153
lines changed

6 files changed

+102
-153
lines changed

rust-runtime/aws-smithy-http-server/Cargo.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ pin-project-lite = "0.2"
3535
once_cell = "1.13"
3636
regex = "1.5.5"
3737
serde_urlencoded = "0.7"
38-
strum_macros = "0.24"
3938
thiserror = "1.0.0"
4039
tracing = "0.1.35"
4140
tokio = { version = "1.23.1", features = ["full"] }

rust-runtime/aws-smithy-http-server/src/macros.rs

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,3 @@ macro_rules! convert_to_request_rejection {
9494
}
9595
};
9696
}
97-
98-
macro_rules! convert_to_response_rejection {
99-
($from:ty, $to:ident) => {
100-
impl From<$from> for ResponseRejection {
101-
fn from(err: $from) -> Self {
102-
Self::$to(crate::Error::new(err))
103-
}
104-
}
105-
};
106-
}

rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs

Lines changed: 16 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,45 +3,34 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
use strum_macros::Display;
7-
86
use crate::rejection::MissingContentTypeReason;
7+
use thiserror::Error;
98

10-
#[derive(Debug, Display)]
9+
#[derive(Debug, Error)]
1110
pub enum ResponseRejection {
12-
Serialization(crate::Error),
13-
Http(crate::Error),
11+
#[error("error serializing JSON-encoded body: {0}")]
12+
Serialization(#[from] aws_smithy_http::operation::error::SerializationError),
13+
#[error("error building HTTP response: {0}")]
14+
HttpBuild(#[from] http::Error),
1415
}
1516

16-
impl std::error::Error for ResponseRejection {}
17-
18-
convert_to_response_rejection!(aws_smithy_http::operation::error::SerializationError, Serialization);
19-
convert_to_response_rejection!(http::Error, Http);
20-
21-
#[derive(Debug, Display)]
17+
#[derive(Debug, Error)]
2218
pub enum RequestRejection {
23-
HttpBody(crate::Error),
24-
MissingContentType(MissingContentTypeReason),
25-
JsonDeserialize(crate::Error),
19+
#[error("error converting non-streaming body to bytes: {0}")]
20+
BufferHttpBodyBytes(crate::Error),
21+
#[error("expected `Content-Type` header not found: {0}")]
22+
MissingContentType(#[from] MissingContentTypeReason),
23+
#[error("error deserializing request HTTP body as JSON: {0}")]
24+
JsonDeserialize(#[from] aws_smithy_json::deserialize::error::DeserializeError),
25+
#[error("request does not adhere to modeled constraints: {0}")]
2626
ConstraintViolation(String),
2727
}
2828

29-
impl std::error::Error for RequestRejection {}
30-
3129
impl From<std::convert::Infallible> for RequestRejection {
3230
fn from(_err: std::convert::Infallible) -> Self {
3331
match _err {}
3432
}
3533
}
3634

37-
impl From<MissingContentTypeReason> for RequestRejection {
38-
fn from(e: MissingContentTypeReason) -> Self {
39-
Self::MissingContentType(e)
40-
}
41-
}
42-
43-
convert_to_request_rejection!(aws_smithy_json::deserialize::error::DeserializeError, JsonDeserialize);
44-
45-
convert_to_request_rejection!(hyper::Error, HttpBody);
46-
47-
convert_to_request_rejection!(Box<dyn std::error::Error + Send + Sync + 'static>, HttpBody);
35+
convert_to_request_rejection!(hyper::Error, BufferHttpBodyBytes);
36+
convert_to_request_rejection!(Box<dyn std::error::Error + Send + Sync + 'static>, BufferHttpBodyBytes);

rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs

Lines changed: 48 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -47,46 +47,47 @@
4747
//!
4848
//! Consult `crate::proto::$protocolName::rejection` for rejection types for other protocols.
4949
50-
use std::num::TryFromIntError;
51-
52-
use strum_macros::Display;
53-
5450
use crate::rejection::MissingContentTypeReason;
51+
use std::num::TryFromIntError;
52+
use thiserror::Error;
5553

5654
/// Errors that can occur when serializing the operation output provided by the service implementer
5755
/// into an HTTP response.
58-
#[derive(Debug, Display)]
56+
#[derive(Debug, Error)]
5957
pub enum ResponseRejection {
6058
/// Used when the service implementer provides an integer outside the 100-999 range for a
6159
/// member targeted by `httpResponseCode`.
6260
/// See <https://github.com/awslabs/smithy/issues/1116>.
61+
#[error("invalid bound HTTP status code; status codes must be inside the 100-999 range: {0}")]
6362
InvalidHttpStatusCode(TryFromIntError),
6463

65-
/// Used when an invalid HTTP header value (a value that cannot be parsed as an
66-
/// `[http::header::HeaderValue]`) is provided for a shape member bound to an HTTP header with
64+
/// Used when an invalid HTTP header name (a value that cannot be parsed as an
65+
/// [`http::header::HeaderName`]) or HTTP header value (a value that cannot be parsed as an
66+
/// [`http::header::HeaderValue`]) is provided for a shape member bound to an HTTP header with
6767
/// `httpHeader` or `httpPrefixHeaders`.
6868
/// Used when failing to serialize an `httpPayload`-bound struct into an HTTP response body.
69-
Build(crate::Error),
70-
71-
/// Used when failing to serialize a struct into a `String` for the HTTP response body (for
72-
/// example, converting a struct into a JSON-encoded `String`).
73-
Serialization(crate::Error),
69+
#[error("error building HTTP response: {0}")]
70+
Build(#[from] aws_smithy_http::operation::error::BuildError),
71+
72+
/// Used when failing to serialize a struct into a `String` for the JSON-encoded HTTP response
73+
/// body.
74+
/// Fun fact: as of writing, this can only happen when date formatting
75+
/// (`aws_smithy_types::date_time::DateTime:fmt`) fails, which can only happen if the
76+
/// supplied timestamp is outside of the valid range when formatting using RFC-3339, i.e. a
77+
/// date outside the `0001-01-01T00:00:00.000Z`-`9999-12-31T23:59:59.999Z` range is supplied.
78+
#[error("error serializing JSON-encoded body: {0}")]
79+
Serialization(#[from] aws_smithy_http::operation::error::SerializationError),
7480

7581
/// Used when consuming an [`http::response::Builder`] into the constructed [`http::Response`]
7682
/// when calling [`http::response::Builder::body`].
7783
/// This error can happen if an invalid HTTP header value (a value that cannot be parsed as an
7884
/// `[http::header::HeaderValue]`) is used for the protocol-specific response `Content-Type`
7985
/// header, or for additional protocol-specific headers (like `X-Amzn-Errortype` to signal
8086
/// errors in RestJson1).
81-
Http(crate::Error),
87+
#[error("error building HTTP response: {0}")]
88+
HttpBuild(#[from] http::Error),
8289
}
8390

84-
impl std::error::Error for ResponseRejection {}
85-
86-
convert_to_response_rejection!(aws_smithy_http::operation::error::BuildError, Build);
87-
convert_to_response_rejection!(aws_smithy_http::operation::error::SerializationError, Serialization);
88-
convert_to_response_rejection!(http::Error, Http);
89-
9091
/// Errors that can occur when deserializing an HTTP request into an _operation input_, the input
9192
/// that is passed as the first argument to operation handlers.
9293
///
@@ -98,59 +99,65 @@ convert_to_response_rejection!(http::Error, Http);
9899
/// deliberate design choice to keep code generation simple. After all, this type is an inner
99100
/// detail of the framework the service implementer does not interact with.
100101
///
101-
/// If a variant takes in a value, it represents the underlying cause of the error. This inner
102-
/// value should be of the type-erased boxed error type `[crate::Error]`. In practice, some of the
103-
/// variants that take in a value are only instantiated with errors of a single type in the
104-
/// generated code. For example, `UriPatternMismatch` is only instantiated with an error coming
105-
/// from a `nom` parser, `nom::Err<nom::error::Error<&str>>`. This is reflected in the converters
106-
/// below that convert from one of these very specific error types into one of the variants. For
107-
/// example, the `RequestRejection` implements `From<hyper::Error>` to construct the `HttpBody`
108-
/// variant. This is a deliberate design choice to make the code simpler and less prone to changes.
102+
/// If a variant takes in a value, it represents the underlying cause of the error.
109103
///
110104
/// The variants are _roughly_ sorted in the order in which the HTTP request is processed.
111-
#[derive(Debug, Display)]
105+
#[derive(Debug, Error)]
112106
pub enum RequestRejection {
113107
/// Used when failing to convert non-streaming requests into a byte slab with
114108
/// `hyper::body::to_bytes`.
115-
HttpBody(crate::Error),
109+
#[error("error converting non-streaming body to bytes: {0}")]
110+
BufferHttpBodyBytes(crate::Error),
116111

117112
/// Used when checking the `Content-Type` header.
118-
MissingContentType(MissingContentTypeReason),
113+
#[error("expected `Content-Type` header not found: {0}")]
114+
MissingContentType(#[from] MissingContentTypeReason),
119115

120116
/// Used when failing to deserialize the HTTP body's bytes into a JSON document conforming to
121117
/// the modeled input it should represent.
122-
JsonDeserialize(crate::Error),
118+
#[error("error deserializing request HTTP body as JSON: {0}")]
119+
JsonDeserialize(#[from] aws_smithy_json::deserialize::error::DeserializeError),
123120

124121
/// Used when failing to parse HTTP headers that are bound to input members with the `httpHeader`
125122
/// or the `httpPrefixHeaders` traits.
126-
HeaderParse(crate::Error),
123+
#[error("error binding request HTTP headers: {0}")]
124+
HeaderParse(#[from] aws_smithy_http::header::ParseError),
127125

126+
// In theory, the next two errors should never happen because the router should have already
127+
// rejected the request.
128128
/// Used when the URI pattern has a literal after the greedy label, and it is not found in the
129129
/// request's URL.
130+
#[error("request URI does not match pattern because of literal suffix after greedy label was not found")]
130131
UriPatternGreedyLabelPostfixNotFound,
131132
/// Used when the `nom` parser's input does not match the URI pattern.
133+
#[error("request URI does not match `@http` URI pattern: {0}")]
132134
UriPatternMismatch(crate::Error),
133135

134136
/// Used when percent-decoding URL query string.
135137
/// Used when percent-decoding URI path label.
136-
InvalidUtf8(crate::Error),
138+
/// This is caused when calling
139+
/// [`percent_encoding::percent_decode_str`](https://docs.rs/percent-encoding/latest/percent_encoding/fn.percent_decode_str.html).
140+
/// This can happen when the percent-encoded data decodes to bytes that are
141+
/// not a well-formed UTF-8 string.
142+
#[error("request URI cannot be percent decoded into valid UTF-8")]
143+
PercentEncodedUriNotValidUtf8(#[from] core::str::Utf8Error),
137144

138145
/// Used when failing to deserialize strings from a URL query string and from URI path labels
139146
/// into an [`aws_smithy_types::DateTime`].
140-
DateTimeParse(crate::Error),
147+
#[error("error parsing timestamp from request URI: {0}")]
148+
DateTimeParse(#[from] aws_smithy_types::date_time::DateTimeParseError),
141149

142150
/// Used when failing to deserialize strings from a URL query string and from URI path labels
143151
/// into "primitive" types.
144-
PrimitiveParse(crate::Error),
152+
#[error("error parsing primitive type from request URI: {0}")]
153+
PrimitiveParse(#[from] aws_smithy_types::primitive::PrimitiveParseError),
145154

146155
/// Used when consuming the input struct builder, and constraint violations occur.
147-
// Unlike the rejections above, this does not take in `crate::Error`, since it is constructed
148-
// directly in the code-generated SDK instead of in this crate.
156+
// This rejection is constructed directly in the code-generated SDK instead of in this crate.
157+
#[error("request does not adhere to modeled constraints: {0}")]
149158
ConstraintViolation(String),
150159
}
151160

152-
impl std::error::Error for RequestRejection {}
153-
154161
// Consider a conversion between `T` and `U` followed by a bubbling up of the conversion error
155162
// through `Result<_, RequestRejection>`. This [`From`] implementation accomodates the special case
156163
// where `T` and `U` are equal, in such cases `T`/`U` a enjoy `TryFrom<T>` with
@@ -170,43 +177,24 @@ impl From<std::convert::Infallible> for RequestRejection {
170177
}
171178
}
172179

173-
impl From<MissingContentTypeReason> for RequestRejection {
174-
fn from(e: MissingContentTypeReason) -> Self {
175-
Self::MissingContentType(e)
176-
}
177-
}
178-
179180
// These converters are solely to make code-generation simpler. They convert from a specific error
180181
// type (from a runtime/third-party crate or the standard library) into a variant of the
181182
// [`crate::rejection::RequestRejection`] enum holding the type-erased boxed [`crate::Error`]
182183
// type. Generated functions that use [crate::rejection::RequestRejection] can thus use `?` to
183184
// bubble up instead of having to sprinkle things like [`Result::map_err`] everywhere.
184185

185-
convert_to_request_rejection!(aws_smithy_json::deserialize::error::DeserializeError, JsonDeserialize);
186-
convert_to_request_rejection!(aws_smithy_http::header::ParseError, HeaderParse);
187-
convert_to_request_rejection!(aws_smithy_types::date_time::DateTimeParseError, DateTimeParse);
188-
convert_to_request_rejection!(aws_smithy_types::primitive::PrimitiveParseError, PrimitiveParse);
189-
convert_to_request_rejection!(serde_urlencoded::de::Error, InvalidUtf8);
190-
191186
impl From<nom::Err<nom::error::Error<&str>>> for RequestRejection {
192187
fn from(err: nom::Err<nom::error::Error<&str>>) -> Self {
193188
Self::UriPatternMismatch(crate::Error::new(err.to_owned()))
194189
}
195190
}
196191

197-
// Used when calling
198-
// [`percent_encoding::percent_decode_str`](https://docs.rs/percent-encoding/latest/percent_encoding/fn.percent_decode_str.html)
199-
// and bubbling up.
200-
// This can happen when the percent-encoded data in e.g. a query string decodes to bytes that are
201-
// not a well-formed UTF-8 string.
202-
convert_to_request_rejection!(std::str::Utf8Error, InvalidUtf8);
203-
204192
// `[crate::body::Body]` is `[hyper::Body]`, whose associated `Error` type is `[hyper::Error]`. We
205193
// need this converter for when we convert the body into bytes in the framework, since protocol
206194
// tests use `[crate::body::Body]` as their body type when constructing requests (and almost
207195
// everyone will run a Hyper-based server in their services).
208-
convert_to_request_rejection!(hyper::Error, HttpBody);
196+
convert_to_request_rejection!(hyper::Error, BufferHttpBodyBytes);
209197

210198
// Useful in general, but it also required in order to accept Lambda HTTP requests using
211199
// `Router<lambda_http::Body>` since `lambda_http::Error` is a type alias for `Box<dyn Error + ..>`.
212-
convert_to_request_rejection!(Box<dyn std::error::Error + Send + Sync + 'static>, HttpBody);
200+
convert_to_request_rejection!(Box<dyn std::error::Error + Send + Sync + 'static>, BufferHttpBodyBytes);

0 commit comments

Comments
 (0)