metosin / malli

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

Should error reports include schema forms? #789

Open julienfantin opened 1 year ago

julienfantin commented 1 year ago

While working on a fix for https://github.com/metosin/malli/issues/770 I've found it really difficult to make sense of validation errors because reports include reified schema objects:

malli.core> (explain [:map [:id string?]] {:id :a})
{:errors ({:in [:id],
           :path [:id],
           :schema #<malli.core$_simple_schema$reify$reify__21622@2ec70b0e>,
           :value :a}),
 :schema #<malli.core$_map_schema$reify$reify__21833@35d35145>,
 :value {:id :a}}

There's nothing in this output that relates #<malli.core$_simple_schema$reify$reify__21622@2ec70b0e> to a userland schema. This is exacerbated during instrumentation because of the loss of local reasoning.

Looking at the readme, it seems like this is what's expected:

malli.core> (explain [:map [:id string?]] {:id :a})
{:errors ({:in [:id], :path [:id], :schema string?, :value :a}),
 :schema [:map [:id string?]],
 :value {:id :a}}

I could spot three groups of functions involved in this issue: malli.core/-fail!, explainers and malli.impl.util/-error. Could you clarify if none/some/all of these should -form schemas prior to reporting an error?

ikitommi commented 1 year ago

It is a feature. Tools like me/humanize pull values out of the :schema programmatically using :path. Reified Schemas can close over registries, which are not (currently) available in the form.

#?(:clj (defmethod print-method ::schema [v ^java.io.Writer w] (.write w (pr-str (-form ^Schema v)))))

Given

(def schema
  (m/schema
   ::tuple
   {:registry {:tuple (m/-tuple-schema)
               :int (m/-int-schema)
               ::tuple [:tuple ::int ::int]
               ::int :int}}))

(m/form schema)
; => :user/tuple

as Schema (works)

(-> schema
    (m/explain "invalid")
    (me/humanize))
; ["invalid type"]

as form (blows)

(-> schema
    (mu/explain-data "invalid")
    (me/humanize))
; =throws=> :malli.core/invalid-schema {:schema :user/int}
julienfantin commented 1 year ago

Thanks for the context!

Here's the problem I have with instrumentation. On the master branch, using CIDER, this is what gets displayed when an instrumented function reports an error:

Screenshot 2022-12-08 at 12 22 16 AM

The exception message, in light blue, shows the result of calling pr-str on the report data, so the schemas are properly -formed but there's no indentation or syntax highlighting, so non trivial errors quickly become unreadable.

The data on the other hand, displayed in pink, is indented with syntax highlighting, but the schemas print as reified objects, so you cannot tell what went wrong just looking at this one either.

If I instead call -form on the various schemas before calling report (i.e. m/-fail!) I get something much more usable:

Screenshot 2022-12-08 at 12 24 29 AM

Data explainers seem much more appropriate for error reporting in general, but using them from malli.core would cause a circular dependency.

What do you recommend?

ikitommi commented 1 year ago

I see.

There are at least two options:

1) PR/config to CIDER to use the -print-method when printing data (I think it would be good anyways, many libraries (like Manifold) have custom print forms for internal Types.

2) define a custom :report option to m/-instrument. It defaults to m/-fail!. You can walk the data and call m/form on all schemas.

it's called like this in m/-instrument:

(report ::invalid-input {:input input, :args args, :schema schema})

I think the "present schemas as forms" walker could be a optional utility in malli.util.