Re-engineer Verb - Githubissues

phadej commented 7 years ago

The idea is that

Instead of having

a :> b :> Verb c

we could have

a :> b :> Verb (c :< d)

which is transformed into something like

a -> b -> m (c, d)

Verb glues RHS and LHS together. This way we can

Have checked exceptions (here "date" endpoint cannot error!)

type MyAPI = "date" :> Verb (Full Day)
        :<|> "time" :> Capture TimeZone :> Verb (ServantErr :< Full ZonedTime)

Get rid of {-# OVERLAPPING #-} instances, as in
```
type NoContentAPI = "tick" :> Verb NoContent
```
NoContent doesn't overlap Full a. Similarly we can have Stream a and Headers ... :< ....

The devil in the details how to fit this into Router and DelayedIO framework we have, but I don't see why it's not possible.

The names of auxiliary class HasServer' and Full combinator are open to bikesheding.

Comments?

alpmestan commented 7 years ago

A little note: Oleg and I have been discussing this over the past couple of days. We want to unify everything to avoid as much duplication as possible. The alternative to being able to "augment" the response information as done here would be to have all the possible "options" be type parameters to Verb. Here's what that would look like: https://gist.github.com/alpmestan/4ad03493cb6fe87e02b514418e9f7f02

So we really want @phadej's idea to work :) This would make many adhoc things non adhoc (stream would not have to be a totally separate hierarchy of response combinators -- cc @gbaz -- while responses that provide some headers would just augment the response with those instead of having the ugly Headers '[...] resp hack -- and it just goes on and on. We could similarly have "exhaustive endpoint descriptions" that describe the happy paths as well as the other ones, so that derived clients would basically be "more strongly typed" than they are now, because for non-happy paths you're stuck with a ServantError value. Redirects would also work pretty easily in this setting (they're just a combination of status code + "Location" header after all).

erewok commented 7 years ago

As a user of Servant and a drive-by commentator, I really like this proposal. (I'd probably bike shed the <: combinator, though.)

alpmestan commented 7 years ago

As a user of Servant and a drive-by commentator, I really like this proposal. (I'd probably bike shed the <: combinator, though.)

We want to hear from our users, maybe even more in this issue than usual. Feel free to suggest another operator and to illustrate with snippets of code you'd like to be able to write.

erewok commented 7 years ago

Thank you, @alpmestan. I should have probably kept my mouth shut until I could suggest something better. I think the reason I objected initially to <: is that my brain wants to coerce it into the opposite of :> whereas Verb ServantErr SomeReturnType looks like Either a b.

I also want to be clear that I think of this as bike-shedding: the overall improvement discussed here is excellent, in my opinion, so don't let me distract from that.

alpmestan commented 7 years ago

Well your point about your brain being disturbed by :< is a good one. In fact when @phadej initially suggested this approach, I think he used :+, which I interpreted to mean "we augment the response with more information".

erewok commented 7 years ago

One question:

Would it be possible to do something like Verb ('[HTML] ServantErr <: '[JSON] SomeReturnType)?

I would probably never do this, just wondering.

alpmestan commented 7 years ago

Being able to express different "response paths" is one of the important goals of this issue. response path == status code + headers + content types + type of the response body, here. Ideally we want full flexibility, so as to assume nothing about what users want to do. Because there's always someone who will want to do that very thing that you thought was irrelevant =) It makes sense to only return errors in JSON while making the "success" response available in JSON and HTML, in some apps, so it's not all that far fetched after all.

erewok commented 7 years ago

Well, in that case then, would it possible to have different code paths depending on, for instance, objects that get submitted? I imagine something like this:

type MyApi = "a" :> ReqBody '[JSON] SomeType :> BasicAuth "api-access" User :> Verb ('[HTML] MyAppErr401 <: '[JSON] MyAppErr404 <: '[JSON] SomeReturnType)

In other words, is it more like an HList than Either a b? Or is it more like Alternative, as in Error1 || SuccessOrError2 where SuccessOrError2 is Error2 || Success...

alpmestan commented 7 years ago

It's the sum-type equivalent of HList that would model things accurately here, indeed. We want to model the overall response of the endpoint as being one of several possibilities. It would be up to the endpoint's code, using some dedicated function(s), to return this or that kind of response depending on its inputs and anything else you want. But at least all the possible responses (including error responses) would be declared and all of this would be documented by servant-docs, clients would with your example give something like postSomeType :: SomeType -> User -> ClientM (Either SomeReturnType (Either MyAppErr401 MyAppErr404)), etc.

sboosali commented 7 years ago

the sum-type equivalent of HList

the vinyl records library has "co-records", which says that any value in some type can be any of a list of valid types. https://hackage.haskell.org/package/vinyl-0.6.0/docs/Data-Vinyl-CoRec.html

for example, CoRec Identity '[SomeReturnType, MyAppErr401, MyAppErr404].

with the type alias that implies that each value can have zero or more errors,

type Result es a = CoRec Identity (a ': es) 
type MyResult = Result SomeReturnType '[MyAppErr401, MyAppErr404]

a few other record libraries have n-ary variants, like the sum-of-products package, I mention vinyl because it's lightweight and the one I'm familiar with. and, since vinyl wraps each field with the same functor, working with effects is easier, since you can say stuff like Rec IO '[Integer, String] and conveniently convert it to IO (Rec Identity '[Integer, String]) with utility functions like rtraverse.

(sorry if this isn't relevant, I only skimmed the discussion)

arianvp commented 7 years ago

I have a little bit of prior work of this in my bachelor project https://github.com/arianvp/servis/raw/master/paper/paper.pdf

See the chapter about Explicit status Codes

arianvp commented 7 years ago

Brainstormed about this with @rightfold yesterday. How about something like this?:

Lets ditch the idea of having a coproduct of return types

You have Verb '[200, 201, 401] GET Signallying that an endpoint can return any of these three status codes. and some type family

type family IsElem x xs :: Constraint where
  IsElem x '[] = TypeError (Text "No")
  IsElem x (x ': xs) = ()
  IsElem x (y ': xs) = IsElem x xs

Then the way for people implementing a server to set a status code would be something like:

setStatusCode :: IsElem x xs => Proxy x -> Server (Verb Get xs) ()

This disallows a server implementing the spec from setting a status code that is not in the spec.

You could go one step further and use something like IxMonad to actually force the user to set the status code of the handler.

erewok commented 7 years ago

I like the direction that suggestion is going in, but (unless I'm misreading) it loses the option to specify different return types and different encodings for each return path.

To put those back in would look like:

Get '[(200, JSON, NormalType), (403, HTML, AuthError), etc.]

Edit:

Now that, I think about it, the original suggestion also had more of a combinator feel, so I could presumably do something like this:

type MyAppAuthErrors = AuthRequired401 :< AuthFailure403
Verb (MyAppAuthErrors :< Full ZonedTime)

alpmestan commented 7 years ago

@arianvp Well, we still need the different response types like @erewok said. They'd just be "annotations" on the status code. And this is all remarquably close to what we've been describing so far with @phadej above =)

arianvp commented 7 years ago

While we're at it, we can get rid of ServantErr right? Usually, you'd do something like err403 to give a different error code, but now that we can put the possible status codes in the type, a "Different status code" is not really an error scenario anymore, just a different code path. This would basically return our base type back to IO which is kind of neat

alpmestan commented 7 years ago

Yes, we want to offer that possibility. Not sure we want to get rid of ServantErr just yet. If we solve this, we can keep ServantErr around a little bit to allow people to smoothly move to the new approach. But we're not there yet anyway :)

jkarni commented 7 years ago

Content-types should be the same for all paths, since we have to resolve content type before running the code.

(We've discussed this idea a few times in the past. I think there's already an issue for it that we should use, though I'm on my phone right now so will only check later)

On Saturday, November 18, 2017, Arian van Putten notifications@github.com wrote:

While we're at it, we can get rid of ServantErr right? Usually, you'd do something like err403 to give a different error code, but now that we can put the possible status codes in the type, a "Different status code" is not really an error scenario anymore, just a different code path. This would basically return our base type back to IO which is kind of neat

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/haskell-servant/servant/issues/841#issuecomment-345451828, or mute the thread https://github.com/notifications/unsubscribe-auth/ABlKmkX2XeGIbHfZJJNieXhNYP40PqI2ks5s3wBHgaJpZM4QCUq6 .

rightfold commented 7 years ago

I like the list of tuples idea. You can verify that the combination of status code and response type exists using a constraint.

respond :: IsResponse api s r => Proxy s -> r -> Server api

respond (Proxy :: Proxy '404) (CustomerNotFound uuid)

alpmestan commented 7 years ago

Note this is all very much the same idea, all throughout the thread, nothing new AFAICT. But the big challenge is to make it work @phadej -style (see first post) instead of exhaustive-style (see the gist that I linked to very early in this thread).

cdepillabout commented 6 years ago

@alpmestan pointed me to this issue in https://github.com/cdepillabout/servant-checked-exceptions/issues/4#issuecomment-378531911.

It would be really nice if servant provided some way to encode what errors are returned by an API in a flexible manner. I'm using https://github.com/cdepillabout/servant-checked-exceptions to do it in a couple projects at work (and it is working pretty well), but it would be nice if Servant provided some sort of built-in support for this.

I think this proposal of re-enginnering Verb would enable this?

alpmestan commented 6 years ago

Yes, that's one of the goals. The big general goal is, now that we have a good idea of what people need from Verb, to make it support all these use cases without the need for ugly and fragile overlapping instances all over the place, but to instead support all those things right from the start.

The first candidate approach is one where we would just add extra annotations to specify that we add response headers, want to stream the response while executing the handler, declare strongly typed error cases (what your package does), etc. See @phadej's post at the very beginning of this thread.

The second one consists in making Verb parametrized by all those things, and to offer specialized combinators that would provide today's behaviour to try and not break things too hard, I suppose.

benweitzman commented 5 years ago

I'm pretty interested in seeing explicit errors land in servant.

Seems like these type of issues are being closed as dupes of this issue, so I'm just commenting here.

Here's a gist showing a way to add these to servant using a Throws e annotation: https://gist.github.com/benweitzman/5875d3e6a8cb1470cea5c32db3180f90

It's not a change to Verb, but it's pretty lightweight and backwards compatible.

Biggest downside I can see is mtl issues surrounding multiple targets of throwError, but I don't see this being avoidable in any context than throw either a ServantError or an application specific error.

Thoughts?

alpmestan commented 5 years ago

I think @cdepillabout has a package for this already (servant-checked-exceptions IIRC?). The work that @phadej has been doing and discussing here is really about enhancing Verb to support various features that are not possible today, out of the box.

Do let us know if @cdepillabout's package doesn't quite fit what you need, in which case it is I suppose relevant to this issue. :-)

cdepillabout commented 5 years ago

@benweitzman As alpmestan said, servant-checked-exceptions provides a Throws types you can use to describe the errors thrown by an endpoint. It uses a couple tricks to make Throws as easy to use as possible (although it makes it slightly more complicated than your gist):

https://github.com/cdepillabout/servant-checked-exceptions

Feel free to open an issue if you have any problems/suggestions.