Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove wrong debug_assert in MatchedPath #1585

Closed
wants to merge 1 commit into from

Conversation

davidpdrsn
Copy link
Member

Fixes #1579


let res = client.get("/foobar").send().await;
assert_eq!(res.status(), StatusCode::OK);
assert_eq!(res.text().await, "/*path/foobar");
Copy link
Member Author

@davidpdrsn davidpdrsn Nov 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what the matched path logically should be. Feels weird saying it is /*path/foobar since the request is just /foobar, so where does /*path come from?

However it kinda matches the way the router is setup 🤔

Regardless it shouldn't panic 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@davidpdrsn davidpdrsn Nov 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think a more correct router setup would be

async fn handler(req: Request<Body>) -> Response {
    // could be cached in a `State`
    let inner = Router::new().route(
        "/foobar",
        get(|matched_path: MatchedPath| async move { matched_path.as_str().to_owned() }),
    );

    // route based on `Host` or whatever else you need
    Router::new().nest("/", inner).oneshot(req).await.unwrap()
}

// run it with
handler.into_make_service();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally like this

I'm using axum to make some framework like

pub struct App {
    pub cfg: Arc<GatewayConfig>,
}

impl Application for App {
    fn frame_cfg(&self) -> &ApiFrameConfig {
        &self.cfg.frame_cfg
    }

    fn router(&self) -> Router {
        Router::new()
            .route("/", get(peace_api::responder::app_root))
            .nest("/bancho", bancho::routers::bancho_client_routes())
    }

    fn apidocs(&self) -> utoipa::openapi::OpenApi {
        GatewayApiDocs::openapi()
    }

    fn match_hostname(
        &self,
        Host(hostname): Host,
        req: &Request<Body>,
    ) -> Option<Router> {
        match hostname {
            n if self.cfg.bancho_hostname.contains(&n) => {
                Some(bancho::routers::bancho_client_routes())
            },
            _ => None,
        }
    }
}

#[tokio::main]
pub async fn main() {
    let app = App::new(GatewayConfig::get());
    http::serve(app).await;
}

In general a reverse proxy can be used to direct the request to a certain path, which may be the router of a certain service

I would like to allow users to enable both general router and hostname based router. This may need to use /*path

possible to reuse some routers

@davidpdrsn
Copy link
Member Author

Superseded by #1935

@davidpdrsn davidpdrsn closed this Apr 14, 2023
@davidpdrsn davidpdrsn deleted the fix-matched-path-panic branch April 14, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange debug_assert! and runtime panics
2 participants