gotham-rs / gotham

A flexible web framework that promotes stability, safety, security and speed.
https://gotham.rs
Other
2.24k stars 125 forks source link

Path extractors don't check path strings have correct names #145

Open illicitonion opened 6 years ago

illicitonion commented 6 years ago

I wrote the following code:

fn router() -> gotham::router::Router {
    gotham::router::builder::build_simple_router(|route| {
        route.get("/feed/:id").with_path_extractor::<FeedPath>().to(feed);
    })  
}

#[derive(Deserialize, StateData, StaticResponseExtender)]
struct FeedPath {
    identifier: String,
}

The error here is that in my path string I used the name "id" but in my struct I used the name "identifier".

What happened was that my code compiled fine, but whenever I tried to hit my endpoint I got a 400.

Given how much Gotham advertises itself on its strong static typing, I would love it if at compile time this could be detected, rather than being a runtime error. I'm aware that the format! macro does some cute magic where if you have a mismatch between the number of format placeholders, and arguments passed (e.g. format!("{}")), it gives a useful compile-time error.

Is such a thing possible here? If not, would it be possible if a macro were used to define routes, instead of a method call?

smangelsdorf commented 6 years ago

Given how much Gotham advertises itself on its strong static typing, I would love it if at compile time this could be detected, rather than being a runtime error.

Me too. 😁 Thanks for raising this, @illicitonion. The way it currently works is imperfect for exactly the reasons you've described. We can't claim to catch all invalid runtime behaviour at compile time, but that's a future I'd love to see for Rust and Gotham.

My understanding of modern type systems is that tackling something like this without macros would require dependent types, since the type information would need to come from the route path. I'm open to any suggestions about how to do this without macros, if it's possible in stable Rust.

I'm aware that the format! macro does some cute magic where if you have a mismatch between the number of format placeholders, and arguments passed (e.g. format!("{}")), it gives a useful compile-time error.

The format! macro is implemented as a compiler built-in, so it isn't bound by the same rules that libraries have to play by when writing macros. When I briefly experimented with having a macro dive into a string for exactly this reason, I found that the compiler had a very deep-seated preference for treating the string as an expression, and that trying to go against the grain caused a lot of problems.

Since the current stable macro system (ignoring custom derives) works basically as a set of AST rewrite rules, it doesn't lend itself to compile-time parsing of the route path.

Is such a thing possible here? If not, would it be possible if a macro were used to define routes, instead of a method call?

I've resigned myself to waiting for Macros 2.0 before trying to do anything to deal with this. I'd love to see a proof of concept in stable Rust, though, if somebody knows how to tackle this now.

bradleybeddoes commented 6 years ago

There are probably a number of reasons why this is a bad idea and I've spent about 1 minute considering it so please feel free to stomp on it.

However it just struck me that one idea to get us half way home, i.e. not properly type safe, is using a macro (via a derive) to implement a method that will pull out all the field names of the target path extraction struct. With that we could do a compare to the tokens we've pulled out of the path itself, at least ensuring a 1:1 mapping of names.

smangelsdorf commented 6 years ago

Anything that causes a compile error when they don't match up would be fine in my opinion (we can worry about error ergonomics another day). It's roughly the idea I was trying to tackle when I tried to do it with Macros 1.0, but as I mentioned the string couldn't be pulled apart easily.

Getting a compile error to happen by inspecting the (potentially private) fields of a type sounds tricky. Worth exploring whether we could do it (maybe we'd need to bring back the PathExtractor derive on the target type).

lpil commented 5 years ago

Perhaps it could be easier to work with a path macro rather than a path string, similar to how warp does (see an example in the README here https://github.com/seanmonstar/warp/blob/9eb4bb49789e18b6241c846095c0d0cb29265d7c/README.md#example)

// Gotham
"/users/:id/profile"

// Warp-like
path!("users" / id / "profile")
colinbankier commented 5 years ago

Thanks for the example @lpil. I this looks like a slightly different approach than initially investigated - perhaps this gets around some of those initial concerns. Certainly happy to accept any PRs if anyone wants to experiment with this.