#23741 closed New feature (wontfix)
[System Check for migrations] Check presence of all foreign keys
Reported by: | notsqrt | Owned by: | notsqrt |
---|---|---|---|
Component: | Core (System checks) | Version: | 1.7 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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:
@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.
Attachments (1)
Change History (15)
by , 10 years ago
Attachment: | 23741-doc.diff added |
---|
comment:1 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → New feature |
comment:5 by , 10 years ago
In the check, using Error is a little too strict, Warning should be enough.
comment:7 by , 10 years ago
I'll look into it.
Do you see any danger I'll have to avoid ?
Where should I put this check ? Per model ?
comment:8 by , 10 years ago
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 ?
comment:9 by , 10 years ago
You can read about how to test using a different database in 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.
comment:10 by , 10 years ago
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 ?
comment:11 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:12 by , 10 years ago
Is django-developers a better place for discussion than comments in a pull request ?
comment:13 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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.
How does the attached look for the doc update?