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

* route priority #593

Closed
spacekookie opened this issue Jun 13, 2020 · 8 comments · Fixed by #607
Closed

* route priority #593

spacekookie opened this issue Jun 13, 2020 · 8 comments · Fixed by #607
Labels
bug Something isn't working

Comments

@spacekookie
Copy link

Currently serde_dir() seems to be broken. It works if it is the only route in an app, but fails to is never routed to when it's not.

pub(crate) fn setup(app: &mut Server<StateRef>, root: PathBuf) {
    app.at("/static").serve_dir(root.join("static")).unwrap();
}

This works, but the following does not:

pub(crate) fn setup(app: &mut Server<StateRef>, root: PathBuf) {
    app.at("/:owner/:repo").get(|_| async move { Ok("Hello world!") });
    app.at("/static").serve_dir(root.join("static")).unwrap();
}
@jbr
Copy link
Member

jbr commented Jun 14, 2020

I believe this is the same issue as #595 — if you add a distinct prefix to the dynamic route, like app.at("/api/:owner/:repo").get(|_| async { Ok("hello world") });, it will route correctly

@spacekookie
Copy link
Author

spacekookie commented Jun 15, 2020 via email

@Fishrock123
Copy link
Member

Fishrock123 commented Jun 15, 2020

Does this work? That is, switching registration order?

pub(crate) fn setup(app: &mut Server<StateRef>, root: PathBuf) {
    app.at("/static").serve_dir(root.join("static")).unwrap();
    app.at("/:owner/:repo").get(|_| async move { Ok("Hello world!") });
}

at("/:param") should reasonably capture anything that is /v, /v/, but at("/:p/:p") should not match just /v or /v/ - if it does that's a bug, imo.

@jbr
Copy link
Member

jbr commented Jun 16, 2020

I've reproduced this problem and I'm not sure what the right solution is, but here's an update. Route prioritization exists in route-recognizer. (edit: I hadn't realized that this was under the http-rs umbrella when I wrote this comment, so it's possible we could change this behavior)

This issue can be reduced to the following failing test (using the test helpers in #601 for convenience):

#[async_std::test]
async fn static_routing() {
    let mut app = tide::Server::new();
    app.at("/:one/:two").get("two segments");
    app.at("/a/*").get("static + star");
    assert_eq!(app.get_body("/a/a").await, "static + star");
}

which results in

---- static_routing stdout ----
thread 'static_routing' panicked at 'assertion failed: `(left == right)`
  left: `"two segments"`,
 right: `"static + star"`', tests/wildcard.rs:127:5

Of note, route-recognizer is not order dependent, and instead uses a relative priority comparison that in this case is not what we want.

@jbr
Copy link
Member

jbr commented Jun 18, 2020

Update: This is fixed in route-recognizer master, so we just need them to publish a patch release

@jbr jbr added the bug Something isn't working label Jun 18, 2020
@jbr jbr changed the title serve_dir() broken on tide 0.11 * route priority Jun 19, 2020
@jbr jbr closed this as completed in #607 Jun 19, 2020
@jbr
Copy link
Member

jbr commented Jun 19, 2020

@spacekookie If you get a chance, could you confirm that this is fixed in tide master? Thanks!

@spacekookie
Copy link
Author

spacekookie commented Jun 21, 2020

The main branch doesn't work for me because i'm getting this error:

error: failed to select a version for `url`.
    ... required by package `serde_urlencoded v0.6.1`
    ... which is depended on by `http-types v2.2.1`
    ... which is depended on by `tide v0.11.0 (https://github.com/http-rs/tide#f8373ee1)`
    ... which is depended on by `octopus v0.1.0 (/home/projects/octopus)` versions that meet the requirements `= 2.1.0` are: 
2.1.0all possible versions conflict with previously selected packages.

  previously selected package `url v2.1.1`
    ... which is depended on by `http-types v2.2.1`
    ... which is depended on by `tide v0.11.0 (https://github.com/http-rs/tide#f8373ee1)`
    ... which is depended on by `octopus v0.1.0 (/home/projects/octopus)`

failed to select a version for `url` which could resolve this conflict

My Cargo.toml:

[package]
name = "octopus"
description = "A lightweight web frontend for git repositories"
version = "0.1.0"
authors = ["Katharina Fey <kookie@spacekookie.de>"]
edition = "2018"

[dependencies]
tide = { git = "https://github.com/http-rs/tide" }

@jbr
Copy link
Member

jbr commented Jun 21, 2020

does cargo update resolve that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants