Implement parameterized routes

[akatechis]

This PR introduces the ability to parameterize routes using the syntax /foo/:id/bar/:id. In this syntax, the name of a parameter is not important, since parameters are captures as a Vec<String> wrapped in a RouteParameters struct, and inserted into the request's extensions map.

This PR also simplifies the external API the crate provides, by removing the Builder structs, and letting the user just create Routes and Routers directly. Also, the RouterService's API has been simplified to have a single find_handler() method that returns (Handler, RouteParameters). When the request does not match any route, the handler that is returned is the not_found handler the router has been configured with, and if the request matches, but the method is not supported, it returns the method_not_supported handler from the Router. Similarly, if the path is static, the RouteParameters struct that is returned just wraps an empty Vec<String>.

One thing that I'm not too sure of is a single failing test: test_parametric_path_with_variable_in_last_position_matches_multiple_segments_as_one. I'm not sure if this is expected behavior that we should implement or if the test should be removed, so I'd like some feedback on this point.

I've removed most of the docs as this PR has rewritten a lot of the functionality, and I'd like to get feedback on it before I go in and document everything from scratch :)

Edit: This PR fixes #19

[akatechis]

One thing that occurred to me just now regarding the RouteParameters struct is that I used that to create a new type to represent the params within the extensions map (since values are keyed by their type, you can only have 1 value per type, I didn't want to "take" Vec<String> for parameters).

One improvement we can try to make is to flatten that type into simply Vec<RouteParameter>, which can then be an enum type over a few different types like String, uNN, fNN, etc. We can then encode that parsing logic into the route as "/foo/:u32" -> "foo/123".

[akatechis]

One thing that occurred to me just now regarding the RouteParameters struct is that I used that to create a new type to represent the params within the extensions map (since values are keyed by their type, you can only have 1 value per type, I didn't want to "take" Vec<String> for parameters).

One improvement we can try to make is to flatten that type into simply Vec<RouteParameter>, which can then be an enum type over a few different types like String, uNN, fNN, etc. We can then encode that parsing logic into the route as "/foo/:u32" -> "foo/123".

Upon further consideration, I think parsing/marshaling parameters into more complex data types should be handled by users, since hyper is intended to be relatively lower level than something like actix-web/rocket/etc.

[marad]

I was thinking more along the lines of just collecting the path params and query params to hashmaps (<String, String> and <String, Vec> respectively).

I don't think I'd like to introduce any further parsing of this arguments as this introduces a layer of complexity that I don't want to deal with in this simple library.

I also don't approve the API simplifications that you made. As I said in #19 - any API changes must be carefully designed and thought through, so I'd rather have two separate PRs for both proposed changes, where we could discuss each proposal alone. Having this in one PR just makes it hard.

[akatechis]

Thanks for the feedback. I appreciate you taking the time to look through my PR.

I was thinking more along the lines of just collecting the path params and query params to hashmaps (<String, String> and <String, Vec> respectively).

I don't think I'd like to introduce any further parsing of this arguments as this introduces a layer of complexity that I don't want to deal with in this simple library.

Makes sense. My rationale was something along the following: The Extensions struct of a request looks like specifically designed for this sort of thing: "Extensions can be used by Request and Response to store extra data derived from the underlying protocol." docs, but I thought that since you can only have a single value per type, it would be better to not "take" something as basic as Vec<String> in case a user wants to use that for something else, so I used a new type to make it more explicit. If you believe that is not as big a concern as I seem to think, I can definitely change it to be just the basic type, rather than the newtype. The same decision can be used when query parameter capture is introduced.

I also don't approve the API simplifications that you made. As I said in #19 - any API changes must be carefully designed and thought through, so I'd rather have two separate PRs for both proposed changes, where we could discuss each proposal alone. Having this in one PR just makes it hard.

Fair enough. I interpreted your response there to mainly mean that a RESTful interface specifically was something we should avoid, rather than something more basic like what I did here. That said, I can absolutely separate the API changes into a separate PR and go into more detail as to why I think they're a positive change.

Thanks!

[marad]

but I thought that since you can only have a single value per type, it would be better to not "take" something as basic as Vec in case a user wants to use that for something else, so I used a new type to make it more explicit. If you believe that is not as big a concern as I seem to think, I can definitely change it to be just the basic type, rather than the newtype. The same decision can be used when query parameter capture is introduced.

Hmm, the extensions seem to be a good place for the data (but I didn't do a lot of research on this :)). Also creating dedicated type(s?) to store the query params and path params was a good idea. I was only talking about managing the types of path params (ie. /orders/:u32/status). I'd rather have it in a hash map. So (/orders/:id/status, and then it would land in a Hashmap like "id" -> "1234".

When it comes to the library API changes - I'd like another pull request where we could discuss it.

[akatechis]

Hi @marad, apologies for the radio silence the past few days, but I think I'm ready for another review. I've reverted the API back to what it currently looks like, and made it simply wrap the new internal implementation.

I added a couple of benchmark tests, and tried a couple of different implementations, one which splits the route into segments at construction time, and matches them at request time, and another which just does character-by-character matching/capture. Neither of these was a clear winner over the other, so I just went with my initial implementation: splitting ahead of time, and left the character-by-character implementation in the feat/optimize-capture (https://github.com/akatechis/hyper-router/tree/optimize-capture) branch of my own repo.

If everything looks good, let me know, and I can add the documentation back in (and update it to demonstrate the new capture functionality).

[marad]

Hey @akatechis I'll look into in during the weekend, thanks for your work :)

[seanpianka]

Any updates or plans on merging this?

[akatechis]

Hi @seanpianka, as I have not heard back from @marad on this, I presume it's on hold?

[marad]

@seanpianka @akatechis Sorry for my lack of responses!
I guess there is never a good time to do this review so I guess I'll try to do this now.
I'm not very up to date with Rust now so it might be a while ;)

[marad]

Why did you remove the documentation from the lib?

[akatechis]

I don't recall 100% why I removed it, since it's been a while since I looked at this PR. IIRC, I was moving stuff around different modules, and so to make it easier, I removed the docs, and was thinking of waiting for the dust to settle before adding the docs back in, and possibly updating them in light of my additions.

I wonder if it might make sense to close this PR, and start from scratch (or I can salvage bits and pieces from this), maybe you can tell me how you envision this functionality working, and we can go from there, with everything fresh in both our minds? 🤣

[marad]

Wouldn't it be a waste of your work? This PR looks quite good. Would you approach this differently now?

[seanpianka]

@seanpianka @akatechis Sorry for my lack of responses!
I guess there is never a good time to do this review so I guess I'll try to do this now.
I'm not very up to date with Rust now so it might be a while ;)

No worries @marad, just curious to see if this project was active!