Database transaction is not rollbacked

[sedlar]

When there are multiple queries executed in fastapi endpoint handler, the transaction rollback doesn't work properly.

@app.post("/notes/")
async def create_note(note: NoteIn):
    # WHEN We execute some query outside transaction
    await execute_some_query()

    # AND THEN execute some other inside transaction
    async with database.transaction():
        query = notes.insert().values(text=note.text, completed=note.completed)
        await database.execute(query)
        raise Exception()
    # The transaction is not rollbacked
    return {}

Full working example is available at https://github.com/sedlar/fastapi-transaction-issue.

It happens when there are queries executed outside of transaction and inside one. It also happens when there are two independent transactions.

When the transaction explicitly acquires connection, everything works as expected.

async with database.connection() as connection:
        async with connection.transaction():
                query = notes.insert().values(text=note.text, completed=note.completed)
                await database.execute(query)
                raise Exception()

This is regression against databases==0.2.6 and sqlalchemy==1.3.12

[aminalaee]

This isn't really related to FastAPI. So maybe you can rename the title.

I could reproduce this that if a query is run before a transaction, the transaction isn't rolled back properly.
I think this is related to this change:

https://github.com/encode/databases/blob/master/databases/core.py#L204

I guess rollback is called on the wrong connection, I'll take a look.

Simple way to reproduce this:

    await database.execute(User.select())

    async with database.transaction():
        await database.execute(User.insert().values({}))
        raise Exception()

The insert will be persisted here. But if the select query is omitted, it works ok.

[aminalaee]

@goteguru I think this is related to the parallel transactions #328 .

Here https://github.com/encode/databases/blob/master/databases/core.py#L206
If we avoid creating new connection and use existing connection, this issue will be fixed.
But that would revert the parallel transactions.

Any ideas?

[goteguru]

@aminalaee without parallel transactions a db abstraction layer is pretty much useless except for the most basic applications.

I'm not sure trying to hide the connection object from the user is a good design choice (looks simple at first glance, but the hassle and confusion later nullify the benefits in the long term). At least the transaction context manager should expose a connection object.

It's a bug nevertheless. The rolled back connection should be the same. Must check.

[tim-connolly]

I have not wrapped my head around the usage of contextvars in this scenario, but I just want to note that modifying the transaction method of Database as below (ie. removing the code dealing with contextvars) allows tests/test_databases.py::test_parallel_transactions to complete successfully:-

    def transaction(
        self, *, force_rollback: bool = False, **kwargs: typing.Any
    ) -> "Transaction":
        try:
            connection = self._connection_context.get()
            is_root = not connection._transaction_stack
            if is_root:
                get_conn = self._new_connection
            else:
                get_conn = self.connection
        except LookupError:
            get_conn = self.connection

        return Transaction(get_conn, force_rollback=force_rollback, **kwargs)

The following transaction rollback test also passes:-

@pytest.mark.parametrize("database_url", DATABASE_URLS)
@mysql_versions
@async_adapter
async def test_root_transaction(database_url):
    async with Database(database_url) as database:
        try:
            query = notes.insert().values(text="example1", completed=True)
            await database.execute(query)

            query = notes.select()
            results = await database.fetch_all(query=query)
            assert len(results) == 1

            transaction = await database.transaction()
            query = notes.update().values(text="update1")
            await database.execute(query)
            query = notes.update().values(text="update2")
            await database.execute(query)
            await transaction.rollback()

            query = notes.select()
            results = await database.fetch_all(query=query)
            assert len(results) == 1
            assert results[0]["text"] == "example1"

        finally:
            query = notes.delete()
            await database.execute(query)

[funkindy]

Got quite the same regression after 0.4.3 -> 0.5.5 upgrade.

[alex-pobeditel-2004]

Looks like it was fixed in 0.6.0 🎉