From d1d41d6587cd4d21b56fa12a87308f60f1b89cf1 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Mon, 16 Jan 2023 21:50:08 +0700
Subject: [PATCH] Fix returns 500 when exception is thrown from handle-page
middleware (#27526)
* fix throwing exceptions in handle-paging mw
---
src/metabase/server/handler.clj | 5 +-
.../server/middleware/offset_paging_test.clj | 74 +++++++++++--------
2 files changed, 45 insertions(+), 34 deletions(-)
diff --git a/src/metabase/server/handler.clj b/src/metabase/server/handler.clj
index f0605aca6a7..f03c0ffbf1f 100644
--- a/src/metabase/server/handler.clj
+++ b/src/metabase/server/handler.clj
@@ -50,8 +50,8 @@
#'mw.browser-cookie/ensure-browser-id-cookie ; add cookie to identify browser; add `:browser-id` to the request
#'mw.security/add-security-headers ; Add HTTP headers to API responses to prevent them from being cached
#'mw.json/wrap-json-body ; extracts json POST body and makes it avaliable on request
- #'mw.json/wrap-streamed-json-response ; middleware to automatically serialize suitable objects as JSON in responses
#'mw.offset-paging/handle-paging ; binds per-request parameters to handle paging
+ #'mw.json/wrap-streamed-json-response ; middleware to automatically serialize suitable objects as JSON in responses
#'wrap-keyword-params ; converts string keys in :params to keyword keys
#'wrap-params ; parses GET and POST params as :query-params/:form-params and both as :params
#'mw.misc/maybe-set-site-url ; set the value of `site-url` if it hasn't been set yet
@@ -68,7 +68,8 @@
#'mw.ssl/redirect-to-https-middleware])
;; ▲▲▲ PRE-PROCESSING ▲▲▲ happens from BOTTOM-TO-TOP
-(defn- apply-middleware [handler]
+(defn- apply-middleware
+ [handler]
(reduce
(fn [handler middleware-fn]
(middleware-fn handler))
diff --git a/test/metabase/server/middleware/offset_paging_test.clj b/test/metabase/server/middleware/offset_paging_test.clj
index 44b38d87bc2..81d9804694b 100644
--- a/test/metabase/server/middleware/offset_paging_test.clj
+++ b/test/metabase/server/middleware/offset_paging_test.clj
@@ -1,45 +1,55 @@
(ns metabase.server.middleware.offset-paging-test
(:require
+ [cheshire.core :as json]
+ [clojure.java.io :as io]
[clojure.test :refer :all]
+ [metabase.server.handler :as handler]
[metabase.server.middleware.offset-paging :as mw.offset-paging]
- [ring.middleware.keyword-params :refer [wrap-keyword-params]]
- [ring.middleware.params :refer [wrap-params]]
+ [metabase.server.middleware.security :as mw.security]
[ring.mock.request :as ring.mock]
- [ring.util.response :as response]
- [schema.core :as s]))
+ [ring.util.response :as response])
+ (:import
+ (java.io PipedInputStream)))
(defn- handler [request]
- (let [handler* (-> (fn [request respond _]
- (respond (response/response {:limit mw.offset-paging/*limit*
- :offset mw.offset-paging/*offset*
- :paged? mw.offset-paging/*paged?*
- :params (:params request)})))
- mw.offset-paging/handle-paging
- wrap-keyword-params
- wrap-params)
- respond identity
- raise (fn [e] (throw e))]
+ (let [handler (fn [request respond _]
+ (respond (response/response {:limit mw.offset-paging/*limit*
+ :offset mw.offset-paging/*offset*
+ :paged? mw.offset-paging/*paged?*
+ :params (:params request)})))
+ handler* (#'handler/apply-middleware handler)
+ respond identity
+ raise (fn [e] (throw e))]
(handler* request respond raise)))
+(defn- read-response
+ "Responses from our hanlders are inputstream, this is read the stream into real body."
+ [response]
+ (update response :body
+ (fn [body]
+ (if (instance? PipedInputStream body)
+ (with-open [r (io/reader body)]
+ (json/parse-stream r))
+ body))))
+
(deftest paging-test
(testing "no paging params"
- (is (= {:status 200
- :headers {}
- :body {:limit nil
- :offset nil
- :paged? false
- :params {}}}
- (handler (ring.mock/request :get "/")))))
+ (is (=? {:status 200
+ :body {"limit" nil
+ "offset" nil
+ "paged?" false
+ "params" {}}}
+ (read-response (handler (ring.mock/request :get "/"))))))
(testing "w/ paging params"
- (is (= {:status 200
- :headers {}
- :body {:limit 100
- :offset 200
- :paged? true
- :params {:whatever "true"}}}
- (handler (ring.mock/request :get "/" {:offset "200", :limit "100", :whatever "true"})))))
+ (is (=? {:status 200
+ :body {"limit" 100
+ "offset" 200
+ "paged?" true
+ "params" {"whatever" "true"}}}
+ (read-response (handler (ring.mock/request :get "/" {:offset "200", :limit "100", :whatever "true"}))))))
+
(testing "invalid params"
- (is (schema= {:status (s/eq 400)
- :headers s/Any
- :body {:message #"Error parsing paging parameters.*" s/Keyword s/Any}}
- (handler (ring.mock/request :get "/" {:offset "abc", :limit "100"}))))))
+ (is (=? {:status 400
+ :headers (mw.security/security-headers)
+ :body {"message" #"Error parsing paging parameters.*"}}
+ (read-response (handler (ring.mock/request :get "/" {:offset "abc", :limit "100"})))))))
--
GitLab