Opened 6 years ago

Closed 4 years ago

#11595 closed Cleanup/optimization (fixed)

Fixture validation errors should report their data

Reported by: freyley Owned by: raulcd
Component: Core (Serialization) Version: 1.0
Severity: Normal Keywords: easy-pickings
Cc: wildfire, pterk Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by lrekucki)

For example, here's what django/db/models/fields/__init__.py has for lines 341-348:

def to_python(self, value):
    if value is None:
        return value
    try:
        return int(value)
    except (TypeError, ValueError):
        raise exceptions.ValidationError(_("This value must be an integer."))

Changing line 348 to:

_("(%s) must be an integer." % value))

means that when you convert a text field to a joined object, you know which one's broken where. (Other places in that file do the same thing)

Attachments (2)

fixture_validation.diff (7.4 KB) - added by raulcd 4 years ago.
Added their data to fixture validation errors and tests
fixture_validationv2.diff (7.7 KB) - added by wildfire 4 years ago.
updated version of this patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by russellm

  • Has patch unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by julien

  • Description modified (diff)
  • Keywords easy-pickings added
  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:3 Changed 4 years ago by raulcd

  • Owner changed from nobody to raulcd
  • Status changed from new to assigned

comment:4 Changed 4 years ago by raulcd

It seems the code is changed since the bug was reported. Now the code has dictionaries with default_error_messages. Example for lines 865-857:

    default_error_messages = {
        'invalid': _("This value must be an integer."),
    }

And in the Exception it has the next, lines 884-890:

    def to_python(self, value):
        if value is None:
            return value
        try: 
            return int(value)
        except (TypeError, ValueError):
            raise exceptions.ValidationError(self.error_messages['invalid'])

Should we change the code to:

    default_error_messages = {
        'invalid': _("(%s) must be an integer."),
    }

    def to_python(self, value):
        if value is None:
            return value
        try: 
            return int(value)
        except (TypeError, ValueError):
            msg = self.error_messages['invalid'] % _(str(value))
            raise exceptions.ValidationError(msg)

I am new and that's why I taked an easy_pickings one. But I want to have your ok to prepare my first patch.

Last edited 4 years ago by lrekucki (previous) (diff)

comment:5 Changed 4 years ago by lrekucki

  • Description modified (diff)

comment:6 Changed 4 years ago by lrekucki

The message proposed by the OP is a bit weird. If you insert the non-valid value to the error message you get something like:

(foobar) must be an integer

I'm a bit puzzled by the assumption, that this code is only used by serialization framework. It could've been true 14 months ago, but now it's a part of model validation. I don't want my users to see messages like these.

comment:7 Changed 4 years ago by raulcd

Do you think we had to close it? Maybe 21 months later since it was opened it makes no sense.

comment:8 Changed 4 years ago by russellm

@raulcd -- What you have proposed sounds right to me. My only comment: you don't need to wrap the value in "_()". That marks a string for translation, which isn't something we can do with user input.

@lrekucki's comment is purely about the literal text for the message. The parentheses in "(foobar) must be an integer" is a strange construction. '"foobar" must be an integer' would make more sense.

comment:9 Changed 4 years ago by raulcd

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

comment:10 Changed 4 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

@raulcd -- As described in the contribution guide and HOWTO, you shouldn't mark your own patch as ready for checkin; that's for someone else to decide. We like to get at least 2 sets of eyes on a patch before marking it for commit.

In this case, your patch doesn't have tests, so it can't be ready for checkin.

comment:11 Changed 4 years ago by raulcd

@russellm -- I am sorry I misunderstood the documentation. I thought the meaning of the ready for chekin was set the ticket as "ey guys, can anyone take a look, I did my patch". I will know that for the next one. About the tests I executed the full test suite and as no test failed I thought the "literal" of the error may not be tested. I will prepare the tests and submit the new patch, just a question (as I am new) where do you think is the best location to create the tests? In /trunk/tests/modeltests/validation or create a new folder with the tests. I am sorry for this questions, but as a newbie I want to follow your standards.

Changed 4 years ago by raulcd

Added their data to fixture validation errors and tests

comment:12 Changed 4 years ago by raulcd

  • Needs tests unset

I finally added tests to a new file. It would be better, as someone in the IRC told me, to move them if you think they fit better in another location. (tests/modeltests/validation/test_error_messages.py is the new test file).

As you told me I will not set the ready for chekin, just wait comments. I removed the need tests checkbox (I don't know if I had to do it or someone who review the patch has to do it).

comment:13 Changed 4 years ago by jacob

  • Easy pickings set

comment:14 Changed 4 years ago by wildfire

  • UI/UX unset

The current patch does not apply cleanly to the tree, I am working on this as part of the DjangoCon sprint and will it up.

Changed 4 years ago by wildfire

updated version of this patch

comment:15 Changed 4 years ago by pterk

  • Triage Stage changed from Accepted to Ready for checkin

Is testing all cases that were fixed, applies cleanly to trunk

comment:16 Changed 4 years ago by wildfire

  • Cc wildfire added

comment:17 Changed 4 years ago by pterk

  • Cc pterk added

comment:18 Changed 4 years ago by mtredinnick

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

In [16638]:

Improved error message display during validation errors.

When reporting a validation error (during model validation or fixture
loading, for example), the error messages now report the bad value as
well as the expected type. This can make identifying the offending field
and problem a bit easier.

Fixed #11595. Patch from raulcd and wildfire with supervision from
Russell.

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