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.

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

get_error_detail: use error_list to get code(s) #5785

Merged
merged 1 commit into from Jul 6, 2018

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 29, 2018

This is necessary to get code(s) for validation errors.

Via e.g. password_validation.validate_password and get_error_detail used in

errors[field.field_name] = get_error_detail(exc)
then.

TODO:

  • clean up (uncovered/unnecessary code)

@blueyed
Copy link
Contributor Author

blueyed commented Jan 29, 2018

None of the current test is failing.

Where should I add a new one?
A new class in tests/test_fields.py ?

@blueyed blueyed changed the title [WIP] get_error_detail: use error_list to get code(s) [RFC] get_error_detail: use error_list to get code(s) Jan 29, 2018
@tomchristie
Copy link
Member

tomchristie commented Jan 29, 2018

Where should I add a new one?
A new class in tests/test_fields.py ?

I guess so, tho I'm not sure what the failing case is?

@blueyed
Copy link
Contributor Author

blueyed commented Jan 29, 2018

what the failing case is?

The problem is with validate_* methods returning a list of errors (wrapped in Django's ValidationError).
I've added a (previously failing) test for it now.

@blueyed
Copy link
Contributor Author

blueyed commented Jan 29, 2018

btw: using/calling password_validation.validate_password in serializer.save in a @detail_route view goes through handle_exception, without mangling the original exception.

blueyed added a commit to lock8/django-rest-framework that referenced this pull request Jan 29, 2018
@carltongibson
Copy link
Collaborator

carltongibson commented Jan 30, 2018

@blueyed I merged #5787. Do you want to update the test here.

btw: using/calling password_validation.validate_password in serializer.save in a @detail_route view goes through handle_exception, without mangling the original exception.

Sorry. (I always struggle to follow) Is there an extra behaviour you're looking for here?

[RFC]...

This seems good to me: additional info captured; I can't see a BC issue. Was there anything you wanted to add?

@carltongibson carltongibson added this to the 3.8 Release milestone Jan 30, 2018
@blueyed blueyed changed the title [RFC] get_error_detail: use error_list to get code(s) get_error_detail: use error_list to get code(s) Jan 30, 2018
@blueyed
Copy link
Contributor Author

blueyed commented Jan 30, 2018

Is there an extra behaviour you're looking for here?

No, I've just mentioned that in this case it is ok.

Was there anything you wanted to add?

No, it's ready - but should be squash-merged.
Let me know if I should push it again.

@rpkilby rpkilby self-requested a review Jan 30, 2018
blueyed added a commit to lock8/django-rest-framework that referenced this pull request Feb 20, 2018
@alexgmin
Copy link

alexgmin commented Mar 9, 2018

This change throws an exception on Django 1.11 if you raise a Django ValidationError with a dict as a message.

ValidationError({'email': 'That email is already being used.'})

Before this the dict was transformed into a list, removing the dict key.

@carltongibson carltongibson removed this from the 3.8 Release milestone Apr 3, 2018
@blueyed blueyed force-pushed the get_error_detail-code branch from ed6c97f to c42df55 Compare May 3, 2018
@blueyed
Copy link
Contributor Author

blueyed commented May 3, 2018

I've added some more tests and fixed the code for them.
Please review again.

@alexgmin
Does it work now for you?
If not, what exception do you see?

code=expected_code
)
]
}
Copy link
Contributor Author

@blueyed blueyed May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not what you would expect really, but tests the current behavior.
Django itself loses the code and key when wrapping dict_errors into a list with ValidationError it seems.

@alexgmin
Copy link

alexgmin commented May 3, 2018

No exceptions anymore. However, this code

def validate(self, attrs):
    raise DjangoValidationError({'email': 'That email is already being used.'})

Before this pull request resulted in

{
    "non_field_errors": [
        "That email is already being used."
    ]
}

now it's

{
    "email": [
        "That email is already being used."
    ]
}

So this fixes another problem that I had when transforming DjangoValidationErrors to RestFrameworkValidation errors, which was solved with this:

try:
    ...
except DjangoValidationError as e:
    # Django Rest Framework transforms Django Validation errors to lists,
    # removing the key indicating the field.
    if hasattr(e, 'error_dict'):
        raise ValidationError(e.message_dict)
    else:
        raise ValidationError(detail=as_serializer_error(e))

detail = ErrorDetail(error.message % (error.params or ()),
code=error.code if error.code else code)
else:
detail = _get_error_detail_dict(error, code)
Copy link
Contributor Author

@blueyed blueyed May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code/line is not executed. Need to look into this, likely also wrapping it in a list, not sure if that's a valid use case though.

@tomchristie
Copy link
Member

tomchristie commented May 8, 2018

@blueyed Do the test cases in this pull request fail without the code changes?

@blueyed
Copy link
Contributor Author

blueyed commented May 8, 2018

@tomchristie
Yes:

______________________________________________________________ TestValidationErrorCode.test_validationerror_code_with_msg[True] ______________________________________________________________
tests/test_fields.py:2205: in test_validationerror_code_with_msg
    assert serializer.errors['password'][0].code == 'exc_code'
E   AssertionError: assert 'invalid' == 'exc_code'
E     - invalid
E     + exc_code
__________________________________________________________ TestValidationErrorCode.test_validationerror_code_with_dict[False-None] ___________________________________________________________
tests/test_fields.py:2241: in test_validationerror_code_with_dict
    assert serializer.errors == {
E   AssertionError: assert {'non_field_e...e='invalid')]} == {'email': ['email error']}
E     Left contains more items:
E     {'non_field_errors': [ErrorDetail(string='email error', code='invalid')]}
E     Right contains more items:
E     {'email': ['email error']}
E     Use -v to get the full diff
________________________________________________________ TestValidationErrorCode.test_validationerror_code_with_dict[False-exc_code] _________________________________________________________
tests/test_fields.py:2241: in test_validationerror_code_with_dict
    assert serializer.errors == {
E   AssertionError: assert {'non_field_e...e='invalid')]} == {'email': ['email error']}
E     Left contains more items:
E     {'non_field_errors': [ErrorDetail(string='email error', code='invalid')]}
E     Right contains more items:
E     {'email': ['email error']}
E     Use -v to get the full diff
_________________________________________________________ TestValidationErrorCode.test_validationerror_code_with_dict[True-exc_code] _________________________________________________________
tests/test_fields.py:2232: in test_validationerror_code_with_dict
    assert serializer.errors == {
E   AssertionError: assert {'non_field_e...e='invalid')]} == {'non_field_er...='exc_code')]}
E     Differing items:
E     {'non_field_errors': [ErrorDetail(string='email error', code='invalid')]} != {'non_field_errors': [ErrorDetail(string='email error', code='exc_code')]}
E     Use -v to get the full diff
______________________________________________________ TestValidationErrorCode.test_validationerror_code_with_dict_list_same_code[None] ______________________________________________________
tests/test_fields.py:2263: in test_validationerror_code_with_dict_list_same_code
    assert serializer.errors == {
E   AssertionError: assert {'non_field_e...e='invalid')]} == {'email': [Err...e='invalid')]}
E     Left contains more items:
E     {'non_field_errors': [ErrorDetail(string='email error 1', code='invalid'),
E                           ErrorDetail(string='email error 2', code='invalid')]}
E     Right contains more items:
E     {'email': [ErrorDetail(string='email error 1', code='invalid'),
E                ErrorDetail(string='email error 2', code='invalid')]}
E     Use -v to get the full diff
____________________________________________________ TestValidationErrorCode.test_validationerror_code_with_dict_list_same_code[exc_code] ____________________________________________________
tests/test_fields.py:2263: in test_validationerror_code_with_dict_list_same_code
    assert serializer.errors == {
E   AssertionError: assert {'non_field_e...e='invalid')]} == {'email': [Err...='exc_code')]}
E     Left contains more items:
E     {'non_field_errors': [ErrorDetail(string='email error 1', code='invalid'),
E                           ErrorDetail(string='email error 2', code='invalid')]}
E     Right contains more items:
E     {'email': [ErrorDetail(string='email error 1', code='exc_code'),
E                ErrorDetail(string='email error 2', code='exc_code')]}
E     Use -v to get the full diff
====================================================================== 6 failed, 267 passed, 1 skipped in 2.41 seconds =======================================================================

@blueyed blueyed force-pushed the get_error_detail-code branch 2 times, most recently from 2c81d66 to b2aa754 Compare May 23, 2018
@blueyed blueyed force-pushed the get_error_detail-code branch from b2aa754 to d9ee7d6 Compare Jun 19, 2018
@blueyed
Copy link
Contributor Author

blueyed commented Jun 19, 2018

Updated and rebased.
Removed the previous uncovered code: error_list won't contain items with error_dict (https://github.com/django/django/blob/83986af95dc432e54ba71f0c5c13f5e5c02c92e5/django/core/exceptions.py#L128-L129).

@carltongibson carltongibson self-requested a review Jun 21, 2018
@carltongibson carltongibson added this to the 3.8.3 Release milestone Jun 21, 2018
Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This looks good to me. (Thanks @blueyed.)

@rpkilby: Any comments? (If not merge it! 🙂)

@rpkilby rpkilby removed their request for review Jun 22, 2018
@rpkilby
Copy link
Member

rpkilby commented Jun 22, 2018

I've removed myself from the reviewer list.

Copy link
Member

@rpkilby rpkilby left a comment

Alright, finally took a look. One minor concern - I'm not sure if the tests as-is are a good candidate for parametrization. Keeping track of the various conditions (eg, in test_validationerror_code_with_dict) makes the test logic a little harder to follow. I typically prefer the expected result to be explicit instead of partially computed.

The tests might be a little more terse/easier to write if they focused on the get_error_detail instead of having to write/test a full serializer class. Possibly write several parameterized unit tests for get_error_detail, then a couple of integration tests for the entire serializer validation/error handling workflow.

Either way, PR looks good as-is. Test changes are just a thought/suggestion.

@blueyed
Copy link
Contributor Author

blueyed commented Jun 30, 2018

@rpkilby
Thanks for your feedback!

I think that light/clear tests are important and will look into simplifying them, but
it might take some time.

@carltongibson
Copy link
Collaborator

carltongibson commented Jul 6, 2018

Lets take this as-is now. It finishes the run of changes to the error structures, and it'll be good to have it. Any work on the tests later/separately is obviously appreciated.

Thanks @blueyed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants