From 851e35ef52c3f5060169ac3f747714a6d5b100a3 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 21 Aug 2017 08:44:51 +0300 Subject: [PATCH 1/9] Validate routes for duplicates (fixes #23) --- src/reitit/core.cljc | 6 ++++++ src/reitit/impl.cljc | 19 +++++++++++++++++ test/cljc/reitit/core_test.cljc | 36 ++++++++++++++++++++++++++++++++- 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index 6ee1c63a..3f8db58b 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -60,6 +60,12 @@ (cond->> (->> (walk data opts) (map-meta merge-meta)) coerce (into [] (keep #(coerce % opts))))) +(defn first-conflicting-routes [routes] + (loop [[r & rest] routes] + (if (seq rest) + (or (some #(if (impl/conflicting-routes? r %) [r %]) rest) + (recur rest))))) + (defn name-lookup [[_ {:keys [name]}] opts] (if name #{name})) diff --git a/src/reitit/impl.cljc b/src/reitit/impl.cljc index 0e95c253..1989fa70 100644 --- a/src/reitit/impl.cljc +++ b/src/reitit/impl.cljc @@ -121,6 +121,25 @@ :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 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) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index ba8c0dbf..854b02ed 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -1,5 +1,5 @@ (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]])]) #?(:clj (:import (reitit.core Match) @@ -138,3 +138,37 @@ :path "/api/user/1/2" :params {:id "1", :sub-id "2"}}) (reitit/match-by-path router "/api/user/1/2")))))) + +(deftest first-conflicting-routes-test + (are [conflicting? data] + (let [routes (reitit/resolve-routes data {})] + (= (if conflicting? routes) + (reitit/first-conflicting-routes + (reitit/resolve-routes routes {})))) + + 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"]])) From f5f110482636061a4c12b10b5bd529df18444893 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 21 Aug 2017 09:02:03 +0300 Subject: [PATCH 2/9] Router option to handle conflicts --- README.md | 17 +++++++++-------- src/reitit/core.cljc | 29 ++++++++++++++++++++--------- test/cljc/reitit/core_test.cljc | 21 ++++++++++++++++++--- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 7cad9a20..ca5a865f 100644 --- a/README.md +++ b/README.md @@ -392,14 +392,15 @@ 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 | - | -----------|-------------| - | `:path` | Base-path for routes (default `""`) - | `:routes` | Initial resolved routes (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`) - | `: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 + | key | description | + | -------------|-------------| + | `:path` | Base-path for routes (default `""`) + | `:routes` | Initial resolved routes (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`) + | `: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 + | `:conflicts` | Function of `[route route] => side-effect` to handle conflicting routes (default `reitit.core/throw-on-conflicts!`)" ## Special thanks diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index 3f8db58b..fc8b3342 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -66,6 +66,12 @@ (or (some #(if (impl/conflicting-routes? r %) [r %]) rest) (recur rest))))) +(defn throw-on-conflicts! [routes] + (throw + (ex-info + (str "router contains conflicting routes: " routes) + {:routes routes}))) + (defn name-lookup [[_ {:keys [name]}] opts] (if name #{name})) @@ -103,7 +109,8 @@ {: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 @@ -191,18 +198,22 @@ If routes contain wildcards, a [[LinearRouter]] is used, otherwise a [[LookupRouter]]. The following options are available: - | key | description | - | -----------|-------------| - | `:path` | Base-path for routes (default `\"\"`) - | `:routes` | Initial resolved routes (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`) - | `: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" + | key | description | + | -------------|-------------| + | `:path` | Base-path for routes (default `\"\"`) + | `:routes` | Initial resolved routes (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`) + | `: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 + | `:conflicts` | Function of `[route route] => side-effect` to handle conflicting routes (default `reitit.core/throw-on-conflicts!`)" ([data] (router data {})) ([data opts] (let [opts (meta-merge default-router-options opts) routes (resolve-routes data opts)] + (when-let [conflicts (:conflicts opts)] + (when-let [conflicting-routes (first-conflicting-routes routes)] + (conflicts conflicting-routes))) ((if (some impl/contains-wilds? (map first routes)) linear-router lookup-router) routes opts)))) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 854b02ed..b099233d 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -1,8 +1,8 @@ (ns reitit.core-test (:require [clojure.test :refer [deftest testing is are]] - [reitit.core :as reitit #?@(:cljs [:refer [Match]])]) + [reitit.core :as reitit #?@(:cljs [:refer [Match Routing]])]) #?(:clj - (:import (reitit.core Match) + (:import (reitit.core Match Routing) (clojure.lang ExceptionInfo)))) (deftest reitit-test @@ -171,4 +171,19 @@ ["/a/*b"]] true [["/v2/public/messages/dataset/bulk"] - ["/v2/public/messages/dataset/:dataset-id"]])) + ["/v2/public/messages/dataset/:dataset-id"]]) + + (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 (instance? + Routing + (reitit/router + [["/a"] ["/a"]] + {:conflicts (constantly nil)})))))) + From 9701a51c5a866fc5362ddae44634668abed29bee Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 21 Aug 2017 09:10:21 +0300 Subject: [PATCH 3/9] Routing -> Router, fix tests --- src/reitit/core.cljc | 6 +++--- test/cljc/reitit/core_test.cljc | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index fc8b3342..a17bfb2e 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -81,7 +81,7 @@ (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]) @@ -131,7 +131,7 @@ [[] {}] compiled) lookup (impl/fast-map lookup)] (reify - Routing + Router (router-type [_] :linear-router) (routes [_] @@ -175,7 +175,7 @@ lookup)]) [{} {}] compiled) data (impl/fast-map data) lookup (impl/fast-map lookup)] - (reify Routing + (reify Router (router-type [_] :lookup-router) (routes [_] diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index b099233d..0195bcda 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -1,8 +1,8 @@ (ns reitit.core-test (:require [clojure.test :refer [deftest testing is are]] - [reitit.core :as reitit #?@(:cljs [:refer [Match Routing]])]) + [reitit.core :as reitit #?@(:cljs [:refer [Match]])]) #?(:clj - (:import (reitit.core Match Routing) + (:import (reitit.core Match) (clojure.lang ExceptionInfo)))) (deftest reitit-test @@ -181,9 +181,9 @@ (reitit/router [["/a"] ["/a"]])))) (testing "can be configured to ignore" - (is (instance? - Routing - (reitit/router - [["/a"] ["/a"]] - {:conflicts (constantly nil)})))))) + (is (not + (nil? + (reitit/router + [["/a"] ["/a"]] + {:conflicts (constantly nil)}))))))) From 066f5752c2b62ebce044da690528e33ece753578 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 21 Aug 2017 20:44:16 +0300 Subject: [PATCH 4/9] Return all conflicts for IO --- README.md | 2 +- src/reitit/core.cljc | 27 ++++++++++++++++----------- test/cljc/reitit/core_test.cljc | 18 ++++++++++++------ 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index ca5a865f..817b1126 100644 --- a/README.md +++ b/README.md @@ -400,7 +400,7 @@ Routers can be configured via options. Options allow things like [`clojure.spec` | `: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` | `: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!`)" + | `:conflicts` | Function of `{route #{route}} => side-effect` to handle conflicting routes (default `reitit.core/throw-on-conflicts!`)" ## Special thanks diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index a17bfb2e..3417f931 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -60,17 +60,22 @@ (cond->> (->> (walk data opts) (map-meta merge-meta)) coerce (into [] (keep #(coerce % opts))))) -(defn first-conflicting-routes [routes] - (loop [[r & rest] routes] - (if (seq rest) - (or (some #(if (impl/conflicting-routes? r %) [r %]) rest) - (recur rest))))) +(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! [routes] +(defn throw-on-conflicts! [conflicts] (throw (ex-info - (str "router contains conflicting routes: " routes) - {:routes routes}))) + (str "router contains conflicting routes: " conflicts) + {:conflicts conflicts}))) (defn name-lookup [[_ {:keys [name]}] opts] (if name #{name})) @@ -206,14 +211,14 @@ | `: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` | `: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!`)" + | `:conflicts` | Function of `{route #{route}} => side-effect` to handle conflicting routes (default `reitit.core/throw-on-conflicts!`)" ([data] (router data {})) ([data opts] (let [opts (meta-merge default-router-options opts) routes (resolve-routes data opts)] (when-let [conflicts (:conflicts opts)] - (when-let [conflicting-routes (first-conflicting-routes routes)] - (conflicts conflicting-routes))) + (when-let [conflicting (conflicting-routes routes)] + (conflicts conflicting))) ((if (some impl/contains-wilds? (map first routes)) linear-router lookup-router) routes opts)))) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 0195bcda..6034767d 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -139,12 +139,11 @@ :params {:id "1", :sub-id "2"}}) (reitit/match-by-path router "/api/user/1/2")))))) -(deftest first-conflicting-routes-test +(deftest conflicting-routes-test (are [conflicting? data] - (let [routes (reitit/resolve-routes data {})] - (= (if conflicting? routes) - (reitit/first-conflicting-routes - (reitit/resolve-routes routes {})))) + (let [routes (reitit/resolve-routes data {}) + conflicts (-> routes (reitit/resolve-routes {}) (reitit/conflicting-routes))] + (if conflicting? (seq conflicts) (nil? conflicts))) true [["/a"] ["/a"]] @@ -173,6 +172,14 @@ 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? @@ -186,4 +193,3 @@ (reitit/router [["/a"] ["/a"]] {:conflicts (constantly nil)}))))))) - From bf0d327570e523ec0a9f4481fac4183bd4876056 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Tue, 22 Aug 2017 08:27:34 +0300 Subject: [PATCH 5/9] Better error messages --- src/reitit/core.cljc | 22 +++++++++++++--------- src/reitit/impl.cljc | 3 +++ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index 3417f931..65d2135e 100644 --- a/src/reitit/core.cljc +++ b/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)))) @@ -74,7 +75,11 @@ (defn throw-on-conflicts! [conflicts] (throw (ex-info - (str "router contains conflicting routes: " conflicts) + (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] @@ -118,7 +123,7 @@ :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 {})) @@ -164,11 +169,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) - {:route route + (str "can't create LookupRouter with wildcard routes: " wilds) + {:wilds wilds :routes routes}))) (let [compiled (map #(compile-route % opts) routes) names (find-names routes opts) @@ -218,7 +223,6 @@ (let [opts (meta-merge default-router-options opts) routes (resolve-routes data opts)] (when-let [conflicts (:conflicts opts)] - (when-let [conflicting (conflicting-routes routes)] - (conflicts conflicting))) - ((if (some impl/contains-wilds? (map first routes)) - linear-router lookup-router) routes opts)))) + (when conflicting (conflicts conflicting))) + + (router routes opts)))) diff --git a/src/reitit/impl.cljc b/src/reitit/impl.cljc index 1989fa70..217a48fb 100644 --- a/src/reitit/impl.cljc +++ b/src/reitit/impl.cljc @@ -129,6 +129,9 @@ (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)] From 3dc1cdfbe2114f8ed9aa14ab0e8bf731d8ca2e12 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Tue, 22 Aug 2017 08:40:21 +0300 Subject: [PATCH 6/9] Welcome :mixed-router (-20% on rest-test) & custom routers --- README.md | 3 +- .../clj/reitit/opensensors_routing_test.clj | 22 ++++----- src/reitit/core.cljc | 46 +++++++++++++++++-- test/cljc/reitit/core_test.cljc | 29 ++++++------ 4 files changed, 72 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 817b1126..9bf269e9 100644 --- a/README.md +++ b/README.md @@ -400,7 +400,8 @@ Routers can be configured via options. Options allow things like [`clojure.spec` | `: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` | `: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!`)" + | `: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 diff --git a/perf-test/clj/reitit/opensensors_routing_test.clj b/perf-test/clj/reitit/opensensors_routing_test.clj index d25b7942..8c2570d9 100644 --- a/perf-test/clj/reitit/opensensors_routing_test.clj +++ b/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}] @@ -208,7 +208,7 @@ "" :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 @@ -268,7 +268,7 @@ "" [: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 "pedestal" pedestal-f) ;; 2737ns 11% - (bench! routes true "reitit-ring" reitit-ring-f) ;; 2845ns 11% - (bench! routes true "compojure-api" compojure-api-f) ;; 10215ns 41% - (bench! routes true "bidi" bidi-f) ;; 19298ns 77% - (bench! routes true "ataraxy" ataraxy-f) ;; 24950ns 100% + (bench! routes true "reitit" reitit-f) ;; 2538ns -> 2028ns + (bench! routes true "reitit-ring" reitit-ring-f) ;; 2845ns -> 2299ns + (bench! routes true "pedestal" pedestal-f) ;; 2737ns + (bench! routes true "compojure-api" compojure-api-f) ;; 9823ns + (bench! routes true "bidi" bidi-f) ;; 16716ns + (bench! routes true "ataraxy" ataraxy-f) ;; 24467ns )) diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index 65d2135e..71d5ae87 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -203,6 +203,37 @@ (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 @@ -216,12 +247,21 @@ | `: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` | `: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!`)" + | `: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) - routes (resolve-routes data opts)] + (let [{:keys [router] :as opts} (meta-merge default-router-options opts) + routes (resolve-routes data opts) + conflicting (conflicting-routes routes) + wilds? (some impl/wild-route? routes) + router (cond + router router + (not wilds?) lookup-router + (not conflicting) mixed-router + :else linear-router)] + (when-let [conflicts (:conflicts opts)] (when conflicting (conflicts conflicting))) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 6034767d..7fe61629 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -1,15 +1,15 @@ (ns reitit.core-test (: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 "mixed router" (let [router (reitit/router ["/api" ["/ipa" ["/:size" ::beer]]])] - (is (= :linear-router (reitit/router-type router))) + (is (= :mixed-router (reitit/router-type router))) (is (= [["/api/ipa/:size" {:name ::beer}]] (reitit/routes router))) (is (= true (map? (reitit/options router)))) @@ -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] @@ -175,10 +182,10 @@ (testing "all conflicts are returned" (is (= {["/a" {}] #{["/*d" {}] ["/:b" {}]}, ["/:b" {}] #{["/c" {}] ["/*d" {}]}, - ["/c" {}] #{["/*d" {}]}})) - (-> [["/a"] ["/:b"] ["/c"] ["/*d"]] - (reitit/resolve-routes {}) - (reitit/conflicting-routes))) + ["/c" {}] #{["/*d" {}]}} + (-> [["/a"] ["/:b"] ["/c"] ["/*d"]] + (reitit/resolve-routes {}) + (reitit/conflicting-routes))))) (testing "router with conflicting routes" (testing "throws by default" @@ -188,8 +195,4 @@ (reitit/router [["/a"] ["/a"]])))) (testing "can be configured to ignore" - (is (not - (nil? - (reitit/router - [["/a"] ["/a"]] - {:conflicts (constantly nil)}))))))) + (is (not (nil? (reitit/router [["/a"] ["/a"]] {:conflicts (constantly nil)}))))))) From dd6d0d5a1c55ebf8a5f5d8915ca1383338c6371d Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Tue, 22 Aug 2017 08:54:12 +0300 Subject: [PATCH 7/9] use linearrouter if all routes are wild --- src/reitit/core.cljc | 2 ++ test/cljc/reitit/core_test.cljc | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index 71d5ae87..a55fac91 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -256,9 +256,11 @@ routes (resolve-routes data opts) conflicting (conflicting-routes routes) 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)] diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 7fe61629..d292fc23 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -7,9 +7,9 @@ (deftest reitit-test - (testing "mixed router" + (testing "linear-router" (let [router (reitit/router ["/api" ["/ipa" ["/:size" ::beer]]])] - (is (= :mixed-router (reitit/router-type router))) + (is (= :linear-router (reitit/router-type router))) (is (= [["/api/ipa/:size" {:name ::beer}]] (reitit/routes router))) (is (= true (map? (reitit/options router)))) @@ -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}]] From f893be3d008c91aa57ca4c3ff40fefb1ae852f78 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Tue, 22 Aug 2017 13:10:16 +0300 Subject: [PATCH 8/9] Transcude names, fix ataraxy routes --- perf-test/clj/reitit/opensensors_routing_test.clj | 10 +++++----- src/reitit/core.cljc | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/perf-test/clj/reitit/opensensors_routing_test.clj b/perf-test/clj/reitit/opensensors_routing_test.clj index 8c2570d9..5487e043 100644 --- a/perf-test/clj/reitit/opensensors_routing_test.clj +++ b/perf-test/clj/reitit/opensensors_routing_test.clj @@ -202,10 +202,10 @@ "whoami" :test/route41 "login" :test/route51} "v2/" {"whoami" :test/route1 - ["users/" :user-id "/"] {"datasets" :test/route2 - "devices" :test/route25 - "topics" {"/bulk" :test/route29 - "" :test/route45}} + ["users/" :user-id] {"/datasets" :test/route2 + "/devices" :test/route25 + "/topics" {"/bulk" :test/route29 + "" :test/route45}} "public/" {["projects/" :project-id] {"/datasets" :test/route3 "" :test/route27} #_#_"messages/dataset/bulk" :test/route20 @@ -264,7 +264,7 @@ "/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]} diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index a55fac91..70246f22 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -86,7 +86,7 @@ (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))]) From b23bcfbd30ab9b4aac91a7b86e5901f4fd4c7de0 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Tue, 22 Aug 2017 16:44:05 +0300 Subject: [PATCH 9/9] Add comment based on Miikka's comments --- src/reitit/core.cljc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/reitit/core.cljc b/src/reitit/core.cljc index 70246f22..003c4040 100644 --- a/src/reitit/core.cljc +++ b/src/reitit/core.cljc @@ -61,6 +61,7 @@ (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 {}]