Fixed based on Miikka's review

This commit is contained in:
Tommi Reiman 2019-03-03 21:55:39 +02:00
parent 549d2a0f97
commit dc34720fee
9 changed files with 98 additions and 94 deletions

View file

@ -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

View file

@ -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)))))))

View file

@ -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]]

View file

@ -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]}]

View file

@ -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]

View file

@ -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]

View file

@ -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}))

View file

@ -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))

View file

@ -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)