[System Check for migrations] Check presence of all foreign keys

[notsqrt]

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.

[Tim Graham]

How does the attached look for the doc update?

[notsqrt]

That's OK for me.

[Tim Graham <timograham@…>]

In f0ff452451c0a2db6db53b6f068c794a47450d78:

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

Thanks NotSqrt for the report; refs #23741.

[Tim Graham <timograham@…>]

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

[notsqrt]

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

[Tim Graham]

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

[notsqrt]

I'll look into it.

Do you see any danger I'll have to avoid ?
Where should I put this check ? Per model ?

[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 ?

[Tim Graham]

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.

[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 ?

[notsqrt]

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

[Tim Graham]

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.

[notsqrt]

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