Opened 20 months ago

Last modified 4 months ago

#21523 assigned Cleanup/optimization

Models DateField to_python method no longer supports mock dates.

Reported by: hugo@… Owned by: auvipy
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: hugo@…, charettes Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In between 1.4 and 1.5 a change was made to django.db.models.fields.DateField.to_python method that broke the use of mock dates in unit testing. This makes testing of models with mock dates impossible.

The to_python method in the DateField (and DateTimeField) is used to parse field input into the relevant python type. In the case of a DateField this involves checking for datetime.date and datetime.datetime types, and in the event of neither of these matching, it is assumed that the input is a string and an attempt is made to parse the value.

The parse_date function internally performs a regex match, and therefore implicitly expects a string argument. If the argument is not a string an error is raised.

This causes a problem when attempting to mock a date or datetime object for testing purposes. Using the method described (see http://bit.ly/1bYLdIC) tests will fail as the FakeDate mock fails all of the above (it is neither datetime nor date, and is not a string). I have submitted a question on StackOverflow (see http://bit.ly/187sUU9) which describes a concrete example.

In Django 1.4.x a mock Date / DateTime object could be used as prior to the parse_date check a call was made to "django.utils.encoding.smart_str", which converted the FakeDate to a parseable string.

Steps to reproduce:

Create a model with a DateField
Create a test that saves the model using datetime.date.today() as the field value (should pass)
Create a second test that uses a FakeDate (see http://bit.ly/1bYLdIC) to mock the datetime.date and provide a fake today() implementation

This second test should fail with "TypeError: expected string or buffer", as the FakeDate cannot be parsed.

Repeat this test with the 1.4.x branch - test should pass.

Attachments (1)

patch_21523.diff (3.8 KB) - added by hugorodgerbrown 20 months ago.
Patch for the fix.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 20 months ago by hugo@…

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 20 months ago by hugo@…

I haven't written formal tests for this, but initial tests demonstrate that adding a call to django.utils.encoding.smart_text before attempting to parse_date will fix this.

comment:3 Changed 20 months ago by hugorodgerbrown

The commit that removed the smart_str call is [0dc9049] (31-May-2012), which
is marked as a fix for another issue (#18407). Commit note below.

Fixed #18407 -- Made model field's to_python methods fully accept unicode.

When generating error message in to_python, any unicode string
containing non-ascii characters triggered a UnicodeEncodeError for
most field types.

Changed 20 months ago by hugorodgerbrown

Patch for the fix.

comment:4 Changed 20 months ago by hugorodgerbrown

I have attached a patch for this. It adds a call to smart_text back in to the DateField and DateTimeField to_python methods. Patch also includes test for this.

comment:5 Changed 20 months ago by claudep

The fact that smart_str/smart_text is fixing this issue is only accidental. I'm the one who removed the smart_str calls and I am strongly opposed to reintroduce them (and that for a third-party testing utility). I can admit that this is a typical case where isinstance can be harmful. In your use case, the basic problem is that isinstance(value, datetime.date) returns False when value is mocked.
I'm not sure we should even accept this ticket, but I'd like to hear the opinion of another core dev.

comment:6 follow-up: Changed 20 months ago by hugorodgerbrown

Happy for it to not be accepted (I don't have strong feelings either way about the fix),
however not being able to mock DateField or DateTimeField values is fairly terminal
for us; our application is very heavily date-dependent, and we need to be able to
run deterministic date-based tests.

If anyone has any alternative solutions to mocking datetime.date(time) classes
I'd love to hear about it.

comment:7 Changed 20 months ago by claudep

One possible fix would be to replace (in DateField.to_python) if isinstance(value, datetime.datetime) by if hasattr(value, 'tzinfo'), and similarly replace if isinstance(value, datetime.date) by if hasattr(value, 'strftime'), tzinfo and strftime being only suggestion of possible attribute testing. This would be more mocking-friendly at least, and maybe a little more Pythonic.

comment:8 in reply to: ↑ 6 Changed 20 months ago by claudep

Replying to hugorodgerbrown:

If anyone has any alternative solutions to mocking datetime.date(time) classes
I'd love to hear about it.

You should generally be able to manually monkeypatch with this pattern:

from datetime import date

def test_something(self):
    old_today = date.today
    date.today = <your today replacement>
    try:
        # Your testing code here
    finally:
        date.today = old_today

comment:9 follow-up: Changed 20 months ago by charettes

I'm not sure of the actual issue here?

import datetime
from django.db import models

class FakeDate(datetime.date):
    @classmethod
    def today(cls):
        return cls(2000, 1, 1)

assert models.DateField().to_python(FakeDate.today()) == datetime.date(2000, 1, 1)

comment:10 follow-up: Changed 20 months ago by charettes

If anyone has any alternative solutions to mocking datetime.date(time) classes
I'd love to hear about it.

You should be able to decorate your test methods with @today(2000, 1, 1) using the following class:

import datetime
from functools import wraps

class today(object):
    def __init__(*args):
        mocked_today = datetime.date(*args)
        self.mocked_date_cls = type(
            'mocked_date_cls', (datetime.date,),
            {'today': staticmethod(lambda: mocked_today)}
        )

    def __call__(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            with mock.patch('datetime.date', self.mocked_date_cls):
                return func(*args, **kwargs)
        return wrapper

comment:11 in reply to: ↑ 10 Changed 20 months ago by hugorodgerbrown

Replying to charettes:

If anyone has any alternative solutions to mocking datetime.date(time) classes
I'd love to hear about it.

You should be able to decorate your test methods with @today(2000, 1, 1) using the following class:

import datetime
from functools import wraps

class today(object):
    def __init__(*args):
        mocked_today = datetime.date(*args)
        self.mocked_date_cls = type(
            'mocked_date_cls', (datetime.date,),
            {'today': staticmethod(lambda: mocked_today)}
        )

    def __call__(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            with mock.patch('datetime.date', self.mocked_date_cls):
                return func(*args, **kwargs)
        return wrapper

Thanks for this - unfortunately this appears to have reversed the issue. The tests still fail on the isinstance(value, datetime.date) call inside to_python:

> /django/db/models/fields/__init__.py(698)to_python()
    697
--> 698         try:
    699             parsed = parse_date(value)

ipdb> value
datetime.date(2012, 12, 7)
ipdb> isinstance(value, datetime.date)
False
ipdb> type(value)
<type 'datetime.date'>
ipdb> dt = datetime.date(2012, 12, 7)
ipdb> dt
mocked_date_cls(2012, 12, 7)

comment:12 in reply to: ↑ 9 Changed 20 months ago by hugorodgerbrown

Replying to charettes:

I'm not sure of the actual issue here?

import datetime
from django.db import models

class FakeDate(datetime.date):
    @classmethod
    def today(cls):
        return cls(2000, 1, 1)

assert models.DateField().to_python(FakeDate.today()) == datetime.date(2000, 1, 1)

Hi there - this code works fine - but the problem appears when using it within a test / mock scenario, as mocking the datetime.date affects the datetime.date(2000, 1, 1) expression also (i.e. it returns a mock, not a genuine date).

comment:13 Changed 20 months ago by charettes

  • Cc charettes added

comment:14 Changed 20 months ago by hugorodgerbrown

I've gone down a bit of a rabbit hole on this one - in principle mocking can work if you can trap every single instance where a date is created - so not just init and today(), but also operators (as adding timedeltas to dates creates a 'real' date as opposed to the mock), parsing dates using datetime.datetime.strptime (as it exists on a django.forms.DateField), etc.

I'm prepared to side with claudep on this that the removal of smart_str may have simply thrown light on an existing issue of mocking complexity rather than a bug per se, and that therefore this doesn't merit acceptance. That said, the hasattr suggestion (rather that isinstance) would certainly make life easier.

comment:15 Changed 20 months ago by hugorodgerbrown

OK - I've found a workaround to my specific issue (overriding the instancecheck so that real date objects return True to the isinstance(value, datetime.date) check when datetime.date is mocked out), but this feels like an even bigger hack that my original one. That said, I'm happy to drop this issue if people think it's invalid.

comment:16 Changed 20 months ago by aaugustin

__isinstancecheck__ is indeed designed for this purpose.

comment:17 Changed 20 months ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

I took more time to think about this issue today. to_python methods are highly sensitive and we shouldn't change them lightly.

The real question is the contract of the to_python method:

  • Should it accept anything and never crash?
  • Should it guarantee that its result is a suitable Python value for the field?

Of course these are incompatible goals. Taking them to the extreme, the former means that to_python should return unknown types (such as mocks) unchanged and hope duck-typing does the rest; the latter that it should reject them like it currently does.

At this point, restoring a call to str and adding tests starts looking like a good compromise. A date-like object is likely to have an unambiguous string representation in the YYYY-MM-DD format.

If we're specifically thinking about mocks, __isinstancecheck__ is the answer because it's the way to declare "I pretend I'm this other class". But if we're also thinking about date-like objects provided by arbitrary libraries, it makes more sense to rely on __str__ than __isinstancecheck__.

Over the years, Django has moved towards more type safety (and I'm partly responsible for that). For instance it's less forgiving when you mix dates and datetimes. But in this case I believe the robustness principle trumps our (my?) desire for correctness.

comment:18 Changed 20 months ago by hugorodgerbrown

I've submitted a PR for this (https://github.com/django/django/pull/2026) - but I've gone off message slightly in using hasattr(value, 'isoformat') instead of tzinfo or strftime as suggested above.

This way, we maintain the isinstance(value, datetime.datetime) and isinstance(value, datetime.date) checks as-is, but if the object fails both of those, and has an isoformatatribute it gets converted into a string and then parsed using parse_date as currently happens. Using isoformat instead of str or smart_str seems tighter, as if an object has such an attribute it's a reasonable assumption to assume that it thinks it's a date (as opposed to str).

comment:19 Changed 20 months ago by aaugustin

#21577 was a duplicate.

comment:20 Changed 18 months ago by carljm

  • Type changed from Uncategorized to Cleanup/optimization

comment:21 Changed 12 months ago by timgraham

  • Has patch set

comment:22 Changed 12 months ago by timgraham

  • Patch needs improvement set

The latest PR uses the mock library which I don't think we want. Not sure of the merits of the patch and this idea is probably outside the scope of this ticket, but I wonder if it might be worth vendoring unittest.mock from Python 3.3+ so we can use it in our tests and have 2.7/3.2 support.

comment:23 Changed 12 months ago by claudep

I've created the ticket #23289 to discuss mock availability in the test suite.

comment:24 Changed 7 months ago by timgraham

  • Patch needs improvement unset

comment:25 Changed 5 months ago by auvipy

  • Owner changed from nobody to auvipy
  • Status changed from new to assigned

comment:26 Changed 4 months ago by timgraham

  • Patch needs improvement set

Reviewed the PR.

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