Reverse-routing with ring/http & request methods

[ikitommi]

Reverse routing uses the :name route data, which is a concept of the core router. Currently it doesn't understand the ring method leaves like :get, :post etc. This mean that there is no way to identify each unique path+method pair. Suggestions:

1) customize reverse routing

• make a router options that allows route :names to be collected & reverse routed via custom functions
• modify reitit.ring/router & reitit.http/router set the value the reverse routing works from both (method) leaves & top-level
• ???: resulting Match of a reverse routing should somehow have both the path & relevant route data parts. New key under Match?

2) new higher level reverse routing with ring/http

• Swagger defines operationId per method endpoint. We could have just :id.
• New HttpRouter protocol with match-by-id & route-ids for both ring & http routers to point to method endpoints
• match-by-id would return the method as a new key in Match

[RokLenarcic]

I'd say that second solution looks more neatly packaged into existing concepts.

The first suggestion is more general with the collecting functions, but it's not clear what other uses this would address besides HTTP methods. And it's hard to design a general facility without having multiple usecases of the bat.

[rschmukler]

I'd like to propose that match-by-name just be extended to behave as match-by-id instead. Introducing match-by-id adds more surface area and potential for some confusing behaviors. What happens if a route has an :id and a :name? Why enforce :name be unique when we have :id? Maybe I am missing something, but I'd ask in which cases would :name and :id not behave the same, and, is the additional complexity worth having those two distinct behaviors?

For my specific use case I wanted to share the routing table from my back-end with my front-end to make calls to the API + handle resolution of specs for request and response formats. Eg imagine something like:

(def routes
  (http/router
    ["/api"
      ["/users"
        [["" {:post {:name       :api.users/create 
                     :parameters {:body :user/new}
                     :responses   {201 {:body {:data {:key string?}}}}]]]))

I'd like to be able to define a (front-end) function like:

(api/make-request :api.users/create {:username "Bob" :password "Foo"})

Which can automatically figure out the request / response specs (for st/coerce) + the HTTP method + path.

[ikitommi]

For 1, some extra considerations:

1) how would the resulting Match indicate what :request-method was matched? a) new key, :request-method which can be nil (top-level match) or a keyword (method-matched) b) no indication, caller has to parse the :data under :compiled-routes from Match to find where the :name was defined

2) route name conflict resolution: needs to be changed to support the composite key of path + $extra-identifier (here: the :request-method)

3) what would the :data be for the Match? Currently, it's the full data for the given path a) as before, user can lookup the endpoint data using the :request-method from 1 b) if a method matched, :data should be the data for that endpoint only c) a new key, which has the endpoint data (both full & endpoint data are easily available)

From user perspective, 1 seems more obvious. From implementation perspective, 2 would much simpler as it doesn't mix the two concepts: core-routing & http-routing. As 2 would just be a helper on top of existing library, we could have an example out of that there is A way to do this. 1 would be a BREAKING change and not going to happen any time soon.

EDIT: if we would just inject the :request-method into the Match it could be considered as an addition, not a BREAKING? Hmm.

[ikitommi]

Extended example, seen through a Match:

(require '[reitit.http :as http])
(require '[reitit.core :as r])

(def router
  (http/router
    ["/api"
     ["/users"
      ["" {:name :api.user/base
           :get {:name :api.user/get}
           :post {:name :api.users/create
                  :parameters {:body :user/new}
                  :responses {201 {:body {:data {:key string?}}}}}}]]]))

(r/match-by-path router "/api/users")
;#reitit.core.Match{:template "/api/users",
;                   :data {:name :api.user/base,
;                          :get {:name :api.user/get},
;                          :post {:name :api.users/create,
;                                 :parameters {:body :user/new},
;                                 :responses {201 {:body {:data {:key #object[clojure.core$string_QMARK___5410
;                                                                             0x3a3e78f
;                                                                             "clojure.core$string_QMARK___5410@3a3e78f"]}}}}}},
;                   :result #reitit.ring.Methods{:get #reitit.http.Endpoint{:data {:name :api.user/get},
;                                                                           :interceptors [],
;                                                                           :queue [],
;                                                                           :handler nil,
;                                                                           :path "/api/users",
;                                                                           :method :get},
;                                                :head nil,
;                                                :post #reitit.http.Endpoint{:data {:name :api.users/create,
;                                                                                   :parameters {:body :user/new},
;                                                                                   :responses {201 {:body {:data {:key #object[clojure.core$string_QMARK___5410
;                                                                                                                               0x3a3e78f
;                                                                                                                               "clojure.core$string_QMARK___5410@3a3e78f"]}}}}},
;                                                                            :interceptors [],
;                                                                            :queue [],
;                                                                            :handler nil,
;                                                                            :path "/api/users",
;                                                                            :method :post},
;                                                :put nil,
;                                                :delete nil,
;                                                :connect nil,
;                                                :options #reitit.http.Endpoint{:data {:name :api.user/base,
;                                                                                      :no-doc true,
;                                                                                      :handler #object[reitit.ring$fn__5733$fn__5742
;                                                                                                       0x1be57706
;                                                                                                       "reitit.ring$fn__5733$fn__5742@1be57706"]},
;                                                                               :interceptors [#reitit.interceptor.Interceptor{:name :reitit.interceptor/handler,
;                                                                                                                              :enter #object[reitit.interceptor$eval5196$fn__5197$fn__5198
;                                                                                                                                             0x63a29ae
;                                                                                                                                             "reitit.interceptor$eval5196$fn__5197$fn__5198@63a29ae"],
;                                                                                                                              :leave nil,
;                                                                                                                              :error nil,
;                                                                                                                              :reitit.interceptor/handler #object[reitit.ring$fn__5733$fn__5742
;                                                                                                                                                                  0x1be57706
;                                                                                                                                                                  "reitit.ring$fn__5733$fn__5742@1be57706"]}],
;                                                                               :queue [#reitit.interceptor.Interceptor{:name :reitit.interceptor/handler,
;                                                                                                                       :enter #object[reitit.interceptor$eval5196$fn__5197$fn__5198
;                                                                                                                                      0x63a29ae
;                                                                                                                                      "reitit.interceptor$eval5196$fn__5197$fn__5198@63a29ae"],
;                                                                                                                       :leave nil,
;                                                                                                                       :error nil,
;                                                                                                                       :reitit.interceptor/handler #object[reitit.ring$fn__5733$fn__5742
;                                                                                                                                                           0x1be57706
;                                                                                                                                                           "reitit.ring$fn__5733$fn__5742@1be57706"]}],
;                                                                               :handler nil,
;                                                                               :path "/api/users",
;                                                                               :method :options},
;                                                :trace nil,
;                                                :patch nil},
;                   :path-params {},
;                   :path "/api/users"}

[rschmukler]

Just echoing that I'm not sure that implementing 1 needs to be a breaking change. It seems that it's additive. A few more considerations:

1) Should conflict detection be a composite key? I think the idea of keeping names unique is a good one, and that the request method shouldn't impact it.

2) Injecting :request-method onto the Match seems like a great way to enhance the feature without breaking anything. For front-ends it could even default to :get instead of nil.

3) match-by-path could also be extended to be match-by-path to a 3 arity variant which takes the method. Eg. (match-by-path "/users/5" :post). Similar to the above, the 2 arity variant could just assume a :get.

4) I think the :data should be the merged data, as it is for the reitit.http interceptors (ie. the meta-merge combination of the parent scopes with the matching route).

5) I'd consider not allowing :name for the paths with multiple methods (ie. api.user/base in your example) since it doesn't actually correspond to anything that could directly match. In your example we don't have a :name for the "/api" intermediate either, and /users feels like the same thing. Perhaps only matchable routes should be able to be named? I think that we can keep things concise by assuming a :get in the cases where it's not specified, and not ambiguous. I believe this is consistent with existing behavior?

[malcolmsparks]

I agree with all these points by @rschmukler, especially keeping :name rather than adding :id which has the potential to be confusing.

It is also potentially confusing have :name as a key, and it's possible (though highly unlikely) for a user to want to create a custom method NAME. Still, I think the non-namespaced keywords should indicate the method, and only the method.

A dev using match-by-name will often be wanting to form a path, or URL, for the purposes of requesting it with a method. Therefore, I think it's reasonable to expect the dev to provide the method they intend to use, in a third parameter of a 3-arity variant, as @rschmukler proposes. If there is no such method on the URL, nil is a reasonable return value (as an HTTP request would result in a 405). It also allows the query and other parameters to be validated according to requirements of the method on the URL, as each method on the URL may have different parameter requirements (and OpenAPI provides for this).

[ikitommi]

Great comments. Relevant PR with intermediate paths #326. I think these should resolved in sync.

[ikitommi]

DIscussed about this, we'll push the :name into endpoints. Should be out soon.