Skip to content

Commit

Permalink
DEBUG-log server request rejections (#2597)
Browse files Browse the repository at this point in the history
This commit logs server request rejections at the `DEBUG` level in an
operation's `FromRequest` implementation.

This commit is analogous to the one in PR #2524 for response rejections.
However, request rejections are _not_ errors, so they shouldn't be
logged at the `ERROR` level. Indeed, they happen every time the server
rejects a malformed request.

Prior to this commit, the `RuntimeError::NotAcceptable` variant was the
only `RuntimeError` variant that was manually constructed. This commit
makes it so that it now results from a conversion from a new
`RequestRejection::NotAcceptable` variant.

We now leverage `futures_util::future::TryFutureExt::map` to map a
future
that uses `RequestRejection` as its error into a future that uses
`RuntimeError`, and centrally log the rejection there. `futures_util` is
already a transitive dependency of server SDKs (via e.g. `hyper` and
`tower`), so adding it is a direct dependency is not worse.

This helps with #2521.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
david-perez authored and rcoh committed Apr 24, 2023
1 parent aef9d79 commit 2c80b15
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.RuntimeConfig
object ServerCargoDependency {
val AsyncTrait: CargoDependency = CargoDependency("async-trait", CratesIo("0.1"))
val FormUrlEncoded: CargoDependency = CargoDependency("form_urlencoded", CratesIo("1"))
val FuturesUtil: CargoDependency = CargoDependency("futures-util", CratesIo("0.3"))
val Mime: CargoDependency = CargoDependency("mime", CratesIo("0.3"))
val Nom: CargoDependency = CargoDependency("nom", CratesIo("7"))
val OnceCell: CargoDependency = CargoDependency("once_cell", CratesIo("1.13"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ class ServerHttpBoundProtocolTraitImplGenerator(
"Cow" to RuntimeType.Cow,
"DateTime" to RuntimeType.dateTime(runtimeConfig),
"FormUrlEncoded" to ServerCargoDependency.FormUrlEncoded.toType(),
"FuturesUtil" to ServerCargoDependency.FuturesUtil.toType(),
"HttpBody" to RuntimeType.HttpBody,
"header_util" to RuntimeType.smithyHttp(runtimeConfig).resolve("header"),
"Hyper" to RuntimeType.Hyper,
Expand Down Expand Up @@ -182,7 +183,7 @@ class ServerHttpBoundProtocolTraitImplGenerator(
rustTemplate(
"""
if !#{SmithyHttpServer}::protocols::accept_header_classifier(request.headers(), ${contentType.dq()}) {
return Err(#{RuntimeError}::NotAcceptable)
return Err(#{RequestRejection}::NotAcceptable);
}
""",
*codegenScope,
Expand All @@ -201,9 +202,7 @@ class ServerHttpBoundProtocolTraitImplGenerator(
?.let { "Some(${it.dq()})" } ?: "None"
rustTemplate(
"""
if #{SmithyHttpServer}::protocols::content_type_header_classifier(request.headers(), $expectedRequestContentType).is_err() {
return Err(#{RuntimeError}::UnsupportedMediaType)
}
#{SmithyHttpServer}::protocols::content_type_header_classifier(request.headers(), $expectedRequestContentType)?;
""",
*codegenScope,
)
Expand All @@ -213,9 +212,9 @@ class ServerHttpBoundProtocolTraitImplGenerator(

// Implement `from_request` trait for input types.
val inputFuture = "${inputSymbol.name}Future"
// TODO(https://github.com/awslabs/smithy-rs/issues/2238): Remove the `Pin<Box<dyn Future>>` and replace with thin wrapper around `Collect`.
rustTemplate(
"""
// TODO(https://github.com/awslabs/smithy-rs/issues/2238): Remove the `Pin<Box<dyn Future>>` and replace with thin wrapper around `Collect`.
#{PinProjectLite}::pin_project! {
/// A [`Future`](std::future::Future) aggregating the body bytes of a [`Request`] and constructing the
/// [`${inputSymbol.name}`](#{I}) using modelled bindings.
Expand Down Expand Up @@ -252,13 +251,17 @@ class ServerHttpBoundProtocolTraitImplGenerator(
.await
.map_err(Into::into)
};
use #{FuturesUtil}::future::TryFutureExt;
let fut = fut.map_err(|e: #{RequestRejection}| {
#{Tracing}::debug!(error = %e, "failed to deserialize request");
#{RuntimeError}::from(e)
});
$inputFuture {
inner: Box::pin(fut)
}
}
}
""".trimIndent(),
""",
*codegenScope,
"I" to inputSymbol,
"Marker" to protocol.markerStruct(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ pub enum ResponseRejection {
pub enum RequestRejection {
#[error("error converting non-streaming body to bytes: {0}")]
BufferHttpBodyBytes(crate::Error),
#[error("request contains invalid value for `Accept` header")]
NotAcceptable,
#[error("expected `Content-Type` header not found: {0}")]
MissingContentType(#[from] MissingContentTypeReason),
#[error("error deserializing request HTTP body as JSON: {0}")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ pub enum RequestRejection {
#[error("error converting non-streaming body to bytes: {0}")]
BufferHttpBodyBytes(crate::Error),

/// Used when the request contained an `Accept` header with a MIME type, and the server cannot
/// return a response body adhering to that MIME type.
#[error("request contains invalid value for `Accept` header")]
NotAcceptable,

/// Used when checking the `Content-Type` header.
#[error("expected `Content-Type` header not found: {0}")]
MissingContentType(#[from] MissingContentTypeReason),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ impl From<RequestRejection> for RuntimeError {
match err {
RequestRejection::MissingContentType(_reason) => Self::UnsupportedMediaType,
RequestRejection::ConstraintViolation(reason) => Self::Validation(reason),
RequestRejection::NotAcceptable => Self::NotAcceptable,
_ => Self::Serialization(crate::Error::new(err)),
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ pub enum RequestRejection {
#[error("error converting non-streaming body to bytes: {0}")]
BufferHttpBodyBytes(crate::Error),

#[error("request contains invalid value for `Accept` header")]
NotAcceptable,

#[error("expected `Content-Type` header not found: {0}")]
MissingContentType(#[from] MissingContentTypeReason),

Expand Down

0 comments on commit 2c80b15

Please sign in to comment.