Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data migration doesn't write to ParentalManyToManyField #96

Open
harrislapiroff opened this issue Apr 19, 2018 · 6 comments
Open

Data migration doesn't write to ParentalManyToManyField #96

harrislapiroff opened this issue Apr 19, 2018 · 6 comments

Comments

@harrislapiroff
Copy link

harrislapiroff commented Apr 19, 2018

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.

@harrislapiroff harrislapiroff changed the title Data migrations doesn't write to ParentalManyToManyField Data migration doesn't write to ParentalManyToManyField Apr 19, 2018
@grasshoppermouse
Copy link

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

chigby added a commit to freedomofpress/pressfreedomtracker.us that referenced this issue Mar 6, 2020
We are phasing out the one-affiliation-per-incident affiliation model
in favor of an Institution model, which can either be applied on a
journalist by journalist basis, or to the incident as a whole, but as
many as desired.

For this, we must move data from the legacy affiliation field to the
new Institution field.  We do this using a data migration, with two
pre- and post- migrations to work around this bug in
djang-modelcluster:
    wagtail/django-modelcluster#96

For the migration, we want these steps:

1. For each incident, examine the `affiliation` field

2. If it null or set to the default value of "Independent," discard
it.  The "independent" value is a placeholder for no definite
affiliation and will not be used in the new scheme.

3. Create, if it doesn't already exist, an Institution with the title
of the affiliation.

4. If this incident has any targeted journalists who do not already
have an associated institution, associate the new institution with
them.

5. If this incident does *not* have any targeted journalists, add the
new institution as a target of this incident.

This will fully bring any affiliation data into the institution model
and apply it to the incidents that this data described.
chigby added a commit to freedomofpress/pressfreedomtracker.us that referenced this issue Mar 6, 2020
We are phasing out the one-affiliation-per-incident affiliation model
in favor of an Institution model, which can either be applied on a
journalist by journalist basis, or to the incident as a whole, but as
many as desired.

For this, we must move data from the legacy affiliation field to the
new Institution field.  We do this using a data migration, with two
pre- and post- migrations to work around this bug in
djang-modelcluster:
    wagtail/django-modelcluster#96

For the migration, we want these steps:

1. For each incident, examine the `affiliation` field

2. If it null or set to the default value of "Independent," discard
it.  The "independent" value is a placeholder for no definite
affiliation and will not be used in the new scheme.

3. Create, if it doesn't already exist, an Institution with the title
of the affiliation.

4. If this incident has any targeted journalists who do not already
have an associated institution, associate the new institution with
them.

5. If this incident does *not* have any targeted journalists, add the
new institution as a target of this incident.

This will fully bring any affiliation data into the institution model
and apply it to the incidents that this data described.
chigby added a commit to freedomofpress/pressfreedomtracker.us that referenced this issue Mar 6, 2020
We are phasing out the one-affiliation-per-incident affiliation model
in favor of an Institution model, which can either be applied on a
journalist by journalist basis, or to the incident as a whole, but as
many as desired.

For this, we must move data from the legacy affiliation field to the
new Institution field.  We do this using a data migration, with two
pre- and post- migrations to work around this bug in
djang-modelcluster:
    wagtail/django-modelcluster#96

For the migration, we want these steps:

1. For each incident, examine the `affiliation` field

2. If it null or set to the default value of "Independent," discard
it.  The "independent" value is a placeholder for no definite
affiliation and will not be used in the new scheme.

3. Create, if it doesn't already exist, an Institution with the title
of the affiliation.

4. If this incident has any targeted journalists who do not already
have an associated institution, associate the new institution with
them.

5. If this incident does *not* have any targeted journalists, add the
new institution as a target of this incident.

This will fully bring any affiliation data into the institution model
and apply it to the incidents that this data described.
chigby added a commit to freedomofpress/pressfreedomtracker.us that referenced this issue Mar 25, 2020
We are phasing out the one-affiliation-per-incident affiliation model
in favor of an Institution model, which can either be applied on a
journalist by journalist basis, or to the incident as a whole, but as
many as desired.

For this, we must move data from the legacy affiliation field to the
new Institution field.  We do this using a data migration, with two
pre- and post- migrations to work around this bug in
djang-modelcluster:
    wagtail/django-modelcluster#96

For the migration, we want these steps:

1. For each incident, examine the `affiliation` field

2. If it null or set to the default value of "Independent," discard
it.  The "independent" value is a placeholder for no definite
affiliation and will not be used in the new scheme.

3. Create, if it doesn't already exist, an Institution with the title
of the affiliation.

4. If this incident has any targeted journalists who do not already
have an associated institution, associate the new institution with
them.

5. If this incident does *not* have any targeted journalists, add the
new institution as a target of this incident.

This will fully bring any affiliation data into the institution model
and apply it to the incidents that this data described.
@chosak
Copy link
Member

chosak commented Apr 6, 2020

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()

harrislapiroff pushed a commit to freedomofpress/pressfreedomtracker.us that referenced this issue Apr 7, 2020
We are phasing out the one-affiliation-per-incident affiliation model
in favor of an Institution model, which can either be applied on a
journalist by journalist basis, or to the incident as a whole, but as
many as desired.

For this, we must move data from the legacy affiliation field to the
new Institution field.  We do this using a data migration, with two
pre- and post- migrations to work around this bug in
djang-modelcluster:
    wagtail/django-modelcluster#96

For the migration, we want these steps:

1. For each incident, examine the `affiliation` field

2. If it null or set to the default value of "Independent," discard
it.  The "independent" value is a placeholder for no definite
affiliation and will not be used in the new scheme.

3. Create, if it doesn't already exist, an Institution with the title
of the affiliation.

4. If this incident has any targeted journalists who do not already
have an associated institution, associate the new institution with
them.

5. If this incident does *not* have any targeted journalists, add the
new institution as a target of this incident.

This will fully bring any affiliation data into the institution model
and apply it to the incidents that this data described.
@pySilver
Copy link

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

@vsalvino
Copy link

vsalvino commented May 26, 2022

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
Copy link
Member

Stormheg commented Sep 4, 2023

Ran into this issue as well!

@rossm6
Copy link

rossm6 commented Oct 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants