Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#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)

23741-doc.diff (960 bytes ) - added by Tim Graham 9 years ago.

Download all attachments as: .zip

Change History (15)

by Tim Graham, 9 years ago

Attachment: 23741-doc.diff added

comment:1 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

How does the attached look for the doc update?

comment:2 by notsqrt, 9 years ago

That's OK for me.

comment:3 by Tim Graham <timograham@…>, 9 years ago

In f0ff452451c0a2db6db53b6f068c794a47450d78:

Added a warning about nonexistent FK constraints when unmigrated apps depend on migrated ones.

Thanks NotSqrt for the report; refs #23741.

comment:4 by Tim Graham <timograham@…>, 9 years ago

In 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

comment:5 by notsqrt, 9 years ago

In the check, using Error is a little too strict, Warning should be enough.

Last edited 9 years ago by notsqrt (previous) (diff)

comment:6 by Tim Graham, 9 years ago

Sounds reasonable -- do you think you can work up a patch with tests?

comment:7 by notsqrt, 9 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 notsqrt, 9 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 Tim Graham, 9 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 notsqrt, 9 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 notsqrt, 9 years ago

Owner: changed from nobody to notsqrt
Status: newassigned

comment:12 by notsqrt, 9 years ago

Is django-developers a better place for discussion than comments in a pull request ?

comment:13 by Tim Graham, 9 years ago

Resolution: wontfix
Status: assignedclosed

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.

comment:14 by notsqrt, 9 years ago

OK, it was incredibly hard to get it right anyway...

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