Skip to content

Commit

Permalink
Allow SQLExecutor to execute within a parent transaction, safely.
Browse files Browse the repository at this point in the history
`SQLExecutor` was built to always require running outside of a
transaction, so that it could manage transactions itself. This was done
because some databases don't really support rollbacks of schema changes
properly within a transaction, and some operations in databases require
running outside a transaction (namely, SQLite renaming of primary keys).

However, it's not feasible to always run a database evolution fully
outside of a transaction. It won't happen, for instance, in Review Board
when installing/upgrading an extension, and it normally won't be a
problem.

Rather than outright fail, we now log a warning ahead of time. We then
check any grouping of SQL statements that are to be executed, check if
any require running outside a transaction (currnetly just the SQLite
primary key case), and then log and raise an exception to prevent
executing those statements.

This should cause statements to fail before any damage would be done,
invalidating the entire batch of evolutions.

Testing Done:
Unit tests pass.

Verified this fixed an issue with unit test runs in Djblets.

Reviewed at https://reviews.reviewboard.org/r/11267/
  • Loading branch information
chipx86 committed Nov 8, 2020
1 parent 4594ed6 commit aa9d70f
Showing 1 changed file with 33 additions and 2 deletions.
35 changes: 33 additions & 2 deletions django_evolution/utils/sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,19 @@

from __future__ import print_function, unicode_literals

import logging

from django.db import connections
from django.db.transaction import TransactionManagementError

from django_evolution.compat import six
from django_evolution.compat.db import atomic
from django_evolution.db import EvolutionOperationsMulti


logger = logging.getLogger(__name__)


class BaseGroupedSQL(object):
"""Base class for a grouped list of SQL statements.
Expand Down Expand Up @@ -90,8 +96,11 @@ def __enter__(self):
connection = self._connection
database = self._database

assert not connection.in_atomic_block, \
'SQLExecutor cannot be run inside a transaction.'
if (connection.in_atomic_block and
not connection.features.can_rollback_ddl):
logger.warning('Some database schema modifications may not be '
'able to be rolled back on this database if '
'something goes wrong.')

if not self._check_constraints:
self._constraints_disabled = \
Expand Down Expand Up @@ -173,6 +182,11 @@ def run_sql(self, sql, capture=False, execute=False):
list of unicode:
The list of SQL statements executed, if passing
``capture_sql=True``. Otherwise, this will just be an empty list.
Raises:
django.db.transaction.TransactionManagementError:
Could not execute a batch of SQL statements inside of an
existing transaction.
"""
qp = self._evolver_backend.quote_sql_param
cursor = self._cursor
Expand All @@ -185,6 +199,23 @@ def run_sql(self, sql, capture=False, execute=False):
batches = self._prepare_transaction_batches(
self._prepare_sql(sql))

if execute and self._connection.in_atomic_block:
# Check if there are any statements that must run outside of
# a transaction.
batches = list(batches)

for batch, use_transaction in batches:
if not use_transaction:
logging.error(
'Unable to execute the following SQL inside of a '
'transaction: %r',
batch)

raise TransactionManagementError(
'Unable to execute SQL inside of an existing '
'transaction. See the logging for more '
'information.')

for i, (batch, use_transaction) in enumerate(batches):
if execute:
if use_transaction:
Expand Down

0 comments on commit aa9d70f

Please sign in to comment.