[Django] #29854: Altering model primary key might cause referred foreign key attribution inconsistent
Django
inconsistent
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Type: | Status: new
Uncategorized |
Component: | Version: 1.11
Migrations | Keywords: MySQL, Migration,
Severity: Normal | Altering primary key,
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Django 1.11.8~16 can reproduce this issue.
DB is MySQL
Reproduced Steps:
1. Create models as following:
{{{
#!python
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.
{{{
#!python
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, 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, and generate wrong SQL command to
alter foreign keys.
--
Ticket URL: <https://code.djangoproject.com/ticket/29854>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
inconsistent
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage:
Altering primary key, | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:1>
Django
inconsistent
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage:
Altering primary key, | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
{{{
#!python
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, and generate wrong SQL command to
alter foreign keys.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:2>
Django
inconsistent
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage:
Altering primary key, | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Django 1.11.8~16 can reproduce this issue.
> 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, and generate wrong SQL command to
> alter foreign keys.
New description:
{{{
#!python
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:3>
Django
attribution inconsistent
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage:
Altering primary key, | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:4>
Django
attribution inconsistent
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage:
Altering primary key, | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:5>
Django
attribution inconsistent
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage:
Altering primary key, | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "t29854.zip" added.
sample app to reproduce
Django
alters the foreign key's NULL attribute
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Altering primary key, |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:6>
Django
alters the foreign key's NULL attribute
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage: Accepted
Altering primary key, |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.11 => master
Comment:
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:
{{{#!python
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):
{{{#!python
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:
{{{#!python
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]:
{{{#!python
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):
{{{#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:7>
Django
alters the foreign key's NULL attribute
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage: Accepted
Altering primary key, |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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 [comment:7 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Rick Yang | Owner: nobody
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage: Accepted
Altering primary key, |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Collin Anderson (added)
* has_patch: 0 => 1
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:9>
Django
alters the foreign key's NULL attribute on MySQL
Reporter: Rick Yang | Owner: Collin
| Anderson
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: MySQL, Migration, | Triage Stage: Accepted
Altering primary key, |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Collin Anderson
* needs_better_patch: 0 => 1
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:10>
Django
alters the foreign key's NULL attribute on MySQL
Reporter: Rick Yang | Owner: Collin
| Anderson
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Altering primary key, | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:11>
Django
alters the foreign key's NULL attribute on MySQL
Reporter: Rick Yang | Owner: Collin
| Anderson
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Keywords: MySQL, Migration, | Triage Stage: Ready for
Altering primary key, | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3b898ea61ec209ef3ef67e3a6bb55c69bae8c85d" 3b898ea6]:
{{{
#!CommitTicketReference repository=""
revision="3b898ea61ec209ef3ef67e3a6bb55c69bae8c85d"
Fixed #29854 -- Made _all_related_fields() return deterministically
ordered fields.
Thanks to Rick Yang and Baptiste Mispelon for the investigation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29854#comment:12>