Configurable combinator errors

[jkarni]

Problem

Currently combinators throw errors in a non-configurable way. If JSON decoding fails, for example, it's hard to get the error message to look the way you want. As another example, one would need to use hacky middleware to make 404 pages one likes.

Proposed approach

One of the exciting things about Context is that it makes it much easier to configure combinators, and makes fixing this relatively trivial. The way I conceive of it is:

data BasicErrorFormatting
  = NotFound (String -> ServantErr)  -- argument is requested route
  | BodyParseError (String -> ServantErr  -- argument is parse error string
  | QueryParamParseError ([String] -> ServantErr)  -- argument is all parse error strings
  ...

instance (..., HasConfigEntry ctx BasicErrorFormatting) => symbol :> rest where
  ....

Or:

newtype NotFound = NotFound (String -> ServantErr) 
newtype BodyParseError = ...
  ...

instance (..., HasConfigEntry ctx NotFound) => symbol :> rest where
  ....

And then we would provide a default BasicErrorFormatting (or set of error datatypes) that mimics current behavior, and make serve use it. serveWithContext would allow configuration.

Open questions

1. Ideally errors too would be responsive to Accept headers, though it's not clear how that would work
2. We could also allow IO before the response (i.e., NotFound (String -> IO ServantErr). The main advantage that I can see would be logging, though perhaps this is well covered by middleware

[maksbotan]

Hi,

I've stumbled into this problem, as many servant users did before me.

I see that these issues gathered a lot of nice idea, however no one volunteered to implement them.

Therefore I'd like to ask if the PR has any chance to get merged, if I manage to make one. Basically, I'm willing to implement Context-based approach to let users pass custom formatters to standard combinators like Capture and ReqBody.

I hope to make it so that users won't have to define custom ReqBodys (like wireapp/wire-server#814) anymore.

(#732 is still unrelated, and it's lower priority since one can use servant-checked-exceptions for that).

cc @fisx.

[fisx]

we're almost done implementing this: https://github.com/wireapp/servant-uverb

it's in a different repo, but we are planning to merge it into servant, and since the maintainers are all pretty much on board...

would you be ok with this approach?

wanna join us on zurihac? :)

[maksbotan]

Nice to hear that, @fisx!
I'll take a look at your repo. Initially I thought it was unrelated :)
Re zurihack: sounds nice. What are the rules?

[maksbotan]

Now I've had a look at servant-uverb. As far as I can see, it's aimed at providing a way to return several specified-at-typelevel responses from the Handler. This is a nice goal for sure!

But this issue is about error messages generated by combinators like Capture and ReqBody. Currently there is no way to override them without reimplementing, like you did in wire-server.

Unless you are not going to go this way too, my proposal still stands.

[jkarni]

I agree we'd ideally have both!

Re: the question of Accept in the original issue, I think an option is to just have a reader over some parameters, or over the entire request. That way you can still use return/pure if you don't want to have specific responses based on content-type (or Accept-Language, or any other params), but can if you wish to.

I also am not sure what I was thinking with the sum, rather than product, type (in BasicErrorFormatting).

One thing to be attentive to is backwards compatibility, both with the current version of servant, and with potential new versions that throw new types of error. Ideally, then, if there is some product type:

data FrameworkError = FrameworkError
  { notFound :: String -> Reader Request ServantErr 
  , bodyParseError :: String :: Reader Request ServantErr 
  ...
  }

Then the FrameworkError constructor should likely not be exposed, but instead just a defaultFrameworkError and the records for updating it. If on the other hand each error handler is a separate item in config entry, then if the default config already has them I think backwards compatibility is already going to work (since later additions override earlier ones, IIRC)

[maksbotan]

Thanks @jkarni,

I'm more inclined to implement the second approach, with separate types for handlers, since context is kinda an open product already. This should be more friendly to external combinators like servant-multipart, that could reuse the same way of passing parameters.

What do you think? Can I try and come up with a WIP PR for this?

[fisx]

Sorry @maksbotan I didn't pay attention when I replied earlier. Yes, this is an orthogonal issue and I should really find the time to read up on the details above...

[maksbotan]

Okay, I will try to work on this, maybe the next week.