Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#12467 closed New feature (fixed)

More helpful error message when loading fixture with invalid date

Reported by: Knut Nesheim Owned by: Raúl Cumplido
Component: Core (Serialization) Version: master
Severity: Normal Keywords:
Cc: raulcumplido@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It would be extremely helpful to have a more helpful error message when loading a fixture an invalid date. This would allow me to actually see the invalid data and I may then correct the data.

Attached is a very simple patch that will include the invalid data in the exception string. It would be extremely helpful to see more data, like the model and pk.

Attachments (3)

12467.diff (684 bytes) - added by Knut Nesheim 7 years ago.
12467.2.diff (6.6 KB) - added by Julien Phalip 5 years ago.
12467.3.diff (6.7 KB) - added by Julien Phalip 5 years ago.

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by Knut Nesheim

Attachment: 12467.diff added

comment:1 Changed 7 years ago by Russell Keith-Magee

Component: Database layer (models, ORM)Serialization
Needs documentation: unset
Needs tests: set
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 6 years ago by Matt McClanahan

Severity: Normal
Type: New feature

comment:3 Changed 5 years ago by Raúl Cumplido

Cc: raulcumplido@… added
Easy pickings: unset
Owner: changed from nobody to Raúl Cumplido
Status: newassigned
UI/UX: unset

comment:4 Changed 5 years ago by Raúl Cumplido

Needs tests: unset

Pull Request provided here at github with tests.

comment:5 Changed 5 years ago by Julien Phalip

The patch looks good. While we're at it, we should also do the same for DateField's "invalid_date" and DateTimeField's "invalid" messages. See amended patch attached.

Changed 5 years ago by Julien Phalip

Attachment: 12467.2.diff added

comment:6 in reply to:  5 Changed 5 years ago by Simon Charette

Replying to julien:

The patch looks good. While we're at it, we should also do the same for DateField's "invalid_date" and DateTimeField's "invalid" messages. See amended patch attached.

That would be a great addition. I have one question concerning the attached patch.

How are the ValueError risen by datetime.date calls supposed to be translated? I see you're calling _(str(e)) before formating the invalid_date message but I can't find where django is supposed to catch i.e. _('month must be in 1..12') when makemessages logic is invoked.

comment:7 Changed 5 years ago by Julien Phalip

That's a good question. One way would be to purposely trigger exceptions while the module is loaded to gather all the various error messages from datetime.datetime(), but that feels dirty... This is also dangerous as there's no guarantee that the messages will be the same on every version of Python.

The most complete way would be for Django to do the same tests as datetime.datetime() and then provide its own messages. However, this wouldn't be very robust and would be annoying to maintain.

Perhaps the safest way is to simply not give any granular error message and just say that it's an invalid date, as suggested in the new patch attached.

Changed 5 years ago by Julien Phalip

Attachment: 12467.3.diff added

comment:8 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: assignedclosed

In [16966]:

Fixed #12467 -- Made the model data validation for DateField and DateTimeField more useful by actually telling what was the value that failed. Also did a bit of PEP8 cleanup in the area. Thanks to knutin for the report, to raulcd for the initial patch and to charettes for the review.

comment:9 Changed 5 years ago by Anssi Kääriäinen

The commit made a couple of always True asserts:

/home/akaj/Django/django_test/django/db/models/fields/__init__.py:513: SyntaxWarning: assertion is always true, perhaps remove parentheses?
  assert (kwargs.get('primary_key', False) is True,
/home/akaj/Django/django_test/django/db/models/fields/__init__.py:539: SyntaxWarning: assertion is always true, perhaps remove parentheses?

comment:10 Changed 5 years ago by Julien Phalip

Well spotted. This is weird, I didn't see those warnings when running the tests. I'll fix that in a minute. Thanks!

comment:11 Changed 5 years ago by Julien Phalip

In [16971]:

Fixed a couple of assert syntax warnings mistakingly introduced in r16966 and added some tests to prevent future regressions. Thanks to Anssi Kääriäinen for the report.
Refs #12467.

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