begin_test_transaction behavior changed in Diesel 2.0

[urkle]

Setup

Versions

• Rust: 1.62.1
• Diesel: 2.0.0-rc.1
• Database: PG 11
• Operating System macOS 12.4

Feature Flags

• diesel: ["postgres", "r2d2", "chrono"]

Problem Description

In my 1.4.8 diesel usage I had a connection customizer that set the begin_test_transaction

Which I injected into my "test-only" connection pool which has a max-size of 1.

Then in my tests I was able to grab the DB connection perform inserts, then drop the connection and pass the pool to the code being tested (e.g. API requests) and it would use the SAME Connection & transaction block.

What are you trying to accomplish?

Happy testing with a single-connection pool.

What is the expected output?

The tests to run correctly.

What is the actual output?

The transaction is rolled back once the connection is returned to the pool and thus breaking the tests. as the code being tests creates a new separate transaction.

Are you seeing any additional errors?

Steps to reproduce

Connection customizer to call begin_test_transaction

    #[derive(Debug, Clone, Copy)]
    struct TestConnectionCustomizer;

    impl<C, E> CustomizeConnection<C, E> for TestConnectionCustomizer
    where
        C: diesel::Connection,
    {
        fn on_acquire(&self, conn: &mut C) -> Result<(), E> {
            conn.begin_test_transaction()
                .expect("Failed to start test transaction");

            Ok(())
        }
    }

Sample test

#[tokio::test]
async fn my_test() {
    let pool  = test_pool();
    let mut db = pool.get().unwrap();
    let user = UserFactory::default().insert(&mut db);
    drop(db);

    let obj = ObjectToTest::new(&pool);
    obj.some_method(user.id);

    let mut db = pool.get().unwrap();
    let updated_user = users::table.filter(users::id.eq(user.id)).first(&mut db).unwrap();
    drop(db);

    assert_eq!(user.name, new_user.name);
}

This code works under Diesel 1.4.8 but does not in Diesel 2.0.0-rc.1

Here is raw SQL queries that were run

• Diesel 1.4.8

execute <unnamed>: SET TIME ZONE 'UTC'
execute <unnamed>: SET CLIENT_ENCODING TO 'UTF8'
statement: BEGIN
execute <unnamed>: SELECT 1
execute <unnamed>: INSERT INTO "users" ("name") VALUES ($1) RETURNING "users"."id", "users"."name", "users"."created_at", "users"."updated_at"
execute <unnamed>: SELECT 1
execute <unnamed>: UPDATE "users" SET "name" = $1 WHERE "users"."id" = $2 RETURNING "users"."id", "users"."name", "users"."created_at", "users"."updated_at"
execute <unnamed>: SELECT 1
execute __diesel_stmt_0: SELECT "users"."id", "users"."name", "users"."created_at", "users"."updated_at" FROM "users" WHERE "users"."id" = $1 LIMIT $2
-- implicit close & rollback

• Diesel 2.0.0-rc.1

execute <unnamed>: SET TIME ZONE 'UTC'
execute <unnamed>: SET CLIENT_ENCODING TO 'UTF8'
statement: BEGIN
execute <unnamed>: SELECT 1
execute <unnamed>: INSERT INTO "users" ("name") VALUES ($1) RETURNING "users"."id", "users"."name", "users"."created_at", "users"."updated_at"
-- implicit close & rollback
execute <unnamed>: SET TIME ZONE 'UTC'
execute <unnamed>: SET CLIENT_ENCODING TO 'UTF8'
statement: BEGIN
execute <unnamed>: SELECT 1
execute <unnamed>: UPDATE "users" SET "name" = $1 WHERE "users"."id" = $2 RETURNING "users"."id", "users"."name", "users"."created_at", "users"."updated_at"
-- implicit close & rollback
execute <unnamed>: SET TIME ZONE 'UTC'
execute <unnamed>: SET CLIENT_ENCODING TO 'UTF8'
statement: BEGIN
execute <unnamed>: SELECT 1
execute __diesel_stmt_0: SELECT "users"."id", "users"."name", "users"."created_at", "users"."updated_at" FROM "users" WHERE "users"."id" = $1 LIMIT $2
-- implicit close & rollback

Checklist

• I have already looked over the issue tracker and the disussion forum for similar possible closed issues.

• This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)
• This issue can be reproduced without requiring a third party crate

[weiznich]

Thanks for reporting this. This I would not consider this to be a bug, as we explicitly changed the behaviour for returning connection to the pool to consider connections with an open transaction as broken. See #2935 for details. This was a long standing bug in diesel that a pool could contain connections with open transactions, which could cause all kinds of surprising behaviour. I'm opposed to roll back that change as it improves the correctness of code in many situations.
I think we should at least document this change more explicitly in our migration guide. If someone has suggestions on how to improve this situation without degrading the other use case, feel free to reach out.

[urkle]

@weiznich I do agree that production use should rollback on pool returning is the best default, sane, and safe behavior.

The question then is there a way I can "reconfigure" a pool (or create a pool-like object) to behave in a way that allows only 1 connection that maintains a transaction for the duration of that pool.

As right now I will be unable to move to Diesel 2 because of this issue as tests will no longer work in this mode. (I was really wanting to gain access to the alias! macro!)

Would you be opposed to a PR adding a "mock" testing pool? The real issue comes down when I am testing APIs that accept a pool not a single connection.. (in my case warp routes, or async_graphql Objects, loaders, and mutations, etc). All things that cannot accept a single connection. The way my code is setup everything access a New Type that I have for my DatabasePool so 99% of my code has no idea what a pool actually is, so long as it provides get()?. (which I would assume most code-bases would behave similarly).

From looking at the code it appears the only thing I need to do is provide an alternate ConnectionManager that doesn't perform the is_broken() checks.. This method, however, seems badly named as the implementations are currently checking for outstanding or failed . transactions. It would be nicer if there was a specific method giving me the transactions state, thus for this "TestConnectionManager" I could only claim "is_broken" when the transaction manager is in an error state only.

[weiznich]

I've opened #3254 with an potential fix for this.