Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#12467 closed New feature (fixed)

More helpful error message when loading fixture with invalid date

Reported by: knutin Owned by: raulcd
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 knutin 4 years ago.
12467.2.diff (6.6 KB) - added by julien 3 years ago.
12467.3.diff (6.7 KB) - added by julien 3 years ago.

Download all attachments as: .zip

Change History (14)

Changed 4 years ago by knutin

comment:1 Changed 4 years ago by russellm

  • Component changed from Database layer (models, ORM) to Serialization
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to New feature

comment:3 Changed 3 years ago by raulcd

  • Cc raulcumplido@… added
  • Easy pickings unset
  • Owner changed from nobody to raulcd
  • Status changed from new to assigned
  • UI/UX unset

comment:4 Changed 3 years ago by raulcd

  • Needs tests unset

Pull Request provided here at github with tests.

comment:5 follow-up: Changed 3 years ago by 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.

Changed 3 years ago by julien

comment:6 in reply to: ↑ 5 Changed 3 years ago by charettes

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 3 years ago by julien

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 3 years ago by julien

comment:8 Changed 3 years ago by julien

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

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 3 years ago by akaariai

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 3 years ago by julien

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 3 years ago by julien

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.