Unverified Commit 2f70ac8e authored by david-perez's avatar david-perez Committed by GitHub
Browse files

`DEBUG`-log server request rejections (#2597) 

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._
parent 63a1e78f

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt

+1 −0
Original line number Diff line number Diff line
@@ -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"))

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

+10 −7
Original line number Diff line number Diff line
@@ -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,
@@ -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,
@@ -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,

                        )
@@ -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.
@@ -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(),

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

+2 −0
Original line number Diff line number Diff line
@@ -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}")]

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

+5 −0
Original line number Diff line number Diff line
@@ -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),

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

+1 −0
Original line number Diff line number Diff line
@@ -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)),

        }

    }