Opened 6 years ago

Closed 18 months ago

#28987 closed Bug (fixed)

Migration changing ManyToManyField target to 'self' doesn't work correctly

Reported by: MSleepyPanda Owned by: Bhuvnesh
Component: Migrations Version: 2.0
Severity: Normal Keywords: ManyToManyField
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Steps to reproduce:
Create Models:

class Bar(models.Model):
    pass

class Foo(models.Model):
    bar = models.ManyToManyField('Bar', blank=True)

Migrate:

./manage.py makemigrations app
./manage.py migrate

Change type of the ManyToManyField to Foo:

class Bar(models.Model):
    pass

class Foo(models.Model):
    bar = models.ManyToManyField('Foo', blank=True)

Migrate (see above)

In the admin page, navigate to "add Foo", click save

You should see an OperationalError, "no such column: app_foo_bar.from_foo_id"

Change History (16)

comment:1 by Tim Graham, 6 years ago

Summary: makemigrations doesn't catch ManyToManyField type change, leaves DB in inconsistent stateMigration changing ManyToManyField target to 'self' doesn't work correctly
Triage Stage: UnreviewedAccepted

I believe it works correctly as long as the new target model isn't 'self'.

comment:2 by SShayashi, 6 years ago

Needs tests: set
Owner: changed from nobody to SShayashi
Status: newassigned

comment:3 by SShayashi, 6 years ago

Can I check out what you want? You can use 'self' instead of 'Foo' like this :

class Foo(models.Model):
    bar = models.ManyToManyField('self', blank=True)

You meant that we should use 'Foo' rather than 'self', right?

in reply to:  3 comment:4 by MSleepyPanda, 6 years ago

Replying to SShayashi:

Can I check out what you want? You can use 'self' instead of 'Foo' like this :

class Foo(models.Model):
    bar = models.ManyToManyField('self', blank=True)

You meant that we should use 'Foo' rather than 'self', right?

Exactly, though i wasn't aware that self would work in that context. I think i like naming the type directly more, since self could be easily confused with the self argument of class methods.

comment:5 by Mariusz Felisiak, 3 years ago

Needs tests: unset

comment:6 by Mariusz Felisiak, 3 years ago

Owner: SShayashi removed
Status: assignednew

comment:7 by Bhuvnesh, 18 months ago

Owner: set to Bhuvnesh
Status: newassigned

comment:8 by Bhuvnesh, 18 months ago

While altering a many to many field , a new table is created (which is later renamed) and data is copied from old table to new table and then old table is deleted. This issue caused while making this new table here, we are only altering the m2m_reverse_fields ignoring the m2m_fields that points to our model. But both m2m_reverse_fieldsand m2m_fields needs to be changed.
Hope i'm proceeding into the right direction.

Last edited 18 months ago by Bhuvnesh (previous) (diff)

comment:9 by Bhuvnesh, 18 months ago

It is also failing for the case if process is reversed, like when we migrate passing self/Foo to the m2m field and then change it to Bar.(due to the same reason that we are not altering m2m_fields)

I tried to alter self pointing field and with the changes below its working as expected for the above issue and passing all the tests as well.

  • django/db/backends/sqlite3/schema.py

    diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py
    index 88fa466f79..970820827e 100644
    a b class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    174174            super().alter_field(model, old_field, new_field, strict=strict)
    175175
    176176    def _remake_table(
    177         self, model, create_field=None, delete_field=None, alter_field=None
     177        self, model, create_field=None, delete_field=None, alter_field=None, alter_self_field=None
    178178    ):
    179179        """
    180180        Shortcut to transform a model from old_model into new_model
    class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    236236                mapping[create_field.column] = self.prepare_default(
    237237                    self.effective_default(create_field),
    238238                )
     239
     240        # Alter field pointing to the model itself.
     241        if alter_self_field:
     242            old_self_field, new_self_field = alter_self_field
     243            body.pop(old_self_field.name, None)
     244            mapping.pop(old_self_field.column, None)
     245            body[new_self_field.name] = new_self_field
     246            mapping[new_self_field.column] = self.quote_name(old_self_field.column)
     247            rename_mapping[old_self_field.name] = new_self_field.name
     248
    239249        # Add in any altered fields
    240250        if alter_field:
    241251            old_field, new_field = alter_field
    class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    507517                        new_field.m2m_reverse_field_name()
    508518                    ),
    509519                ),
     520                alter_self_field=(
     521                    old_field.remote_field.through._meta.get_field(
     522                        old_field.m2m_field_name()
     523                    ),
     524                    new_field.remote_field.through._meta.get_field(
     525                        new_field.m2m_field_name()
     526                    ),
     527                ),
    510528            )
    511529            return
Last edited 18 months ago by Bhuvnesh (previous) (diff)

comment:10 by Carlton Gibson, 18 months ago

Hi Bhuvnesh

I tried to alter self pointing field and with the changes below its working as expected for the above issue and passing all the tests as well.

If the tests are all passing it's worth opening a PR to make review easier. (Make sure to add a new test for the issue here too.)

Thanks.

comment:12 by Bhuvnesh, 18 months ago

Has patch: set

comment:13 by Mariusz Felisiak, 18 months ago

Patch needs improvement: set

comment:14 by Bhuvnesh, 18 months ago

Patch needs improvement: unset

comment:15 by Mariusz Felisiak, 18 months ago

Triage Stage: AcceptedReady for checkin

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 18 months ago

Resolution: fixed
Status: assignedclosed

In 81b1c16:

Fixed #28987 -- Fixed altering ManyToManyField when changing to self-referential.

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