From 83c31e35bc15e4b2e3cace4cf1cc8955e03cee64 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Wed, 18 Jan 2023 20:28:26 +0200 Subject: [PATCH 1/2] Revert "Revert "Merge pull request #554 from just-sultanov/add-support-for-fragment-parameters"" This reverts commit 4d1b00edfab4c50bac22973e80f4de22aeb52ea5. --- modules/reitit-core/src/reitit/coercion.cljc | 3 +- .../reitit-frontend/src/reitit/frontend.cljs | 20 ++++++++- test/cljs/reitit/frontend/core_test.cljs | 43 +++++++++++++++---- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/modules/reitit-core/src/reitit/coercion.cljc b/modules/reitit-core/src/reitit/coercion.cljc index 4436214c..75312c86 100644 --- a/modules/reitit-core/src/reitit/coercion.cljc +++ b/modules/reitit-core/src/reitit/coercion.cljc @@ -39,7 +39,8 @@ :body (->ParameterCoercion :body-params :body false false) :form (->ParameterCoercion :form-params :string true true) :header (->ParameterCoercion :headers :string true true) - :path (->ParameterCoercion :path-params :string true true)}) + :path (->ParameterCoercion :path-params :string true true) + :fragment (->ParameterCoercion :fragment-params :string true true)}) (defn ^:no-doc request-coercion-failed! [result coercion value in request serialize-failed-result] (throw diff --git a/modules/reitit-frontend/src/reitit/frontend.cljs b/modules/reitit-frontend/src/reitit/frontend.cljs index d1e9c50a..922b5a8b 100644 --- a/modules/reitit-frontend/src/reitit/frontend.cljs +++ b/modules/reitit-frontend/src/reitit/frontend.cljs @@ -1,5 +1,6 @@ (ns reitit.frontend (:require [clojure.set :as set] + [clojure.string :as str] [reitit.coercion :as coercion] [reitit.core :as r]) (:import goog.Uri @@ -20,6 +21,19 @@ (map (juxt keyword #(query-param q %))) (into {})))) +(defn fragment-params + "Given goog.Uri, read fragment parameters into Clojure map." + [^Uri uri] + (let [fp (.getFragment uri)] + (if-not (seq fp) + {} + (into {} + (comp + (map #(str/split % #"=")) + (map (fn [[k v]] + [(keyword k) v]))) + (str/split fp #"&"))))) + (defn match-by-path "Given routing tree and current path, return match with possibly coerced parameters. Return nil if no match found. @@ -37,12 +51,14 @@ coercion/coerce!)] (if-let [match (r/match-by-path router (.getPath uri))] (let [q (query-params uri) - match (assoc match :query-params q) + fp (fragment-params uri) + match (assoc match :query-params q :fragment-params fp) ;; Return uncoerced values if coercion is not enabled - so ;; that tha parameters are always accessible from same property. parameters (or (coerce! match) {:path (:path-params match) - :query q})] + :query q + :fragment fp})] (assoc match :parameters parameters)))))) (defn match-by-name diff --git a/test/cljs/reitit/frontend/core_test.cljs b/test/cljs/reitit/frontend/core_test.cljs index 72bf9428..42a45bde 100644 --- a/test/cljs/reitit/frontend/core_test.cljs +++ b/test/cljs/reitit/frontend/core_test.cljs @@ -21,9 +21,11 @@ :data {:name ::frontpage} :path-params {} :query-params {} + :fragment-params {} :path "/" :parameters {:query {} - :path {}}}) + :path {} + :fragment {}}}) (rf/match-by-path router "/"))) (is (= "/" @@ -34,9 +36,11 @@ :data {:name ::foo} :path-params {} :query-params {} + :fragment-params {} :path "/foo" :parameters {:query {} - :path {}}}) + :path {} + :fragment {}}}) (rf/match-by-path router "/foo"))) (is (= (r/map->Match @@ -44,9 +48,11 @@ :data {:name ::foo} :path-params {} :query-params {:mode ["foo", "bar"]} + :fragment-params {} :path "/foo" :parameters {:query {:mode ["foo", "bar"]} - :path {}}}) + :path {} + :fragment {}}}) (rf/match-by-path router "/foo?mode=foo&mode=bar"))) (is (= "/foo" @@ -64,7 +70,12 @@ (let [router (r/router ["/" [":id" {:name ::foo :parameters {:path {:id s/Int} - :query {(s/optional-key :mode) s/Keyword}}}]] + :query {(s/optional-key :mode) s/Keyword} + :fragment {(s/optional-key :access_token) s/Str + (s/optional-key :refresh_token) s/Str + (s/optional-key :expires_in) s/Int + (s/optional-key :provider_token) s/Str + (s/optional-key :token_type) s/Str}}}]] {:compile rc/compile-request-coercers :data {:coercion rsc/coercion}})] @@ -72,9 +83,11 @@ {:template "/:id" :path-params {:id "5"} :query-params {} + :fragment-params {} :path "/5" :parameters {:query {} - :path {:id 5}}}) + :path {:id 5} + :fragment {}}}) (m (rf/match-by-path router "/5")))) (is (= "/5" @@ -98,23 +111,35 @@ {:template "/:id" :path-params {:id "5"} :query-params {:mode "foo"} + :fragment-params {} :path "/5" :parameters {:path {:id 5} - :query {:mode :foo}}}) + :query {:mode :foo} + :fragment {}}}) (m (rf/match-by-path router "/5?mode=foo")))) (is (= "/5?mode=foo" (r/match->path (rf/match-by-name router ::foo {:id 5}) {:mode :foo})))) - (testing "fragment is ignored" + (testing "fragment is read" (is (= (r/map->Match {:template "/:id" :path-params {:id "5"} :query-params {:mode "foo"} + :fragment-params {:access_token "foo" + :refresh_token "bar" + :provider_token "baz" + :token_type "bearer" + :expires_in "3600"} :path "/5" :parameters {:path {:id 5} - :query {:mode :foo}}}) - (m (rf/match-by-path router "/5?mode=foo#fragment"))))) + :query {:mode :foo} + :fragment {:access_token "foo" + :refresh_token "bar" + :provider_token "baz" + :token_type "bearer" + :expires_in 3600}}}) + (m (rf/match-by-path router "/5?mode=foo#access_token=foo&refresh_token=bar&provider_token=baz&token_type=bearer&expires_in=3600"))))) (testing "console warning about missing params" (is (= [{:type :warn From 2494f702d9d70484aa8ec7a6a445496fb5a6e29c Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Wed, 18 Jan 2023 20:24:33 +0200 Subject: [PATCH 2/2] Read fragment string without decoding Users can use Malli decoding to control decoding per schema. --- modules/reitit-core/src/reitit/coercion.cljc | 2 +- .../reitit-frontend/src/reitit/frontend.cljs | 23 +-- test/cljs/reitit/frontend/core_test.cljs | 135 ++++++++++++++---- 3 files changed, 112 insertions(+), 48 deletions(-) diff --git a/modules/reitit-core/src/reitit/coercion.cljc b/modules/reitit-core/src/reitit/coercion.cljc index 75312c86..8c382e9a 100644 --- a/modules/reitit-core/src/reitit/coercion.cljc +++ b/modules/reitit-core/src/reitit/coercion.cljc @@ -40,7 +40,7 @@ :form (->ParameterCoercion :form-params :string true true) :header (->ParameterCoercion :headers :string true true) :path (->ParameterCoercion :path-params :string true true) - :fragment (->ParameterCoercion :fragment-params :string true true)}) + :fragment (->ParameterCoercion :fragment :string true true)}) (defn ^:no-doc request-coercion-failed! [result coercion value in request serialize-failed-result] (throw diff --git a/modules/reitit-frontend/src/reitit/frontend.cljs b/modules/reitit-frontend/src/reitit/frontend.cljs index 922b5a8b..faa258e0 100644 --- a/modules/reitit-frontend/src/reitit/frontend.cljs +++ b/modules/reitit-frontend/src/reitit/frontend.cljs @@ -1,6 +1,5 @@ (ns reitit.frontend (:require [clojure.set :as set] - [clojure.string :as str] [reitit.coercion :as coercion] [reitit.core :as r]) (:import goog.Uri @@ -21,19 +20,6 @@ (map (juxt keyword #(query-param q %))) (into {})))) -(defn fragment-params - "Given goog.Uri, read fragment parameters into Clojure map." - [^Uri uri] - (let [fp (.getFragment uri)] - (if-not (seq fp) - {} - (into {} - (comp - (map #(str/split % #"=")) - (map (fn [[k v]] - [(keyword k) v]))) - (str/split fp #"&"))))) - (defn match-by-path "Given routing tree and current path, return match with possibly coerced parameters. Return nil if no match found. @@ -51,14 +37,17 @@ coercion/coerce!)] (if-let [match (r/match-by-path router (.getPath uri))] (let [q (query-params uri) - fp (fragment-params uri) - match (assoc match :query-params q :fragment-params fp) + fragment (when (.hasFragment uri) + (.getFragment uri)) + match (assoc match + :query-params q + :fragment fragment) ;; Return uncoerced values if coercion is not enabled - so ;; that tha parameters are always accessible from same property. parameters (or (coerce! match) {:path (:path-params match) :query q - :fragment fp})] + :fragment fragment})] (assoc match :parameters parameters)))))) (defn match-by-name diff --git a/test/cljs/reitit/frontend/core_test.cljs b/test/cljs/reitit/frontend/core_test.cljs index 42a45bde..d0684398 100644 --- a/test/cljs/reitit/frontend/core_test.cljs +++ b/test/cljs/reitit/frontend/core_test.cljs @@ -4,12 +4,23 @@ [reitit.frontend :as rf] [reitit.coercion :as rc] [schema.core :as s] - [reitit.coercion.schema :as rsc] + [reitit.coercion.schema :as rcs] + [reitit.coercion.malli :as rcm] [reitit.frontend.test-utils :refer [capture-console]])) (defn m [x] (assoc x :data nil :result nil)) +(defn decode-form [s] + ;; RFC 6749 4.2.2 specifies OAuth token response uses + ;; form-urlencoded format to encode values in the fragment string. + ;; Use built-in JS function to decode. + ;; ring.util.codec/decode-form works on Clj. + (when s + (->> (.entries (js/URLSearchParams. s)) + (map (fn [[k v]] [(keyword k) v])) + (into {})))) + (deftest match-by-path-test (testing "simple" (let [router (r/router ["/" @@ -21,11 +32,11 @@ :data {:name ::frontpage} :path-params {} :query-params {} - :fragment-params {} :path "/" + :fragment nil :parameters {:query {} :path {} - :fragment {}}}) + :fragment nil}}) (rf/match-by-path router "/"))) (is (= "/" @@ -36,11 +47,11 @@ :data {:name ::foo} :path-params {} :query-params {} - :fragment-params {} :path "/foo" + :fragment nil :parameters {:query {} :path {} - :fragment {}}}) + :fragment nil}}) (rf/match-by-path router "/foo"))) (is (= (r/map->Match @@ -48,11 +59,11 @@ :data {:name ::foo} :path-params {} :query-params {:mode ["foo", "bar"]} - :fragment-params {} :path "/foo" + :fragment nil :parameters {:query {:mode ["foo", "bar"]} :path {} - :fragment {}}}) + :fragment nil}}) (rf/match-by-path router "/foo?mode=foo&mode=bar"))) (is (= "/foo" @@ -71,23 +82,19 @@ [":id" {:name ::foo :parameters {:path {:id s/Int} :query {(s/optional-key :mode) s/Keyword} - :fragment {(s/optional-key :access_token) s/Str - (s/optional-key :refresh_token) s/Str - (s/optional-key :expires_in) s/Int - (s/optional-key :provider_token) s/Str - (s/optional-key :token_type) s/Str}}}]] + :fragment (s/maybe s/Str)}}]] {:compile rc/compile-request-coercers - :data {:coercion rsc/coercion}})] + :data {:coercion rcs/coercion}})] (is (= (r/map->Match {:template "/:id" :path-params {:id "5"} :query-params {} - :fragment-params {} :path "/5" + :fragment nil :parameters {:query {} :path {:id 5} - :fragment {}}}) + :fragment nil}}) (m (rf/match-by-path router "/5")))) (is (= "/5" @@ -111,35 +118,27 @@ {:template "/:id" :path-params {:id "5"} :query-params {:mode "foo"} - :fragment-params {} :path "/5" + :fragment nil :parameters {:path {:id 5} :query {:mode :foo} - :fragment {}}}) + :fragment nil}}) (m (rf/match-by-path router "/5?mode=foo")))) (is (= "/5?mode=foo" (r/match->path (rf/match-by-name router ::foo {:id 5}) {:mode :foo})))) - (testing "fragment is read" + (testing "fragment string is read" (is (= (r/map->Match {:template "/:id" :path-params {:id "5"} :query-params {:mode "foo"} - :fragment-params {:access_token "foo" - :refresh_token "bar" - :provider_token "baz" - :token_type "bearer" - :expires_in "3600"} :path "/5" + :fragment "fragment" :parameters {:path {:id 5} :query {:mode :foo} - :fragment {:access_token "foo" - :refresh_token "bar" - :provider_token "baz" - :token_type "bearer" - :expires_in 3600}}}) - (m (rf/match-by-path router "/5?mode=foo#access_token=foo&refresh_token=bar&provider_token=baz&token_type=bearer&expires_in=3600"))))) + :fragment "fragment"}}) + (m (rf/match-by-path router "/5?mode=foo#fragment"))))) (testing "console warning about missing params" (is (= [{:type :warn @@ -151,4 +150,80 @@ (:messages (capture-console (fn [] - (rf/match-by-name! router ::foo {})))))))))) + (rf/match-by-name! router ::foo {}))))))))) + + (testing "malli coercion" + (let [router (r/router ["/" + [":id" {:name ::foo + :parameters {:path [:map + [:id :int]] + :query [:map + [:mode {:optional true} :keyword]] + :fragment [:maybe + [:map + {:decode/string decode-form} + [:access_token :string] + [:refresh_token :string] + [:expires_in :int] + [:provider_token :string] + [:token_type :string]]]}}]] + {:compile rc/compile-request-coercers + :data {:coercion rcm/coercion}})] + + (is (= (r/map->Match + {:template "/:id" + :path-params {:id "5"} + :query-params {} + :path "/5" + :fragment nil + :parameters {:query {} + :path {:id 5} + :fragment nil}}) + (m (rf/match-by-path router "/5")))) + + (is (= "/5" + (r/match->path (rf/match-by-name router ::foo {:id 5})))) + + (testing "coercion error" + (testing "throws without options" + (is (thrown? js/Error (m (rf/match-by-path router "/a"))))) + + (testing "thows and calles on-coercion-error" + (let [exception (atom nil) + match (atom nil)] + (is (thrown? js/Error (m (rf/match-by-path router "/a" {:on-coercion-error (fn [m e] + (reset! match m) + (reset! exception e))})))) + (is (= {:id "a"} (-> @match :path-params))) + (is (= {:id "a"} (-> @exception (ex-data) :value)))))) + + (testing "query param is read" + (is (= (r/map->Match + {:template "/:id" + :path-params {:id "5"} + :query-params {:mode "foo"} + :path "/5" + :fragment nil + :parameters {:path {:id 5} + :query {:mode :foo} + :fragment nil}}) + (m (rf/match-by-path router "/5?mode=foo")))) + + (is (= "/5?mode=foo" + (r/match->path (rf/match-by-name router ::foo {:id 5}) {:mode :foo})))) + + (testing "fragment string is read" + (is (= (r/map->Match + {:template "/:id" + :path-params {:id "5"} + :query-params {:mode "foo"} + :path "/5" + :fragment "access_token=foo&refresh_token=bar&provider_token=baz&token_type=bearer&expires_in=3600" + :parameters {:path {:id 5} + :query {:mode :foo} + :fragment {:access_token "foo" + :refresh_token "bar" + :provider_token "baz" + :token_type "bearer" + :expires_in 3600}}}) + (m (rf/match-by-path router "/5?mode=foo#access_token=foo&refresh_token=bar&provider_token=baz&token_type=bearer&expires_in=3600"))))))))