[Django] #29745: Infinte migrations when changing 'ordering' Meta option that contains expressions
Django
expressions
-------------------------------------+-------------------------------------
Reporter: bacilla- | Owner: nobody
ru |
Type: | Status: new
Uncategorized |
Component: | Version: 2.1
Migrations | Keywords: migration meta
Severity: Normal | ordering expression
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changing a model's meta option `ordering` that contains expressions causes
Django to generate an infinite number of migrations.
Test model:
{{{#!python
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:
{{{#!bash
$ python manage.py makemigrations app
Migrations for 'app':
app/migrations/0001_initial.py
- Create model TestEntity
}}}
Then `ordering` have been changed.
{{{#!python
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:
{{{#!bash
$ 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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29745>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
expressions
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Migrations | Version: 2.1
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage:
ordering expression | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "migrations_bug.tar.gz" added.
test project
Django
expressions
-------------------------------------+-------------------------------------
Reporter: bacilla-ru | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 2.1
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage:
ordering expression | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:1>
Django
expressions
-------------------------------------+-------------------------------------
Reporter: bacilla | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 2.1 => 2.0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
Thanks for filling up a new bug report.
This is caused by the fact `models.Field.__eq__`
[https://github.com/django/django/blob/570402ffd7efd529eb597d86525b80a6fbfca0e7/django/db/models/fields/__init__.py#L495-L499
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 ordering. 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 [https://www.python.org/dev/peps/pep-0520/ 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__`
[https://github.com/django/django/blob/570402ffd7efd529eb597d86525b80a6fbfca0e7/django/db/models/expressions.py#L371-L374
even more]. This would surely be a less invasive change if we plan to
backport the adjustment.
Related to #26257.
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: bacilla | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: bacilla | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by bacilla):
Seems like a working hack.
{{{#!python
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)),
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:4>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Simon Charette
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:5>
Django
state
-------------------------------------+-------------------------------------
Reporter: bacilla | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
Patch which is still missing a few tests
https://github.com/django/django/pull/10378
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:6>
Django
state
-------------------------------------+-------------------------------------
Reporter: bacilla | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
{{{#!python
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))
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:7>
Django
state
-------------------------------------+-------------------------------------
Reporter: bacilla | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 2.0
Severity: Normal | Resolution:
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:8>
Django
state
-------------------------------------+-------------------------------------
Reporter: bacilla | Owner: Simon
| Charette
Component: Migrations | Version: 2.0
Severity: Normal | Resolution: fixed
Keywords: migration meta | Triage Stage: Accepted
ordering expression |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"bc7e288ca9554ac1a0a19941302dea19df1acd21" bc7e288]:
{{{
#!CommitTicketReference repository=""
revision="bc7e288ca9554ac1a0a19941302dea19df1acd21"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29745#comment:9>