Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
he-la opened this issue Sep 7, 2024 · 2 comments
Open

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

he-la opened this issue Sep 7, 2024 · 2 comments

Comments

@he-la
Copy link

he-la commented Sep 7, 2024

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
Copy link
Author

he-la commented Sep 7, 2024

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

@he-la he-la changed the title me/humanize fails on schemas with :ref when using -resolve-root-error -ref-schema returns wrong path in -explainer when the ref is a var Sep 7, 2024
@opqdonut
Copy link
Member

opqdonut commented Nov 8, 2024

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.

(let [accept (fn [] (-inner walker (rf) (into path [0 0])

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

(-get [_ key default] (if (= key 0) (-pointer ref (rf) options) default))

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"]}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

2 participants