Opened 10 years ago

Last modified 8 years ago

#21523 new Cleanup/optimization

Models DateField to_python method no longer supports mock dates.

Reported by: hugo@… Owned by:
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: hugo@…, Simon Charette 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 Hugo Rodger-Brown 10 years ago.
Patch for the fix.

Download all attachments as: .zip

Change History (37)

comment:1 by hugo@…, 10 years ago

Component: UncategorizedDatabase layer (models, ORM)

comment:2 by hugo@…, 10 years ago

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 by Hugo Rodger-Brown, 10 years ago

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.

by Hugo Rodger-Brown, 10 years ago

Attachment: patch_21523.diff added

Patch for the fix.

comment:4 by Hugo Rodger-Brown, 10 years ago

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 by Claude Paroz, 10 years ago

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 by Hugo Rodger-Brown, 10 years ago

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 by Claude Paroz, 10 years ago

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.

in reply to:  6 comment:8 by Claude Paroz, 10 years ago

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 by Simon Charette, 10 years ago

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 by Simon Charette, 10 years ago

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

in reply to:  10 comment:11 by Hugo Rodger-Brown, 10 years ago

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)

in reply to:  9 comment:12 by Hugo Rodger-Brown, 10 years ago

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 by Simon Charette, 10 years ago

Cc: Simon Charette added

comment:14 by Hugo Rodger-Brown, 10 years ago

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 by Hugo Rodger-Brown, 10 years ago

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 by Aymeric Augustin, 10 years ago

__isinstancecheck__ is indeed designed for this purpose.

comment:17 by Aymeric Augustin, 10 years ago

Triage Stage: UnreviewedAccepted

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 by Hugo Rodger-Brown, 10 years ago

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 by Aymeric Augustin, 10 years ago

#21577 was a duplicate.

comment:20 by Carl Meyer, 10 years ago

Type: UncategorizedCleanup/optimization

comment:21 by Tim Graham, 10 years ago

Has patch: set

comment:22 by Tim Graham, 10 years ago

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 by Claude Paroz, 10 years ago

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

comment:24 by Tim Graham, 9 years ago

Patch needs improvement: unset

comment:25 by Asif Saifuddin Auvi, 9 years ago

Owner: changed from nobody to Asif Saifuddin Auvi
Status: newassigned

comment:26 by Tim Graham, 9 years ago

Patch needs improvement: set

Reviewed the PR.

comment:27 by Asif Saifuddin Auvi, 8 years ago

Owner: Asif Saifuddin Auvi removed
Status: assignednew

comment:28 by Akshesh Doshi, 8 years ago

Owner: set to Akshesh Doshi
Status: newassigned

comment:29 by Akshesh Doshi, 8 years ago

Patch needs improvement: unset

comment:30 by Claude Paroz, 8 years ago

@akki Could you also explore the duck-typing strategy? See https://gist.github.com/claudep/1ec5600d7232bb8007862014a5607c07

comment:31 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:32 by Akshesh Doshi, 8 years ago

@claudep I have updated my PR. Please see if that agrees with what you were looking for.

comment:33 by Claude Paroz, 8 years ago

Patch needs improvement: unset

comment:34 by Philip James, 8 years ago

Triage Stage: AcceptedReady for checkin

The patch looks good, is documented, and has tests.

comment:35 by Tim Graham, 8 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Added some comments for improvement on the PR.

comment:36 by Akshesh Doshi, 8 years ago

Owner: Akshesh Doshi removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top