Skip to content

Wrap exceptions caused by parsers in ParseError in request wrapper #9453

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 1 commit into from

Conversation

sevdog
Copy link
Contributor

@sevdog sevdog commented Jun 28, 2024

Description

To address #9433 (which is caused by python/cpython#90143) simply wraps AttributeErrors which are raised in the parse process into a ParseError.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

the change seems reasonable. can you fix the lint error please?

@james-mchugh
Copy link
Contributor

Looking at request.py, it looks like there is already a precedent for handling this type of error with the wrap_attributeerror context manager defined at https://github.com/encode/django-rest-framework/blob/master/rest_framework/request.py#L68 and then used at

I've opened another PR (#9455) that utilizes this context manager through a safe_property decorator that is utilized by each of the properties in the Request class.

@sevdog sevdog force-pushed the requests-attribute-errors branch from 3640913 to c375e15 Compare July 8, 2024 08:11
@auvipy
Copy link
Member

auvipy commented Jul 8, 2024

Description

To address #9433 (which is caused by python/cpython#90143) simply wraps AttributeErrors which are raised in the parse process into a ParseError.

can you please check the approach followed here please? #9455

@sevdog
Copy link
Contributor Author

sevdog commented Jul 8, 2024

@auvipy yes, that may work better.

I can also point out that the WrappedAttributeError it should be added every other properties (inside them, as it was pointed out that a custom decorator is slower).

I belive that "wrapping" the response may not be an optimal solution (I got in the past errors when handling with DRF requests in non-DRF code which was expecting an instance of HTTPRequest because there is no inheritance). However this would require a bigger rework and a deeper discussion.

Than I close this PR in favor or #9455.

@sevdog sevdog closed this Jul 8, 2024
@sevdog
Copy link
Contributor Author

sevdog commented Jul 8, 2024

PS: I am curious to see if the same issue would be raised by cached_property, this because many properties on DRF Request looks something which can also be implemented using that descriptor.

@sevdog sevdog deleted the requests-attribute-errors branch July 8, 2024 09:07
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