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

Support annotated-exception #1762

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

parsonsmatt
Copy link
Contributor

@parsonsmatt parsonsmatt commented Apr 13, 2022

Fixes #1761

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@parsonsmatt
Copy link
Contributor Author

I've made a PR to add annotated-exception to STackage which is probably a prerequisite: commercialhaskell/stackage#6540

@@ -116,7 +117,7 @@ basicRunHandler rhe handler yreq resState = do
return (HCContent defaultStatus tc))
(\e ->
case fromException e of
Just e' -> return e'
Just (AnnotatedException _ e') -> return e'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a bit of commentary.

The instance Exception e => Exception (AnnotatedException e) has a slightly magical terrible horrifying unusual implementation of fromException.

The tests for this demonstrate the pattern, but tl;dr, the Exception instance here on AnnotatedException will work to catch both an AnnotatedException HandlerContents and a HandlerContents, which is then promoted to AnnotatedException mempty (hc :: HandlerContents).

@schoettl
Copy link
Contributor

Hi Matt,
Maybe it makes sense to clarify first, if this library can make it into Yesod as a dependency at the moment. Before you put too much work in this PR.
Don't get me wrong but I'm not sure about that. Because your library is pretty new, has no ratings yet and very few downloads on hackage. And maintainers are usually reluctant to add dependencies that are not (yet) popular, at least not if they are not adding a great value for many users, I think.

@parsonsmatt
Copy link
Contributor Author

I'm happy to let this sit until folks feel good integrating it. At the very least, the issue and PR presence is useful for folks that inevitably run into trouble when using annotated-exception with Yesod.

@snoyberg
Copy link
Member

snoyberg commented Dec 9, 2022

I’m OK with the change if you find this useful. But there are merge conflicts now, sorry for the delayed review

@lorenzo
Copy link

lorenzo commented Jun 9, 2023

@parsonsmatt do you need help finishing this? I'm interested

@parsonsmatt
Copy link
Contributor Author

I need to verify that the HCError <$> toHandlerError e does the right thing here - I suspect it may not have the right behavior

Copy link
Contributor Author

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a Yesod middleware at work that handles this unwrapping, and as long as it's the last middleware, everything works fine.

I want to write some tests and verify that this is properly integrated before recommending merging.

@@ -116,7 +117,7 @@ basicRunHandler rhe handler yreq resState = do
return (HCContent defaultStatus tc))
(\e ->
case fromException e of
Just e' -> return e'
Just (AnnotatedException _ e') -> return e'
Nothing -> HCError <$> toErrorHandler e)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, digging into this some more:

-- | Convert a synchronous exception into an ErrorResponse
toErrorHandler :: SomeException -> IO ErrorResponse
toErrorHandler e0 = handleAny errFromShow $
    case fromException e0 of
        Just (HCError x) -> evaluate $!! x
        _ -> errFromShow e0

So I think this function needs to also check for AnnotatedException - since the fromException e0 :: Maybe HCContents won't work if the underlying application code is calling throwWithCallStack.

-- | Generate an @ErrorResponse@ based on the shown version of the exception
errFromShow :: SomeException -> IO ErrorResponse
errFromShow x = do
  text <- evaluate (T.pack $ show x) `catchAny` \_ ->
          return (T.pack "Yesod.Core.Internal.Run.errFromShow: show of an exception threw an exception")
  return $ InternalError text

Fortunately this should work fine - perhaps we want to unwrap the AnnotatedException before showing it?

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.

Support annotated-exception
4 participants