Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#15536 closed Uncategorized (invalid)

ModelForm validation regression in 1.2 when using TimeField with choices

Reported by: 3point2 Owned by: nobody
Component: Forms Version: 1.4
Severity: Normal Keywords: blocker, regression
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is my test case models.py:

from django.db import models
from django import forms


TIME_CHOICES = (
    ("09:00:00", "9 AM"),
    ("17:00:00", "5 PM")
)

class TestModel(models.Model):
    time = models.TimeField(choices=TIME_CHOICES)


class TestForm(forms.ModelForm):
    class Meta:
        model = TestModel

This is what happens with Django 1.1.4:

>>> f = TestForm({"time": "09:00:00"})
>>> f.errors
{}

and this is what happens with Django 1.2.5:

>>> f = TestForm({"time": "09:00:00"})
>>> f.errors
{'time': [u'Value datetime.time(9, 0) is not a valid choice.']}

It looks like in 1.2 the supplied time is being converted into a datetime instance before being compared to the valid choices. It would be great to see this incompatible change from 1.1 fixed or documented.

Change History (9)

comment:1 Changed 5 years ago by 3point2

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

solution: using datetime.time instances for the choices seems to work on both 1.1 and 1.2.

Last edited 5 years ago by 3point2 (previous) (diff)

comment:2 Changed 5 years ago by russellm

  • Component changed from Uncategorized to Forms
  • Keywords blocker regression added
  • milestone set to 1.3
  • Triage Stage changed from Unreviewed to Accepted

If it's a TimeField, it shouldn't be converting anything to datetimes. Since this is a regression, it's a blocker for the 1.3 release.

comment:3 Changed 5 years ago by 3point2

thanks for the quick reply. sorry, i didn't phrase things as precisely as i should have. when i used the word 'datetime' i was referring to the entire datetime module, not just to the datetime.datetime type.

so to put it more precisely, it looks like the ModelForm is converting the TimeField into the correct corresponding datetime.time type, but does this before comparing it to the valid choices (which are ISO time strings). the solution i'm using right now is to just use datetime.time instances in my TIME_CHOICES.

finally, i strongly suspect (but haven't tested) that the same will happen with DateFields (converted to datetime.date instances) and DateTimeFields (converted to datetime.datetime instances).

Last edited 5 years ago by 3point2 (previous) (diff)

comment:4 Changed 5 years ago by russellm

  • Resolution set to invalid
  • Status changed from new to closed

Ok - after a closer look, this is a case of an error in your code that is now being caught.

This change was introduced by r12098 -- the introduction of model-based validation. This improved the validation of inputs across the board, including the case that you have described.

In your specific example, the error that has been caught is the fact that "09:00:00" *isn't* a valid value for a Date. It may be a valid form input, but it isn't a valid database value. As you suspect, this same change will affect other fields -- because a string isn't a valid value for a date field of any stripe.

Django's backwards compatibility guarantee doesn't apply to bugs -- it only applies to features when used correctly (or as documented). In this case, it appears that you were relying on an undocumented looseness in Django's handling of dates. r12098 improved the validation of these values, and your code has been caught in the crossfire.

comment:5 Changed 5 years ago by 3point2

  • Resolution invalid deleted
  • Status changed from closed to reopened

I hope I'm not going to get on anyone's nerves by reopening this, but I think you misunderstood something: Of course "09:00:00" isn't a valid value for a Date. However, my example uses a TimeField, not a DateField. As far as I can tell (http://docs.djangoproject.com/en/1.2/ref/forms/fields/#django.forms.TimeField.input_formats), "09:00:00" is a valid value for a time. Thoughts?

comment:6 follow-up: Changed 5 years ago by russellm

  • Resolution set to invalid
  • Status changed from reopened to closed

Apologies -- I mistyped in my explanation.

No, "09:00:00" is not a valid value for a time. It's a valid *string* that can be converted into a Time value *on a form*, using the format you describe (which is, as you will note in the forms documentation). You're dealing with a Model. It's the model validation that is failing.

comment:7 Changed 5 years ago by 3point2

It seems like setting the time to a valid string directly on the model works fine:

>>> t = TestModel()
>>> t.time = "09:00:00"
>>> t.save()
>>>

This is what happens when I give an invalid time string:

>>> t.time = "09"
>>> t.save()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/vasili/.virtualenvs/testing/lib/python2.6/site-packages/django/db/models/base.py", line 431, in save
    self.save_base(force_insert=force_insert, force_update=force_update)
  File "/home/vasili/.virtualenvs/testing/lib/python2.6/site-packages/django/db/models/base.py", line 495, in save_base
    rows = manager.filter(pk=pk_val)._update(values)
  File "/home/vasili/.virtualenvs/testing/lib/python2.6/site-packages/django/db/models/query.py", line 446, in _update
    query.add_update_fields(values)
  File "/home/vasili/.virtualenvs/testing/lib/python2.6/site-packages/django/db/models/sql/subqueries.py", line 249, in add_update_fields
    val = field.get_db_prep_save(val)
  File "/home/vasili/.virtualenvs/testing/lib/python2.6/site-packages/django/db/models/fields/__init__.py", line 202, in get_db_prep_save
    return self.get_db_prep_value(value)
  File "/home/vasili/.virtualenvs/testing/lib/python2.6/site-packages/django/db/models/fields/__init__.py", line 920, in get_db_prep_value
    return connection.ops.value_to_db_time(self.to_python(value))
  File "/home/vasili/.virtualenvs/testing/lib/python2.6/site-packages/django/db/models/fields/__init__.py", line 908, in to_python
    _('Enter a valid time in HH:MM[:ss[.uuuuuu]] format.'))
ValidationError: Enter a valid time in HH:MM[:ss[.uuuuuu]] format.

comment:8 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:9 in reply to: ↑ 6 Changed 2 years ago by guillaumechorn@…

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset
  • Version changed from 1.2 to 1.4

Replying to russellm:

Apologies -- I mistyped in my explanation.

No, "09:00:00" is not a valid value for a time. It's a valid *string* that can be converted into a Time value *on a form*, using the format you describe (which is, as you will note in the forms documentation). You're dealing with a Model. It's the model validation that is failing.

If I understand correctly, you're saying that choices for a TimeField need to be datetime.time values. However, if I set my choices like so:

TIMES = (
        (u'00:00:00',u'All Day'),
        (u'12:00:00',u'Noon')
    )

I find that while forms reject the '12:00:00' value (same error as above: 'Value datetime.time(12, 0) is not a valid choice.'), they don't reject the '00:00:00' value. This is strange. Based on the first case, it seems like what 3point2 was suggesting is happening: Django converts the '12:00:00' value from the form to datetime.time(12, 0) and then compares it to the valid choices (which are in string format), and throws an error since the two don't match up. After all, if this was simply a model validation issue, wouldn't the error message say 'Value "12:00:00" is not a valid choice.'? But then if you submit '00:00:00' on the form, no error is thrown--which doesn't agree with your explanation *or* 3point2's. So what is going on here?

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