-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
restapi/client: don't use DRF parser for parsing #4160
Conversation
This fixes #4157 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution.
I didn't find any errors on production regarding this (production runs 2.7) and either in my local instance (I run 3.6). Do you think this is only happening in 3.5?
I'm trying to understand why this custom serialized was added but I'm not sure yet. I found that it was added in this PR: #1563 in this commit: bbc5e52.
Since you remove the loads
method and all the test keep passing, I'd say that we want to have this covered with a specific test case for this. Is it possible to write a test case for this?
Also, I'm wondering if we really need this custom serializer at all.
@agjohnson do you know why this was added and how this serializer helps us?
No idea because I'm seeing this and you don't. BTW I'm not removing the loads method, i'm just inheriting it from the default serializer class. Looks like the client is mocked in the tests so it's not tested at all. |
Rebased and added tests for DrfJsonSerializer. Regarding the usefulness of this custom serializer, the DRF code does something more than the plain json.dumps like always escaping some chars and given this was not tested better not change behaviour imho. |
Nope, i don't have any extra context into this. |
Since we have tests, can we have the needed: tests removed and the read for review back please? |
@xrmx Removed the label. We no longer have 'PR: ready for review' as a label; someone will get to this soon, though. Thanks. |
Friendly ping :) |
Ping, could someone please review this? I don't know how you are not seeing this but there's good chances the current code makes little sense :) |
The DRF parser expect a file-like object containing bytes while we are passing it a unicode string. The outcome of this is slumber serialization not working and returning plain bytes instead of unserialized data. The fix is trivial and it's just using slumber code for parsing instead of providing our own. For reference after making slumber dump exceptions instead of swallowing here's the root cause: Traceback (most recent call last): [celery.redirected:235] File "/venv/lib/python3.5/site-packages/slumber/__init__.py", line 134, in _try_to_serialize_response return stype.loads(resp.content.decode(encoding)) [celery.redirected:235] File "/home/readthedocs/restapi/client.py", line 28, in loads return JSONParser().parse(data) [celery.redirected:235] File "/venv/lib/python3.5/site-packages/rest_framework/parsers.py", line 66, in parse return json.load(decoded_stream) [celery.redirected:235] File "/usr/lib/python3.5/json/__init__.py", line 265, in load return loads(fp.read(), [celery.redirected:235] File "/usr/lib/python3.5/codecs.py", line 493, in read newdata = self.stream.read() [celery.redirected:235] AttributeError: 'str' object has no attribute 'read' [celery.redirected:235] Fix readthedocs#4157
fafd51a
to
423b42f
Compare
This was added a while back and we aren't sure why (#4160 (comment)) Removing it still works locally, and tests still pass. Closes #4160 #4157
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this locally and it works fine. Since we don't know why this was added, I'm going to merge it and mark it for QA on the next deploy. 👍
The DRF parser expect a file-like object containing bytes while
we are passing it a unicode string. The outcome of this is
slumber serialization not working and returning plain bytes
instead of deserialized data.
The fix is trivial and it's just using slumber code for
parsing instead of providing our own.
For reference after making slumber dump exceptions instead of
swallowing them here's the root cause: