From dd835e73a89a02fad84853ebf31bc270afe96692 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Fri, 11 Apr 2025 08:27:44 +0300 Subject: [PATCH 1/4] feat: allow :default response status code again it is an old feature, but didn't have a test, so it was broken by #715 also add a test so we don't break it again --- modules/reitit-core/src/reitit/coercion.cljc | 4 +- test/cljc/reitit/openapi_test.clj | 46 ++++++++-------- test/cljc/reitit/ring_coercion_test.cljc | 56 ++++++++++++++++++++ 3 files changed, 81 insertions(+), 25 deletions(-) diff --git a/modules/reitit-core/src/reitit/coercion.cljc b/modules/reitit-core/src/reitit/coercion.cljc index ad96102b..4266fea9 100644 --- a/modules/reitit-core/src/reitit/coercion.cljc +++ b/modules/reitit-core/src/reitit/coercion.cljc @@ -184,8 +184,8 @@ (defn response-coercers [coercion responses opts] (some->> (for [[status model] responses] (do - (when-not (int? status) - (throw (ex-info "Response status must be int" {:status status}))) + (when-not (or (= :default status) (int? status)) + (throw (ex-info "Response status must be int or :default" {:status status}))) [status (response-coercer coercion model opts)])) (filter second) (seq) (into {}))) diff --git a/test/cljc/reitit/openapi_test.clj b/test/cljc/reitit/openapi_test.clj index c474fbf3..3e0f85b4 100644 --- a/test/cljc/reitit/openapi_test.clj +++ b/test/cljc/reitit/openapi_test.clj @@ -67,9 +67,9 @@ :responses {200 {:description "success" :body {:total int?}} 500 {:description "fail"} - 504 {:description "default" - :content {:default {:schema {:error string?}}} - :body {:masked string?}}} + :default {:description "default" + :content {:default {:schema {:error string?}}} + :body {:masked string?}}} :handler (fn [{{{:keys [z]} :path xs :body} :parameters}] {:status 200, :body {:total (+ (reduce + xs) z)}})}}]] @@ -96,9 +96,9 @@ :responses {200 {:description "success" :body [:map [:total int?]]} 500 {:description "fail"} - 504 {:description "default" - :content {:default {:schema {:error string?}}} - :body {:masked string?}}} + :default {:description "default" + :content {:default {:schema {:error string?}}} + :body {:masked string?}}} :handler (fn [{{{:keys [z]} :path xs :body} :parameters}] {:status 200, :body {:total (+ (reduce + xs) z)}})}}]] @@ -125,9 +125,9 @@ :responses {200 {:description "success" :body {:total s/Int}} 500 {:description "fail"} - 504 {:description "default" - :content {:default {:schema {:error s/Str}}} - :body {:masked s/Str}}} + :default {:description "default" + :content {:default {:schema {:error s/Str}}} + :body {:masked s/Str}}} :handler (fn [{{{:keys [z]} :path xs :body} :parameters}] {:status 200, :body {:total (+ (reduce + xs) z)}})}}]]] @@ -200,10 +200,10 @@ 400 {:content {"application/json" {:schema {:type "string"}}} :description "kosh"} 500 {:description "fail"} - 504 {:description "default" - :content {"application/json" {:schema {:properties {"error" {:type "string"}} - :required ["error"] - :type "object"}}}}} + :default {:description "default" + :content {"application/json" {:schema {:properties {"error" {:type "string"}} + :required ["error"] + :type "object"}}}}} :summary "plus with body"}} "/api/malli/plus/{z}" {:get {:parameters [{:in "query" :name :x @@ -242,11 +242,11 @@ 400 {:description "kosh" :content {"application/json" {:schema {:type "string"}}}} 500 {:description "fail"} - 504 {:description "default" - :content {"application/json" {:schema {:additionalProperties false - :properties {:error {:type "string"}} - :required [:error] - :type "object"}}}}} + :default {:description "default" + :content {"application/json" {:schema {:additionalProperties false + :properties {:error {:type "string"}} + :required [:error] + :type "object"}}}}} :summary "plus with body"}} "/api/schema/plus/{z}" {:get {:parameters [{:in "query" :name "x" @@ -292,11 +292,11 @@ 400 {:description "kosh" :content {"application/json" {:schema {:type "string"}}}} 500 {:description "fail"} - 504 {:description "default" - :content {"application/json" {:schema {:additionalProperties false - :properties {"error" {:type "string"}} - :required ["error"] - :type "object"}}}}} + :default {:description "default" + :content {"application/json" {:schema {:additionalProperties false + :properties {"error" {:type "string"}} + :required ["error"] + :type "object"}}}}} :summary "plus with body"}}}}] (is (= expected spec)) (is (= nil (validate spec)))))) diff --git a/test/cljc/reitit/ring_coercion_test.cljc b/test/cljc/reitit/ring_coercion_test.cljc index 91fa5135..3c93cae9 100644 --- a/test/cljc/reitit/ring_coercion_test.cljc +++ b/test/cljc/reitit/ring_coercion_test.cljc @@ -680,6 +680,62 @@ (call (request "application/json" "application/transit" {:request :json :response :json})))))))))))) +#?(:clj + (deftest response-coercion-test + (doseq [[coercion schema-200 schema-default] + [[malli/coercion + [:map [:a :int]] + [:map [:b :int]]] + [schema/coercion + {:a s/Int} + {:b s/Int}] + [spec/coercion + {:a int?} + {:b int?}]]] + (testing (str coercion) + (let [app (ring/ring-handler + (ring/router + ["/foo" {:post {:responses {200 {:content {:default {:schema schema-200}}} + :default {:content {"application/json" {:schema schema-default}}}} + :handler (fn [req] + {:status (-> req :body-params :status) + :body (-> req :body-params :response)})}}] + {:validate reitit.ring.spec/validate + :data {:middleware [rrc/coerce-request-middleware + rrc/coerce-response-middleware] + :coercion coercion}})) + call (fn [request] + (try + (app request) + (catch ExceptionInfo e + (select-keys (ex-data e) [:type :in])))) + request (fn [body] + {:request-method :post + :uri "/foo" + :muuntaja/request {:format "application/json"} + :muuntaja/response {:format (:format body "application/json")} + :body-params body})] + (testing "explicit response schema" + (is (= {:status 200 :body {:a 1}} + (call (request {:status 200 :response {:a 1}}))) + "valid response") + (is (= {:type :reitit.coercion/response-coercion, :in [:response :body]} + (call (request {:status 200 :response {:b 1}}))) + "invalid response") + (is (= {:type :reitit.coercion/response-coercion, :in [:response :body]} + (call (request {:status 200 :response {:b 1} :format "application/edn"}))) + "invalid response, different content-type")) + (testing "default response schema" + (is (= {:status 300 :body {:b 2}} + (call (request {:status 300 :response {:b 2}}))) + "valid response") + (is (= {:type :reitit.coercion/response-coercion, :in [:response :body]} + (call (request {:status 300 :response {:a 2}}))) + "invalid response") + (is (= {:status 300 :body "anything goes!"} + (call (request {:status 300 :response "anything goes!" :format "application/edn"}))) + "no coercion applied due to content-type"))))))) + #?(:clj (deftest muuntaja-test (let [app (ring/ring-handler From 8058cecae03573dd080580762ab980af465860b2 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Fri, 11 Apr 2025 11:45:12 +0300 Subject: [PATCH 2/4] doc: document :responses :default --- doc/ring/coercion.md | 5 +++-- examples/openapi/src/example/server.clj | 14 ++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/doc/ring/coercion.md b/doc/ring/coercion.md index 342f1020..ca908bfc 100644 --- a/doc/ring/coercion.md +++ b/doc/ring/coercion.md @@ -37,7 +37,7 @@ Coercion can be attached to route data under `:coercion` key. There can be multi Parameters are defined in route data under `:parameters` key. It's value should be a map of parameter `:type` -> Coercion Schema. -Responses are defined in route data under `:responses` key. It's value should be a map of http status code to a map which can contain `:body` key with Coercion Schema as value. +Responses are defined in route data under `:responses` key. It's value should be a map of http status code to a map which can contain `:body` key with Coercion Schema as value. Additionally, the key `:default` specifies the coercion for other status codes. Below is an example with [Plumatic Schema](https://github.com/plumatic/schema). It defines schemas for `:query`, `:body` and `:path` parameters and for http 200 response `:body`. @@ -54,7 +54,8 @@ Handlers can access the coerced parameters via the `:parameters` key in the requ :parameters {:query {:x s/Int} :body {:y s/Int} :path {:z s/Int}} - :responses {200 {:body {:total PositiveInt}}} + :responses {200 {:body {:total PositiveInt}} + :default {:body {:error s/Str}}} :handler (fn [{:keys [parameters]}] (let [total (+ (-> parameters :query :x) (-> parameters :body :y) diff --git a/examples/openapi/src/example/server.clj b/examples/openapi/src/example/server.clj index 83e3a6b0..9dcb4c06 100644 --- a/examples/openapi/src/example/server.clj +++ b/examples/openapi/src/example/server.clj @@ -69,7 +69,7 @@ {:status 200 :body {:color :red :pineapple true}})} - :post {:summary "Create a pizza | Multiple content-types, multiple examples" + :post {:summary "Create a pizza | Multiple content-types, multiple examples | Default response schema" :request {:description "Create a pizza using json or EDN" :content {"application/json" {:schema [:map [:color :keyword] @@ -83,10 +83,16 @@ :pineapple false})}}}}} :responses {200 {:description "Success" :content {:default {:schema [:map [:success :boolean]] - :example {:success true}}}}} + :example {:success true}}}} + :default {:description "Not success" + :content {:default {:schema [:map [:error :string]] + :example {:error "error"}}}}} :handler (fn [_request] - {:status 200 - :body {:success true}})}}] + (if (< (Math/random) 0.5) + {:status 200 + :body {:success true}} + {:status 500 + :body {:error "an error happened"}}))}}] ["/contact" From f038fe1941e8c2a2ee5e9768bcc3bc2a56d09adf Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Fri, 11 Apr 2025 12:40:27 +0300 Subject: [PATCH 3/4] test: corner cases in how :responses :default gets applied --- test/cljc/reitit/ring_coercion_test.cljc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/cljc/reitit/ring_coercion_test.cljc b/test/cljc/reitit/ring_coercion_test.cljc index 3c93cae9..c4538968 100644 --- a/test/cljc/reitit/ring_coercion_test.cljc +++ b/test/cljc/reitit/ring_coercion_test.cljc @@ -696,6 +696,9 @@ (let [app (ring/ring-handler (ring/router ["/foo" {:post {:responses {200 {:content {:default {:schema schema-200}}} + 201 {:content {"application/edn" {:schema schema-200}}} + 202 {:description "status code and content-type explicitly mentioned, but no :schema" + :content {"application/edn" {} "application/json" {}}} :default {:content {"application/json" {:schema schema-default}}}} :handler (fn [req] {:status (-> req :body-params :status) @@ -725,6 +728,19 @@ (is (= {:type :reitit.coercion/response-coercion, :in [:response :body]} (call (request {:status 200 :response {:b 1} :format "application/edn"}))) "invalid response, different content-type")) + (testing "explicit response schema, but for the wrong content-type" + ;; TODO: we might want to rethink this behaviour! + (is (= {:status 201 :body "anything goes!"} + (call (request {:status 201 :response "anything goes!"}))) + "no coercion applied")) + (testing "response config without :schema - default applies" + ;; TODO: we might want to rethink this behaviour! + (is (= {:status 202 :body {:b 1}} + (call (request {:status 202 :response {:b 1}}))) + "valid response") + (is (= {:type :reitit.coercion/response-coercion, :in [:response :body]} + (call (request {:status 202 :response {:a 1}}))) + "invalid response")) (testing "default response schema" (is (= {:status 300 :body {:b 2}} (call (request {:status 300 :response {:b 2}}))) From a8b4bc0d2ded8e331a12a296096a8cea52f9ca11 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Fri, 25 Apr 2025 10:59:10 +0300 Subject: [PATCH 4/4] feat: rework & document response coercer defaulting rules --- doc/ring/coercion.md | 8 +++ doc/ring/compiling_middleware.md | 14 ++-- modules/reitit-core/src/reitit/coercion.cljc | 72 +++++++++---------- .../reitit-http/src/reitit/http/coercion.cljc | 4 +- .../reitit-ring/src/reitit/ring/coercion.cljc | 6 +- test/cljc/reitit/ring_coercion_test.cljc | 16 ++--- 6 files changed, 62 insertions(+), 58 deletions(-) diff --git a/doc/ring/coercion.md b/doc/ring/coercion.md index ca908bfc..c450b43c 100644 --- a/doc/ring/coercion.md +++ b/doc/ring/coercion.md @@ -207,6 +207,14 @@ is: rrc/coerce-response-middleware]}}))) ``` +The resolution logic for response coercers is: +1. Get the response status, or `:default` from the `:responses` map +2. From this map, get use the first of these to coerce: + 1. `:content :schema` + 2. `:content :default :schema` + 3. `:body` +3. If nothing was found, do not coerce + ## Pretty printing spec errors Spec problems are exposed as is in request & response coercion errors. Pretty-printers like [expound](https://github.com/bhb/expound) can be enabled like this: diff --git a/doc/ring/compiling_middleware.md b/doc/ring/compiling_middleware.md index f9404cc9..270d231f 100644 --- a/doc/ring/compiling_middleware.md +++ b/doc/ring/compiling_middleware.md @@ -27,8 +27,8 @@ To demonstrate the two approaches, below is the response coercion middleware wri coercion (-> match :data :coercion) opts (-> match :data :opts)] (if (and coercion responses) - (let [coercers (response-coercers coercion responses opts)] - (coerce-response coercers request response)) + (let [coercer (response-coercer coercion responses opts)] + (coercer request response)) response))) ([request respond raise] (let [method (:request-method request) @@ -37,8 +37,8 @@ To demonstrate the two approaches, below is the response coercion middleware wri 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 %)))) + (let [coercer (response-coercer coercion responses opts)] + (handler request #(respond (coercer request %)))) (handler request respond raise)))))) ``` @@ -60,13 +60,13 @@ To demonstrate the two approaches, below is the response coercion middleware wri :spec ::rs/responses :compile (fn [{:keys [coercion responses]} opts] (if (and coercion responses) - (let [coercers (coercion/response-coercers coercion responses opts)] + (let [coercer (coercion/response-coercer coercion responses opts)] (fn [handler] (fn ([request] - (coercion/coerce-response coercers request (handler request))) + (coercer request (handler request))) ([request respond raise] - (handler request #(respond (coercion/coerce-response coercers request %)) raise)))))))}) + (handler request #(respond (coercer request %)) raise)))))))}) ``` It has 50% less code, it's much easier to reason about and is much faster. diff --git a/modules/reitit-core/src/reitit/coercion.cljc b/modules/reitit-core/src/reitit/coercion.cljc index 4266fea9..7a3d4458 100644 --- a/modules/reitit-core/src/reitit/coercion.cljc +++ b/modules/reitit-core/src/reitit/coercion.cljc @@ -130,29 +130,6 @@ (request-coercion-failed! result coercion value in request serialize-failed-result) result))))))) -(defn extract-response-format-default [request _] - (-> request :muuntaja/response :format)) - -(defn response-coercer [coercion {:keys [content body]} {:keys [extract-response-format serialize-failed-result] - :or {extract-response-format extract-response-format-default}}] - (if coercion - (let [format->coercer (some->> (concat (when body - [[:default (-response-coercer coercion body)]]) - (for [[format {:keys [schema]}] content, :when schema] - [format (-response-coercer coercion schema)])) - (filter second) (seq) (into (array-map)))] - (when format->coercer - (fn [request response] - (let [format (extract-response-format request response) - value (:body response) - coercer (or (format->coercer format) - (format->coercer :default) - -identity-coercer) - result (coercer value format)] - (if (error? result) - (response-coercion-failed! result coercion value request response serialize-failed-result) - result))))))) - (defn encode-error [data] (-> data (dissoc :request :response) @@ -165,12 +142,6 @@ (impl/fast-assoc acc k (coercer request))) {} coercers)) -(defn coerce-response [coercers request response] - (if response - (if-let [coercer (or (coercers (:status response)) (coercers :default))] - (impl/fast-assoc response :body (coercer request response)) - response))) - (defn request-coercers ([coercion parameters opts] (some->> (for [[k v] parameters, :when v] @@ -181,13 +152,42 @@ rcs (request-coercers coercion parameters (cond-> opts route-request (assoc ::skip #{:body})))] (if (and crc rcs) (into crc (vec rcs)) (or crc rcs))))) -(defn response-coercers [coercion responses opts] - (some->> (for [[status model] responses] - (do - (when-not (or (= :default status) (int? status)) - (throw (ex-info "Response status must be int or :default" {:status status}))) - [status (response-coercer coercion model opts)])) - (filter second) (seq) (into {}))) +(defn extract-response-format-default [request _] + (-> request :muuntaja/response :format)) + +(defn -format->coercer [coercion {:keys [content body]} _opts] + (->> (concat (when body + [[:default (-response-coercer coercion body)]]) + (for [[format {:keys [schema]}] content, :when schema] + [format (-response-coercer coercion schema)])) + (filter second) (into (array-map)))) + +(defn response-coercer [coercion responses {:keys [extract-response-format serialize-failed-result] + :or {extract-response-format extract-response-format-default} + :as opts}] + (when coercion + (let [status->format->coercer + (into {} + (for [[status model] responses] + (do + (when-not (or (= :default status) (int? status)) + (throw (ex-info "Response status must be int or :default" {:status status}))) + [status (-format->coercer coercion model opts)])))] + (when-not (every? empty? (vals status->format->coercer)) ;; fast path: return nil if there are no models to coerce + (fn [request response] + (let [format->coercer (or (status->format->coercer (:status response)) + (status->format->coercer :default)) + format (extract-response-format request response) + coercer (or (format->coercer format) + (format->coercer :default))] + (if-not coercer + response + (let [value (:body response) + coerced (coercer (:body response) format) + result (if (error? coerced) + (response-coercion-failed! coerced coercion value request response serialize-failed-result) + coerced)] + (impl/fast-assoc response :body result))))))))) (defn -compile-parameters [data coercion] (impl/path-update data [[[:parameters any?] #(-compile-model coercion % nil)]])) diff --git a/modules/reitit-http/src/reitit/http/coercion.cljc b/modules/reitit-http/src/reitit/http/coercion.cljc index 4807f3a3..c8165979 100644 --- a/modules/reitit-http/src/reitit/http/coercion.cljc +++ b/modules/reitit-http/src/reitit/http/coercion.cljc @@ -41,11 +41,11 @@ (not responses) {} ;; mount :else - (if-let [coercers (coercion/response-coercers coercion responses opts)] + (if-let [coercer (coercion/response-coercer coercion responses opts)] {:leave (fn [ctx] (let [request (:request ctx) response (:response ctx) - response (coercion/coerce-response coercers request response)] + response (coercer request response)] (assoc ctx :response response)))} {})))}) diff --git a/modules/reitit-ring/src/reitit/ring/coercion.cljc b/modules/reitit-ring/src/reitit/ring/coercion.cljc index c870c180..3223432c 100644 --- a/modules/reitit-ring/src/reitit/ring/coercion.cljc +++ b/modules/reitit-ring/src/reitit/ring/coercion.cljc @@ -58,13 +58,13 @@ (not responses) {} ;; mount :else - (if-let [coercers (coercion/response-coercers coercion responses opts)] + (if-let [coercer (coercion/response-coercer coercion responses opts)] (fn [handler] (fn ([request] - (coercion/coerce-response coercers request (handler request))) + (coercer request (handler request))) ([request respond raise] - (handler request #(respond (coercion/coerce-response coercers request %)) raise)))) + (handler request #(respond (coercer request %)) raise)))) {})))}) (def coerce-exceptions-middleware diff --git a/test/cljc/reitit/ring_coercion_test.cljc b/test/cljc/reitit/ring_coercion_test.cljc index c4538968..cf8364a8 100644 --- a/test/cljc/reitit/ring_coercion_test.cljc +++ b/test/cljc/reitit/ring_coercion_test.cljc @@ -698,7 +698,8 @@ ["/foo" {:post {:responses {200 {:content {:default {:schema schema-200}}} 201 {:content {"application/edn" {:schema schema-200}}} 202 {:description "status code and content-type explicitly mentioned, but no :schema" - :content {"application/edn" {} "application/json" {}}} + :content {"application/edn" {} + "application/json" {}}} :default {:content {"application/json" {:schema schema-default}}}} :handler (fn [req] {:status (-> req :body-params :status) @@ -729,18 +730,13 @@ (call (request {:status 200 :response {:b 1} :format "application/edn"}))) "invalid response, different content-type")) (testing "explicit response schema, but for the wrong content-type" - ;; TODO: we might want to rethink this behaviour! (is (= {:status 201 :body "anything goes!"} (call (request {:status 201 :response "anything goes!"}))) "no coercion applied")) - (testing "response config without :schema - default applies" - ;; TODO: we might want to rethink this behaviour! - (is (= {:status 202 :body {:b 1}} - (call (request {:status 202 :response {:b 1}}))) - "valid response") - (is (= {:type :reitit.coercion/response-coercion, :in [:response :body]} - (call (request {:status 202 :response {:a 1}}))) - "invalid response")) + (testing "response config without :schema" + (is (= {:status 202 :body "anything goes!"} + (call (request {:status 202 :response "anything goes!"}))) + "no coercion applied")) (testing "default response schema" (is (= {:status 300 :body {:b 2}} (call (request {:status 300 :response {:b 2}})))