5677: Respect variable precedence in session

[mateusduboli]

• Move Session#merge_environment_variables from Session#request to Session#send to make it consistent
• On Session#send change variable precedence to (higher precedence first) kwargs -> session args -> environment.

[mateusduboli]

Personally I don't like to reuse kwargs too much, but it reduce the amount of changes

[junqfisica]

I propose the following change at line:

requests/requests/sessions.py

Line 530 in 913880c

proxies = proxies or {}

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

[mateusduboli]

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 pypa/pip#9691 @junqfisica ?

[junqfisica]

@mateusduboli Yes, your commit 57ddecd, also fixes the problem with pypa/pip#9691. :)

[CrazyBoyFeng]

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]

Sorry, my test result is that no_proxy is still valid. There is no problem with your modification.

So why rebuild_proxies() is here before?

[mateusduboli]

Before #5681, the send method was not using the environment variables at all, this was here to ensure that those would be read at some point.

However, this MR also moves the call for merge_environment_settings to send, thus rebuilding proxies is not necessary. Before, it was only issued in the request method.

[mateusduboli]

@CrazyBoyFeng Do we need any additional review on this, so it can be merged?

[CrazyBoyFeng]

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.

[mateusduboli]

@sigmavirus24, can you take a look at these changes?

[tscheburaschka]

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

[NullPointerMaker]

Hope this fix can be confirmed as soon as possible. It makes a lot of software does not work with custom proxy while the system proxy is set, including pip.

In addition, it is recommended that type "fix #5677" in the PR description to associate this PR with the issue.

[junqfisica]

These changes will make the hierarchy request kwargs -> Session args -> environment to be respected. This also solves problems with pip as discussed here

[mateusduboli]

@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?

[CrazyBoyFeng]

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?

[nateprewitt]

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.

[nateprewitt]

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)