Skip to content
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

Merged
merged 1 commit into from Oct 31, 2018

Conversation

@xrmx
Copy link
Contributor

@xrmx xrmx commented May 30, 2018

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:

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]
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented May 30, 2018

This fixes #4157

Copy link
Member

@humitos humitos left a comment

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?

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented May 30, 2018

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.

@xrmx xrmx force-pushed the restapiclientparser branch from 9fcea49 to 80e9d24 Jun 5, 2018
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 5, 2018

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.

@xrmx xrmx force-pushed the restapiclientparser branch from 80e9d24 to fafd51a Jun 5, 2018
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 8, 2018

do you know why this was added and how this serializer helps us?

Nope, i don't have any extra context into this.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 11, 2018

Since we have tests, can we have the needed: tests removed and the read for review back please?

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Jun 14, 2018

@xrmx Removed the label. We no longer have 'PR: ready for review' as a label; someone will get to this soon, though. Thanks.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jul 4, 2018

Friendly ping :)

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jul 18, 2018

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
@xrmx xrmx force-pushed the restapiclientparser branch from fafd51a to 423b42f Aug 14, 2018
ericholscher added a commit that referenced this issue Oct 31, 2018
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
Copy link
Member

@ericholscher ericholscher left a comment

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. 👍

@ericholscher ericholscher merged commit b9dfe95 into readthedocs:master Oct 31, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants