Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#29928 closed Bug (fixed)

TestCase doesn't check for foreign key constraints when using sqlite

Reported by: Michel Samia Owned by: Simon Charette
Component: Testing framework Version: dev
Severity: Normal Keywords: sqlite db foreign key TestCase
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Michel Samia)

Hi,

When I create some stupid insertion, foreign keys are not checked in test when using default sqlite engine. It looks like a regression to https://code.djangoproject.com/ticket/11665
sqlite3 driver doesn't enforce foreign key constraints until commit is called - impossible to use them in TestCase

In the first test method (low level) I ensure that sqlite is able to catch such foreign key violations. In the second (high level) I prove that django effectively disables this check in TestCase.

models.py:

class Person(models.Model):
    name = models.CharField(max_length=20)
    mom = models.ForeignKey('Person', on_delete=models.CASCADE, null=True)

tests.py:

import sqlite3

from django.test import TestCase
from .models import Person


# Create your tests here.
class AppTests(TestCase):
    def test_sqlite_constraints_low_level(self):
        conn = sqlite3.connect(':memory:')
        c = conn.cursor()

        # Create table
        c.execute('''CREATE TABLE contacts (
         id INTEGER PRIMARY KEY,
         name TEXT NOT NULL,
         mom INTEGER,
         FOREIGN KEY(mom) REFERENCES contacts(id)
        )
        ''')

        c.execute('PRAGMA foreign_keys = ON')

        c.execute("insert into contacts(id, name, mom) values(1, 'Marge', null)")
        c.execute("insert into contacts(id, name, mom) values(2, 'Bart', 1)")

        with self.assertRaises(sqlite3.IntegrityError):
            c.execute("insert into contacts(id, name, mom) values(3, 'devil', 100)")

        conn.commit()
        conn.close()

    def test_constraints_high_level(self):
        """
        this should fail, but doesn't because of deferred constraints checking:
        https://github.com/django/django/blame/803840abf7dcb6ac190f021a971f1e3dc8f6792a/django/db/backends/sqlite3/schema.py#L16

        In the related issue Simon Charette explicitly requests the defered checking
        https://code.djangoproject.com/ticket/14204#comment:19

        actually the deferred behavior is needed by loading fixtures with incorrect order of inserts or with object pointing to itself

        However, in test is should not be check at the end of the test as discussed in https://code.djangoproject.com/ticket/11665

        Related stack overflow question https://stackoverflow.com/questions/42238857/how-can-i-enable-foreign-key-checks-in-pytest-using-sqllite/53194777#53194777
        """
        marge = Person.objects.create(name='Marge')
        Person.objects.create(name='Bart', mom=marge)
        ids = list(Person.objects.values_list('id', flat=True).order_by('id'))
        biggest_id = ids[-1]

        with self.assertRaises(sqlite3.IntegrityError):
            Person.objects.create(name='devil', mom_id=biggest_id + 100)

Change History (20)

comment:1 by Michel Samia, 5 years ago

Keywords: sqlite db foreign key TestCase added

comment:2 by Michel Samia, 5 years ago

Description: modified (diff)

comment:3 by Michel Samia, 5 years ago

workaround: to check foreign keys at the end of a test we can run this:

from django.db import connections
connections['default'].check_constraints()

but this is definitely something you don't want to do in each test

comment:4 by Michel Samia, 5 years ago

ah, according to this commit, it may be fixed in 2.x! https://github.com/django/django/commit/169c3b3e07829d9ffa409b6eb5c1094d8ef918a8

comment:5 by Michel Samia, 5 years ago

Version: 1.112.1

upgraded to 2.1, still seeing this

comment:6 by Tim Graham, 5 years ago

Triage Stage: UnreviewedAccepted

Setting DatabaseFeatures.can_defer_constraint_checks = True for SQLite fixes the problem, however, there's a large performance penalty. On my machine, the runtime for Django's test suite increases from several machines to over an hour. If the problem can't be fixed, perhaps the limitation should be documented.

Version 0, edited 5 years ago by Tim Graham (next)

comment:7 by Michel Samia, 5 years ago

I see more possible solutions here. We can go sequentailly over them:

  1. (short term) document that sqlite isn't checking foreign keys in tests
  2. (mid-term) allow changing the checking mode for sqlite in settings.py via and option in the database dict and document it. Mention it can cause slowness
  3. (holy grail) contribute to sqlite a simple refactor/feature which will allow calling something like check_constraints on an uncommitted transaction and get the integrity errors more effectively (checking only uncommitted objects, not whole db). I guess this is the way how it works now for postgreSQL and Oracle. I'm already thinking of filing a feature request in sqlite for this but first I would like to hear your opinion on it :)
Last edited 5 years ago by Michel Samia (previous) (diff)

comment:8 by Michel Samia, 5 years ago

regarding step 3: actually sqlite itself probably supports getting number of unsatisfied constraints for given connection. It should be this call: https://www.sqlite.org/c3ref/db_status.html with this parameter: SQLITE_DBSTATUS_DEFERRED_FKS (https://www.sqlite.org/c3ref/c_dbstatus_options.html). I checked briefly if this function is exposed to pysqlite but didn't find it there. I'll ask in the github issue of pysqlite. This would allow us to query sqlite for fk violations and only if it says that number of fk violations > 0, we would run our full constraint checking.

comment:9 by Michel Samia, 5 years ago

regarding my previous comment, issue created https://github.com/ghaering/pysqlite/issues/131

comment:10 by Simon Charette, 5 years ago

I'm not convinced SQLITE_DBSTATUS_DEFERRED_FKS would help here, foreign key constraints would always be deferred from what I understand.

The main issue here is that our implementation of check_constraints() is pretty slow as it performs a ton of introspection queries:

  1. 1 query to retrieve all table names
  2. 1 query per table to retrieve the PK column name
  3. 1 query per foreign key per table to assert foreign data integrity

Given we create a ton of tables for the test suite running this check on each test teardown is not an option unless we find a way to speedup the process.

comment:11 by Simon Charette, 5 years ago

I had to implement fast constraint checking on SQLite when working on #30033 to avoid slowing down migrations and it looks like once it lands it could be as easy as turning can_defer_constraint_checks to fix this ticket. The new check is fast enough to not be noticeable at all on SQLite 3.20+ at least.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:12 by Simon Charette, 5 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:13 by Simon Charette, 5 years ago

Has patch: set
Version: 2.1master

comment:14 by Carlton Gibson, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:15 by Carlton Gibson <carlton.gibson@…>, 5 years ago

In a939d630:

Refs #29928 -- Implemented fast constraint checking on SQLite 3.20+.

comment:16 by Carlton Gibson <carlton.gibson@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 1939dd4:

Fixed #29928 -- Enabled deferred constraint checks on SQLite 3.20+.

Refs #11665, #14204.

Thanks Michel Samia for the report.

comment:17 by Tim Graham <timograham@…>, 5 years ago

In c5b58d7:

Refs #29928 -- Adjusted release notes of SQLite test constraint checking.

comment:18 by Tim Graham <timograham@…>, 5 years ago

In f3eb1cfb:

Refs #29928 -- Corrected SQLite's can_defer_constraint_checks feature flag.

comment:19 by Tim Graham <timograham@…>, 5 years ago

In 6b9bd09:

Refs #29928 -- Added supports_pragma_foreign_key_check SQLite feature flag.

comment:20 by Tim Graham, 5 years ago

Django's test suite takes about 20-30% longer after this was properly enabled in f3eb1cfb58084bd2c547e64a217cb6da0ddfb0df. Do you think the performance penalty is acceptable?

Note: See TracTickets for help on using tickets.
Back to Top