[Django] #23630: AlterModelTable doesn't rename auto-created tables
Django
----------------------------+--------------------
Reporter: Naddiseo | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
`AlterModelTable` only renames a model's table, but doesn't rename
corresponding tables created for m2m fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Naddiseo | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Hi,
Indeed, I can reproduce this issue like so:
Create an app with the following models:
{{{
#!python
class Foo(models.Model):
pass
class Bar(models.Model):
foos = models.ManyToManyField(Foo)
}}}
Run `makemigrations` to create the initial migration then change the `Foo`
model to:
{{{#!python
class Foo(models.Model):
class Meta:
db_table = 'asdf'
}}}
Since the `db_table` change isn't auto-detected (#23629), also create a
migration (`makemigrations --empty appname`):
{{{#!python
class Migration(migrations.Migration):
dependencies = [
('bug23630', '0001_initial'),
]
operations = [
migrations.AlterModelTable(
name='Foo',
table='asdf',
)
]
}}}
After that, run `migrate` to sync the database.
Once that's done, running `Foo.objects.create()` triggers the following
error:
{{{
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "./django/db/models/manager.py", line 92, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "./django/db/models/query.py", line 372, in create
obj.save(force_insert=True, using=self.db)
File "./django/db/models/base.py", line 591, in save
force_update=force_update, update_fields=update_fields)
File "./django/db/models/base.py", line 619, in save_base
updated = self._save_table(raw, cls, force_insert, force_update,
using, update_fields)
File "./django/db/models/base.py", line 700, in _save_table
result = self._do_insert(cls._base_manager, using, fields, update_pk,
raw)
File "./django/db/models/base.py", line 733, in _do_insert
using=using, raw=raw)
File "./django/db/models/manager.py", line 92, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "./django/db/models/query.py", line 921, in _insert
return query.get_compiler(using=using).execute_sql(return_id)
File "./django/db/models/sql/compiler.py", line 920, in execute_sql
cursor.execute(sql, params)
File "./django/db/backends/utils.py", line 81, in execute
return super(CursorDebugWrapper, self).execute(sql, params)
File "./django/db/backends/utils.py", line 65, in execute
return self.cursor.execute(sql, params)
File "./django/db/utils.py", line 94, in __exit__
six.reraise(dj_exc_type, dj_exc_value, traceback)
File "./django/utils/six.py", line 549, in reraise
raise value.with_traceback(tb)
File "./django/db/backends/utils.py", line 65, in execute
return self.cursor.execute(sql, params)
File "./django/db/backends/sqlite3/base.py", line 485, in execute
return Database.Cursor.execute(self, query, params)
django.db.utils.OperationalError: no such table: asdf
}}}
Ideally, the `AlterModelTable` operation should be smart enough rename the
M2M and foreignkeys pointing to the model whose table has changed.
If that's not possible, the limitation should definitely be documented.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:1>
Django
Reporter: Naddiseo | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* cc: shaib (added)
Comment:
comment:1 : The error seems to be about table `asdf`, the named table of
the model. Are you sure you ran the migration?
Also, according to https://docs.djangoproject.com/en/dev/ref/models/fields
/#ref-manytomany changing the table name for model Foo should not change
anything; the M2M table should still be named `app_bar_foos`. A change
should happen if the `Bar` table name is modified.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:2>
Django
Reporter: Naddiseo | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* status: new => closed
* resolution: => wontfix
Comment:
Replying to [comment:2 shaib]:
> Are you sure you ran the migration?
Ugh, not sure what I did there but I can't seem to be able to reproduce it
now so probably you're right.
I also don't see any reason to rename the M2M table, so I'm marking this
as `wontfix`.
Feel free to reopen this ticket if you have some more arguments for this
feature, or you can also raise the issue on the django-developers mailing
list.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:3>
Django
Reporter: Naddiseo | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Naddiseo):
* status: closed => new
* resolution: wontfix =>
Comment:
Steps to reproduce:
1. Create models as such:
{{{#!python
class Spam(models.Model):
class Meta:
db_table = 'hello_spam'
class Eggs(models.Model):
class Meta:
db_table = 'hello_eggs'
m2m = models.ManyToManyField(Spam)
}}}
2. Create, and run, a migration. Tables created:
{{{
$ sqlite3 db.sqlite3 .tables
django_migrations hello_eggs hello_eggs_m2m hello_spam
}}}
3. Remove the `Meta.db_table` entries.
4. Create an empty migration.
5. Add the following to the migration (this step should be automated, but
is blocked on #23629):
{{{#!python
class Migration(migrations.Migration):
dependencies = [
('spam', '0001_initial'),
]
operations = [
# Rename the models to what they should default to
migrations.AlterModelTable(
name = 'Spam',
table = 'spam_spam'
),
migrations.AlterModelTable(
name = 'Eggs',
table = 'spam_eggs'
),
]
}}}
6. Run migrations. Current tables:
{{{
$ sqlite3 db.sqlite3 .tables
django_migrations hello_eggs_m2m spam_eggs spam_spam
}}}
As you can see, the m2m table isn't renamed, and one would have to
manually add the following to the migration:
{{{#!python
migrations.RunPython(lambda a, e: e.alter_db_table(None, 'hello_eggs_m2m',
'spam_eggs_m2m'))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:4>
Django
Reporter: Naddiseo | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* severity: Normal => Release blocker
Old description:
> `AlterModelTable` only renames a model's table, but doesn't rename
> corresponding tables created for m2m fields.
New description:
According to https://docs.djangoproject.com/en/dev/ref/models/fields/#ref-
manytomany, when a m2m field uses an automatic (implicit) through model,
the through model's table is named by combining the containing model's
table-name and the field name. Consequently, if a model contains such m2m
fields and its table name is changed by a migration, the m2m field's
tables should also be renamed.
This doesn't happen (see comment:4 and comment:5 for details), leading to
broken models.
--
Comment:
I've been able to reproduce the problem as described in comment:4. I've
also verified that this is independent of whether the (main) models use
the default table name -- the behavior is the same when changing from non-
default to default (as in comment:4), from default to non-default, or
between two different non-default names.
Marking as a release blocker as well -- this is, IMO, as important as
#23629.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:5>
Django
Reporter: Naddiseo | Owner: timgraham
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => assigned
* owner: nobody => timgraham
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:6>
Django
Reporter: Naddiseo | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: andrewgodwin (added)
Comment:
My thinking is to add a check in the
[https://github.com/django/django/blob/3a34e45fdbecc1f1ead0a3c2f1c01111a865710e/django/db/migrations/autodetector.py#L923-L929
generate_altered_db_table()] method and add an `AlterModelTable` operation
for the implicit M2M model to fix this, but `m2m_field.rel.through` is
returning `None` there instead of behaving the way it does in a normal
shell: `Eggs._meta.get_field('m2m').rel.through._meta.db_table` ->
`'hello_eggs_m2m'`. Andrew, do you have any guidance?
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:7>
Django
Reporter: Naddiseo | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
tim: During autodetection, the relations occasionally don't have
rel.through populated and I'm not sure why that's the case. There's a
couple of checks elsewhere for this same behaviour, but I think before it
was just ForeignKeys having an unused through or something (it's been long
enough that I can't remember).
Technically, the rename should be done by AlterModelTable rather than by
the autogenerator making extra stanzas of changes (in the same way that
RenameField also modifies FKs that point to it), but I don't mind this
solution either if it works well.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:8>
Django
Reporter: Naddiseo | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
Thanks, Andrew. I made a [https://github.com/django/django/pull/3409 PR]
based on your advice, although there's one thing there I'm not quite sure
about (noted with a "hack" comment). If you could take a look and advise
I'd appreciate it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:9>
Django
Reporter: Naddiseo | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by timgraham):
* cc: andrewgodwin (removed)
Comment:
Review from Shai has addressed the hack.
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:10>
Django
Reporter: Naddiseo | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:11>
Django
Reporter: Naddiseo | Owner: timgraham
Component: Migrations | Version: 1.7
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"41b337efa085b6b9cfdb2cf724d977005ff77e75"]:
{{{
#!CommitTicketReference repository=""
revision="41b337efa085b6b9cfdb2cf724d977005ff77e75"
Fixed #23630 -- Made AlterModelTable rename auto-created M2M tables.
Thanks Naddiseo for the report, Andrew Godwin for guidance,
and Shai Berger for review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:12>
Django
Reporter: Naddiseo | Owner: timgraham
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f70a733abc7eb9b585e11be1167513d972f9d419"]:
{{{
#!CommitTicketReference repository=""
revision="f70a733abc7eb9b585e11be1167513d972f9d419"
[1.7.x] Fixed #23630 -- Made AlterModelTable rename auto-created M2M
tables.
Thanks Naddiseo for the report, Andrew Godwin for guidance,
and Shai Berger for review.
Backport of 41b337efa0 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23630#comment:13>