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

@@ -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."

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

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

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