-
Notifications
You must be signed in to change notification settings - Fork 77
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
[SVCS-421] Removing unsizable flag from ResponseStreamReader #273
[SVCS-421] Removing unsizable flag from ResponseStreamReader #273
Conversation
This flag breaks chunked encoding. It appears to no longer be needed either
LGTM 👍 |
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.
Please refer to the comments for two possible solutions.
@@ -140,12 +140,10 @@ def _make_boundary_stream(self): | |||
|
|||
class ResponseStreamReader(BaseStream): | |||
|
|||
def __init__(self, response, size=None, name=None, unsizable=False): |
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.
size=None
and unsizable=False
are used together, should we remove both? And please make sure that no one calls ResponseStreamReader
with the removed kwargs.
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.
Or maybe keep those two but improve conditional logic. However, we still need to make sure that int(size)
does not throw exceptions (or we need to catch them when that happens). Whether size
is of type int
or string
makes a difference.
elif not unsizable and size is not None:
try:
elf._size = int(size)
except:
elf._size = None
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.
So after a little research I've found the unsizable argument is never used, and the size argument is used, but seems to usually be None, so I've decided to change the code to match the suspected intention of original writer. When a size is given the stream will read to that size, if it is None, the whole stream will be read.
def __init__(self, response, size=None, name=None):
super().__init__()
if 'Content-Length' in response.headers:
self._size = int(response.headers['Content-Length'])
else:
self._size = size
self._name = name
self.response = response
PR closed. Work continues in #309. |
refs: https://openscience.atlassian.net/browse/SVCS-421
closes #217
Purpose
The unsizable flag is breaking chunked encoding responses. It is also no longer used and can be removed.
Summary of changes
Removed unsizable flag from ResponseStreamReader
Test Notes/ QA
Git archeology showed that not much had used this flag, or was using it. It seems like googledrive was only ever using it, and that has been changed.
On the Jira ticket, there is a script which tests the difference between the old and new ResponseStreamReader (demonstrating the bug).