[Django] #23741: [System Check for migrations] Check presence of all foreign keys
Django
--------------------------------------+--------------------
Reporter: notsqrt | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (System checks) | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Hi !
Just had some troubles when upgrading to django 1.7, which led to some
discussions on https://groups.google.com/forum/#!topic/django-
users/x_7Fluj2ty8
The bottom line is : make sure that any app that depends on apps with
migrations also uses migrartions.
This is indeed stated in the docs :
https://docs.djangoproject.com/en/1.7/topics/migrations/#unmigrated-
dependencies
But the consequences are, I think, not sufficiently flagged.
One of the symptoms is that '''foreign key constraints won't be
created'''.
This ticket suggests two things:
* highlighting more clearly the consequences of not using migrations
(probably simply by putting that section in a "danger" block)
* adding a system check that all foreign keys are effectively enforced.
For the second point, it goes like:
{{{#!python
@checks.register()
def check_db_constraints(*args, **kwargs):
from django.apps import apps
from django.db import connection
from django.db.models.fields.related import RelatedField
errors = []
for model in apps.get_models():
if not model._meta.managed:
continue
relations =
connection.introspection.get_relations(connection.cursor(),
model._meta.db_table)
for field_id, field_info in
enumerate(model._meta.get_fields_with_model()):
field_model = field_info[0]
if isinstance(field_model, RelatedField):
if field_id not in relations:
errors.append(
checks.Error(
"Field %s of model %s has no detected database
relation." % (field_model, model.__name__),
hint="Check that the app has migrations if it
depends on apps that use them.",
)
)
return errors
}}}
It probably needs some refinements for multiple db connections and
detection of backends that do not support constraints.
--
Ticket URL: <https://code.djangoproject.com/ticket/23741>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* type: Uncategorized => New feature
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
How does the attached look for the doc update?
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:1>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by notsqrt):
That's OK for me.
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:2>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f0ff452451c0a2db6db53b6f068c794a47450d78"]:
{{{
#!CommitTicketReference repository=""
revision="f0ff452451c0a2db6db53b6f068c794a47450d78"
Added a warning about nonexistent FK constraints when unmigrated apps
depend on migrated ones.
Thanks NotSqrt for the report; refs #23741.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:3>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d373e223bdb0722b1d7ff9c470f3bf562815507c"]:
{{{
#!CommitTicketReference repository=""
revision="d373e223bdb0722b1d7ff9c470f3bf562815507c"
[1.7.x] Added a warning about nonexistent FK constraints when unmigrated
apps depend on migrated ones.
Thanks NotSqrt for the report; refs #23741.
Backport of f0ff452451 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:4>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by notsqrt):
Using Error is a little too strict, Warning should be enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:5>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Sounds reasonable -- do you think you can work up a patch with tests?
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:6>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by notsqrt):
I'll look into it.
Do you see any danger I'll have to avoid ?
Where should I put this check ? Per model ?
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:7>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by notsqrt):
An initial implementation at https://github.com/NotSqrt/django/compare/fk-
check
It's harder than I thought:
* how to ignore models that have to be migrated ? (in the actual apps, and
in django tests)
* how to use migrations in django test suite if we ignore unmigrated
models ?
* Is there a standard way for django test suite to test against various DB
backends ? SQLite does not support foreign key constraints in the first
place, so it's not interesting to test against a SQLite DB.. (I have not
looked into the details of djangoci)
* Do I really have to check each model in each database ? or is there
something intelligent to do via database routers ?
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:8>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
You can read about how to test using a different database in
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/unit-tests/#using-another-settings-module Using another settings module].
I thought implementing this might be tricky, and I'm not really sure I
have a good answer for your other questions.
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:9>
Django
Reporter: notsqrt | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by notsqrt):
Regarding the settings, that's what I did with a local PostgreSQL DB.
I was also wondering how Django does this in their CI platform.
The tricky parts are:
* I have a migrated model, I can check the DB constraints
* I have a new/modified model, the checks must be partial or skipped, but
how to detect this lightly ?
* I am testing the check themselves : if I use a model that does not live
in an app, it won't be present in the DB; otherwise I have to create an
app with models and migrations for my tests, and this seems to be avoid if
possible in django test suite.
I was about to open a pull request, with these questions, so it can be
seen by more people, to get more feedback.
Is it a good idea ?
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:10>
Django
Reporter: notsqrt | Owner: notsqrt
Type: New feature | Status: assigned
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by notsqrt):
* owner: nobody => notsqrt
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:11>
Django
Reporter: notsqrt | Owner: notsqrt
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by notsqrt):
Is django-developers a better place for discussion than comments in a pull
request ?
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:12>
Django
Reporter: notsqrt | Owner: notsqrt
Component: Core (System checks) | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: assigned => closed
* resolution: => wontfix
Comment:
I think the time to add this check has passed since 1.8 is feature frozen
and 1.9 won't support apps without migrations.
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:13>
Django
Reporter: notsqrt | Owner: notsqrt
Type: New feature | Status: closed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by notsqrt):
OK, it was incredibly hard to get it right anyway...
--
Ticket URL: <https://code.djangoproject.com/ticket/23741#comment:14>