From c3d1a35b03f093bf61fc9e510e9f57e8079adacb Mon Sep 17 00:00:00 2001
From: Bryan Maass <bryan.maass@gmail.com>
Date: Wed, 11 Jan 2023 16:00:38 -0700
Subject: [PATCH] Ensure api errors remain strings + provide structural hints
(#27563)
* - ensure errors are strings + provide strucrual hints
Instead of just overwriting the (string) erorr messages, swap in the
malli string description at [:errors :<field-name>] key. Adds humanized
error (which highlights which parts of req were not sufficient) under
the :specific-errors key.
* fix tests + note about auto-cohersion
---
src/metabase/api/common/internal.clj | 13 +++--
test/metabase/api/action_test.clj | 24 ++++++---
test/metabase/api/common/internal_test.clj | 33 +++++++++----
test/metabase/util/malli/describe_test.clj | 57 +++++++++++-----------
4 files changed, 77 insertions(+), 50 deletions(-)
diff --git a/src/metabase/api/common/internal.clj b/src/metabase/api/common/internal.clj
index 7ca4c5e1936..2ec34ea13b0 100644
--- a/src/metabase/api/common/internal.clj
+++ b/src/metabase/api/common/internal.clj
@@ -281,11 +281,14 @@
"Validate a parameter against its respective malli schema, or throw an Exception."
[field-name value schema]
(when-not (mc/validate schema value)
- (let [explained (mc/explain schema value)]
- (throw (ex-info (tru "Invalid m field: {0}" field-name)
- {:status-code 400
- :errors {(keyword field-name)
- (me/humanize (me/with-spell-checking explained))}})))))
+ (throw (ex-info (tru "Invalid m field: {0}" field-name)
+ {:status-code 400
+ :errors {(keyword field-name) (umd/describe schema)}
+ :specific-errors {(keyword field-name)
+ (-> schema
+ (mc/explain value)
+ me/with-spell-checking
+ me/humanize)}}))))
(defn validate-param
"Validate a parameter against its respective schema, or throw an Exception."
diff --git a/test/metabase/api/action_test.clj b/test/metabase/api/action_test.clj
index ed2f7bf9d24..4e167c84770 100644
--- a/test/metabase/api/action_test.clj
+++ b/test/metabase/api/action_test.clj
@@ -178,21 +178,31 @@
action-path (str "action/" (:id created-action))]
(testing "Validate POST"
(testing "Required fields"
- (is (partial= {:errors {:name ["should be a string"]}}
+ (is (partial= {:errors {:name "string"},
+ :specific-errors {:name ["should be a string"]}}
(mt/user-http-request :crowberto :post 400 "action" {:type "http"})))
- (is (partial= {:errors {:model_id ["should be a positive int"]}}
+ (is (partial= {:errors {:model_id "integer greater than 0"},
+ :specific-errors {:model_id ["should be a positive int"]}}
(mt/user-http-request :crowberto :post 400 "action" {:type "http" :name "test"}))))
(testing "Handles need to be valid jq"
- (is (partial= {:errors {:response_handle ["must be a valid json-query, something like '.item.title'"]}}
+ (is (partial= {:errors {:response_handle "nullable string, and must be a valid json-query, something like '.item.title'"},
+ :specific-errors {:response_handle ["must be a valid json-query, something like '.item.title'"]}}
(mt/user-http-request :crowberto :post 400 "action" (assoc initial-action :response_handle "body"))))
- (is (partial= {:errors {:error_handle ["must be a valid json-query, something like '.item.title'"]}}
+ (is (partial= {:errors {:error_handle "nullable string, and must be a valid json-query, something like '.item.title'"},
+ :specific-errors {:error_handle ["must be a valid json-query, something like '.item.title'"]}}
(mt/user-http-request :crowberto :post 400 "action" (assoc initial-action :error_handle "x"))))))
(testing "Validate PUT"
(testing "Template needs method and url"
- (is (partial= {:errors {:template {:method ["missing required key",] :url ["missing required key"]}}}
+ (is (partial= {:errors
+ {:template
+ "nullable map where {:method -> <enum of GET, POST, PUT, DELETE, PATCH>, :url -> <string with length <= 1>, :body (optional) -> <nullable string>, :headers (optional) -> <nullable string>, :parameters (optional) -> <nullable sequence of map>, :parameter_mappings (optional) -> <nullable map>} with no other keys"},
+ :specific-errors {:template {:method ["missing required key"],
+ :url ["missing required key"]}}}
(mt/user-http-request :crowberto :put 400 action-path {:type "http" :template {}}))))
(testing "Handles need to be valid jq"
- (is (partial= {:errors {:response_handle ["must be a valid json-query, something like '.item.title'"]}}
+ (is (partial= {:errors {:response_handle "nullable string, and must be a valid json-query, something like '.item.title'"},
+ :specific-errors {:response_handle ["must be a valid json-query, something like '.item.title'"]}}
(mt/user-http-request :crowberto :put 400 action-path (assoc initial-action :response_handle "body"))))
- (is (partial= {:errors {:error_handle ["must be a valid json-query, something like '.item.title'"]}}
+ (is (partial= {:errors {:error_handle "nullable string, and must be a valid json-query, something like '.item.title'"},
+ :specific-errors {:error_handle ["must be a valid json-query, something like '.item.title'"]}}
(mt/user-http-request :crowberto :put 400 action-path (assoc initial-action :error_handle "x")))))))))))
diff --git a/test/metabase/api/common/internal_test.clj b/test/metabase/api/common/internal_test.clj
index 254a9c4d46a..5c24c6bf14a 100644
--- a/test/metabase/api/common/internal_test.clj
+++ b/test/metabase/api/common/internal_test.clj
@@ -17,8 +17,12 @@
[:map
{:title "Address"}
[:id string?]
- ;; TODO it is possible to coerce things like this automatically:
- ;; [:tags [:set keyword?]]
+ ;; TODO: coerce stuff automatically via:
+ ;; (mc/decode
+ ;; [:map [:tags [:set keyword?]]]
+ ;; (json/decode "{\"tags\": [\"a\", \"b\"]}" true)
+ ;; (mtx/json-transformer))
+ ;; ;; => {:tags #{:b :a}}
[:tags [:vector string?]]
[:address
[:map
@@ -75,7 +79,9 @@
(is (= {:a 1 :b 2} (:body (post! "/post/any" {:a 1 :b 2}))))
(is (= {:id 1} (:body (post! "/post/id-int" {:id 1}))))
- (is (= {:errors {:id ["should be an int"]}} (:body (post! "/post/id-int" {:id "1"}))))
+ (is (= {:errors {:id "integer"},
+ :specific-errors {:id ["should be an int"]}}
+ (:body (post! "/post/id-int" {:id "1"}))))
(is (= {:id "myid"
:tags ["abc"]
@@ -88,12 +94,17 @@
:zip 2999
:lonlat [0.0 0.0]}}))))
- (is (= {:errors {:address {:id ["missing required key"]
- :tags ["missing required key"]
- :address ["missing required key"]}}}
+ (is (= {:errors
+ {:address "map where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>}>}"},
+ :specific-errors {:address {:id ["missing required key"],
+ :tags ["missing required key"],
+ :address ["missing required key"]}}}
(:body (post! "/post/test-address" {:x "1"}))))
- (is (= {:errors {:address {:id ["should be a string"]
+ (is (= {:errors
+ {:address "map where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>}>}"},
+ :specific-errors {:address
+ {:id ["should be a string"]
:tags ["invalid type"]
:address {:street ["missing required key"]
:zip ["should be an int"]}}}}
@@ -105,9 +116,11 @@
:lonlat [0.0 0.0]}}))))
(is (= {:errors
- {:address {:address ["missing required key"]
- :a ["disallowed key"]
- :b ["disallowed key"]}}}
+ {:address "map where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>} with no other keys>} with no other keys"},
+ :specific-errors {:address
+ {:address ["missing required key"],
+ :a ["disallowed key"],
+ :b ["disallowed key"]}}}
(:body (post! "/post/closed-test-address" {:id "1" :tags [] :a 1 :b 2}))))))
(deftest route-fn-name-test
diff --git a/test/metabase/util/malli/describe_test.clj b/test/metabase/util/malli/describe_test.clj
index 7c8ef1485b2..29468e51eeb 100644
--- a/test/metabase/util/malli/describe_test.clj
+++ b/test/metabase/util/malli/describe_test.clj
@@ -53,34 +53,35 @@
(is (= "Order which is: <Country is map where {:name -> <enum of :FI, :PO>, :neighbors (optional) -> <vector of \"Country\">} with no other keys, Burger is map where {:name -> <string>, :description (optional) -> <string>, :origin -> <nullable Country>, :price -> <integer greater than 0>}, OrderLine is map where {:burger -> <Burger>, :amount -> <integer>} with no other keys, Order is map where {:lines -> <vector of OrderLine>, :delivery -> <map where {:delivered -> <boolean>, :address -> <map where {:street -> <string>, :zip -> <integer>, :country -> <Country>}>} with no other keys>} with no other keys>"
(umd/describe [:schema
- {:registry {"Country" [:map
- {:closed true}
- [:name [:enum :FI :PO]]
- [:neighbors
- {:optional true}
- [:vector [:ref "Country"]]]],
- "Burger" [:map
- [:name string?]
- [:description {:optional true} string?]
- [:origin [:maybe "Country"]]
- [:price pos-int?]],
- "OrderLine" [:map
- {:closed true}
- [:burger "Burger"]
- [:amount int?]],
- "Order" [:map
- {:closed true}
- [:lines [:vector "OrderLine"]]
- [:delivery
- [:map
- {:closed true}
- [:delivered boolean?]
- [:address
- [:map
- [:street string?]
- [:zip int?]
- [:country "Country"]]]]]]}}
- "Order"])))
+ {:registry
+ {"Country" [:map
+ {:closed true}
+ [:name [:enum :FI :PO]]
+ [:neighbors
+ {:optional true}
+ [:vector [:ref "Country"]]]],
+ "Burger" [:map
+ [:name string?]
+ [:description {:optional true} string?]
+ [:origin [:maybe "Country"]]
+ [:price pos-int?]],
+ "OrderLine" [:map
+ {:closed true}
+ [:burger "Burger"]
+ [:amount int?]],
+ "Order" [:map
+ {:closed true}
+ [:lines [:vector "OrderLine"]]
+ [:delivery
+ [:map
+ {:closed true}
+ [:delivered boolean?]
+ [:address
+ [:map
+ [:street string?]
+ [:zip int?]
+ [:country "Country"]]]]]]}}
+ "Order"])))
(is (= "ConsCell <nullable vector with exactly 2 items of type: integer, \"ConsCell\">"
(umd/describe [:schema
--
GitLab