wagtail / django-modelcluster

Django extension to allow working with 'clusters' of models as a single unit, independently of the database
BSD 3-Clause "New" or "Revised" License
484 stars 66 forks source link

Data migration doesn't write to ParentalManyToManyField #96

Open harrislapiroff opened 6 years ago

harrislapiroff commented 6 years ago

I ran into an issue writing a data migration for a ParentalManyToManyField today where the data migration wouldn't write any data to the tables for that field. The migration looked something like this:

# -*- coding: utf-8 -*-
# Generated by Django 1.11.11 on 2018-04-19 14:10
from __future__ import unicode_literals

from django.db import migrations

def forwards(apps, schema_editor):
    """
    Move all data from Topic, Country, and Language to TemporaryTopic,
    TemporaryCountry, and TemporaryLanguage
    """

    TemporaryTopic = apps.get_model('directory', 'TemporaryTopic')
    TemporaryCountry = apps.get_model('directory', 'TemporaryCountry')
    TemporaryLanguage = apps.get_model('directory', 'TemporaryLanguage')
    DirectoryEntry = apps.get_model('directory', 'DirectoryEntry')

    # ...

    # Move DirectoryEntry fks
    for entry in DirectoryEntry.objects.all():
        topic_titles = list(entry.topics.values_list('title', flat=True))
        country_titles = list(entry.countries.values_list('title', flat=True))
        language_titles = list(entry.languages.values_list('title', flat=True))
        entry.temporary_topics.set(
            TemporaryTopic.objects.filter(title__in=topic_titles)
        )
        entry.temporary_countries.set(
            TemporaryCountry.objects.filter(title__in=country_titles)
        )
        entry.temporary_languages.set(
            TemporaryLanguage.objects.filter(title__in=language_titles)
        )
        entry.save()

    # ...

class Migration(migrations.Migration):
    # ...
    operations = [
        migrations.RunPython(forwards),
    ]

@gasman suspects the fake model that gets built by django's migration logic doesn't inherit from ClusterableModel and is therefore missing some of the logic necessary to make this data migration work.

I ended up using a workaround in which I had a migration convert my ParentalManyToManyFields to Django's built-in ManyToManyField before the data migration and then another migration afterward to convert them back, which seems to have worked fine.

grasshoppermouse commented 5 years ago

Ran into the same problem. Did the data migration manually outside the migration.

chosak commented 4 years ago

I also ran into this. The explanation above seems correct; in migrations calling save() on a model calls the base Model.save() method and thus skips the ClusterableModel.save() call that would be necessary to persist the changes being made.

@harrislapiroff I was also able to use your suggested workaround of three migration steps:

  1. AlterField to a models.ManyToManyField.
  2. RunPython that does the data migration.
  3. AlterField back to a modelcluster.fields.ParentalManyToManyField.

Another simpler option that seems to work (using an undocumented API on the field) is to manually invoke commit() on the relation, e.g.

for entry in DirectoryEntry.objects.all():
    topic_titles = list(entry.topics.values_list('title', flat=True))
    entry.temporary_topics.set(
        TemporaryTopic.objects.filter(title__in=topic_titles)
    )

    # This seems to commit the related objects, and doesn't require an entry.save() call.
    entry.temporary_topics.commit()
pySilver commented 3 years ago

This should be mentioned in some caveats on a frontpage I guess! :)

vsalvino commented 2 years ago

In a perverse coincidence, we had been using the workaround first Initial State -> ManyToMany -> RunPython -> ParentalManyToMany, then decided to clean it up and do it the "correct way" e.g. Initial State -> ParentalManyToMany -> RunPython. After hours of confusion eventually found this issue. Definitely needs to be called out on in the README!

Perhaps Andy's workaround with calling commit() in the migration should/could be documented? Would the maintainers (@gasman ) foresee any issues with documenting that?

Stormheg commented 1 year ago

Ran into this issue as well!

rossm6 commented 1 week ago

Got around it by creating the new record using the through model instead