Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

union verbs #1314

Merged
merged 42 commits into from
Oct 31, 2020
Merged

union verbs #1314

merged 42 commits into from
Oct 31, 2020

Conversation

fisx
Copy link
Member

@fisx fisx commented Jun 16, 2020

Fixes #841

TODO:

TODO elsewhere, outside this PR:

@maksbotan
Copy link
Contributor

maksbotan commented Jun 21, 2020

Hi. I would like to hear your opinion about my idea.

Since UVerb (probably) will mostly be used for error-like responses, it may be desirable to be able to early abort handler, like with current servant one would use throwError with ServerError.

newtype UVerbT xs m a = UVerbT { unUVerbT :: ExceptT (Union xs) m a }
  deriving newtype (Functor, Applicative, Monad, MonadTrans)

-- | Deliberately hide 'ExceptT's 'MonadError' instance to be able to use
-- underlying monad's instance.
instance MonadError e m => MonadError e (UVerbT xs m) where
  throwError = lift . throwError
  catchError (UVerbT act) h = UVerbT $ ExceptT $
    runExceptT act `catchError` (runExceptT . unUVerbT . h)

-- | This combinator runs 'UVerbT'. It applies 'respond' internally, so the handler
-- may use the usual 'return'.
runUVerbT :: (Monad m, HasStatus x, IsMember x xs) => UVerbT xs m x -> m (Union xs)
runUVerbT (UVerbT act) = either id id <$> runExceptT (act >>= respond)

-- | Short-circuit 'UVerbT' computation returning one of the response types.
throwUVerb :: (Monad m, HasStatus x, IsMember x xs) => x -> UVerbT xs m a
throwUVerb = UVerbT . ExceptT . fmap Left . respond

Example usage:

h :: Handler (Union '[Foo, WithStatus 400 Bar])
h = runUVerbT $
  when (something bad) $
    throwUVerb $ WithStatus @400 Bar

  when (really bad) $
    throwError $ err500

  -- a lot of code here...

  return $ Foo 1 2 3

@erewok
Copy link
Contributor

erewok commented Jun 21, 2020

@maksbotan I like that. I have to admit that I always get really confused with errors: do I want throwE, throwError, or something else? I am in favor of anything simple and ergonomic.

Still, I don't tend to think of returning a 400 as "throwing" anything. I handle bad requests all the time and the server is operating fine, so in my mind these are not exceptions. With this in mind, I actually would not recommend making it seem like it's throw-ing an error. I'd probably just call it something like uresponse because I think uresponse $ WithStatus @400 Bar would be easy to teach to my coworkers.

This is a drive-by opinion, though, by someone who's never bothered to learn all the error/exception stuff in Haskell, so feel free to dismiss.

@maksbotan
Copy link
Contributor

Well, I made this for the case when you are doing some validation in your handlers, and let's say there are a lot of cases you need to check and all of them result in some kind of 400 response. So for me it's convenient to have short-circuiting way to return these 400 responses without having to have a lot of nested cases.

Anyway, naming is a very subjective matter. I do not have strong preference for throw* stuff, but I do think that it goes well along the other short-circuiting stuff.

@fisx
Copy link
Member Author

fisx commented Jun 22, 2020

@maksbotan I also like your idea, but I would (slightly) prefer to move it to a separate PR, in the hope this will get merged faster. Do you have an opinion?

@fisx
Copy link
Member Author

fisx commented Jun 22, 2020

Hm... Shouldn't runUVerbT return Union (a ': xs), though?

@maksbotan
Copy link
Contributor

Sure, I wasn't suggesting including it into this PR :) It's just that I was experimenting with servant-uverb and came up with this.

Re Union (a ': xs): that's actually a nice idea! Let's you separate "main" response from "error" ones.

@arianvp
Copy link
Member

arianvp commented Jun 24, 2020

Having an ExceptT-wrapper for this sounds neat. I wouldn't want to integrate it into the uverb-mechanism directly though as I personally want to get away from the idea that "status codes" are "exceptional" and should be "short circuiting" as i do not think that is the case in general. It should be something a person can opt in to by wrapping in a UVerbT . Anyhow it's good food for thought. I also see some interesting parallels with extensible-effect libraries.

But I totally agree having the option to short-circuit is extremely convenient. Scotty for example has raiseStatus and respondStatus for short-circuiting and non-short-circuiting

@fisx
Copy link
Member Author

fisx commented Jun 25, 2020

Having an ExceptT-wrapper for this sounds neat. I wouldn't want to integrate it into the uverb-mechanism directly though as I personally want to get away from the idea that "status codes" are "exceptional" and should be "short circuiting" as i do not think that is the case in general. It should be something a person can opt in to by wrapping in a UVerbT . Anyhow it's good food for thought. I also see some interesting parallels with extensible-effect libraries.

But I totally agree having the option to short-circuit is extremely convenient. Scotty for example has raiseStatus and respondStatus for short-circuiting and non-short-circuiting

Note that UVerbT as sketched here is in no way forcing you to use it. We can add it to Servant.Server.UVerb and clearly mark it as voluntary and optional in the haddocks, and people can decide whether to use it or not on a per-handler basis.

@arianvp
Copy link
Member

arianvp commented Jun 25, 2020

Yep completely agreed. I just wonder if servant is the right place for this .. This feels like a sort of type that must be available somewhere in some library already? If it's not; I wonder why

As said; I see some eer-y parallels with extensible effects libraries

I think that e.g. with the extensible-effects library you can model something like this already:

https://hackage.haskell.org/package/extensible-effects-5.0.0.1/docs/Control-Eff-Exception.html

myAction :: Eff '[Exc Unauthorised, Exc NotFound] User
myAction = do
  throwError Unauthorised
  throwError NotFound
  pure User 

Or in fused-effects http://hackage.haskell.org/package/fused-effects-1.0.2.2/docs/Control-Effect-Throw.html

I'm not sure if we should hook into a specific effects library for this; as servant has always been MTL-based so far... but it feels odd to reinvent this behaviour if it's somewhere else already

@maksbotan
Copy link
Contributor

I'm sorry for stirring the confusion!
I do not at all insist that this belongs to servant.

@arianvp
Copy link
Member

arianvp commented Jun 25, 2020

Don't feel sorry! It sounds like a super useful pattern. It is at the least worth mentioning in whatever tutorial we come up for with this,

@arianvp arianvp marked this pull request as ready for review June 25, 2020 09:24
@arianvp arianvp marked this pull request as draft June 25, 2020 09:26
@fisx
Copy link
Member Author

fisx commented Jun 30, 2020

I'm sorry for stirring the confusion!
I do not at all insist that this belongs to servant.

I took the liberty of adding it to the cookbook and making you the author of the commit: 084941a

I hope that was ok? Thanks again for dumping the idea here, I think that was very helpful! :)

(@arianvp feel free to add your thoughts on which libraries could be used for that.)

@maksbotan
Copy link
Contributor

Thanks @fisx!

@maksbotan
Copy link
Contributor

By the way, what's the status of this one?

@fisx
Copy link
Member Author

fisx commented Jul 31, 2020

By the way, what's the status of this one?

it's all in the description. :)

i'll try to talk to @arianvp about this today, perhaps we can merge it as well, and then make the ultimate release.

@maksbotan
Copy link
Contributor

Hi @fisx! Is there any progress on this? I would love to see UVerbs in servant master :)

@fisx
Copy link
Member Author

fisx commented Oct 17, 2020

Trying to build with cabal now, and I also have no luck. With cabal 3.2.0.0, ghc 8.8.4, I get a number of entertaining errors, but this is my favorite:

Configuring library for hspec-discover-2.7.4..
cabal: ghc-pkg dump failed: dieVerbatim: user error (cabal:
'/home/mf/.ghcup/bin/ghc-pkg' exited with an error:
ghc-pkg: /home/mf/.cabal/store/ghc-8.10.2/package.db/package.cache:
GHC.PackageDb.readPackageDb: inappropriate type (Not a valid Unicode code
point!)
)

@maksbotan
Copy link
Contributor

That's strange... Maybe you could try nuking your cabal store?

As far as I see, it built successfully on Travis. And I have no problems building with Cabal (via haskell.nix) locally.

@fisx
Copy link
Member Author

fisx commented Oct 17, 2020

That's strange... Maybe you could try nuking your cabal store?

Yes, I wanted to avoid that, but it's probably a good idea...

As far as I see, it built successfully on Travis. And I have no problems building with Cabal (via haskell.nix) locally.

Ah... nix! :) that probably helps, yes.

@arianvp can you still reproduce the problem on your machine?

@fisx fisx requested a review from arianvp October 25, 2020 13:19
@fisx fisx merged commit c110589 into haskell-servant:master Oct 31, 2020
@erewok
Copy link
Contributor

erewok commented Oct 31, 2020

Nice work, @fisk!

@bergmark
Copy link

This is amazing, thanks!! Error handling was a large gap in servant before this landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-engineer Verb
7 participants