metosin / malli

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

`-ref-schema` returns wrong path in `-explainer` when the ref is a `var` #1106

Open he-la opened 2 months ago

he-la commented 2 months ago

What

I believe the path returned by the explainer of -ref-schema is wrong when the ref is a var. This originally turned up as follows:

Using malli.error/humanize on a schema that uses :ref to refer to another schema in a var throws :malli.core/invalid-schema.

For example, the following will throw:

(def Referred [:map [:foo :int]])
(def Schema [:ref #'Referred])

(me/humanize
  (m/explain Schema {:foo "2"})
  {:resolve me/-resolve-root-error})

Analysis indicates that this is because the path passed to the resolver when using :ref is too short: it receives [0 :foo], but (mu/get-in Schema [0 :foo]) is nil. The path should be [0 0 :foo], i.e. an extra leading 0 to "deref" (?) the var of the reference:

(mu/get-in Schema [0 :foo])    ; => nil
(my/get-in Schema [0])         ; => #'dev/Referred
(mu/get-in Schema [0 0 :foo])  ; => :int

This causes the schema in the let-binding here to be nil, which in turn causes the properties call on the subsequent line to throw invalid-schema as nil is not a valid schema.

Steps to reproduce

Add the following test to error_test.clj:

(deftest humanize-with-refs
  (testing "direct error" ; works!
    (is (= {:foo ["should be an integer"]}
           (with-local-vars [referred [:map [:foo :int]]]
             (me/humanize
              (m/explain [:ref referred] {:foo "2"})
              {:resolve me/-resolve-direct-error})))))

  (testing "root error" ; fails :(
    (is (= {:foo ["should be an integer"]}
           (with-local-vars [referred [:map [:foo :int]]]
             (me/humanize
              (m/explain [:ref referred] {:foo "2"})
              {:resolve me/-resolve-root-error}))))))
he-la commented 2 months ago

Here is a patch that fixes what I think the underlying error is, namely that the -explainer of the -ref-schema returns the wrong path when the ref is a var. All tests pass but I'm not 100% sure about this, so leaving it here more as a suggestion rather than a PR:

diff --git a/src/malli/core.cljc b/src/malli/core.cljc
index 05f2fc6..9d3faff 100644
--- a/src/malli/core.cljc
+++ b/src/malli/core.cljc
@@ -1706,7 +1706,7 @@
              (let [validator (-memoize (fn [] (-validator (rf))))]
                (fn [x] ((validator) x))))
            (-explainer [_ path]
-             (let [explainer (-memoize (fn [] (-explainer (rf) (conj path 0))))]
+             (let [explainer (-memoize (fn [] (-explainer (rf) (if (var? ref) (conj path 0 0) (conj path 0)))))]
                (fn [x in acc] ((explainer) x in acc))))
            (-parser [_] (->parser -parser))
            (-unparser [_] (->parser -unparser))
opqdonut commented 4 days ago

I'm not sure I have time to dig properly into this, but I did some investigation.

mu/subschemas agrees with mu/get-in:

(def Referred [:map [:foo :int]])
(def Schema [:ref #'Referred])
(mu/subschemas Schema)
;; ==> [{:path [], :in [], :schema [:ref #'user/Referred]}
;;      {:path [0 0], :in [], :schema [:map [:foo :int]]}
;;      {:path [0 0 :foo], :in [:foo], :schema :int}]

The same problem occurs with a non-var :ref:

(me/humanize
 (m/explain [:ref {:registry {::referred [:map [:foo :int]]}} ::referred] {:foo "2"})
 {:resolve me/-resolve-root-error})
;; ==> :malli.core/invalid-schema

(:errors (m/explain [:ref {:registry {::referred [:map [:foo :int]]}} ::referred] {:foo "2"}))
;; ==> ({:path [0 :foo], :in [:foo], :schema :int, :value "2"})

(mu/subschemas [:ref {:registry {::referred [:map [:foo :int]]}} ::referred])
;; ==> [{:path [], :in [], :schema [:ref {:registry #:user{:referred [:map [:foo :int]]}} :user/referred]}
;;      {:path [0 0], :in [], :schema [:map [:foo :int]]}
;;      {:path [0 0 :foo], :in [:foo], :schema :int}]

Could the fix be to always add the extra 0 to the path? Note how -walk always adds two 0s.

https://github.com/metosin/malli/blob/1749e03b89cf194d68a470dcb1ef9b10d3001c7c/src/malli/core.cljc#L1721

I don't really understand why -get on a -ref-schema uses -pointer. That's what seems to add the extra layer.

https://github.com/metosin/malli/blob/1749e03b89cf194d68a470dcb1ef9b10d3001c7c/src/malli/core.cljc#L1736

Using :schema instead of :ref works:

(def Schema2 [:schema #'Referred])
(:errors (m/explain Schema2 {:foo "2"}))
;; ==> ({:path [0 0 :foo], :in [:foo], :schema :int, :value "2"})
(me/humanize
 (m/explain Schema2 {:foo "2"})
 {:resolve me/-resolve-root-error})
;; ==> {:foo ["should be an integer"]}