From 8019cebdc745cd0c197b65b3e7b80ffa971c6ae8 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 20 Nov 2017 08:11:18 +0200 Subject: [PATCH 01/17] Segment-router to rule 'em all --- modules/reitit-core/src/reitit/segment.cljc | 50 +++++++++++++++++++ .../clj/reitit/prefix_tree_perf_test.clj | 21 +++++++- 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 modules/reitit-core/src/reitit/segment.cljc diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc new file mode 100644 index 00000000..c6300103 --- /dev/null +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -0,0 +1,50 @@ +(ns reitit.segment + (:require [reitit.impl :as impl])) + +(defrecord Match [data params]) + +(defprotocol Segment + (-insert [this ps data]) + (-lookup [this ps params])) + +(extend-protocol Segment + nil + (-insert [this ps data]) + (-lookup [this ps params])) + +(defn- segments [^String path] + (mapv + (fn [^String p] + (if (impl/wild-param? p) (-> p (subs 1) keyword) p)) + (.split path "/"))) + +;; TODO: catch-all +(defn- segment + ([] + (segment {} #{} nil)) + ([children wilds data] + (let [children' (impl/fast-map children)] + (reify + Segment + (-insert [_ [p & ps] data] + (if-not p + (segment children wilds data) + (let [wilds (if (keyword? p) (conj wilds p) wilds) + children (update children p #(-insert (or % (segment)) ps data))] + (segment children wilds data)))) + (-lookup [_ [p & ps] params] + (if (nil? p) + (assoc data :params params) + (or (-lookup (impl/fast-get children' p) ps params) + (some #(-lookup (impl/fast-get children' %) ps (assoc params % p)) wilds)))))))) + +(defn create [paths] + (reduce + (fn [segment [p data]] + (let [ps (segments p)] + (-insert segment ps (map->Match {:data data})))) + (segment) paths)) + +(defn lookup [segment ^String path] + (let [ps (.split path "/")] + (-lookup segment ps {}))) diff --git a/perf-test/clj/reitit/prefix_tree_perf_test.clj b/perf-test/clj/reitit/prefix_tree_perf_test.clj index c607d675..7996201f 100644 --- a/perf-test/clj/reitit/prefix_tree_perf_test.clj +++ b/perf-test/clj/reitit/prefix_tree_perf_test.clj @@ -2,6 +2,7 @@ (:require [clojure.test :refer :all] [io.pedestal.http.route.prefix-tree :as p] [reitit.trie :as trie] + [reitit.segment :as segment] [criterium.core :as cc])) ;; @@ -75,6 +76,9 @@ (trie/insert acc p d)) nil routes)) +(def reitit-segment + (segment/create routes)) + (defn bench! [] ;; 2.3ms @@ -99,7 +103,22 @@ ;; 0.8ms (fix payloads) (cc/quick-bench (dotimes [_ 1000] - (trie/lookup reitit-tree "/v1/orgs/1/topics" {})))) + (trie/lookup reitit-tree "/v1/orgs/1/topics" {}))) + + ;; 0.9ms (initial) + ;; 0.5ms (protocols) + ;; 1.0ms (with path params) + ;; 1.0ms (Match records) + ;; 0.63ms (Single sweep path params) + ;; 0.51ms (Cleanup) + (cc/quick-bench + (dotimes [_ 1000] + (segment/lookup reitit-segment "/v1/orgs/1/topics")))) (comment (bench!)) + +(comment + (p/lookup pedestal-tree "/v1/orgs/1/topics") + (trie/lookup reitit-tree "/v1/orgs/1/topics" {}) + (segment/lookup reitit-segment "/v1/orgs/1/topics")) From 636c7ecd24e5120a8375fa3c05fea2195e6ee540 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Thu, 23 Nov 2017 15:52:59 +0200 Subject: [PATCH 02/17] Clean up internals --- modules/reitit-core/src/reitit/impl.cljc | 75 ++++++++---------------- modules/reitit-core/src/reitit/trie.cljc | 32 +++++++--- 2 files changed, 50 insertions(+), 57 deletions(-) diff --git a/modules/reitit-core/src/reitit/impl.cljc b/modules/reitit-core/src/reitit/impl.cljc index 70c44fe8..3607d81b 100644 --- a/modules/reitit-core/src/reitit/impl.cljc +++ b/modules/reitit-core/src/reitit/impl.cljc @@ -15,6 +15,31 @@ [clojure.set :as set]) (:import #?(:clj (java.util.regex Pattern)))) +(defn wild? [s] + (contains? #{\: \*} (first (str s)))) + +(defn catch-all? [s] + (= \* (first (str s)))) + +(defn wild-param [s] + (let [ss (str s)] + (if (= \: (first ss)) + (keyword (subs ss 1))))) + +(defn catch-all-param [s] + (let [ss (str s)] + (if (= \* (first ss)) + (keyword ss)))) + +(defn wild-or-catch-all-param? [x] + (boolean (or (wild-param x) (catch-all-param x)))) + +(defn segments [^String path] + (into [] (.split path "/" 666))) + +(defn contains-wilds? [path] + (boolean (some wild-or-catch-all-param? (segments path)))) + ;; ;; https://github.com/pedestal/pedestal/blob/master/route/src/io/pedestal/http/route/path.clj ;; @@ -65,48 +90,6 @@ (when-let [m (re-matches path-re path)] (zipmap path-params (rest m)))))) -;; -;; (c) https://github.com/pedestal/pedestal/blob/master/route/src/io/pedestal/http/route/prefix_tree.clj -;; - -(defn wild? [s] - (contains? #{\: \*} (first s))) - -(defn wild-param? - "Return true if a string segment starts with a wildcard string." - [segment] - (= \: (first segment))) - -(defn catch-all-param? - "Return true if a string segment starts with a catch-all string." - [segment] - (= \* (first segment))) - -(defn partition-wilds - "Given a path-spec string, return a seq of strings with wildcards - and catch-alls separated into their own strings. Eats the forward - slash following a wildcard." - [path-spec] - (let [groups (partition-by wild? (str/split path-spec #"/")) - first-groups (butlast groups) - last-group (last groups)] - (flatten - (conj (mapv #(if (wild? (first %)) - % - (str (str/join "/" %) "/")) - first-groups) - (if (wild? (first last-group)) - last-group - (str/join "/" last-group)))))) - -(defn contains-wilds? - "Return true if the given path-spec contains any wildcard params or - catch-alls." - [path-spec] - (let [parts (partition-wilds path-spec)] - (or (> (count parts) 1) - (wild? (first parts))))) - ;; ;; Routing (c) Metosin ;; @@ -129,14 +112,6 @@ :path-params :params}) (map->Route $)))) -(defn segments [path] - (let [ss (-> (str/split path #"/") rest vec)] - (if (str/ends-with? path "/") - (conj ss "") ss))) - -(defn- catch-all? [segment] - (= \* (first segment))) - (defn wild-route? [[path]] (contains-wilds? path)) diff --git a/modules/reitit-core/src/reitit/trie.cljc b/modules/reitit-core/src/reitit/trie.cljc index a0a14776..6f2208d3 100644 --- a/modules/reitit-core/src/reitit/trie.cljc +++ b/modules/reitit-core/src/reitit/trie.cljc @@ -1,5 +1,6 @@ (ns reitit.trie - (:require [reitit.impl :as impl])) + (:require [reitit.impl :as impl] + [clojure.string :as str])) ;; ;; original https://github.com/pedestal/pedestal/blob/master/route/src/io/pedestal/http/route/prefix_tree.clj @@ -116,24 +117,41 @@ tree node." [segment data] (cond - (impl/wild-param? segment) + (impl/wild-param segment) (wild-node segment (keyword (subs segment 1)) nil data) - (impl/catch-all-param? segment) + (impl/catch-all-param segment) (catch-all-node segment (keyword (subs segment 1)) nil data) :else (static-node segment nil data))) +(defn partition-wilds + "Given a path-spec string, return a seq of strings with wildcards + and catch-alls separated into their own strings. Eats the forward + slash following a wildcard." + [path-spec] + (let [groups (partition-by impl/wild? (str/split path-spec #"/")) + first-groups (butlast groups) + last-group (last groups)] + (flatten + (conj (mapv #(if (impl/wild? (first %)) + % + (str (str/join "/" %) "/")) + first-groups) + (if (impl/wild? (first last-group)) + last-group + (str/join "/" last-group)))))) + (defn- new-node "Given a path-spec and a payload object, return a new tree node. If the path-spec contains wildcards or catch-alls, will return parent node of a tree (linked list)." [path-spec data] (if (impl/contains-wilds? path-spec) - (let [parts (impl/partition-wilds path-spec)] + (let [parts (partition-wilds path-spec)] (reduce (fn [child segment] - (when (impl/catch-all-param? segment) + (when (impl/catch-all-param segment) (throw (ex-info "catch-all may only appear at the end of a path spec" {:patch-spec path-spec}))) (-> (make-node segment nil) @@ -187,7 +205,7 @@ (set-data node data) ;; handle case where path-spec is a wildcard param - (impl/wild-param? path-spec) + (impl/wild-param path-spec) (let [lcs (calc-lcs segment path-spec) common (subs path-spec 0 lcs)] (if (= common segment) @@ -200,7 +218,7 @@ ;; in the case where path-spec is a catch-all, node should always be nil. ;; getting here means we have an invalid route specification - (impl/catch-all-param? path-spec) + (impl/catch-all-param path-spec) (throw (ex-info "route conflict" {:node node :path-spec path-spec From ec35c2ebbf5babe9cd77b7564d0448ce46ddc25a Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Thu, 23 Nov 2017 16:01:40 +0200 Subject: [PATCH 03/17] Polish segment-router --- modules/reitit-core/src/reitit/segment.cljc | 24 +++++++++++---------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc index c6300103..ac27305f 100644 --- a/modules/reitit-core/src/reitit/segment.cljc +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -12,12 +12,6 @@ (-insert [this ps data]) (-lookup [this ps params])) -(defn- segments [^String path] - (mapv - (fn [^String p] - (if (impl/wild-param? p) (-> p (subs 1) keyword) p)) - (.split path "/"))) - ;; TODO: catch-all (defn- segment ([] @@ -29,22 +23,30 @@ (-insert [_ [p & ps] data] (if-not p (segment children wilds data) - (let [wilds (if (keyword? p) (conj wilds p) wilds) - children (update children p #(-insert (or % (segment)) ps data))] - (segment children wilds data)))) + (let [wild (impl/wild-param p) + wilds (if wild (conj wilds wild) wilds) + children (update children (or wild p) #(-insert (or % (segment)) ps data))] + (segment children wilds nil)))) (-lookup [_ [p & ps] params] (if (nil? p) - (assoc data :params params) + (if data (assoc data :params params)) (or (-lookup (impl/fast-get children' p) ps params) (some #(-lookup (impl/fast-get children' %) ps (assoc params % p)) wilds)))))))) (defn create [paths] (reduce (fn [segment [p data]] - (let [ps (segments p)] + (let [ps (impl/segments p)] (-insert segment ps (map->Match {:data data})))) (segment) paths)) (defn lookup [segment ^String path] (let [ps (.split path "/")] (-lookup segment ps {}))) + +(comment + (-> [["/:abba" 1] + ["/kikka/*kakka" 2]] + (create) + (lookup "/kikka") + (./aprint))) From 102fd35f04e59da2da62a087b7553e868cd1a96a Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Fri, 24 Nov 2017 07:34:14 +0200 Subject: [PATCH 04/17] Functional segemnt-router --- modules/reitit-core/src/reitit/impl.cljc | 2 +- modules/reitit-core/src/reitit/segment.cljc | 41 ++++++++++++--------- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/modules/reitit-core/src/reitit/impl.cljc b/modules/reitit-core/src/reitit/impl.cljc index 3607d81b..7bbae117 100644 --- a/modules/reitit-core/src/reitit/impl.cljc +++ b/modules/reitit-core/src/reitit/impl.cljc @@ -29,7 +29,7 @@ (defn catch-all-param [s] (let [ss (str s)] (if (= \* (first ss)) - (keyword ss)))) + (keyword (subs ss 1))))) (defn wild-or-catch-all-param? [x] (boolean (or (wild-param x) (catch-all-param x)))) diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc index ac27305f..a418afe0 100644 --- a/modules/reitit-core/src/reitit/segment.cljc +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -1,5 +1,6 @@ (ns reitit.segment - (:require [reitit.impl :as impl])) + (:require [reitit.impl :as impl] + [clojure.string :as str])) (defrecord Match [data params]) @@ -12,41 +13,45 @@ (-insert [this ps data]) (-lookup [this ps params])) -;; TODO: catch-all +(defn- -catch-all [catch-all data params p ps] + (if catch-all + (assoc data :params (assoc params catch-all (str/join "/" (cons p ps)))))) + (defn- segment - ([] - (segment {} #{} nil)) - ([children wilds data] + ([] (segment {} #{} nil nil)) + ([children wilds catch-all data] (let [children' (impl/fast-map children)] + ^{:type ::segment} (reify Segment - (-insert [_ [p & ps] data] + (-insert [_ [p & ps] d] (if-not p - (segment children wilds data) - (let [wild (impl/wild-param p) - wilds (if wild (conj wilds wild) wilds) - children (update children (or wild p) #(-insert (or % (segment)) ps data))] - (segment children wilds nil)))) + (segment children wilds catch-all d) + (let [[w c] ((juxt impl/wild-param impl/catch-all-param) p) + wilds (if w (conj wilds w) wilds) + catch-all (or c catch-all) + children (update children (or w c p) #(-insert (or % (segment)) ps d))] + (segment children wilds catch-all data)))) (-lookup [_ [p & ps] params] (if (nil? p) (if data (assoc data :params params)) (or (-lookup (impl/fast-get children' p) ps params) - (some #(-lookup (impl/fast-get children' %) ps (assoc params % p)) wilds)))))))) + (some #(-lookup (impl/fast-get children' %) ps (assoc params % p)) wilds) + (-catch-all catch-all data params p ps)))))))) (defn create [paths] (reduce (fn [segment [p data]] - (let [ps (impl/segments p)] - (-insert segment ps (map->Match {:data data})))) + (-insert segment (impl/segments p) (map->Match {:data data}))) (segment) paths)) (defn lookup [segment ^String path] - (let [ps (.split path "/")] - (-lookup segment ps {}))) + (-lookup segment (.split path "/") {})) (comment (-> [["/:abba" 1] - ["/kikka/*kakka" 2]] + ["/:abba/:dabba" 2] + ["/kikka/*kakka" 3]] (create) - (lookup "/kikka") + (lookup "/kikka/1/2") (./aprint))) From 5d7670de604540ba0bd736ad8ed0a03a22446cdf Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Fri, 24 Nov 2017 09:01:05 +0200 Subject: [PATCH 05/17] Welcome segment-router! --- modules/reitit-core/src/reitit/core.cljc | 42 +++++++++++ modules/reitit-core/src/reitit/segment.cljc | 7 +- test/cljc/reitit/core_test.cljc | 79 +++++++++++++-------- 3 files changed, 95 insertions(+), 33 deletions(-) diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 5447ef6b..0dc5d6a7 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -2,6 +2,7 @@ (:require [meta-merge.core :refer [meta-merge]] [clojure.string :as str] [reitit.trie :as trie] + [reitit.segment :as segment] [reitit.impl :as impl #?@(:cljs [:refer [Route]])]) #?(:clj (:import (reitit.impl Route)))) @@ -260,6 +261,47 @@ (if-let [match (impl/fast-get lookup name)] (match params))))))) +(defn segment-router + "Creates a special prefix-tree style segment router from resolved routes and optional + expanded options. See [[router]] for available options" + ([routes] + (segment-router routes {})) + ([routes opts] + (let [compiled (compile-routes routes opts) + names (find-names routes opts) + [pl nl] (reduce + (fn [[pl nl] [p {:keys [name] :as data} result]] + (let [{:keys [params] :as route} (impl/create [p data result]) + f #(if-let [path (impl/path-for route %)] + (->Match p data result % path) + (->PartialMatch p data result % params))] + [(segment/insert pl p (->Match p data result nil nil)) + (if name (assoc nl name f) nl)])) + [nil {}] compiled) + lookup (impl/fast-map nl)] + ^{:type ::router} + (reify + Router + (router-name [_] + :segment-router) + (routes [_] + compiled) + (options [_] + opts) + (route-names [_] + names) + (match-by-path [_ path] + (if-let [match (segment/lookup pl path)] + (-> (:data match) + (assoc :params (:params match)) + (assoc :path path)))) + (match-by-name [_ name] + (if-let [match (impl/fast-get lookup name)] + (match nil))) + (match-by-name [_ name params] + (if-let [match (impl/fast-get lookup name)] + (match params))))))) + (defn single-static-path-router "Creates a fast router of 1 static route(s) and optional expanded options. See [[router]] for available options" diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc index a418afe0..dc84b06c 100644 --- a/modules/reitit-core/src/reitit/segment.cljc +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -39,11 +39,14 @@ (some #(-lookup (impl/fast-get children' %) ps (assoc params % p)) wilds) (-catch-all catch-all data params p ps)))))))) +(defn insert [root path data] + (-insert (or root (segment)) (impl/segments path) (map->Match {:data data}))) + (defn create [paths] (reduce (fn [segment [p data]] - (-insert segment (impl/segments p) (map->Match {:data data}))) - (segment) paths)) + (insert segment p data)) + nil paths)) (defn lookup [segment ^String path] (-lookup segment (.split path "/") {})) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index b9442546..9f684faa 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -9,41 +9,57 @@ (testing "routers handling wildcard paths" (are [r name] - (let [router (r/router ["/api" ["/ipa" ["/:size" ::beer]]] {:router r})] - (is (= name (r/router-name router))) - (is (= [["/api/ipa/:size" {:name ::beer} nil]] - (r/routes router))) - (is (= true (map? (r/options router)))) - (is (= (r/map->Match - {:template "/api/ipa/:size" - :data {:name ::beer} - :path "/api/ipa/large" - :params {:size "large"}}) - (r/match-by-path router "/api/ipa/large"))) - (is (= (r/map->Match - {:template "/api/ipa/:size" - :data {:name ::beer} - :path "/api/ipa/large" - :params {:size "large"}}) - (r/match-by-name router ::beer {:size "large"}))) - (is (= nil (r/match-by-name router "ILLEGAL"))) - (is (= [::beer] (r/route-names router))) + (testing "wild" - (testing "name-based routing with missing parameters" - (is (= (r/map->PartialMatch - {:template "/api/ipa/:size" - :data {:name ::beer} - :required #{:size} - :params nil}) - (r/match-by-name router ::beer))) - (is (= true (r/partial-match? (r/match-by-name router ::beer)))) - (is (thrown-with-msg? - ExceptionInfo - #"^missing path-params for route /api/ipa/:size -> \#\{:size\}$" - (r/match-by-name! router ::beer))))) + (testing "simple" + (let [router (r/router ["/api" ["/ipa" ["/:size" ::beer]]] {:router r})] + (is (= name (r/router-name router))) + (is (= [["/api/ipa/:size" {:name ::beer} nil]] + (r/routes router))) + (is (= true (map? (r/options router)))) + (is (= (r/map->Match + {:template "/api/ipa/:size" + :data {:name ::beer} + :path "/api/ipa/large" + :params {:size "large"}}) + (r/match-by-path router "/api/ipa/large"))) + (is (= (r/map->Match + {:template "/api/ipa/:size" + :data {:name ::beer} + :path "/api/ipa/large" + :params {:size "large"}}) + (r/match-by-name router ::beer {:size "large"}))) + (is (= nil (r/match-by-name router "ILLEGAL"))) + (is (= [::beer] (r/route-names router))) + + (testing "name-based routing with missing parameters" + (is (= (r/map->PartialMatch + {:template "/api/ipa/:size" + :data {:name ::beer} + :required #{:size} + :params nil}) + (r/match-by-name router ::beer))) + (is (= true (r/partial-match? (r/match-by-name router ::beer)))) + (is (thrown-with-msg? + ExceptionInfo + #"^missing path-params for route /api/ipa/:size -> \#\{:size\}$" + (r/match-by-name! router ::beer)))))) + + (testing "complex" + (let [router (r/router + [["/:abba" ::abba] + ["/abba/1" ::abba1] + ["/:abba/:dabba/doo" ::doo] + ["/abba/:dabba/boo" ::boo]] {:router r}) + matches #(-> router (r/match-by-path %) :data :name)] + (is (= ::abba (matches "/abba"))) + (is (= ::abba1 (matches "/abba/1"))) + (is (= ::doo (matches "/abba/1/doo"))) + (is (= ::boo (matches "/abba/1/boo")))))) r/linear-router :linear-router r/prefix-tree-router :prefix-tree-router + r/segment-router :segment-router r/mixed-router :mixed-router)) (testing "routers handling static paths" @@ -80,6 +96,7 @@ r/single-static-path-router :single-static-path-router r/linear-router :linear-router r/prefix-tree-router :prefix-tree-router + r/segment-router :segment-router r/mixed-router :mixed-router)) (testing "route coercion & compilation" From 4490fc1685f49b3c6cf35da0985d4b552c89670e Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Fri, 24 Nov 2017 09:42:53 +0200 Subject: [PATCH 06/17] Test the segment-router. prefix-tree fails on complex tests... --- modules/reitit-core/src/reitit/core.cljc | 6 +++--- modules/reitit-core/src/reitit/impl.cljc | 5 +++-- modules/reitit-core/src/reitit/segment.cljc | 4 ++-- test/cljc/reitit/core_test.cljc | 21 ++++++++++++++------- test/cljc/reitit/impl_test.cljc | 9 +++++++++ test/cljs/reitit/doo_runner.cljs | 2 ++ 6 files changed, 33 insertions(+), 14 deletions(-) create mode 100644 test/cljc/reitit/impl_test.cljc diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 0dc5d6a7..43d7656f 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -339,7 +339,7 @@ (defn mixed-router "Creates two routers: [[lookup-router]] or [[single-static-path-router]] for - static routes and [[prefix-tree-router]] for wildcard routes. All + static routes and [[segment-router]] for wildcard routes. All routes should be non-conflicting. Takes resolved routes and optional expanded options. See [[router]] for options." ([routes] @@ -348,7 +348,7 @@ (let [{wild true, lookup false} (group-by impl/wild-route? routes) compiled (compile-routes routes opts) ->static-router (if (= 1 (count lookup)) single-static-path-router lookup-router) - wildcard-router (prefix-tree-router wild opts) + wildcard-router (segment-router wild opts) static-router (->static-router lookup opts) names (find-names routes opts)] ^{:type ::router} @@ -399,7 +399,7 @@ (and (= 1 (count routes)) (not wilds?)) single-static-path-router conflicting linear-router (not wilds?) lookup-router - all-wilds? prefix-tree-router + all-wilds? segment-router :else mixed-router)] (when-let [conflicts (:conflicts opts)] diff --git a/modules/reitit-core/src/reitit/impl.cljc b/modules/reitit-core/src/reitit/impl.cljc index 7bbae117..4a962b48 100644 --- a/modules/reitit-core/src/reitit/impl.cljc +++ b/modules/reitit-core/src/reitit/impl.cljc @@ -34,8 +34,9 @@ (defn wild-or-catch-all-param? [x] (boolean (or (wild-param x) (catch-all-param x)))) -(defn segments [^String path] - (into [] (.split path "/" 666))) +(defn segments [path] + #?(:clj (.split ^String path "/" 666) + :cljs (.split path #"/" 666))) (defn contains-wilds? [path] (boolean (some wild-or-catch-all-param? (segments path)))) diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc index dc84b06c..2e9316d2 100644 --- a/modules/reitit-core/src/reitit/segment.cljc +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -48,8 +48,8 @@ (insert segment p data)) nil paths)) -(defn lookup [segment ^String path] - (-lookup segment (.split path "/") {})) +(defn lookup [segment path] + (-lookup segment (impl/segments path) {})) (comment (-> [["/:abba" 1] diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 9f684faa..ef518ffa 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -45,20 +45,27 @@ #"^missing path-params for route /api/ipa/:size -> \#\{:size\}$" (r/match-by-name! router ::beer)))))) + ;; TODO (testing "complex" (let [router (r/router - [["/:abba" ::abba] - ["/abba/1" ::abba1] - ["/:abba/:dabba/doo" ::doo] - ["/abba/:dabba/boo" ::boo]] {:router r}) + [["/:abba" ::abba] + ["/abba/1" ::abba2] + ["/:jabba/2" ::jabba2] + ["/:abba/:dabba/doo" ::doo] + ["/abba/:dabba/boo" ::boo] + #_["/:jabba/:dabba/:doo/*foo" ::wild]] + {:router r}) matches #(-> router (r/match-by-path %) :data :name)] (is (= ::abba (matches "/abba"))) - (is (= ::abba1 (matches "/abba/1"))) + (is (= ::abba2 (matches "/abba/1"))) + (is (= ::jabba2 (matches "/abba/2"))) (is (= ::doo (matches "/abba/1/doo"))) - (is (= ::boo (matches "/abba/1/boo")))))) + (is (= ::boo (matches "/abba/1/boo"))) + #_(is (= ::wild (matches "/olipa/kerran/avaruus/vaan/ei/toista/kertaa"))) + ))) r/linear-router :linear-router - r/prefix-tree-router :prefix-tree-router + #_#_r/prefix-tree-router :prefix-tree-router r/segment-router :segment-router r/mixed-router :mixed-router)) diff --git a/test/cljc/reitit/impl_test.cljc b/test/cljc/reitit/impl_test.cljc new file mode 100644 index 00000000..dced731a --- /dev/null +++ b/test/cljc/reitit/impl_test.cljc @@ -0,0 +1,9 @@ +(ns reitit.impl-test + (:require [clojure.test :refer [deftest testing is are]] + [reitit.impl :as impl])) + +(deftest segments-test + (is (= ["" "api" "ipa" "beer" "craft" "bisse"] + (into [] (impl/segments "/api/ipa/beer/craft/bisse")))) + (is (= ["" "a" "" "b" "" "c" ""] + (into [] (impl/segments "/a//b//c/"))))) diff --git a/test/cljs/reitit/doo_runner.cljs b/test/cljs/reitit/doo_runner.cljs index aaa2fc1b..57b7dc2b 100644 --- a/test/cljs/reitit/doo_runner.cljs +++ b/test/cljs/reitit/doo_runner.cljs @@ -2,6 +2,7 @@ (:require [doo.runner :refer-macros [doo-tests]] reitit.coercion-test reitit.core-test + reitit.impl-test reitit.middleware-test reitit.ring-test #_reitit.spec-test)) @@ -10,6 +11,7 @@ (doo-tests 'reitit.coercion-test 'reitit.core-test + 'reitit.impl-test 'reitit.middleware-test 'reitit.ring-test #_'reitit.spec-test) From e0786b73f0ee635365d136f58511350710f5f97b Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Fri, 24 Nov 2017 09:45:52 +0200 Subject: [PATCH 07/17] Remove prefix-tree router as it's broken --- modules/reitit-core/src/reitit/core.cljc | 42 --- modules/reitit-core/src/reitit/trie.cljc | 241 ------------------ .../clj/reitit/prefix_tree_perf_test.clj | 7 +- test/cljc/reitit/core_test.cljc | 2 - 4 files changed, 3 insertions(+), 289 deletions(-) delete mode 100644 modules/reitit-core/src/reitit/trie.cljc diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 43d7656f..656f7156 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -1,7 +1,6 @@ (ns reitit.core (:require [meta-merge.core :refer [meta-merge]] [clojure.string :as str] - [reitit.trie :as trie] [reitit.segment :as segment] [reitit.impl :as impl #?@(:cljs [:refer [Route]])]) #?(:clj @@ -220,47 +219,6 @@ (if-let [match (impl/fast-get lookup name)] (match params))))))) -(defn prefix-tree-router - "Creates a prefix-tree router from resolved routes and optional - expanded options. See [[router]] for available options" - ([routes] - (prefix-tree-router routes {})) - ([routes opts] - (let [compiled (compile-routes routes opts) - names (find-names routes opts) - [pl nl] (reduce - (fn [[pl nl] [p {:keys [name] :as data} result]] - (let [{:keys [params] :as route} (impl/create [p data result]) - f #(if-let [path (impl/path-for route %)] - (->Match p data result % path) - (->PartialMatch p data result % params))] - [(trie/insert pl p (->Match p data result nil nil)) - (if name (assoc nl name f) nl)])) - [nil {}] compiled) - lookup (impl/fast-map nl)] - ^{:type ::router} - (reify - Router - (router-name [_] - :prefix-tree-router) - (routes [_] - compiled) - (options [_] - opts) - (route-names [_] - names) - (match-by-path [_ path] - (if-let [match (trie/lookup pl path {})] - (-> (:data match) - (assoc :params (:params match)) - (assoc :path path)))) - (match-by-name [_ name] - (if-let [match (impl/fast-get lookup name)] - (match nil))) - (match-by-name [_ name params] - (if-let [match (impl/fast-get lookup name)] - (match params))))))) - (defn segment-router "Creates a special prefix-tree style segment router from resolved routes and optional expanded options. See [[router]] for available options" diff --git a/modules/reitit-core/src/reitit/trie.cljc b/modules/reitit-core/src/reitit/trie.cljc deleted file mode 100644 index 6f2208d3..00000000 --- a/modules/reitit-core/src/reitit/trie.cljc +++ /dev/null @@ -1,241 +0,0 @@ -(ns reitit.trie - (:require [reitit.impl :as impl] - [clojure.string :as str])) - -;; -;; original https://github.com/pedestal/pedestal/blob/master/route/src/io/pedestal/http/route/prefix_tree.clj -;; - -(declare insert) - -(defn- char-key [s i] - (if (< i (count s)) - (subs s i (inc i)))) - -(defn- maybe-wild-node [children] - (get children ":")) - -(defn- maybe-catch-all-node [children] - (get children "*")) - -(defprotocol Node - (lookup [this path params]) - (get-segment [this]) - (update-segment [this subs lcs]) - (get-data [this]) - (set-data [this data]) - (get-chidren [this]) - (add-child [this key child]) - (insert-child [this key path-spec data])) - -(extend-protocol Node - nil - (lookup [_ _ _]) - (get-segment [_])) - -(defrecord Match [data params]) - -(defn- wild-node [segment param children data] - (let [?wild (maybe-wild-node children) - ?catch (maybe-catch-all-node children) - children' (impl/fast-map children)] - ^{:type ::node} - (reify - Node - (lookup [_ path params] - (let [i (.indexOf ^String path "/")] - (if (pos? i) - (let [value (subs path 0 i)] - (let [child (impl/fast-get children' (char-key path (inc i))) - path' (subs path (inc i)) - params (assoc params param value)] - (or (lookup child path' params) - (lookup ?wild path' params) - (lookup ?catch path' params)))) - (->Match data (assoc params param path))))) - (get-segment [_] - segment) - (get-data [_] - data) - (set-data [_ data] - (wild-node segment param children data)) - (get-chidren [_] - children) - (add-child [_ key child] - (wild-node segment param (assoc children key child) data)) - (insert-child [_ key path-spec child-data] - (wild-node segment param (update children key insert path-spec child-data) data))))) - -(defn- catch-all-node [segment children param data] - ^{:type ::node} - (reify - Node - (lookup [_ path params] - (->Match data (assoc params param path))) - (get-segment [_] - segment) - (get-data [_] - data) - (get-chidren [_] - children))) - -(defn- static-node [^String segment children data] - (let [size (count segment) - ?wild (maybe-wild-node children) - ?catch (maybe-catch-all-node children) - children' (impl/fast-map children)] - ^{:type ::node} - (reify - Node - (lookup [_ path params] - (if (#?(:clj .equals, :cljs =) segment path) - (->Match data params) - (let [p (if (>= (count path) size) (subs path 0 size))] - (if (#?(:clj .equals, :cljs =) segment p) - (let [child (impl/fast-get children' (char-key path size)) - path (subs path size)] - (or (lookup child path params) - (lookup ?wild path params) - (lookup ?catch path params))))))) - (get-segment [_] - segment) - (update-segment [_ subs lcs] - (static-node (subs segment lcs) children data)) - (get-data [_] - data) - (set-data [_ data] - (static-node segment children data)) - (get-chidren [_] - children) - (add-child [_ key child] - (static-node segment (assoc children key child) data)) - (insert-child [_ key path-spec child-data] - (static-node segment (update children key insert path-spec child-data) data))))) - -(defn- make-node - "Given a path-spec segment string and a payload object, return a new - tree node." - [segment data] - (cond - (impl/wild-param segment) - (wild-node segment (keyword (subs segment 1)) nil data) - - (impl/catch-all-param segment) - (catch-all-node segment (keyword (subs segment 1)) nil data) - - :else - (static-node segment nil data))) - -(defn partition-wilds - "Given a path-spec string, return a seq of strings with wildcards - and catch-alls separated into their own strings. Eats the forward - slash following a wildcard." - [path-spec] - (let [groups (partition-by impl/wild? (str/split path-spec #"/")) - first-groups (butlast groups) - last-group (last groups)] - (flatten - (conj (mapv #(if (impl/wild? (first %)) - % - (str (str/join "/" %) "/")) - first-groups) - (if (impl/wild? (first last-group)) - last-group - (str/join "/" last-group)))))) - -(defn- new-node - "Given a path-spec and a payload object, return a new tree node. If - the path-spec contains wildcards or catch-alls, will return parent - node of a tree (linked list)." - [path-spec data] - (if (impl/contains-wilds? path-spec) - (let [parts (partition-wilds path-spec)] - (reduce (fn [child segment] - (when (impl/catch-all-param segment) - (throw (ex-info "catch-all may only appear at the end of a path spec" - {:patch-spec path-spec}))) - (-> (make-node segment nil) - (add-child (subs (get-segment child) 0 1) child))) - (let [segment (last parts)] - (make-node segment data)) - (reverse (butlast parts)))) - (make-node path-spec data))) - -(defn- calc-lcs - "Given two strings, return the end index of the longest common - prefix string." - [s1 s2] - (loop [i 1] - (cond (or (< (count s1) i) - (< (count s2) i)) - (dec i) - - (= (subs s1 0 i) - (subs s2 0 i)) - (recur (inc i)) - - :else (dec i)))) - -(defn- split - "Given a node, a path-spec, a payload object to insert into the tree - and the lcs, split the node and return a new parent node with the - old contents of node and the new item as children. - lcs is the index of the longest common string in path-spec and the - segment of node." - [node path-spec data lcs] - (let [segment (get-segment node) - common (subs path-spec 0 lcs) - parent (new-node common nil)] - (if (= common path-spec) - (-> (set-data parent data) - (add-child (char-key segment lcs) (update-segment node subs lcs))) - (-> parent - (add-child (char-key segment lcs) (update-segment node subs lcs)) - (insert-child (char-key path-spec lcs) (subs path-spec lcs) data))))) - -(defn insert - "Given a tree node, a path-spec and a payload object, return a new - tree with payload inserted." - [node path-spec data] - (let [segment (get-segment node)] - (cond (nil? node) - (new-node path-spec data) - - (= segment path-spec) - (set-data node data) - - ;; handle case where path-spec is a wildcard param - (impl/wild-param path-spec) - (let [lcs (calc-lcs segment path-spec) - common (subs path-spec 0 lcs)] - (if (= common segment) - (let [path-spec (subs path-spec (inc lcs))] - (insert-child node (subs path-spec 0 1) path-spec data)) - (throw (ex-info "route conflict" - {:node node - :path-spec path-spec - :segment segment})))) - - ;; in the case where path-spec is a catch-all, node should always be nil. - ;; getting here means we have an invalid route specification - (impl/catch-all-param path-spec) - (throw (ex-info "route conflict" - {:node node - :path-spec path-spec - :segment segment})) - - :else - (let [lcs (calc-lcs segment path-spec)] - (cond (= lcs (count segment)) - (insert-child node (char-key path-spec lcs) (subs path-spec lcs) data) - - :else - (split node path-spec data lcs)))))) - -(defn view - "Returns a view representation of a prefix-tree." - [x] - (vec (concat - [(get-segment x)] - (some->> (get-chidren x) vals seq (map view)) - (some->> (get-data x) vector)))) diff --git a/perf-test/clj/reitit/prefix_tree_perf_test.clj b/perf-test/clj/reitit/prefix_tree_perf_test.clj index 7996201f..1f09421e 100644 --- a/perf-test/clj/reitit/prefix_tree_perf_test.clj +++ b/perf-test/clj/reitit/prefix_tree_perf_test.clj @@ -1,7 +1,6 @@ (ns reitit.prefix-tree-perf-test (:require [clojure.test :refer :all] [io.pedestal.http.route.prefix-tree :as p] - [reitit.trie :as trie] [reitit.segment :as segment] [criterium.core :as cc])) @@ -70,7 +69,7 @@ (p/insert acc p d)) nil routes)) -(def reitit-tree +#_(def reitit-tree (reduce (fn [acc [p d]] (trie/insert acc p d)) @@ -101,7 +100,7 @@ ;; 0.8ms (flattened matching) ;; 0.8ms (return route-data) ;; 0.8ms (fix payloads) - (cc/quick-bench + #_(cc/quick-bench (dotimes [_ 1000] (trie/lookup reitit-tree "/v1/orgs/1/topics" {}))) @@ -120,5 +119,5 @@ (comment (p/lookup pedestal-tree "/v1/orgs/1/topics") - (trie/lookup reitit-tree "/v1/orgs/1/topics" {}) + #_(trie/lookup reitit-tree "/v1/orgs/1/topics" {}) (segment/lookup reitit-segment "/v1/orgs/1/topics")) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index ef518ffa..0062cccb 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -65,7 +65,6 @@ ))) r/linear-router :linear-router - #_#_r/prefix-tree-router :prefix-tree-router r/segment-router :segment-router r/mixed-router :mixed-router)) @@ -102,7 +101,6 @@ r/lookup-router :lookup-router r/single-static-path-router :single-static-path-router r/linear-router :linear-router - r/prefix-tree-router :prefix-tree-router r/segment-router :segment-router r/mixed-router :mixed-router)) From 85c4a0a8c0a63970b20bef0308e01f8c0670a6b5 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Fri, 24 Nov 2017 18:58:17 +0200 Subject: [PATCH 08/17] don't test compojure-api, it's compojure all over --- perf-test/clj/reitit/bide_perf_test.clj | 37 ++- perf-test/clj/reitit/lupapiste_perf_test.clj | 3 +- .../clj/reitit/opensensors_perf_test.clj | 241 ++++++------------ project.clj | 2 +- 4 files changed, 98 insertions(+), 185 deletions(-) diff --git a/perf-test/clj/reitit/bide_perf_test.clj b/perf-test/clj/reitit/bide_perf_test.clj index 3a0f4ff1..98ab54fc 100644 --- a/perf-test/clj/reitit/bide_perf_test.clj +++ b/perf-test/clj/reitit/bide_perf_test.clj @@ -4,9 +4,8 @@ [reitit.perf-utils :refer :all] [bidi.bidi :as bidi] - [compojure.api.sweet :refer [api routes GET]] - [compojure.api.routes :as routes] [ataraxy.core :as ataraxy] + [compojure.core :as compojure] [io.pedestal.http.route.definition.table :as table] [io.pedestal.http.route.map-tree :as map-tree] @@ -34,14 +33,11 @@ [["auth/recovery/token/" :token] :auth/recovery] ["workspace/" [[[:project "/" :page] :workspace/page]]]]]) -(def compojure-api-routes - (routes - (GET "/auth/login" [] {:name :auth/login} (constantly "")) - (GET "/auth/recovery/token/:token" [] {:name :auth/recovery} (constantly "")) - (GET "/workspace/:project/:page" [] {:name :workspace/page} (constantly "")))) - -(def compojure-api-request - {:compojure.api.request/lookup (routes/route-lookup-table (routes/get-routes (api compojure-api-routes)))}) +(def compojure-routes + (compojure/routes + (compojure/GET "/auth/login" [] (constantly "")) + (compojure/GET "/auth/recovery/token/:token" [] (constantly "")) + (compojure/GET "/workspace/:project/:page" [] (constantly "")))) (def ataraxy-routes (ataraxy/compile @@ -94,13 +90,13 @@ (dotimes [_ 1000] (pedestal/find-route pedestal-router {:path-info "/auth/login" :request-method :get})))) - ;; 1500 µs - (title "compojure-api") + ;; 1400 µs + (title "compojure") (let [request {:uri "/auth/login", :request-method :get}] - (assert (compojure-api-routes request)) + (assert (compojure-routes request)) (cc/quick-bench (dotimes [_ 1000] - (compojure-api-routes request)))) + (compojure-routes request)))) ;; 3.2 µs (300-500x) (title "reitit") @@ -136,16 +132,17 @@ (dotimes [_ 1000] (pedestal/find-route pedestal-router request)))) - ;; 3500 µs - (title "compojure-api") + ;; 3400 µs + (title "compojure") (let [request {:uri "/workspace/1/1", :request-method :get}] - (assert (compojure-api-routes request)) + (assert (compojure-routes request)) (cc/quick-bench (dotimes [_ 1000] - (compojure-api-routes request)))) + (compojure-routes request)))) ;; 710 µs (3-18x) ;; 530 µs (4-24x) -25% prefix-tree-router + ;; 710 µs (3-18x) segment-router (title "reitit") (assert (reitit/match-by-path reitit-routes "/workspace/1/1")) (cc/quick-bench @@ -174,10 +171,6 @@ ;; 4.9µs (title "compojure-api") - (let [call #(routes/path-for* :workspace/page compojure-api-request {:project "1", :page "1"})] - (assert (= "/workspace/1/1" (call))) - (cc/quick-bench - (call))) ;; 850ns (-83%) (title "reitit") diff --git a/perf-test/clj/reitit/lupapiste_perf_test.clj b/perf-test/clj/reitit/lupapiste_perf_test.clj index 77188ad9..dff097ce 100644 --- a/perf-test/clj/reitit/lupapiste_perf_test.clj +++ b/perf-test/clj/reitit/lupapiste_perf_test.clj @@ -11,7 +11,6 @@ [ataraxy.core :as ataraxy] - [compojure.api.sweet :refer [api routes context ANY]] [compojure.core :as compojure] [io.pedestal.http.route.definition.table :as table] @@ -342,7 +341,7 @@ (def cqrs-routes-compojure (apply routes (map (fn [command] (compojure/ANY (str "/command/" (name command)) [] handler)) commands))) -;; Method code too large +;; Method code too large! #_(def cqrs-routes-ataraxy (ataraxy/compile (into {} (mapv (fn [command] [(str "/command/" (name command)) [command]]) commands)))) diff --git a/perf-test/clj/reitit/opensensors_perf_test.clj b/perf-test/clj/reitit/opensensors_perf_test.clj index 95fcab88..05f0c268 100644 --- a/perf-test/clj/reitit/opensensors_perf_test.clj +++ b/perf-test/clj/reitit/opensensors_perf_test.clj @@ -11,8 +11,7 @@ [ataraxy.core :as ataraxy] - [compojure.api.sweet :refer [api routes context ANY]] - [compojure.core :as compojure] + [compojure.core :refer [routes context ANY]] [io.pedestal.http.route.definition.table :as table] [io.pedestal.http.route.map-tree :as map-tree] @@ -230,157 +229,86 @@ ["topics/" topic] [:test/route47 topic] "topics" [:test/route50]}})) -(def opensensors-compojure-routes - (compojure/routes - (compojure/context "/v1" [] - (compojure/context "/public" [] - (compojure/ANY "/topics/:topic" [] {:name :test/route4} handler) - (compojure/ANY "/users/:user-id" [] {:name :test/route16} handler) - (compojure/ANY "/orgs/:org-id" [] {:name :test/route18} handler)) - (compojure/context "/users/:user-id" [] - (compojure/ANY "/orgs/:org-id" [] {:name :test/route5} handler) - (compojure/ANY "/invitations" [] {:name :test/route7} handler) - (compojure/ANY "/topics" [] {:name :test/route9} handler) - (compojure/ANY "/bookmarks/followers" [] {:name :test/route10} handler) - (compojure/context "/devices" [] - (compojure/ANY "/" [] {:name :test/route15} handler) - #_(compojure/ANY "/bulk" [] {:name :test/route21} handler) - (compojure/ANY "/:client-id" [] {:name :test/route35} handler) - (compojure/ANY "/:client-id/reset-password" [] {:name :test/route49} handler)) - (compojure/ANY "/device-errors" [] {:name :test/route22} handler) - (compojure/ANY "/usage-stats" [] {:name :test/route24} handler) - (compojure/ANY "/claim-device/:client-id" [] {:name :test/route26} handler) - (compojure/ANY "/owned-orgs" [] {:name :test/route31} handler) - (compojure/ANY "/bookmark/:topic" [] {:name :test/route33} handler) - (compojure/ANY "/" [] {:name :test/route36} handler) - (compojure/ANY "/orgs" [] {:name :test/route52} handler) - (compojure/ANY "/api-key" [] {:name :test/route43} handler) - (compojure/ANY "/bookmarks" [] {:name :test/route56} handler)) - (compojure/ANY "/search/topics/:term" [] {:name :test/route6} handler) - (compojure/context "/orgs" [] - (compojure/ANY "/" [] {:name :test/route55} handler) - (compojure/context "/:org-id" [] - (compojure/context "/devices" [] - (compojure/ANY "/" [] {:name :test/route37} handler) - (compojure/ANY "/:device-id" [] {:name :test/route13} handler) - #_(compojure/ANY "/:batch/:type" [] {:name :test/route8} handler)) - (compojure/ANY "/usage-stats" [] {:name :test/route12} handler) - (compojure/ANY "/invitations" [] {:name :test/route19} handler) - (compojure/context "/members" [] - (compojure/ANY "/:user-id" [] {:name :test/route34} handler) - (compojure/ANY "/" [] {:name :test/route38} handler) - #_(compojure/ANY "/invitation-data/:user-id" [] {:name :test/route39} handler)) - (compojure/ANY "/errors" [] {:name :test/route17} handler) - (compojure/ANY "/" [] {:name :test/route42} handler) - (compojure/ANY "/confirm-membership/:token" [] {:name :test/route46} handler) - (compojure/ANY "/topics" [] {:name :test/route57} handler))) - (compojure/context "/messages" [] - (compojure/ANY "/user/:user-id" [] {:name :test/route14} handler) - (compojure/ANY "/device/:client-id" [] {:name :test/route30} handler) - (compojure/ANY "/topic/:topic" [] {:name :test/route48} handler)) - (compojure/context "/topics" [] - (compojure/ANY "/:topic" [] {:name :test/route32} handler) - (compojure/ANY "/" [] {:name :test/route54} handler)) - (compojure/ANY "/whoami" [] {:name :test/route41} handler) - (compojure/ANY "/login" [] {:name :test/route51} handler)) - (compojure/context "/v2" [] - (compojure/ANY "/whoami" [] {:name :test/route1} handler) - (compojure/context "/users/:user-id" [] - (compojure/ANY "/datasets" [] {:name :test/route2} handler) - (compojure/ANY "/devices" [] {:name :test/route25} handler) - (compojure/context "/topics" [] - (compojure/ANY "/bulk" [] {:name :test/route29} handler) - (compojure/ANY "/" [] {:name :test/route54} handler)) - (compojure/ANY "/" [] {:name :test/route45} handler)) - (compojure/context "/public" [] - (compojure/context "/projects/:project-id" [] - (compojure/ANY "/datasets" [] {:name :test/route3} handler) - (compojure/ANY "/" [] {:name :test/route27} handler)) - #_(compojure/ANY "/messages/dataset/bulk" [] {:name :test/route20} handler) - (compojure/ANY "/datasets/:dataset-id" [] {:name :test/route28} handler) - (compojure/ANY "/messages/dataset/:dataset-id" [] {:name :test/route53} handler)) - (compojure/ANY "/datasets/:dataset-id" [] {:name :test/route11} handler) - (compojure/ANY "/login" [] {:name :test/route23} handler) - (compojure/ANY "/orgs/:org-id/topics" [] {:name :test/route40} handler) - (compojure/ANY "/schemas" [] {:name :test/route44} handler) - (compojure/ANY "/topics/:topic" [] {:name :test/route47} handler) - (compojure/ANY "/topics" [] {:name :test/route50} handler)))) +(comment + (declare routes) + (declare context) + (declare ANY)) -(def opensensors-compojure-api-routes +(def opensensors-compojure-routes (routes (context "/v1" [] - (context "/public" [] - (ANY "/topics/:topic" [] {:name :test/route4} handler) - (ANY "/users/:user-id" [] {:name :test/route16} handler) - (ANY "/orgs/:org-id" [] {:name :test/route18} handler)) - (context "/users/:user-id" [] - (ANY "/orgs/:org-id" [] {:name :test/route5} handler) - (ANY "/invitations" [] {:name :test/route7} handler) - (ANY "/topics" [] {:name :test/route9} handler) - (ANY "/bookmarks/followers" [] {:name :test/route10} handler) - (context "/devices" [] - (ANY "/" [] {:name :test/route15} handler) - #_(ANY "/bulk" [] {:name :test/route21} handler) - (ANY "/:client-id" [] {:name :test/route35} handler) - (ANY "/:client-id/reset-password" [] {:name :test/route49} handler)) - (ANY "/device-errors" [] {:name :test/route22} handler) - (ANY "/usage-stats" [] {:name :test/route24} handler) - (ANY "/claim-device/:client-id" [] {:name :test/route26} handler) - (ANY "/owned-orgs" [] {:name :test/route31} handler) - (ANY "/bookmark/:topic" [] {:name :test/route33} handler) - (ANY "/" [] {:name :test/route36} handler) - (ANY "/orgs" [] {:name :test/route52} handler) - (ANY "/api-key" [] {:name :test/route43} handler) - (ANY "/bookmarks" [] {:name :test/route56} handler)) - (ANY "/search/topics/:term" [] {:name :test/route6} handler) - (context "/orgs" [] - (ANY "/" [] {:name :test/route55} handler) - (context "/:org-id" [] - (context "/devices" [] - (ANY "/" [] {:name :test/route37} handler) - (ANY "/:device-id" [] {:name :test/route13} handler) - #_(ANY "/:batch/:type" [] {:name :test/route8} handler)) - (ANY "/usage-stats" [] {:name :test/route12} handler) - (ANY "/invitations" [] {:name :test/route19} handler) - (context "/members" [] - (ANY "/:user-id" [] {:name :test/route34} handler) - (ANY "/" [] {:name :test/route38} handler) - #_(ANY "/invitation-data/:user-id" [] {:name :test/route39} handler)) - (ANY "/errors" [] {:name :test/route17} handler) - (ANY "/" [] {:name :test/route42} handler) - (ANY "/confirm-membership/:token" [] {:name :test/route46} handler) - (ANY "/topics" [] {:name :test/route57} handler))) - (context "/messages" [] - (ANY "/user/:user-id" [] {:name :test/route14} handler) - (ANY "/device/:client-id" [] {:name :test/route30} handler) - (ANY "/topic/:topic" [] {:name :test/route48} handler)) - (context "/topics" [] - (ANY "/:topic" [] {:name :test/route32} handler) - (ANY "/" [] {:name :test/route54} handler)) - (ANY "/whoami" [] {:name :test/route41} handler) - (ANY "/login" [] {:name :test/route51} handler)) + (context "/public" [] + (ANY "/topics/:topic" [] {:name :test/route4} handler) + (ANY "/users/:user-id" [] {:name :test/route16} handler) + (ANY "/orgs/:org-id" [] {:name :test/route18} handler)) + (context "/users/:user-id" [] + (ANY "/orgs/:org-id" [] {:name :test/route5} handler) + (ANY "/invitations" [] {:name :test/route7} handler) + (ANY "/topics" [] {:name :test/route9} handler) + (ANY "/bookmarks/followers" [] {:name :test/route10} handler) + (context "/devices" [] + (ANY "/" [] {:name :test/route15} handler) + #_(ANY "/bulk" [] {:name :test/route21} handler) + (ANY "/:client-id" [] {:name :test/route35} handler) + (ANY "/:client-id/reset-password" [] {:name :test/route49} handler)) + (ANY "/device-errors" [] {:name :test/route22} handler) + (ANY "/usage-stats" [] {:name :test/route24} handler) + (ANY "/claim-device/:client-id" [] {:name :test/route26} handler) + (ANY "/owned-orgs" [] {:name :test/route31} handler) + (ANY "/bookmark/:topic" [] {:name :test/route33} handler) + (ANY "/" [] {:name :test/route36} handler) + (ANY "/orgs" [] {:name :test/route52} handler) + (ANY "/api-key" [] {:name :test/route43} handler) + (ANY "/bookmarks" [] {:name :test/route56} handler)) + (ANY "/search/topics/:term" [] {:name :test/route6} handler) + (context "/orgs" [] + (ANY "/" [] {:name :test/route55} handler) + (context "/:org-id" [] + (context "/devices" [] + (ANY "/" [] {:name :test/route37} handler) + (ANY "/:device-id" [] {:name :test/route13} handler) + #_(ANY "/:batch/:type" [] {:name :test/route8} handler)) + (ANY "/usage-stats" [] {:name :test/route12} handler) + (ANY "/invitations" [] {:name :test/route19} handler) + (context "/members" [] + (ANY "/:user-id" [] {:name :test/route34} handler) + (ANY "/" [] {:name :test/route38} handler) + #_(ANY "/invitation-data/:user-id" [] {:name :test/route39} handler)) + (ANY "/errors" [] {:name :test/route17} handler) + (ANY "/" [] {:name :test/route42} handler) + (ANY "/confirm-membership/:token" [] {:name :test/route46} handler) + (ANY "/topics" [] {:name :test/route57} handler))) + (context "/messages" [] + (ANY "/user/:user-id" [] {:name :test/route14} handler) + (ANY "/device/:client-id" [] {:name :test/route30} handler) + (ANY "/topic/:topic" [] {:name :test/route48} handler)) + (context "/topics" [] + (ANY "/:topic" [] {:name :test/route32} handler) + (ANY "/" [] {:name :test/route54} handler)) + (ANY "/whoami" [] {:name :test/route41} handler) + (ANY "/login" [] {:name :test/route51} handler)) (context "/v2" [] - (ANY "/whoami" [] {:name :test/route1} handler) - (context "/users/:user-id" [] - (ANY "/datasets" [] {:name :test/route2} handler) - (ANY "/devices" [] {:name :test/route25} handler) - (context "/topics" [] - (ANY "/bulk" [] {:name :test/route29} handler) - (ANY "/" [] {:name :test/route54} handler)) - (ANY "/" [] {:name :test/route45} handler)) - (context "/public" [] - (context "/projects/:project-id" [] - (ANY "/datasets" [] {:name :test/route3} handler) - (ANY "/" [] {:name :test/route27} handler)) - #_(ANY "/messages/dataset/bulk" [] {:name :test/route20} handler) - (ANY "/datasets/:dataset-id" [] {:name :test/route28} handler) - (ANY "/messages/dataset/:dataset-id" [] {:name :test/route53} handler)) - (ANY "/datasets/:dataset-id" [] {:name :test/route11} handler) - (ANY "/login" [] {:name :test/route23} handler) - (ANY "/orgs/:org-id/topics" [] {:name :test/route40} handler) - (ANY "/schemas" [] {:name :test/route44} handler) - (ANY "/topics/:topic" [] {:name :test/route47} handler) - (ANY "/topics" [] {:name :test/route50} handler)))) + (ANY "/whoami" [] {:name :test/route1} handler) + (context "/users/:user-id" [] + (ANY "/datasets" [] {:name :test/route2} handler) + (ANY "/devices" [] {:name :test/route25} handler) + (context "/topics" [] + (ANY "/bulk" [] {:name :test/route29} handler) + (ANY "/" [] {:name :test/route54} handler)) + (ANY "/" [] {:name :test/route45} handler)) + (context "/public" [] + (context "/projects/:project-id" [] + (ANY "/datasets" [] {:name :test/route3} handler) + (ANY "/" [] {:name :test/route27} handler)) + #_(ANY "/messages/dataset/bulk" [] {:name :test/route20} handler) + (ANY "/datasets/:dataset-id" [] {:name :test/route28} handler) + (ANY "/messages/dataset/:dataset-id" [] {:name :test/route53} handler)) + (ANY "/datasets/:dataset-id" [] {:name :test/route11} handler) + (ANY "/login" [] {:name :test/route23} handler) + (ANY "/orgs/:org-id/topics" [] {:name :test/route40} handler) + (ANY "/schemas" [] {:name :test/route44} handler) + (ANY "/topics/:topic" [] {:name :test/route47} handler) + (ANY "/topics" [] {:name :test/route50} handler)))) (def opensensors-pedestal-routes (map-tree/router @@ -489,11 +417,6 @@ (if-not match (println route)))) - (doseq [route (valid-urls (reitit/router opensensors-routes))] - (let [match (opensensors-compojure-api-routes {:uri route :request-method :get})] - (if-not match - (println route)))) - (doseq [route (valid-urls (reitit/router opensensors-routes))] (let [match (pedestal/find-route opensensors-pedestal-routes {:path-info route :request-method :get})] (if-not match @@ -507,27 +430,25 @@ bidi-f #(bidi/match-route opensensors-bidi-routes (:uri %)) ataraxy-f (partial ataraxy/matches opensensors-ataraxy-routes) compojure-f opensensors-compojure-routes - compojure-api-f opensensors-compojure-api-routes pedestal-f (partial pedestal/find-route opensensors-pedestal-routes) b! (partial bench!! routes (fn [path] {:request-method :get, :uri path, :path-info path}) true)] ;; 2538ns ;; 2065ns ;; 662ns (prefix-tree-router) + ;; 567ns (segment-router) (b! "reitit" reitit-f) ;; 2845ns ;; 2316ns ;; 819ns (prefix-tree-router) + ;; 723ns (segment-router) (b! "reitit-ring" reitit-ring-f) - ;; 2540ns + ;; 2821ns (b! "pedestal" pedestal-f) - ;; 10300ns - (b! "compojure-api" compojure-api-f) - - ;; 10800ns + ;; 11615ns (b! "compojure" compojure-f) ;; 15034ns diff --git a/project.clj b/project.clj index 0155895a..e7872068 100644 --- a/project.clj +++ b/project.clj @@ -49,7 +49,7 @@ "-Xmx4096m" "-Dclojure.compiler.direct-linking=true"] :test-paths ["perf-test/clj"] - :dependencies [[metosin/compojure-api "2.0.0-alpha12"] + :dependencies [[compojure "1.6.0"] [io.pedestal/pedestal.route "0.5.3"] [org.clojure/core.async "0.3.443"] [ataraxy "0.4.0"] From 293274fb6863281e34bd5afd48b281eec037bf17 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Fri, 24 Nov 2017 18:58:48 +0200 Subject: [PATCH 09/17] implement catch-all params for segment-router --- modules/reitit-core/src/reitit/segment.cljc | 9 ++++++--- test/cljc/reitit/core_test.cljc | 6 ++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc index 2e9316d2..7a3aa86a 100644 --- a/modules/reitit-core/src/reitit/segment.cljc +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -13,9 +13,12 @@ (-insert [this ps data]) (-lookup [this ps params])) -(defn- -catch-all [catch-all data params p ps] +(defn- -catch-all [children catch-all data params p ps] (if catch-all - (assoc data :params (assoc params catch-all (str/join "/" (cons p ps)))))) + (-lookup + (impl/fast-get children catch-all) + nil + (assoc data :params (assoc params catch-all (str/join "/" (cons p ps))))))) (defn- segment ([] (segment {} #{} nil nil)) @@ -37,7 +40,7 @@ (if data (assoc data :params params)) (or (-lookup (impl/fast-get children' p) ps params) (some #(-lookup (impl/fast-get children' %) ps (assoc params % p)) wilds) - (-catch-all catch-all data params p ps)))))))) + (-catch-all children' catch-all data params p ps)))))))) (defn insert [root path data] (-insert (or root (segment)) (impl/segments path) (map->Match {:data data}))) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 0062cccb..cbc1e266 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -45,7 +45,6 @@ #"^missing path-params for route /api/ipa/:size -> \#\{:size\}$" (r/match-by-name! router ::beer)))))) - ;; TODO (testing "complex" (let [router (r/router [["/:abba" ::abba] @@ -53,7 +52,7 @@ ["/:jabba/2" ::jabba2] ["/:abba/:dabba/doo" ::doo] ["/abba/:dabba/boo" ::boo] - #_["/:jabba/:dabba/:doo/*foo" ::wild]] + ["/:jabba/:dabba/:doo/*foo" ::wild]] {:router r}) matches #(-> router (r/match-by-path %) :data :name)] (is (= ::abba (matches "/abba"))) @@ -61,8 +60,7 @@ (is (= ::jabba2 (matches "/abba/2"))) (is (= ::doo (matches "/abba/1/doo"))) (is (= ::boo (matches "/abba/1/boo"))) - #_(is (= ::wild (matches "/olipa/kerran/avaruus/vaan/ei/toista/kertaa"))) - ))) + (is (= ::wild (matches "/olipa/kerran/avaruus/vaan/ei/toista/kertaa")))))) r/linear-router :linear-router r/segment-router :segment-router From 861c16b19589e2540282911127bc1c5287119574 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Fri, 24 Nov 2017 19:09:45 +0200 Subject: [PATCH 10/17] initial commit --- modules/reitit-schema/project.clj | 10 ++ .../src/reitit/ring/coercion/schema.cljc | 112 ++++++++++++++++++ project.clj | 7 +- scripts/lein-modules | 2 +- 4 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 modules/reitit-schema/project.clj create mode 100644 modules/reitit-schema/src/reitit/ring/coercion/schema.cljc diff --git a/modules/reitit-schema/project.clj b/modules/reitit-schema/project.clj new file mode 100644 index 00000000..e405cfdb --- /dev/null +++ b/modules/reitit-schema/project.clj @@ -0,0 +1,10 @@ +(defproject metosin/reitit-schema "0.1.0-SNAPSHOT" + :description "Reitit: Plumatic Schema coercion" + :url "https://github.com/metosin/reitit" + :license {:name "Eclipse Public License" + :url "http://www.eclipse.org/legal/epl-v10.html"} + :plugins [[lein-parent "0.3.2"]] + :parent-project {:path "../../project.clj" + :inherit [:deploy-repositories :managed-dependencies]} + :dependencies [[metosin/reitit-ring] + [metosin/schema-tools]]) diff --git a/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc b/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc new file mode 100644 index 00000000..565424bb --- /dev/null +++ b/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc @@ -0,0 +1,112 @@ +(ns reitit.ring.coercion.schema + (:require [clojure.spec.alpha :as s] + [spec-tools.core :as st #?@(:cljs [:refer [Spec]])] + [spec-tools.data-spec :as ds] + [spec-tools.conform :as conform] + [spec-tools.swagger.core :as swagger] + [reitit.ring.coercion.protocol :as protocol]) + #?(:clj + (:import (spec_tools.core Spec)))) + +(def string-conforming + (st/type-conforming + (merge + conform/string-type-conforming + conform/strip-extra-keys-type-conforming))) + +(def json-conforming + (st/type-conforming + (merge + conform/json-type-conforming + conform/strip-extra-keys-type-conforming))) + +(def default-conforming + ::default) + +(defprotocol IntoSpec + (into-spec [this name])) + +(extend-protocol IntoSpec + + #?(:clj clojure.lang.PersistentArrayMap + :cljs cljs.core.PersistentArrayMap) + (into-spec [this name] + (ds/spec name this)) + + #?(:clj clojure.lang.PersistentHashMap + :cljs cljs.core.PersistentHashMap) + (into-spec [this name] + (ds/spec name this)) + + Spec + (into-spec [this _] this) + + #?(:clj Object + :cljs default) + (into-spec [this _] + (st/create-spec {:spec this}))) + +;; TODO: proper name! +(def memoized-into-spec + (memoize #(into-spec %1 (gensym "spec")))) + +(defmulti coerce-response? identity :default ::default) +(defmethod coerce-response? ::default [_] true) + +(defrecord SpecCoercion [name conforming coerce-response?] + + protocol/Coercion + (get-name [_] name) + + (compile [_ model _] + (memoized-into-spec model)) + + (get-apidocs [_ _ {:keys [parameters responses] :as info}] + (cond-> (dissoc info :parameters :responses) + parameters (assoc + ::swagger/parameters + (into + (empty parameters) + (for [[k v] parameters] + [k memoized-into-spec]))) + responses (assoc + ::swagger/responses + (into + (empty responses) + (for [[k response] responses] + [k (update response :schema memoized-into-spec)]))))) + + (make-open [_ spec] spec) + + (encode-error [_ error] + (update error :spec (comp str s/form))) + + (request-coercer [_ type spec] + (let [spec (memoized-into-spec spec) + {:keys [formats default]} (conforming type)] + (fn [value format] + (if-let [conforming (or (get formats format) default)] + (let [conformed (st/conform spec value conforming)] + (if (s/invalid? conformed) + (let [problems (st/explain-data spec value conforming)] + (protocol/map->CoercionError + {:spec spec + :problems (::s/problems problems)})) + (s/unform spec conformed))) + value)))) + + (response-coercer [this spec] + (if (coerce-response? spec) + (protocol/request-coercer this :response spec)))) + +(def default-options + {:coerce-response? coerce-response? + :conforming {:body {:default default-conforming + :formats {"application/json" json-conforming}} + :string {:default string-conforming} + :response {:default default-conforming}}}) + +(defn create [{:keys [conforming coerce-response?]}] + (->SpecCoercion :spec conforming coerce-response?)) + +(def coercion (create default-options)) diff --git a/project.clj b/project.clj index e7872068..abb2fbaf 100644 --- a/project.clj +++ b/project.clj @@ -13,9 +13,11 @@ [metosin/reitit-core "0.1.0-SNAPSHOT"] [metosin/reitit-ring "0.1.0-SNAPSHOT"] [metosin/reitit-spec "0.1.0-SNAPSHOT"] + [metosin/reitit-schema "0.1.0-SNAPSHOT"] [meta-merge "1.0.0"] - [metosin/spec-tools "0.5.1"]] + [metosin/spec-tools "0.5.1"] + [metosin/schema-tools "0.9.1"]] :plugins [[jonase/eastwood "0.2.5"] [lein-doo "0.1.8"] @@ -30,7 +32,8 @@ :source-paths ["modules/reitit/src" "modules/reitit-core/src" "modules/reitit-ring/src" - "modules/reitit-spec/src"] + "modules/reitit-spec/src" + "modules/reitit-schema/src"] :dependencies [[org.clojure/clojure "1.9.0-RC1"] [org.clojure/clojurescript "1.9.946"] diff --git a/scripts/lein-modules b/scripts/lein-modules index feab8f2e..8ae825fd 100755 --- a/scripts/lein-modules +++ b/scripts/lein-modules @@ -3,6 +3,6 @@ set -e # Modules -for ext in reitit-core reitit-ring reitit-spec reitit; do +for ext in reitit-core reitit-ring reitit-spec reitit-schema reitit; do cd modules/$ext; lein "$@"; cd ../..; done From 4d772c62e12a5d1e7ce3f3d64f76c7546a73bde6 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Sun, 26 Nov 2017 21:51:21 +0200 Subject: [PATCH 11/17] Polish implementations --- .../reitit-ring/src/reitit/ring/coercion.cljc | 18 ++- .../src/reitit/ring/coercion/schema.cljc | 132 ++++++++---------- .../src/reitit/ring/coercion/spec.cljc | 9 +- modules/reitit/project.clj | 3 +- project.clj | 5 +- 5 files changed, 79 insertions(+), 88 deletions(-) diff --git a/modules/reitit-ring/src/reitit/ring/coercion.cljc b/modules/reitit-ring/src/reitit/ring/coercion.cljc index 1a394558..50f5d4e0 100644 --- a/modules/reitit-ring/src/reitit/ring/coercion.cljc +++ b/modules/reitit-ring/src/reitit/ring/coercion.cljc @@ -15,6 +15,8 @@ (defrecord ParameterCoercion [in style keywordize? open?]) +(def valid-type? #{::request-coercion ::response-coercion}) + (def ring-parameter-coercion {:query (->ParameterCoercion :query-params :string true true) :body (->ParameterCoercion :body-params :string false true) @@ -47,6 +49,8 @@ :request request :response response})))) +;; TODO: support faster key walking, walk/keywordize-keys is quite slow... + (defn request-coercer [coercion type model] (if coercion (let [{:keys [keywordize? open? in style]} (ring-parameter-coercion type) @@ -81,7 +85,7 @@ ;; middleware ;; -(defn- coerce-parameters [coercers request] +(defn- coerce-request [coercers request] (reduce-kv (fn [acc k coercer] (impl/fast-assoc acc k (coercer request))) @@ -131,21 +135,21 @@ (handler (impl/fast-assoc request :parameters coerced) respond raise))))))) (def gen-wrap-coerce-parameters - "Generator for pluggable request coercion middleware. + "Middleware for pluggable request coercion. Expects a :coercion of type `reitit.coercion.protocol/Coercion` and :parameters from route data, otherwise does not mount." (middleware/create {:name ::coerce-parameters - :gen-wrap (fn [{:keys [parameters coercion]} _] + :gen-wrap (fn [{:keys [coercion parameters]} _] (if (and coercion parameters) (let [coercers (request-coercers coercion parameters)] (fn [handler] (fn ([request] - (let [coerced (coerce-parameters coercers request)] + (let [coerced (coerce-request coercers request)] (handler (impl/fast-assoc request :parameters coerced)))) ([request respond raise] - (let [coerced (coerce-parameters coercers request)] + (let [coerced (coerce-request coercers request)] (handler (impl/fast-assoc request :parameters coerced) respond raise))))))))})) (defn wrap-coerce-response @@ -177,12 +181,12 @@ (handler request respond raise)))))) (def gen-wrap-coerce-response - "Generator for pluggable response coercion middleware. + "Middleware for pluggable response coercion. Expects a :coercion of type `reitit.coercion.protocol/Coercion` and :responses from route data, otherwise does not mount." (middleware/create {:name ::coerce-response - :gen-wrap (fn [{:keys [responses coercion opts]} _] + :gen-wrap (fn [{:keys [coercion responses opts]} _] (if (and coercion responses) (let [coercers (response-coercers coercion responses opts)] (fn [handler] diff --git a/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc b/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc index 565424bb..7733d574 100644 --- a/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc +++ b/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc @@ -1,112 +1,90 @@ (ns reitit.ring.coercion.schema - (:require [clojure.spec.alpha :as s] - [spec-tools.core :as st #?@(:cljs [:refer [Spec]])] - [spec-tools.data-spec :as ds] - [spec-tools.conform :as conform] + (:require [schema.core :as s] + [schema-tools.core :as st] + [schema.coerce :as sc] + [schema.utils :as su] + [schema-tools.coerce :as stc] [spec-tools.swagger.core :as swagger] + [clojure.walk :as walk] [reitit.ring.coercion.protocol :as protocol]) - #?(:clj - (:import (spec_tools.core Spec)))) + (:import (schema.core OptionalKey RequiredKey) + (schema.utils ValidationError NamedError))) -(def string-conforming - (st/type-conforming - (merge - conform/string-type-conforming - conform/strip-extra-keys-type-conforming))) +(def string-coercion-matcher + stc/string-coercion-matcher) -(def json-conforming - (st/type-conforming - (merge - conform/json-type-conforming - conform/strip-extra-keys-type-conforming))) +(def json-coercion-matcher + stc/json-coercion-matcher) -(def default-conforming - ::default) - -(defprotocol IntoSpec - (into-spec [this name])) - -(extend-protocol IntoSpec - - #?(:clj clojure.lang.PersistentArrayMap - :cljs cljs.core.PersistentArrayMap) - (into-spec [this name] - (ds/spec name this)) - - #?(:clj clojure.lang.PersistentHashMap - :cljs cljs.core.PersistentHashMap) - (into-spec [this name] - (ds/spec name this)) - - Spec - (into-spec [this _] this) - - #?(:clj Object - :cljs default) - (into-spec [this _] - (st/create-spec {:spec this}))) - -;; TODO: proper name! -(def memoized-into-spec - (memoize #(into-spec %1 (gensym "spec")))) +(def default-coercion-matcher + (constantly nil)) (defmulti coerce-response? identity :default ::default) (defmethod coerce-response? ::default [_] true) -(defrecord SpecCoercion [name conforming coerce-response?] +(defn stringify [schema] + (walk/prewalk + (fn [x] + (cond + (class? x) (.getName ^Class x) + (instance? OptionalKey x) (pr-str (list 'opt (:k x))) + (instance? RequiredKey x) (pr-str (list 'req (:k x))) + (and (satisfies? s/Schema x) (record? x)) (try (pr-str (s/explain x)) (catch Exception _ x)) + (instance? ValidationError x) (str (su/validation-error-explain x)) + (instance? NamedError x) (str (su/named-error-explain x)) + :else x)) + schema)) + +(defrecord SchemaCoercion [name matchers coerce-response?] protocol/Coercion (get-name [_] name) (compile [_ model _] - (memoized-into-spec model)) + model) (get-apidocs [_ _ {:keys [parameters responses] :as info}] (cond-> (dissoc info :parameters :responses) parameters (assoc ::swagger/parameters - (into - (empty parameters) - (for [[k v] parameters] - [k memoized-into-spec]))) + parameters) responses (assoc ::swagger/responses - (into - (empty responses) - (for [[k response] responses] - [k (update response :schema memoized-into-spec)]))))) + responses))) - (make-open [_ spec] spec) + (make-open [_ schema] (st/open-schema schema)) (encode-error [_ error] - (update error :spec (comp str s/form))) + (-> error + (update :schema stringify) + (update :errors stringify))) - (request-coercer [_ type spec] - (let [spec (memoized-into-spec spec) - {:keys [formats default]} (conforming type)] + ;; TODO: create all possible coercers ahead of time + (request-coercer [_ type schema] + (let [{:keys [formats default]} (matchers type)] (fn [value format] - (if-let [conforming (or (get formats format) default)] - (let [conformed (st/conform spec value conforming)] - (if (s/invalid? conformed) - (let [problems (st/explain-data spec value conforming)] - (protocol/map->CoercionError - {:spec spec - :problems (::s/problems problems)})) - (s/unform spec conformed))) + (if-let [matcher (or (get formats format) default)] + (let [coercer (sc/coercer schema matcher) + coerced (coercer value)] + (if-let [error (su/error-val coerced)] + (protocol/map->CoercionError + {:schema schema + :errors error}) + coerced)) value)))) - (response-coercer [this spec] - (if (coerce-response? spec) - (protocol/request-coercer this :response spec)))) + (response-coercer [this schema] + (if (coerce-response? schema) + (protocol/request-coercer this :response schema)))) (def default-options {:coerce-response? coerce-response? - :conforming {:body {:default default-conforming - :formats {"application/json" json-conforming}} - :string {:default string-conforming} - :response {:default default-conforming}}}) + :matchers {:body {:default default-coercion-matcher + :formats {"application/json" json-coercion-matcher}} + :string {:default string-coercion-matcher} + :response {:default default-coercion-matcher}}}) -(defn create [{:keys [conforming coerce-response?]}] - (->SpecCoercion :spec conforming coerce-response?)) +(defn create [{:keys [matchers coerce-response?]}] + (->SchemaCoercion :schema matchers coerce-response?)) (def coercion (create default-options)) diff --git a/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc b/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc index a2afa3c8..edf96ce2 100644 --- a/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc +++ b/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc @@ -50,6 +50,11 @@ (def memoized-into-spec (memoize #(into-spec %1 (gensym "spec")))) +(defn stringify-pred [pred] + (str (if (instance? clojure.lang.LazySeq pred) + (seq pred) + pred))) + (defmulti coerce-response? identity :default ::default) (defmethod coerce-response? ::default [_] true) @@ -79,7 +84,9 @@ (make-open [_ spec] spec) (encode-error [_ error] - (update error :spec (comp str s/form))) + (-> error + (update :spec (comp str s/form)) + (update :problems (partial mapv #(update % :pred stringify-pred))))) (request-coercer [_ type spec] (let [spec (memoized-into-spec spec) diff --git a/modules/reitit/project.clj b/modules/reitit/project.clj index b0f4404b..6ac2562b 100644 --- a/modules/reitit/project.clj +++ b/modules/reitit/project.clj @@ -8,4 +8,5 @@ :inherit [:deploy-repositories :managed-dependencies]} :dependencies [[metosin/reitit-core] [metosin/reitit-ring] - [metosin/reitit-spec]]) + [metosin/reitit-spec] + [metosin/reitit-schema]]) diff --git a/project.clj b/project.clj index abb2fbaf..8b5fcfb7 100644 --- a/project.clj +++ b/project.clj @@ -17,7 +17,7 @@ [meta-merge "1.0.0"] [metosin/spec-tools "0.5.1"] - [metosin/schema-tools "0.9.1"]] + [metosin/schema-tools "0.10.0-SNAPSHOT"]] :plugins [[jonase/eastwood "0.2.5"] [lein-doo "0.1.8"] @@ -38,8 +38,9 @@ :dependencies [[org.clojure/clojure "1.9.0-RC1"] [org.clojure/clojurescript "1.9.946"] - ;; all modules dependencies + ;; modules dependencies [metosin/reitit] + [metosin/schema-tools "0.10.0-SNAPSHOT"] [expound "0.3.2"] [orchestra "2017.08.13"] From 03d4e8c4bf9b0f9f397c44c7775b83b4c4435dec Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Sun, 26 Nov 2017 21:51:43 +0200 Subject: [PATCH 12/17] Implement coercion error handling --- .../reitit-ring/src/reitit/ring/coercion.cljc | 43 +++- test/cljc/reitit/coercion_test.cljc | 188 +++++++++++++----- 2 files changed, 176 insertions(+), 55 deletions(-) diff --git a/modules/reitit-ring/src/reitit/ring/coercion.cljc b/modules/reitit-ring/src/reitit/ring/coercion.cljc index 50f5d4e0..fe21d594 100644 --- a/modules/reitit-ring/src/reitit/ring/coercion.cljc +++ b/modules/reitit-ring/src/reitit/ring/coercion.cljc @@ -81,9 +81,11 @@ (response-coercion-failed! result coercion value request response) result)))))) -;; -;; middleware -;; +(defn encode-error [data] + (-> data + (dissoc :request :response) + (update :coercion protocol/get-name) + (->> (protocol/encode-error (:coercion data))))) (defn- coerce-request [coercers request] (reduce-kv @@ -133,6 +135,20 @@ (let [coercers (request-coercers coercion parameters) coerced (coerce-parameters coercers request)] (handler (impl/fast-assoc request :parameters coerced) respond raise))))))) +(defn handle-coercion-exception [e respond raise] + (let [data (ex-data e)] + (if-let [status (condp = (:type data) + ::request-coercion 400 + ::response-coercion 500 + nil)] + (respond + {:status status + :body (encode-error data)}) + (raise e)))) + +;; +;; middleware +;; (def gen-wrap-coerce-parameters "Middleware for pluggable request coercion. @@ -195,3 +211,24 @@ (coerce-response coercers request (handler request))) ([request respond raise] (handler request #(respond (coerce-response coercers request %)) raise)))))))})) + +(def gen-wrap-coerce-exceptions + "Middleare for coercion exception handling. + Expects a :coercion of type `reitit.coercion.protocol/Coercion` + and :parameters or :responses from route data, otherwise does not mount." + (middleware/create + {:name ::coerce-exceptions + :gen-wrap (fn [{:keys [coercion parameters responses]} _] + (if (and coercion (or parameters responses)) + (fn [handler] + (fn + ([request] + (try + (handler request) + (catch Exception e + (handle-coercion-exception e identity #(throw %))))) + ([request respond raise] + (try + (handler request respond (fn [e] (handle-coercion-exception e respond raise))) + (catch Throwable e + (handle-coercion-exception e respond raise))))))))})) diff --git a/test/cljc/reitit/coercion_test.cljc b/test/cljc/reitit/coercion_test.cljc index ea4dfd19..a9b8a62f 100644 --- a/test/cljc/reitit/coercion_test.cljc +++ b/test/cljc/reitit/coercion_test.cljc @@ -2,62 +2,146 @@ (:require [clojure.test :refer [deftest testing is]] [reitit.ring :as ring] [reitit.ring.coercion :as coercion] - [reitit.ring.coercion.spec :as spec]) + [reitit.ring.coercion.spec :as spec] + [schema.core :as s] + [reitit.ring.coercion.schema :as schema]) #?(:clj (:import (clojure.lang ExceptionInfo)))) -(defn handler - ([{:keys [::mw]}] - {:status 200 :body (conj mw :ok)}) - ([request respond raise] - (respond (handler request)))) +(defn handler [{{{:keys [a]} :query + {:keys [b]} :body + {:keys [c]} :form + {:keys [d]} :header + {:keys [e]} :path} :parameters}] + {:status 200 + :body {:total (+ a b c d e)}}) -(deftest coercion-test - (let [app (ring/ring-handler - (ring/router - ["/api" - ["/plus/:e" - {:get {:parameters {:query {:a int?} - :body {:b int?} - :form {:c int?} - :header {:d int?} - :path {:e int?}} - :responses {200 {:schema {:total pos-int?}}} - :handler (fn [{{{:keys [a]} :query - {:keys [b]} :body - {:keys [c]} :form - {:keys [d]} :header - {:keys [e]} :path} :parameters}] - {:status 200 - :body {:total (+ a b c d e)}})}}]] - {:data {:middleware [coercion/gen-wrap-coerce-parameters - coercion/gen-wrap-coerce-response] - :coercion spec/coercion}}))] +(def valid-request + {:uri "/api/plus/5" + :request-method :get + :query-params {"a" "1"} + :body-params {:b 2} + :form-params {:c 3} + :header-params {:d 4}}) - (testing "all good" - (is (= {:status 200 - :body {:total 15}} - (app {:uri "/api/plus/5" - :request-method :get - :query-params {"a" "1"} - :body-params {:b 2} - :form-params {:c 3} - :header-params {:d 4}})))) +(def invalid-request + {:uri "/api/plus/5" + :request-method :get}) - (testing "invalid request" - (is (thrown-with-msg? - ExceptionInfo - #"Request coercion failed" - (app {:uri "/api/plus/5" - :request-method :get})))) +(def invalid-request2 + {:uri "/api/plus/5" + :request-method :get + :query-params {"a" "1"} + :body-params {:b 2} + :form-params {:c 3} + :header-params {:d -40}}) - (testing "invalid response" - (is (thrown-with-msg? - ExceptionInfo - #"Response coercion failed" - (app {:uri "/api/plus/5" - :request-method :get - :query-params {"a" "1"} - :body-params {:b 2} - :form-params {:c 3} - :header-params {:d -40}})))))) +(deftest spec-coercion-test + (let [create (fn [middleware] + (ring/ring-handler + (ring/router + ["/api" + ["/plus/:e" + {:get {:parameters {:query {:a int?} + :body {:b int?} + :form {:c int?} + :header {:d int?} + :path {:e int?}} + :responses {200 {:schema {:total pos-int?}}} + :handler handler}}]] + {:data {:middleware middleware + :coercion spec/coercion}})))] + + (testing "withut exception handling" + (let [app (create [coercion/gen-wrap-coerce-parameters + coercion/gen-wrap-coerce-response])] + + (testing "all good" + (is (= {:status 200 + :body {:total 15}} + (app valid-request)))) + + (testing "invalid request" + (is (thrown-with-msg? + ExceptionInfo + #"Request coercion failed" + (app invalid-request)))) + + (testing "invalid response" + (is (thrown-with-msg? + ExceptionInfo + #"Response coercion failed" + (app invalid-request2)))))) + + (testing "with exception handling" + (let [app (create [coercion/gen-wrap-coerce-exceptions + coercion/gen-wrap-coerce-parameters + coercion/gen-wrap-coerce-response])] + + (testing "all good" + (is (= {:status 200 + :body {:total 15}} + (app valid-request)))) + + (testing "invalid request" + (let [{:keys [status body]} (app invalid-request)] + (is (= 400 status)))) + + (testing "invalid response" + (let [{:keys [status body]} (app invalid-request2)] + (is (= 500 status)))))))) + +(deftest schema-coercion-test + (let [create (fn [middleware] + (ring/ring-handler + (ring/router + ["/api" + ["/plus/:e" + {:get {:parameters {:query {:a s/Int} + :body {:b s/Int} + :form {:c s/Int} + :header {:d s/Int} + :path {:e s/Int}} + :responses {200 {:schema {:total (s/constrained s/Int pos? 'positive)}}} + :handler handler}}]] + {:data {:middleware middleware + :coercion schema/coercion}})))] + + (testing "withut exception handling" + (let [app (create [coercion/gen-wrap-coerce-parameters + coercion/gen-wrap-coerce-response])] + + (testing "all good" + (is (= {:status 200 + :body {:total 15}} + (app valid-request)))) + + (testing "invalid request" + (is (thrown-with-msg? + ExceptionInfo + #"Request coercion failed" + (app invalid-request)))) + + (testing "invalid response" + (is (thrown-with-msg? + ExceptionInfo + #"Response coercion failed" + (app invalid-request2)))) + + (testing "with exception handling" + (let [app (create [coercion/gen-wrap-coerce-exceptions + coercion/gen-wrap-coerce-parameters + coercion/gen-wrap-coerce-response])] + + (testing "all good" + (is (= {:status 200 + :body {:total 15}} + (app valid-request)))) + + (testing "invalid request" + (let [{:keys [status body]} (app invalid-request)] + (is (= 400 status)))) + + (testing "invalid response" + (let [{:keys [status body]} (app invalid-request2)] + (is (= 500 status)))))))))) From 7979c9de9d42afd611e13da40502ac8ea1c0c0e3 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Sun, 26 Nov 2017 21:51:55 +0200 Subject: [PATCH 13/17] Remove normal middleware, slow and noicy --- .../reitit-ring/src/reitit/ring/coercion.cljc | 53 ------------------- 1 file changed, 53 deletions(-) diff --git a/modules/reitit-ring/src/reitit/ring/coercion.cljc b/modules/reitit-ring/src/reitit/ring/coercion.cljc index fe21d594..41c98627 100644 --- a/modules/reitit-ring/src/reitit/ring/coercion.cljc +++ b/modules/reitit-ring/src/reitit/ring/coercion.cljc @@ -110,31 +110,6 @@ [status (response-coercer coercion schema opts)]) (into {}))) -(defn wrap-coerce-parameters - "Pluggable request coercion middleware. - Expects a :coercion of type `reitit.coercion.protocol/Coercion` - and :parameters from route data, otherwise will do nothing." - [handler] - (fn - ([request] - (let [method (:request-method request) - match (ring/get-match request) - parameters (-> match :result method :data :parameters) - coercion (-> match :data :coercion)] - (if (and coercion parameters) - (let [coercers (request-coercers coercion parameters) - coerced (coerce-parameters coercers request)] - (handler (impl/fast-assoc request :parameters coerced))) - (handler request)))) - ([request respond raise] - (let [method (:request-method request) - match (ring/get-match request) - parameters (-> match :result method :data :parameters) - coercion (-> match :data :coercion)] - (if (and coercion parameters) - (let [coercers (request-coercers coercion parameters) - coerced (coerce-parameters coercers request)] - (handler (impl/fast-assoc request :parameters coerced) respond raise))))))) (defn handle-coercion-exception [e respond raise] (let [data (ex-data e)] (if-let [status (condp = (:type data) @@ -168,34 +143,6 @@ (let [coerced (coerce-request coercers request)] (handler (impl/fast-assoc request :parameters coerced) respond raise))))))))})) -(defn wrap-coerce-response - "Pluggable response coercion middleware. - Expects a :coercion of type `reitit.coercion.protocol/Coercion` - and :responses from route data, otherwise will do nothing." - [handler] - (fn - ([request] - (let [response (handler request) - method (:request-method request) - match (ring/get-match request) - responses (-> match :result method :data :responses) - coercion (-> match :data :coercion) - opts (-> match :data :opts)] - (if (and coercion responses) - (let [coercers (response-coercers coercion responses opts)] - (coerce-response coercers request response)) - response))) - ([request respond raise] - (let [method (:request-method request) - match (ring/get-match request) - responses (-> match :result method :data :responses) - coercion (-> match :data :coercion) - opts (-> match :data :opts)] - (if (and coercion responses) - (let [coercers (response-coercers coercion responses opts)] - (handler request #(respond (coerce-response coercers request %)))) - (handler request respond raise)))))) - (def gen-wrap-coerce-response "Middleware for pluggable response coercion. Expects a :coercion of type `reitit.coercion.protocol/Coercion` From 132240b422e5b18a2aa9837353a5c76f810d5ac6 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Sun, 26 Nov 2017 22:04:51 +0200 Subject: [PATCH 14/17] ClojureScriptify Exceptions --- modules/reitit-ring/src/reitit/ring/coercion.cljc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/reitit-ring/src/reitit/ring/coercion.cljc b/modules/reitit-ring/src/reitit/ring/coercion.cljc index 41c98627..295ba4de 100644 --- a/modules/reitit-ring/src/reitit/ring/coercion.cljc +++ b/modules/reitit-ring/src/reitit/ring/coercion.cljc @@ -172,10 +172,10 @@ ([request] (try (handler request) - (catch Exception e + (catch #?(:clj Exception :cljs js/Error) e (handle-coercion-exception e identity #(throw %))))) ([request respond raise] (try (handler request respond (fn [e] (handle-coercion-exception e respond raise))) - (catch Throwable e + (catch #?(:clj Exception :cljs js/Error) e (handle-coercion-exception e respond raise))))))))})) From becd30386d6a3be0b83790c4b98c62d27b2221a7 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 27 Nov 2017 08:00:27 +0200 Subject: [PATCH 15/17] Works with ClojureScript! --- .../reitit-ring/src/reitit/ring/coercion.cljc | 4 ++-- .../src/reitit/ring/coercion/schema.cljc | 24 +++++++------------ .../src/reitit/ring/coercion/spec.cljc | 4 +--- test/cljc/reitit/coercion_test.cljc | 2 +- 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/modules/reitit-ring/src/reitit/ring/coercion.cljc b/modules/reitit-ring/src/reitit/ring/coercion.cljc index 295ba4de..40820699 100644 --- a/modules/reitit-ring/src/reitit/ring/coercion.cljc +++ b/modules/reitit-ring/src/reitit/ring/coercion.cljc @@ -160,7 +160,7 @@ (handler request #(respond (coerce-response coercers request %)) raise)))))))})) (def gen-wrap-coerce-exceptions - "Middleare for coercion exception handling. + "Middleware for handling coercion exceptions. Expects a :coercion of type `reitit.coercion.protocol/Coercion` and :parameters or :responses from route data, otherwise does not mount." (middleware/create @@ -176,6 +176,6 @@ (handle-coercion-exception e identity #(throw %))))) ([request respond raise] (try - (handler request respond (fn [e] (handle-coercion-exception e respond raise))) + (handler request respond #(handle-coercion-exception % respond raise)) (catch #?(:clj Exception :cljs js/Error) e (handle-coercion-exception e respond raise))))))))})) diff --git a/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc b/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc index 7733d574..5029f6ba 100644 --- a/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc +++ b/modules/reitit-schema/src/reitit/ring/coercion/schema.cljc @@ -6,9 +6,7 @@ [schema-tools.coerce :as stc] [spec-tools.swagger.core :as swagger] [clojure.walk :as walk] - [reitit.ring.coercion.protocol :as protocol]) - (:import (schema.core OptionalKey RequiredKey) - (schema.utils ValidationError NamedError))) + [reitit.ring.coercion.protocol :as protocol])) (def string-coercion-matcher stc/string-coercion-matcher) @@ -26,12 +24,12 @@ (walk/prewalk (fn [x] (cond - (class? x) (.getName ^Class x) - (instance? OptionalKey x) (pr-str (list 'opt (:k x))) - (instance? RequiredKey x) (pr-str (list 'req (:k x))) - (and (satisfies? s/Schema x) (record? x)) (try (pr-str (s/explain x)) (catch Exception _ x)) - (instance? ValidationError x) (str (su/validation-error-explain x)) - (instance? NamedError x) (str (su/named-error-explain x)) + #?@(:clj [(class? x) (.getName ^Class x)]) + (instance? schema.core.OptionalKey x) (pr-str (list 'opt (:k x))) + (instance? schema.core.RequiredKey x) (pr-str (list 'req (:k x))) + (and (satisfies? s/Schema x) (record? x)) (try (pr-str (s/explain x)) (catch #?(:clj Exception :cljs js/Error) _ x)) + (instance? schema.utils.ValidationError x) (str (su/validation-error-explain x)) + (instance? schema.utils.NamedError x) (str (su/named-error-explain x)) :else x)) schema)) @@ -45,12 +43,8 @@ (get-apidocs [_ _ {:keys [parameters responses] :as info}] (cond-> (dissoc info :parameters :responses) - parameters (assoc - ::swagger/parameters - parameters) - responses (assoc - ::swagger/responses - responses))) + parameters (assoc ::swagger/parameters parameters) + responses (assoc ::swagger/responses responses))) (make-open [_ schema] (st/open-schema schema)) diff --git a/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc b/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc index edf96ce2..cd2aba4f 100644 --- a/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc +++ b/modules/reitit-spec/src/reitit/ring/coercion/spec.cljc @@ -51,9 +51,7 @@ (memoize #(into-spec %1 (gensym "spec")))) (defn stringify-pred [pred] - (str (if (instance? clojure.lang.LazySeq pred) - (seq pred) - pred))) + (str (if (seq? pred) (seq pred) pred))) (defmulti coerce-response? identity :default ::default) (defmethod coerce-response? ::default [_] true) diff --git a/test/cljc/reitit/coercion_test.cljc b/test/cljc/reitit/coercion_test.cljc index a9b8a62f..c9d5ec71 100644 --- a/test/cljc/reitit/coercion_test.cljc +++ b/test/cljc/reitit/coercion_test.cljc @@ -1,9 +1,9 @@ (ns reitit.coercion-test (:require [clojure.test :refer [deftest testing is]] + [schema.core :as s] [reitit.ring :as ring] [reitit.ring.coercion :as coercion] [reitit.ring.coercion.spec :as spec] - [schema.core :as s] [reitit.ring.coercion.schema :as schema]) #?(:clj (:import (clojure.lang ExceptionInfo)))) From 97598ce19422f2d9ce713c16f273e3ec12d87d7d Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 27 Nov 2017 08:01:52 +0200 Subject: [PATCH 16/17] Fix :body coercion (not open, :body coercer) --- modules/reitit-ring/src/reitit/ring/coercion.cljc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/reitit-ring/src/reitit/ring/coercion.cljc b/modules/reitit-ring/src/reitit/ring/coercion.cljc index 40820699..3dda55dc 100644 --- a/modules/reitit-ring/src/reitit/ring/coercion.cljc +++ b/modules/reitit-ring/src/reitit/ring/coercion.cljc @@ -19,7 +19,7 @@ (def ring-parameter-coercion {:query (->ParameterCoercion :query-params :string true true) - :body (->ParameterCoercion :body-params :string false true) + :body (->ParameterCoercion :body-params :body false false) :form (->ParameterCoercion :form-params :string true true) :header (->ParameterCoercion :header-params :string true true) :path (->ParameterCoercion :path-params :string true true)}) From 9538d74ae06d59ff5800149a2b2a7f715214f93c Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Mon, 27 Nov 2017 08:02:35 +0200 Subject: [PATCH 17/17] Tune docs --- README.md | 8 +- doc/README.md | 8 +- doc/SUMMARY.md | 3 +- doc/faq.md | 1 + doc/ring/README.md | 2 +- doc/ring/coercion.md | 209 +++++++++++++++++++++++++++++++ doc/ring/compiling_middleware.md | 28 ++--- doc/ring/parameter_coercion.md | 96 -------------- 8 files changed, 237 insertions(+), 118 deletions(-) create mode 100644 doc/faq.md create mode 100644 doc/ring/coercion.md delete mode 100644 doc/ring/parameter_coercion.md diff --git a/README.md b/README.md index a7757fe2..d5b5e9d6 100644 --- a/README.md +++ b/README.md @@ -6,9 +6,10 @@ A friendly data-driven router for Clojure(Script). * Route [conflict resolution](https://metosin.github.io/reitit/basics/route_conflicts.html) * First-class [route data](https://metosin.github.io/reitit/basics/route_data.html) * Bi-directional routing -* [Ring-router](https://metosin.github.io/reitit/ring.html) with data-driven [middleware](https://metosin.github.io/reitit/ring/compiling_middleware.html) -* [Pluggable coercion](https://metosin.github.io/reitit/ring/parameter_coercion.html) ([clojure.spec](https://clojure.org/about/spec)) +* [Ring-router](https://metosin.github.io/reitit/ring/ring.html) with [data-driven middleware](https://metosin.github.io/reitit/ring/data_driven_middleware.html) +* [Pluggable coercion](https://metosin.github.io/reitit/ring/coercion.html) ([schema](https://github.com/plumatic/schema) & [clojure.spec](https://clojure.org/about/spec)) * Extendable +* Modular * [Fast](https://metosin.github.io/reitit/performance.html) See the [full documentation](https://metosin.github.io/reitit/) for details. @@ -26,7 +27,8 @@ Optionally, the parts can be required separately: ```clj [metosin/reitit-core "0.1.0-SNAPSHOT"] ; just the router [metosin/reitit-ring "0.1.0-SNAPSHOT"] ; ring-router -[metosin/reitit-spec "0.1.0-SNAPSHOT"] ; spec-coercion +[metosin/reitit-spec "0.1.0-SNAPSHOT"] ; spec coercion +[metosin/reitit-schema "0.1.0-SNAPSHOT"] ; schema coercion ``` ## Quick start diff --git a/doc/README.md b/doc/README.md index 4dfd2318..bde4bab0 100644 --- a/doc/README.md +++ b/doc/README.md @@ -3,12 +3,13 @@ [Reitit](https://github.com/metosin/reitit) is a small Clojure(Script) library for data-driven routing. * Simple data-driven [route syntax](./basics/route_syntax.md) -* [Route conflict resolution](./advanced/route_conflicts.md) +* [Route conflict resolution](./basics/route_conflicts.md) * First-class [route data](./basics/route_data.md) * Bi-directional routing -* [Pluggable coercion](./ring/parameter_coercion.md) ([clojure.spec](https://clojure.org/about/spec)) -* supports both [Middleware](./ring/compiling_middleware.md) & Interceptors +* [Ring-router](./ring/ring.html) with [data-driven middleware](./ring/data_driven_middleware.html) +* [Pluggable coercion](./ring/coercion.html) ([schema](https://github.com/plumatic/schema) & [clojure.spec](https://clojure.org/about/spec)) * Extendable +* Modular * [Fast](performance.md) To use Reitit, add the following dependecy to your project: @@ -23,6 +24,7 @@ Optionally, the parts can be required separately: [metosin/reitit-core "0.1.0-SNAPSHOT"] ; just the router [metosin/reitit-ring "0.1.0-SNAPSHOT"] ; ring-router [metosin/reitit-spec "0.1.0-SNAPSHOT"] ; spec-coercion +[metosin/reitit-schema "0.1.0-SNAPSHOT"] ; schema coercion ``` For discussions, there is a [#reitit](https://clojurians.slack.com/messages/reitit/) channel in [Clojurians slack](http://clojurians.net/). diff --git a/doc/SUMMARY.md b/doc/SUMMARY.md index f221c216..8930d756 100644 --- a/doc/SUMMARY.md +++ b/doc/SUMMARY.md @@ -16,8 +16,9 @@ * [Ring-router](ring/ring.md) * [Dynamic extensions](ring/dynamic_extensions.md) * [Data-driven Middleware](ring/data_driven_middleware.md) - * [Parameter coercion](ring/parameter_coercion.md) + * [Pluggable Coercion](ring/coercion.md) * [Compiling middleware](ring/compiling_middleware.md) * [Performance](performance.md) +* [FAQ](faq.md) * TODO: Swagger & OpenAPI * TODO: Interceptors diff --git a/doc/faq.md b/doc/faq.md new file mode 100644 index 00000000..318b08dc --- /dev/null +++ b/doc/faq.md @@ -0,0 +1 @@ +# Frequently Asked Questions diff --git a/doc/ring/README.md b/doc/ring/README.md index 9bbe9b59..7db18acf 100644 --- a/doc/ring/README.md +++ b/doc/ring/README.md @@ -3,5 +3,5 @@ * [Ring-router](ring.md) * [Dynamic extensions](dynamic_extensions.md) * [Data-driven Middleware](data_driven_middleware.md) -* [Parameter coercion](parameter_coercion.md) +* [Pluggable Coercion](coercion.md) * [Compiling middleware](compiling_middleware.md) diff --git a/doc/ring/coercion.md b/doc/ring/coercion.md new file mode 100644 index 00000000..a2a730a0 --- /dev/null +++ b/doc/ring/coercion.md @@ -0,0 +1,209 @@ +# Pluggable Coercion + +Reitit provides pluggable parameter coercion via `reitit.ring.coercion.protocol/Coercion` protocol, originally introduced in [compojure-api](https://clojars.org/metosin/compojure-api). + +Reitit ships with the following coercion modules: + +* `reitit.ring.coercion.schema/SchemaCoercion` for [plumatic schema](https://github.com/plumatic/schema). +* `reitit.ring.coercion.spec/SpecCoercion` for both [clojure.spec](https://clojure.org/about/spec) and [data-specs](https://github.com/metosin/spec-tools#data-specs). + +### Ring request and response coercion + +To use `Coercion` with Ring, one needs to do the following: + +1. Define parameters and responses as data into route data, in format adopted from [ring-swagger](https://github.com/metosin/ring-swagger#more-complete-example): + * `:parameters` map, with submaps for different parameters: `:query`, `:body`, `:form`, `:header` and `:path`. Parameters are defined in the format understood by the `Coercion`. + * `:responses` map, with response status codes as keys (or `:default` for "everything else") with maps with `:schema` and optionally `:description` as values. +2. Set a `Coercion` implementation to route data under `:coercion` +3. Mount request & response coercion middleware to the routes (can be done for all routes as the middleware are only mounted to routes which have the parameters &/ responses defined): + * `reitit.ring.coercion/gen-wrap-coerce-parameters` + * `reitit.ring.coercion/gen-wrap-coerce-response` + +If the request coercion succeeds, the coerced parameters are injected into request under `:parameters`. + +If either request or response coercion fails, an descriptive error is thrown. To turn the exceptions into http responses, one can also mount the `reitit.ring.coercion/gen-wrap-coerce-exceptions` middleware + +### Example with Schema + +```clj +(require '[reitit.ring :as ring]) +(require '[reitit.ring.coercion :as coercion]) +(require '[reitit.ring.coercion.schema :as schema]) +(require '[schema.core :as s]) + +(def app + (ring/ring-handler + (ring/router + ["/api" + ["/ping" {:parameters {:body {:x s/Int, :y s/Int}} + :responses {200 {:schema {:total (s/constrained s/Int pos?}}} + :get {:handler (fn [{{{:keys [x y]} :body} :parameters}] + {:status 200 + :body {:total (+ x y)}})}}]] + {:data {:middleware [coercion/gen-wrap-coerce-exceptions + coercion/gen-wrap-coerce-parameters + coercion/gen-wrap-coerce-response] + :coercion schema/coercion}}))) +``` + +Valid request: + +```clj +(app + {:request-method :get + :uri "/api/ping" + :body-params {:x 1, :y 2}}) +; {:status 200 +; :body {:total 3}} +``` + +Invalid request: + +```clj +(app + {:request-method :get + :uri "/api/ping" + :body-params {:x 1, :y "2"}}) +; {:status 400, +; :body {:type :reitit.ring.coercion/request-coercion +; :coercion :schema +; :in [:request :body-params] +; :value {:x 1, :y "2"} +; :schema {:x "Int", :y "Int"} +; :errors {:y "(not (integer? \"2\"))"}}} +``` + +### Example with data-specs + +```clj +(require '[reitit.ring :as ring]) +(require '[reitit.ring.coercion :as coercion]) +(require '[reitit.ring.coercion.spec :as spec]) + +(def app + (ring/ring-handler + (ring/router + ["/api" + ["/ping" {:parameters {:body {:x int?, :y int?}} + :responses {200 {:schema {:total pos-int?}}} + :get {:handler (fn [{{{:keys [x y]} :body} :parameters}] + {:status 200 + :body {:total (+ x y)}})}}]] + {:data {:middleware [coercion/gen-wrap-coerce-exceptions + coercion/gen-wrap-coerce-parameters + coercion/gen-wrap-coerce-response] + :coercion spec/coercion}}))) +``` + +Valid request: + +```clj +(app + {:request-method :get + :uri "/api/ping" + :body-params {:x 1, :y 2}}) +; {:status 200 +; :body {:total 3}} +``` + +Invalid request: + +```clj +(app + {:request-method :get + :uri "/api/ping" + :body-params {:x 1, :y "2"}}) +; {:status 400, +; :body {:type ::coercion/request-coercion +; :coercion :spec +; :in [:request :body-params] +; :value {:x 1, :y "2"} +; :spec "(spec-tools.core/spec {:spec (clojure.spec.alpha/keys :req-un [:$spec37747/x :$spec37747/y]), :type :map, :keys #{:y :x}, :keys/req #{:y :x}})" +; :problems [{:path [:y] +; :pred "clojure.core/int?" +; :val "2" +; :via [:$spec37747/y] +; :in [:y]}]}} +``` + +### Example with clojure.spec + +Currently, `clojure.spec` [doesn't support runtime transformations via conforming](https://dev.clojure.org/jira/browse/CLJ-2116), so one needs to wrap all specs with `spec-tools.core/spec`. + +```clj +(require '[reitit.ring :as ring]) +(require '[reitit.ring.coercion :as coercion]) +(require '[reitit.ring.coercion.spec :as spec]) +(require '[clojure.spec.alpha :as s]) +(require '[spec-tools.core :as st]) + +(s/def ::x (st/spec int?)) +(s/def ::y (st/spec int?)) +(s/def ::total int?) +(s/def ::request (s/keys :req-un [::x ::y])) +(s/def ::response (s/keys :req-un [::total])) + +(def app + (ring/ring-handler + (ring/router + ["/api" + ["/ping" {:parameters {:body ::request} + :responses {200 {:schema ::response}} + :get {:handler (fn [{{{:keys [x y]} :body} :parameters}] + {:status 200 + :body {:total (+ x y)}})}}]] + {:data {:middleware [coercion/gen-wrap-coerce-exceptions + coercion/gen-wrap-coerce-parameters + coercion/gen-wrap-coerce-response] + :coercion spec/coercion}}))) +``` + +Valid request: + +```clj +(app + {:request-method :get + :uri "/api/ping" + :body-params {:x 1, :y 2}}) +; {:status 200 +; :body {:total 3}} +``` + +Invalid request: + +```clj +(app + {:request-method :get + :uri "/api/ping" + :body-params {:x 1, :y "2"}}) +; {:status 400, +; :body {:type ::coercion/request-coercion +; :coercion :spec +; :in [:request :body-params] +; :value {:x 1, :y "2"} +; :spec "(spec-tools.core/spec {:spec (clojure.spec.alpha/keys :req-un [:reitit.coercion-test/x :reitit.coercion-test/y]), :type :map, :keys #{:y :x}, :keys/req #{:y :x}})" +; :problems [{:path [:y] +; :pred "clojure.core/int?" +; :val "2" +; :via [::request ::y] +; :in [:y]}]}} +``` + +### Custom coercion + +Both Schema and Spec Coercion can be configured via options, see the source code for details. + +To plug in new validation engine, see the +`reitit.ring.coercion.protocol/Coercion` protocol. + +```clj +(defprotocol Coercion + "Pluggable coercion protocol" + (get-name [this] "Keyword name for the coercion") + (compile [this model name] "Compiles a coercion model") + (get-apidocs [this model data] "???") + (make-open [this model] "Returns a new map model which doesn't fail on extra keys") + (encode-error [this error] "Converts error in to a serializable format") + (request-coercer [this type model] "Returns a `value format => value` request coercion function") + (response-coercer [this model] "Returns a `value format => value` response coercion function")) +``` diff --git a/doc/ring/compiling_middleware.md b/doc/ring/compiling_middleware.md index 19afd272..c522e8e8 100644 --- a/doc/ring/compiling_middleware.md +++ b/doc/ring/compiling_middleware.md @@ -2,13 +2,13 @@ The [dynamic extensions](dynamic_extensions.md) is a easy way to extend the system. To enable fast lookups into route data, we can compile them into any shape (records, functions etc.) we want, enabling fast access at request-time. -Still, we can do much better. As we know the exact route that middleware/interceptor is linked to, we can pass the (compiled) route information into the middleware/interceptor at creation-time. It can do local reasoning: extract and transform relevant data just for it and pass it into the actual request-handler via a closure - yielding much faster runtime processing. It can also decide not to mount itself by returning `nil`. Why mount a `wrap-enforce-roles` middleware for a route if there are no roles required for it? +But, we can do much better. As we know the exact route that middleware/interceptor is linked to, we can pass the (compiled) route information into the middleware/interceptor at creation-time. It can do local reasoning: extract and transform relevant data just for it and pass it into the actual request-handler via a closure - yielding much faster runtime processing. It can also decide not to mount itself by returning `nil`. Why mount a `wrap-enforce-roles` middleware for a route if there are no roles required for it? To enable this we use [middleware records](data_driven_middleware.md) `:gen-wrap` key instead of the normal `:wrap`. `:gen-wrap` expects a function of `route-data router-opts => ?wrap`. -To demonstrate the two approaches, below are response coercion middleware written as normal ring middleware function and as middleware record with `:gen-wrap`. Actual codes can be found in [`reitit.ring.coercion`](https://github.com/metosin/reitit/blob/master/src/reitit/ring/coercion.cljc): +To demonstrate the two approaches, below are response coercion middleware written as normal ring middleware function and as middleware record with `:gen-wrap`. -## Naive +## Normal Middleware * Reads the compiled route information on every request. @@ -42,7 +42,7 @@ To demonstrate the two approaches, below are response coercion middleware writte (handler request respond raise)))))) ``` -## Compiled +## Compiled Middleware * Route information is provided via a closure * Pre-compiled coercers @@ -52,20 +52,20 @@ To demonstrate the two approaches, below are response coercion middleware writte (require '[reitit.ring.middleware :as middleware]) (def gen-wrap-coerce-response - "Generator for pluggable response coercion middleware. + "Middleware for pluggable response coercion. Expects a :coercion of type `reitit.coercion.protocol/Coercion` and :responses from route data, otherwise does not mount." (middleware/create {:name ::coerce-response - :gen-wrap (fn [{:keys [responses coercion opts]} _] - (if (and coercion responses) - (let [coercers (response-coercers coercion responses opts)] - (fn [handler] - (fn - ([request] - (coerce-response coercers request (handler request))) - ([request respond raise] - (handler request #(respond (coerce-response coercers request %)) raise)))))))})) + :gen-wrap (fn [{:keys [coercion responses opts]} _] + (if (and coercion responses) + (let [coercers (response-coercers coercion responses opts)] + (fn [handler] + (fn + ([request] + (coerce-response coercers request (handler request))) + ([request respond raise] + (handler request #(respond (coerce-response coercers request %)) raise)))))))})) ``` The latter has 50% less code, is easier to reason about and is much faster. diff --git a/doc/ring/parameter_coercion.md b/doc/ring/parameter_coercion.md deleted file mode 100644 index 10b20d6b..00000000 --- a/doc/ring/parameter_coercion.md +++ /dev/null @@ -1,96 +0,0 @@ -# Parameter coercion - -Reitit provides pluggable parameter coercion via `reitit.ring.coercion.protocol/Coercion` protocol, originally introduced in [compojure-api](https://clojars.org/metosin/compojure-api). Reitit ships with `reitit.ring.coercion.spec/SpecCoercion` providing implemenation for [clojure.spec](https://clojure.org/about/spec) and [data-specs](https://github.com/metosin/spec-tools#data-specs). - -**NOTE**: Before Clojure 1.9.0 is shipped, to use the spec-coercion, one needs to add the following dependencies manually to the project: - -```clj -[org.clojure/clojure "1.9.0-beta2"] -[org.clojure/spec.alpha "0.1.123"] -[metosin/spec-tools "0.4.0"] -``` - -### Ring request and response coercion - -To use `Coercion` with Ring, one needs to do the following: - -1. Define parameters and responses as data into route data, in format adopted from [ring-swagger](https://github.com/metosin/ring-swagger#more-complete-example): - * `:parameters` map, with submaps for different parameters: `:query`, `:body`, `:form`, `:header` and `:path`. Parameters are defined in the format understood by the `Coercion`. - * `:responses` map, with response status codes as keys (or `:default` for "everything else") with maps with `:schema` and optionally `:description` as values. -2. Define a `Coercion` to route data under `:coercion` -3. Mount request & response coercion middleware to the routes (recommended to mount to all routes under router as they mounted only to routes which have the parameters / responses defined): - * `reitit.ring.coercion/gen-wrap-coerce-parameters` - * `gen-wrap-coerce-parameters/gen-wrap-coerce-responses` - -If the request coercion succeeds, the coerced parameters are injected into request under `:parameters`. - -If either request or response coercion fails, an descriptive error is thrown. - -#### Example with data-specs - -```clj -(require '[reitit.ring :as ring]) -(require '[reitit.ring.coercion :as coercion]) -(require '[reitit.ring.coercion.spec :as spec]) - -(def app - (ring/ring-handler - (ring/router - ["/api" - ["/ping" {:parameters {:body {:x int?, :y int?}} - :responses {200 {:schema {:total pos-int?}}} - :get {:handler (fn [{{{:keys [x y]} :body} :parameters}] - {:status 200 - :body {:total (+ x y)}})}}]] - {:data {:middleware [coercion/gen-wrap-coerce-parameters - coercion/gen-wrap-coerce-response] - :coercion spec/coercion}}))) -``` - - -```clj -(app - {:request-method :get - :uri "/api/ping" - :body-params {:x 1, :y 2}}) -; {:status 200, :body {:total 3}} -``` - -#### Example with specs - -Currently, `clojure.spec` [doesn't support runtime transformations via conforming](https://dev.clojure.org/jira/browse/CLJ-2116), so one needs to wrap all specs with `spec-tools.core/spec`. - -```clj -(require '[reitit.ring :as ring]) -(require '[reitit.ring.coercion :as coercion]) -(require '[reitit.ring.coercion.spec :as spec]) -(require '[clojure.spec.alpha :as s]) -(require '[spec-tools.core :as st]) - -(s/def ::x (st/spec int?)) -(s/def ::y (st/spec int?)) -(s/def ::total int?) -(s/def ::request (s/keys :req-un [::x ::y])) -(s/def ::response (s/keys :req-un [::total])) - -(def app - (ring/ring-handler - (ring/router - ["/api" - ["/ping" {:parameters {:body ::request} - :responses {200 {:schema ::response}} - :get {:handler (fn [{{{:keys [x y]} :body} :parameters}] - {:status 200 - :body {:total (+ x y)}})}}]] - {:data {:middleware [coercion/gen-wrap-coerce-parameters - coercion/gen-wrap-coerce-response] - :coercion spec/coercion}}))) -``` - -```clj -(app - {:request-method :get - :uri "/api/ping" - :body-params {:x 1, :y 2}}) -; {:status 200, :body {:total 3}} -```