Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#33953 closed Bug (fixed)

RenameModel breaking change in 4.1 with ManyToManyField

Reported by: Timothy Thomas Owned by: Iuri de Silvio
Component: Migrations Version: 4.1
Severity: Release blocker Keywords: RenameModel
Cc: Iuri de Silvio 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:
1) Create model with a ManyToManyField and a db_table in the Meta.
2) Rename model and keep db_table the same as before

The generated db schema generated from generating migrations would change the columns on the ManyToMany relationship table to match the model name. This matched the generated sql for interacting with this model as well.

It is possible the db_table not changing is not required.

Example:

class Publication(models.Model):
    title = models.CharField(max_length=30)

class Article(models.Model):
    class Meta:
        db_table = "article"

    headline = models.CharField(max_length=100)
    publications = models.ManyToManyField(Publication)

Changed to:

class OtherArticle(models.Model):
    class Meta:
        db_table = "article"

    headline = models.CharField(max_length=100)
    publications = models.ManyToManyField(Publication)

with the corresponding migration:

    operations = [
        migrations.RenameModel(
            old_name="Article",
            new_name="OtherArticle",
        ),
    ]

Previous behavior (4.0.6):
The corresponding (functional) SQL gets generated:

'SELECT COUNT(*) AS "__count" FROM "publication" INNER JOIN "article_publications" ON ("article"."id" = "article_publications"."publication_id") WHERE "article_publications"."otherarticle_id" = %s'

Current behavior (4.1):
The same SQL gets generated but fails. The schema that is now generated from running the same set of migrations on a fresh db would require:

'SELECT COUNT(*) AS "__count" FROM "publication" INNER JOIN "article_publications" ON ("article"."id" = "article_publications"."publication_id") WHERE "article_publications"."article_id" = %s'

as the column now matches the old model name. However, running that SQL would fail against our existing DB as it has ended up in a state that does not match the behavior of the existing migrations in the current version.

Change History (10)

comment:1 Changed 3 months ago by Carlton Gibson

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

This was changed in afeafd6036616bac8263d762c1610f22241c0187 for #33201, which made RenameModel a no-op where the db_table is specified and unchanged.

Something looks amiss though.

With the models you provide, creating migrations for Article and then the renamed OtherArticle, I see the following in the shell:

>>> from app.models import Publication, OtherArticle
>>> p = Publication.objects.create(title="Testing")
>>> a = OtherArticle.objects.create(headline="Will this work?")
>>> a.publications.add(p)
Traceback (most recent call last):
  File "/Users/carlton/Projects/Django/django/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
  File "/Users/carlton/Projects/Django/django/django/db/backends/sqlite3/base.py", line 369, in execute
    return Database.Cursor.execute(self, query, params)
sqlite3.OperationalError: table article_publications has no column named otherarticle_id

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/carlton/Projects/Django/django/django/db/models/fields/related_descriptors.py", line 1114, in add
    self._add_items(

...[snip]...

  File "/Users/carlton/Projects/Django/django/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
  File "/Users/carlton/Projects/Django/django/django/db/backends/sqlite3/base.py", line 369, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.OperationalError: table article_publications has no column named otherarticle_id
>>> ^D

Relevant DB schema:

sqlite> .tables
app_publication             auth_user_groups          
article                     auth_user_user_permissions
article_publications        django_admin_log          
auth_group                  django_content_type       
auth_group_permissions      django_migrations         
auth_permission             django_session            
auth_user    

sqlite> .schema article
CREATE TABLE IF NOT EXISTS "article" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "headline" varchar(100) NOT NULL);

sqlite> .schema article_publications
CREATE TABLE IF NOT EXISTS "article_publications" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "article_id" bigint NOT NULL REFERENCES "article" ("id") DEFERRABLE INITIALLY DEFERRED, "publication_id" bigint NOT NULL REFERENCES "app_publication" ("id") DEFERRABLE INITIALLY DEFERRED);
CREATE UNIQUE INDEX "article_publications_article_id_publication_id_8dd463d8_uniq" ON "article_publications" ("article_id", "publication_id");
CREATE INDEX "article_publications_article_id_f32c80b0" ON "article_publications" ("article_id");
CREATE INDEX "article_publications_publication_id_7425f904" ON "article_publications" ("publication_id");

comment:2 Changed 3 months ago by Carlton Gibson

Cc: Iuri de Silvio added

comment:3 Changed 3 months ago by Iuri de Silvio

Owner: changed from nobody to Iuri de Silvio
Status: newassigned

I would expect the field name using the db_table to generate their name instead of the model name, but I agree it should not change the existing behavior.

Looks like an easy fix, I'll write a test and try it today.

comment:4 Changed 3 months ago by Iuri de Silvio

Has patch: set

I wrote a broken test and fixed the implementation.

https://github.com/django/django/pull/15993

comment:5 Changed 3 months ago by Iuri de Silvio

Patch needs improvement: set

comment:6 Changed 3 months ago by Mariusz Felisiak

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:7 Changed 3 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 166a3b32:

Fixed #33953 -- Reverted "Fixed #33201 -- Made RenameModel operation a noop for models with db_table."

Regression in afeafd6036616bac8263d762c1610f22241c0187.
This reverts afeafd6036616bac8263d762c1610f22241c0187.

Thanks Timothy Thomas for the report.

comment:8 Changed 3 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 7d5ccbbe:

[4.1.x] Fixed #33953 -- Reverted "Fixed #33201 -- Made RenameModel operation a noop for models with db_table."

Regression in afeafd6036616bac8263d762c1610f22241c0187.
This reverts afeafd6036616bac8263d762c1610f22241c0187.

Thanks Timothy Thomas for the report.

Backport of 166a3b32632c141541d1c3f0eff18e1d8b389404 from main

comment:9 Changed 3 months ago by GitHub <noreply@…>

In a9e7beb:

Refs #33953 -- Fixed test_rename_model_with_db_table_rename_m2m() crash on SQLite < 3.20.

comment:10 Changed 3 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 524ea6b7:

[4.1.x] Refs #33953 -- Fixed test_rename_model_with_db_table_rename_m2m() crash on SQLite < 3.20.

Backport of a9e7beb959bc726eab1c192d2625d6ff6cfa70f4 from main

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