Opened 22 months ago

Last modified 5 months ago

#29854 new Bug

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

Reported by: Rick Yang Owned by: nobody
Component: Migrations Version: master
Severity: Normal Keywords: MySQL, Migration, Altering primary key,
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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)
  1. 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),
        ),
  1. 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.

Attachments (1)

t29854.zip (3.3 KB) - added by Tim Graham 22 months ago.
sample app to reproduce

Download all attachments as: .zip

Change History (9)

comment:1 Changed 22 months ago by Rick Yang

Type: UncategorizedBug

comment:2 Changed 22 months ago by Rick Yang

Description: modified (diff)

comment:3 Changed 22 months ago by Rick Yang

Description: modified (diff)

comment:4 Changed 22 months ago by Rick Yang

Summary: Altering model primary key might cause referred foreign key attribution inconsistentAltering model primary key might cause referred foreign key's nullable attribution inconsistent

comment:5 Changed 22 months ago by 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.

Changed 22 months ago by Tim Graham

Attachment: t29854.zip added

sample app to reproduce

comment:6 Changed 22 months ago by Tim Graham

Summary: Altering model primary key might cause referred foreign key's nullable attribution inconsistentAltering the primary key targeted by several foreign keys incorrectly alters the foreign key's NULL attribute
Triage Stage: UnreviewedAccepted

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.

comment:7 Changed 8 months ago by Baptiste Mispelon

Version: 1.11master

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

comment:8 in reply to:  7 Changed 5 months ago by 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!

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