-
Notifications
You must be signed in to change notification settings - Fork 292
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
Add Acquire based interfaces for connections. #984
Conversation
The LTS-9 looks to be due to a different with being in scope/ I don't get the MySQL error in nightly, seems to be unrelated? |
LTS-9 failure is indeed due to ResourceT not being based on MySQL error in nightly appears to be a parse error in timestamps (which doesn't seem remotely related to anything I changed). |
I suspect that it's because the
A compatibility module that had some CPP and used |
Not sure that'd even work, because this is part of the visible persistent API, since there's no way to guarantee that every |
@parsonsmatt What would dropping lts-9 support entail, just deleting the lts-9 line from I'd like to see this and #983 merged and making it onto Hackage (although, since I'd also like to bump the SQLite version shipped, perhaps a new Hackage release should wait until I open a PR for that). |
@parsonsmatt Ping? I'm not really sure what dropping lts-9 support entails beyond deleting the tests from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Thanks!
-- on a connection that cannot be done within a transaction, such as VACUUM in | ||
-- Sqlite. | ||
-- | ||
-- @since 2.10.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3 docs
@@ -28,15 +86,16 @@ import Database.Persist.Sql.Raw | |||
runSqlPool | |||
:: (MonadUnliftIO m, BackendCompatible SqlBackend backend) | |||
=> ReaderT backend m a -> Pool backend -> m a | |||
runSqlPool r pconn = withRunInIO $ \run -> withResource pconn $ run . runSqlConn r | |||
runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runReaderT r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from runSqlConn
to runReaderT
drops the logic around transactions and exception handling. Why'd this get changed?
runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runReaderT r | |
runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runSqlConn r |
|
||
-- | Like 'runSqlPool', but supports specifying an isolation level. | ||
-- | ||
-- @since 2.9.0 | ||
runSqlPoolWithIsolation | ||
:: (MonadUnliftIO m, BackendCompatible SqlBackend backend) | ||
=> ReaderT backend m a -> Pool backend -> IsolationLevel -> m a | ||
runSqlPoolWithIsolation r pconn i = withRunInIO $ \run -> withResource pconn $ run . (\conn -> runSqlConnWithIsolation r conn i) | ||
runSqlPoolWithIsolation r pconn i = | ||
with (acquireSqlConnFromPoolWithIsolation i pconn) $ runReaderT r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - runSqlConn
does all the transaction stuff, so runReaderT
here voids that safety. (Man I really need to newtype
SqlPersistT ...)
with (acquireSqlConnFromPoolWithIsolation i pconn) $ runReaderT r | |
with (acquireSqlConnFromPoolWithIsolation i pconn) $ \conn -> runSqlConnWithIsolation r conn i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, no, I think I see what's going on here. You've moved that responsibility into the Acquire
interface. So runSqlConn
has it's own logic for handling exceptions, and that logic is now sort of duplicated into the Acquire
interface.
This is fine. Ignore the suggestions. I've reviewed the relevant code and as far as I can tell the behavior will be identical.
(restore $ connRollback conn' getter) | ||
restore $ connCommit conn' getter | ||
return x | ||
runSqlConn r conn = with (acquireSqlConn conn) $ runReaderT r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂 teaches me to review a PR before at least skimming it fully
@merijn OK, we've dropped support for Thanks! |
Add Acquire based interface for starting transactions and getting connections from a Pool for use in MonadResource instances that are not MonadUnliftIO, such as Conduit.
Turns out it's needed to do somethings using SQLite and there's no point in not exporting it and making users reimplement it themselves.
ed67e8a
to
341c3a8
Compare
@parsonsmatt hmmm, mongodb seems to fail for some reason? |
Which seems weird, as it works in your master and the failure seems to be caused by MonadFail, which I certainly didn't touch... |
yeah the |
I can make another PR with an upperbound on mongodb to get the tests passing and rebase on top of that... |
Released in |
Add Acquire based interface for starting transactions and getting
connections from a Pool for use in MonadResource instances that are not
MonadUnliftIO, such as Conduit.
Before submitting your PR, check that you've:
@since
declarations to the HaddockAfter submitting your PR: