Implement invalid route handling at api-option

* don't check at route creation time
* optionally check at api-creation time
* remove the *fail-on-missing-route-info* dynamic var

Author: ikitommi

CHANGELOG.md

@@ -29,7 +29,8 @@
 
 * **BREAKING** Vanilla Compojure routes will not produce any swagger-docs (as they do not satisfy the 
 `Routing` protocol. They can still be used for handling request, just without docs.
-  * **TODO**: a new api-level option to declare how to handle routes not satisfying the `Routing` protocol (fail, warn or ignore)
+  * a new api-level option `[:api :invalid-routes-fn]` to declare how to handle routes not satisfying
+  the `Routing` protocol. Default implementation logs invalid routes as WARNINGs.
 
 * **BREAKING** compojure.core imports are removed from `compojure.api.sweet`:
   * `let-request`, `routing`, `wrap-routes`

src/compojure/api/api.clj

@@ -5,32 +5,46 @@
             [compojure.api.routes :as routes]
             [compojure.api.common :as common]
             [clojure.tools.macro :as macro]
-            [ring.swagger.swagger2 :as swagger2]))
+            [ring.swagger.common :as rsc]))
+
+(def api-defaults
+  (merge
+    middleware/api-middleware-defaults
+    {:api {:invalid-routes-fn routes/log-invalid-child-routes}}))
 
 (defn
   ^{:doc (str
   "Returns a ring handler wrapped in compojure.api.middleware/api-middlware.
-  Creates the route-table at run-time and passes that into the request via
-  ring-swagger middlewares. The mounted api-middleware can be configured by
+  Creates the route-table at run-time and injects that into the request via
+  middlewares. Api and the mounted api-middleware can be configured by
   optional options map as the first parameter:
 
       (api
         {:formats [:json :edn]}
         (context \"/api\" []
           ...))
 
-  API middleware options:
+  ### direct api options:
+
+  - **:api**                       All api options are under `:api`.
+    - **:invalid-routes-fn**         A 2-arity function taking handler and a sequence of
+                                     invalid routes (not satisfying compojure.api.route.Routing)
+                                     setting value to nil ignores invalid routes completely.
+                                     defaults to `compojure.api.routes/log-invalid-child-routes`
+
+  ### api-middleware options
 
   " (:doc (meta #'compojure.api.middleware/api-middleware)))}
   api
   [& body]
   (let [[options handlers] (common/extract-parameters body)
+        options (rsc/deep-merge api-defaults options)
         handler (apply core/routes handlers)
-        routes (routes/get-routes handler)
+        routes (routes/get-routes handler (:api options))
         paths (-> routes routes/ring-swagger-paths swagger/transform-operations)
-        lookup (routes/route-lookup-table handler)
+        lookup (routes/route-lookup-table routes)
         api-handler (-> handler
-                        (middleware/api-middleware options)
+                        (middleware/api-middleware (dissoc options :api))
                         (middleware/wrap-options {:paths paths
                                                   :lookup lookup}))]
     (routes/create nil nil {} [handler] api-handler)))

src/compojure/api/common.clj

@@ -34,3 +34,9 @@
 
     :else
     [{} (seq c)]))
+
+(defn group-with
+  "Groups a sequence with predicate returning a tuple of sequences."
+  [pred coll]
+  [(seq (filter pred coll))
+   (seq (remove pred coll))])

src/compojure/api/routes.clj

@@ -4,6 +4,7 @@
             [cheshire.core :as json]
             [compojure.api.middleware :as mw]
             [compojure.api.impl.logging :as logging]
+            [compojure.api.common :as common]
             [ring.swagger.common :as rsc]
             [clojure.string :as str]
             [linked.core :as linked]
@@ -14,29 +15,40 @@
 ;; Route records
 ;;
 
-(def ^:dynamic *fail-on-missing-route-info* false)
-
 (defn- ->path [path]
   (if-not (= path "/") path))
 
 (defn- ->paths [p1 p2]
   (->path (str p1 (->path p2))))
 
 (defprotocol Routing
-  (get-routes [handler]))
+  (-get-routes [handler options]))
 
 (extend-protocol Routing
   nil
-  (get-routes [_] []))
+  (-get-routes [_ _] []))
+
+(defn filter-routes [{:keys [childs] :as handler} {:keys [invalid-routes-fn]}]
+  (let [[valid-childs invalid-childs] (common/group-with (partial satisfies? Routing) childs)]
+    (when (and invalid-routes-fn invalid-childs)
+      (invalid-routes-fn handler invalid-childs))
+    valid-childs))
+
+(defn get-routes
+  ([handler]
+   (get-routes handler nil))
+  ([handler options]
+   (-get-routes handler options)))
 
 (defrecord Route [path method info childs handler]
   Routing
-  (get-routes [_]
-    (if (seq childs)
-      (vec
-        (for [[p m i] (mapcat get-routes (filter (partial satisfies? Routing) childs))]
-          [(->paths path p) m (rsc/deep-merge info i)]))
-      (into [] (if path [[path method info]]))))
+  (-get-routes [this options]
+    (let [valid-childs (filter-routes this options)]
+      (if (seq childs)
+        (vec
+          (for [[p m i] (mapcat #(get-routes % options) valid-childs)]
+            [(->paths path p) m (rsc/deep-merge info i)]))
+        (into [] (if path [[path method info]])))))
 
   IFn
   (invoke [_ request]
@@ -45,17 +57,23 @@
     (AFn/applyToHelper this args)))
 
 (defn create [path method info childs handler]
-  (when-let [invalid-childs (seq (remove (partial satisfies? Routing) childs))]
-    (let [message "Not all child routes satisfy compojure.api.routing/Routing."
-          data {:path path
-                :method method
-                :info info
-                :invalid invalid-childs}]
-      (if *fail-on-missing-route-info*
-        (throw (ex-info message data))
-        (logging/log! :warn (str message ": " data)))))
   (->Route path method info childs handler))
 
+;;
+;; Invalid route handlers
+;;
+
+(defn fail-on-invalid-child-routes
+  [handler invalid-childs]
+  (throw (ex-info "Not all child routes satisfy compojure.api.routing/Routing."
+                  (merge (select-keys handler [:path :method]) {:invalid (vec invalid-childs)}))))
+
+(defn log-invalid-child-routes [handler invalid-childs]
+  (logging/log! :warn (str "Not all child routes satisfy compojure.api.routing/Routing. "
+                           (select-keys handler [:path :method]) ", invalid child routes: "
+                           (vec invalid-childs))))
+
+
 ;;
 ;; Swagger paths
 ;;
@@ -79,14 +97,14 @@
 (defn ring-swagger-paths [routes]
   {:paths
    (reduce
-    (fn [acc [path method info]]
-      (update-in
-        acc [path method]
-        (fn [old-info]
-          (let [info (or old-info info)]
-            (ensure-path-parameters path info)))))
-    (linked/map)
-    routes)})
+     (fn [acc [path method info]]
+       (update-in
+         acc [path method]
+         (fn [old-info]
+           (let [info (or old-info info)]
+             (ensure-path-parameters path info)))))
+     (linked/map)
+     routes)})
 
 ;;
 ;; Route lookup
@@ -96,8 +114,8 @@
   (for [[id freq] (frequencies seq)
         :when (> freq 1)] id))
 
-(defn route-lookup-table [handler]
-  (let [entries (for [[path endpoints] (-> handler get-routes ring-swagger-paths :paths)
+(defn route-lookup-table [routes]
+  (let [entries (for [[path endpoints] (-> routes ring-swagger-paths :paths)
                       [method {:keys [x-name parameters]}] endpoints
                       :let [params (:path parameters)]
                       :when x-name]

src/compojure/api/swagger.clj

@@ -90,6 +90,7 @@
 
 (defn swagger-spec-path [app]
   (some-> app
+          routes/get-routes
           routes/route-lookup-table
           ::swagger
           keys

test/compojure/api/common_test.clj

@@ -5,3 +5,7 @@
 (fact "map-of"
   (let [a 1 b true c [:abba :jabba]]
     (map-of a b c) => {:a 1 :b true :c [:abba :jabba]}))
+
+(fact "group-with"
+  (group-with pos? [1 -10 2 -4 -1 999]) => [[1 2 999] [-10 -4 -1]]
+  (group-with pos? [1 2 999]) => [[1 2 999] nil])

test/compojure/api/integration_test.clj

@@ -9,7 +9,8 @@
             [ring.swagger.core :as rsc]
             [ring.util.http-status :as status]
             [compojure.api.middleware :as mw]
-            [ring.swagger.middleware :as rsm]))
+            [ring.swagger.middleware :as rsm]
+            [compojure.api.routes :as routes]))
 
 ;;
 ;; Data
@@ -1106,3 +1107,19 @@
       (let [[status body] (get* app "/ping")]
         status => 200
         body => 1))))
+
+(fact "handling invalid routes with api"
+  (let [invalid-routes (routes (constantly nil))]
+
+    (fact "by default, logs the exception"
+      (api invalid-routes) => truthy
+      (provided
+        (compojure.api.impl.logging/log! :warn irrelevant) => irrelevant :times 1))
+
+    (fact "ignoring invalid routes doesn't log"
+      (api {:api {:invalid-routes-fn nil}} invalid-routes) => truthy
+      (provided
+        (compojure.api.impl.logging/log! :warn irrelevant) => irrelevant :times 0))
+
+    (fact "throwing exceptions"
+      (api {:api {:invalid-routes-fn routes/fail-on-invalid-child-routes}} invalid-routes)) => throws))

test/compojure/api/routes_test.clj

@@ -86,3 +86,19 @@
   (routes/get-routes (routes (routes))) => []
   (routes/get-routes (routes (swagger-ui))) => []
   (routes/get-routes (routes (routes (GET "/ping" [] "pong")))) => [["/ping" :get {}]])
+
+(fact "invalid route options"
+  (let [r (routes (constantly nil))]
+
+    (fact "ignore 'em all"
+      (routes/get-routes r) => []
+      (routes/get-routes r nil) => []
+      (routes/get-routes r {:invalid-routes-fn nil}) => [])
+
+    (fact "log warnings"
+      (routes/get-routes r {:invalid-routes-fn routes/log-invalid-child-routes}) => []
+      (provided
+        (compojure.api.impl.logging/log! :warn irrelevant) => irrelevant :times 1))
+
+    (fact "throw exception"
+      (routes/get-routes r {:invalid-routes-fn routes/fail-on-invalid-child-routes})) => throws))

test/compojure/api/swagger_test.clj

@@ -55,13 +55,12 @@
         (GET+ "/true" [] identity))) => {"/api/xxx/true" {:get {}}})
 
   (fact "Vanilla Compojure defroutes are NOT followed"
-    (ignore-non-documented-route-warning
-      (compojure.core/defroutes even-more-routes (GET "/even" [] identity))
-      (compojure.core/defroutes more-routes (context "/more" [] even-more-routes))
-      (extract-paths
-        (context "/api" []
-          (GET "/true" [] identity)
-          more-routes)) => {"/api/true" {:get {}}}))
+    (compojure.core/defroutes even-more-routes (GET "/even" [] identity))
+    (compojure.core/defroutes more-routes (context "/more" [] even-more-routes))
+    (extract-paths
+      (context "/api" []
+        (GET "/true" [] identity)
+        more-routes)) => {"/api/true" {:get {}}})
 
   (fact "Compojure Api defroutes and def routes are followed"
     (def even-more-routes (GET "/even" [] identity))

test/compojure/api/test_utils.clj

@@ -80,15 +80,6 @@
   (let [[status body] (raw-post* app uri "" nil headers)]
     [status (parse-body body)]))
 
-;;
-;; Route compilation
-;;
-
-(defmacro ignore-non-documented-route-warning [& body]
-  `(with-out-str
-     (binding [routes/*fail-on-missing-route-info* false]
-       ~@body)))
-
 ;;
 ;; get-spec
 ;;