Fixed #30023 -- Prevented SQLite schema alterations while foreign key checks are enabled.

Prior to this change foreign key constraint references could be left pointing
at tables dropped during operations simulating unsupported table alterations
because of an unexpected failure to disable foreign key constraint checks.

SQLite3 does not allow disabling such checks while in a transaction so they
must be disabled beforehand.

Thanks ezaquarii for the report and Carlton and Tim for the review.
This commit is contained in:
Simon Charette 2018-12-07 19:55:47 -05:00
parent a394289b58
commit 315357ad25
8 changed files with 89 additions and 42 deletions

View File

@ -247,11 +247,13 @@ class DatabaseWrapper(BaseDatabaseWrapper):
self.connection.isolation_level = level self.connection.isolation_level = level
def disable_constraint_checking(self): def disable_constraint_checking(self):
if self.in_atomic_block: with self.cursor() as cursor:
# sqlite3 cannot disable constraint checking inside a transaction. cursor.execute('PRAGMA foreign_keys = OFF')
return False # Foreign key constraints cannot be turned off while in a multi-
self.cursor().execute('PRAGMA foreign_keys = OFF') # statement transaction. Fetch the current state of the pragma
return True # to determine if constraints are effectively disabled.
enabled = cursor.execute('PRAGMA foreign_keys').fetchone()[0]
return not bool(enabled)
def enable_constraint_checking(self): def enable_constraint_checking(self):
self.cursor().execute('PRAGMA foreign_keys = ON') self.cursor().execute('PRAGMA foreign_keys = ON')

View File

@ -19,8 +19,15 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
def __enter__(self): def __enter__(self):
# Some SQLite schema alterations need foreign key constraints to be # Some SQLite schema alterations need foreign key constraints to be
# disabled. Enforce it here for the duration of the transaction. # disabled. Enforce it here for the duration of the schema edition.
self.connection.disable_constraint_checking() if not self.connection.disable_constraint_checking():
raise NotSupportedError(
'SQLite schema editor cannot be used while foreign key '
'constraint checks are enabled. Make sure to disable them '
'before entering a transaction.atomic() context because '
'SQLite3 does not support disabling them in the middle of '
'a multi-statement transaction.'
)
self.connection.cursor().execute('PRAGMA legacy_alter_table = ON') self.connection.cursor().execute('PRAGMA legacy_alter_table = ON')
return super().__enter__() return super().__enter__()

View File

@ -16,3 +16,6 @@ Bugfixes
* Fixed a schema corruption issue on SQLite 3.26+. You might have to drop and * Fixed a schema corruption issue on SQLite 3.26+. You might have to drop and
rebuild your SQLite database if you applied a migration while using an older rebuild your SQLite database if you applied a migration while using an older
version of Django with SQLite 3.26 or later (:ticket:`29182`). version of Django with SQLite 3.26 or later (:ticket:`29182`).
* Prevented SQLite schema alterations while foreign key checks are enabled to
avoid the possibility of schema corruption (:ticket:`30023`).

View File

@ -15,3 +15,6 @@ Bugfixes
* Fixed a schema corruption issue on SQLite 3.26+. You might have to drop and * Fixed a schema corruption issue on SQLite 3.26+. You might have to drop and
rebuild your SQLite database if you applied a migration while using an older rebuild your SQLite database if you applied a migration while using an older
version of Django with SQLite 3.26 or later (:ticket:`29182`). version of Django with SQLite 3.26 or later (:ticket:`29182`).
* Prevented SQLite schema alterations while foreign key checks are enabled to
avoid the possibility of schema corruption (:ticket:`30023`).

View File

@ -2,7 +2,7 @@ import re
import threading import threading
import unittest import unittest
from django.db import connection from django.db import connection, transaction
from django.db.models import Avg, StdDev, Sum, Variance from django.db.models import Avg, StdDev, Sum, Variance
from django.db.models.fields import CharField from django.db.models.fields import CharField
from django.db.utils import NotSupportedError from django.db.utils import NotSupportedError
@ -16,22 +16,6 @@ from ..models import Author, Item, Object, Square
class Tests(TestCase): class Tests(TestCase):
longMessage = True longMessage = True
def test_autoincrement(self):
"""
auto_increment fields are created with the AUTOINCREMENT keyword
in order to be monotonically increasing (#10164).
"""
with connection.schema_editor(collect_sql=True) as editor:
editor.create_model(Square)
statements = editor.collected_sql
match = re.search('"id" ([^,]+),', statements[0])
self.assertIsNotNone(match)
self.assertEqual(
'integer NOT NULL PRIMARY KEY AUTOINCREMENT',
match.group(1),
'Wrong SQL used to create an auto-increment column on SQLite'
)
def test_aggregation(self): def test_aggregation(self):
""" """
Raise NotImplementedError when aggregating on date/time fields (#19360). Raise NotImplementedError when aggregating on date/time fields (#19360).
@ -82,6 +66,52 @@ class SchemaTests(TransactionTestCase):
available_apps = ['backends'] available_apps = ['backends']
def test_autoincrement(self):
"""
auto_increment fields are created with the AUTOINCREMENT keyword
in order to be monotonically increasing (#10164).
"""
with connection.schema_editor(collect_sql=True) as editor:
editor.create_model(Square)
statements = editor.collected_sql
match = re.search('"id" ([^,]+),', statements[0])
self.assertIsNotNone(match)
self.assertEqual(
'integer NOT NULL PRIMARY KEY AUTOINCREMENT',
match.group(1),
'Wrong SQL used to create an auto-increment column on SQLite'
)
def test_disable_constraint_checking_failure_disallowed(self):
"""
SQLite schema editor is not usable within an outer transaction if
foreign key constraint checks are not disabled beforehand.
"""
msg = (
'SQLite schema editor cannot be used while foreign key '
'constraint checks are enabled. Make sure to disable them '
'before entering a transaction.atomic() context because '
'SQLite3 does not support disabling them in the middle of '
'a multi-statement transaction.'
)
with self.assertRaisesMessage(NotSupportedError, msg):
with transaction.atomic(), connection.schema_editor(atomic=True):
pass
def test_constraint_checks_disabled_atomic_allowed(self):
"""
SQLite3 schema editor is usable within an outer transaction as long as
foreign key constraints checks are disabled beforehand.
"""
def constraint_checks_enabled():
with connection.cursor() as cursor:
return bool(cursor.execute('PRAGMA foreign_keys').fetchone()[0])
with connection.constraint_checks_disabled(), transaction.atomic():
with connection.schema_editor(atomic=True):
self.assertFalse(constraint_checks_enabled())
self.assertFalse(constraint_checks_enabled())
self.assertTrue(constraint_checks_enabled())
def test_field_rename_inside_atomic_block(self): def test_field_rename_inside_atomic_block(self):
""" """
NotImplementedError is raised when a model field rename is attempted NotImplementedError is raised when a model field rename is attempted

View File

@ -25,12 +25,12 @@ class SchemaIndexesTests(TestCase):
""" """
Index names should be deterministic. Index names should be deterministic.
""" """
with connection.schema_editor() as editor: editor = connection.schema_editor()
index_name = editor._create_index_name( index_name = editor._create_index_name(
table_name=Article._meta.db_table, table_name=Article._meta.db_table,
column_names=("c1",), column_names=("c1",),
suffix="123", suffix="123",
) )
self.assertEqual(index_name, "indexes_article_c1_a52bd80b123") self.assertEqual(index_name, "indexes_article_c1_a52bd80b123")
def test_index_name(self): def test_index_name(self):
@ -41,12 +41,12 @@ class SchemaIndexesTests(TestCase):
* Include a deterministic hash. * Include a deterministic hash.
""" """
long_name = 'l%sng' % ('o' * 100) long_name = 'l%sng' % ('o' * 100)
with connection.schema_editor() as editor: editor = connection.schema_editor()
index_name = editor._create_index_name( index_name = editor._create_index_name(
table_name=Article._meta.db_table, table_name=Article._meta.db_table,
column_names=('c1', 'c2', long_name), column_names=('c1', 'c2', long_name),
suffix='ix', suffix='ix',
) )
expected = { expected = {
'mysql': 'indexes_article_c1_c2_looooooooooooooooooo_255179b2ix', 'mysql': 'indexes_article_c1_c2_looooooooooooooooooo_255179b2ix',
'oracle': 'indexes_a_c1_c2_loo_255179b2ix', 'oracle': 'indexes_a_c1_c2_loo_255179b2ix',

View File

@ -1,7 +1,9 @@
from django.apps import apps from django.apps import apps
from django.conf import settings from django.conf import settings
from django.db import connection from django.db import connection
from django.test import TestCase, skipIfDBFeature, skipUnlessDBFeature from django.test import (
TransactionTestCase, skipIfDBFeature, skipUnlessDBFeature,
)
from .models.tablespaces import ( from .models.tablespaces import (
Article, ArticleRef, Authors, Reviewers, Scientist, ScientistRef, Article, ArticleRef, Authors, Reviewers, Scientist, ScientistRef,
@ -21,7 +23,8 @@ def sql_for_index(model):
# We can't test the DEFAULT_TABLESPACE and DEFAULT_INDEX_TABLESPACE settings # We can't test the DEFAULT_TABLESPACE and DEFAULT_INDEX_TABLESPACE settings
# because they're evaluated when the model class is defined. As a consequence, # because they're evaluated when the model class is defined. As a consequence,
# @override_settings doesn't work, and the tests depend # @override_settings doesn't work, and the tests depend
class TablespacesTests(TestCase): class TablespacesTests(TransactionTestCase):
available_apps = ['model_options']
def setUp(self): def setUp(self):
# The unmanaged models need to be removed after the test in order to # The unmanaged models need to be removed after the test in order to

View File

@ -11,9 +11,7 @@ from django.conf import settings
from django.core.exceptions import ImproperlyConfigured from django.core.exceptions import ImproperlyConfigured
from django.core.management import call_command from django.core.management import call_command
from django.core.management.base import SystemCheckError from django.core.management.base import SystemCheckError
from django.test import ( from django.test import TransactionTestCase, skipUnlessDBFeature, testcases
TestCase, TransactionTestCase, skipUnlessDBFeature, testcases,
)
from django.test.runner import DiscoverRunner from django.test.runner import DiscoverRunner
from django.test.testcases import connections_support_transactions from django.test.testcases import connections_support_transactions
from django.test.utils import dependency_ordered from django.test.utils import dependency_ordered
@ -242,8 +240,9 @@ class Ticket17477RegressionTests(AdminScriptTestCase):
self.assertNoOutput(err) self.assertNoOutput(err)
class Sqlite3InMemoryTestDbs(TestCase): class Sqlite3InMemoryTestDbs(TransactionTestCase):
multi_db = True multi_db = True
available_apps = ['test_runner']
@unittest.skipUnless(all(db.connections[conn].vendor == 'sqlite' for conn in db.connections), @unittest.skipUnless(all(db.connections[conn].vendor == 'sqlite' for conn in db.connections),
"This is an sqlite-specific issue") "This is an sqlite-specific issue")