Fix #3252

[weiznich]

This commit fixes an issue where test transactions caused connection to
be marked as broken when returned to a r2d2 connection pool. It is a
quite common pattern to create a connection pool with 1 connection, and
call begin_test_transaction on the connection before putting it into
the pool. This allows users to easily test applications using connection
pools without modifying an existing schema. The recent changes to our
transaction manager implementation + our r2d2 implementation broke this
pattern.

This commit introduces an additional flag inside of the transaction
manager state, which tracks whether begin_test_transaction was called
or not. This flag is later used to decide whether a connection should be
considered broken or not.

In addition this commit centralises the is_broken implementation as
part of the transaction manager itself + marks
transaction_manager_status_mut as part of the unstable public
API (this method is only useful for third party connection or
transactionmanager implementations)

[Ten0]

It's generally better to not complexify general code with test code so I would tend to not favor this approach if possible.

How about an approach where we would create a TestTransactionConnection<T> wrapper around any R2D2Connection that would also have R2D2Connection implemented but would automatically open a test transaction and override the is_broken behavior and prevent from touching transaction status (in particular prevent from committing)?

Or how about we propose an update to CustomizeConnection so that it can hook on is_valid and/or (take_from_pool and release_to_pool), possibly opening transactions and rolling them back everytime they are taken from the pool/released?

Otherwise this looks correct. :)

[weiznich]

To be clear about this upfront: I do not claim that the presented solution is the best way to solve this issue. Everything that follows is just trying to collect some potential problems with other suggested variants.

The largest issue with the presented approach is as already pointed out the fact that it "infects" internal code with test-specific stuff.

How about an approach where we would create a TestTransactionConnection wrapper around any R2D2Connection that would also have R2D2Connection implemented but would automatically open a test transaction and override the is_broken behavior and prevent from touching transaction status (in particular prevent from committing)?

For this approach I see a major usability issue, as that requires users to use different pool/connection types based on whether they run tests or not. Technically that could be solved by accepting a &mut impl LoadConnection<Backend = Pg> or similar as function argument, but I assume that most existing code uses a fixed type at this position. Introducing a "special" connection type for this would require changing the type to either the generic variant (may affect compile times?) or conditionally on whether tests are run or not (hard to do in a multi-crate setup). The same argument more or less applies to the suggested alternative ConnectionManager impl specifically tailored to tests as suggested in the issue as well.

Or how about we propose an update to CustomizeConnection so that it can hook on is_valid and/or (take_from_pool and release_to_pool), possibly opening transactions and rolling them back everytime they are taken from the pool/released

This might be an solution, but would require changes to upstream code. I'm not sure if they are open to such a change. Also what would an API look like, that would allow to conditionally disable the has_broken check?

[Ten0]

I do not claim that the presented solution is the best way to solve this issue. Everything that follows is just trying to collect some potential problems with other suggested variants.

I often forget to make such disclaimers 😁
Please be assured I'm also always very open 🤗

that requires users to use different pool/connection types based on whether they run tests or not

In our codebase, our functions take &mut PgConnection (which is kind-of-forced by the transaction api), but straight-out-of-the-pool we get a wrapped connection, which we DerefMut on to call these functions (or when we use the transaction api).

If people do that as well then as long as we impl DerefMut<Target = C> for TestTransactionConnection<C> then couldn't users still test their app functions in the same way?

I'm not sure if they are open to such a change.

Let's know. sfackler/r2d2#130

Also what would an API look like, that would allow to conditionally disable the has_broken check?

Hmm I think if we call the release_to_pool hook before checking for has_broken that would allow the caller to attempt closing any open transactions, releasing an effectively non-broken connection to the pool (also releasing locks on the DB and not polluting other tests) so we wouldn't need to really hook is_broken.

[urkle]

@Ten0 @weiznich I'm very appreciative of the active discussion around this issue and finding solutions. :)

I'm thinking the "CustomizeConnection" is the best approach as that would also allow for handling the integration test case more easily where you can't "swap out" the structs used in your crate. (mind you currently my code-base lacks integration tests so this isn't an immediate need for me but we should definitely account for it as others may)

[weiznich]

Hmm I think if we call the release_to_pool hook before checking for has_broken that would allow the caller to attempt closing any open transactions, releasing an effectively non-broken connection to the pool (also releasing locks on the DB and not polluting other tests) so we wouldn't need to really hook is_broken.

I'm not sure if that solves the problem outlined in #3252, as that specifically would require to leave the connection open even if it's checked in to the pool again.

[Ten0]

I'm not sure if that solves the problem outlined in #3252, as that specifically would require to leave the connection open even if it's checked in to the pool again.

I imagined that opening the transaction automatically and rolling it back automatically at every fetch/release from/to the pool would also be fit for testing purposes.
@urkle wdyt?

[jtgeibel]

I imagined that opening the transaction automatically and rolling it back automatically at every fetch/release from/to the pool would also be fit for testing purposes.

I don't think that would work for our crates.io tests. Some of our tests require a pool with a single connection, where the transaction persists even if the connection is returned to the pool and then taken again elsewhere in the codebase. If the transaction is automatically rolled back, then our (test) background worker would never see the jobs enqueued earlier (in the now rolled back transaction).

[weiznich]

@jtgeibel Thanks for reaching out here. That's what I had expected as well. Our internal code base uses a similar patterns. This PR would allow this pattern in the future, while other solutions likely require more changes.

That written: I definitively see the point that it would be great to have more hook points in r2d2. That could be implemented as well in parallel in my opinion as those features are independent.

[urkle]

I imagined that opening the transaction automatically and rolling it back automatically at every fetch/release from/to the pool would also be fit for testing purposes.

@Ten0 that was the start of opening the original bug report as Diesel 2.0 changed the behavior to roll it back when returning to the pool which breaks 99% of my tests as they are testing code that only can accept a Pool not a DB connection. Thus in my tests I need to

1. grab a connection
2. create factories
3. release the connection (if I don't do this then the code under test cannot obtain the connection)
4. run the tests passing the cloned pool to the object under test
5. grab a connection
6. inspect the changed models (assert!s)
7. fin!

for examples

#[tokio::test]
async fn  updated_user() {
   let pool = test_pool();
   
   let db = pool.get().expect("failed to obtain DB connection");
   let user = UserFactory::default().insert(&db);
   drop(db);

   let schema = build_schema(&pool); // this method internally creates a new async-graphql schema for the object under test and binds a clone of the pool into it's context.
   let response = build_request(...).run(&schema).await;
  
   let db = pool.get().expect("failed to obtain DB connection");
   let record = users::table
           .filter(users::id.eq(user.id))
           .first::<User>(&db)
           .unwrap();
    drop(db);

     // assertions go here against user vs record etc..
}

[weiznich]

@diesel-rs/reviewers Any final opinion here? Otherwise I suggest to merge this PR next week.