lift IO exceptions from PG to MonadError

[turboMaCk]

This is proof of concept solution for #27

Unfortunately this is hard to test using current testing approach which seems not to be really queering any real DB table even though it requires database connection.

$ psql -h localhost -p 5432 -U postgres -n pg_named
psql (11.6, server 10.5)
Type "help" for help.

pg_named=# \dt
Did not find any relations.
pg_named=# 

[jhrcek]

Please make sure to handle all IO functions that can throw SQLError

[jhrcek]

There are other places can potentially throw SQLError:
E.g. search for PG.queryWith, PG.query

[turboMaCk]

This can probably become helper function that replaces all usages of liftIO within the module.

[jhrcek]

Looks ok to me. But let's please have @arbus take a look if he had a different error handling strategy in mind.

[jhrcek]

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

[turboMaCk]

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 ?

[jhrcek]

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.

[turboMaCk]

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.

[oishikg]

@turboMaCk and @jhrcek, this looks good to me! Are we waiting on @arbus' approval to merge this PR?

[jhrcek]

Definitely this is relatively big API change which will impact all users of this package, so let's wait with merging it.

[jhrcek]

@turboMaCk before you dive deeper into this, be aware that postgresq-simple can throw bunch of other execeptions:
https://hackage.haskell.org/package/postgresql-simple-0.6.4/docs/Database-PostgreSQL-Simple.html#g:13

• SqlError
• QueryError
• FormatError
• ResultError

I'm not even sure if this package should be responsible for making the decision as to which errors are "more important" than others.

[turboMaCk]

well it should not it should just provide unified interface for handling these. Right now the confusing part is that functions of this library have MonadError PgNamedError => but still throw in IO. I think we will need to just expose all of these.

[jhrcek]

I don't know. The haddocs say

data PgNamedError

PostgreSQL error type for named parameters.

It's true that there are 2 ways that the functions can fail (IO exceptions vs. MonadError) but adding the constructors to PgNamedError feels to me like tying the api too much to the underlying postgresql-simple library and it feels to me like it's out-of-scope of what this library is trying to do.

Although I like the idea of unifying the API, I'm a bit afraid that with such a change we'd be opening a can of worms.

Also there are further exceptions IOExceptions that postgresql-libpq can throw (example from pi) - how would you justify that we're catching the more higher level exceptions from postgresql-simple, but no the low-level exceptions from postgresql-libpq?

[turboMaCk]

If it would be up to me I would not design this library with MonadError and lifting of io. That way consumer of the lib would have control over how this is done. Anyway since we already do it we need to take care of consequences I think.

This PR was just proof of concept. How this should probably look like is that PgNamedError would be only one of the error types. We would need to have another type that would put together all different types of errors which can be thrown in IO and lift them. That is kind of unpleasant task since it means building typed api around untyped 3rd party code. I'm not even sure if it would be possible to cover everything that can possibly happen in IO.

Anyway current situation is that one type of programmer error (trouble with named arguments) is being thrown in MonadError but errors that even more likely deserve exception handling (like uniqness violation for instance) are thrown in IO which makes them hard to handle. It feels wrong to make error that is unlikely to be handled be easily handable while obscuring exceptions that almost every production application will need to handle in liftedIO.

[turboMaCk]

I'm a bit afraid that with such a change we'd be opening a can of worms.

I'm of an opinion that that can is already open since this lib already do liftIO internally. The only alternative is to put big warnings everywhere that there might be exceptions happening and that user will need to implement functions that could catch these io exceptions in lifted io we create.

[jhrcek]

errors that even more likely deserve exception handling (like uniqness violation for instance) are thrown in IO which makes them hard to handle

Even though the package is using MonadError it's still possible for users to catch those exceptions from using stuff from lifted-base or unliftio. It's not like this package's usage of MonadError is preventing those ways of dealing with exceptions.

[turboMaCk]

I never said it makes it impossible. I said it makes it harder and that it obscures exceptions by lifting which I think it does. The fact that additional errors are using MonadError and postgresql-simple ones are lifted makes it harder to do error handling correctly. I think we should fix that and make it easier to write error handlers. That way all downstream projects will have simpler error handling. At the moment every downstream project has to handle this in some way (like using functions from lifted-base) or just give up. The Golden Rule of SW Quality

[jhrcek]

Marking as request change so it's not merged without wider discussion.

[oishikg]

As I understand, the main issue (as @turboMaCk points out) is:

• Currently, only exceptions that this library has control over (no names param, named param not specified, empty named param) are lifted into the MonadError instance
• On the other hand, errors propagating from the postgres-simple library are left unhandled, and so are thrown by the wrapper functions in this library in IO, leading an inconsistent and confusing API for users of this library

@turboMaCk 's solution is to add another constructor to the error type to handle the postgres-simple errors. However, as @jhrcek correctly points out, this doesn't cover all the possible errors that might be thrown by the library, since this solution only accounts for one of the types of errors (SqlError).

Wouldn't the simplest solution be to have the fourth constructor be something like | PgSimpleError e, where e can be any error type represented in the postgres-simple library? That way we wouldn't have to bother about the underlying error definitions of the library, but simply lift whatever error that propagates through to this library.

Regarding the concern around the API change, this will definitely be a breaking change, so we will need to have more discussions around this.

[jhrcek]

PgSimpleError e, where e can be any error type represented in the postgres-simple library

This wouldn't be that simple to implement, because as far as I understand there is no single exception type in postgresql-simple that would be the parent of the hierarchy of exceptions that can be thrown by that library, so we would have to list all the types throwable by that lib somewhere.

Also I would recommend reading https://www.fpcomplete.com/blog/2016/11/exceptions-best-practices-haskell/
in particular the "The bad ExceptT IO anti-pattern" part to see why I'm not fan of moving more exceptions to ExceptT (MonadError)

[turboMaCk]

I also recommend that article. The problem is that we're already doing this by introducing named errors as an ExceptT and lifting io. Exceptions from postgresql-simple should be easy to catch because it's important to handle them correctly.

[turboMaCk]

This is now old and rotted. I'm making a step back and first creating an issue to discuss the problem #40