From 25dd0abcaf1c497e71490d6bb802dc545d1fd1a3 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Tue, 21 Jan 2025 14:40:56 +0200 Subject: [PATCH] Use coercion to encode query-string values in match->path --- modules/reitit-core/src/reitit/coercion.cljc | 3 +- modules/reitit-core/src/reitit/core.cljc | 21 ++++++++- .../src/reitit/coercion/malli.cljc | 4 +- .../src/reitit/coercion/schema.cljc | 5 ++- .../reitit-spec/src/reitit/coercion/spec.cljc | 5 ++- test/cljc/reitit/coercion_test.cljc | 43 +++++++++++++++++-- 6 files changed, 72 insertions(+), 9 deletions(-) diff --git a/modules/reitit-core/src/reitit/coercion.cljc b/modules/reitit-core/src/reitit/coercion.cljc index bc3dc6ca..0891a0ca 100644 --- a/modules/reitit-core/src/reitit/coercion.cljc +++ b/modules/reitit-core/src/reitit/coercion.cljc @@ -19,7 +19,8 @@ (-open-model [this model] "Returns a new model which allows extra keys in maps") (-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")) + (-response-coercer [this model] "Returns a `value format => value` response coercion function") + (-query-string-coercer [this model] "Returns a `value => value` query string coercion function")) #?(:clj (defmethod print-method ::coercion [coercion ^Writer w] diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 95633aec..b1416a01 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -1,7 +1,9 @@ (ns reitit.core (:require [reitit.exception :as exception] [reitit.impl :as impl] - [reitit.trie :as trie])) + [reitit.trie :as trie] + ;; FIXME: Should avoid coercion require here? + [reitit.coercion :as coercion])) ;; ;; Expand @@ -71,7 +73,22 @@ ([match] (match->path match nil)) ([match query-params] - (some-> match :path (cond-> (seq query-params) (str "?" (impl/query-string query-params)))))) + (some-> match :path (cond-> (seq query-params) + ;; TODO: Should the coercion be applied elsewhere (FE ns?) so the core ns doesn't depend + ;; on the coercion? + ;; NOTE: Re-creates coercer on every call, could this be pre-compiled somewhere + ;; or memoized? Does it matter much? + (str "?" (let [schema (-> match :data :parameters :query + ;; FIXME: Why? + first) + coercion (-> match :data :coercion) + coercer (when (and schema coercion) + (coercion/-query-string-coercer coercion schema)) + query-params (or (when coercer + (coercer query-params :default)) + query-params)] + ;; Default encoding for values will handle values that aren't encoded using coercer + (impl/query-string query-params))))))) ;; ;; Different routers diff --git a/modules/reitit-malli/src/reitit/coercion/malli.cljc b/modules/reitit-malli/src/reitit/coercion/malli.cljc index b94b17b3..3416079a 100644 --- a/modules/reitit-malli/src/reitit/coercion/malli.cljc +++ b/modules/reitit-malli/src/reitit/coercion/malli.cljc @@ -176,6 +176,8 @@ (-request-coercer [_ type schema] (-coercer schema type transformers :decode opts)) (-response-coercer [_ schema] - (-coercer schema :response transformers :encode opts)))))) + (-coercer schema :response transformers :encode opts)) + (-query-string-coercer [_ schema] + (-coercer schema :string transformers :encode opts)))))) (def coercion (create default-options)) diff --git a/modules/reitit-schema/src/reitit/coercion/schema.cljc b/modules/reitit-schema/src/reitit/coercion/schema.cljc index b2fd14d1..adbbcf05 100644 --- a/modules/reitit-schema/src/reitit/coercion/schema.cljc +++ b/modules/reitit-schema/src/reitit/coercion/schema.cljc @@ -101,6 +101,9 @@ value)))) (-response-coercer [this schema] (if (coerce-response? schema) - (coercion/-request-coercer this :response schema))))) + (coercion/-request-coercer this :response schema))) + (-query-string-coercer [this schema] + ;; TODO: Can this be implemented? + nil))) (def coercion (create default-options)) diff --git a/modules/reitit-spec/src/reitit/coercion/spec.cljc b/modules/reitit-spec/src/reitit/coercion/spec.cljc index bf4bed82..56c6e6a7 100644 --- a/modules/reitit-spec/src/reitit/coercion/spec.cljc +++ b/modules/reitit-spec/src/reitit/coercion/spec.cljc @@ -148,6 +148,9 @@ value)))) (-response-coercer [this spec] (if (coerce-response? spec) - (coercion/-request-coercer this :response spec))))) + (coercion/-request-coercer this :response spec))) + (-query-string-coercer [this spec] + ;; TODO: Can this be implemented? + nil))) (def coercion (create default-options)) diff --git a/test/cljc/reitit/coercion_test.cljc b/test/cljc/reitit/coercion_test.cljc index ee18c59c..6ab91745 100644 --- a/test/cljc/reitit/coercion_test.cljc +++ b/test/cljc/reitit/coercion_test.cljc @@ -1,5 +1,8 @@ (ns reitit.coercion-test - (:require [clojure.test :refer [deftest is testing]] + (:require [clojure.spec.alpha :as cs] + [clojure.string :as str] + [clojure.test :refer [deftest is testing]] + [malli.core :as m] [malli.experimental.lite :as l] [reitit.coercion :as coercion] [reitit.coercion.malli] @@ -7,8 +10,8 @@ [reitit.coercion.spec] [reitit.core :as r] [schema.core :as s] - [clojure.spec.alpha :as cs] - [spec-tools.data-spec :as ds]) + [spec-tools.data-spec :as ds] + [malli.transform :as mt]) #?(:clj (:import (clojure.lang ExceptionInfo)))) @@ -150,3 +153,37 @@ {:compile coercion/compile-request-coercers})] (is (= {:path {:user-id 123, :company "metosin"}} (:parameters (match-by-path-and-coerce! router "/metosin/users/123")))))) + +(deftest match->path-parameter-coercion-test + (testing "default handling for query-string collection" + (let [router (r/router ["/:a/:b" ::route])] + (is (= "/olipa/kerran?x=a&x=b" + (-> router + (r/match-by-name! ::route {:a "olipa", :b "kerran"}) + (r/match->path {:x [:a :b]})))))) + + (testing "custom encode/string for a collection" + (let [router (r/router ["/:a/:b" + {:name ::route + :coercion reitit.coercion.malli/coercion + :parameters {:query [:map + [:x + [:vector + {:encode/string (fn [xs] + (str/join "," (map name xs))) + :decode/string (fn [s] + (if (string? s) + (mapv keyword (str/split s #",")) + s))} + :keyword]]]}}] + {:compile coercion/compile-request-coercers})] + ;; NOTE: "," is urlencoded by the impl/query-string step, is that ok? + (is (= "/olipa/kerran?x=a%2Cb" + (-> router + (r/match-by-name! ::route {:a "olipa", :b "kerran"}) + (r/match->path {:x [:a :b]})))) + + (is (= {:query {:x [:a :b]}} + (-> (r/match-by-path router "/olipa/kerran") + (assoc :query-params {:x "a,b"}) + (coercion/coerce!)))))))