Infinite migrations when using SimpleLazyObject in field arguments

[Javier Buzzi]

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.

[Tim Graham]

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.

[Javier Buzzi]

Yup, that looks correct. Only when we compare its _setupfunc do we get a hit. Where exactly does that occur? (The comparison)

My local tests seem to work, im going to write a proper test as soon as i have time. ​https://github.com/django/django/pull/10516

[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.

[Simon Charette]

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

> 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

> datetime.datetime.now() == datetime.datetime.now()
False
> SimpleLazyObject(datetime.datetime.now) == SimpleLazyObject(datetime.datetime.now)
True  # Currently False

[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?

[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

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()) fields.W161 check is 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.

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 ​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.

[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.

[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 at validator initialization time 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?

[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)

[Tim Graham]

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.

[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!

[Javier Buzzi]

Yes. The new ticket is #29860.