[Django] #29852: SimpleLazyObject Causing Migrations to be created over and over
Django
-----------------------------------------+------------------------
Reporter: Javier Buzzi | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Reference: https://code.djangoproject.com/ticket/29772
{{{
from django.db import models
from django.utils.functional import SimpleLazyObject
from django.core.validators import MinValueValidator
import datetime
# Create your models here.
class Thing(models.Model):
day =
models.DateTimeField(validators=[MinValueValidator(SimpleLazyObject(datetime.datetime.now))])
}}}
This works great right up until you try running migrations:
1. First time it creates a ` - Create model Thing`
2. Then you run the same command again and again, and will always generate
a new migration ` - Alter field day on thing`
{{{
class Migration(migrations.Migration):
dependencies = [
('app', '0005_auto_20181015_2203'),
]
operations = [
migrations.AlterField(
model_name='thing',
name='day',
field=models.DateTimeField(validators=[django.core.validators.MinValueValidator(datetime.datetime(2018,
10, 15, 22, 3, 41, 390769))]),
),
]
}}}
The issue being that the `now()` is being evaluated and thus is always
different.
I got it 50% working with this diff:
{{{
diff --git a/django/db/migrations/serializer.py
b/django/db/migrations/serializer.py
index 911cf0f..ef2ad43 100644
--- a/django/db/migrations/serializer.py
+++ b/django/db/migrations/serializer.py
@@ -12,7 +12,7 @@ import uuid
from django.db import models
from django.db.migrations.operations.base import Operation
from django.db.migrations.utils import COMPILED_REGEX_TYPE, RegexObject
-from django.utils.functional import LazyObject, Promise
+from django.utils.functional import LazyObject, Promise, SimpleLazyObject
from django.utils.timezone import utc
from django.utils.version import get_docs_version
@@ -273,6 +273,8 @@ def serializer_factory(value):
from django.db.migrations.writer import SettingsReference
if isinstance(value, Promise):
value = str(value)
+ elif isinstance(value, SimpleLazyObject):
+ value = value._setupfunc
elif isinstance(value, LazyObject):
# The unwrapped value is returned as the first item of the
arguments
# tuple.
}}}
Turns the migrations into:
{{{
class Migration(migrations.Migration):
dependencies = [
('app', '0004_auto_20181015_2203'),
]
operations = [
migrations.AlterField(
model_name='thing',
name='day',
field=models.DateTimeField(validators=[django.core.validators.MinValueValidator(datetime.datetime.now)]),
),
]
}}}
While it's a great improvement, it still generates a new one every time.
I'm a little over my head with this one, this code is very dense. I could
keep looking at it tomorrow, but i need someone to point me in the right
direction + where in the world do i put the tests for this thing?? Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/29852>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
------------------------------+------------------------------------
Reporter: Javier Buzzi | Owner: nobody
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
* component: Uncategorized => Migrations
Old description:
New description:
{{{
class Migration(migrations.Migration):
Turns the migrations into:
{{{
class Migration(migrations.Migration):
--
Comment:
I think the problem is this:
{{{
>>> a = SimpleLazyObject(datetime.datetime.now)
>>> b = SimpleLazyObject(datetime.datetime.now)
>>> a == b
}}}
The two must be considered equal to prevent infinite migrations. See
91f701f4fc324cd2feb7dbf151338a358ca0ea18 for a similar issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:1>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Javier Buzzi):
Yup, that looks correct. Only when we compare its `_setupfunc` do we get a
hit. Where exactly does that occur? (The comparison)
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:2>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Javier Buzzi):
@TimGraham Looks like i just hit a snag with the SimpleLazyObject test in
`django/tests/migrations/test_writer.py:256` now sure how to overcome
this, I see the need for this to be evaluated right then and there, and i
also see a need for the underlying object to be compared.. Not sure how
you want to handle this.
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:3>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
As I mentioned on the PR serializing
`MinValueValidator(SimpleLazyObject(datetime.datetime.now))` to
`MinValueValidator(datetime.datetime.now)` won't work as it generates
broken code
{{{#!python
> from datetime import datetime
> from django.core.validators import MinValueValidator
> MinValueValidator(datetime.now)(datetime(2018, 5, 15))
TypeError: can't compare datetime.datetime to builtin_function_or_method
}}}
The only solution I can think of involves adjusting
`SimpleLazyObject.__eq__(other)` to special case `isinstance(other,
SimpleLazyObject)` but that would break some assumptions
e.g. if it was to be changed
{{{#!python
> datetime.datetime.now() == datetime.datetime.now()
False
> SimpleLazyObject(datetime.datetime.now) ==
SimpleLazyObject(datetime.datetime.now)
True # Currently False
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:4>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Javier Buzzi):
@SimonCharette This somewhat does the trick but still break things...
{{{
diff --git a/django/utils/functional.py b/django/utils/functional.py
index 1481bf4a5e..4da48bdb1b 100644
--- a/django/utils/functional.py
+++ b/django/utils/functional.py
@@ -209,6 +209,9 @@ empty = object()
def new_method_proxy(func):
def inner(self, *args):
+ if args and isinstance(args[0], type(self)):
+ return func(self._wrapped, args[0]._wrapped)
if self._wrapped is empty:
self._setup()
return func(self._wrapped, *args)
}}}
What do you suggest?
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:5>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
The more I think of it the more I feel like the infinite migrations here
are expected and this is misuse of `SimpleLazyObject`.
`SimpleLazyObject(foo)` is really just a way of saying ''execute `foo` on
first access but not now'' as demonstrated by the following snippet
{{{#!python
In [1]: from datetime import datetime
In [2]: from django.utils.functional import SimpleLazyObject
In [3]: lazy_now = SimpleLazyObject(datetime.now)
In [4]: lazy_now.strftime("%Y-%m-%d %H:%M:%S")
Out[4]: '2018-10-16 15:26:54'
In [5]: import time; time.sleep(1)
In [6]: lazy_now.strftime("%Y-%m-%d %H:%M:%S")
Out[6]: '2018-10-16 15:26:54'
}}}
In this sense `MinValueValidator(SimpleLazyObject(datetime.now))` is
really just a slightly deferred `datetime.now()` as the first time the
wrapped function is evaluated the returned value is persisted. In other
words `MinValueValidator(SimpleLazyObject(datetime.now))` is really
equivalent to `MinValueValidator(datetime.now())` and is the same class of
problems the `DateTimeField(default=datetime.now())` check was dealing
with.
Now what I think you were trying to do here is provide a validator that
makes sure it's not possible to provide a past datetime value and this was
not working appropriately as the validator was actually performing a check
against the first time the `SimpleLazyObject` wrapped function was
resolved. For this purpose you should be using `django.utils.lazy`
instead.
{{{#!python
In [1]: from datetime import datetime, timedelta
In [2]: from django.utils.functional import lazy
In [3]: lazy_now = lazy(datetime.now, datetime)()
In [4]: from django.core.validators import MinValueValidator
In [5]: validator = MinValueValidator(lazy_now)
In [6]: lazy_now
Out[6]: datetime.datetime(2018, 10, 16, 14, 38, 59, 125760)
In [7]: type(lazy_now)
Out[7]: django.utils.functional.__proxy__
In [8]: validator(datetime.now())
ValidationError: [u'Ensure this value is greater than or equal to
2018-10-16 14:39:24.060516.']
In [9]: validator(datetime.now() + timedelta(seconds=1)) # account for
the function call duration
}}}
The only issue here is that `Promise` serialization assumes `lazy`
[https://github.com/django/django/blob/1c0bf95ff6f0dd32dd0424f0becbdfcf344d37be/django/db/migrations/serializer.py#L275-L276
is always dealing with strings] because the main use of them internally is
for translations through the `gettext` function #21008.
In summary, this is a misuse of `SimpleLazyObject` and `lazy` should have
been used instead but `Promise` serialization currently assumes it's only
used for string values and it should be adjusted to deal with other types
as well. What I suggest doing is inspecting wrapped types of the promise
object and only perform a `str` on the value if the types are `(str,)`.
Else the value should be deconstructed as a `django.utils.functional.lazy`
import and a `lazy(...)()` reconstruction.
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:6>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Javier Buzzi):
@SimonCharette all this makes 100% sense.
As a followup, going back to my original example, what is the acceptable
migration that is to be generated?
{{{
class Migration(migrations.Migration):
initial = True
dependencies = [
]
operations = [
migrations.CreateModel(
name='Thing',
fields=[
('id', models.AutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
('day',
models.DateTimeField(validators=[django.core.validators.MinValueValidator(lazy(datetime.datetime.now,
datetime.datetime))])),
('day2',
models.DateTimeField(validators=[hello.app.models.valid_less_than2])),
],
),
]
}}}
or perhaps
{{{
('day',
models.DateTimeField(validators=[django.core.validators.MinValueValidator(datetime.datetime.now))])),
}}}
If we can agree to what is the desired outcome, perhaps i'll be able to
work backwards.
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:7>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Well I think the current behaviour of the serializer regarding
`SimpleLazyObject` is correct but `Promise` objects handling would need to
be adjusted.
In the end we're facing the same `__eq__` proxy issue I mentioned in
previous comments; `lazy(datetime.datetime.now, datetime.datetime)()`
needs to be passed for the validation to be valid but
`lazy(datetime.datetime.now, datetime.datetime)() !=
lazy(datetime.datetime.now, datetime.datetime)()` because
`datetime.datetime.now() != datetime.datetime.now()`.
I beginning to think that the only way is to resolve this is allow
callables to be passed as `BaseValidator(limit_value)`. That would allow
you to specify `MinValueValidator(datetime.now)` which is easily
deconstructible and doesn't involve promise wrapping. Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:8>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Javier Buzzi):
So something around the lines of?
{{{
diff --git a/django/core/validators.py b/django/core/validators.py
index c1c9cd1c87..c2fb66e241 100644
--- a/django/core/validators.py
+++ b/django/core/validators.py
@@ -317,8 +317,11 @@ class BaseValidator:
def __call__(self, value):
cleaned = self.clean(value)
- params = {'limit_value': self.limit_value, 'show_value': cleaned,
'value': value}
- if self.compare(cleaned, self.limit_value):
+ limit_value = self.limit_value
+ if callable(limit_value):
+ limit_value = self.limit_value()
+ params = {'limit_value': limit_value, 'show_value': cleaned,
'value': value}
+ if self.compare(cleaned, limit_value):
raise ValidationError(self.message, code=self.code,
params=params)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:9>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => wontfix
Comment:
Looks reasonable to me. Let's open a separate ticket for that so that we
have a record of the "wontfix" decision to support `SimpleLazyObject` in
field arguments.
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:10>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Sounds good Tim.
Javier could you file a new feature request for the callable `limit_value`
in `BaseValidator`? The diff you suggested in comment:9 should do. It
would also require tests and docs adjustments but this is something we can
discuss on the new ticket/PR. Thanks for continued efforts here by the
way!
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:11>
Django
------------------------------+------------------------------------
Component: Migrations | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Javier Buzzi):
Yes. The new ticket is here: https://code.djangoproject.com/ticket/29860
--
Ticket URL: <https://code.djangoproject.com/ticket/29852#comment:12>