From 16856749b198a4f73b800a4751037c7644370d7c Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Sat, 21 Jul 2018 08:49:20 +0300 Subject: [PATCH] handle name conflicts in router --- CHANGELOG.md | 1 + doc/basics/route_conflicts.md | 58 ++++++++++++++--- modules/reitit-core/src/reitit/core.cljc | 40 +++++++++--- modules/reitit-core/src/reitit/spec.cljc | 2 +- test/cljc/reitit/coercion_test.cljc | 9 +-- test/cljc/reitit/core_test.cljc | 81 ++++++++++++++---------- test/cljc/reitit/spec_test.cljc | 2 +- 7 files changed, 130 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e82d5080..d758611d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * **BREAKING**: the router option key to extract body format has been renamed: `:extract-request-format` => `:reitit.coercion/extract-request-format` * should only concern you if you are not using [Muuntaja](https://github.com/metosin/muuntaja). * the `r/routes` returns just the path + data tuples as documented, not the compiled route results. To get the compiled results, use `r/compiled-routes` instead. +* welcome route name conflict resolution! If router has routes with same names, router can't be created. fix 'em. ## `reitit-swagger` diff --git a/doc/basics/route_conflicts.md b/doc/basics/route_conflicts.md index 8a16ce29..a5f50125 100644 --- a/doc/basics/route_conflicts.md +++ b/doc/basics/route_conflicts.md @@ -1,10 +1,12 @@ # Route Conflicts -Most routing libraries allow conflicting paths within a router. On lookup, the first match is used making rest of the matching routes effecively unreachable. This is not good, especially if route tree is merged from multiple sources. +We should fast if a router contains conflicting paths or route names. -Reitit resolves this by running explicit conflicit resolution when a Router is created. Conflicting routes are passed into a `:conflicts` callback. Default implementation throws `ex-info` with a descriptive message. +When a `Router` is created via `reitit.core/router`, both path and route name conflicts are checked automatically. By default, in case of conflict, an `ex-info` is thrown with a descriptive message. Is some (legacy api) cases, path conflicts are should be allowed and one can override the path conflict resolution via `:conflicts` router option. -Examples router with conflicting routes: +## Path Conflicts + +Routes with path conflicts: ```clj (require '[reitit.core :as r]) @@ -17,11 +19,11 @@ Examples router with conflicting routes: ["/:version/status"]]) ``` -By default, `ExceptionInfo` is thrown: +Creating router with defaults: ```clj (r/router routes) -; CompilerException clojure.lang.ExceptionInfo: Router contains conflicting routes: +; CompilerException clojure.lang.ExceptionInfo: Router contains conflicting route paths: ; ; /:user-id/orders ; -> /public/*path @@ -35,22 +37,58 @@ By default, `ExceptionInfo` is thrown: ; ``` -Just logging the conflicts: +To ignore the conflicts: ```clj (r/router routes - {:conflicts (comp println reitit/conflicts-str)}) -; Router contains conflicting routes: + {:conflicts nil}) +; => #object[reitit.core$linear_router$reify] +``` + +To just log the conflicts: + +```clj +(r/router + routes + {:conflicts (fn [conflicts] + (println (r/path-conflicts-str conflicts)))}) +; Router contains conflicting route paths: ; ; /:user-id/orders ; -> /public/*path ; -> /bulk/:bulk-id ; -; /bulk/:bulk-id +; /bulk/:bulk-id ; -> /:version/status ; -; /public/*path +; /public/*path ; -> /:version/status ; +; => #object[reitit.core$linear_router$reify] +``` + +## Name conflicts + +Routes with name conflicts: + +```clj +(def routes + [["/ping" ::ping] + ["/admin" ::admin] + ["/admin/ping" ::ping]]) +``` + +Creating router with defaults: + +```clj +(r/router routes) +;CompilerException clojure.lang.ExceptionInfo: Router contains conflicting route names: +; +;:reitit.core/ping +;-> /ping +;-> /admin/ping +; ``` + +There is no way to disable the name conflict resolution. diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 88ae1f32..62a77c6d 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -63,7 +63,7 @@ coerce (into [] (keep #(coerce % opts))))) ;; This whole function might be more efficient and easier to understand with transducers. -(defn conflicting-routes [routes] +(defn path-conflicting-routes [routes] (some->> (loop [[r & rest] routes, acc {}] (if (seq rest) @@ -74,17 +74,33 @@ (seq) (into {}))) -(defn conflicts-str [conflicts] - (apply str "Router contains conflicting routes:\n\n" +(defn path-conflicts-str [conflicts] + (apply str "Router contains conflicting route paths:\n\n" (mapv (fn [[[path] vals]] (str " " path "\n-> " (str/join "\n-> " (mapv first vals)) "\n\n")) conflicts))) -(defn throw-on-conflicts! [conflicts] +(defn name-conflicting-routes [routes] + (some->> routes + (group-by (comp :name second)) + (remove (comp nil? first)) + (filter (comp pos? count butlast second)) + (seq) + (map (fn [[k v]] [k (set v)])) + (into {}))) + +(defn name-conflicts-str [conflicts] + (apply str "Router contains conflicting route names:\n\n" + (mapv + (fn [[name vals]] + (str name "\n-> " (str/join "\n-> " (mapv first vals)) "\n\n")) + conflicts))) + +(defn throw-on-conflicts! [f conflicts] (throw (ex-info - (conflicts-str conflicts) + (f conflicts) {:conflicts conflicts}))) (defn name-lookup [[_ {:keys [name]}] _] @@ -144,7 +160,7 @@ :expand expand :coerce (fn [route _] route) :compile (fn [[_ {:keys [handler]}] _] handler) - :conflicts throw-on-conflicts!}) + :conflicts (partial throw-on-conflicts! path-conflicts-str)}) (defn linear-router "Creates a linear-router from resolved routes and optional @@ -370,16 +386,17 @@ ([raw-routes] (router raw-routes {})) ([raw-routes opts] - (let [{:keys [router] :as opts} (meta-merge default-router-options opts) + (let [{:keys [router] :as opts} (merge default-router-options opts) routes (resolve-routes raw-routes opts) - conflicting (conflicting-routes routes) + path-conflicting (path-conflicting-routes routes) + name-conflicting (name-conflicting-routes routes) compiled-routes (compile-routes routes opts) wilds? (boolean (some impl/wild-route? compiled-routes)) all-wilds? (every? impl/wild-route? compiled-routes) router (cond router router (and (= 1 (count compiled-routes)) (not wilds?)) single-static-path-router - conflicting linear-router + path-conflicting linear-router (not wilds?) lookup-router all-wilds? segment-router :else mixed-router)] @@ -388,6 +405,9 @@ (validate compiled-routes opts)) (when-let [conflicts (:conflicts opts)] - (when conflicting (conflicts conflicting))) + (when path-conflicting (conflicts path-conflicting))) + + (when name-conflicting + (throw-on-conflicts! name-conflicts-str name-conflicting)) (router compiled-routes opts)))) diff --git a/modules/reitit-core/src/reitit/spec.cljc b/modules/reitit-core/src/reitit/spec.cljc index cf98cf24..1c0800f6 100644 --- a/modules/reitit-core/src/reitit/spec.cljc +++ b/modules/reitit-core/src/reitit/spec.cljc @@ -51,7 +51,7 @@ (s/def :reitit.router/expand fn?) (s/def :reitit.router/coerce fn?) (s/def :reitit.router/compile fn?) -(s/def :reitit.router/conflicts fn?) +(s/def :reitit.router/conflicts (s/nilable fn?)) (s/def :reitit.router/router fn?) (s/def ::opts diff --git a/test/cljc/reitit/coercion_test.cljc b/test/cljc/reitit/coercion_test.cljc index b286b286..d752de0c 100644 --- a/test/cljc/reitit/coercion_test.cljc +++ b/test/cljc/reitit/coercion_test.cljc @@ -12,18 +12,15 @@ (deftest coercion-test (let [r (r/router [["/schema" {:coercion reitit.coercion.schema/coercion} - ["/:number/:keyword" {:name ::user - :parameters {:path {:number s/Int + ["/:number/:keyword" {:parameters {:path {:number s/Int :keyword s/Keyword} :query (s/maybe {:int s/Int})}}]] ["/spec" {:coercion reitit.coercion.spec/coercion} - ["/:number/:keyword" {:name ::user - :parameters {:path {:number int? + ["/:number/:keyword" {:parameters {:path {:number int? :keyword keyword?} :query (ds/maybe {:int int?})}}]] ["/none" - ["/:number/:keyword" {:name ::user - :parameters {:path {:number int? + ["/:number/:keyword" {:parameters {:path {:number int? :keyword keyword?}}}]]] {:compile coercion/compile-request-coercers})] diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index df3c4278..19091509 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -197,55 +197,66 @@ (r/match-by-path router "/api/user/1/2")))))) (deftest conflicting-routes-test - (are [conflicting? data] - (let [routes (r/resolve-routes data {}) - conflicts (-> routes (r/resolve-routes {}) (r/conflicting-routes))] - (if conflicting? (seq conflicts) (nil? conflicts))) + (testing "path conflicts" + (are [conflicting? data] + (let [routes (r/resolve-routes data {}) + conflicts (-> routes + (r/resolve-routes {}) + (r/path-conflicting-routes))] + (if conflicting? (seq conflicts) (nil? conflicts))) - true [["/a"] - ["/a"]] + true [["/a"] + ["/a"]] - true [["/a"] - ["/:b"]] + true [["/a"] + ["/:b"]] - true [["/a"] - ["/*b"]] + true [["/a"] + ["/*b"]] - true [["/a/1/2"] - ["/*b"]] + true [["/a/1/2"] + ["/*b"]] - false [["/a"] - ["/a/"]] + false [["/a"] + ["/a/"]] - false [["/a"] - ["/a/1"]] + false [["/a"] + ["/a/1"]] - false [["/a"] - ["/a/:b"]] + false [["/a"] + ["/a/:b"]] - false [["/a"] - ["/a/*b"]] + false [["/a"] + ["/a/*b"]] - true [["/v2/public/messages/dataset/bulk"] - ["/v2/public/messages/dataset/:dataset-id"]]) + 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"]] - (r/resolve-routes {}) - (r/conflicting-routes))))) + (testing "all conflicts are returned" + (is (= {["/a" {}] #{["/*d" {}] ["/:b" {}]}, + ["/:b" {}] #{["/c" {}] ["/*d" {}]}, + ["/c" {}] #{["/*d" {}]}} + (-> [["/a"] ["/:b"] ["/c"] ["/*d"]] + (r/resolve-routes {}) + (r/path-conflicting-routes))))) - (testing "router with conflicting routes" - (testing "throws by default" + (testing "router with conflicting routes" + (testing "throws by default" + (is (thrown-with-msg? + ExceptionInfo + #"Router contains conflicting route paths" + (r/router + [["/a"] ["/a"]])))) + (testing "can be configured to ignore" + (is (not (nil? (r/router [["/a"] ["/a"]] {:conflicts nil}))))))) + + (testing "name conflicts" + (testing "router with conflicting routes always throws" (is (thrown-with-msg? ExceptionInfo - #"Router contains conflicting routes" + #"Router contains conflicting route names" (r/router - [["/a"] ["/a"]])))) - (testing "can be configured to ignore" - (is (not (nil? (r/router [["/a"] ["/a"]] {:conflicts (constantly nil)}))))))) + [["/1" ::1] ["/2" ::1]])))))) (deftest match->path-test (let [router (r/router ["/:a/:b" ::route])] diff --git a/test/cljc/reitit/spec_test.cljc b/test/cljc/reitit/spec_test.cljc index 880328e5..38988e77 100644 --- a/test/cljc/reitit/spec_test.cljc +++ b/test/cljc/reitit/spec_test.cljc @@ -79,7 +79,7 @@ {:expand nil} {:coerce nil} {:compile nil} - {:conflicts nil} + {:conflicts "invalid"} {:router nil})))) (deftest route-data-validation-test