Altering the primary key targeted by several foreign keys incorrectly alters the foreign key's NULL attribute on MySQL

[Rick Yang]

Django 1.11.8~16 can reproduce this issue.
DB is MySQL

Reproduced Steps:

1. Create models as following:

class TestModel(models.Model):
    filing_no = models.CharField(max_length=16, primary_key=True)


class OtherModel1(models.Model):
    id = models.AutoField(primary_key=True)

    # null=True
    f = models.ForeignKey(TestModel, null=True)


class OtherModel2(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel3(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel4(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel5(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel6(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel7(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel8(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel9(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)


class OtherModel10(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel)

2. Based on above models, we change max_length of TestModel's filing_no and auto-generate migration code as following, and then perform migrating.

        migrations.AlterField(
            model_name='testmodel',
            name='filing_no',
            field=models.CharField(max_length=24, primary_key=True, serialize=False),
        ),

3. Check DB schema of table OtherModel1~10, field f's nullable attribution becomes False, and field f's nullable attribution in some other OtherModel becomes True.

My investigation:

In function BaseDatabaseSchemaEditor._alter_field(), the function call _related_non_m2m_objects(old_field, new_field) will return inconsistent "old" and "new" related_objects pairs (the model order is different), and generate wrong SQL command to alter foreign keys.

[Tim Graham]

Can you confirm the bug with the latest stable release (Django 2.1.2)? It may have been fixed since Django 1.11.

[Tim Graham]

sample app to reproduce

[Tim Graham]

I'm attaching a sample project that reproduces the issue on master (tested at dc5e75d419893bde33b7e439b59bdf271fc1a3f2). Using the repeated-django-test.sh script in the archive, a failure will usually occur within about 10 test runs. PostgreSQL doesn't seem affected.

[Baptiste Mispelon]

After a bit of digging, I managed to get a consistent test failure on the latest master (c33eb6dcd0c211f8f02b2976fe3b3463f0a54498) which helped me diagnose a bit further.
Bear with me, it's going to be a bit verbose :)

1 - How to get a consistent failure

I took Tim's sample project (thanks Tim!) and managed to simplify it by playing around with PYTHONHASHSEED [1] so I could get a consistent failure (mysql only, as noted in the original ticket)..

Here are my models:

class TestModel(models.Model):
    filing_no = models.CharField(max_length=24, primary_key=True)


class OtherModel1(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel, null=True, on_delete=models.CASCADE)


class OtherModel2(models.Model):
    id = models.AutoField(primary_key=True)
    f = models.ForeignKey(TestModel, on_delete=models.CASCADE)

And here's the initial migration (note the last operation which is the crux of this issue):

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):
    initial = True
    dependencies = [ ]

    operations = [
        migrations.CreateModel(
            name='OtherModel1',
            fields=[('id', models.AutoField(primary_key=True, serialize=False))],
        ),
        migrations.CreateModel(
            name='OtherModel2',
            fields=[('id', models.AutoField(primary_key=True, serialize=False))],
        ),
        migrations.CreateModel(
            name='TestModel',
            fields=[('filing_no', models.CharField(max_length=16, primary_key=True, serialize=False))],
        ),
        migrations.AddField(
            model_name='othermodel2',
            name='f',
            field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='t29854.TestModel'),
        ),
        migrations.AddField(
            model_name='othermodel1',
            name='f',
            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to='t29854.TestModel'),
        ),
        migrations.AlterField(
            model_name='testmodel',
            name='filing_no',
            field=models.CharField(max_length=24, primary_key=True, serialize=False),
        ),
    ]

With this setup, you can trigger an intermittent test failure with this very simple test:

class Tests(TestCase):
    def test(self):
        # Fails with IntegrityError: (1048, "Column 'f_id' cannot be null")
        OtherModel1.objects.create()

By playing with PYTHONHASHSEED, you can make the test failure consistent. Start with PYTHONHASHSEED=1 python manage.py test and increase by 1 until you get a failure (the magic number for me was PYTHONHASHSEED=2, I'm not sure if that would work for other people as well).

2 - Source of the issue

After much debugging I managed to end up at the same place as the original reporter: _related_non_m2m_objects [2]:

return zip(
        (obj for obj in _all_related_fields(old_field.model) if _is_relevant_relation(obj, old_field)),
        (obj for obj in _all_related_fields(new_field.model) if _is_relevant_relation(obj, new_field)),
    )

With _all_related_fields() (defined in the same source file, just above) being a wrapper around model._meta.get_fields() (with some hardcoded parameters).

You can probably start to see where the problem is coming from: get_fields() doesn't seem to guarantee a deterministic order of the returned fields, which leads to old fields and new fields being mismatched in some cases.

Even though this bug is present in all backends, it only seems to affect mysql (I only tested mysql, sqlite and postgresql though).
It's mostly a lucky coincidence:

• The sqlite backend overrides BaseDatabaseSchemaEditor._alter_field and doesn't make use of related_non_m2m_objects
• The postgresql hits the same problematic code path but has a different way of handling the constraints (NULL vs NOT NULL) which avoids the problem somehow.

3 - Fixing the problem

A quick workaround which seems to work is to change the code for _all_related_fields so it's deterministic (adding a sort for example):

def _all_related_fields(model):
    all_fields = model._meta._get_fields(forward=False, reverse=True, include_hidden=True)
    return sorted(all_fields, key=lambda f: f.name)

I'm not sure if that's a good fix though: it feels like fixing a symptom rather than the underlying problem.

I also wonder if there might be other tricky issues caused by the non-deterministic nature of meta.get_fields() but I'm not sure how to investigate that more systematically.

[1] ​https://docs.python.org/3/using/cmdline.html#envvar-PYTHONHASHSEED
[2] ​https://github.com/django/django/blob/c33eb6dcd0c211f8f02b2976fe3b3463f0a54498/django/db/backends/base/schema.py#L38-L41

[Sanskar Jaiswal]

Hi I would like to work on this issue, and was hoping to get some doubts cleared, before I start ;)

Replying to Baptiste Mispelon:

You can probably start to see where the problem is coming from: get_fields() doesn't seem to guarantee a deterministic order of the returned fields, which leads to old fields and new fields being mismatched in some cases.

What does it mean when you say, that "get_fields() doesn't guarantee a deterministic order of the returned fields"?

Thanks!

[Collin Anderson]

I'm just confirming this is still an issue on mysql and that Baptiste's "quick workaround" above (sorting the fields) worked for me. I don't know what the proper solution should be, but it might be worth committing the workaround.

In my particular case, I'm trying to migrate from an auto-created id = AutoField(primary_key=True) field to id = models.CharField(max_length=15, primary_key=True), and there's a ForeignKey(MyModel, null=True) that was getting NOT NULL, likely because there are also some ForeignKey(MyModel, null=False) fields referencing this model, and they're getting mixed up.

I think this one's complicated enough that I don't think I'm able to create a minimal test case, but here's a patch without any tests that fixes it for me:

​https://github.com/django/django/pull/15635

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3b898ea6:

Fixed #29854 -- Made _all_related_fields() return deterministically ordered fields.

Thanks to Rick Yang and Baptiste Mispelon for the investigation.