-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Comments
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 |
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 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?
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. |
I found Altogether, the required changes seem to be:
I think that’s it. |
I've implemented a version of this in the wildcard-method branch. It still needs testing (and tests), and I'm not satisfied with the memory trade-off currently implemented in the router. To support this properly, we need to make the router a bit more sophisticated. The tests that need to be added are those that:
Furthermore, we should run the fuzzer which checks the required matcher/collision properties (#2790 (comment)). |
Hi there... I'm looking for this same functionality, a way to match any method for a specified URI. |
@andyolivares I'll see if I can get to it later this week. |
This commit introduces support for method-less routes and route attributes, which match _any_ valid method: `#[route("/")]`. The `Route` structure's `method` field is now accordingly of type `Option<Route>`. The syntax for the `route` attribute has changed in a breaking manner. To set a method, a key/value of `method = NAME` must be introduced: ```rust #[route("/", method = GET)] ``` If the method's name is a valid identifier, it can be used without quotes. Otherwise it must be quoted: ```rust // `GET` is a valid identifier, but `VERSION-CONTROL` is not #[route("/", method = "VERSION-CONTROL")] ``` Closes #2731.
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:
Ideal Solution
From the look of it, the following changes should suffice:
Route::method
intoMethodSet
, an alias forenumset<Method>
. This should allow most of the existing code to work without changes.#[any(…)]
attribute macro – essentially identical to#[get(…)]
but settingRoute::method
toMethodSet::all()
.#[route(…)]
attribute macro to accept multiple methods:#[route(GET | POST, …)]
.#[route(…)]
attribute macro to accept the special keywordANY
as method parameter meaningMethodSet::all()
. An alternative approach would making the method optional here and default toMethodSet::all()
but personally I’d rather have things explicit.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:
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
The text was updated successfully, but these errors were encountered: