Unexpected migration on Parent -> child model inheritence

[Anthony King]

After upgrading to 1.11, migrations are generated on unchanged fields for parent pointers.

models.py

class Parent(models.Model):
    pass


class Child(Parent):
    pass

initial_migration.py

# -*- coding: utf-8 -*-
# Generated by Django 1.10.5 on 2017-01-18 11:06
from __future__ import unicode_literals

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


class Migration(migrations.Migration):

    dependencies = [
        ('app', '0263_previous_migration'),
    ]

    operations = [
        migrations.CreateModel(
            name='Parent',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
            ],
        ),
        migrations.CreateModel(
            name='Child',
            fields=[
                ('parent_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='app.Parent')),
            ],
            bases=('app.parent',),
        ),
    ]

post upgrade migration.py

# -*- coding: utf-8 -*-
# Generated by Django 1.11a1 on 2017-01-18 11:06
from __future__ import unicode_literals

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


class Migration(migrations.Migration):

    dependencies = [
        ('app', '0264_child_parent'),
    ]

    operations = [
        migrations.AlterField(
            model_name='child',
            name='parent_ptr',
            field=models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, to='app.Parent'),
        ),
    ]

[Tim Graham]

Bisected to 74a575eb7296fb04e1fc2bd4e3f68dee3c66ee0a. It might be a needed migration as a result of that change. If so, this could be documented in the release notes to prevent confusion.

[Anthony King]

Mentioned this on IRC.

serialize isn't a documented field attribute, should this have ever gotten into the migration files?

[Tim Graham]

Yes, migrations don't exclude any attributes.

[Tim Graham]

Hi João, do you see any alternative to fixing #24607 or must we accept the new migrations here?

[João Paulo Melo de Sampaio]

Hi, Tim. In my understanding of the problem, we need to accept the migrations!

Rationale: the way Django serialized the objects before my patch caused *information loss*. When data was in the database, you could know who was the parent of a given child (thanks to the parent_ptr field), and after the serialization, you couldn't, because that field was not serialized. So you need to serialize multi-table inheritance OneToOneFields. Since, in the old code and migrations, these fields had serialize=False, that is just plain wrong! They should be serialize=True, so the new migrations are required. (I assume that, when the field is absent, True is assumed, since the field is not in the new migration.)

So, yes, I think the way to go is to add a release note about it.

What do you think?

[Anthony King]

What are the implications for distributed packages that have parent -> child models, and intend to support older versions of Django?

We currently have a testCase at work to ensure all migrations have been generated. This test would fail if we were on Django 1.10, and the library had the serialize migration applied in Django 1.11 (or vice-versa).

[Markus Holtermann]

Replying to Anthony King:

What are the implications for distributed packages that have parent -> child models, and intend to support older versions of Django?

Django's version support for migrations is outlined in ​the documentation . I see how this will lead to errors on 1.11 if you want to check that all required migrations have been applied. I'll try to come up with a solution or workaround for your case.

[Tim Graham]

I'm still not convinced that the fix for #24607 is ideal. It also marks a OneToOneField that's a primary key of a model (non-multi-table inheritance) as serialize=True which doesn't seem desirable or necessary. If a better fix for that ticket can't be developed by Feb. 20 (1.11 beta), I'm thinking to revert it.

[João Paulo Melo de Sampaio]

Tim, you said that the OneToOneField that is also a primary key (and, in Django's solution to this, relates the parent and the child) should not be serialized. How do you suggest we relate the parent and the child in a pool of instances of the parent class and instances of the child class? There's no way to relate them if we don't know their primary keys.

Reverting this, we go back to a state that exporting multi-table inheritance models with dumpdata incurs in loss of information.

[Tim Graham]

For example with these models:

from django.db import models

class Parent(models.Model):
    pass

class O2OPK(models.Model):
    id = models.OneToOneField(Parent, primary_key=True, on_delete=models.CASCADE)

and this data:

from ser.models import Parent, O2OPK

o = O2OPK.objects.create(id=Parent.objects.create())

dumpdata gives:

[{"model": "ser.parent", "pk": 1, "fields": {}}, {"model": "ser.o2opk", "pk": 1, "fields": {}}]

both on 1.10 and after the patch, yet a new migration is required because OneToOneField has serialize=True now.

The solution you proposed isn't obviously correct to me. It changes serialize for all OneToOneField primary keys to solve a case involving natural primary keys. If we can avoid it, it would be better if migrations weren't required for this niche case. I don't much usage experience with natural keys, and I haven't had time to dig into the issue to propose an alternative solution. Did you consider alternatives presented in :ticket:24607#comment:8?

[João Paulo Melo de Sampaio]

Hmm, I understand your point now. Then the change required is not needed at the models level, but at the serializer level. The serializer must treat those O2OPK fields differently whether natural primary keys are enabled or not. I agree that my patch must be reverted.

[Tim Graham <timograham@…>]

In 0595bca:

Fixed #27742 -- Reverted "Fixed #24607 -- Serialized natural keys in multi-table inheritance models."

This reverts commit 74a575eb7296fb04e1fc2bd4e3f68dee3c66ee0a as it causes
unexpected migrations and doesn't seem to be the best solution.

[Tim Graham <timograham@…>]

In eeb28f47:

[1.11.x] Fixed #27742 -- Reverted "Fixed #24607 -- Serialized natural keys in multi-table inheritance models."

This reverts commit 74a575eb7296fb04e1fc2bd4e3f68dee3c66ee0a as it causes
unexpected migrations and doesn't seem to be the best solution.

Backport of 0595bca221825c0c6bd572a32f3bf9eff7069328 from master