Expose Route::matches() to allow external implementation of Options handling

[DanielJoyce]

Currently rocket requires to redefine the same route twice if you want to add options support.

Two Options

1. When you use a method annotation such as get, it also generates the option route support, it just ensures that all the method is checked for guards and will match the request, it doesn't actually call it.

or

2. Just enhance the router logic so when an Options request comes in, it checks it against all existing routes, doing exactly what it does now, but doesn't actually call the matched fn.

[DanielJoyce]

In general, making the router 'queryable' would definitely enhance it's utility for all kinds of middleware.

[SergioBenitez]

1. When you use a method annotation such as get, it also generates the option route support, it just ensures that all the method is checked for guards and will match the request, it doesn't actually call it.

Can you rephrase this? What does "that all the method is checked for guards" mean?

2. Just enhance the router logic so when an Options request comes in, it checks it against all existing routes, doing exactly what it does now, but doesn't actually call the matched fn.

This is an expensive operation, but it would be fairly straightforward to implement outside of Rocket if we exposed just a bit more. There is Route::matches() that can be used to check if a route matches a given request. All of the routes can be retrieved via Rocket::routes(). So writing a fairing that does this if we expose Request::rocket() and Route::matches() would be rather straightforward:

if this is not an options or if this was already handled {
    return;
}

let original = request.method();

let allow = &[Get, Put, Post, Delete, ..].iter()
    .filter(|method| {
        request.set_method(method);
        request.rocket()
            .routes()
            .any(|r| r.matches(request))
    })
    .map(|method| method.as_str())
    .join(", ");

request.set_method(original);

/* set the response */

Ideas on whether we should expose these two API elements, @jebrosen? My only concern is wanting to change matches() in a point release, but I suppose if we change matches we change routing, so it would have to be backwards compatible anyway.

[jebrosen]

I don't mind Route::matches(); it seems quite useful for some kinds of middleware and possibly for unit tests.

I'm a bit less happy about the idea of exposing Request::rocket() - currently it's mostly an implementation detail that Request borrows data from or has direct access to a Rocket instance. on_response could take an &Rocket, but I think that might have already been deliberately avoided in the past? So, on this point I'm less sure.

[SergioBenitez]

I'm a bit less happy about the idea of exposing Request::rocket() - currently it's mostly an implementation detail that Request borrows data from or has direct access to a Rocket instance. on_response could take an &Rocket, but I think that might have already been deliberately avoided in the past? So, on this point I'm less sure.

I don't believe I've purposefully avoided exposing &Rocket. My thinking here is two-fold: 1) we already expose much of &Rocket via many methods like config() and shutdown(), and 2) it is already possible to access everything in &Rocket in request contexts by cloning and putting the item in managed state. The only additional items we would be exposing directly are figment(), catchers(), and routes(). Exposing figment() may not be ideal as it may encourage config lookups at run-time, but exposing the latter two seems like a win. What's more, I think request.rocket().config() is more illustrative than request.config(), which seems to imply that the request configuration, not the application configuration, is being retrieved; a similar effect can be seen with the rest of the methods.

In all, exposing &Rocket directly seems like an ergonomic, functionality, and maintainability win with little downside.

[DanielJoyce]

1. When you use a method annotation such as get, it also generates the option route support, it just ensures that all the method is checked for guards and will match the request, it doesn't actually call it.

Can you rephrase this? What does "that all the method is checked for guards" mean?

Well I don't how it's implemented internal, but rocket checks that a given request can satisfy all the guards/data that a handler function needs before calling it. The only difference for options is checks that the function could be called, that all the preconditions match ( A routing match ), but simply doesn't call it.

The simplest solution might be for the get!/put!/post!/ etc macros to take an optional options flag which means that it should generate a Options route for that hanlders as well. A route that checks that all the precoditions for invoking the handler are satisfied, but doesn't actually call it. It just returns the appropriate Options response.

[SergioBenitez]

The simplest solution might be for the get!/put!/post!/ etc macros to take an optional options flag which means that it should generate a Options route for that hanlders as well. A route that checks that all the precoditions for invoking the handler are satisfied, but doesn't actually call it. It just returns the appropriate Options response.

This would be arbitrarily expensive as guards can perform arbitrary work. Rocket won't do this.

[DanielJoyce]

Reading up on Options, it's just a CORS check, and apparently the options response makes no other determination where or not an actual PUT/GET/Etc request would succeed or fail. It's just "Hey, what are your CORs requirements"

In that case, I think, shipping with a simple CORS fairing OOTB, and the other Issue dealing with Forward being able to pass a responder or error or something to use if matching fails would cover all of my cases.

I was worried there might be some coupling between options and routes, but doesn't seem that way.

I guess if we want to allow customization, having an options macro ( like get, etc ) would let people overrride the CORS fairing behavior, and not worry about expensive guards too.

[DanielJoyce]

I know I've been proposing a lot of features and not contributing any, but I've been crunching away with rocket, and not had time. I should be able to contribute when things die down.

[dongcarl]

Hmmm, is this all that's needed?

diff --git a/core/lib/src/router/collider.rs b/core/lib/src/router/collider.rs
index 1b6d82b6..39abb918 100644
--- a/core/lib/src/router/collider.rs
+++ b/core/lib/src/router/collider.rs
@@ -96,7 +96,7 @@ impl Route {
     ///   * All static components in the route's query string are also in the
     ///     request query string, though in any position. If there is no query
     ///     in the route, requests with/without queries match.
-    pub(crate) fn matches(&self, req: &Request<'_>) -> bool {
+    pub fn matches(&self, req: &Request<'_>) -> bool {
         self.method == req.method()
             && paths_match(self, req)
             && queries_match(self, req)

I guess the ergonomics can be improved by adding a macro for creating enums with a method to iterate over all variants (for Method)

[SergioBenitez]

Hmmm, is this all that's needed?

diff --git a/core/lib/src/router/collider.rs b/core/lib/src/router/collider.rs

index 1b6d82b6..39abb918 100644

--- a/core/lib/src/router/collider.rs

+++ b/core/lib/src/router/collider.rs

@@ -96,7 +96,7 @@ impl Route {

     ///   * All static components in the route's query string are also in the

     ///     request query string, though in any position. If there is no query

     ///     in the route, requests with/without queries match.

-    pub(crate) fn matches(&self, req: &Request<'_>) -> bool {

+    pub fn matches(&self, req: &Request<'_>) -> bool {

         self.method == req.method()

             && paths_match(self, req)

             && queries_match(self, req)

I guess the ergonomics can be improved by adding a macro for creating enums with a method to iterate over all variants (for Method)

Technically, yes, this is all that's needed. But much more consideration needs to be given before this is done. This is low hanging fruit with a high thought-to-actual-work ratio. It is my plan to address this once all of the other pieces for the release are ready.

[dongcarl]

So writing a fairing that does this if we expose Request::rocket() and Route::matches() would be rather straightforward:

Would this belong in contrib? Also, why a fairing and not a request guard?

[SergioBenitez]

So writing a fairing that does this if we expose Request::rocket() and Route::matches() would be rather straightforward:

Would this belong in contrib? Also, why a fairing and not a request guard?

At the moment, there is no intention of providing this functionality in any official capacity. A fairing can intercept all OPTIONs requests and emit a response; a guard cannot do this.

[SergioBenitez]

I am not ready to commit Route.matches() into the public API, so I'm pushing this back for a decision later in time.