From 315357ad25a6590e7f4564ec2e56a22132b09001 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Fri, 7 Dec 2018 19:55:47 -0500 Subject: [PATCH] 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. --- django/db/backends/sqlite3/base.py | 12 +++-- django/db/backends/sqlite3/schema.py | 11 ++++- docs/releases/2.0.10.txt | 3 ++ docs/releases/2.1.5.txt | 3 ++ tests/backends/sqlite/tests.py | 64 ++++++++++++++++++------- tests/indexes/tests.py | 24 +++++----- tests/model_options/test_tablespaces.py | 7 ++- tests/test_runner/tests.py | 7 ++- 8 files changed, 89 insertions(+), 42 deletions(-) diff --git a/django/db/backends/sqlite3/base.py b/django/db/backends/sqlite3/base.py index 846bd8a6f3..2a906cea40 100644 --- a/django/db/backends/sqlite3/base.py +++ b/django/db/backends/sqlite3/base.py @@ -247,11 +247,13 @@ class DatabaseWrapper(BaseDatabaseWrapper): self.connection.isolation_level = level def disable_constraint_checking(self): - if self.in_atomic_block: - # sqlite3 cannot disable constraint checking inside a transaction. - return False - self.cursor().execute('PRAGMA foreign_keys = OFF') - return True + with self.cursor() as cursor: + cursor.execute('PRAGMA foreign_keys = OFF') + # Foreign key constraints cannot be turned off while in a multi- + # statement transaction. Fetch the current state of the pragma + # to determine if constraints are effectively disabled. + enabled = cursor.execute('PRAGMA foreign_keys').fetchone()[0] + return not bool(enabled) def enable_constraint_checking(self): self.cursor().execute('PRAGMA foreign_keys = ON') diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py index 255f80dd18..c8de794a16 100644 --- a/django/db/backends/sqlite3/schema.py +++ b/django/db/backends/sqlite3/schema.py @@ -19,8 +19,15 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): def __enter__(self): # Some SQLite schema alterations need foreign key constraints to be - # disabled. Enforce it here for the duration of the transaction. - self.connection.disable_constraint_checking() + # disabled. Enforce it here for the duration of the schema edition. + 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') return super().__enter__() diff --git a/docs/releases/2.0.10.txt b/docs/releases/2.0.10.txt index ece2521867..18901490e0 100644 --- a/docs/releases/2.0.10.txt +++ b/docs/releases/2.0.10.txt @@ -16,3 +16,6 @@ Bugfixes * 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 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`). diff --git a/docs/releases/2.1.5.txt b/docs/releases/2.1.5.txt index 9f1f3a4e8a..f5306192a4 100644 --- a/docs/releases/2.1.5.txt +++ b/docs/releases/2.1.5.txt @@ -15,3 +15,6 @@ Bugfixes * 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 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`). diff --git a/tests/backends/sqlite/tests.py b/tests/backends/sqlite/tests.py index 86723a72d2..46122dc38b 100644 --- a/tests/backends/sqlite/tests.py +++ b/tests/backends/sqlite/tests.py @@ -2,7 +2,7 @@ import re import threading 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.fields import CharField from django.db.utils import NotSupportedError @@ -16,22 +16,6 @@ from ..models import Author, Item, Object, Square class Tests(TestCase): 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): """ Raise NotImplementedError when aggregating on date/time fields (#19360). @@ -82,6 +66,52 @@ class SchemaTests(TransactionTestCase): 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): """ NotImplementedError is raised when a model field rename is attempted diff --git a/tests/indexes/tests.py b/tests/indexes/tests.py index 43b9938b5d..ae79f375f5 100644 --- a/tests/indexes/tests.py +++ b/tests/indexes/tests.py @@ -25,12 +25,12 @@ class SchemaIndexesTests(TestCase): """ Index names should be deterministic. """ - with connection.schema_editor() as editor: - index_name = editor._create_index_name( - table_name=Article._meta.db_table, - column_names=("c1",), - suffix="123", - ) + editor = connection.schema_editor() + index_name = editor._create_index_name( + table_name=Article._meta.db_table, + column_names=("c1",), + suffix="123", + ) self.assertEqual(index_name, "indexes_article_c1_a52bd80b123") def test_index_name(self): @@ -41,12 +41,12 @@ class SchemaIndexesTests(TestCase): * Include a deterministic hash. """ long_name = 'l%sng' % ('o' * 100) - with connection.schema_editor() as editor: - index_name = editor._create_index_name( - table_name=Article._meta.db_table, - column_names=('c1', 'c2', long_name), - suffix='ix', - ) + editor = connection.schema_editor() + index_name = editor._create_index_name( + table_name=Article._meta.db_table, + column_names=('c1', 'c2', long_name), + suffix='ix', + ) expected = { 'mysql': 'indexes_article_c1_c2_looooooooooooooooooo_255179b2ix', 'oracle': 'indexes_a_c1_c2_loo_255179b2ix', diff --git a/tests/model_options/test_tablespaces.py b/tests/model_options/test_tablespaces.py index 79b0a8bb75..8502d8790f 100644 --- a/tests/model_options/test_tablespaces.py +++ b/tests/model_options/test_tablespaces.py @@ -1,7 +1,9 @@ from django.apps import apps from django.conf import settings from django.db import connection -from django.test import TestCase, skipIfDBFeature, skipUnlessDBFeature +from django.test import ( + TransactionTestCase, skipIfDBFeature, skipUnlessDBFeature, +) from .models.tablespaces import ( 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 # because they're evaluated when the model class is defined. As a consequence, # @override_settings doesn't work, and the tests depend -class TablespacesTests(TestCase): +class TablespacesTests(TransactionTestCase): + available_apps = ['model_options'] def setUp(self): # The unmanaged models need to be removed after the test in order to diff --git a/tests/test_runner/tests.py b/tests/test_runner/tests.py index b496d79369..be6c8b9fa0 100644 --- a/tests/test_runner/tests.py +++ b/tests/test_runner/tests.py @@ -11,9 +11,7 @@ from django.conf import settings from django.core.exceptions import ImproperlyConfigured from django.core.management import call_command from django.core.management.base import SystemCheckError -from django.test import ( - TestCase, TransactionTestCase, skipUnlessDBFeature, testcases, -) +from django.test import TransactionTestCase, skipUnlessDBFeature, testcases from django.test.runner import DiscoverRunner from django.test.testcases import connections_support_transactions from django.test.utils import dependency_ordered @@ -242,8 +240,9 @@ class Ticket17477RegressionTests(AdminScriptTestCase): self.assertNoOutput(err) -class Sqlite3InMemoryTestDbs(TestCase): +class Sqlite3InMemoryTestDbs(TransactionTestCase): multi_db = True + available_apps = ['test_runner'] @unittest.skipUnless(all(db.connections[conn].vendor == 'sqlite' for conn in db.connections), "This is an sqlite-specific issue")