Opened 7 years ago
Closed 2 years 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 , 7 years ago
Summary: | makemigrations doesn't catch ManyToManyField type change, leaves DB in inconsistent state → Migration changing ManyToManyField target to 'self' doesn't work correctly |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 7 years ago
Needs tests: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
follow-up: 4 comment:3 by , 7 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?
comment:4 by , 7 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 , 4 years ago
Needs tests: | unset |
---|
comment:6 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:7 by , 2 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:8 by , 2 years 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.
Hope i'm proceeding into the right direction.
comment:9 by , 2 years 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): 174 174 super().alter_field(model, old_field, new_field, strict=strict) 175 175 176 176 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 178 178 ): 179 179 """ 180 180 Shortcut to transform a model from old_model into new_model … … class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): 236 236 mapping[create_field.column] = self.prepare_default( 237 237 self.effective_default(create_field), 238 238 ) 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 239 249 # Add in any altered fields 240 250 if alter_field: 241 251 old_field, new_field = alter_field … … class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): 507 517 new_field.m2m_reverse_field_name() 508 518 ), 509 519 ), 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 ), 510 528 ) 511 529 return
comment:10 by , 2 years 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 , 2 years ago
Has patch: | set |
---|
comment:13 by , 2 years ago
Patch needs improvement: | set |
---|
comment:14 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:15 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I believe it works correctly as long as the new target model isn't 'self'.