requests
requests copied to clipboard
5677: Respect variable precedence in session
- Move
Session#merge_environment_variables
fromSession#request
toSession#send
to make it consistent - On
Session#send
change variable precedence to (higher precedence first)kwargs
->session args
->environment
.
I propose the following change at line: https://github.com/psf/requests/blob/913880c45a3a8c3bf6b298e9c38709cd95a9c97c/requests/sessions.py#L530
Replace it by:
proxies = proxies or self.proxies
This change will make this hierarchy kwargs
-> session args
-> environment
to be respected with no need to change anywhere else. Solving problems also with pip as discussed here
I've added a test case, and changed the strategy.
Now we are merging the session and request settings before we ever look into the environment, that way the precedence becomes more explicit.
Can you test this patch with https://github.com/pypa/pip/issues/9691 @junqfisica ?
@mateusduboli Yes, your commit 57ddecdd2af7bf45f85f38cf63cc5d72de529336, also fixes the problem with pypa/pip#9691. :)
proxies = proxies or self.proxies
in fact, the solutions of you two are similar: merging func args
with session.self
first, then with env.
and mateusduboli
does more work: move all merging (not only proxies) to merge_environment_settings()
, adjust the code sequence after merging so that kwargs
will not be overwritten.
@CrazyBoyFeng Do we need any additional review on this, so it can be merged?
Do we need any additional review on this, so it can be merged?
I used this fix with pip
, and it works fine with the --proxy
parameter.
I think it can be merged.
@sigmavirus24, can you take a look at these changes?
@nateprewitt, can you maybe take a look at this PR before the next release? We would really appreciate this bugfix included in the next available version of requests. Thank you.
@nateprewitt thanks for taking a look at this!
I'm wondering why this is a tagged as a Breaking API Change
, I can understand that it should not go through a patch
revision, but it does keeps the API as intended on the docs.
I'm guessing if we not consider it as such it would be less hassle to merge it, could you clarify it better?
This should be considered as a bug, I think.
When there are both environment variables and argument of requests.session
about the proxy, requests
eventually takes the system environment variable. This is not the usual practice.
If this is not considered as a bug, then it needs to be answered: how to use session
with a custom proxy argument when the environment variable http_proxy
or https_proxy
exists?
Hi @mateusduboli, the reason we have this marked as a breaking change is we're doing a pretty fundamental change to the Session API here. The last PR (#5888) that was introduced for this had some non-trivial impact that's made Requests 2.26.0 unusable for a number of our dependents. It arguably shouldn't have been merged to begin with.
I know we've discussed the session precedence issue more than once previously, but I wasn't able to find actual issue numbers. send
and request
are not intended to function identically and trying to accomplish that breaks a few different workflows. send
is intended to do almost nothing to the request and send the PreparedRequest exactly as it was created. This has drifted over the years, so it's no longer entirely true, but this change will make the situation worse.
I think we need to reevaluate how these interfaces actually operate going into a new major version. Until then though, this has been the behavior of send
for 7+ years, and there's a lot of infrastructure that will stop behaving correctly because of this.
If this is not considered as a bug, then it needs to be answered: how to use session with a custom proxy argument when the environment variable http_proxy or https_proxy exists?
@CrazyBoyFeng I'm reading over the pip issue more and while I agree this far from optimal, as I've stated above, it's kind of where we are. We would like to fix this but can't currently.
For the interim, I'm failing to see why pip
cannot handle this the same way they do timeout. Requests 2.x was specifically designed to ignore Session.timeout because the maintainers at the time didn't agree with it being a session level property. To get around this, Pip created its own timeout
attribute and sets it on each request before calling into Requests. The same thing can be done with proxies. This doesn't require a patch or change to the vendored code, and produces the desired outcome.
e.g.
https://github.com/pypa/pip/blob/3ab760aaa17fdc7f00c468a529241164b070b353/src/pip/_internal/network/session.py#L443-L449
Would become:
def request(self, method, url, *args, **kwargs):
# type: (str, str, *Any, **Any) -> Response
# Allow setting a default timeout on a session
kwargs.setdefault("timeout", self.timeout)
+ if self.proxies:
+ kwargs.setdefault("proxies", self.proxies)
# Dispatch the actual request
return super().request(method, url, *args, **kwargs)