handle name conflicts in router

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`
 

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,13 +37,23 @@ By default, `ExceptionInfo` is thrown:
 ;
 ```
 
-Just logging the conflicts:
+To ignore the conflicts:
 
 ```clj
 (r/router
   routes
-  {:conflicts (comp println reitit/conflicts-str)})
+  {:conflicts nil})
-; Router contains conflicting routes:
+; => #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
@@ -53,4 +65,30 @@ Just logging the conflicts:
 ; /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.

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]
+(defn path-conflicts-str [conflicts]
-  (apply str "Router contains conflicting routes:\n\n"
+  (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))))

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

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
+              ["/:number/:keyword" {:parameters {:path {:number s/Int
-                                    :parameters {:path {:number s/Int
                                                         :keyword s/Keyword}
                                                  :query (s/maybe {:int s/Int})}}]]
              ["/spec" {:coercion reitit.coercion.spec/coercion}
-              ["/:number/:keyword" {:name ::user
+              ["/:number/:keyword" {:parameters {:path {:number int?
-                                    :parameters {:path {:number int?
                                                         :keyword keyword?}
                                                  :query (ds/maybe {:int int?})}}]]
              ["/none"
-              ["/:number/:keyword" {:name ::user
+              ["/:number/:keyword" {:parameters {:path {:number int?
-                                    :parameters {:path {:number int?
                                                         :keyword keyword?}}}]]]
             {:compile coercion/compile-request-coercers})]
 

test/cljc/reitit/core_test.cljc

@@ -197,9 +197,12 @@
              (r/match-by-path router "/api/user/1/2"))))))
 
 (deftest conflicting-routes-test
+  (testing "path conflicts"
     (are [conflicting? data]
       (let [routes (r/resolve-routes data {})
-          conflicts (-> routes (r/resolve-routes {}) (r/conflicting-routes))]
+            conflicts (-> routes
+                          (r/resolve-routes {})
+                          (r/path-conflicting-routes))]
         (if conflicting? (seq conflicts) (nil? conflicts)))
 
       true [["/a"]
@@ -235,17 +238,25 @@
               ["/c" {}] #{["/*d" {}]}}
              (-> [["/a"] ["/:b"] ["/c"] ["/*d"]]
                  (r/resolve-routes {})
-               (r/conflicting-routes)))))
+                 (r/path-conflicting-routes)))))
 
     (testing "router with conflicting routes"
       (testing "throws by default"
         (is (thrown-with-msg?
               ExceptionInfo
-            #"Router contains conflicting routes"
+              #"Router contains conflicting route paths"
               (r/router
                 [["/a"] ["/a"]]))))
       (testing "can be configured to ignore"
-      (is (not (nil? (r/router [["/a"] ["/a"]] {:conflicts (constantly nil)})))))))
+        (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 route names"
+            (r/router
+              [["/1" ::1] ["/2" ::1]]))))))
 
 (deftest match->path-test
   (let [router (r/router ["/:a/:b" ::route])]

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