diff --git a/libs/utils/src/http/endpoint.rs b/libs/utils/src/http/endpoint.rs index 4bfb5bf994..4a78f16cfb 100644 --- a/libs/utils/src/http/endpoint.rs +++ b/libs/utils/src/http/endpoint.rs @@ -1,5 +1,5 @@ use crate::auth::{Claims, JwtAuth}; -use crate::http::error; +use crate::http::error::{api_error_handler, route_error_handler, ApiError}; use anyhow::{anyhow, Context}; use hyper::header::{HeaderName, AUTHORIZATION}; use hyper::http::HeaderValue; @@ -16,8 +16,6 @@ use std::future::Future; use std::net::TcpListener; use std::str::FromStr; -use super::error::ApiError; - static SERVE_METRICS_COUNT: Lazy = Lazy::new(|| { register_int_counter!( "libmetrics_metric_handler_requests_total", @@ -38,6 +36,8 @@ struct RequestId(String); /// Use this to distinguish between logs of different HTTP requests: every request handler wrapped /// in this type will get request info logged in the wrapping span, including the unique request ID. /// +/// This also handles errors, logging them and converting them to an HTTP error response. +/// /// There could be other ways to implement similar functionality: /// /// * procmacros placed on top of all handler methods @@ -54,21 +54,19 @@ struct RequestId(String); /// tries to achive with its `.instrument` used in the current approach. /// /// If needed, a declarative macro to substitute the |r| ... closure boilerplate could be introduced. -pub struct RequestSpan(pub H) +pub struct RequestSpan(pub H) where - E: Into> + 'static, - R: Future, E>> + Send + 'static, + R: Future, ApiError>> + Send + 'static, H: Fn(Request) -> R + Send + Sync + 'static; -impl RequestSpan +impl RequestSpan where - E: Into> + 'static, - R: Future, E>> + Send + 'static, + R: Future, ApiError>> + Send + 'static, H: Fn(Request) -> R + Send + Sync + 'static, { /// Creates a tracing span around inner request handler and executes the request handler in the contex of that span. /// Use as `|r| RequestSpan(my_handler).handle(r)` instead of `my_handler` as the request handler to get the span enabled. - pub async fn handle(self, request: Request) -> Result, E> { + pub async fn handle(self, request: Request) -> Result, ApiError> { let request_id = request.context::().unwrap_or_default().0; let method = request.method(); let path = request.uri().path(); @@ -83,15 +81,22 @@ where info!("Handling request"); } - // Note that we reuse `error::handler` here and not returning and error at all, - // yet cannot use `!` directly in the method signature due to `routerify::RouterBuilder` limitation. - // Usage of the error handler also means that we expect only the `ApiError` errors to be raised in this call. - // - // Panics are not handled separately, there's a `tracing_panic_hook` from another module to do that globally. + // No special handling for panics here. There's a `tracing_panic_hook` from another + // module to do that globally. let res = (self.0)(request).await; cancellation_guard.disarm(); + // Log the result if needed. + // + // We also convert any errors into an Ok response with HTTP error code here. + // `make_router` sets a last-resort error handler that would do the same, but + // we prefer to do it here, before we exit the request span, so that the error + // is still logged with the span. + // + // (Because we convert errors to Ok response, we never actually return an error, + // and we could declare the function to return the never type (`!`). However, + // using `routerify::RouterBuilder` requires a proper error type.) match res { Ok(response) => { let response_status = response.status(); @@ -102,7 +107,7 @@ where } Ok(response) } - Err(e) => Ok(error::handler(e.into()).await), + Err(err) => Ok(api_error_handler(err)), } } .instrument(request_span) @@ -210,7 +215,7 @@ pub fn make_router() -> RouterBuilder { .get("/metrics", |r| { RequestSpan(prometheus_metrics_handler).handle(r) }) - .err_handler(error::handler) + .err_handler(route_error_handler) } pub fn attach_openapi_ui( diff --git a/libs/utils/src/http/error.rs b/libs/utils/src/http/error.rs index 3c6023eb80..4eff16b6a3 100644 --- a/libs/utils/src/http/error.rs +++ b/libs/utils/src/http/error.rs @@ -83,13 +83,24 @@ impl HttpErrorBody { } } -pub async fn handler(err: routerify::RouteError) -> Response { - let api_error = err - .downcast::() - .expect("handler should always return api error"); +pub async fn route_error_handler(err: routerify::RouteError) -> Response { + match err.downcast::() { + Ok(api_error) => api_error_handler(*api_error), + Err(other_error) => { + // We expect all the request handlers to return an ApiError, so this should + // not be reached. But just in case. + error!("Error processing HTTP request: {other_error:?}"); + HttpErrorBody::response_from_msg_and_status( + other_error.to_string(), + StatusCode::INTERNAL_SERVER_ERROR, + ) + } + } +} +pub fn api_error_handler(api_error: ApiError) -> Response { // Print a stack trace for Internal Server errors - if let ApiError::InternalServerError(_) = api_error.as_ref() { + if let ApiError::InternalServerError(_) = api_error { error!("Error processing HTTP request: {api_error:?}"); } else { error!("Error processing HTTP request: {api_error:#}");