From 169c3b3e07829d9ffa409b6eb5c1094d8ef918a8 Mon Sep 17 00:00:00 2001 From: Claude Paroz Date: Sat, 10 Jan 2015 17:24:16 +0100 Subject: [PATCH] Fixed #14204 -- Enforced SQLite foreign key constraints. Thanks Tim Graham for contributing to the patch and Simon Charette for advice and review. --- django/core/management/commands/flush.py | 8 ++--- django/db/backends/base/operations.py | 8 +++++ django/db/backends/base/schema.py | 6 ++-- django/db/backends/sqlite3/base.py | 11 ++++++ django/db/backends/sqlite3/features.py | 1 - django/db/backends/sqlite3/introspection.py | 13 +++++++ django/db/backends/sqlite3/operations.py | 9 +++-- django/db/backends/sqlite3/schema.py | 23 ++++++------ docs/releases/2.0.txt | 40 +++++++++++++++++++++ tests/schema/tests.py | 2 +- 10 files changed, 95 insertions(+), 26 deletions(-) diff --git a/django/core/management/commands/flush.py b/django/core/management/commands/flush.py index 3e9d9fefc9..0df5f67e39 100644 --- a/django/core/management/commands/flush.py +++ b/django/core/management/commands/flush.py @@ -5,7 +5,7 @@ from django.apps import apps from django.core.management.base import BaseCommand, CommandError from django.core.management.color import no_style from django.core.management.sql import emit_post_migrate_signal, sql_flush -from django.db import DEFAULT_DB_ALIAS, connections, transaction +from django.db import DEFAULT_DB_ALIAS, connections class Command(BaseCommand): @@ -59,11 +59,7 @@ Are you sure you want to do this? if confirm == 'yes': try: - with transaction.atomic(using=database, - savepoint=connection.features.can_rollback_ddl): - with connection.cursor() as cursor: - for sql in sql_list: - cursor.execute(sql) + connection.ops.execute_sql_flush(database, sql_list) except Exception as exc: raise CommandError( "Database %s couldn't be flushed. Possible reasons:\n" diff --git a/django/db/backends/base/operations.py b/django/db/backends/base/operations.py index cf6b5f9166..3e00f0cce0 100644 --- a/django/db/backends/base/operations.py +++ b/django/db/backends/base/operations.py @@ -4,6 +4,7 @@ from importlib import import_module from django.conf import settings from django.core.exceptions import ImproperlyConfigured +from django.db import transaction from django.db.backends import utils from django.utils import timezone from django.utils.dateparse import parse_duration @@ -366,6 +367,13 @@ class BaseDatabaseOperations: """ raise NotImplementedError('subclasses of BaseDatabaseOperations must provide an sql_flush() method') + def execute_sql_flush(self, using, sql_list): + """Execute a list of SQL statements to flush the database.""" + with transaction.atomic(using=using, savepoint=self.connection.features.can_rollback_ddl): + with self.connection.cursor() as cursor: + for sql in sql_list: + cursor.execute(sql) + def sequence_reset_by_name_sql(self, style, sequences): """ Return a list of the SQL statements required to reset sequences diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py index ed5f2e869f..2d468a8c66 100644 --- a/django/db/backends/base/schema.py +++ b/django/db/backends/base/schema.py @@ -260,13 +260,13 @@ class BaseDatabaseSchemaEditor: if field.remote_field and field.db_constraint: to_table = field.remote_field.model._meta.db_table to_column = field.remote_field.model._meta.get_field(field.remote_field.field_name).column - if self.connection.features.supports_foreign_keys: - self.deferred_sql.append(self._create_fk_sql(model, field, "_fk_%(to_table)s_%(to_column)s")) - elif self.sql_create_inline_fk: + if self.sql_create_inline_fk: definition += " " + self.sql_create_inline_fk % { "to_table": self.quote_name(to_table), "to_column": self.quote_name(to_column), } + elif self.connection.features.supports_foreign_keys: + self.deferred_sql.append(self._create_fk_sql(model, field, "_fk_%(to_table)s_%(to_column)s")) # Add the SQL to our big list column_sqls.append("%s %s" % ( self.quote_name(field.column), diff --git a/django/db/backends/sqlite3/base.py b/django/db/backends/sqlite3/base.py index faa8b16f1f..e820c6bd2e 100644 --- a/django/db/backends/sqlite3/base.py +++ b/django/db/backends/sqlite3/base.py @@ -173,6 +173,7 @@ class DatabaseWrapper(BaseDatabaseWrapper): conn.create_function("regexp", 2, _sqlite_regexp) conn.create_function("django_format_dtdelta", 3, _sqlite_format_dtdelta) conn.create_function("django_power", 2, _sqlite_power) + conn.execute('PRAGMA foreign_keys = ON') return conn def init_connection_state(self): @@ -213,6 +214,16 @@ class DatabaseWrapper(BaseDatabaseWrapper): with self.wrap_database_errors: 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 + + def enable_constraint_checking(self): + self.cursor().execute('PRAGMA foreign_keys = ON') + def check_constraints(self, table_names=None): """ Check each table name in `table_names` for rows with invalid foreign diff --git a/django/db/backends/sqlite3/features.py b/django/db/backends/sqlite3/features.py index 2209adaa56..80af6068ff 100644 --- a/django/db/backends/sqlite3/features.py +++ b/django/db/backends/sqlite3/features.py @@ -17,7 +17,6 @@ class DatabaseFeatures(BaseDatabaseFeatures): max_query_params = 999 supports_mixed_date_datetime_comparisons = False has_bulk_insert = True - supports_foreign_keys = False supports_column_check_constraints = False autocommits_when_autocommit_is_off = True can_introspect_decimal_field = False diff --git a/django/db/backends/sqlite3/introspection.py b/django/db/backends/sqlite3/introspection.py index 0e87dcf0c6..49c2095a1c 100644 --- a/django/db/backends/sqlite3/introspection.py +++ b/django/db/backends/sqlite3/introspection.py @@ -289,4 +289,17 @@ class DatabaseIntrospection(BaseDatabaseIntrospection): "check": False, "index": False, } + # Get foreign keys + cursor.execute('PRAGMA foreign_key_list(%s)' % self.connection.ops.quote_name(table_name)) + for row in cursor.fetchall(): + # Remaining on_update/on_delete/match values are of no interest here + id_, seq, table, from_, to = row[:5] + constraints['fk_%d' % id_] = { + 'columns': [from_], + 'primary_key': False, + 'unique': False, + 'foreign_key': (table, to), + 'check': False, + 'index': False, + } return constraints diff --git a/django/db/backends/sqlite3/operations.py b/django/db/backends/sqlite3/operations.py index f030c9ed99..28db849828 100644 --- a/django/db/backends/sqlite3/operations.py +++ b/django/db/backends/sqlite3/operations.py @@ -149,9 +149,6 @@ class DatabaseOperations(BaseDatabaseOperations): return -1 def sql_flush(self, style, tables, sequences, allow_cascade=False): - # NB: The generated SQL below is specific to SQLite - # Note: The DELETE FROM... SQL generated below works for SQLite databases - # because constraints don't exist sql = ['%s %s %s;' % ( style.SQL_KEYWORD('DELETE'), style.SQL_KEYWORD('FROM'), @@ -161,6 +158,12 @@ class DatabaseOperations(BaseDatabaseOperations): # sql_flush() implementations). Just return SQL at this point return sql + def execute_sql_flush(self, using, sql_list): + # To prevent possible violation of foreign key constraints, deactivate + # constraints outside of the transaction created in super(). + with self.connection.constraint_checks_disabled(): + super().execute_sql_flush(using, sql_list) + def adapt_datetimefield_value(self, value): if value is None: return None diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py index 3bf71e071a..93cf077dd2 100644 --- a/django/db/backends/sqlite3/schema.py +++ b/django/db/backends/sqlite3/schema.py @@ -11,26 +11,20 @@ from django.db.backends.ddl_references import Statement class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): sql_delete_table = "DROP TABLE %(table)s" - sql_create_inline_fk = "REFERENCES %(to_table)s (%(to_column)s)" + sql_create_fk = None + sql_create_inline_fk = "REFERENCES %(to_table)s (%(to_column)s) DEFERRABLE INITIALLY DEFERRED" sql_create_unique = "CREATE UNIQUE INDEX %(name)s ON %(table)s (%(columns)s)" sql_delete_unique = "DROP INDEX %(name)s" def __enter__(self): - with self.connection.cursor() as c: - # Some SQLite schema alterations need foreign key constraints to be - # disabled. This is the default in SQLite but can be changed with a - # build flag and might change in future, so can't be relied upon. - # Enforce it here for the duration of the transaction. - c.execute('PRAGMA foreign_keys') - self._initial_pragma_fk = c.fetchone()[0] - c.execute('PRAGMA foreign_keys = 0') + # 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() return super().__enter__() def __exit__(self, exc_type, exc_value, traceback): super().__exit__(exc_type, exc_value, traceback) - with self.connection.cursor() as c: - # Restore initial FK setting - PRAGMA values can't be parametrized - c.execute('PRAGMA foreign_keys = %s' % int(self._initial_pragma_fk)) + self.connection.enable_constraint_checking() def quote_value(self, value): # The backend "mostly works" without this function and there are use @@ -259,6 +253,11 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): """Perform a "physical" (non-ManyToMany) field update.""" # Alter by remaking table self._remake_table(model, alter_field=(old_field, new_field)) + # Rebuild tables with FKs pointing to this field if the PK type changed. + if old_field.primary_key and new_field.primary_key and old_type != new_type: + for rel in new_field.model._meta.related_objects: + if not rel.many_to_many: + self._remake_table(rel.related_model) def _alter_many_to_many(self, model, old_field, new_field, strict): """Alter M2Ms to repoint their to= endpoints.""" diff --git a/docs/releases/2.0.txt b/docs/releases/2.0.txt index e58c2588cc..03f700163c 100644 --- a/docs/releases/2.0.txt +++ b/docs/releases/2.0.txt @@ -435,6 +435,46 @@ raises an exception and should be replaced with:: models.Index(fields=['headline', '-pub_date'], name='index_name') +Foreign key constraints are now enabled on SQLite +------------------------------------------------- + +This will appear as a backwards-incompatible change (``IntegrityError: +FOREIGN KEY constraint failed``) if attempting to save an existing model +instance that's violating a foreign key constraint. + +Foreign keys are now created with ``DEFERRABLE INITIALLY DEFERRED`` instead of +``DEFERRABLE IMMEDIATE``. Thus, tables may need to be rebuilt to recreate +foreign keys with the new definition, particularly if you're using a pattern +like this:: + + from django.db import transaction + + with transaction.atomic(): + Book.objects.create(author_id=1) + Author.objects.create(id=1) + +If you don't recreate the foreign key as ``DEFERRED``, the first ``create()`` +would fail now that foreign key constraints are enforced. + +Backup your database first! After upgrading to Django 2.0, you can then +rebuild tables using a script similar to this:: + + from django.apps import apps + from django.db import connection + + for app in apps.get_app_configs(): + for model in app.get_models(include_auto_created=True): + if model._meta.managed and not (model._meta.proxy or model._meta.swapped): + for base in model.__bases__: + if hasattr(base, '_meta'): + base._meta.local_many_to_many = [] + model._meta.local_many_to_many = [] + with connection.schema_editor() as editor: + editor._remake_table(model) + +This script hasn't received extensive testing and needs adaption for various +cases such as multiple databases. Feel free to contribute improvements. + Miscellaneous ------------- diff --git a/tests/schema/tests.py b/tests/schema/tests.py index e2b4f8ca6b..e7f47865d4 100644 --- a/tests/schema/tests.py +++ b/tests/schema/tests.py @@ -1953,7 +1953,7 @@ class SchemaTests(TransactionTestCase): editor.alter_field(model, get_field(unique=True), field, strict=True) self.assertNotIn(constraint_name, self.get_constraints(model._meta.db_table)) - if connection.features.supports_foreign_keys: + if editor.sql_create_fk: constraint_name = "CamelCaseFKConstraint" editor.execute( editor.sql_create_fk % {