Fork me on GitHub
#reitit
<
2020-02-08
>
wombawomba08:02:27

@ikitommi thanks. For the schema thing, I was previously using reitit.ring.coercion/coerce-exceptions-middleware (https://github.com/metosin/reitit/blob/0.3.9/modules/reitit-ring/src/reitit/ring/coercion.cljc#L67-L85), but I was able to customize the errors by replacing this with reitit.ring.middleware.exception/create-exception-middleware (https://github.com/metosin/reitit/blob/master/modules/reitit-middleware/src/reitit/ring/middleware/exception.clj#L119-L180). I think it's a bit unclear how these two are supposed to fit together.. For instance, I only want to expose request coercion exceptions in the response — I want everything that's not a result of a user error to result in an opaque 500 response and a detailed message in the service logs — should I be using one or the other, or both?

ikitommi08:02:36

I would use only the latter

ikitommi08:02:34

the first one was done first, if one only want's coercion errors to be handled, but the latter works for all exceptions

ikitommi08:02:10

not intended to be used together, not clear in the docs

wombawomba08:02:13

Okay, thanks 🙂 Another question: It seems like the coercion errors are only reported for the first of the body/path-params/query-params check that happens to fail, where AFAICT the checks run in the order they happen to be iterated over here: https://github.com/metosin/reitit/blob/0.3.9/modules/reitit-core/src/reitit/coercion.cljc#L123-L128. This leads to a (sort of) possibly confusing behavior that I'd like to avoid if possible. To me, it'd be preferable if I all the checks could be performed always, so that I could report all errors to the user in a consistent manner. Is this possible?

ikitommi08:02:27

I think it would be ok to collect those all, but would require a code change. Would you like to write an issue and draft a PR out of that?

ikitommi08:02:23

oh, it would break the current contract of error reporting.

ikitommi08:02:19

which is a map of just one. But there could be an option to support multiple errors, via a mw option. What do you think?

ikitommi08:02:33

(or a coercion option)

ikitommi08:02:05

draft of "all errors with one return" message would be a good start. For 1.0.0, we can break things

wombawomba08:02:32

Yeah, it's a bit unfortunate because the ex-data keys that end up being returned are :in and :errors.. :errors can easily be made to accommodate all errors (via something like {:body <body-errors>, :query <query-param-errors>, :path <path-param-errors>}), but I don't see how to make multiple error types work with :in.

wombawomba08:02:50

I suppose if you fold :query/`:body`/`:path` into :errors, :in would be redundant and could be deprecated?

wombawomba08:02:55

By coercion option, do you mean an option in the route specification? To me a middleware option seems like it'd make more sense (it'd probably be a sensible default, too), because it'd be a bit weird to have error reporting work differently for different endpoints. Although I suppose there could be use-cases where people might prefer it (for performance reasons), so dunno.

ikitommi08:02:39

by coercion option I meant when you define schema-coercion once in the route data with options. There the option would effect how it's thrown (fail-fast or collect all). In exception mw, you can just effect how it's rendered. I think it's ok always to collect those all and let the formatter to decide how to render

ikitommi08:02:56

As this would be a silent breakage, it would need a major version bump, so the default should be the current one, until 1.0.0, where we could swap the default to "collect all errors" (and remove the current fail-fast impl)

ikitommi08:02:39

issue and pr welcome!

wombawomba09:02:33

@ikitommi so I looked into this, and it seems like what happens is that whichever of the path/route/body coercers fails first throws an exception (that's not caught by the middleware, via request-coercion-failed!) here: https://github.com/metosin/reitit/blob/0.3.9/modules/reitit-core/src/reitit/coercion.cljc#L81-L87 Do you think it's preferable to keep these exceptions (and catch, combine, and 'rethrow' them in the middleware), or to have the coercer functions return some sort of error container object instead (that can be used to build an exception)?

ikitommi12:02:55

I would loop the results without throwing, collect error s in a seq, in any, report them all with ex, otherwise, reduce the result