From dc34720fee98d964065b654999fd5e0e251deac7 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Sun, 3 Mar 2019 21:55:39 +0200 Subject: [PATCH] Fixed based on Miikka's review --- CHANGELOG.md | 2 +- modules/reitit-core/src/reitit/core.cljc | 3 +- modules/reitit-core/src/reitit/exception.cljc | 12 ++-- modules/reitit-core/src/reitit/spec.cljc | 2 +- modules/reitit-dev/src/reitit/dev/pretty.cljc | 63 ++++++++++--------- project.clj | 2 +- test/cljc/reitit/errors_test.cljc | 52 --------------- test/cljc/reitit/exception_test.cljc | 54 ++++++++++++++++ test/cljs/reitit/doo_runner.cljs | 2 + 9 files changed, 98 insertions(+), 94 deletions(-) delete mode 100644 test/cljc/reitit/errors_test.cljc create mode 100644 test/cljc/reitit/exception_test.cljc diff --git a/CHANGELOG.md b/CHANGELOG.md index b7003d83..55cab06d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ * new module for friendly router creation time exception handling * new option `:exception` in `r/router`, of type `Exception => Exception` (default `reitit.exception/exception`) - * new exception pretty-printer `reitit.dev.pretty/exception`, based on [fipp](https://github.com/brandonbloom/fipp) and [expund](https://github.com/bhb/expound) for human readable, newbie-friendly errors. + * new exception pretty-printer `reitit.dev.pretty/exception`, based on [fipp](https://github.com/brandonbloom/fipp) and [expound](https://github.com/bhb/expound) for human readable, newbie-friendly errors. #### Conflicting paths diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 8ce2d3dc..3c16434c 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -389,5 +389,4 @@ (router compiled-routes opts)) (catch #?(:clj Exception, :cljs js/Error) e - (let [exception (:exception opts)] - (throw (if exception (exception e) e)))))))) + (throw ((get opts :exception identity) e))))))) diff --git a/modules/reitit-core/src/reitit/exception.cljc b/modules/reitit-core/src/reitit/exception.cljc index b7c1c3e6..79cbad2b 100644 --- a/modules/reitit-core/src/reitit/exception.cljc +++ b/modules/reitit-core/src/reitit/exception.cljc @@ -7,28 +7,28 @@ ([type data] (throw (ex-info (str type) {:type type, :data data})))) -(defmulti format-type (fn [type _ _] type)) +(defmulti format-exception (fn [type _ _] type)) (defn exception [e] (let [data (ex-data e) - message (format-type (:type data) #?(:clj (.getMessage ^Exception e) :cljs (ex-message e)) (:data data))] - (ex-info message (or data {})))) + message (format-exception (:type data) #?(:clj (.getMessage ^Exception e) :cljs (ex-message e)) (:data data))] + (ex-info message (assoc (or data {}) ::cause e)))) ;; ;; Formatters ;; -(defmethod format-type :default [_ message data] +(defmethod format-exception :default [_ message data] (str message (if data (str "\n\n" (pr-str data))))) -(defmethod format-type :path-conflicts [_ _ conflicts] +(defmethod format-exception :path-conflicts [_ _ 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))) -(defmethod format-type :name-conflicts [_ _ conflicts] +(defmethod format-exception :name-conflicts [_ _ conflicts] (apply str "Router contains conflicting route names:\n\n" (mapv (fn [[name vals]] diff --git a/modules/reitit-core/src/reitit/spec.cljc b/modules/reitit-core/src/reitit/spec.cljc index 5d0ca66d..fbe340d9 100644 --- a/modules/reitit-core/src/reitit/spec.cljc +++ b/modules/reitit-core/src/reitit/spec.cljc @@ -123,7 +123,7 @@ ::invalid-route-data {:problems problems}))) -(defmethod exception/format-type :reitit.spec/invalid-route-data [_ _ {:keys [problems]}] +(defmethod exception/format-exception :reitit.spec/invalid-route-data [_ _ {:keys [problems]}] (apply str "Invalid route data:\n\n" (mapv (fn [{:keys [path scope data spec]}] diff --git a/modules/reitit-dev/src/reitit/dev/pretty.cljc b/modules/reitit-dev/src/reitit/dev/pretty.cljc index 62b358bf..8ea1f70b 100644 --- a/modules/reitit-dev/src/reitit/dev/pretty.cljc +++ b/modules/reitit-dev/src/reitit/dev/pretty.cljc @@ -1,6 +1,7 @@ (ns reitit.dev.pretty (:require [clojure.string :as str] [clojure.spec.alpha :as s] + [reitit.exception :as exception] [arrangement.core] ;; expound [expound.ansi] @@ -44,10 +45,10 @@ :apropos-namespace 243 :error 196}) -(defn -color [color & text] - (str "\033[38;5;" (colors color color) "m" (apply str text) "\u001B[0m")) - (comment + (defn- -color [color & text] + (str "\033[38;5;" (colors color color) "m" (apply str text) "\u001B[0m")) + (doseq [c (range 0 255)] (println (-color c "kikka") "->" c)) @@ -55,11 +56,10 @@ (println (-color c "kikka") "->" c n)) (doseq [[k v] expound.ansi/sgr-code] - (println (expound.ansi/sgr "kikka" k) "->" k)) - ) + (println (expound.ansi/sgr "kikka" k) "->" k))) -(defn -start [x] (str "\033[38;5;" x "m")) -(defn -end [] "\u001B[0m") +(defn- -start [x] (str "\033[38;5;" x "m")) +(defn- -end [] "\u001B[0m") (defn color [color & text] [:span @@ -78,25 +78,25 @@ (visit-unknown [this x] (fipp.visit/visit this (fipp.ednize/edn x))) - (visit-nil [this] + (visit-nil [_] (color :text "nil")) - (visit-boolean [this x] + (visit-boolean [_ x] (color :text (str x))) - (visit-string [this x] + (visit-string [_ x] (color :string (pr-str x))) - (visit-character [this x] + (visit-character [_ x] (color :text (pr-str x))) - (visit-symbol [this x] + (visit-symbol [_ x] (color :text (str x))) - (visit-keyword [this x] + (visit-keyword [_ x] (color :constant (pr-str x))) - (visit-number [this x] + (visit-number [_ x] (color :text (pr-str x))) (visit-seq [this x] @@ -136,18 +136,18 @@ [:align [:span "^" (fipp.visit/visit this m)] :line (fipp.visit/visit* this x)] (fipp.visit/visit* this x))) - (visit-var [this x] + (visit-var [_ x] [:text (str x)]) - (visit-pattern [this x] + (visit-pattern [_ x] [:text (pr-str x)]) (visit-record [this x] (fipp.visit/visit this (fipp.ednize/record->tagged x)))) -(defn ->printer +(defn printer ([] - (->printer nil)) + (printer nil)) ([options] (map->EdnPrinter (merge @@ -161,7 +161,7 @@ (defn pprint ([x] (pprint x {})) ([x options] - (let [printer (->printer (dissoc options :margin)) + (let [printer (printer (dissoc options :margin)) margin (apply str (take (:margin options 0) (repeat " ")))] (binding [*print-meta* false] (fipp.engine/pprint-document [:group margin [:group (fipp.visit/visit printer x)]] options))))) @@ -215,16 +215,17 @@ (footer printer)] printer))) -(defmulti format-type (fn [type _ _] type)) +(defmulti format-exception (fn [type _ _] type)) (defn exception [e] (let [data (-> e ex-data :data) - message (format-type (-> e ex-data :type) #?(:clj (.getMessage ^Exception e) :cljs (ex-message e)) data) - source (->> e Throwable->map :trace - (drop-while #(not= (name (first %)) "reitit.core$router")) - (drop-while #(= (name (first %)) "reitit.core$router")) - next first source-str)] - (ex-info (exception-str message source (->printer)) (or data {})))) + message (format-exception (-> e ex-data :type) #?(:clj (.getMessage ^Exception e) :cljs (ex-message e)) data) + source #?(:clj (->> e Throwable->map :trace + (drop-while #(not= (name (first %)) "reitit.core$router")) + (drop-while #(= (name (first %)) "reitit.core$router")) + next first source-str) + :cljs "unkwown")] + (ex-info (exception-str message source (printer)) (assoc (or data {}) ::exception/cause e)))) (defn de-expound-colors [^String s mappings] (let [s' (reduce @@ -259,10 +260,10 @@ ;; Formatters ;; -(defmethod format-type :default [_ message data] +(defmethod format-exception :default [_ message data] (into [:group (text message)] (if data [[:break] [:break] (edn data)]))) -(defmethod format-type :path-conflicts [_ _ conflicts] +(defmethod format-exception :path-conflicts [_ _ conflicts] [:group (text "Router contains conflicting route paths:") [:break] [:break] @@ -287,7 +288,7 @@ (color :white "https://cljdoc.org/d/metosin/reitit/CURRENT/doc/basics/route-conflicts") [:break]]) -(defmethod format-type :name-conflicts [_ _ conflicts] +(defmethod format-exception :name-conflicts [_ _ conflicts] [:group (text "Router contains conflicting route names:") [:break] [:break] @@ -308,9 +309,9 @@ (color :white "https://cljdoc.org/d/metosin/reitit/CURRENT/doc/basics/route-conflicts") [:break]]) -(defmethod format-type :reitit.spec/invalid-route-data [_ _ {:keys [problems]}] +(defmethod format-exception :reitit.spec/invalid-route-data [_ _ {:keys [problems]}] [:group - (text "Route data validation failed:") + (text "Invalid route data:") [:break] [:break] (into [:group] diff --git a/project.clj b/project.clj index b633ea5c..a8c4c7cf 100644 --- a/project.clj +++ b/project.clj @@ -69,7 +69,7 @@ :java-source-paths ["modules/reitit-core/java-src"] :dependencies [[org.clojure/clojure "1.10.0"] - [org.clojure/clojurescript "1.10.439"] + [org.clojure/clojurescript "1.10.520"] ;; modules dependencies [metosin/schema-tools] diff --git a/test/cljc/reitit/errors_test.cljc b/test/cljc/reitit/errors_test.cljc deleted file mode 100644 index 11839edb..00000000 --- a/test/cljc/reitit/errors_test.cljc +++ /dev/null @@ -1,52 +0,0 @@ -(ns reitit.errors-test - (:require [reitit.spec :as rs] - [reitit.core :as r] - [reitit.dev.pretty :as pretty] - [clojure.spec.alpha :as s])) - -(s/def ::role #{:admin :manager}) -(s/def ::roles (s/coll-of ::role :into #{})) -(s/def ::data (s/keys :req [::role ::roles])) - -(comment - - ;; route conflicts - (r/router - [["/:a/1"] - ["/1/:a"]] - {:exception pretty/exception}) - - ;; path conflicts - (r/router - [["/kikka" ::kikka] - ["/kukka" ::kikka]] - {:exception pretty/exception}) - - ;; - ;; trie - ;; - - ;; two terminators - (r/router - [["/{a}.pdf"] - ["/{a}-pdf"]] - {:exception pretty/exception}) - - ;; two following wilds - (r/router - ["/{a}{b}"] - {:exception pretty/exception}) - - ;; unclosed brackers - (r/router - ["/api/{ipa"] - {:exception pretty/exception}) - - ;; - ;; spec - ;; - - (r/router - ["/api/ipa" {::roles #{:adminz}}] - {:validate rs/validate - :exception pretty/exception})) diff --git a/test/cljc/reitit/exception_test.cljc b/test/cljc/reitit/exception_test.cljc new file mode 100644 index 00000000..462085d1 --- /dev/null +++ b/test/cljc/reitit/exception_test.cljc @@ -0,0 +1,54 @@ +(ns reitit.exception-test + (:require [clojure.test :refer [deftest testing is are]] + [reitit.spec :as rs] + [reitit.core :as r] + [reitit.dev.pretty :as pretty] + [clojure.spec.alpha :as s] + [reitit.exception :as exception]) + #?(:clj + (:import (clojure.lang ExceptionInfo)))) + +(s/def ::role #{:admin :manager}) +(s/def ::roles (s/coll-of ::role :into #{})) +(s/def ::data (s/keys :req [::role ::roles])) + +(deftest errors-test + + (are [exception] + (are [error routes opts] + (is (thrown-with-msg? + ExceptionInfo + error + (r/router + routes + (merge {:exception exception} opts)))) + + #"Router contains conflicting route paths" + [["/:a/1"] + ["/1/:a"]] + nil + + #"Router contains conflicting route names" + [["/kikka" ::kikka] + ["/kukka" ::kikka]] + nil + + #":reitit.trie/multiple-terminators" + [["/{a}.pdf"] + ["/{a}-pdf"]] + nil + + #":reitit.trie/following-parameters" + ["/{a}{b}"] + nil + + #":reitit.trie/unclosed-brackets" + ["/api/{ipa"] + nil + + #"Invalid route data" + ["/api/ipa" {::roles #{:adminz}}] + {:validate rs/validate}) + + exception/exception + pretty/exception)) diff --git a/test/cljs/reitit/doo_runner.cljs b/test/cljs/reitit/doo_runner.cljs index 3fbc392b..a8ddd1b8 100644 --- a/test/cljs/reitit/doo_runner.cljs +++ b/test/cljs/reitit/doo_runner.cljs @@ -6,6 +6,7 @@ reitit.middleware-test reitit.ring-test #_reitit.spec-test + reitit.exception-test reitit.frontend.core-test reitit.frontend.history-test reitit.frontend.controllers-test)) @@ -18,6 +19,7 @@ 'reitit.middleware-test 'reitit.ring-test #_'reitit.spec-test + 'reitit.exception-test 'reitit.frontend.core-test 'reitit.frontend.history-test 'reitit.frontend.controllers-test)