-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Attribute Errors raised in Parser Silently Ignored #9433
Attribute Errors raised in Parser Silently Ignored #9433
Comments
This is the behaviour of The issue is with this method overridden it intercepts and hides every That wrapper was previously implemented using It seems that also this implementation has its own problems since it is going to mask every AttributeError even if not raised by the |
Maybe it is also a problem with python itself, because it is invoking this method when the |
I'm still not sure that this explains why
(edited to clarify since there are technically two |
Found the bug https://bugs.python.org/issue45985 The reason is that |
I do not believe that bug is related. The issue is that a Python property is being used to lazily parse the data, but that parsing can be pretty advanced and |
Just try it! class TestBrokenPerser(SimpleTestCase):
def setUp(self):
self.factory = APIRequestFactory()
def test_post_accessed_in_post_method(self):
django_request = self.factory.post('/', {'foo': 'bar'}, content_type='application/json')
request = Request(django_request, parsers=[BrokenParser()])
with self.assertRaises(AttributeError, msg='no in parse'):
request._parse()
with self.assertRaises(AttributeError, msg='no in data'):
request.data
If you follow the call-stack:
The |
To be fair, it is somewhat related, but it's not clear to me if the Python team even sees that as a bug (opened over 2 years ago with no feedback). It seems to me to be more of just how Python handles attribute accesses. Properties are attributes too, so it makes sense that the The core issue here also isn't a confusing error message, it's that DRF's handling of the error completely suppresses the errors (no error messages at all). I would gladly accept a confusing error message due to the weird interplay between |
The bug with the parser may be resolved just by wrapping any exception raised by parser in a --- a/rest_framework/request.py
+++ b/rest_framework/request.py
@@ -356,7 +356,7 @@ class Request:
try:
parsed = parser.parse(stream, media_type, self.parser_context)
- except Exception:
+ except Exception as exc:
# If we get an exception during parsing, fill in empty data and
# re-raise. Ensures we don't simply repeat the error when
# attempting to render the browsable renderer response, or when
@@ -364,7 +364,9 @@ class Request:
self._data = QueryDict('', encoding=self._request._encoding)
self._files = MultiValueDict()
self._full_data = self._data
- raise
+ if not isinstance(exc, ParseError):
+ raise ParseError(str(exc))
+ raise exc
# Parser classes may return the raw data, or a
# DataAndFiles object. Unpack the result as required. This will not address the bug of Python itself (I also put here the reference to the github issue python/cpython#90143 for better linking). The problem may also be fixed by using |
I mostly agree here. I think the bug can be fixed easily by adding error handling to the property in the @property
def data(self):
if not _hasattr(self, '_full_data'):
try:
self._load_data_and_files()
except AttributeError as exc:
raise RuntimeError(exc) from exc
return self._full_data It could be handled in
I don't necessarily agree that this a bug with Python, as there is no confirmation from the Python team that it is a bug. It hasn't been triaged on for over 2 years. Just because someone opens an issue with a project doesn't mean that there is a confirmed bug.
I think you may be misunderstanding me here. I'm not suggesting that the implementation moves back to using If |
I've been digging into this some more as I have some time over the weekend. First, I'm convinced that python/cpython#90143 is not a bug, as it is documented right here in the Python Docs that an
Second, I experimented with swapping the re-calling of Looking closer at the code in |
Discussed in #9426
Originally posted by james-mchugh June 3, 2024
Hello.
First, thank you for the hard work on DRF. It has been great workin with it.
There seems to be an issue where an
AttributeError
raised in a parser is being swallowed by theRequest
class's__getattr__
method. I've noticed a similar issue reported previously (#4896), but it was closed as the reporter associated with an error from a separate package.I encountered this because I wrote the server and tested using Python 3.11, and another developer attempted to test using 3.10. My custom parser used a function that was not available in Python 3.10 (hashlib.file_digest), which caused an
AttributeError
when hitting certain endpoints. Instead of seeing a 500 response and a traceback in the server logs, we were getting a 200 response with an empty dictionary in the response body. We were scratching our heads for a while, as there was no indication anything was going wrong.Environment
OS: Darwin 23.4.0 Darwin Kernel Version 23.4.0 x86_64
Python: 3.10 + 3.11
DRF: 3.15.1
Django: 5.0.6
Expected Behavior
500 Internal server error response and traceback in server logs.
Actual Behavior
200 response and no errors in server logs.
$ curl --json '{"foo": "bar"}' http://localhost:8000/foos/ {}
Code to Reproduce
Investigation
This appears to be happening because accessing the
data
property lazily parses data. If the parsing raises anAttributeError
, this is raised up to https://github.com/encode/django-rest-framework/blob/master/rest_framework/request.py#L359 whereself._full_data
is set to an empty dictionary before the error is re-raised.This error then raises up and causes the attribute access to fallback to the
Request.__getattr__
method via Python's data model. Here, thedata
attribute is attempted to be retrieved from theWSGIRequest
stored inRequest._request
, and once again, this raises anAttributeError
which is caught by https://github.com/encode/django-rest-framework/blob/master/rest_framework/request.py#L423. From here, the originalRequest.__getattribute__
handling runs again. Now when thedata
getter runs,self._full_data
is already set to an empty dictionary which is then returned.In the end, an empty response is returned and the error is silently ignored.
Noob question, but why does
Request.__getattr__
callself.__getattribute__
when anAttributeError
occurs? Based on the Python Data model linked above,__getattr__
should only run if__getattribute__
fails in the first place, so by the time__getattr__
runs,__getattribute__
already tried and failed.The text was updated successfully, but these errors were encountered: