Auto merge of #3373 - jtgeibel:simplify-routing, r=Turbo87

Move router error handling into new middleware layer

This moves handling of routing errors into middleware and adds a test for the conversion.

Additionally we previously called `router.recognize()` instead of `router.call()`. By switching we can let the `conduit-router` logic handle adding `Params` to the request extensions.

A potential allocation is removed from json error handling, although the compiler may be able to eliminate that allocation already.

r? `@Turbo87`

Author: bors

src/middleware.rs

@@ -7,6 +7,7 @@ use self::app::AppMiddleware;
 use self::debug::*;
 use self::ember_html::EmberHtml;
 use self::head::Head;
+use self::known_error_to_json::KnownErrorToJson;
 use self::log_connection_pool_status::LogConnectionPoolStatus;
 use self::static_or_continue::StaticOrContinue;
 
@@ -17,6 +18,7 @@ mod debug;
 mod ember_html;
 mod ensure_well_formed_500;
 mod head;
+mod known_error_to_json;
 mod log_connection_pool_status;
 pub mod log_request;
 mod normalize_path;
@@ -26,14 +28,14 @@ mod static_or_continue;
 use conduit_conditional_get::ConditionalGet;
 use conduit_cookie::{Middleware as Cookie, SessionMiddleware};
 use conduit_middleware::MiddlewareBuilder;
+use conduit_router::RouteBuilder;
 
 use std::env;
 use std::sync::Arc;
 
-use crate::router::R404;
 use crate::{App, Env};
 
-pub fn build_middleware(app: Arc<App>, endpoints: R404) -> MiddlewareBuilder {
+pub fn build_middleware(app: Arc<App>, endpoints: RouteBuilder) -> MiddlewareBuilder {
     let mut m = MiddlewareBuilder::new(endpoints);
     let config = app.config.clone();
     let env = config.env;
@@ -67,6 +69,7 @@ pub fn build_middleware(app: Arc<App>, endpoints: R404) -> MiddlewareBuilder {
     ));
 
     m.add(AppMiddleware::new(app));
+    m.add(KnownErrorToJson);
 
     // Note: The following `m.around()` middleware is run from bottom to top
 

src/middleware/ensure_well_formed_500.rs

@@ -2,8 +2,6 @@
 
 use super::prelude::*;
 
-// Can't derive debug because of Handler.
-#[allow(missing_debug_implementations)]
 #[derive(Default)]
 pub struct EnsureWellFormed500;
 

src/middleware/known_error_to_json.rs

@@ -0,0 +1,51 @@
+//! Converts known error types into friendly JSON errors
+//!
+//! Some similar logic exists in `crate::util::errors::AppError::try_convert()`. That low-level
+//! handling needs to remain in place, because some endpoint logic relys on detecting those
+//! normalized errors. Errors produced by the router cannot be seen by endpoints, so the conversion
+//! can be deferred until here.
+
+use super::prelude::*;
+use crate::util::errors::NotFound;
+
+use conduit_router::RouterError;
+
+#[derive(Default)]
+pub struct KnownErrorToJson;
+
+impl Middleware for KnownErrorToJson {
+    fn after(&self, _: &mut dyn RequestExt, res: AfterResult) -> AfterResult {
+        res.or_else(|e| {
+            if e.downcast_ref::<RouterError>().is_some() {
+                return Ok(NotFound.into());
+            }
+
+            Err(e)
+        })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::KnownErrorToJson;
+
+    use conduit::{Body, Handler, Method, StatusCode};
+    use conduit_middleware::MiddlewareBuilder;
+    use conduit_router::RouteBuilder;
+    use conduit_test::MockRequest;
+
+    #[test]
+    fn router_errors_become_not_found_response() {
+        let route_builder = RouteBuilder::new();
+        let mut middleware = MiddlewareBuilder::new(route_builder);
+        middleware.add(KnownErrorToJson);
+
+        let mut req = MockRequest::new(Method::GET, "/");
+        let (parts, body) = middleware.call(&mut req).unwrap().into_parts();
+        assert_eq!(parts.status, StatusCode::NOT_FOUND);
+        assert!(matches!(
+            body,
+            Body::Owned(vec) if vec == br#"{"errors":[{"detail":"Not Found"}]}"#
+        ));
+    }
+}

src/router.rs

@@ -4,11 +4,11 @@ use conduit::{Handler, HandlerResult, RequestExt};
 use conduit_router::{RequestParams, RouteBuilder};
 
 use crate::controllers::*;
-use crate::util::errors::{std_error, AppError, NotFound};
+use crate::util::errors::{std_error, AppError};
 use crate::util::EndpointResult;
 use crate::{App, Env};
 
-pub fn build_router(app: &App) -> R404 {
+pub fn build_router(app: &App) -> RouteBuilder {
     let mut api_router = RouteBuilder::new();
 
     // Route used by both `cargo search` and the frontend
@@ -103,7 +103,7 @@ pub fn build_router(app: &App) -> R404 {
         C(user::me::regenerate_token_and_send),
     );
     api_router.get("/site_metadata", C(site_metadata::show_deployed_sha));
-    let api_router = Arc::new(R404(api_router));
+    let api_router = Arc::new(api_router);
 
     let mut router = RouteBuilder::new();
 
@@ -133,7 +133,7 @@ pub fn build_router(app: &App) -> R404 {
         router.post("/git/index/*path", R(s));
     }
 
-    R404(router)
+    router
 }
 
 struct C(pub fn(&mut dyn RequestExt) -> EndpointResult);
@@ -166,23 +166,6 @@ impl<H: Handler> Handler for R<H> {
     }
 }
 
-// Can't derive Debug because of RouteBuilder.
-#[allow(missing_debug_implementations)]
-pub struct R404(pub RouteBuilder);
-
-impl Handler for R404 {
-    fn call(&self, req: &mut dyn RequestExt) -> HandlerResult {
-        let R404(ref router) = *self;
-        match router.recognize(&req.method(), req.path()) {
-            Ok(m) => {
-                req.mut_extensions().insert(m.params().clone());
-                m.handler().call(req)
-            }
-            Err(..) => Ok(NotFound.into()),
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

src/util/errors/json.rs

@@ -14,11 +14,11 @@ fn json_error(detail: &str, status: StatusCode) -> AppResponse {
     }
     #[derive(Serialize)]
     struct Bad<'a> {
-        errors: Vec<StringError<'a>>,
+        errors: [StringError<'a>; 1],
     }
 
     let mut response = json_response(&Bad {
-        errors: vec![StringError { detail }],
+        errors: [StringError { detail }],
     });
     *response.status_mut() = status;
     response