Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#23630 closed Bug (fixed)

AlterModelTable doesn't rename auto-created tables

Reported by: Richard Eames Owned by: Tim Graham
Component: Migrations Version: 1.7
Severity: Release blocker Keywords:
Cc: github@…, Shai Berger Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Shai Berger)

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.

Change History (13)

comment:1 Changed 6 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted

Hi,

Indeed, I can reproduce this issue like so:

Create an app with the following models:

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:

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

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.

comment:2 Changed 6 years ago by Shai Berger

Cc: Shai Berger added

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.

comment:3 in reply to:  2 Changed 6 years ago by Baptiste Mispelon

Resolution: wontfix
Status: newclosed

Replying to 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.

comment:4 Changed 6 years ago by Richard Eames

Resolution: wontfix
Status: closednew

Steps to reproduce:

  1. Create models as such:
class Spam(models.Model):
        class Meta:
                db_table = 'hello_spam'

class Eggs(models.Model):
        class Meta:
                db_table = 'hello_eggs'
        
        m2m = models.ManyToManyField(Spam)
  1. Create, and run, a migration. Tables created:
    $ sqlite3 db.sqlite3 .tables
    django_migrations  hello_eggs         hello_eggs_m2m     hello_spam     
    
  2. Remove the Meta.db_table entries.
  3. Create an empty migration.
  4. Add the following to the migration (this step should be automated, but is blocked on #23629):
    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'
            ),
        ]
    
  5. 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:

migrations.RunPython(lambda a, e: e.alter_db_table(None, 'hello_eggs_m2m', 'spam_eggs_m2m'))

comment:5 Changed 6 years ago by Shai Berger

Description: modified (diff)
Severity: NormalRelease blocker

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.

comment:6 Changed 6 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

comment:7 Changed 6 years ago by Tim Graham

Cc: Andrew Godwin added

My thinking is to add a check in the 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?

comment:8 Changed 6 years ago by Andrew Godwin

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.

comment:9 Changed 6 years ago by Tim Graham

Has patch: set
Patch needs improvement: set

Thanks, Andrew. I made a 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.

comment:10 Changed 6 years ago by Tim Graham

Cc: Andrew Godwin removed

Review from Shai has addressed the hack.

comment:11 Changed 6 years ago by Tim Graham

Patch needs improvement: unset

comment:12 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 41b337efa085b6b9cfdb2cf724d977005ff77e75:

Fixed #23630 -- Made AlterModelTable rename auto-created M2M tables.

Thanks Naddiseo for the report, Andrew Godwin for guidance,
and Shai Berger for review.

comment:13 Changed 6 years ago by Tim Graham <timograham@…>

In 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

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