Opened 9 months ago

Closed 8 months ago

#29745 closed Bug (fixed)

Argument based equality check of BaseExpression causes unstable migration state

Reported by: bacilla Owned by: Simon Charette
Component: Migrations Version: 2.0
Severity: Normal Keywords: migration meta ordering expression
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Changing a model's meta option ordering that contains expressions causes Django to generate an infinite number of migrations.

Test model:

from django.db import models
from django.db.models.functions import Cast, Concat, Right


class TestEntity(models.Model):
    some_property = models.TextField()

    class Meta:
        ordering = ('some_property',)

Makemigrations log:

$ python manage.py makemigrations app
Migrations for 'app':
  app/migrations/0001_initial.py
    - Create model TestEntity

Then ordering have been changed.

class TestEntity(models.Model):
    some_property = models.TextField()

    class Meta:
        ordering = (
            Cast(models.Func(models.F('some_property'), models.Value('^\d+'), function='SUBSTRING'), models.IntegerField()),
            Right(Concat(models.Value('00000'), models.F('some_property')), models.Value(5))
        )

Makemigrations log:

$ python manage.py makemigrations app
Migrations for 'app':
  app/migrations/0002_auto_20180907_1712.py
    - Change Meta options on testentity
$ python manage.py makemigrations app
Migrations for 'app':
  app/migrations/0003_auto_20180907_1713.py
    - Change Meta options on testentity
$ python manage.py makemigrations app
Migrations for 'app':
  app/migrations/0004_auto_20180907_1713.py
    - Change Meta options on testentity

Attachments (1)

migrations_bug.tar.gz (3.0 KB) - added by bacilla 9 months ago.
test project

Download all attachments as: .zip

Change History (10)

Changed 9 months ago by bacilla

Attachment: migrations_bug.tar.gz added

test project

comment:1 Changed 9 months ago by bacilla

Summary: Infinte migrations when changing 'ordering' Meta option that contains expressionsInfinite migrations when changing 'ordering' Meta option that contains expressions

comment:2 Changed 9 months ago by Simon Charette

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug
Version: 2.12.0

Thanks for filling up a new bug report.

This is caused by the fact models.Field.__eq__ is based of creation_counter which makes models.IntegerField() != models.IntegerField() and thus Cast(expr, models.IntegerField()) != Cast(expr, models.IntegerField()).

I feel like this is going to be hard to solve at the Field level without revisiting how we determine field definition order. This has always been a hack to work around unordered type.__new__(attrs) anyway which is not necessary in Python 3.6+ and can be worked around in Python 3.5+ thanks to PEP 520's type.__prepare__ which could be added to ModelBase.

Another solution could be to special case output_field equality checks in BaseExpression.__eq__ even more. This would surely be a less invasive change if we plan to backport the adjustment.

Related to #26257.

Last edited 9 months ago by Simon Charette (previous) (diff)

comment:3 Changed 9 months ago by Simon Charette

Summary: Infinite migrations when changing 'ordering' Meta option that contains expressionsMeta.ordering containing Field instances causes infinite migrations

comment:4 Changed 9 months ago by bacilla

Seems like a working hack.

class ToInteger(models.IntegerField):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.creation_counter = 1


class TestEntity(models.Model):
    some_property = models.TextField()

    class Meta:
        ordering = (
            Cast(models.Func(models.F('some_property'), models.Value('^\d+'), function='SUBSTRING'), ToInteger()),
            Right(Concat(models.Value('00000'), models.F('some_property')), models.Value(5)),
        )

comment:5 Changed 9 months ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:6 Changed 9 months ago by Simon Charette

Has patch: set
Patch needs improvement: set
Summary: Meta.ordering containing Field instances causes infinite migrationsArgument based equality check of BaseExpression causes unstable migration state

Patch which is still missing a few tests

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

Version 0, edited 9 months ago by Simon Charette (next)

comment:7 Changed 9 months ago by Simon Charette

@bacilla by the way a less invasive workaround is to pass Cast(output_field) as a keyword argument instead of a positional one.

class TestEntity(models.Model):
    some_property = models.TextField()

    class Meta:
        ordering = (
            Cast(models.Func(models.F('some_property'), models.Value('^\d+'), function='SUBSTRING'), output_field=models.IntegerField()),
            Right(Concat(models.Value('00000'), models.F('some_property')), models.Value(5))
        )

comment:8 Changed 8 months ago by Simon Charette

Patch needs improvement: unset

comment:9 Changed 8 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In bc7e288:

Fixed #29745 -- Based Expression equality on detailed initialization signature.

The old implementation considered objects initialized with an equivalent
signature different if some arguments were provided positionally instead of
as keyword arguments.

Refs #11964, #26167.

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