Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#27742 closed Bug (fixed)

Unexpected migration on Parent -> child model inheritence

Reported by: Anthony King Owned by: nobody
Component: Migrations Version: 1.11
Severity: Release blocker Keywords:
Cc: João Paulo Melo de Sampaio Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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'),
        ),
    ]

Change History (13)

comment:1 Changed 19 months ago by Tim Graham

Component: UncategorizedMigrations
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 19 months ago by Anthony King

Mentioned this on IRC.

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

comment:3 Changed 19 months ago by Tim Graham

Yes, migrations don't exclude any attributes.

comment:4 Changed 19 months ago by Tim Graham

Cc: João Paulo Melo de Sampaio added

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

comment:5 Changed 19 months ago by 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?

comment:6 Changed 19 months ago by 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).

comment:7 in reply to:  6 Changed 19 months ago by 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.

Last edited 18 months ago by Tim Graham (previous) (diff)

comment:8 Changed 18 months ago by 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.

comment:9 Changed 18 months ago by 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.

comment:10 Changed 18 months ago by 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?

comment:11 Changed 18 months ago by 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.

comment:12 Changed 18 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

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.

comment:13 Changed 18 months ago by 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

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