metosin / malli

High-performance data-driven data specification library for Clojure/Script.
Eclipse Public License 2.0
1.44k stars 204 forks source link

Humanize gobbles some errors when interacting with :vector #974

Open irigarae opened 7 months ago

irigarae commented 7 months ago

Hi,

Just reporting what I think is an unexpected behaviour, if it's intentional I would be interested in seeing what's the reasoning behind. Errors are correctly m/explained but me/humanize hides some of them. Hope the report is useful!

(require '[malli.core :as m]
        '[malli.error :as me])

(me/humanize
 (m/explain
  [:and
   [:map
    [:key [:fn {:error/message "visible"}
           (constantly false)]]]
   [:fn {:error/path [:key]
         :error/message "non-visible"}
    (constantly false)]]
  {:key [:whatever]}))

#_=> {:key ["visible" "non-visible"]}

;; when overlapping with :vector one of the errors explained is not humanized

(me/humanize
 (m/explain
  [:and
   [:map
    [:key [:vector [:fn {:error/message "visible"}
                    (constantly false)]]]]
   [:fn {:error/path [:key]
         :error/message "non-visible"}
    (constantly false)]]
  {:key [:whatever]}))

#_=> {:key [["visible"]]}
ikitommi commented 7 months ago

What would be more correct answer to this? me/humanize follows the same structure as the data, so the latter error is inside a vector (as the value itself). Errors are wrapped in (meta-data tagged) vector, the error container, so the latter reads:

"map with :key having a vector of values where the first value has errors :whatever".

malli accumulates errors in same path IF there is already an error container in that path. the latter error can't be attached anymore as there is no error container in path :key, but a normal vector instead.

some samples do demonstrate how this works:

;; vector-in-vector
(me/humanize
 (m/explain
  [:and
   [:map
    [:key [:vector [:vector [:fn {:error/message "visible"}
                             (constantly false)]]]]]
   [:fn {:error/path [:key]
         :error/message "non-visible"}
    (constantly false)]]
  {:key [[:whatever]]}))
; => {:key [[["visible"]]]}

;; the latter error has correct path, "first element of vector"
(me/humanize
 (m/explain
  [:and
   [:map
    [:key [:vector [:fn {:error/message "visible"}
                    (constantly false)]]]]
   [:fn {:error/path [:key 0]
         :error/message "non-visible"}
    (constantly false)]]
  {:key [:whatever]}))
; => {:key [["visible" "non-visible"]]}

I you think this should work in a different way, I'm all ears :)

irigarae commented 7 months ago

Thanks for the quick answer. I now understand that it tries to follow the same structure as the input. I didn't think that it was the main objective since I've also seen :malli/error as an extra key "collector" of errors.

I've used me/humanize as a glorified error log, instead of writing my own code to interpret the output of explain, so I don't have a formed opinion on how it could behave in a globally better way :)

(me/humanize
 (m/explain
  [:and
   [:map]
   [:fn {:error/path []
         :error/message "visible"}
    (constantly false)]]
  {:key [:whatever]}))

#_=> ["visible"]

(me/humanize
 (m/explain
  [:and
   [:map [:k :any]]
   [:fn {:error/path []
         :error/message "visible"}
    (constantly false)]]
  {:key [:whatever]}))

#_=> {:k ["missing required key"], :malli/error ["visible"]}

Sorry to not bring any specific suggestion, but I guess thinking what's the objective of me/humanize would be enough, if it's following the structure to the letter, then maybe documenting that some errors may be hidden at first is good enough. Also then maybe :malli/error also doesn't fit entirely in me/humanize?

Or now that I'm writing, a way to wrap a vector or anything in a map with {:malli/error [] :malli/in []} to accumulate extra errors and :malli/in telling you what should've been at that level without the map wrapping? E.g., hypothetical output to give an idea

(me/humanize
 (m/explain
  [:and
   [:map
    [:key [:vector [:fn {:error/message "visible"}
                    (constantly false)]]]]
   [:fn {:error/path [:key]
         :error/message "non-visible"}
    (constantly false)]]
  {:key [:whatever]}))

;; instead of
#_=> {:key [["visible"]]}
;; do
#_=> {:key {:malli/in [["visible"]]
            :malli/error ["non-visible"]}}

(me/humanize
 (m/explain
  [:and
   [:map [:k :any]]
   [:fn {:error/path []
         :error/message "visible"}
    (constantly false)]]
  {:key [:whatever]}))

;; instead of
#_=> {:k ["missing required key"], :malli/error ["visible"]}
;; do
#_=> {:malli/in {:k ["missing required key"]}, :malli/error ["visible"]}

But definitely think it through on your side, I just had this idea on the spot and maybe it doesn't fit with the rest of the library.

Thanks!