From 2f70ac8ec08aacc5896bf51e02e6a992c0190370 Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 19 Apr 2023 13:00:17 +0200 Subject: [PATCH] `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._ --- .../server/smithy/ServerCargoDependency.kt | 1 + .../ServerHttpBoundProtocolGenerator.kt | 17 ++++++++++------- .../src/proto/aws_json/rejection.rs | 2 ++ .../src/proto/rest_json_1/rejection.rs | 5 +++++ .../src/proto/rest_json_1/runtime_error.rs | 1 + .../src/proto/rest_xml/rejection.rs | 3 +++ 6 files changed, 22 insertions(+), 7 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt index 1726057a9f..956a88ecf2 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt @@ -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")) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt index 889967412e..85b624b567 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt @@ -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>` and replace with thin wrapper around `Collect`. rustTemplate( """ - // TODO(https://github.com/awslabs/smithy-rs/issues/2238): Remove the `Pin>` 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(), diff --git a/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs b/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs index 10b94964df..491e865dd6 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/aws_json/rejection.rs @@ -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}")] diff --git a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs index 87925e2f03..5ccf0225f7 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/rejection.rs @@ -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), diff --git a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs index fdf55e623a..0525d9b576 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/rest_json_1/runtime_error.rs @@ -115,6 +115,7 @@ impl From 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)), } } diff --git a/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs b/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs index 829d330dfe..51b0ac0ad6 100644 --- a/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/proto/rest_xml/rejection.rs @@ -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),