Return a 404 when paring the route fails (#3860)

* return a 404 when paring the route fails

* add a playwright test for fullstack routing status codes

* Fix new playwright test

* document SSRError

* Make SSRError public and fix clippy

* Move the error display bound to the routable trait

* fix clippy

* return a more generic 404 message

* fix router tests

Cargo.lock

@@ -3921,6 +3921,7 @@ dependencies = [
  "dioxus-isrg",
  "dioxus-lib",
  "dioxus-mobile",
+ "dioxus-router",
  "dioxus-ssr",
  "dioxus-web",
  "dioxus_server_macro",
@@ -4123,6 +4124,15 @@ dependencies = [
  "tokio",
 ]
 
+[[package]]
+name = "dioxus-playwright-fullstack-routing-test"
+version = "0.1.0"
+dependencies = [
+ "dioxus",
+ "serde",
+ "tokio",
+]
+
 [[package]]
 name = "dioxus-playwright-fullstack-test"
 version = "0.1.0"
@@ -15400,9 +15410,9 @@ checksum = "1e9df38ee2d2c3c5948ea468a8406ff0db0b29ae1ffde1bcf20ef305bcc95c51"
 
 [[package]]
 name = "wry"
-version = "0.50.3"
+version = "0.50.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d2ec139df5102db821f92a42033c3fa0467c5ab434511e79c65881d6bdf2b369"
+checksum = "804a7d1613bd699beccaa60f3b3c679acee21cebba1945a693f5eab95c08d1fa"
 dependencies = [
  "base64 0.22.1",
  "block2 0.6.0",

Cargo.toml

@@ -98,6 +98,7 @@ members = [
     "packages/playwright-tests/web",
     "packages/playwright-tests/fullstack",
     "packages/playwright-tests/fullstack-mounted",
+    "packages/playwright-tests/fullstack-routing",
     "packages/playwright-tests/suspense-carousel",
     "packages/playwright-tests/nested-suspense",
     "packages/playwright-tests/cli-optimization",

packages/core/src/error_boundary.rs

@@ -3,7 +3,7 @@ use crate::{
     Properties, ScopeId, Template, TemplateAttribute, TemplateNode, VNode,
 };
 use std::{
-    any::{Any, TypeId},
+    any::Any,
     backtrace::Backtrace,
     cell::{Ref, RefCell},
     error::Error,
@@ -493,11 +493,7 @@ impl Display for CapturedError {
 impl CapturedError {
     /// Downcast the error type into a concrete error type
     pub fn downcast<T: 'static>(&self) -> Option<&T> {
-        if TypeId::of::<T>() == (*self.error).type_id() {
         self.error.as_any().downcast_ref::<T>()
-        } else {
-            None
-        }
     }
 }
 

packages/fullstack/Cargo.toml

@@ -25,6 +25,7 @@ generational-box = { workspace = true }
 # Dioxus + SSR
 dioxus-ssr = { workspace = true, optional = true }
 dioxus-isrg = { workspace = true, optional = true }
+dioxus-router = { workspace = true, optional = true }
 hyper = { workspace = true, optional = true }
 http = { workspace = true, optional = true }
 
@@ -96,6 +97,7 @@ server = [
     "dep:tokio-stream",
     "dep:dioxus-ssr",
     "dep:dioxus-isrg",
+    "dep:dioxus-router",
     "dep:tower",
     "dep:hyper",
     "dep:http",

packages/fullstack/src/render.rs

@@ -6,6 +6,7 @@ use dioxus_cli_config::base_path;
 use dioxus_interpreter_js::INITIALIZE_STREAMING_JS;
 use dioxus_isrg::{CachedRender, IncrementalRendererError, RenderFreshness};
 use dioxus_lib::document::Document;
+use dioxus_router::prelude::ParseRouteError;
 use dioxus_ssr::Renderer;
 use futures_channel::mpsc::Sender;
 use futures_util::{Stream, StreamExt};
@@ -50,6 +51,14 @@ where
     }
 }
 
+/// Errors that can occur during server side rendering before the initial chunk is sent down
+pub enum SSRError {
+    /// An error from the incremental renderer. This should result in a 500 code
+    Incremental(IncrementalRendererError),
+    /// An error from the dioxus router. This should result in a 404 code
+    Routing(ParseRouteError),
+}
+
 struct SsrRendererPool {
     renderers: RwLock<Vec<Renderer>>,
     incremental_cache: Option<RwLock<dioxus_isrg::IncrementalRenderer>>,
@@ -112,7 +121,7 @@ impl SsrRendererPool {
             RenderFreshness,
             impl Stream<Item = Result<String, dioxus_isrg::IncrementalRendererError>>,
         ),
-        dioxus_isrg::IncrementalRendererError,
+        SSRError,
     > {
         struct ReceiverWithDrop {
             receiver: futures_channel::mpsc::Receiver<
@@ -145,6 +154,8 @@ impl SsrRendererPool {
             Result<String, dioxus_isrg::IncrementalRendererError>,
         >(1000);
 
+        let (initial_result_tx, initial_result_rx) = futures_channel::oneshot::channel();
+
         // before we even spawn anything, we can check synchronously if we have the route cached
         if let Some(freshness) = self.check_cached_route(&route, &mut into) {
             return Ok((
@@ -188,7 +199,7 @@ impl SsrRendererPool {
             virtual_dom.provide_root_context(Rc::new(history) as Rc<dyn dioxus_history::History>);
             virtual_dom.provide_root_context(document.clone() as std::rc::Rc<dyn Document>);
 
-            // poll the future, which may call server_context()
+            // rebuild the virtual dom, which may call server_context()
             with_server_context(server_context.clone(), || virtual_dom.rebuild_in_place());
 
             // If streaming is disabled, wait for the virtual dom to finish all suspense work
@@ -197,6 +208,41 @@ impl SsrRendererPool {
                 ProvideServerContext::new(virtual_dom.wait_for_suspense(), server_context.clone())
                     .await
             }
+            // check if there are any errors
+            let errors = with_server_context(server_context.clone(), || {
+                virtual_dom.in_runtime(|| {
+                    let error_context: ErrorContext = ScopeId::APP
+                        .consume_context()
+                        .expect("The root should be under an error boundary");
+                    let errors = error_context.errors();
+                    errors.to_vec()
+                })
+            });
+            if errors.is_empty() {
+                // If routing was successful, we can return a 200 status and render into the stream
+                _ = initial_result_tx.send(Ok(()));
+            } else {
+                // If there was an error while routing, return the error with a 400 status
+                // Return a routing error if any of the errors were a routing error
+                let routing_error = errors.iter().find_map(|err| err.downcast().cloned());
+                if let Some(routing_error) = routing_error {
+                    _ = initial_result_tx.send(Err(SSRError::Routing(routing_error)));
+                    return;
+                }
+                #[derive(thiserror::Error, Debug)]
+                #[error("{0}")]
+                pub struct ErrorWhileRendering(String);
+                let mut all_errors = String::new();
+                for error in errors {
+                    all_errors += &error.to_string();
+                    all_errors += "\n"
+                }
+                let error = ErrorWhileRendering(all_errors);
+                _ = initial_result_tx.send(Err(SSRError::Incremental(
+                    IncrementalRendererError::Other(Box::new(error)),
+                )));
+                return;
+            }
 
             let mut pre_body = String::new();
 
@@ -325,6 +371,11 @@ impl SsrRendererPool {
             myself.renderers.write().unwrap().push(renderer);
         });
 
+        // Wait for the initial result which determines the status code
+        initial_result_rx.await.map_err(|err| {
+            SSRError::Incremental(IncrementalRendererError::Other(Box::new(err)))
+        })??;
+
         Ok((
             RenderFreshness::now(None),
             ReceiverWithDrop {
@@ -447,7 +498,7 @@ impl SSRState {
             RenderFreshness,
             impl Stream<Item = Result<String, dioxus_isrg::IncrementalRendererError>>,
         ),
-        dioxus_isrg::IncrementalRendererError,
+        SSRError,
     > {
         self.renderers
             .clone()

packages/fullstack/src/server/mod.rs

@@ -69,6 +69,7 @@ use http::header::*;
 
 use std::sync::Arc;
 
+use crate::render::SSRError;
 use crate::{prelude::*, ContextProviders};
 
 /// A extension trait with utilities for integrating Dioxus with your Axum router.
@@ -413,10 +414,17 @@ pub async fn render_handler(
             apply_request_parts_to_response(headers, &mut response);
             Result::<http::Response<axum::body::Body>, StatusCode>::Ok(response)
         }
-        Err(e) => {
+        Err(SSRError::Incremental(e)) => {
             tracing::error!("Failed to render page: {}", e);
             Ok(report_err(e).into_response())
         }
+        Err(SSRError::Routing(e)) => {
+            tracing::trace!("Page not found: {}", e);
+            Ok(Response::builder()
+                .status(StatusCode::NOT_FOUND)
+                .body(Body::from("Page not found"))
+                .unwrap())
+        }
     }
 }
 

packages/isrg/src/lib.rs

@@ -156,7 +156,7 @@ pub enum IncrementalRendererError {
     /// An IO error occurred while rendering a route.
     #[error("IoError: {0}")]
     IoError(#[from] std::io::Error),
-    /// An IO error occurred while rendering a route.
+    /// An error occurred while rendering a route.
     #[error("Other: {0}")]
     Other(#[from] Box<dyn std::error::Error + Send + Sync>),
 }

packages/playwright-tests/fullstack-routing.spec.js

@@ -0,0 +1,53 @@
+// @ts-check
+const { test, expect } = require("@playwright/test");
+
+// Wait for the build to finish
+async function waitForBuild(request) {
+  for (let i = 0; i < 10; i++) {
+    const build = await request.get("http://localhost:8888");
+    let text = await build.text();
+    if (!text.includes("Backend connection failed")) {
+      return;
+    }
+    await new Promise((r) => setTimeout(r, 1000));
+  }
+}
+
+// The home and id routes should return 200
+test("home route", async ({ request }) => {
+  await waitForBuild(request);
+  const response = await request.get("http://localhost:8888");
+
+  expect(response.status()).toBe(200);
+
+  const text = await response.text();
+  expect(text).toContain("Home");
+});
+
+test("blog route", async ({ request }) => {
+  await waitForBuild(request);
+  const response = await request.get("http://localhost:8888/blog/123");
+
+  expect(response.status()).toBe(200);
+
+  const text = await response.text();
+  expect(text).toContain("id: 123");
+});
+
+// The error route should return 500
+test("error route", async ({ request }) => {
+  await waitForBuild(request);
+  const response = await request.get("http://localhost:8888/error");
+
+  expect(response.status()).toBe(500);
+});
+
+// An unknown route should return 404
+test("unknown route", async ({ request }) => {
+  await waitForBuild(request);
+  const response = await request.get(
+    "http://localhost:8888/this-route-does-not-exist"
+  );
+
+  expect(response.status()).toBe(404);
+});

packages/playwright-tests/fullstack-routing/.gitignore

@@ -0,0 +1,3 @@
+.dioxus
+dist
+target

packages/playwright-tests/fullstack-routing/Cargo.toml

@@ -0,0 +1,17 @@
+[package]
+name = "dioxus-playwright-fullstack-routing-test"
+version = "0.1.0"
+edition = "2021"
+publish = false
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+dioxus = { workspace = true, features = ["fullstack", "router"] }
+serde = "1.0.159"
+tokio = { workspace = true, features = ["full"], optional = true }
+
+[features]
+default = []
+server = ["dioxus/server", "dep:tokio"]
+web = ["dioxus/web"]

packages/playwright-tests/fullstack-routing/src/main.rs

@@ -0,0 +1,54 @@
+// This test is used by playwright configured in the root of the repo
+// Tests:
+// - 200 Routes
+// - 404 Routes
+// - 500 Routes
+
+#![allow(non_snake_case)]
+use dioxus::{prelude::*, CapturedError};
+
+fn main() {
+    dioxus::LaunchBuilder::new()
+        .with_cfg(server_only! {
+            dioxus::fullstack::ServeConfig::builder().enable_out_of_order_streaming()
+        })
+        .launch(app);
+}
+
+fn app() -> Element {
+    rsx! { Router::<Route> {} }
+}
+
+#[derive(Clone, Routable, Debug, PartialEq, serde::Serialize, serde::Deserialize)]
+enum Route {
+    #[route("/")]
+    Home,
+
+    #[route("/blog/:id/")]
+    Blog { id: i32 },
+
+    #[route("/error")]
+    ThrowsError,
+}
+
+#[component]
+fn Blog(id: i32) -> Element {
+    rsx! {
+        Link { to: Route::Home {}, "Go home" }
+        "id: {id}"
+    }
+}
+
+#[component]
+fn ThrowsError() -> Element {
+    return Err(RenderError::Aborted(CapturedError::from_display(
+        "This route tests uncaught errors in the server",
+    )));
+}
+
+#[component]
+fn Home() -> Element {
+    rsx! {
+        "Home"
+    }
+}

packages/playwright-tests/playwright.config.js

@@ -111,6 +111,15 @@ module.exports = defineConfig({
       reuseExistingServer: !process.env.CI,
       stdout: "pipe",
     },
+    {
+      cwd: path.join(process.cwd(), "fullstack-routing"),
+      command:
+        'cargo run --package dioxus-cli --release -- serve --force-sequential --platform web --addr "127.0.0.1" --port 8888',
+      port: 8888,
+      timeout: 50 * 60 * 1000,
+      reuseExistingServer: !process.env.CI,
+      stdout: "pipe",
+    },
     {
       cwd: path.join(process.cwd(), "suspense-carousel"),
       command:

packages/router/src/components/router.rs

@@ -1,7 +1,5 @@
 use dioxus_lib::prelude::*;
 
-use std::str::FromStr;
-
 use crate::{
     prelude::{provide_router_context, Outlet},
     routable::Routable,
@@ -39,10 +37,7 @@ impl<R: Clone> PartialEq for RouterProps<R> {
 }
 
 /// A component that renders the current route.
-pub fn Router<R: Routable + Clone>(props: RouterProps<R>) -> Element
-where
-    <R as FromStr>::Err: std::fmt::Display,
-{
+pub fn Router<R: Routable + Clone>(props: RouterProps<R>) -> Element {
     use crate::prelude::{outlet::OutletContext, RouterContext};
 
     use_hook(|| {

packages/router/src/contexts/router.rs

@@ -1,5 +1,7 @@
 use std::{
     collections::HashSet,
+    error::Error,
+    fmt::Display,
     sync::{Arc, Mutex},
 };
 
@@ -11,6 +13,19 @@ use crate::{
     prelude::SiteMapSegment, routable::Routable, router_cfg::RouterConfig,
 };
 
+/// An error that is thrown when the router fails to parse a route
+#[derive(Debug, Clone)]
+pub struct ParseRouteError {
+    message: String,
+}
+
+impl Error for ParseRouteError {}
+impl Display for ParseRouteError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.message.fmt(f)
+    }
+}
+
 /// This context is set in the root of the virtual dom if there is a router present.
 #[derive(Clone, Copy)]
 struct RootRouterContext(Signal<Option<RouterContext>>);
@@ -94,10 +109,7 @@ pub struct RouterContext {
 }
 
 impl RouterContext {
-    pub(crate) fn new<R: Routable + 'static>(cfg: RouterConfig<R>) -> Self
-    where
-        <R as std::str::FromStr>::Err: std::fmt::Display,
-    {
+    pub(crate) fn new<R: Routable + 'static>(cfg: RouterConfig<R>) -> Self {
         let subscribers = Arc::new(Mutex::new(HashSet::new()));
         let mapping = consume_child_route_mapping();
 
@@ -233,15 +245,21 @@ impl RouterContext {
         let absolute_route = self.full_route_string();
         // If this is a child route, map the absolute route to the child route before parsing
         let mapping = consume_child_route_mapping::<R>();
-        match mapping.as_ref() {
+        let route = match mapping.as_ref() {
             Some(mapping) => mapping
                 .parse_route_from_root_route(&absolute_route)
-                .unwrap_or_else(|| {
-                    panic!("route's display implementation must be parsable by FromStr")
-                }),
-            None => R::from_str(&absolute_route).unwrap_or_else(|_| {
-                panic!("route's display implementation must be parsable by FromStr")
-            }),
+                .ok_or_else(|| "Failed to parse route".to_string()),
+            None => {
+                R::from_str(&absolute_route).map_err(|err| format!("Failed to parse route {err}"))
+            }
+        };
+
+        match route {
+            Ok(route) => route,
+            Err(err) => {
+                throw_error(ParseRouteError { message: err });
+                "/".parse().unwrap_or_else(|err| panic!("{err}"))
+            }
         }
     }
 

packages/router/src/lib.rs

@@ -39,7 +39,7 @@ mod contexts {
     pub(crate) mod router;
     pub use navigator::*;
     pub(crate) use router::*;
-    pub use router::{root_router, GenericRouterContext, RouterContext};
+    pub use router::{root_router, GenericRouterContext, ParseRouteError, RouterContext};
 }
 
 mod router_cfg;

packages/router/src/routable.rs

@@ -592,7 +592,7 @@ type SiteMapFlattened<'a> = FlatMap<
         note = "Routable should generally be derived using the `#[derive(Routable)]` macro."
     )
 )]
-pub trait Routable: FromStr + Display + Clone + 'static {
+pub trait Routable: FromStr<Err: Display> + Display + Clone + 'static {
     /// The error that can occur when parsing a route.
     const SITE_MAP: &'static [SiteMapSegment];
 

packages/router/src/router_cfg.rs

@@ -43,7 +43,6 @@ impl<R> Default for RouterConfig<R> {
 impl<R> RouterConfig<R>
 where
     R: Routable,
-    <R as std::str::FromStr>::Err: std::fmt::Display,
 {
     /// A function to be called whenever the routing is updated.
     ///

packages/router/tests/via_ssr/link.rs

@@ -1,19 +1,13 @@
 use dioxus::prelude::*;
 use dioxus_history::{History, MemoryHistory};
 use dioxus_router::components::HistoryProvider;
-use std::{rc::Rc, str::FromStr};
+use std::rc::Rc;
 
-fn prepare<R: Routable>() -> String
-where
-    <R as FromStr>::Err: std::fmt::Display,
-{
+fn prepare<R: Routable>() -> String {
     prepare_at::<R>("/")
 }
 
-fn prepare_at<R: Routable>(at: impl ToString) -> String
-where
-    <R as FromStr>::Err: std::fmt::Display,
-{
+fn prepare_at<R: Routable>(at: impl ToString) -> String {
     let mut vdom = VirtualDom::new_with_props(
         App,
         AppProps::<R> {
@@ -46,10 +40,7 @@ where
     }
 
     #[allow(non_snake_case)]
-    fn App<R: Routable>(props: AppProps<R>) -> Element
-    where
-        <R as FromStr>::Err: std::fmt::Display,
-    {
+    fn App<R: Routable>(props: AppProps<R>) -> Element {
         rsx! {
             h1 { "App" }
             HistoryProvider {