Better error messages for decoding exceptions

[xificurC]

  (let [parse #(Long/parseLong %)]
    (m/decode [:map
               [:x {:decode/num parse} :int]
               [:y {:decode/num parse} :int]]
              {:x "1" :y "b2"}
              (mt/transformer {:name :num})))

throws

Execution error (NumberFormatException) at java.lang.NumberFormatException/forInputString (NumberFormatException.java:65).
For input string: "b2"

which is easy to spot in this case, not with a map that has 10-20 fields with the same decoder. The stacktrace doesn't point back to the schema line (I don't think it can either) and the error message doesn't tell me which key is the culprit. If the error could be caught and wrapped with more debug information that would be very helpful. Currently I'm thinking of changing the decoders to not throw and return a bogus object carrying the input value so that I can find the culprit(s) through m/explain.

[nilern]

encode and decode are 'best-effort'; the standard encoders and decoders return the original value if it did not match the schema. One must first ensure that the input is valid with validate and get the errors with explain if not. So encode and decode do not validate while validate and explain do not use the encoders or decoders. This makes transcoders efficient but is error-prone (and I would argue nothing is gained because usually validate is also called and the input traversed twice...).

Anyway, as it stands your decoders should pass bogus inputs through unchanged. Even if they throw instead, explain should not even call them so you should have been able to find the culprit(s).

[xificurC]

You're expecting for validate to always return false on values that didn't get decoded. While this example is such a scenario I don't see why that would always hold true. What if e.g. a string comes in base-64 encoded and you want to decode it? The validation might just be string? and non-decoded values will pass through decoding and validating unnoticed.

[nilern]

Nice point, although one could argue that a base-64 encoded value should not have such a liberal schema.

I forget the details but the seqex transcoder implementation is also a bit awkward because of stuff like (decode [:cat [:* [:int {:decode/num parse}]] [:* :string]] ["1" "2" "foo"] (mt/transformer {:name :num})).

[xificurC]

Yes, the schema of such a case should be less liberal, but I think it demonstrates the point. Is the suggestion of throwing a more informative exception too hard to implement?

[ikitommi]

It could be done, but for all feature requests, we need to balance between code complexity, performance and code size for cljs. Current design is that transformers don't throw.

Added malli.transform/-safe helper for this, see #381

[nilern]

Transformers could have some more documentation, especially around implementing custom ones.

[ikitommi]

Looked at the code again. Individual transformers do not know the path they are attached to, but the m/-value-transformer is a single place that could be modified to support wrapping any interceptor with a custom exception handler that would know the schema to be transformed.

So, adding an new option to allow custom exception handler in the chain would allow bit better errors, but not explain good ones.

Adding path-information to transformer creation, would be a bigger (and breaking) change. Could be evaluated, at some point.

[xificurC]

I think documentation about how a custom transformer should behave and what are the consequences would've been enough to steer me in the right direction. While I don't like the concept of a decoder silently returning the value unchanged I can learn to live with it ;)

Adding path-information to transformer creation, would be a bigger (and breaking) change. Could be evaluated, at some point.

My mental model was a bit different. Since the decoder is a function and I'm passing it to the framework (you call me) I expected the caller to have context about where in the schema is it applying the decoding function, therefore being able to enrich the exception with schema metadata

[ikitommi]

I have the same mental model, just that in the current implementation, the :path and :in are not accumulated inside the transformation engine. Adding those would breaking change - just but just in the extender api part.

[ikitommi]

adding those would open up a better view into what happens in the chain - one could interleave a interceptor that logs the current path, in, schema, phase and value before and after. Reitit has similar request/response debug printer.

This all would come with zero runtime perf penalty.

But, a lot of work. Interested in a PR?

[nilern]

Well the path vector would not come for free, at least with dynamic keys (e.g. :map-of).

The documentation needs to be improved in any case; the nonvalidating best-effort model is not what I (and @xificurC?) would expect.

[xificurC]

Interested in a PR?

Sorry but no. Documenting the behavior would be enough for now, I guess this isn't exactly a hot feature