Avoid calling get_serializer during error handling

[amw]

get_serializer can be a complex method that is allowed to throw errors both itself and in get_serializer_context that it depends on, e.g. query a database and throw Http404. format_drf_errors will attempt to call it again potentially raising the same issue or even a new one due to for example missing required argument (a view may implement def get_serializer(instance) while format_drf_errors always calls it as get_serializer()).

This patch does two things:

1. Uses get_serializer_class instead of get_serializer to find relationship fields. The reasoning is that get_serializer_class should give us the field information that we need while avoiding the additional custom view logic potentially contained in get_serializer or get_serializer_context.
2. Surrounds the code that computes relationship_fields in format_drf_errors in a try-except block to attempt to continue formatting the errors without knowing relationship fields instead of causing 500 error.

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

[sliverc]

Thanks for raising this issue. I see that this could be a problem. I do not think it is a good idea to use get_serializer_class here instead of get_serializer as we actually need an instance of serializer which a user can overwrite the logic how to retrieve it in the view.

Also catching a generic exception might end up in swallowing exceptions and it might just not be clear why a pointer is missing.

So what I suggest instead is to catch the exceptions which are handled by DRF which are APIException, Http404 and PermissionDenied.

Also your change will lead to a pointer always rendered as attributes which might just be wrong. Better is in the except clause to set has_serializer to False. Line below then also needs to be in the try block.

[amw]

That logic for creating the serializer instance along with its context is exactly what can fail and failed in many cases when I was transitioning my application to drf-jsonapi 4.3. Maybe there's a way to read these fields from the class without creating the instance?

I do not agree about catching only DRF exceptions. The reason is that by calling get_serializer/get_serializer_class outside normal view flow and inside our error handler we can trigger new exceptions different than the one currently being processed. The handler is already rendering DRF-compatible exception.

Calling get_serializer here is simply dangerous and can fail leading to 500 error code due to many reasons:
– we might be calling it without proper arguments (TypeError, ValueError)
– we might be calling it after DB transaction error re-running serializer/context queries in a broken transaction (TransactionManagementError)
– we might be calling it for the first time ignoring view's decision that request failed to satisfy requirements for creating serializer instance
– we might be calling it again running code that was not supposed to run multiple times

Calling get_serializer_class could be dangerous or give unexpected results too, but I think less so than getting serializer context and instance. In either case we should gracefully clean up our failed attempts regardless of the exception type and continue to render the error we have received from DRF.

[sliverc]

Calling get_serializer_class instead of get_serializer just moves the problem to a different spot. So I think to solve this problem we need a different approach where access to the serializer is not needed when formatting an error. This could also potentially solve issues like discussed in #1044

Main issue is that the exception resp. error does not reference back to the serializer field itself which raised the error. If the field would be attached then we could simply use that to determine whether it is an attribute or relationship.

So an approach could be to overwrite to_internal_value in Serializer and ModelSerializer within DJA. When a ValidationException has been raised go through the errors and set the serializer field on the error detail. This field can then be retrieved when formatting the error in format_drf_errors.

[amw]

Adding field information in serializer subclass would indeed be a very clean solution, but it wouldn't properly attribute source for errors manually raised in views. I have code like this in a view that does use serializer later in the execution path:

# in view.py before using serializer
raise ValidationError(dict(email=['User not found.']))

It's true that serializer class does have a similar issue, but I think it's to a lesser degree. I think the probability of encountering these errors is lower than with serializer instance and its context.

[sliverc]

I agree it is not ideal although in such a case it could at least be fixed within the client code by passing on the field before raising the exception.

Anyway when thinking about it some more I am not sure Django REST Framework meant the get_serializer method to raise any API exception even when it is called several times (Of course it could always throw an random error which will end up as 500 but those exceptions are not handled in our handler). The reason is that the serializer defines the schema and get_serializer should return a valid serializer without a specific request for this to work. This is used by features like openapi etc. Sometimes even in the DRF code get_serializer gets called twice (see here).

So as initially stated it might make sense to catch handled exceptions (APIException etc.). Other 500 errors which could be caused when get_serializer is called several times in my opinion need to be handled in the client code. And changing to get_serializer_class is not a way forward because it breaks DRF compatibility. Features like dynamic modification of fields which get_serializer is a good spot to do would not work properly.

This is of course my opinion and I am very open to discuss this more. For this a PR is not ideal and it would best to open a discussion to get more people involved.

[amw]

You make good points. I don't think this current situation is good, but I will go back to my app and try to adjust all get_serializer and get_serializer_context to never throw.

I've tried that once already and it was very inconvenient when I needed to 404 on a combination of kwarg and request.user. I could do it in dispatch, but that doesn't have request.user yet or I need to do it in every individual viewset action – and the best shared method for all actions is serializer context where the resource needs to end up anyway.

Feel free to close the PR.

[sliverc]

I see. I guess you could create your own mixin which has a method you can overwrite in your view. This method will then be called before every action in the mixin.

Closing this PR. Still feel free to open a discussion if there is a need.