Merge pull request #25 from metosin/DuplicateRoutes

Handle conflicting routes, custom & mixed-router

README.md

@@ -392,14 +392,16 @@ Authorized access to guarded route:
 
 Routers can be configured via options. Options allow things like [`clojure.spec`](https://clojure.org/about/spec) validation for meta-data and fast, compiled handlers. The following options are available for the `reitit.core/router`:
 
-  | key        | description |
+  | key          | description |
-  | -----------|-------------|
+  | -------------|-------------|
-  | `:path`    | Base-path for routes (default `""`)
+  | `:path`      | Base-path for routes (default `""`)
-  | `:routes`  | Initial resolved routes (default `[]`)
+  | `:routes`    | Initial resolved routes (default `[]`)
-  | `:meta`    | Initial expanded route-meta vector (default `[]`)
+  | `:meta`      | Initial expanded route-meta vector (default `[]`)
-  | `:expand`  | Function of `arg opts => meta` to expand route arg to route meta-data (default `reitit.core/expand`)
+  | `:expand`    | Function of `arg opts => meta` to expand route arg to route meta-data (default `reitit.core/expand`)
-  | `:coerce`  | Function of `route opts => route` to coerce resolved route, can throw or return `nil`
+  | `:coerce`    | Function of `route opts => route` to coerce resolved route, can throw or return `nil`
-  | `:compile` | Function of `route opts => handler` to compile a route handler
+  | `:compile`   | Function of `route opts => handler` to compile a route handler
+  | `:conflicts` | Function of `{route #{route}} => side-effect` to handle conflicting routes (default `reitit.core/throw-on-conflicts!`)
+  | `:router`    | Function of `routes opts => router` to override the actual router implementation
 
 ## Special thanks
 

perf-test/clj/reitit/opensensors_routing_test.clj

@@ -120,7 +120,7 @@
    ["/v1/orgs/:org-id/errors" {:handler handler, :name :test/route17}]
    ["/v1/public/orgs/:org-id" {:handler handler, :name :test/route18}]
    ["/v1/orgs/:org-id/invitations" {:handler handler, :name :test/route19}]
-   ["/v2/public/messages/dataset/bulk" {:handler handler, :name :test/route20}]
+   #_["/v2/public/messages/dataset/bulk" {:handler handler, :name :test/route20}]
    #_["/v1/users/:user-id/devices/bulk" {:handler handler, :name :test/route21}]
    ["/v1/users/:user-id/device-errors" {:handler handler, :name :test/route22}]
    ["/v2/login" {:handler handler, :name :test/route23}]
@@ -202,13 +202,13 @@
                "whoami" :test/route41
                "login" :test/route51}
         "v2/" {"whoami" :test/route1
-               ["users/" :user-id "/"] {"datasets" :test/route2
+               ["users/" :user-id] {"/datasets" :test/route2
-                                        "devices" :test/route25
+                                    "/devices" :test/route25
-                                        "topics" {"/bulk" :test/route29
+                                    "/topics" {"/bulk" :test/route29
-                                                  "" :test/route45}}
+                                               "" :test/route45}}
                "public/" {["projects/" :project-id] {"/datasets" :test/route3
                                                      "" :test/route27}
-                          "messages/dataset/bulk" :test/route20
+                          #_#_"messages/dataset/bulk" :test/route20
                           ["datasets/" :dataset-id] :test/route28
                           ["messages/dataset/" :dataset-id] :test/route53}
                ["datasets/" :dataset-id] :test/route11
@@ -264,11 +264,11 @@
       "/v2/" {"whoami" [:test/route1]
               ["users/" user-id] {"/datasets" [:test/route2 user-id]
                                   "/devices" [:test/route25 user-id]
-                                  "topics/" {"bulk" [:test/route29 user-id]
+                                  "/topics" {"/bulk" [:test/route29 user-id]
                                              "" [:test/route45 user-id]}}
               "public/" {["projects/" project-id] {"/datasets" [:test/route3 project-id]
                                                    "" [:test/route27 project-id]}
-                         "messages/dataset/bulk" [:test/route20]
+                         #_#_"messages/dataset/bulk" [:test/route20]
                          ["datasets/" dataset-id] [:test/route28 dataset-id]
                          ["messages/dataset/" dataset-id] [:test/route53 dataset-id]}
               ["datasets/" dataset-id] [:test/route11 dataset-id]
@@ -344,7 +344,7 @@
         (context "/projects/:project-id" []
           (ANY "/datasets" [] {:name :test/route3} handler)
           (ANY "/" [] {:name :test/route27} handler))
-        (ANY "/messages/dataset/bulk" [] {:name :test/route20} handler)
+        #_(ANY "/messages/dataset/bulk" [] {:name :test/route20} handler)
         (ANY "/datasets/:dataset-id" [] {:name :test/route28} handler)
         (ANY "/messages/dataset/:dataset-id" [] {:name :test/route53} handler))
       (ANY "/datasets/:dataset-id" [] {:name :test/route11} handler)
@@ -376,7 +376,7 @@
        ["/v1/orgs/:org-id/errors" :get handler :route-name :test/route17]
        ["/v1/public/orgs/:org-id" :get handler :route-name :test/route18]
        ["/v1/orgs/:org-id/invitations" :get handler :route-name :test/route19]
-       ["/v2/public/messages/dataset/bulk" :get handler :route-name :test/route20]
+       #_["/v2/public/messages/dataset/bulk" :get handler :route-name :test/route20]
        #_["/v1/users/:user-id/devices/bulk" :get handler :route-name :test/route21]
        ["/v1/users/:user-id/device-errors" :get handler :route-name :test/route22]
        ["/v2/login" :get handler :route-name :test/route23]
@@ -493,12 +493,12 @@
         compojure-api-f #(opensensors-compojure-api-routes {:uri % :request-method :get})
         pedestal-f #(pedestal/find-route opensensors-pedestal-routes {:path-info % :request-method :get})]
 
-    (bench! routes true "reitit" reitit-f)                  ;;  2538ns    10%
+    (bench! routes true "reitit" reitit-f)                  ;;  2538ns -> 2028ns
-    (bench! routes true "pedestal" pedestal-f)              ;;  2737ns    11%
+    (bench! routes true "reitit-ring" reitit-ring-f)        ;;  2845ns -> 2299ns
-    (bench! routes true "reitit-ring" reitit-ring-f)        ;;  2845ns    11%
+    (bench! routes true "pedestal" pedestal-f)              ;;  2737ns
-    (bench! routes true "compojure-api" compojure-api-f)    ;; 10215ns    41%
+    (bench! routes true "compojure-api" compojure-api-f)    ;;  9823ns
-    (bench! routes true "bidi" bidi-f)                      ;; 19298ns    77%
+    (bench! routes true "bidi" bidi-f)                      ;; 16716ns
-    (bench! routes true "ataraxy" ataraxy-f)                ;; 24950ns   100%
+    (bench! routes true "ataraxy" ataraxy-f)                ;; 24467ns
 
     ))
 

src/reitit/core.cljc

@@ -1,5 +1,6 @@
 (ns reitit.core
   (:require [meta-merge.core :refer [meta-merge]]
+            [clojure.string :as str]
             [reitit.impl :as impl #?@(:cljs [:refer [Route]])])
   #?(:clj
      (:import (reitit.impl Route))))
@@ -60,16 +61,38 @@
   (cond->> (->> (walk data opts) (map-meta merge-meta))
            coerce (into [] (keep #(coerce % opts)))))
 
+;; This whole function might be more efficient and easier to understand with transducers.
+(defn conflicting-routes [routes]
+  (some->>
+    (loop [[r & rest] routes, acc {}]
+      (if (seq rest)
+        (let [conflicting (set (keep #(if (impl/conflicting-routes? r %) %) rest))]
+          (recur rest (update acc r (fnil (comp set concat) #{}) conflicting)))
+        acc))
+    (filter (comp seq second))
+    (seq)
+    (into {})))
+
+(defn throw-on-conflicts! [conflicts]
+  (throw
+    (ex-info
+      (apply str "router contains conflicting routes:\n\n"
+             (mapv
+               (fn [[[path] vals]]
+                 (str "   " path "\n-> " (str/join "\n-> " (mapv first vals)) "\n\n"))
+               conflicts))
+      {:conflicts conflicts})))
+
 (defn name-lookup [[_ {:keys [name]}] opts]
   (if name #{name}))
 
 (defn find-names [routes opts]
-  (into [] (keep #(-> % second :name) routes)))
+  (into [] (keep #(-> % second :name)) routes))
 
 (defn compile-route [[p m :as route] {:keys [compile] :as opts}]
   [p m (if compile (compile route opts))])
 
-(defprotocol Routing
+(defprotocol Router
   (router-type [this])
   (routes [this])
   (options [this])
@@ -97,10 +120,11 @@
   {:lookup name-lookup
    :expand expand
    :coerce (fn [route _] route)
-   :compile (fn [[_ {:keys [handler]}] _] handler)})
+   :compile (fn [[_ {:keys [handler]}] _] handler)
+   :conflicts throw-on-conflicts!})
 
 (defn linear-router
-  "Creates a [[LinearRouter]] from resolved routes and optional
+  "Creates a LinearRouter from resolved routes and optional
   expanded options. See [[router]] for available options"
   ([routes]
    (linear-router routes {}))
@@ -118,7 +142,7 @@
                          [[] {}] compiled)
          lookup (impl/fast-map lookup)]
      (reify
-       Routing
+       Router
        (router-type [_]
          :linear-router)
        (routes [_]
@@ -146,11 +170,11 @@
   ([routes]
    (lookup-router routes {}))
   ([routes opts]
-   (when-let [route (some impl/contains-wilds? (map first routes))]
+   (when-let [wilds (seq (filter impl/wild-route? routes))]
      (throw
        (ex-info
-         (str "can't create LookupRouter with wildcard routes: " route)
+         (str "can't create LookupRouter with wildcard routes: " wilds)
-         {:route route
+         {:wilds wilds
           :routes routes})))
    (let [compiled (map #(compile-route % opts) routes)
          names (find-names routes opts)
@@ -162,7 +186,7 @@
                               lookup)]) [{} {}] compiled)
          data (impl/fast-map data)
          lookup (impl/fast-map lookup)]
-     (reify Routing
+     (reify Router
        (router-type [_]
          :lookup-router)
        (routes [_]
@@ -180,23 +204,68 @@
          (if-let [match (impl/fast-get lookup name)]
            (match params)))))))
 
+(defn mixed-router
+  "Creates two routers: [[lookup-router]] for static routes and
+  [[linear-router]] for wildcard routes. All routes should be
+  non-conflicting. Takes resolved routes and optional
+  expanded options. See [[router]] for options."
+  ([routes]
+   (mixed-router routes {}))
+  ([routes opts]
+   (let [{linear true, lookup false} (group-by impl/wild-route? routes)
+         linear-router (linear-router linear opts)
+         lookup-router (lookup-router lookup opts)
+         names (find-names routes opts)]
+     (reify Router
+       (router-type [_]
+         :mixed-router)
+       (routes [_]
+         routes)
+       (options [_]
+         opts)
+       (route-names [_]
+         names)
+       (match-by-path [_ path]
+         (or (match-by-path lookup-router path)
+             (match-by-path linear-router path)))
+       (match-by-name [_ name]
+         (or (match-by-name lookup-router name)
+             (match-by-name linear-router name)))
+       (match-by-name [_ name params]
+         (or (match-by-name lookup-router name params)
+             (match-by-name linear-router name params)))))))
+
 (defn router
   "Create a [[Router]] from raw route data and optionally an options map.
   If routes contain wildcards, a [[LinearRouter]] is used, otherwise a
   [[LookupRouter]]. The following options are available:
 
-  | key        | description |
+  | key          | description |
-  | -----------|-------------|
+  | -------------|-------------|
-  | `:path`    | Base-path for routes (default `\"\"`)
+  | `:path`      | Base-path for routes (default `\"\"`)
-  | `:routes`  | Initial resolved routes (default `[]`)
+  | `:routes`    | Initial resolved routes (default `[]`)
-  | `:meta`    | Initial expanded route-meta vector (default `[]`)
+  | `:meta`      | Initial expanded route-meta vector (default `[]`)
-  | `:expand`  | Function of `arg opts => meta` to expand route arg to route meta-data (default `reitit.core/expand`)
+  | `:expand`    | Function of `arg opts => meta` to expand route arg to route meta-data (default `reitit.core/expand`)
-  | `:coerce`  | Function of `route opts => route` to coerce resolved route, can throw or return `nil`
+  | `:coerce`    | Function of `route opts => route` to coerce resolved route, can throw or return `nil`
-  | `:compile` | Function of `route opts => handler` to compile a route handler"
+  | `:compile`   | Function of `route opts => handler` to compile a route handler
+  | `:conflicts` | Function of `{route #{route}} => side-effect` to handle conflicting routes (default `reitit.core/throw-on-conflicts!`)
+  | `:router`    | Function of `routes opts => router` to override the actual router implementation"
   ([data]
    (router data {}))
   ([data opts]
-   (let [opts (meta-merge default-router-options opts)
+   (let [{:keys [router] :as opts} (meta-merge default-router-options opts)
-         routes (resolve-routes data opts)]
+         routes (resolve-routes data opts)
-     ((if (some impl/contains-wilds? (map first routes))
+         conflicting (conflicting-routes routes)
-        linear-router lookup-router) routes opts))))
+         wilds? (some impl/wild-route? routes)
+         all-wilds? (every? impl/wild-route? routes)
+         router (cond
+                  router router
+                  (not wilds?) lookup-router
+                  all-wilds? linear-router
+                  (not conflicting) mixed-router
+                  :else linear-router)]
+
+     (when-let [conflicts (:conflicts opts)]
+       (when conflicting (conflicts conflicting)))
+
+     (router routes opts))))

src/reitit/impl.cljc

@@ -121,6 +121,28 @@
                  :matcher #(if (= path %) {})
                  :handler handler})))
 
+(defn segments [path]
+  (let [ss (-> (str/split path #"/") rest vec)]
+    (if (str/ends-with? path "/")
+      (conj ss "") ss)))
+
+(defn- catch-all? [segment]
+  (= \* (first segment)))
+
+(defn wild-route? [[path]]
+  (contains-wilds? path))
+
+(defn conflicting-routes? [[p1 :as route1] [p2 :as route2]]
+  (loop [[s1 & ss1] (segments p1)
+         [s2 & ss2] (segments p2)]
+    (cond
+      (= s1 s2 nil) true
+      (or (nil? s1) (nil? s2)) false
+      (or (catch-all? s1) (catch-all? s2)) true
+      (or (wild? s1) (wild? s2)) (recur ss1 ss2)
+      (not= s1 s2) false
+      :else (recur ss1 ss2))))
+
 (defn path-for [^Route route params]
   (if-let [required (:params route)]
     (if (every? #(contains? params %) required)

test/cljc/reitit/core_test.cljc

@@ -1,13 +1,13 @@
 (ns reitit.core-test
-  (:require [clojure.test :refer [deftest testing is]]
+  (:require [clojure.test :refer [deftest testing is are]]
-            [reitit.core :as reitit #?@(:cljs [:refer [Match]])])
+            [reitit.core :as reitit #?@(:cljs [:refer [Match Router]])])
   #?(:clj
-     (:import (reitit.core Match)
+     (:import (reitit.core Match Router)
               (clojure.lang ExceptionInfo))))
 
 (deftest reitit-test
 
-  (testing "linear router"
+  (testing "linear-router"
     (let [router (reitit/router ["/api" ["/ipa" ["/:size" ::beer]]])]
       (is (= :linear-router (reitit/router-type router)))
       (is (= [["/api/ipa/:size" {:name ::beer}]]
@@ -40,7 +40,7 @@
               #"^missing path-params for route /api/ipa/:size: \#\{:size\}$"
               (reitit/match-by-name! router ::beer))))))
 
-  (testing "lookup router"
+  (testing "lookup-router"
     (let [router (reitit/router ["/api" ["/ipa" ["/large" ::beer]]])]
       (is (= :lookup-router (reitit/router-type router)))
       (is (= [["/api/ipa/large" {:name ::beer}]]
@@ -106,6 +106,13 @@
           (is handler)
           (is (= "ok" (handler)))))))
 
+  (testing "custom router"
+    (let [router (reitit/router ["/ping"] {:router (fn [_ _]
+                                                     (reify Router
+                                                       (reitit/router-type [_]
+                                                         ::custom)))})]
+      (is (= ::custom (reitit/router-type router)))))
+
   (testing "bide sample"
     (let [routes [["/auth/login" :auth/login]
                   ["/auth/recovery/token/:token" :auth/recovery]
@@ -138,3 +145,54 @@
                 :path "/api/user/1/2"
                 :params {:id "1", :sub-id "2"}})
              (reitit/match-by-path router "/api/user/1/2"))))))
+
+(deftest conflicting-routes-test
+  (are [conflicting? data]
+    (let [routes (reitit/resolve-routes data {})
+          conflicts (-> routes (reitit/resolve-routes {}) (reitit/conflicting-routes))]
+      (if conflicting? (seq conflicts) (nil? conflicts)))
+
+    true [["/a"]
+          ["/a"]]
+
+    true [["/a"]
+          ["/:b"]]
+
+    true [["/a"]
+          ["/*b"]]
+
+    true [["/a/1/2"]
+          ["/*b"]]
+
+    false [["/a"]
+           ["/a/"]]
+
+    false [["/a"]
+           ["/a/1"]]
+
+    false [["/a"]
+           ["/a/:b"]]
+
+    false [["/a"]
+           ["/a/*b"]]
+
+    true [["/v2/public/messages/dataset/bulk"]
+          ["/v2/public/messages/dataset/:dataset-id"]])
+
+  (testing "all conflicts are returned"
+    (is (= {["/a" {}] #{["/*d" {}] ["/:b" {}]},
+            ["/:b" {}] #{["/c" {}] ["/*d" {}]},
+            ["/c" {}] #{["/*d" {}]}}
+           (-> [["/a"] ["/:b"] ["/c"] ["/*d"]]
+               (reitit/resolve-routes {})
+               (reitit/conflicting-routes)))))
+
+  (testing "router with conflicting routes"
+    (testing "throws by default"
+      (is (thrown-with-msg?
+            ExceptionInfo
+            #"router contains conflicting routes"
+            (reitit/router
+              [["/a"] ["/a"]]))))
+    (testing "can be configured to ignore"
+      (is (not (nil? (reitit/router [["/a"] ["/a"]] {:conflicts (constantly nil)})))))))