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

lift IO exceptions from PG to MonadError #28

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/PgNamed.hs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ module PgNamed
, withNamedArgs
) where

import Control.Exception (try)
import Control.Monad (void)
import Control.Monad.Except (MonadError (throwError))
import Control.Monad.IO.Class (MonadIO (liftIO))
Expand Down Expand Up @@ -83,6 +84,8 @@ data PgNamedError
| PgNoNames PG.Query
-- | Query contains an empty name.
| PgEmptyName PG.Query
-- | Query failed to evaluate due to database error.
| PgSqlError PG.SqlError
deriving stock (Eq)


Expand All @@ -96,6 +99,8 @@ instance Show PgNamedError where
"Query has no names but was called with named functions: " ++ BS.unpack q
PgEmptyName (PG.Query q) ->
"Query contains an empty name: " ++ BS.unpack q
PgSqlError err ->
"Query failed with SQL exception: " ++ show err

-- | Checks whether the 'Name' is in the list and returns its parameter.
lookupName :: Name -> [NamedParam] -> Maybe PG.Action
Expand Down Expand Up @@ -204,7 +209,7 @@ queryNamed
-> m [res] -- ^ Resulting rows
queryNamed conn qNamed params =
withNamedArgs qNamed params >>= \(q, actions) ->
liftIO $ PG.query conn q (toList actions)
handleIO $ PG.query conn q (toList actions)

{- | Queries the database with a given row parser, 'PG.Query', and named parameters
and expects a list of rows in return.
Expand Down Expand Up @@ -249,7 +254,7 @@ queryWithNamed
-> m [res] -- ^ Resulting rows
queryWithNamed rowParser conn qNamed params =
withNamedArgs qNamed params >>= \(q, actions) ->
liftIO $ PG.queryWith rowParser conn q (toList actions)
handleIO $ PG.queryWith rowParser conn q (toList actions)

{- | Modifies the database with a given query and named parameters
and expects a number of the rows affected.
Expand All @@ -270,7 +275,7 @@ executeNamed
-> m Int64 -- ^ Number of the rows affected by the given query
executeNamed conn qNamed params =
withNamedArgs qNamed params >>= \(q, actions) ->
liftIO $ PG.execute conn q (toList actions)
handleIO $ PG.execute conn q (toList actions)

{- | Same as 'executeNamed' but discard the nubmer of rows affected by the given
query. This function is useful when you're not interested in this number.
Expand Down Expand Up @@ -302,3 +307,12 @@ withNamedArgs qNamed namedArgs = do
Right r -> pure r
args <- namesToRow names namedArgs
pure (q, args)


handleIO :: (MonadIO m, WithNamedError m) => IO a -> m a
handleIO io = do
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'd rather call this something more specific like handleSqlError

Copy link
Member Author

Choose a reason for hiding this comment

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

keep in mind this also lifts IO to MonadIO m. I used handle as it's also function from Control.Exception (flip catch) as it sounded better than catchIO. With this in mind how about calling this handlePgIO ?

Copy link
Contributor

@jhrcek jhrcek Aug 17, 2020

Choose a reason for hiding this comment

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

I'd like to see the word exception/error in function name for clarity, so handleIO/handlePgIO don't cut it for me :-)
But it doesn't matter much, since it's not exposed anyway. How about rethrowSqlError? The fact that it lifts from IO is already visible from the type. The fact that it does something with exceptions is not.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. well in my mind handle already refers to exception handling but I agree it's not that explicit. We can change this to rethrowSqlError to be explicit.

res <- liftIO $ try io
case res of
Right a -> pure a
Left err -> throwError $ PgSqlError err
{-# INLINE handleIO #-}