Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validation error codes with string like object #3775

Closed
wants to merge 2 commits into from
Closed

Validation error codes with string like object #3775

wants to merge 2 commits into from

Conversation

johnraz
Copy link
Contributor

@johnraz johnraz commented Dec 26, 2015

This one is here to help compare work done in #3716

@johnraz johnraz mentioned this pull request Dec 26, 2015
Closed
@xordoquy xordoquy added this to the 3.3.3 Release milestone Jan 14, 2016
@xordoquy
Copy link
Collaborator

Adding this to the 3.3.3 milestone to keep on our radars.

@tomchristie
Copy link
Member

Looking feasible at this point. One way to move forward - take a stab at documenting (even if only in the comments) - how does a developer get access to the status codes in an error. Are they visible only if working with the serializer explicitly, or are they available to the existing error handler without any further changes?

@johnraz
Copy link
Contributor Author

johnraz commented Jan 14, 2016

Error codes are available everywhere the error strings are available, so basically at every level. I Will add decent documentation asap (dont have access to my computer until tomorrow).

@johnraz
Copy link
Contributor Author

johnraz commented Jan 15, 2016

So to be more specific:

Are they visible only if working with the serializer explicitly

No.

Are they available to the existing error handler without any further changes?

Yes. See here: https://github.com/tomchristie/django-rest-framework/pull/3775/files#diff-3a4f34405474c71b1b3c0a0a3b2fbf28R32

@johnraz
Copy link
Contributor Author

johnraz commented Jan 21, 2016

@tomchristie : do you want me to go more in depth and come up with an actual markdown doc update or is it enough for now? (I'm open to do it, just asking)

@tomchristie tomchristie mentioned this pull request Jan 27, 2016
Closed
@tomchristie
Copy link
Member

@johnraz - It's okay in it's current state - thank you! Still on the backlog, and still queued up behind other stuff that I'm having to focus on ATM - apologies as ever for the snails pace.

@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
@johnraz johnraz force-pushed the validation-error-codes-string-like-object branch from 7971343 to c62d19f Compare February 23, 2016 23:31
@johnraz
Copy link
Contributor Author

johnraz commented Feb 23, 2016

@tomchristie, @xordoquy rebased, hoping to bump this up a little ;-)

@matheusbrat
Copy link

This is great! Do we need any change on this? If you need any help, please let me know! :D

@johnraz
Copy link
Contributor Author

johnraz commented Feb 25, 2016

it is awaiting final review and :shipit:

@johnraz
Copy link
Contributor Author

johnraz commented Mar 30, 2016

ping @tomchristie: I put quite a few efforts on this and would really like to see it going forward as I need the feature badly and I'm sure i'm not the only one 😄

Just trying to promote the feature a little bit more here 😉

@johnraz johnraz force-pushed the validation-error-codes-string-like-object branch from c62d19f to 2668611 Compare April 7, 2016 13:53
@johnraz johnraz mentioned this pull request Apr 12, 2016
Closed
@tomchristie
Copy link
Member

I'm still lukewarm, but not going to write it off.

ValidationErrorMessage is an extra layer of indirection, in a bit of the codebase that lots of users are going to be scanning a lot of the time. Yes it provides an extra bit of functionality, but it also comes at a cost.

It might be useful to see an example of how you'd use this together with some view code or custom exception handler.

@johnraz
Copy link
Contributor Author

johnraz commented Apr 12, 2016

There is already an example in the tests here, I can put together a more detailed example if that would help you get your head around it, just let me know.

@johnraz
Copy link
Contributor Author

johnraz commented Apr 13, 2016

@tomchristie ?

johnraz added 2 commits August 28, 2016 11:45
@johnraz
Introduce error code for validation errors.
df0d814 
This patch is meant to fix encode#3111, regarding comments made to encode#3137
and encode#3169.

The `ValidationError` will now contain a `code` attribute.

Before this patch, `ValidationError.detail` only contained a
`dict` with values equal to a  `list` of string error messages or
directly a `list` containing string error messages.

Now, the string error messages are replaced with `ValidationError`.

This means that, depending on the case, you will not only get a
string back but a all object containing both the error message and
the error code, respectively `ValidationError.detail` and
`ValidationError.code`.

It is important to note that the `code` attribute is not relevant
when the `ValidationError` represents a combination of errors and
hence is `None` in such cases.

The main benefit of this change is that the error message and
error code are now accessible the custom exception handler and can
be used to format the error response.

An custom exception handler example is available in the
`TestValidationErrorWithCode` test.

We keep `Serializer.errors`'s return type unchanged in
order to maintain backward compatibility.

The error codes will only be propagated to the `exception_handler`
or accessible through the `Serializer._errors` private attribute.
@johnraz
Introduce ValidationErrorMessage
2bf6ee4 
`ValidationErrorMessage` is a string-like object that holds a code
attribute.

The code attribute has been removed from ValidationError to be able
to maintain better backward compatibility.

`ValidationErrorMessage` is abstracted in `ValidationError`'s
constructor
@johnraz johnraz force-pushed the validation-error-codes-string-like-object branch from 2668611 to 2bf6ee4 Compare August 28, 2016 09:46
@johnraz
Copy link
Contributor Author

johnraz commented Aug 28, 2016

@tomchristie: getting back to this one again, rebased on master and also simplified the commit history a bit.
I will probably squash everything up in the end anyway but it made my life easier for the rebase.
It would be awesome to push this forward for next release 😄

@tomchristie
Copy link
Member

Gotcha, will make sure to consider it for 3.5 yup!

@decentral1se
Copy link
Contributor

decentral1se commented Sep 8, 2016 •
edited
Loading

@johnraz Thanks for your great work. I am trying to understand your changes a bit more as I would like to see this arrive soon / help in some way. This PR brings:

  1. We can now do this ValidationError('foo', code='bar')
  2. No breaking changes to existing ValidationError handling code
  3. A new ValidationError.code API to be used as necessary in an api_exception_handler.

Is that correct?

The __str__ for ValidationErrorMessage is off (looks like a string!):

(Pdb) exc
ValidationError({'integer': ['This field is required.'], 'char': ['This field is required.']},)
(Pdb) type(exc)
<class 'rest_framework.exceptions.ValidationError'>
(Pdb) error
'This field is required.'
(Pdb) type(error)
<class 'rest_framework.exceptions.ValidationErrorMessage'>

Finally, is there documentation on the way for this? I could help if needed.

@johnraz
Copy link
Contributor Author

johnraz commented Sep 8, 2016

@lwm you got the grasp of it yes. The fact that the ValidationErrorMessage looks like a string is made on purpose to avoid breaking ValidationError (point 2). There is no documentation yet.

@johnraz
Copy link
Contributor Author

johnraz commented Sep 8, 2016

Mmm one thing I overlooked in what you wrote, the new code API is on ValidationErrorMessage.code, not on ValidationError itself.

@tomchristie
Copy link
Member

Related thread: https://groups.google.com/d/msg/django-rest-framework/fGS5VsYy1Xg/F1J2YTIiAwAJ

@johnraz
Copy link
Contributor Author

johnraz commented Sep 28, 2016

@tomchristie I've just been told the customer I'm working on this for, just started sponsorship on the project 🎉 Hope this will help you push this forward!

@tomchristie
Copy link
Member

👍

@tomchristie tomchristie mentioned this pull request Sep 29, 2016
Merged
26 tasks
This was referenced Oct 10, 2016
Closed
Merged
@tomchristie
Copy link
Member

Now superseded by #4550.
Thanks so much for both your hard work on this, and your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
3.5.0 Release
Development

Successfully merging this pull request may close these issues.
5 participants
@johnraz @xordoquy @tomchristie @matheusbrat @decentral1se