Opened 3 years ago

Closed 3 years ago

#26521 closed Cleanup/optimization (fixed)

CreateModel allows duplicate field names

Reported by: w0rp Owned by: nobody
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I hit this bug while manually editing a migration file after squashing files. If you define the same field twice in a list of fields for CreateModel, then migrate will take the second field definition without reporting any errors.

class Migration(migrations.Migration):
    operations = [
        migrations.CreateModel(
            name='Foo',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
                ('created', models.DateTimeField(auto_now_add=True, db_index=True)),
                # This one will be used.
                ('created', models.DateField(auto_now_add=True, db_index=True)),
            ],
        ),
    ]

I think the migration code should check for duplicate fields, and then report an error if you have mistakenly defined the same field twice for the fields array.

Change History (10)

comment:1 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Probably wouldn't hurt.

comment:2 Changed 3 years ago by Tim Graham

Has patch: set

comment:3 Changed 3 years ago by Markus Holtermann

I don't know if we really need this check, but if we do that, the manager names need to be unique and so do the base classes.

comment:4 Changed 3 years ago by Markus Holtermann

Patch needs improvement: set
Version: 1.9master

comment:5 Changed 3 years ago by Simon Charette <charettes@…>

Resolution: fixed
Status: newclosed

In 417e083e:

Fixed #26521 -- Validated CreateModel bases, fields and managers for duplicates.

comment:6 Changed 3 years ago by Simon Charette <charettes@…>

In a877a2f8:

Refs #26521 -- Added the duplicated value to CreateModel validation messages.

Thanks Tim for the suggestion.

comment:7 Changed 3 years ago by Collin Anderson

Resolution: fixed
Status: closednew

I think this broke non-model base classes:

  File "/home/collin/comcenter/../comcenter/category/migrations/0001_initial.py", line 9, in <module>
    class Migration(migrations.Migration):
  File "/home/collin/comcenter/../comcenter/category/migrations/0001_initial.py", line 35, in Migration
    bases=(models.Model, comcenter.product.models.WhiteBlackListMixin),
  File "/home/collin/comcenter/django/db/migrations/operations/models.py", line 62, in __init__
    for base in self.bases
  File "/home/collin/comcenter/django/db/migrations/operations/models.py", line 17, in _check_for_duplicates
    for val in objs:
  File "/home/collin/comcenter/django/db/migrations/operations/models.py", line 63, in <genexpr>
    if base is not models.Model)
AttributeError: type object 'WhiteBlackListMixin' has no attribute 'lower'

comment:8 Changed 3 years ago by Simon Charette

Will take care of this. I'm a bit surprised CreateModel is untested with mixins.

comment:9 Changed 3 years ago by Simon Charette <charettes@…>

In f951bb78:

Refs #26521 -- Adjusted CreateModel bases validation to account for mixins.

Thanks Collin for the report.

comment:10 Changed 3 years ago by Simon Charette

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top