Skip to content

Commit

Permalink
Improve invalid HTTP status code responses failure mode handling (#2522)
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 authored and rcoh committed Apr 24, 2023
1 parent bd2577c commit d83c5a6
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 d83c5a6

Please sign in to comment.