Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make exception middleware return proper Ring responses #712

Merged
merged 2 commits into from
Dec 8, 2024
Merged

Make exception middleware return proper Ring responses #712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc1cd11
Make exception middleware return proper Ring responses
p-himik Dec 7, 2024
1abff49
Update exception_test.clj
ikitommi Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions modules/reitit-middleware/src/reitit/ring/middleware/exception.clj
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading