Opened 7 years ago

Closed 18 months ago

Last modified 6 months ago

#8898 closed Bug (fixed)

`required` validation bypassed when using `DateTimeField` with `SplitDateTimeWidget`.

Reported by: mrmachine Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords: DateTimeField SplitDateTimeWidget
Cc: real.human@…, jarek.zgoda@…, doza@…, gertvangool@…, jgelens@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

DateTimeField.clean calls Field.clean for the required part of its validation, but Field.clean doesn't understand multiple values, and so DateTimeField.clean continues with the assumption that a value has been provided, which raises an invalid error incorrectly when the DateTimeField is *not* required and no values have been provided. This also means that an invalid error is raised instead of a required error when the DateTimeField *is* required and no values have been provided.

Attachments (2)

8898-DateTimeField-r8966.diff (2.0 KB) - added by mrmachine 7 years ago.
8898-DateTimeField-r8983.diff (2.1 KB) - added by mrmachine 7 years ago.
Update tests to match None return value.

Download all attachments as: .zip

Change History (33)

Changed 7 years ago by mrmachine

comment:1 Changed 7 years ago by mrmachine

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

The attachment above includes tests for both cases (required and not required) which fail before the patch is applied, and pass after it is applied.

comment:2 Changed 7 years ago by mrmachine

  • Summary changed from `required` validation bypassed when using `SplitDateTimeWidget`. to `required` validation bypassed when using `DateTimeField` with `SplitDateTimeWidget`.

comment:3 Changed 7 years ago by mrmachine

  • Triage Stage changed from Unreviewed to Design decision needed

After some further digging and discussion in #django-dev, I believe that the validation in DateTimeField.clean which is specific for SplitDateTimeWidget should be removed, and users should be guided to use SplitDateTimeField if they want to use SplitDateTimeWidget.

It seems that the intention is for MultiValueField and MultiValueWidget to be used together, and catching this edge case in DateTimeField to allow it to work with SplitDateTimeWidget actually makes it more difficult for users who might want to subclass DateTimeField and override clean to validate against their own MultiValueWidget, because the super class contains validation specifically for a two item tuple value from MultiValueWidget.

This special case validation doesn't actually work properly now anyway, as it bypasses the required validation.

I'm not sure about the history of DateTimeField or how and why this validation was added, so I'm bumping this to DDN.

comment:4 Changed 7 years ago by zgoda

  • Cc jarek.zgoda@… added

comment:5 Changed 7 years ago by mrmachine

  • Triage Stage changed from Design decision needed to Accepted

As per Karen's request, I'm moving this back to accepted, as the DDN was not intended to raise any red flags. The bug should be fixed, has a working patch with tests, and the previous suggestion to remove SplitDateTimeWidget validation from DateTimeField would be a backwards incompatible change that should have been raised in a separate ticket.

comment:6 Changed 7 years ago by kmtracey

I don't believe the patch is correct as coded. DateTimeField is documented to return None for an empty value (http://docs.djangoproject.com/en/dev/ref/forms/fields/#datetimefield) and the patch has it returning a list in the case of empty input from a SplitDateTimeWidget. But it's not necessary to post another patch because that I could fix myself.

Before fixing this I think it would be good to understand why DateTimeField is even worried about getting input from a SplitDateTimeWidget. As noted above, it seems that is what SplitDateTimeField is for? The code was added in [6577] without reference to a ticket. Malcolm, do you recall why you added this? And do you have any recommendation as to whether it would be better to remove the half-working support entirely, make a fix along the lines of the patch, or ...? Strict backwards-compatibility would seem to require we can't remove it at this point....

comment:7 Changed 7 years ago by kmtracey

  • Patch needs improvement set

comment:8 follow-up: Changed 7 years ago by mrmachine

If we can't remove it, and it seems we can't due to backwards compatibility (regardless of the reasons why it was added), can we get this fix committed? It's currently impossible to use an optional DateTimeField in the Django admin, with an exception being generated if no value is specified.

Changed 7 years ago by mrmachine

Update tests to match None return value.

comment:9 Changed 7 years ago by mrmachine

  • Patch needs improvement unset

Just to make sure that the attached patch shouldn't be considered a red flag or incomplete, I've updated it to return None as per the documentation and updated the tests to match.

comment:10 in reply to: ↑ 8 Changed 6 years ago by jacob

  • Triage Stage changed from Accepted to Design decision needed

Replying to mrmachine:

It's currently impossible to use an optional DateTimeField in the Django admin, with an exception being generated if no value is specified.

That doesn't seem to be true; this works fine for me:

class Bug8898(models.Model):
    timestamp = models.DateTimeField(blank=True, null=True)

I'm thinking there's not a bug here -- as Karen said, SplitDateTimeField is the right choice here -- but if there is can you please clarify how (if at all) this breaks forms and/or the admin?

comment:11 Changed 6 years ago by doza

  • Cc doza@… added

comment:12 Changed 6 years ago by mrmachine

I was using a custom DateTimeField in my models which had a formfield() method that always used django.contrib.localflavor.generic.forms.DateTimeField as the form class so I wouldn't have to set input formats everywhere.

This was overriding the form class specified in BaseModelAdmin.formfield_for_dbfield for DateTimeField db fields and exposing the validation bug in forms.DateTimeField, which was to bypass the "required" validation when used with SplitDateTimeWidget.

In [9760] ModelAdmin.formfield_for_dbfield was re-factored and no longer treated my custom DateTimeField as a models.DateTimeField, because the new code checks if db_field.__class__ is in self.formfield_overrides instead of using isinstance().

To summarise:

  • The bug in forms.DateTimeField when used with SplitDateTimeWidget still exists, though this combination is not normally found in Django core. I suggest that we should still remove this vestigial code to avoid confusion and make subclassing forms.DateTimeField easier. After all, MultiWidget should only be used with MultiValueField.
  • A new bug (I'd call it a bug - it is a change in behaviour) exists where subclassed model fields are not recognised as their parent class in BaseModelAdmin.formfield_for_dbfield(). I suppose the fix for this would be to loop over self.formfield_overrides and check with isinstance() in each iteration. When there's a match, update kwargs and return.

comment:13 Changed 6 years ago by mrmachine

The new bug I stumbled across already has a ticket with patch, #10059, but this vestigial code should still be removed :)

comment:14 Changed 6 years ago by kmtracey

Also related: #9721. That one wants to mix together the SplitDateTimeWidget with a DateTimeField in order to allow for an optional time component. Perhaps that is why DateTimeField was given some support for getting input from a SplitDateTimeWidget in the first place? If so I think a better approach could be to add support for that kind of thing to the SplitDateTimeField/SplitDateTimeWidget combo rather than trying to mix a split widget with a non-split field.

comment:15 Changed 6 years ago by gvangool

  • Cc gertvangool@… added

comment:16 Changed 6 years ago by anonymous

This bug has wider implications - if you have a form field with a MultiWidget, and you set it to required = False, then if you don't input anything in the widgets - it will tell you that you must input a value. The code which is flawed is this, inside clean() of a Field:

if value in EMPTY_VALUES:

return None

if you have 2 widgets then value == [, ] for an empty input, and the above doesn't work.

I will circumvent this in my code, but it'd be nice if it got corrected :)

comment:17 Changed 6 years ago by kmtracey

#12303 reported this again, and provided an alternative fix.

comment:18 Changed 6 years ago by samueladam

Another idea could be to attach EMPTY_VALUES to the Field and have the Widget edit it.

I bypassed this bug using forms.SplitDateTimeField.

comment:19 Changed 5 years ago by lrekucki

Duplicated again by #14044.

comment:20 Changed 5 years ago by carljm

  • Triage Stage changed from Design decision needed to Accepted

I agree with Karen that DateTimeField should not be used with SplitDateTimeWidget, and that adding ad-hoc multi-widget support to formfield clean() methods is a bad idea. That code in DateTimeField.clean() can't be removed immediately due to backwards-compat, but I would consider a patch with a PendingDeprecationWarning that refers people to SplitDateTimeField and allows us to remove that code in a couple releases. This should come with appropriate documentation and a note in the deprecation docs as well.

comment:21 Changed 5 years ago by jgelens

  • Cc jgelens@… added

comment:22 Changed 4 years ago by lukeplant

  • Patch needs improvement set

Marking as patch needs improvement on the basis of Carl's comments.

comment:23 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

See #13205 for a related issue.

comment:24 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:25 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:26 Changed 19 months ago by claudep

  • Patch needs improvement unset

comment:27 Changed 19 months ago by claudep

Reading ticket #9721, it appears that DateTimeField with SplitDateTimeWidget may be used when only the time part of the field is optional, which would not be possible with SplitDateTimeField. To be investigated...

comment:28 Changed 19 months ago by claudep

It may be that #15511 filled the above use case (setting require_all_fields to False, then SplitDateTimeField.fields[1].required to False in a SplitDateTimeField subclass).
@mrmachine, can you confirm?

comment:29 Changed 18 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

Needs a line in the release notes, otherwise LGTM.

comment:30 Changed 18 months ago by Claude Paroz <claude@…>

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

In 0179852d7faf461d55cf3ae69393abb3f3cd2910:

Fixed #8898 -- Obsoleted SplitDateTimeWidget usage with DateTimeField

Thanks Tim Graham for the review.

comment:31 Changed 6 months ago by Tim Graham <timograham@…>

In 714277cb4cedd8290101f9c6b3e6382f192ae177:

Removed support for SplitDateTimeWidget with DateTimeField per deprecation timeline.

refs #8898

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