encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.39k stars 6.84k forks source link

Add context to exception handler. #2236

Closed gaffney closed 9 years ago

gaffney commented 9 years ago

I propose to add the request (or some request context object) to the exception handler method signature.

exception_handler(exc) to exception_handler(request, exc)

This would cover a legitimate use case wherein the user needs contextual information to determine how to handle the error. For instance, the user might want to silently ignore harmless errors (say, requests that have 'WhiteHat Security' in the HTTP_USER_AGENT), whereas for all other requests the user would want to track the exception via analytics tools and/or bug tracking applications.

jpadilla commented 9 years ago

Worth noting that the last conversation about this is was at #1671.

gaffney commented 9 years ago

Thanks, I searched and did not see that.

I feel this is a pretty common use case for exception handling, not an edge case. Makes me sad I will have to hack around this one.

tomchristie commented 9 years ago

Okay let's leave this open to reconsideration. This sort of use case is more compelling than previously presented.

tomchristie commented 9 years ago

So (exc, request) and (exc, context) both valid options. I guess we might want to lean towards context?

maryokhin commented 9 years ago

To me it would make sense to call context if it would be like in the serializer, request + something else that you can pass yourself, if it's just the request, then request makes sense.

tomchristie commented 9 years ago

The view instance, and URL args, kwargs might also be reasonable things to put in the context.

gaffney commented 9 years ago

Glad this is open for reconsideration. Also wanted to point out that Django's process_exception passes the request, so I would be more inclined to do it that way for the sake of consistency. Not sure how useful additional context would be, but maybe I have not thought of some use cases you have.

kevin-brown commented 9 years ago

Right now in the JSON API renderers, we have to try to detect when an error response is coming through, as JSON API has a custom error format. While we can currently work around only having the exception, having additional context within the exception handler would also be useful.

The view instance, and URL args, kwargs might also be reasonable things to put in the context.

:+1: If we could pass something similar to the serializer context that'd cover most of the bases.

jpadilla commented 9 years ago

Thinking about working on this. I'd stick with context and have it include view, args, kwargs and request. I was actually thinking about just "recycling" get_renderer_context(). Also, thought about just injecting context into exc. That way the signature on existing exception handlers doesn't change.

exc.context = self.get_renderer_context()
response = self.settings.EXCEPTION_HANDLER(exc)

Thoughts?

tomchristie commented 9 years ago

No problem with adding another keyword argument to the signature - it's very easy to handle both cases during the deprecation period.

Agree wrt the context, seems like the best approach.

tomchristie commented 9 years ago

Not sure why I milestoned for 3.0.2 - should be 3.1.0, given the API deprecation.

tomchristie commented 9 years ago

Now resolved in the version-3.1 branch.