Fix Issue of Ignoring Session-level Settings

[AndTheDaysGoBy]

A re-write of #4935
Addresses: #4938
and perhaps others.

In short, there are three types of values: the instance values (e.g. those passed in via the get(...), request(...), etc.), the session values (e.g. those set for the session such as s.verify = "/some/cert"), and system or environment level values (e.g. the environment variable for the CA_BUNDLE that, at times, becomes the value for the verify flag).

Working with @sorech02, we determined that the best course of action was to just move the session level merging before the environment merging. I.e., the current flow is as follows: one of the simple request calls is made, e.g. request, get, delete, etc., then, the merge_environment_settings method is called wherein environment values are merged in (e.g. the CA_BUNDLE value) and then the session level settings are merged in via the merge_settings calls in merge_environment_settings.

This is too late because, by default, trust_env is set to True, which will result in the verify value passed into the session merging to not be none. One solution is to make sure you set trust_env to False before using the verify flag with a custom certificate, but we deemed this to only be a work around.

The true fix, as described above, is to ensure that the settings which are constructed give the proper precedence to the values given. I.e. passed > session > environment.
We were forced to leave the setdefaults (which effectively reapply the session values if there are no values set) in the def send alone because, at times, one might use send itself when constructing prepared requests (i.e. not via request, get, etc.). In which case, the function must still ensure to use the session level settings.

[AndTheDaysGoBy]

Sadly, running on my machine, the unit tests pass. I would hope someone else could attempt to run the tests of this branch and hopefully replicate the results Travis has.

Edit: managed to recreate on a different machine. Although, oddly enough, that has 2 failing instead of one.

[AndTheDaysGoBy]

The Travis CI passes for everything but Python 2.7 (due to Python 3.X exclusive super() shorthand). Consequently, coverage isn't started. However, all other builds pass.

[AndTheDaysGoBy]

@nateprewitt Pardon my asking, but when you get a chance, could you review this change? It impacts things like home-assistant, docker-py, etc. I would consider this fixing a regression considering it worked 7 years ago, before the existence of merge_environment_settings.

[AndTheDaysGoBy]

Anyone willing to review? Considering the nature of the issue, I'd consider it something that affects anything with custom certificates.

[s00500]

Is there anyone still tracking this issue ?

[sigmavirus24]

I believe this was explicitly scoped for the next major release and so no one is tracking this because that's not remotely on the horizon

[AndTheDaysGoBy]

tl;dr it's never happening. Before my issue was made, there had been another issue for the same thing. I forget if there was a PR though. After my issue, a duplicate issue was made of this one. In short, this issue has been reported several times and never fixed since it's a "major" change. The "major" change would have to occur in the next major release, i.e. 3.0.0, which is not happening any time in the foreseeable future.

You'd have better luck seeing if httpx or some requests derivative would be willing to fix this, if they haven't already.

[nateprewitt]

@AndTheDaysGoBy Yeah, this was already merged into the 3.0 proposal branch in #2839 in 2016. The project has diverged pretty significantly from that branch at this point, and we no longer have resources working to release a Requests 3.0.

I think we're unfortunately at deadlock on getting a fix released. We can't merge #2839 into 2.x releases since it's breaking.

[nateprewitt]

Didn't mean to resolve, but I don't know if this PR is actionable at this point.

[sigmavirus24]

The sarcasm dripping from quoting the word "major" is ridiculously condescending. Is this a bug that's not fun and hits a fair number of people? Absolutely. Does that mean we should just break anyone unknowingly relying on that behaviour? No. Can I keep asking myself easy questions? Absolutely, but I'll stop.

Drive-by fixes and complaints do nothing but detract time and energy from the already ridiculously small amount of time anyone has to work on this project.

[AndTheDaysGoBy]

@sigmavirus24 that's mere projection. I meant major in quotes so as to distinguish from minor since "minor" changes could be merged into the existing requests due to backwards compatibility. My post was to state: the issue has persisted for a while (I believe 2014 to present), is a "major" change (not minor, thus it has to be in the 3.0.0 version), the 3.0.0 version seems to witness repeated delay in release, and recommending an example library, that is itself a derivative for requests, for getting a package with this fix out on PyPi.

I would've used italics had I wished to use a condescending tone.

@nateprewitt thank you for the clarification. Based off of your statements, you're agreeing with me. It would be nice to have somewhere though a reference to a library that does have this fix (I gave httpx as an example of a requests derivative, not as one that has this fix).