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

Better support for overlapping parameters #13

Closed
ibraheemdev opened this issue Dec 30, 2021 · 8 comments · Fixed by #37
Closed

Better support for overlapping parameters #13

ibraheemdev opened this issue Dec 30, 2021 · 8 comments · Fixed by #37
Labels
feature New feature or request

Comments

@ibraheemdev
Copy link
Owner

Currently routes like /:a/:b and /:a are supported, but only if they have a common prefix (/:a). This means a route like /:a/:b conflicts with /:c. We can support this by normalizing route parameters positionally when they are first inserted. /:user/:id would become /:a/:b, and /:id becomes /:a. Param nodes get an additional field representing the user-defined parameter name, which is used to construct the parameter list.

@BratSinot
Copy link

Also, another variant:

use axum::{
    routing::{get, post},
    Router,
};

fn main() {
    /* these are configurable */
    let base_path = "/";
    let get_base_path = "/";
    let post_base_path = "/";

    let get_route = Router::new().nest(
        get_base_path,
        Router::new().route("/get", get(|| std::future::ready("Ho".to_owned()))),
    );
    let post_route = Router::new().nest(
        post_base_path,
        Router::new().route("/post", post(|| std::future::ready("Ho".to_owned()))),
    );
    let app: Router<String> =
        Router::new().nest(base_path, Router::new().merge(get_route).merge(post_route));
}

cause this error:

thread 'main' panicked at 'Invalid route "/": insertion failed due to conflict with previously registered route: /*__private__axum_nest_tail_param', src/main.rs:21:70
stack backtrace:

@ibraheemdev
Copy link
Owner Author

@BratSinot I'm not 100% familiar with how axum uses matchit internally, but that looks more like an axum issue than a matchit one. Maybe open an axum issue?

@BratSinot
Copy link

@BratSinot I'm not 100% familiar with how axum uses matchit internally, but that looks more like an axum issue than a matchit one. Maybe open an axum issue?

Ow, sorry! I came here from axum issues list =)

@ibraheemdev
Copy link
Owner Author

I realized this is more complicated because the name of the parameter would change depending on the rest of the path, which hasn't been parsed yet. There's another approach that could work by storing a remapping at the end of each route, but I'm starting to think it's not worth handling this edge case.

@ibraheemdev
Copy link
Owner Author

Storing the remapping at leaf nodes turned out to be simpler than expected. I've implemented it in #37, this should be released as part of 0.8.0.

@ibraheemdev ibraheemdev mentioned this issue Aug 3, 2023
3 tasks
@davidpdrsn
Copy link

davidpdrsn commented Aug 4, 2023

Great news! Thanks for looking into it.

Any change you would back port that to 0.7 as well? I think axum users would really appreciate that :)

@ibraheemdev
Copy link
Owner Author

Released in 0.7.3 🎉

@davidpdrsn
Copy link

Great news! Thanks for fixing it 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants