Skip to content

Commit

Permalink
Improve invalid HTTP status code responses failure mode handling
Browse files Browse the repository at this point in the history
If the model only uses the `@http` trait to define the response code,
Smithy already checks that this is a valid status code, so the
conversion cannot fail, and we can avoid rejecting with
`ResponseRejection::InvalidHttpStatusCode`.

This commit also removes this variant from RPC protocols, where HTTP
binding traits are not allowed, and so this failure mode can never
happen.
  • Loading branch information
david-perez committed Apr 3, 2023
1 parent f708076 commit d91de83
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -439,17 +439,18 @@ class ServerHttpBoundProtocolTraitImplGenerator(
Attribute.AllowUnusedMut.render(this)
rustTemplate("let mut builder = #{http}::Response::builder();", *codegenScope)
serverRenderResponseHeaders(operationShape)
// Fallback to the default code of `@http`, 200.
// Fallback to the default code of `@http`, which should be 200.
val httpTraitDefaultStatusCode = HttpTrait
.builder().method("GET").uri(UriPattern.parse("/")) /* Required to build */
.build()
.code
check(httpTraitDefaultStatusCode == 200)
val httpTraitStatusCode = operationShape.getTrait<HttpTrait>()?.code ?: httpTraitDefaultStatusCode
bindings.find { it.location == HttpLocation.RESPONSE_CODE }
?.let {
serverRenderResponseCodeBinding(it, httpTraitStatusCode)(this)
}
// no binding, use http's
// No binding, use `@http`.
?: serverRenderHttpResponseCode(httpTraitStatusCode)(this)

operationShape.outputShape(model).findStreamingMember(model)?.let {
Expand Down Expand Up @@ -555,46 +556,42 @@ class ServerHttpBoundProtocolTraitImplGenerator(
)
}

private fun serverRenderHttpResponseCode(
defaultCode: Int,
): Writable {
return writable {
rustTemplate(
"""
let status = $defaultCode;
let http_status: u16 = status.try_into()
.map_err(|_| #{ResponseRejection}::InvalidHttpStatusCode)?;
builder = builder.status(http_status);
""".trimIndent(),
*codegenScope,
)
private fun serverRenderHttpResponseCode(defaultCode: Int) = writable {
check(defaultCode in 100..999) {
"""
Smithy library lied to us. According to https://smithy.io/2.0/spec/http-bindings.html#http-trait,
"The provided value SHOULD be between 100 and 599, and it MUST be between 100 and 999".
""".replace("\n", "").trimIndent()
}
rustTemplate(
"""
let http_status: u16 = $defaultCode;
builder = builder.status(http_status);
""",
*codegenScope,
)
}

private fun serverRenderResponseCodeBinding(
binding: HttpBindingDescriptor,
/** This is the status code to fall back on if the member shape bound with `@httpResponseCode` is not
* `@required` and the user did not provide a value for it at runtime. **/
fallbackStatusCode: Int,
): Writable {
check(binding.location == HttpLocation.RESPONSE_CODE)

return writable {
val memberName = symbolProvider.toMemberName(binding.member)
rust("let status = output.$memberName")
if (symbolProvider.toSymbol(binding.member).isOptional()) {
rustTemplate(
"""
.unwrap_or($fallbackStatusCode)
""".trimIndent(),
*codegenScope,
)
withBlock("let status = output.$memberName", ";") {
if (symbolProvider.toSymbol(binding.member).isOptional()) {
rust(".unwrap_or($fallbackStatusCode)")
}
}
rustTemplate(
"""
;
let http_status: u16 = status.try_into()
.map_err(|_| #{ResponseRejection}::InvalidHttpStatusCode)?;
let http_status: u16 = status.try_into().map_err(#{ResponseRejection}::InvalidHttpStatusCode)?;
builder = builder.status(http_status);
""".trimIndent(),
""",
*codegenScope,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use crate::rejection::MissingContentTypeReason;

#[derive(Debug, Display)]
pub enum ResponseRejection {
InvalidHttpStatusCode,
Serialization(crate::Error),
Http(crate::Error),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@
//!
//! Consult `crate::proto::$protocolName::rejection` for rejection types for other protocols.
use std::num::TryFromIntError;

use strum_macros::Display;

use crate::rejection::MissingContentTypeReason;
Expand All @@ -58,7 +60,7 @@ pub enum ResponseRejection {
/// Used when the service implementer provides an integer outside the 100-999 range for a
/// member targeted by `httpResponseCode`.
/// See <https://github.com/awslabs/smithy/issues/1116>.
InvalidHttpStatusCode,
InvalidHttpStatusCode(TryFromIntError),

/// Used when an invalid HTTP header value (a value that cannot be parsed as an
/// `[http::header::HeaderValue]`) is provided for a shape member bound to an HTTP header with
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
//! [`crate::proto::rest_json_1::rejection::RequestRejection::JsonDeserialize`] is swapped for
//! [`RequestRejection::XmlDeserialize`].
use std::num::TryFromIntError;

use strum_macros::Display;

#[derive(Debug, Display)]
pub enum ResponseRejection {
InvalidHttpStatusCode,
InvalidHttpStatusCode(TryFromIntError),
Build(crate::Error),
Serialization(crate::Error),
Http(crate::Error),
Expand Down

0 comments on commit d91de83

Please sign in to comment.