Support requests.response.raw being a file-like object #1094
Conversation
IlyaSukhanov
force-pushed
the
transport_plugin_sans_urlib3
branch
from
1428841
to
d0e702c
Compare
IlyaSukhanov
force-pushed
the
transport_plugin_sans_urlib3
branch
from
d0e702c
to
8c67a17
Compare
IlyaSukhanov
force-pushed
the
transport_plugin_sans_urlib3
branch
2 times, most recently
from
db9953e
to
2c2cf0d
Compare
|
Thank you for review and feedback. Latest changes:
|
Codecov Report
@@ Coverage Diff @@
## master #1094 +/- ##
==========================================
+ Coverage 95.86% 95.98% +0.11%
==========================================
Files 62 64 +2
Lines 4040 4107 +67
==========================================
+ Hits 3873 3942 +69
+ Misses 167 165 -2
Continue to review full report at Codecov.
|
Previously httpie relied on requests.models.Response.raw being urllib3.HTTPResponse. The Requests documentation specifies that (requests.models.Response.raw)[https://docs.python-requests.org/en/master/api/#requests.Response.raw] is a File-like object but allows for other types for internal use. This change introduces graceful handling for scenarios when requests.models.Response.raw is not urllib3.HTTPResponse. In such a scenario httpie now falls back to extracting metadata from requests.models.Response directly instead of direct access from protected protected members such as response.raw._original_response. A side effect in this fallback procedure is that we can no longer determine HTTP protocol version and report it as `??`. This change is necessary to make it possible to implement TransportPlugins without having to also needing to emulate internal behavior of urlib3 and http.client.
…response.msg._headers response.cookies was not utilized as it not possible to construct original payload from http.cookiejar.Cookie. Data is stored in lossy format. For example Cookie.secure defaults to False so we cannot distinguish if Cookie.secure was set to False or was not set at all. Same problem applies to other fields also.
IlyaSukhanov
force-pushed
the
transport_plugin_sans_urlib3
branch
from
2c2cf0d
to
8600912
Compare
|
| ( | ||
| 'hello=world; Path=/; Expires=Thu, 01-Jan-1970 00:00:00 GMT; HttpOnly, ' | ||
| 'pea=pod; Path=/ab; Expires=Thu, 01-Jan-1970 00:00:00 GMT; HttpOnly' | ||
| ), |
There was a problem hiding this comment.
Cookies were stored as a list in response.raw._original_response.msg._headers however with this change cookies are extracted from response.headers.get('Set-Cookie', '') which is a comma separated string.
get_expired_cookies has been changed from accepting list-of-tuple headers to string of comma separated strings.
I don't see of a way to access more-or-less raw cookie data from requests in any way but parsing them out of the comma separated string without resorting to using protected members. Cookie jar has more formatted cookies but they are stored in a lossy way.
With all this in mind, to keep tests as lists the cookie splitting logic can be moved to outside of get_expired_cookies(), but I'm not sure if this is what you had in mind by your comment.
Thanks
--IAS
There was a problem hiding this comment.
@IlyaSukhanov Is it an option to add a test case to ensure response.headers.get('Set-Cookie', '') returns a string with concatenated cookies? It will help us prevent potential regressions for upcoming requests versions and to be more comfortable with those changes too :)
After that, we will be good to merge.
There was a problem hiding this comment.
Added test to ensure that multiple cookies get extracted correctly.
There was a problem hiding this comment.
Looks like cicd checks is failing. Is there a tried approach for starting a local http server to test against?
The way it's erroring out it's not clear what the issue is. Few of suspicions I have:
- Multiprocess does not start for some reason
- Multiprocess does not start fast enough (single threaded machine?)
- Flask is unable to open port for some reason (seems unlikely as no error to that effect is presented)
Without knowing what the error is or much experience with GitHub check infrastructure. My only idea is to add a retry functionality in test, to call httpie multiple time until hopefully the background server becomes responsive.
There was a problem hiding this comment.
I was referring to the incorrect usage of tuples for what by nature is a list.
( # <= tuple because fixed-length collection where each item is a different thing
( # <= list because it’s a simple list of things
'hello=world; Path=/; Expires=Thu, 01-Jan-1970 00:00:00 GMT; HttpOnly, '
'pea=pod; Path=/ab; Expires=Thu, 01-Jan-1970 00:00:00 GMT; HttpOnly'
),
None,
[ # <= list because it’s a simple list of things
{'name': 'hello', 'path': '/'},
{'name': 'pea', 'path': '/ab'}
]
),There was a problem hiding this comment.
I think I see what you mean. But the cookie values are not tuples its a string that is line wrapped, and thus grouped using parenthesis.
Tuple:
(
'foo',
'bar'
)
String:
(
'foo'
'bar'
)
Note how in this example and in the pr the lines are not separated by comma which is required in tuple definition.
I hope this clears up any confusion.
If project has different style requirement for line wrapping long strings, please let me know and I'll make this change conform to that.
There was a problem hiding this comment.
@IlyaSukhanov right, I see it now! Yeah, this style of string-wrapping is the preferred one, but in this context, next to a bunch of tuples, it’s easy to misread it (🙋♂️), so I’d vote for using a different style here.
Co-authored-by: Jakub Roztocil <jakub@roztocil.co>
|
If the objective is to test internal of requests to ensure that it returns cookies in right format (and does not regress in future), the viable options I can think of are:
Its also possible to mock |
|
Maybe something "lighter" like done in https://github.com/getsentry/responses/blob/bdf4c3b1273025338c6a475a556d741552ffa819/responses/test_responses.py#L845 could be interesting? I would love to see something using "raw" objects (from I do not have a strong opinion as soon as tests are green et possibly not too random. |
|
@IlyaSukhanov I'll handle the cookies test, it is a bonus and you've done so much already 💪 |
|
OK it should be good now. Do you mind having a final look at the test @IlyaSukhanov? |
|
Elegant! An observation, and not a problem: The thread is not actively terminated. I'm not sure if pytest kills threads of between each test. Even if it doesn't I cannot think of a scenario where current implementation would become problematic, if more tests of this sort were added each test server would be isolated to its own thread/port. And obviously pytest finishes execution so it must be terminating the thread at least at the completion of all tests. 👍 🎆 |
|
Yes, about the zombie thread, it is not an issue right now. And I failed to find a multi-platform solution. It could be improved if we ever need more tests like that. |
|
That's awesome, thanks a lot for your work and patience @IlyaSukhanov 🥧 🍾 |
Previously httpie relied on requests.models.Response.raw being
urllib3.HTTPResponse. The Requests documentation specifies that
requests.models.Response.raw
is a File-like object but allows for other types for internal use.
This change introduces graceful handling for scenarios when
requests.models.Response.raw is not urllib3.HTTPResponse. In such a scenario
httpie now falls back to extracting metadata from requests.models.Response
directly instead of direct access from protected protected members such as
response.raw._original_response. A side effect in this fallback procedure is
that we can no longer determine HTTP protocol version and report it as
??.This change is necessary to make it possible to implement TransportPlugins
without having to also needing to emulate internal behavior of urlib3 and
http.client.
This pull requests is an attempt to address #1093