Skip to content

Commit

Permalink
Merge pull request #712 from p-himik/p-himik/708-incorrect-ring-respo…
Browse files Browse the repository at this point in the history 
…nses

Make exception middleware return proper Ring responses
  • Loading branch information
@ikitommi
ikitommi authored Dec 8, 2024
2 parents e866625 + 1abff49 commit ada41ec
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 47 deletions.
2 changes: 2 additions & 0 deletions modules/reitit-middleware/src/reitit/ring/middleware/exception.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"Default safe handler for any exception."
[^Exception e _]
{:status 500
:headers {}
:body {:type "exception"
:class (.getName (.getClass e))}})

Expand All @@ -77,6 +78,7 @@
[status]
(fn [e _]
{:status status
:headers {}
:body (coercion/encode-error (ex-data e))}))

(defn http-response-handler
Expand Down
1 change: 1 addition & 0 deletions modules/reitit-ring/src/reitit/ring/coercion.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
nil)]
(respond
{:status status
:headers {}
:body (coercion/encode-error data)})
(raise e))))

Expand Down
116 changes: 70 additions & 46 deletions test/clj/reitit/ring/middleware/exception_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
{:data {:middleware [(exception/create-exception-middleware
(merge
exception/default-handlers
{::kikka (constantly {:status 400, :body "kikka"})
SQLException (constantly {:status 400, :body "sql"})
{::kikka (constantly (http-response/bad-request "kikka"))
SQLException (constantly (http-response/bad-request "sql"))
::exception/wrap wrap}))]}}))))]

(testing "normal calls work ok"
Expand All @@ -44,19 +44,21 @@
(is (= response (app {:request-method :get, :uri "/defaults"})))))

(testing "unknown exception"
(let [app (create (fn [_] (throw (NullPointerException.))))]
(is (= {:status 500
:body {:type "exception"
:class "java.lang.NullPointerException"}}
(app {:request-method :get, :uri "/defaults"}))))
(let [app (create (fn [_] (throw (ex-info "fail" {:type ::invalid}))))]
(is (= {:status 500
:body {:type "exception"
:class "clojure.lang.ExceptionInfo"}}
(app {:request-method :get, :uri "/defaults"})))))
(let [app (create (fn [_] (throw (NullPointerException.))))
{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 500))
(is (= body {:type "exception"
:class "java.lang.NullPointerException"})))
(let [app (create (fn [_] (throw (ex-info "fail" {:type ::invalid}))))
{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 500))
(is (= body {:type "exception"
:class "clojure.lang.ExceptionInfo"}))))

(testing "::ring/response"
(let [response {:status 200, :body "ok"}
(let [response (http-response/ok "ok")
app (create (fn [_] (throw (ex-info "fail" {:type ::ring/response, :response response}))))]
(is (= response (app {:request-method :get, :uri "/defaults"})))))

Expand All @@ -75,57 +77,74 @@

(testing "::coercion/request-coercion"
(let [app (create (fn [{{{:keys [x y]} :query} :parameters}]
{:status 200, :body {:total (+ x y)}}))]
(http-response/ok {:total (+ x y)})))]

(let [{:keys [status body]} (app {:request-method :get
:uri "/coercion"
:query-params {"x" "1", "y" "2"}})]
(let [{:keys [status body] :as resp} (app {:request-method :get
:uri "/coercion"
:query-params {"x" "1", "y" "2"}})]
(is (http-response/response? resp))
(is (= 200 status))
(is (= {:total 3} body)))

(let [{:keys [status body]} (app {:request-method :get
:uri "/coercion"
:query-params {"x" "abba", "y" "2"}})]
(let [{:keys [status body] :as resp} (app {:request-method :get
:uri "/coercion"
:query-params {"x" "abba", "y" "2"}})]
(is (http-response/response? resp))
(is (= 400 status))
(is (= :reitit.coercion/request-coercion (:type body))))

(let [{:keys [status body]} (app {:request-method :get
:uri "/coercion"
:query-params {"x" "-10", "y" "2"}})]
(let [{:keys [status body] :as resp} (app {:request-method :get
:uri "/coercion"
:query-params {"x" "-10", "y" "2"}})]
(is (http-response/response? resp))
(is (= 500 status))
(is (= :reitit.coercion/response-coercion (:type body)))))))

(testing "exact :type"
(let [app (create (fn [_] (throw (ex-info "fail" {:type ::kikka}))))]
(is (= {:status 400, :body "kikka"}
(app {:request-method :get, :uri "/defaults"})))))
(let [app (create (fn [_] (throw (ex-info "fail" {:type ::kikka}))))
{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 400))
(is (= body "kikka"))))

(testing "parent :type"
(let [app (create (fn [_] (throw (ex-info "fail" {:type ::kukka}))))]
(is (= {:status 400, :body "kikka"}
(app {:request-method :get, :uri "/defaults"})))))
(let [app (create (fn [_] (throw (ex-info "fail" {:type ::kukka}))))
{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 400))
(is (= body "kikka"))))

(testing "exact Exception"
(let [app (create (fn [_] (throw (SQLException.))))]
(is (= {:status 400, :body "sql"}
(app {:request-method :get, :uri "/defaults"})))))
(let [app (create (fn [_] (throw (SQLException.))))
{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 400))
(is (= body "sql"))))

(testing "Exception SuperClass"
(let [app (create (fn [_] (throw (SQLWarning.))))]
(is (= {:status 400, :body "sql"}
(app {:request-method :get, :uri "/defaults"})))))
(let [app (create (fn [_] (throw (SQLWarning.))))
{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 400))
(is (= body "sql"))))

(testing "::exception/wrap"
(let [calls (atom 0)
app (create (fn [_] (throw (SQLWarning.)))
(fn [handler exception request]
(if (< (swap! calls inc) 2)
(handler exception request)
{:status 500, :body "too many tries"})))]
(is (= {:status 400, :body "sql"}
(app {:request-method :get, :uri "/defaults"})))
(is (= {:status 500, :body "too many tries"}
(app {:request-method :get, :uri "/defaults"})))))))
{:status 500
:headers {}
:body "too many tries"})))]
(let [{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 400))
(is (= body "sql")))
(let [{:keys [status body] :as resp} (app {:request-method :get, :uri "/defaults"})]
(is (http-response/response? resp))
(is (= status 500))
(is (= body "too many tries")))))))

(deftest spec-coercion-exception-test
(let [app (ring/ring-handler
Expand All @@ -135,22 +154,26 @@
{:parameters {:query {:x int?, :y int?}}
:responses {200 {:body {:total pos-int?}}}
:handler (fn [{{{:keys [x y]} :query} :parameters}]
{:status 200, :body {:total (+ x y)}})}}]
(http-response/ok {:total (+ x y)}))}}]
{:data {:coercion reitit.coercion.spec/coercion
:middleware [(exception/create-exception-middleware
(merge
exception/default-handlers
{::coercion/request-coercion (fn [e _] {:status 400, :body (ex-data e)})
::coercion/response-coercion (fn [e _] {:status 500, :body (ex-data e)})}))
{::coercion/request-coercion (fn [e _] (http-response/bad-request (ex-data e)) )
::coercion/response-coercion (fn [e _] {:status 500
:headers {}
:body (ex-data e)})}))
reitit.ring.coercion/coerce-request-middleware
reitit.ring.coercion/coerce-response-middleware]}}))]
(testing "success"
(let [{:keys [status body]} (app {:uri "/plus", :request-method :get, :query-params {"x" "1", "y" "2"}})]
(let [{:keys [status body] :as resp} (app {:uri "/plus", :request-method :get, :query-params {"x" "1", "y" "2"}})]
(is (http-response/response? resp))
(is (= 200 status))
(is (= body {:total 3}))))

(testing "request error"
(let [{:keys [status body]} (app {:uri "/plus", :request-method :get, :query-params {"x" "1", "y" "fail"}})]
(let [{:keys [status body] :as resp} (app {:uri "/plus", :request-method :get, :query-params {"x" "1", "y" "fail"}})]
(is (http-response/response? resp))
(is (= 400 status))
(testing "spec error is exposed as is"
(let [problems (:problems body)]
Expand All @@ -159,7 +182,8 @@
(is (contains? problems ::s/problems))))))

(testing "response error"
(let [{:keys [status body]} (app {:uri "/plus", :request-method :get, :query-params {"x" "1", "y" "-2"}})]
(let [{:keys [status body] :as resp} (app {:uri "/plus", :request-method :get, :query-params {"x" "1", "y" "-2"}})]
(is (http-response/response? resp))
(is (= 500 status))
(testing "spec error is exposed as is"
(let [problems (:problems body)]
Expand Down
2 changes: 1 addition & 1 deletion test/cljc/reitit/ring_coercion_test.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@

(testing "encoding errors"
(let [app (->app {:encode-error (fn [error] {:errors (:humanized error)})})]
(is (= {:status 400, :body {:errors {:x ["missing required key"]}}}
(is (= {:status 400, :headers {}, :body {:errors {:x ["missing required key"]}}}
(app (assoc (->request "closed") :body-params {}))))))

(testing "when schemas are not closed and extra keys are not stripped"
Expand Down

0 comments on commit ada41ec

Please sign in to comment.