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