VR-4744: Don't follow 302s#798
Conversation
|
|
||
| if response is not initial_response: | ||
| # insert initial response back into history, b/c `resolve_redirects()` removed it | ||
| response.history.insert(0, initial_response) |
There was a problem hiding this comment.
I don't understand why this is necessary or what it is doing exactly
There was a problem hiding this comment.
requests.Response has a history sequence attribute that keeps track of redirects that occurred.
Because of how requests implemented resolve_redirects(), the initial response isn't included in the beginning of that history.
This is only necessary if something downstream needs an accurate response history, which my tests do.
There was a problem hiding this comment.
Oh, interesting. I'd assume history is irrelevant because it's not following redirects. Can you add a comment clarifying this?
There was a problem hiding this comment.
Oh ugh you're right, thanks. I failed to catch this but there actually isn't any history recorded in the final response.
I'd still prefer to have that full history intact though, both for testing and because I think the caller shouldn't lose access to it just because redirects are being done manually.
| if response.status_code == 302: | ||
| if not conn.ignore_conn_err: | ||
| raise RuntimeError( | ||
| "received status 302 from {}," | ||
| " which is not supported by the Client".format(response.url) | ||
| ) | ||
| else: | ||
| return fabricate_200() |
There was a problem hiding this comment.
Why would we fabricate a 200 on a 302? If the user wants to ignore errors, shouldn't we just leave it be and return the original response?
There was a problem hiding this comment.
For context, the way that the Client's ignore_conn_err flag is intended to function is that this util returns a non-error HTTP response with an empty JSON object in its body. Then when the body is loaded into protobuf objects by the caller in the Client, the message's fields will be populated with their default values. So the Client can continue to execute as much as possible even if it's receiving 4XXs, 5XXs, or connection errors.
So for me, the key part here as that empty JSON body, because it enables that default protobuf object behavior in the caller. The original 302 might not have that, but fabricate_200() does.
Alternatively, I could reassign the content of the original 302.
Actually, altering its content is probably a bad idea since it might interfere with how requests handles connections. I think a new object has to be created.
2 participants
Neither
requestsnorurllib3has a filter for specific redirect codes, so the redirect chain has to be manually inspected.