[Django] #28987: makemigrations doesn't catch ManyToManyField type change, leaves DB in inconsistent state
Django
inconsistent state
----------------------------------------+---------------------------------
Reporter: MSleepyPanda | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 2.0
Severity: Normal | Keywords: ManyToManyField
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+---------------------------------
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"
--
Ticket URL: <https://code.djangoproject.com/ticket/28987>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 2.0
Keywords: ManyToManyField | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
I believe it works correctly as long as the new target model isn't 'self'.
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:1>
Django
Reporter: MSleepyPanda | Owner: SShayashi
Type: Bug | Status: assigned
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by SShayashi):
* status: new => assigned
* owner: nobody => SShayashi
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:2>
Django
---------------------------------+-------------------------------------
Reporter: MSleepyPanda | Owner: SShayashi
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:3>
Django
---------------------------------+-------------------------------------
Reporter: MSleepyPanda | Owner: SShayashi
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by MSleepyPanda):
Replying to [comment:3 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:4>
Django
---------------------------------+-------------------------------------
Reporter: MSleepyPanda | Owner: SShayashi
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:5>
Django
---------------------------------+------------------------------------
Type: Bug | Status: new
Component: Migrations | Version: 2.0
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: SShayashi => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:6>
Django
---------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* owner: (none) => Bhuvnesh
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:7>
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Bhuvnesh):
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
[https://github.com/django/django/blob/main/django/db/backends/sqlite3/schema.py#L496
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:8>
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Bhuvnesh):
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}}})
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:9>
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:10>
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Bhuvnesh):
[https://github.com/django/django/pull/16281 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:11>
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:12>
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:13>
Django
---------------------------------+------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:14>
Django
Reporter: MSleepyPanda | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ManyToManyField | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: MSleepyPanda | Owner: Bhuvnesh
Component: Migrations | Version: 2.0
Severity: Normal | Resolution: fixed
Keywords: ManyToManyField | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"81b1c167bf919ddbd5aa0289f9f3761fc62addf3" 81b1c16]:
{{{
#!CommitTicketReference repository=""
revision="81b1c167bf919ddbd5aa0289f9f3761fc62addf3"
Fixed #28987 -- Fixed altering ManyToManyField when changing to self-
referential.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28987#comment:16>