Skip to content

WIP: Error handling #787

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

Closed
wants to merge 20 commits into from
Closed

Conversation

bpleshakov
Copy link
Contributor

Fixes error handling we discussed in #776

Description of the Change

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@bpleshakov bpleshakov changed the title Patch 5 WIP: Error handling May 13, 2020
@sliverc
Copy link
Member

sliverc commented Jun 28, 2020

@sapiosexual Do you think you will find some time in the near future to work on this PR again?

It would be good to have this merged before we release another version (which I would love to do soon). Let me know.



urlpatterns = [
path(r'^entries-nested/$', DummyTestView.as_view(),
Copy link
Contributor

Choose a reason for hiding this comment

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

regex not needed on path()

@bpleshakov
Copy link
Contributor Author

@sliverc I am about to start.
If nothing will go wrong and I won't get any sudden significant amount of work or any other kind of duties I will definitely start next week.

@nattyg93
Copy link
Contributor

nattyg93 commented Jul 23, 2020

@sapiosexual I'm keen to use your nested serializer feature, so it would be great to get something merged for exception handling. Is there anything I can do to help?

I am currently installing this package from git (commit: b14d8ab) into my project and substituting this exception handler below for the in built:

"""Exception handler with support for nested serializers."""
import inspect

from django.http import Http404
from rest_framework import exceptions
from rest_framework_json_api import utils
from rest_framework_json_api.exceptions import rendered_with_json_api
from rest_framework_json_api.settings import json_api_settings


def format_drf_errors(response, context, exc):
    """Format the drf error as a JSON:API error."""
    errors = []
    context["view"].resource_name = "errors"
    # handle generic errors. ValidationError('test') in a view for example
    if isinstance(response.data, list):
        for message in response.data:
            errors.append(utils.format_error_object(message, "/data", response))
        response.data = errors
        return errors
    # handle all errors thrown from serializers
    for field, error in response.data.items():
        errors.extend(_handle_error_value(response, exc, field, error))
    response.data = errors
    return response


def _handle_error_value(response, exc, field, error, key_prefix=None) -> list:
    field = utils.format_value(field)
    if key_prefix is None:
        key_prefix = "/data/attributes"
    pointer = f"{key_prefix}/{field}"
    if isinstance(error, dict):
        errors = []
        for nested_field, nested_error in error.items():
            errors.extend(
                _handle_error_value(response, exc, nested_field, nested_error, pointer)
            )
        return errors
    if isinstance(exc, Http404) and isinstance(error, str):
        # 404 errors don't have a pointer
        return [utils.format_error_object(error, None, response)]
    if isinstance(error, str):
        classes = inspect.getmembers(exceptions, inspect.isclass)
        # DRF sets the `field` to 'detail' for its own exceptions
        if isinstance(exc, tuple(x[1] for x in classes)):
            pointer = "/data"
        return [utils.format_error_object(error, pointer, response)]
    if isinstance(error, list):
        return [
            utils.format_error_object(message, pointer, response) for message in error
        ]
    return [utils.format_error_object(error, pointer, response)]


def exception_handler(exc, context):
    """Return errors from DJA views formatted as per JSON:API spec."""
    # Import this here to avoid potential edge-case circular imports, which
    # crashes with:
    # "ImportError: Could not import 'rest_framework_json_api.parsers.JSONParser' for API setting
    # 'DEFAULT_PARSER_CLASSES'. ImportError: cannot import name 'exceptions'.'"
    #
    # Also see: https://github.com/django-json-api/django-rest-framework-json-api/issues/158
    from rest_framework.views import exception_handler as drf_exception_handler

    # Render exception with DRF
    response = drf_exception_handler(exc, context)
    if not response:
        return response

    # Use regular DRF format if not rendered by DRF JSON API and not uniform
    is_json_api_view = rendered_with_json_api(context["view"])
    is_uniform = json_api_settings.UNIFORM_EXCEPTIONS
    if not is_json_api_view and not is_uniform:
        return response

    # Convert to DRF JSON API error format
    response = format_drf_errors(response, context, exc)

    # Add top-level 'errors' object when not rendered by DRF JSON API
    if not is_json_api_view:
        response.data = utils.format_errors(response.data)

    return response

I'm not the biggest fan of my implementation being recursive - I'm sure it could be re-factored to be iterative. I haven't had the time to refactor due to other commitments.

Also, unfortunately, the implementation of the ListSerializer (when using nested serializers with many=True) does not supply the index of the item that caused the error. There is not much we can do about this, short of changing its to_internal_value implementation to match that of the ListField. (Which I have done for my project.)

@sliverc
Copy link
Member

sliverc commented Aug 18, 2020

@sapiosexual Any progress on this?

This has been hanging for a bit longer than I hoped for... 😄 I would love to release but I have a feeling without proper support of error handling I would need to temporarily remove the nested serializer support from master.

Also when thinking about issue #796 I actually noticed that this issue affects the nested serializer support. As with nested serializer support the nested attribute is not a value in terms of DRF and could be properly formatted with the field names setting. Issue with this is if we fix this later it won't be backwards compatible. So I think before releasing this needs to be addressed as well (or discussed if opinions differ 😉).

I don't have a lot of time at hand to work on those issues myself. Shall we temporarily remove the support for nested serializers or do you think you would find time to work on this?

Thanks for your feedback.

@sliverc
Copy link
Member

sliverc commented Aug 24, 2020

I have found some time to look into this. Extracted some code bits from here and adjusted it. See #815 Please comment in that PR if I have missed out on anything.

Closing this PR in favor of #815

@sliverc sliverc closed this Aug 24, 2020
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.

4 participants