django-json-api / django-rest-framework-json-api

JSON:API support for Django REST framework
https://django-rest-framework-json-api.readthedocs.org/
BSD 2-Clause "Simplified" License
1.19k stars 295 forks source link

Improve error handling when accessing request.data in get_serializer_class #325

Open DeadLemon opened 7 years ago

DeadLemon commented 7 years ago

Hi there! I'm trying to specify different serializers depend on user type or request data.

class UserViewSet(serializers.ModelSerializer):
    ...
    def get_serializer_class(self):
        serializers_map = {}
        if self.request.user.is_authenticated:
            pass
        else:
            serializers_map.update({'create': serializers.CreateSerializerForAnonymousUser})
        return serializers_map.get(view.action, None)

So, I get an error: [{'detail': 'JSON parse error - Expecting value: line 1 column 1 (char 0)', 'source': {'pointer': '/data'}, 'status': '400'}] Here is my test code:

class UserEndpointCreateTest(APITestCase):
    def setUp(self):
        self.path = reverse('user-list')
        self.data = {
            'data': {
                'type': format_resource_type('user'),
                'attributes': {
                    'phone_number': '+79000000001',
                    'password': 'password'
                }
            }
        }

    def test_anonymous_user(self):
        response = self.client.post(reverse('user-list'), json.dumps(self.data), content_type='application/vnd.api+json')
        print(response.data)

Also, I tried this code with default rest_framework serializer, renderers and parsers - it works properly. Note that this problem exposes only when I'm trying to access request.user or request.data

sliverc commented 6 years ago

Could it be that this is a simply test setup issue and would be solved with changes of #396?

sliverc commented 6 years ago

As there hasn't been any feedback I assume this has been solved with changes in #396

Closing but if there is still a question concerning this issue please comment and we can reopen.

aradbar commented 4 years ago

@sliverc I just ran into this issue as well. From a quick look it seems that JSONParser uses get_serializer_class() as part of trying to determine the resource_name. So when the view implements a custom get_serializer_class that includes a reference to request.data this error occurs.

https://github.com/django-json-api/django-rest-framework-json-api/blob/master/rest_framework_json_api/parsers.py#L121 https://github.com/django-json-api/django-rest-framework-json-api/blob/master/rest_framework_json_api/utils.py#L55

sliverc commented 4 years ago

@aradbar Thanks for the additional information and the pointer. I see that accessing request.data in get_serializer_class causes issues although in the initial example of this issue it actually accesses request.user which in my opinion should work.

Anyhow I do not really see how this can be fixed while DJA continues supporting polymorphic serializers seamlessly. Or do you have any ideas?

A workaround which would work would be to define resource_name on the view which overwrites get_serializer_class. This might be good enough for affected users who might stumble upon this issue.

aradbar commented 4 years ago

@sliverc I agree with what you said but I think it's problematic for DJA to use get_serializer_class internally, as it is a DRF hook and can contain unexpected logic. I would expect DJA to use serializer_class like it does here: https://github.com/django-json-api/django-rest-framework-json-api/blob/2033bc5c23343b467499e3d5441909cbf55959c8/rest_framework_json_api/parsers.py#L143 With an assertion error that tells the user do define resource_name in case serializer_class is not defined

sliverc commented 4 years ago

@aradbar I think this makes the situation worse for users not using polymorphic serializers as with such a change every user of DJA which has get_serializer_class overwritten will have an error. In the current state only users which access request.data within get_serializer_class do and I would argue accessing request.data is rare as it doesn't sound very resty to me.

Anyhow thinking about it again the error message is confusing. I think it would be good to improve it so states something in the lines that an parser error occurred while calling get_serializer_class as most likely request.data has been access and that overwriting resource_name could solve this issue.

So reoping this issue and marking so this can be addressed.

avacaru commented 1 year ago

I have a similar issue in DJA 6.0.0, but I'm not sure if they are related or I should create a new issue.

Basically the exception handler is calling get_serializer() which together with get_serializer_class() and get_serializer_context() can raise different exceptions.

For example, if you try to add a related object to the serializer context, but that object doesn't exist, a Http404 is raised. The code goes through the exception handler which when it reaches the DJA format_drf_errors() function, the exception is raised again, but never caught.

def format_drf_errors(response, context, exc):
    errors = []
    # handle generic errors. ValidationError('test') in a view for example
    if isinstance(response.data, list):
        for message in response.data:
            errors.extend(format_error_object(message, "/data", response))
    # handle all errors thrown from serializers
    else:
        # Avoid circular deps
        from rest_framework import generics

        has_serializer = isinstance(context["view"], generics.GenericAPIView)
        if has_serializer:
            serializer = context["view"].get_serializer()
            fields = get_serializer_fields(serializer) or dict()
            relationship_fields = [
                format_field_name(name)
                for name, field in fields.items()
                if is_relationship_field(field)
            ]

        for field, error in response.data.items():
            field = format_field_name(field)
            pointer = None
            # pointer can be determined only if there's a serializer.
            if has_serializer:
                rel = "relationships" if field in relationship_fields else "attributes"
                pointer = f"/data/{rel}/{field}"
            if isinstance(exc, Http404) and isinstance(error, str):
                # 404 errors don't have a pointer
                errors.extend(format_error_object(error, None, response))
            elif 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"
                errors.extend(format_error_object(error, pointer, response))
            elif isinstance(error, list):
                errors.extend(format_error_object(error, pointer, response))
            else:
                errors.extend(format_error_object(error, pointer, response))

    context["view"].resource_name = "errors"
    response.data = errors

    return response
sliverc commented 1 year ago

@avacaru No this issue is specifically about accessing request.data in get_serializer_class. Your issue is actually related to #1140 and when implemented as mentioned in https://github.com/django-json-api/django-rest-framework-json-api/issues/1140#issuecomment-1483270751 your issue should be fixed as well.