[Django] #29928: TestCase doesn't check for foreign key constraints when using sqlite
Django
---------------------------------------------+------------------------
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
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:
{{{#!python
class Person(models.Model):
name = models.CharField(max_length=20)
mom = models.ForeignKey('Person', on_delete=models.CASCADE, null=True)
}}}
tests.py:
{{{#!python
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 + 1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29928>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: sqlite db foreign | Triage Stage:
key TestCase | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michel Samia):
* keywords: => sqlite db foreign key TestCase
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:1>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: sqlite db foreign | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Michel Samia:
Old description:
New description:
Hi,
tests.py:
{{{#!python
import sqlite3
conn.commit()
conn.close()
with self.assertRaises(sqlite3.IntegrityError):
Person.objects.create(name='devil', mom_id=biggest_id + 100)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:2>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: sqlite db foreign | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michel Samia):
workaround: to check foreign keys at the end of a test we can run this:
{{{#!python
from django.db import connections
connections['default'].check_constraints()
}}}
but this is definitely something you don't want to do in each test
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:3>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: sqlite db foreign | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michel Samia):
ah, according to this commit, it may be fixed in 2.x!
https://github.com/django/django/commit/169c3b3e07829d9ffa409b6eb5c1094d8ef918a8
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:4>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michel Samia):
* version: 1.11 => 2.1
Comment:
upgraded to 2.1, still seeing this
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:5>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
key TestCase |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:6>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Accepted
key TestCase |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michel Samia):
I see more possible solutions here. We can go sequentailly over them:
1. (short term) document that sqlite isn't checking foreign keys in 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 :)
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:7>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Accepted
key TestCase |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michel Samia):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:8>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Accepted
key TestCase |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michel Samia):
regarding my previous comment, issue created
https://github.com/ghaering/pysqlite/issues/131
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:9>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Accepted
key TestCase |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:10>
Django
Reporter: Michel Samia | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Accepted
key TestCase |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
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`. The new check
is fast enough to not be noticeable at all on SQLite 3.20+ at least.
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:11>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.1
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Accepted
key TestCase |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:12>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Accepted
key TestCase |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
* version: 2.1 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:13>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
key TestCase | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:14>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: sqlite db foreign | Triage Stage: Ready for
key TestCase | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"a939d630a429abb53002a09fc04785445fc64bb4" a939d630]:
{{{
#!CommitTicketReference repository=""
revision="a939d630a429abb53002a09fc04785445fc64bb4"
Refs #29928 -- Implemented fast constraint checking on SQLite 3.20+.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:15>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Component: Testing framework | Version: master
Keywords: sqlite db foreign | Triage Stage: Ready for
key TestCase | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson <carlton.gibson@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1939dd49d142b65fa22eb5f85cee0d20864d3730" 1939dd4]:
{{{
#!CommitTicketReference repository=""
revision="1939dd49d142b65fa22eb5f85cee0d20864d3730"
Fixed #29928 -- Enabled deferred constraint checks on SQLite 3.20+.
Refs #11665, #14204.
Thanks Michel Samia for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:16>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: sqlite db foreign | Triage Stage: Ready for
key TestCase | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"c5b58d77673b9f9d3a90e1c6a5bf15dce76f3d45" c5b58d7]:
{{{
#!CommitTicketReference repository=""
revision="c5b58d77673b9f9d3a90e1c6a5bf15dce76f3d45"
Refs #29928 -- Adjusted release notes of SQLite test constraint checking.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:17>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: sqlite db foreign | Triage Stage: Ready for
key TestCase | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6b9bd0933e0ce7e9bfcb2bcc5c8a6dc3486b4257" 6b9bd09]:
{{{
#!CommitTicketReference repository=""
revision="6b9bd0933e0ce7e9bfcb2bcc5c8a6dc3486b4257"
Refs #29928 -- Added supports_pragma_foreign_key_check SQLite feature
flag.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:19>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: sqlite db foreign | Triage Stage: Ready for
key TestCase | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f3eb1cfb58084bd2c547e64a217cb6da0ddfb0df" f3eb1cfb]:
{{{
#!CommitTicketReference repository=""
revision="f3eb1cfb58084bd2c547e64a217cb6da0ddfb0df"
Refs #29928 -- Corrected SQLite's can_defer_constraint_checks feature
flag.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:18>
Django
Reporter: Michel Samia | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: sqlite db foreign | Triage Stage: Ready for
key TestCase | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Django's test suite takes about 20-30% longer after this was properly
enabled in f3eb1cfb58084bd2c547e64a217cb6da0ddfb0df. Do you think the
performance penalty is acceptable?
--
Ticket URL: <https://code.djangoproject.com/ticket/29928#comment:20>