AlterModelTable doesn't rename auto-created tables

[Richard Eames]

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.

[Baptiste Mispelon]

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.

[Shai Berger]

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.

[Baptiste Mispelon]

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.

[Richard Eames]

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)

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

   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:

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

[Shai Berger]

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.

[Tim Graham]

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?

[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.

[Tim Graham]

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.

[Tim Graham]

Review from Shai has addressed the hack.

[Tim Graham <timograham@…>]

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.

[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