Improved `mu/defn` error messages (#32966)

* Simplified impl.

* Fix kondo errors

* Cljs test fix 

shared/src/metabase/mbql/schema.cljc

@@ -736,6 +736,7 @@
 (def ^:private FieldOrExpressionRefOrRelativeDatetime
   [:multi
    {:error/message ":field or :expression reference or :relative-datetime"
+    :error/fn      (constantly ":field or :expression reference or :relative-datetime")
     :dispatch      (fn [x]
                      (if (is-clause? :relative-datetime x)
                        :relative-datetime

shared/test/metabase/mbql/schema_test.cljc

@@ -4,7 +4,8 @@
    [clojure.test :refer [are deftest is testing]]
    [malli.core :as mc]
    [malli.error :as me]
-   [metabase.mbql.schema :as mbql.s]))
+   [metabase.mbql.schema :as mbql.s]
+   [metabase.util.malli.humanize :as mu.humanize]))
 
 (deftest ^:parallel temporal-literal-test
   (testing "Make sure our schema validates temporal literal clauses correctly"
@@ -131,3 +132,23 @@
       mbql.s/value
       @#'mbql.s/EqualityComparable
       [:or mbql.s/absolute-datetime mbql.s/value])))
+
+(deftest ^:parallel or-test
+  (are [schema expected] (= expected
+                            (mu.humanize/humanize (mc/explain schema [:value "192.168.1.1" {:base_type :type/FK}])))
+    mbql.s/absolute-datetime
+    "not an :absolute-datetime clause"
+
+    [:or mbql.s/absolute-datetime]
+    "not an :absolute-datetime clause"
+
+    mbql.s/value
+    [nil nil {:base_type "Not a valid base type: :type/FK"}]
+
+    [:or mbql.s/value]
+    [nil nil {:base_type "Not a valid base type: :type/FK"}]
+
+    [:or mbql.s/absolute-datetime :string mbql.s/value]
+    ["not an :absolute-datetime clause"
+     "should be a string"
+     [nil nil {:base_type "Not a valid base type: :type/FK"}]]))

src/metabase/util/malli/fn.clj

@@ -7,6 +7,7 @@
    [malli.error :as me]
    [malli.experimental :as mx]
    [metabase.shared.util.i18n :as i18n]
+   [metabase.util.malli.humanize :as mu.humanize]
    [metabase.util.malli.registry :as mr]))
 
 (defn- add-default-map-schemas
@@ -53,7 +54,7 @@
                                                 (cons '&f fn-tail)))]
     (when (= parsed ::mc/invalid)
       (let [error     (mc/explain mx/SchematizedParams fn-tail)
-            humanized (me/humanize error)]
+            humanized (mu.humanize/humanize error)]
         (throw (ex-info (format "Invalid function tail: %s" humanized)
                         {:fn-tail   fn-tail
                          :error     error

src/metabase/util/malli/humanize.cljc

+(ns metabase.util.malli.humanize
+  (:require
+   [malli.error :as me]))
+
+(defn- resolve-error
+  "This is the same behavior as what [[malli.error/humanize]] does to resolve errors."
+  [explanation error]
+  (me/-resolve-direct-error explanation error {:wrap :message, :resolve me/-resolve-direct-error}))
+
+(defn- flatten-error
+  "Given a `[path message]` pair like
+
+    [[2] \"some error\"]
+
+  return a flattened error message like
+
+    [nil nil \"some error\"]"
+  [[path message]]
+  (if (empty? path)
+    message
+    (recur
+     [(butlast path)
+      (if (integer? (last path))
+        (me/-push [] (last path) message nil)
+        {(last path) message})])))
+
+(defn- merge-errors
+  "Merge two flattened errors into a single error, e.g.
+
+    (merge-errors {:x \"oops\"}
+                  {:x \"oh no\"})
+    ;; => {:x (\"oops\" \"oh no\")}
+
+  List-like structures are used to differentiate multiple errors (e.g., the result of an `:or` schema) from single
+  errors (which use a vector instead)."
+  [msg-1 msg-2]
+  (cond
+    (= msg-1 msg-2)
+    msg-1
+
+    (nil? msg-1)
+    msg-2
+
+    (seq? msg-1)
+    (distinct (concat msg-1 (if (seq? msg-2) msg-2 [msg-2])))
+
+    (and (map? msg-1)
+         (map? msg-2))
+    (merge-with merge-errors msg-1 msg-2)
+
+    (and (vector? msg-1)
+         (vector? msg-2)
+         (= (count msg-1) (count msg-2)))
+    (mapv merge-errors msg-1 msg-2)
+
+    :else
+    (distinct (list msg-1 msg-2))))
+
+(defn humanize
+  "Improved version of [[malli.error/humanize]]. This is mostly similar to vanilla [[malli.error/humanize]], but
+  combines 'resolved' errors in a different way that avoids discarding errors in `:or` schemas when they occur at
+  different levels of nesting (see [[metabase.util.malli.humanize-test/basic-test]]
+  or [[metabase.util.malli.humanize-test/basic-test-2]] for example) and eliminates duplicates."
+  [{:keys [errors], :as explanation}]
+  (transduce
+   (comp (map (fn [error]
+                (resolve-error explanation error)))
+         (map flatten-error))
+   (completing merge-errors)
+   nil
+   errors))

test/metabase/lib/schema/join_test.cljc

+(ns metabase.lib.schema.join-test
+  (:require
+   [clojure.test :refer [deftest is]]
+   [malli.core :as mc]
+   [metabase.lib.schema :as lib.schema]
+   [metabase.lib.schema.join :as lib.schema.join]
+   [metabase.util.malli.humanize :as mu.humanize]
+   #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
+
+#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
+
+(deftest ^:parallel join-schema-test
+  (is (=? {:stages ["end of input"]}
+          (mu.humanize/humanize (mc/explain ::lib.schema.join/join {:stages []}))))
+  ;; not sure why these errors are repeated.
+  (is (=? {:stages [{:joins [{:stages [{:lib/type ["missing required key"
+                                                   "invalid dispatch value"]}]}]}]}
+          (mu.humanize/humanize (mc/explain ::lib.schema/query {:stages [{:lib/type :mbql.stage/mbql
+                                                                          :joins    [{:lib/type :mbql/join
+                                                                                      :stages   [{}]}]}]})))))

test/metabase/util/malli/humanize_test.cljc

+(ns metabase.util.malli.humanize-test
+  (:require
+   [clojure.test :refer [deftest are]]
+   [malli.core :as mc]
+   [malli.error :as me]
+   [metabase.lib.schema :as lib.schema]
+   [metabase.lib.schema.join :as lib.schema.join]
+   [metabase.mbql.schema :as mbql.s]
+   [metabase.util.malli.humanize :as mu.humanize]
+   [metabase.util.malli.registry :as mr]
+   #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
+
+#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
+
+(deftest ^:parallel merge-errors-test
+  (are [x y expected] (= expected
+                         (#'mu.humanize/merge-errors x y))
+
+    '("a" "b")
+    "c"
+    '("a" "b" "c")
+
+    {:stages [{:joins [{:stages [{:lib/type "a"}]}]}]}
+    {:stages [{:joins [{:stages [{:lib/type "b"}]}]}]}
+    {:stages [{:joins [{:stages [{:lib/type '("a" "b")}]}]}]}
+
+    '("a" "b")
+    [nil nil {:base_type "c"}]
+    '("a" "b" [nil nil {:base_type "c"}])
+
+    '("a" [nil nil {:base_type "c"}])
+    "b"
+    '("a" [nil nil {:base_type "c"}] "b")
+
+    ;; not a list, but list-like
+    {:stages [{:joins [{:stages [{:lib/type (cons "a" (list "b" "c"))}], :alias "d"}]}]}
+    {:stages [{:joins [{:stages [{:lib/type "e"}]}]}]}
+    {:stages [{:joins [{:stages [{:lib/type '("a" "b" "c" "e")}], :alias "d"}]}]}
+
+    '("a" "b")
+    '("c" "d")
+    '("a" "b" "c" "d")
+
+    ;; eliminate duplicates
+    '("a" "b")
+    '("b" "c")
+    '("a" "b" "c")
+
+    "a"
+    "a"
+    "a"))
+
+(deftest ^:parallel basic-test
+  (let [error (mc/explain
+               [:or
+                :int
+                mbql.s/value]
+               [:value "192.168.1.1" {:base_type :type/FK}])]
+    (are [f expected] (= expected
+                         (f error))
+      ;; note the missing error for [[mbql.s/value]]
+      me/humanize
+      ["should be an integer"]
+
+      mu.humanize/humanize
+      ["should be an integer"
+       [nil nil {:base_type "Not a valid base type: :type/FK"}]])))
+
+(deftest ^:parallel basic-test-2
+  (let [error (mc/explain
+               [:map
+                [:x [:or
+                     :int
+                     mbql.s/value]]]
+               {:x [:value "192.168.1.1" {:base_type :type/FK}]})]
+    (are [f expected] (= expected
+                         (f error))
+      ;; note the missing error for [[mbql.s/value]]
+      me/humanize
+      {:x ["should be an integer"]}
+
+      mu.humanize/humanize
+      {:x ["should be an integer"
+           [nil nil {:base_type "Not a valid base type: :type/FK"}]]})))
+
+(deftest ^:parallel or-test
+  (let [error (mc/explain
+               [:or
+                :string
+                [:tuple {:error/message ":value clause"}
+                 [:= :value]
+                 any?
+                 :map]
+                number?]
+               [:value 1 nil])]
+    (are [f expected] (= expected
+                         (f error))
+      me/humanize
+      ["should be a string" "should be a number"]
+
+      mu.humanize/humanize
+      '["should be a string"
+        #_(":value clause" [nil nil "invalid type"])
+        [nil nil "invalid type"]
+        "should be a number"])))
+
+(mr/def ::absolute-datetime
+  [:multi {:error/message "valid :absolute-datetime clause"
+           :dispatch      identity}
+   [::mc/default [:fn
+                  {:error/message "not an :absolute-datetime clause"}
+                  (constantly false)]]])
+
+(deftest ^:parallel ref-test
+  (are [f expected] (= expected
+                       (f (mc/explain [:or
+                                       [:ref ::absolute-datetime]]
+                                      [:value "192.168.1.1" {:base_type :type/FK}])))
+    me/humanize
+    ["not an :absolute-datetime clause"]
+
+    mu.humanize/humanize
+    "not an :absolute-datetime clause"))
+
+(deftest ^:parallel ref-test-2
+  (are [f expected] (= expected
+                       (f (mc/explain [:or mbql.s/value] [:value "192.168.1.1" {:base_type :type/FK}])))
+    me/humanize
+    [nil nil {:base_type ["Not a valid base type: :type/FK"]}]
+
+    mu.humanize/humanize
+    [nil nil {:base_type "Not a valid base type: :type/FK"}]))
+
+(deftest ^:parallel map-test
+  (let [error (mc/explain
+               [:map
+                {:error/message "map with :a"}
+                [:a
+                 [:map
+                  {:error/message "map with :b"}
+                  [:b
+                   [:map
+                    {:error/fn (constantly "map with :c")}
+                    [:c string?]]]]]]
+               {:a {:b {:c 1}}})]
+    (are [f expected] (= expected
+                         (f error))
+      me/humanize
+      {:a {:b {:c ["should be a string"]}}}
+
+      mu.humanize/humanize
+      {:a {:b {:c "should be a string"}}})))
+
+(deftest ^:parallel map-test-2
+  (let [error (mc/explain ::lib.schema.join/join {:stages 1})]
+    (are [f expected] (=? expected
+                          (f error))
+      me/humanize
+      {:lib/type ["missing required key"]}
+
+      mu.humanize/humanize
+      {:lib/type "missing required key"})))
+
+(deftest ^:parallel map-test-3
+  (let [error (mc/explain ::lib.schema/query {:lib/type :mbql/query
+                                              :database 1
+                                              :stages   [{:lib/type     :mbql.stage/mbql
+                                                          :source-table 1
+                                                          :joins        [{:lib/type    :mbql/join
+                                                                          :lib/options {:lib/uuid (str (random-uuid))}
+                                                                          :stages      [{}]
+                                                                          :conditions  [true]}]}]})]
+    (are [f expected] (= expected
+                         (f error))
+      ;; not sure why these errors are repeated.
+      me/humanize
+      {:stages [{:joins       [{:stages [{:lib/type ["missing required key"
+                                                     "invalid dispatch value"
+                                                     "missing required key"
+                                                     "invalid dispatch value"]}]
+                                :alias  ["missing required key"
+                                         "missing required key"]}]
+                 :source-card ["missing required key"]}]}
+
+      mu.humanize/humanize
+      {:stages [{:joins       [{:stages [{:lib/type ["missing required key"
+                                                     "invalid dispatch value"]}]
+                                :alias  "missing required key"}]
+                 :source-card "missing required key"}]})))

test/metabase/util/malli/registry_test.cljc

@@ -15,3 +15,11 @@
   (testing "cache explainers"
     (is (identical? (mr/explainer ::int)
                     (mr/explainer ::int)))))
+
+(deftest ^:parallel resolve-test
+  (is (mc/schema? (mr/resolve-schema :int)))
+  (is (mc/schema? (mr/resolve-schema ::int)))
+  #?(:clj
+     (is (= ":int"
+            (pr-str (mr/resolve-schema ::int))
+            (pr-str (mr/resolve-schema [:ref ::int]))))))