From 1e5fb601da76199d1b7221dd17e72213db8e11c5 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Thu, 10 Mar 2022 13:42:53 +0200 Subject: [PATCH 1/4] fix #538 --- modules/reitit-core/src/reitit/core.cljc | 99 ++++++++++++++---------- test/cljc/reitit/core_test.cljc | 7 ++ 2 files changed, 66 insertions(+), 40 deletions(-) diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index c6776db4..78db2699 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -73,6 +73,23 @@ ;; Different routers ;; +(defn empty-router + "Creates an empty routes, not matching to anything" + ([] (empty-router nil)) + ([opts] + ^{:type ::router} + (reify Router + (router-name [_] + :empty-router) + (routes [_]) + (compiled-routes [_]) + (options [_] + opts) + (route-names [_]) + (match-by-path [_ _]) + (match-by-name [_ _]) + (match-by-name [_ _ _])))) + (defn linear-router "Creates a linear-router from resolved routes and optional expanded options. See [[router]] for available options, plus the following: @@ -180,46 +197,48 @@ ([compiled-routes] (trie-router compiled-routes {})) ([compiled-routes opts] - (let [compiler (::trie/trie-compiler opts (trie/compiler)) - names (impl/find-names compiled-routes opts) - [pl nl] (reduce - (fn [[pl nl] [p {:keys [name] :as data} result]] - (let [{:keys [path-params] :as route} (impl/parse p opts) - f #(if-let [path (impl/path-for route %)] - (->Match p data result (impl/url-decode-coll %) path) - (->PartialMatch p data result (impl/url-decode-coll %) path-params))] - [(trie/insert pl p (->Match p data result nil nil) opts) - (if name (assoc nl name f) nl)])) - [nil {}] - compiled-routes) - matcher (trie/compile pl compiler) - match-by-path (trie/path-matcher matcher compiler) - lookup (impl/fast-map nl) - routes (impl/uncompile-routes compiled-routes)] - ^{:type ::router} - (reify - Router - (router-name [_] - :trie-router) - (routes [_] - routes) - (compiled-routes [_] - compiled-routes) - (options [_] - opts) - (route-names [_] - names) - (match-by-path [_ path] - (if-let [match (match-by-path path)] - (-> (:data match) - (assoc :path-params (:params match)) - (assoc :path path)))) - (match-by-name [_ name] - (if-let [match (impl/fast-get lookup name)] - (match nil))) - (match-by-name [_ name path-params] - (if-let [match (impl/fast-get lookup name)] - (match (impl/path-params path-params)))))))) + (if-not compiled-routes + (empty-router opts) + (let [compiler (::trie/trie-compiler opts (trie/compiler)) + names (impl/find-names compiled-routes opts) + [pl nl] (reduce + (fn [[pl nl] [p {:keys [name] :as data} result]] + (let [{:keys [path-params] :as route} (impl/parse p opts) + f #(if-let [path (impl/path-for route %)] + (->Match p data result (impl/url-decode-coll %) path) + (->PartialMatch p data result (impl/url-decode-coll %) path-params))] + [(trie/insert pl p (->Match p data result nil nil) opts) + (if name (assoc nl name f) nl)])) + [nil {}] + compiled-routes) + matcher (trie/compile pl compiler) + match-by-path (trie/path-matcher matcher compiler) + lookup (impl/fast-map nl) + routes (impl/uncompile-routes compiled-routes)] + ^{:type ::router} + (reify + Router + (router-name [_] + :trie-router) + (routes [_] + routes) + (compiled-routes [_] + compiled-routes) + (options [_] + opts) + (route-names [_] + names) + (match-by-path [_ path] + (if-let [match (match-by-path path)] + (-> (:data match) + (assoc :path-params (:params match)) + (assoc :path path)))) + (match-by-name [_ name] + (if-let [match (impl/fast-get lookup name)] + (match nil))) + (match-by-name [_ name path-params] + (if-let [match (impl/fast-get lookup name)] + (match (impl/path-params path-params))))))))) (defn single-static-path-router "Creates a fast router of 1 static route(s) and optional diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 1780982c..4c044fa1 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -431,3 +431,10 @@ {:conflicts nil})] (is (= :root (-> (r/match-by-path router "/") :data :name))) (is (= :root (-> (r/match-by-path router2 "/") :data :name))))) + +(deftest routing-bug-test-538 + (let [router (r/router + [["/:a"] + ["/:b"]] + {:conflicts nil})] + (is (nil? (r/match-by-path router ""))))) From f9841363c562eea1ea7f4dd30c45d397bb4ddb8a Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Thu, 10 Mar 2022 14:19:30 +0200 Subject: [PATCH 2/4] faster impl, removes all intermediate steps + cleanup --- modules/reitit-core/src/reitit/core.cljc | 164 ++++++++--------------- test/cljc/reitit/core_test.cljc | 7 +- 2 files changed, 62 insertions(+), 109 deletions(-) diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 78db2699..026bb1ea 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -69,27 +69,25 @@ ([match query-params] (some-> match :path (cond-> (seq query-params) (str "?" (impl/query-string query-params)))))) +(defn- matcher [router1 router2] + (if (and router1 router2) + (reify + Router + (match-by-path [_ path] + (or (match-by-path router1 path) + (match-by-path router2 path))) + (match-by-name [_ name] + (or (match-by-name router1 name) + (match-by-name router2 name))) + (match-by-name [_ name path-params] + (or (match-by-name router1 name path-params) + (match-by-name router2 name path-params)))) + (or router1 router2))) + ;; ;; Different routers ;; -(defn empty-router - "Creates an empty routes, not matching to anything" - ([] (empty-router nil)) - ([opts] - ^{:type ::router} - (reify Router - (router-name [_] - :empty-router) - (routes [_]) - (compiled-routes [_]) - (options [_] - opts) - (route-names [_]) - (match-by-path [_ _]) - (match-by-name [_ _]) - (match-by-name [_ _ _])))) - (defn linear-router "Creates a linear-router from resolved routes and optional expanded options. See [[router]] for available options, plus the following: @@ -120,16 +118,11 @@ ^{:type ::router} (reify Router - (router-name [_] - :linear-router) - (routes [_] - routes) - (compiled-routes [_] - compiled-routes) - (options [_] - opts) - (route-names [_] - names) + (router-name [_] :linear-router) + (routes [_] routes) + (compiled-routes [_] compiled-routes) + (options [_] opts) + (route-names [_] names) (match-by-path [_ path] (if-let [match (match-by-path path)] (-> (:data match) @@ -167,16 +160,11 @@ routes (impl/uncompile-routes compiled-routes)] ^{:type ::router} (reify Router - (router-name [_] - :lookup-router) - (routes [_] - routes) - (compiled-routes [_] - compiled-routes) - (options [_] - opts) - (route-names [_] - names) + (router-name [_] :lookup-router) + (routes [_] routes) + (compiled-routes [_] compiled-routes) + (options [_] opts) + (route-names [_] names) (match-by-path [_ path] (impl/fast-get data path)) (match-by-name [_ name] @@ -197,8 +185,7 @@ ([compiled-routes] (trie-router compiled-routes {})) ([compiled-routes opts] - (if-not compiled-routes - (empty-router opts) + (when compiled-routes (let [compiler (::trie/trie-compiler opts (trie/compiler)) names (impl/find-names compiled-routes opts) [pl nl] (reduce @@ -218,16 +205,11 @@ ^{:type ::router} (reify Router - (router-name [_] - :trie-router) - (routes [_] - routes) - (compiled-routes [_] - compiled-routes) - (options [_] - opts) - (route-names [_] - names) + (router-name [_] :trie-router) + (routes [_] routes) + (compiled-routes [_] compiled-routes) + (options [_] opts) + (route-names [_] names) (match-by-path [_ path] (if-let [match (match-by-path path)] (-> (:data match) @@ -257,25 +239,17 @@ routes (impl/uncompile-routes compiled-routes)] ^{:type ::router} (reify Router - (router-name [_] - :single-static-path-router) - (routes [_] - routes) - (compiled-routes [_] - compiled-routes) - (options [_] - opts) - (route-names [_] - names) + (router-name [_] :single-static-path-router) + (routes [_] routes) + (compiled-routes [_] compiled-routes) + (options [_] opts) + (route-names [_] names) (match-by-path [_ path] - (if (#?(:clj .equals :cljs =) p path) - match)) + (if (#?(:clj .equals :cljs =) p path) match)) (match-by-name [_ name] - (if (= n name) - match)) + (if (= n name) match)) (match-by-name [_ name path-params] - (if (= n name) - (impl/fast-assoc match :path-params (impl/path-params path-params)))))))) + (if (= n name) (impl/fast-assoc match :path-params (impl/path-params path-params)))))))) (defn mixed-router "Creates two routers: [[lookup-router]] or [[single-static-path-router]] for @@ -290,28 +264,18 @@ wildcard-router (trie-router wild opts) static-router (->static-router lookup opts) names (impl/find-names compiled-routes opts) - routes (impl/uncompile-routes compiled-routes)] + routes (impl/uncompile-routes compiled-routes) + router (matcher static-router wildcard-router)] ^{:type ::router} (reify Router - (router-name [_] - :mixed-router) - (routes [_] - routes) - (compiled-routes [_] - compiled-routes) - (options [_] - opts) - (route-names [_] - names) - (match-by-path [_ path] - (or (match-by-path static-router path) - (match-by-path wildcard-router path))) - (match-by-name [_ name] - (or (match-by-name static-router name) - (match-by-name wildcard-router name))) - (match-by-name [_ name path-params] - (or (match-by-name static-router name path-params) - (match-by-name wildcard-router name path-params))))))) + (router-name [_] :mixed-router) + (routes [_] routes) + (compiled-routes [_] compiled-routes) + (options [_] opts) + (route-names [_] names) + (match-by-path [_ path] (match-by-path router path)) + (match-by-name [_ name] (match-by-name router name)) + (match-by-name [_ name path-params] (match-by-name router name path-params)))))) (defn quarantine-router "Creates two routers: [[mixed-router]] for non-conflicting routes @@ -326,28 +290,18 @@ linear-router (linear-router conflicting opts) mixed-router (mixed-router non-conflicting opts) names (impl/find-names compiled-routes opts) - routes (impl/uncompile-routes compiled-routes)] + routes (impl/uncompile-routes compiled-routes) + router (matcher mixed-router linear-router)] ^{:type ::router} (reify Router - (router-name [_] - :quarantine-router) - (routes [_] - routes) - (compiled-routes [_] - compiled-routes) - (options [_] - opts) - (route-names [_] - names) - (match-by-path [_ path] - (or (match-by-path mixed-router path) - (match-by-path linear-router path))) - (match-by-name [_ name] - (or (match-by-name mixed-router name) - (match-by-name linear-router name))) - (match-by-name [_ name path-params] - (or (match-by-name mixed-router name path-params) - (match-by-name linear-router name path-params))))))) + (router-name [_] :quarantine-router) + (routes [_] routes) + (compiled-routes [_] compiled-routes) + (options [_] opts) + (route-names [_] names) + (match-by-path [_ path] (match-by-path router path)) + (match-by-name [_ name] (match-by-name router name)) + (match-by-name [_ name path-params] (match-by-name router name path-params)))))) ;; ;; Creating Routers diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 4c044fa1..347d63d3 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -433,8 +433,7 @@ (is (= :root (-> (r/match-by-path router2 "/") :data :name))))) (deftest routing-bug-test-538 - (let [router (r/router - [["/:a"] - ["/:b"]] - {:conflicts nil})] + (let [router (r/router [["/:a"] ["/:b"]] {:conflicts nil})] (is (nil? (r/match-by-path router ""))))) + + From b0602d60c9d9c6790d2971f9b3436d3a19ed33bf Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Thu, 10 Mar 2022 15:21:15 +0200 Subject: [PATCH 3/4] one more time --- modules/reitit-core/src/reitit/core.cljc | 116 +++++++++++------------ modules/reitit-core/src/reitit/trie.cljc | 3 +- 2 files changed, 56 insertions(+), 63 deletions(-) diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 026bb1ea..0b32fd73 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -69,21 +69,6 @@ ([match query-params] (some-> match :path (cond-> (seq query-params) (str "?" (impl/query-string query-params)))))) -(defn- matcher [router1 router2] - (if (and router1 router2) - (reify - Router - (match-by-path [_ path] - (or (match-by-path router1 path) - (match-by-path router2 path))) - (match-by-name [_ name] - (or (match-by-name router1 name) - (match-by-name router2 name))) - (match-by-name [_ name path-params] - (or (match-by-name router1 name path-params) - (match-by-name router2 name path-params)))) - (or router1 router2))) - ;; ;; Different routers ;; @@ -185,42 +170,41 @@ ([compiled-routes] (trie-router compiled-routes {})) ([compiled-routes opts] - (when compiled-routes - (let [compiler (::trie/trie-compiler opts (trie/compiler)) - names (impl/find-names compiled-routes opts) - [pl nl] (reduce - (fn [[pl nl] [p {:keys [name] :as data} result]] - (let [{:keys [path-params] :as route} (impl/parse p opts) - f #(if-let [path (impl/path-for route %)] - (->Match p data result (impl/url-decode-coll %) path) - (->PartialMatch p data result (impl/url-decode-coll %) path-params))] - [(trie/insert pl p (->Match p data result nil nil) opts) - (if name (assoc nl name f) nl)])) - [nil {}] - compiled-routes) - matcher (trie/compile pl compiler) - match-by-path (trie/path-matcher matcher compiler) - lookup (impl/fast-map nl) - routes (impl/uncompile-routes compiled-routes)] - ^{:type ::router} - (reify - Router - (router-name [_] :trie-router) - (routes [_] routes) - (compiled-routes [_] compiled-routes) - (options [_] opts) - (route-names [_] names) - (match-by-path [_ path] - (if-let [match (match-by-path path)] - (-> (:data match) - (assoc :path-params (:params match)) - (assoc :path path)))) - (match-by-name [_ name] - (if-let [match (impl/fast-get lookup name)] - (match nil))) - (match-by-name [_ name path-params] - (if-let [match (impl/fast-get lookup name)] - (match (impl/path-params path-params))))))))) + (let [compiler (::trie/trie-compiler opts (trie/compiler)) + names (impl/find-names compiled-routes opts) + [pl nl] (reduce + (fn [[pl nl] [p {:keys [name] :as data} result]] + (let [{:keys [path-params] :as route} (impl/parse p opts) + f #(if-let [path (impl/path-for route %)] + (->Match p data result (impl/url-decode-coll %) path) + (->PartialMatch p data result (impl/url-decode-coll %) path-params))] + [(trie/insert pl p (->Match p data result nil nil) opts) + (if name (assoc nl name f) nl)])) + [nil {}] + compiled-routes) + matcher (trie/compile pl compiler) + match-by-path (if matcher (trie/path-matcher matcher compiler)) + lookup (impl/fast-map nl) + routes (impl/uncompile-routes compiled-routes)] + ^{:type ::router} + (reify + Router + (router-name [_] :trie-router) + (routes [_] routes) + (compiled-routes [_] compiled-routes) + (options [_] opts) + (route-names [_] names) + (match-by-path [_ path] + (if-let [match (and match-by-path (match-by-path path))] + (-> (:data match) + (assoc :path-params (:params match)) + (assoc :path path)))) + (match-by-name [_ name] + (if-let [match (impl/fast-get lookup name)] + (match nil))) + (match-by-name [_ name path-params] + (if-let [match (impl/fast-get lookup name)] + (match (impl/path-params path-params)))))))) (defn single-static-path-router "Creates a fast router of 1 static route(s) and optional @@ -264,8 +248,7 @@ wildcard-router (trie-router wild opts) static-router (->static-router lookup opts) names (impl/find-names compiled-routes opts) - routes (impl/uncompile-routes compiled-routes) - router (matcher static-router wildcard-router)] + routes (impl/uncompile-routes compiled-routes)] ^{:type ::router} (reify Router (router-name [_] :mixed-router) @@ -273,9 +256,15 @@ (compiled-routes [_] compiled-routes) (options [_] opts) (route-names [_] names) - (match-by-path [_ path] (match-by-path router path)) - (match-by-name [_ name] (match-by-name router name)) - (match-by-name [_ name path-params] (match-by-name router name path-params)))))) + (match-by-path [_ path] + (or (match-by-path static-router path) + (match-by-path wildcard-router path))) + (match-by-name [_ name] + (or (match-by-name static-router name) + (match-by-name wildcard-router name))) + (match-by-name [_ name path-params] + (or (match-by-name static-router name path-params) + (match-by-name wildcard-router name path-params))))))) (defn quarantine-router "Creates two routers: [[mixed-router]] for non-conflicting routes @@ -290,8 +279,7 @@ linear-router (linear-router conflicting opts) mixed-router (mixed-router non-conflicting opts) names (impl/find-names compiled-routes opts) - routes (impl/uncompile-routes compiled-routes) - router (matcher mixed-router linear-router)] + routes (impl/uncompile-routes compiled-routes)] ^{:type ::router} (reify Router (router-name [_] :quarantine-router) @@ -299,9 +287,15 @@ (compiled-routes [_] compiled-routes) (options [_] opts) (route-names [_] names) - (match-by-path [_ path] (match-by-path router path)) - (match-by-name [_ name] (match-by-name router name)) - (match-by-name [_ name path-params] (match-by-name router name path-params)))))) + (match-by-path [_ path] + (or (match-by-path mixed-router path) + (match-by-path linear-router path))) + (match-by-name [_ name] + (or (match-by-name mixed-router name) + (match-by-name linear-router name))) + (match-by-name [_ name path-params] + (or (match-by-name mixed-router name path-params) + (match-by-name linear-router name path-params))))))) ;; ;; Creating Routers diff --git a/modules/reitit-core/src/reitit/trie.cljc b/modules/reitit-core/src/reitit/trie.cljc index cda97066..22c6356b 100644 --- a/modules/reitit-core/src/reitit/trie.cljc +++ b/modules/reitit-core/src/reitit/trie.cljc @@ -364,8 +364,7 @@ (into (for [[p c] catch-all] (catch-all-matcher compiler (:value p) params (:data c)))))] (cond (> (count matchers) 1) (linear-matcher compiler matchers false) - (= (count matchers) 1) (first matchers) - :else (data-matcher compiler {} nil))))) + (= (count matchers) 1) (first matchers))))) (defn pretty "Returns a simplified EDN structure of a compiled trie for printing purposes." From 650ff3d6b307f896790f0af2ced478e2f335bede Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Thu, 10 Mar 2022 15:23:07 +0200 Subject: [PATCH 4/4] . --- test/cljc/reitit/core_test.cljc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 347d63d3..fc112239 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -435,5 +435,3 @@ (deftest routing-bug-test-538 (let [router (r/router [["/:a"] ["/:b"]] {:conflicts nil})] (is (nil? (r/match-by-path router ""))))) - -