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

Allow routes that match any method #2731

Open
2 tasks done
palant opened this issue Feb 19, 2024 · 4 comments
Open
2 tasks done

Allow routes that match any method #2731

palant opened this issue Feb 19, 2024 · 4 comments
Labels
request Request for new functionality

Comments

@palant
Copy link

palant commented Feb 19, 2024

What's missing?

There are some scenarios which currently require cloning a route multiple times to make sure each and every method is accepted. For me these scenarios are at the very least:

  • HTTP -> HTTPS redirector
  • Proxying requests to another server (not redirecting because it isn’t accessible from the web)
  • Internal redirects (modifying a request and dispatching it, making sure it’s processed elsewhere in the app)

Ideal Solution

From the look of it, the following changes should suffice:

  • Change type of Route::method into MethodSet, an alias for enumset<Method>. This should allow most of the existing code to work without changes.
  • Add #[any(…)] attribute macro – essentially identical to #[get(…)] but setting Route::method to MethodSet::all().
  • Modify #[route(…)] attribute macro to accept multiple methods: #[route(GET | POST, …)].
  • Modify #[route(…)] attribute macro to accept the special keyword ANY as method parameter meaning MethodSet::all(). An alternative approach would making the method optional here and default to MethodSet::all() but personally I’d rather have things explicit.
  • Modify Route::matches() implementation, this change is trivial. From what I can tell, Rocket doesn’t optimize route matching, so the only changes required otherwise should be some logging code.

I should be able to create a PR if this approach is approved.

Why can't this be implemented outside of Rocket?

Creating attribute macros to mimic those built into Rocket would require considerable complexity (duplication of logic). Also, it would still require creating an excessive number of routes in this scenario.

Are there workarounds usable today?

This limitation can certainly be worked around, see redirector in the TLS example in this repository. The work-around there is heavy on boilerplate however instead of using the usual descriptive approach of Rocket.

My work-around does slightly better:

#[rocket::get("/<_..>")]
async fn redirect(
    config: &State<Config>,
    host: &Host<'_>,
    uri: &Origin<'_>,
) -> Result<Redirect, Status> {}

let redirects = [Get, Put, Post, Delete, Options, Head, Trace, Connect, Patch]
    .into_iter()
    .map(|m| {
        let mut route: Route = rocket::routes![redirect].remove(0);
        route.method = m;
        route
    })
    .collect::<Vec<_>>();

However, this still creates nine routes instead of one. If nothing else, this has an impact on performance.

Also, this looks like a route for the GET method, simply because I have to specify some method. This makes code harder to read, the intent behind this route is no longer obvious on the first glance and has to be deduced from context.

Alternative Solutions

Rather than working with sets of methods, it would be possible to define a special Any method. The result would be largely the same, only without routes supporting a subset of all methods.

Additional Context

No response

System Checks

  • I do not believe that this feature can or should be implemented outside of Rocket.
  • I was unable to find a previous request for this feature.
@palant palant added the request Request for new functionality label Feb 19, 2024
@SergioBenitez
Copy link
Member

This has been requested in the past, but there hasn't been much support behind the request. It seems rare that one needs this feature, and when it is needed it's so one-off, and the work-arounds easy-enough, that it outweighs the complexity to implement the feature.

I should note that there is no performance impact caused by the work-around beyond creating the multiple routes. The router indexes by method, so there's no additional cost when the application is running.

As to actually doing this, I'm not opposed to the idea, but we'd need a way for Rocket to continue to statically check that only payload-bearing methods are allowed to have a data component. The most elegant approach would probably be to allow a data attribute iff all of the methods in the set support a payload. That might work. What do you think?

@SergioBenitez
Copy link
Member

Oh, one last thing to note: one thing I'd like Rocket to do in the near future is to allow non-standard HTTP methods in addition to the current set of standard methods. Ideally whatever implementation we choose here wouldn't need to be rethought to support non-standard methods.

@palant
Copy link
Author

palant commented Feb 20, 2024

Oh, one last thing to note: one thing I'd like Rocket to do in the near future is to allow non-standard HTTP methods in addition to the current set of standard methods. Ideally whatever implementation we choose here wouldn't need to be rethought to support non-standard methods.

Yes, I’ve seen this issue. Unfortunately, I don’t see how this can be made to work with enumset. So it’s either Any as a special wildcard “method” (and no arbitrary combinations of methods) or making the method field a HashSet<String>. The latter would require more significant code changes and have some impact on memory usage. Whether it is worth it depends on whether you consider combinations like GET | POST worth implementing – they aren’t necessary for my use case.

I’d also have to take a closer look at routing, if you say that it is indexed by method then I apparently missed something. There is the question of how it can process the scenario where matching routes exist for both the specific method and “any.” Should it always take the route for the specific method? Or maybe go by rank, so that there can be a “default” GET route invoked when everything else fails?

The most elegant approach would probably be to allow a data attribute iff all of the methods in the set support a payload.

I’d rather say: iff any of the methods in the set support a payload. Proxying and internal redirects generally need to forward data if present, while supporting whatever methods there could be. Yes, weaker static guarantees in this case.

Fun fact: I did encounter web applications using GET requests with a payload.

@palant
Copy link
Author

palant commented Feb 21, 2024

I found Router::route method which I missed before, this one will have to be adjusted as well. And here it is in fact easier to have Method::Any as a wildcard, arbitrary combinations of methods are much more complex and not worth the effort. Eventually, there will also be Method::Custom(String) but this won’t really change much.

Altogether, the required changes seem to be:

  • Add Method::Any enum value and #[any(…)] attribute macro that will create a route with this “method.” Also modify #[route(…)] attribute macro to allow ANY as method.
  • Modify Route::matches() implementation: if method is Any, any method parameter is accepted.
  • Modify Router::route() implementation: it needs to retrieve the list of routes both for the actual method and for Method::Any, then merge both iterators. As both are sorted by rank, producing a merged iterator that will keep the sorting is fairly simple.

I think that’s it.

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

No branches or pull requests

2 participants