Database Cleaning After Tests: A Comprehensive Guide

by Admin 53 views
Database Cleaning After Tests: A Comprehensive Guide

Hey guys! Ever run into issues with your database after running tests in Invenio? It's a common headache, especially with SQLAlchemy and nested transactions. Let's dive into the nitty-gritty of how to properly clean your database after tests, ensuring a smooth development process.

Understanding the Bug: SQLAlchemy Transactions Inside Invenio

The core of the issue lies in how SQLAlchemy handles transactions, particularly nested ones, within the Invenio codebase. This becomes especially apparent when using pytest-invenio. So, let's break it down.

Different Behavior of Nested Transactions in SQLAlchemy 2.0

In SQLAlchemy 2.0, there's a significant change in how nested transactions and savepoints are managed compared to the 1.x series. Consider this scenario:

# starting nested transaction
db.session.begin_nested()
blah = Blah()
db.session.add(blah)
db.session.commit()

...
# rolling back outer transaction
db.session.rollback()

db.session.query(Blah).all()
# returns [blah] in SQLAlchemy 2.0

In SQLAlchemy 2.0, the documentation clearly states that when using a SAVEPOINT via Session.begin_nested() or Connection.begin_nested(), the transaction object returned must be used to commit or rollback the SAVEPOINT. Calling Session.commit() or Connection.commit() will always commit the outermost transaction. This is a key difference from SQLAlchemy 1.x.

The correct way to handle the above scenario in SQLAlchemy 2.0 is:

savepoint = db.session.begin_nested()
blah = Blah()
db.session.add(blah)
savepoint.commit()  # commit the savepoint, not the whole session
...
db.session.rollback()  # rollback the outer transaction
db.session.query(Blah).all()  # returns []

This distinction is crucial for understanding why your tests might be leaving unwanted data in the database.

Transactions in Tests

The pytest-invenio fixture, specifically the db fixture, creates a nested transaction for each test and rolls it back at the end. This is designed to keep your tests isolated and prevent data bleeding between tests. Here’s the relevant snippet from the pytest-invenio fixture:

def db():
    ...
    session.begin_nested()
    ...
    yield database
    ...
    session.rollback()

However, the problem arises when the code being tested directly calls db.session.commit() or db.session.rollback(). Because the db fixture creates a nested transaction, these direct calls commit or rollback the outer transaction, effectively nullifying the intended rollback at the end of the test. This leads to objects persisting in the database that should have been removed, causing unexpected and potentially misleading test results. It's like thinking you've cleaned your room, but you've only tidied up the surface!

Problematic Places in the Codebase

Several areas in the Invenio codebase are prone to this issue. Let's highlight a few.

UnitOfWork Commit/Rollback

The UnitOfWork implementation in invenio-db directly calls db.session.commit() and db.session.rollback() on the session object. This makes it incompatible with nested transactions, inevitably polluting the database during tests. The UnitOfWork pattern is intended to manage changes to the database in a controlled manner, but these direct calls bypass the nested transaction context.

Explicit Invocation of db.session.commit() and db.session.rollback()

A significant number of files within the Invenio codebase explicitly call db.session.commit(). A quick search reveals numerous instances where this occurs:

18:15 $ find . -name "*.py" | grep -v ".venv" | grep -v "tests" |
        xargs grep "db.session.commit" |
        egrep ".py:[ \t]*db.session.commit" |
        grep -v migrat | grep -v pytest | sort -u

./dependent/invenio-access/invenio_access/cli.py: db.session.commit()
./dependent/invenio-accounts/invenio_accounts/admin.py: db.session.commit()
./dependent/invenio-accounts/invenio_accounts/cli.py: db.session.commit()
./dependent/invenio-accounts/invenio_accounts/tasks.py: db.session.commit()
./dependent/invenio-accounts/invenio_accounts/tasks.py: db.session.commit()  # Commit after each batch
./dependent/invenio-accounts/invenio_accounts/views/rest.py: db.session.commit()
./dependent/invenio-accounts/invenio_accounts/views/security.py: db.session.commit()
./dependent/invenio-app-rdm/invenio_app_rdm/fixtures/pages.py: db.session.commit()
./dependent/invenio-app-rdm/invenio_app_rdm/tasks.py: db.session.commit()
./dependent/invenio-communities/invenio_communities/fixtures/tasks.py: db.session.commit()
./dependent/invenio-files-rest/invenio_files_rest/cli.py: db.session.commit()
./dependent/invenio-files-rest/invenio_files_rest/tasks.py: db.session.commit()
./dependent/invenio-files-rest/invenio_files_rest/views.py: db.session.commit()
./dependent/invenio-github/invenio_github/api.py: db.session.commit()
./dependent/invenio-github/invenio_github/oauth/handlers.py: db.session.commit()
./dependent/invenio-github/invenio_github/receivers.py: db.session.commit()
./dependent/invenio-github/invenio_github/tasks.py: db.session.commit()
./dependent/invenio-github/invenio_github/views/github.py: db.session.commit()
./dependent/invenio-jobs/invenio_jobs/services/scheduler.py: db.session.commit()
./dependent/invenio-jobs/invenio_jobs/tasks.py: db.session.commit()
./dependent/invenio-oaiserver/invenio_oaiserver/__init__.py: db.session.commit()
./dependent/invenio-oauth2server/invenio_oauth2server/cli.py: db.session.commit()
./dependent/invenio-oauth2server/invenio_oauth2server/provider.py: db.session.commit()
./dependent/invenio-oauth2server/invenio_oauth2server/views/settings.py: db.session.commit()
./dependent/invenio-oauthclient/invenio_oauthclient/handlers/authorized.py: db.session.commit()
./dependent/invenio-oauthclient/invenio_oauthclient/handlers/base.py: db.session.commit()
./dependent/invenio-oauthclient/invenio_oauthclient/handlers/rest.py: db.session.commit()
./dependent/invenio-oauthclient/invenio_oauthclient/handlers/token.py: db.session.commit()
./dependent/invenio-oauthclient/invenio_oauthclient/handlers/ui.py: db.session.commit()
./dependent/invenio-oauthclient/invenio_oauthclient/models.py: db.session.commit()
./dependent/invenio-oauthclient/invenio_oauthclient/views/client.py: db.session.commit()
./dependent/invenio-pidstore/invenio_pidstore/cli.py: db.session.commit()
./dependent/invenio-records-resources/invenio_records_resources/services/files/tasks.py: db.session.commit()
./dependent/invenio-records-rest/invenio_records_rest/views.py: db.session.commit()
./dependent/invenio-records/invenio_records/admin.py: db.session.commit()
./dependent/invenio-search-ui/invenio_search_ui/__init__.py: db.session.commit()
./dependent/invenio-users-resources/invenio_users_resources/services/domains/tasks.py: db.session.commit()
./dependent/invenio-vocabularies/invenio_vocabularies/contrib/subjects/services.py: db.session.commit()
./dependent/invenio-webhooks/invenio_webhooks/models.py: db.session.commit()
./dependent/invenio-webhooks/invenio_webhooks/views.py: db.session.commit()
./invenio-db/invenio_db/utils.py: db.session.commit()

These direct calls bypass the intended nested transaction behavior, leading to the aforementioned data persistence issues.

Explicit Invocation in Test Fixtures

To compound the problem, some tests and test fixtures also explicitly invoke db.session.commit() and db.session.rollback(), further disrupting the intended transaction management.

Fixing the Problem

So, what can we do about it? There are a couple of approaches, each with its own trade-offs.

The "Proper" Solution

The ideal solution would be to refactor the entire Invenio codebase to be fully aware of nested transactions. This would involve ensuring that every instance of db.session.commit() and db.session.rollback() intelligently handles whether it’s inside a nested transaction or not, and calls the appropriate commit/rollback method.

This could be achieved by:

  1. Fixing the UnitOfWork and using it consistently throughout the codebase.
  2. Updating every direct call to db.session.commit() or db.session.rollback() to handle nested transactions correctly.

While the first option offers a more structured approach, it requires a significant refactor. The second option, while more direct, could lead to a more complex and harder-to-maintain codebase. Both are massive undertakings.

Partial Solutions for Tests

Given the scale of a full refactor, a more practical approach might be to focus on a partial solution that primarily addresses the issue in the context of tests. The goal here is to modify the pytest-invenio db fixture to handle nested transactions correctly, without necessarily changing the core behavior of the Invenio codebase in normal operation.

One such solution involves wrapping the db.session object within the db fixture with a handler that ensures db.session.commit() and db.session.rollback() calls are always invoked within the nested transaction created by the fixture. This approach is implemented in PR #129. You can see the results in a table at invenio patch verifier.

Subsequent Problems with This Approach

This approach isn't without its drawbacks. Some parts of the Invenio code expect certain types to be passed to the database or rely on values that are only available after a full commit. For example, consider this snippet from invenio-banners:

Banner(start_datetime = date(2022, 7, 20).strftime("%Y-%m-%d %H:%M:%S"))

Here, start_datetime is expected to be a DateTime field. Normally, this works because the database handles the conversion during the commit, and the Banner object is reloaded from the database, ensuring start_datetime is a proper DateTime object.

However, with nested transactions, the object isn't reloaded from the database, and the field remains a string, leading to errors later on, particularly during marshmallow dumping. It’s a subtle but significant difference.

An Alternative Partial Solution

Another approach is to leverage the fact that tests typically don't modify initial data (i.e., data created in module fixtures using database, not db). This allows for a simpler cleanup strategy:

  1. Before returning from the db fixture, enumerate all rows in all tables and store them in memory.
  2. After each test, purge the entire database, keeping only the rows that exactly match the initial rows stored in memory.

This method is implemented in PR #130, with results available at invenio patch verifier.

Important Note: The cleanup needs to be done in a new connection because the previous connection might be in a broken state after the test. Even with this precaution, there can still be issues. For instance, in invenio-pages, tests may deadlock during cleanup (outside of the cleanup code itself).

Steps to Reproduce

To reproduce the issue, refer to the provided test logs and examine the behavior of database interactions within the tests.

By understanding these nuances, you can better navigate the challenges of database cleaning after tests in Invenio and ensure your tests are reliable and your development process is smooth. Happy coding!