Use transaction settings

[kawu]

An attempt to resolve #22 using a newtype wrapper over the internal DB effect stack. The wrapper itself has a MonadDB instance which avoids the re-implementation of withTransaction.

[kawu]

@arybczak I used a newtype wrapper to avoid re-implementing withTransaction. What do you think about this approach?

@kubek2k I reorganized the tests, would be great if you could check I haven't broken anything 🙏.

[arybczak]

Hmm, using a newtype, providing an instance for it and then simply delegating in the "canonical" instance is quite clever, I didn't think of this 👍 Too bad wrapping and unwrapping gets quite messy.

I think it's fine, since it also makes #27 easy to solve.

[arybczak]

Yeah, I don't think it's worth it, I'd suggest removing it.

[kawu]

Alright, removed. I added some helper functions (get, put, ...) instead.

[kubek2k]

I think we could also have an original test to prove that transaction isolation works between connections.

[arybczak]

Tests need sorting out anyway (e.g. if the new connection test fails, the table is not removed and subsequent test runs fail, void . runQuery is runQuery_), but let's do that in a separate PR.

[kawu]

Ok, let's create a ticket once this is merged and improve the tests in a separate PR.

[arybczak]

I added a commit that makes the interpreter take ConnectionSourceM just as runDBT does (needed in order for the runner to be able to take a connection source that e.g. uses a logger).

This needed a new utility function: haskell-effectful/effectful#73

I'll release a new effectful version after you'll have a look at what I did.

[kawu]

Nice! Things get a bit complicated so it took me some time to understand what's going on :). But it looks good to me.

In the process I realized we could (I think) also get away without raiseWith or even bracket and instead use raise and run separate internal state (as this is done in DBT I believe). Something like:

  withNewConnection action = do
    dbState0 <- get
    (result, dbState') <- DBEff . raise $ do
      PQ.withConnection (PQ.dbConnectionSource dbState0) $ \newConn -> do
        let transactionSettings = PQ.dbTransactionSettings dbState0
            dbState = mkDBState (PQ.dbConnectionSource dbState0) newConn transactionSettings
        runState dbState . unDBEff $ do
          handleAutoTransaction transactionSettings PQ.withTransaction' action
    result <$ put dbState'

I think it should do the same thing? Maybe avoiding to run an internal state is better, though, I'm not sure 🤔

[kawu]

Oh this version actually doesn't work, it makes the withNewConnection test fail. It looks like action still uses the external state, even though it's run within the context of a new, internal state. Is that normal? I will have to investigate that...

[arybczak]

Yes, that's normal (although somewhat subtle).

If you look at runEffectDB (we should rename it to runDB btw 🙂 ), the code that actually gets executed in tests when you call withNewConnection is the following:

WithNewConnection (action :: Eff localEs b) ->
      unDBEff . PQ.withNewConnection . DBEff $
        localSeqUnlift env $
          \unlift -> unlift action

so the action is run using the provided LocalEnv (which really is just an opaque wrapper for Env that's in use at the point where withNewConnection was called in tests). This LocalEnv somewhere inside has a handler for DB and the handler holds a reference to Env of the handler that has the original State created by runWithState.

Now, when you call raise in the handler, a new branch of Env that can be modified independently of the original Env of the handler is created (it has to be independent since if it wasn't, you could call raise and then overwrite the last effect on the stack with whatever you want). The LocalEnv doesn't see that independent branch anymore, hence it won't see the State that is created after the raise in that independent branch.

That's a minus of using the newtype and intermediate instance of DBEff - this is not immediately obvious at all until you mentally inline the definition of withNewConnection from the DBEff instance into the interpreter.

Does that make sense?

In fact, in the beginning I did what you did and was confused for a moment why it doesn't work, until I realized the above 🙂

[kawu]

If you look at runEffectDB (we should rename it to runDB btw slightly_smiling_face ), the code that actually gets executed in tests when you call withNewConnection is the following:

Ah yes, that's what I was missing, I guess that makes sense 🙂.

Still, this entire combination of raise and runState with unlifted action is somewhat confusing to me. Since the dbState provided to the internal runState is irrelevant (because action uses the state provided in LocalEnv), as in the following snippet:

  ...
  raise $ do
    ...
    runState dbState $ localSeqUnlift env $ \unlift -> unlift action

one could expect the following to have the same behavior:

  ...
  raise $ do
    ...
    runState () $ localSeqUnlift env $ \unlift -> unlift action

but the second version doesn't compile - the internal state might be irrelevant, but it must have the same type as the original state 🤯.

But I guess I will get used to that 🙂.

[arybczak]

Still, this entire combination of raise and runState with unlifted action is somewhat confusing to me. Since the dbState provided to the internal runState is irrelevant (because action uses the state provided in LocalEnv), as in the following snippet:

  ...
  raise $ do
    ...
    runState dbState $ localSeqUnlift env $ \unlift -> unlift action

one could expect the following to have the same behavior:

  ...
  raise $ do
    ...
    runState () $ localSeqUnlift env $ \unlift -> unlift action

but the second version doesn't compile - the internal state might be irrelevant, but it must have the same type as the original state exploding_head.

It doesn't, but for unrelated reasons. It doesn't compile because SuffixOf constraint won't let it. This was supposed to prevent doing a few nasty things, but it turns out it's too restrictive, which causes confusion (as you found out) and also it doesn't really behave as advertised because type families are weird. I fixed it in haskell-effectful/effectful#74 (and documented the "nasty things" in the haddock). After this is merged, your second version will work as well as this one:

  ...
  raise $ do
    ...
    localSeqUnlift env $ \unlift -> unlift action

[kawu]

Great, this is much more predictable indeed, thanks!

[kawu]

I've made some additional cosmetic changes (rename runEffectDB as runDB, use runQuery_ in tests). Are we ready to merge?

[arybczak]

Sure, feel free to merge 👍 I'll make another PR that gets rid of the git revision of effectful-core when I release 1.2.0.0 (in a few days).