loosen URL handling for non-native URL schemes

Requests supports adding other URL schemes by mounting an adapter for them.  For
schemes that have unfamiliar URLs, requests currently prevents the request from ever reaching the adapter, making it impossible to extend requests with sometimes convenient extensions.  This change allows requests to detect this and gives the adapter author the full ability to shoot themselves firmly in the foot.

NOTE:  This is to be compatible with the previous work I did to add a file URL scheme to Requests (which is now available here: https://github.com/jvantuyl/requests-file).  I've since added a data URL scheme implementation (available https://github.com/jvantuyl/requests-data).


FYI, I'm waiting to release requests-file until the latest release of requests is compatible with it.  Hopefully this change isn't too controversial, as I'd love to be able to release it ASAP.


Closed and reopened to get a rebuild.

The code changes here look totally fine to me, so in principle this is acceptable to merge. I'm also in principle happy with this change: it makes using unusual protocols in Transport Adapters not too big a problem.

However, it's a significant enough diversion that I want to hear from @sigmavirus24 and @kennethreitz first. =)


Not that I'm trying to dissuade anyone from accepting my pull request, but I recognize that this is kind of a hack.  Ideally the request preparation should be protocol neutral and there would be a hook or something to wire in any protocol-specific from the adapter (where I feel it belongs).

However, I didn't really think such a convoluted shift was warranted for such a minor change of functionality.


@jvantuyl I actually disagree with that part. Requests is fundamentally a HTTP library. It's nice that Transport Adapters make it possible to use whole different protocols (with `file://` and `ftp://` being the only ones I've seen done), but it's not actually what Requests is intended to do. When we don't need to make it harder we shouldn't, but neither should we make Requests protocol-agnostic.


So I'm personally :-1: on this because the changes here smell. Requests are fundamentally HTTP related and should stay so. All HTTP requests require a host so limiting this to `http` and `https` schemes just introduces one more avenue for obscure failure.


@Lukasa Hmmmmm, I'm not suggesting it needs to be entirely protocol agnostic; merely that, if we're going to purport to have pluggable protocols at all, we should compartmentalize them as much as reasonable.

@sigmavirus24 Please describe the smell.  Am I looking for cheese or rotting meat?  Seriously though, the file RFC specifies that empty hosts equate to localhost.  file:///yada/yada/yada is perhaps the most common form of file URL that there is.  Refusing to accept that as possible is refusing the file protocol in its entirety.

How do you propose to allow this work?  Back in Pull Request #1109, I was told that this was "exactly the type of thing that I was hoping people would make with Connection Adapters" and that "just doesn't belong in Requests itself, but another module" and "it would be awesome".  Are we making this impossible now?  That would not be awesome.

Then again, one man's cheese is another man's mold?  Indulge me for a minute.  Sample my cheese and see if you like the smell after tasting the flavor.  As it turns out, this eliminates something phenomenally useful.  Let me explain what I actually _do_ with this code, as it all comes down to two very useful things.

Firstly, I like to test my code.  Tested code is great.  Testing code with requests is hard, because it's network-active.  Essentially, you have to mock it out; which, in turn, makes your tests inaccurate.  When requests changes, my mocks don't, and my tests don't reflect the changes.

To get good coverage, this forced me to set up a mock web-API and run it on a small web server.  It made my code have environmental dependencies for testing, and building the test environment was a pain.  As I developed this mock server, I realized that 95% of the handlers were just returning a file.

I thought to myself, "Why not just use file:/// into the test directory and work with the files? That makes it self-contained, and only requires a different URL for testing, which I already do!"  That's when I implemented the file:/// URL because, well, it was easy.

Secondly, I love documentation.  The more I started developing my code this way, I started to collect a battery of files.  These files were checked in with my code, contained JSON of the actual requests, and turned out to entirely document the expectations I had for the API I was interacting with.  I could even look in the git history to see when I started using a feature, etc.  It made my code much more accessible to other developers because it was so much "look here, see what it's doing".

So, here are two very real benefits in very real code that are made possible by this rather modest change.  I feel that they heavily outweigh any small smell that they may cause (of course, I'm biased).  This is a five-line change limiting HTTP-specific behavior to HTTP-specific protocols in a system that is architected to support additional protocols through plugins in order to support one of the other most widely-used RFC-specified protocols on the planet.  Isn't it a smell if a minor, overzealous sanity check prevents this from being attainable?

If that's too much, can I just remove the checkl?  Or can I move it into the HTTP adapter so it happens later?  I don't think either of those make it more maintainable, thought they at least make things possible.


@jvantuyl In principle, sure. In practice, Requests only knows about HTTP. Everything about the Transport Adapter is totally about HTTP. If we were moving protocol-specific features to the Transport Adapter, we'd just have `RequestsAdapter`. =D

Don't get me wrong. I'm in favour of supporting `file://` URLs, which is why I'm in favour of this change. I'm just trying to make it clear that I'll otherwise draw a fairly strong line in the sand at "this far but no further". There is a limit to what Requests should be able to do in non-HTTP protocols.


> Please describe the smell.

Lots of branches of logic smell to me, especially repeated branches of logic. That is no confident coding, that is fearful coding. Fear has a distinct smell to it which I vastly dislike.

> How do you propose to allow this work? Back in Pull Request #1109, I was told that this was "exactly the type of thing that I was hoping people would make with Connection Adapters" and that "just doesn't belong in Requests itself, but another module" and "it would be awesome".

The greater context is that we have all been vastly adverse to having the `file://` protocol ever supported. I think very early on it was and it was awful and buggy. We've shot down every request thus far to add it or support it. I don't see a reason why this should be different, but I'll address other issues you raise.

> Firstly, I like to test my code. Tested code is great. Testing code with requests is hard, because it's network-active.

I like to test my code to. I like to test my code so much I wrote a requests-specific tool to test it called Betamax. It's cool, you should check it out. This allows your request to hit the network once, record the details of the request and response and save it for playback in the future. You can match it however you like and even define your own matchers (to match on the request you're making). You can match in a fuzzy way or in a very strict way. There's a lot of freedom in it for as opinionated as it is.

> These files were checked in with my code, contained JSON of the actual requests

I check in the files (saved with JSON) of my interactions that are recorded with Betamax. I call them cassettes. Again, you should check out betamax.

Finally, instead of having to change the URL you're requesting against, Betamax allows you to just keep requesting the same thing. If there's a matching request and response it will use that. If there isn't and you told it not to interact with the internet you'll get a helpful exception. If there isn't and you tell it that it can talk to the internet, it will record it for you!

And on top of everything, it requires no changes to requests. It just works. It's also documented but I am not content with the documentation I've written. In fact, now that I think of it, I'm not sure I documented how to write your own matcher very well so I'm going to go rewrite it.


I guess what I really mean, is that your goal is achievable without these changes which may be minor but which are likely to be removed in future refactors of the code-base. As @Lukasa explained, we're really primarily concerned with HTTP and while other adapters don't run into this issue, there are probably other ways around this. One thing is that the implementation details of the Adapter are completely opaque to requests and should always be. Certainly `file://` is common among browsers and even curl, but why not do something more clever?

For example, you could just as easily have your adapter receive requests on: `http://localhost:64000/` which I'm pretty sure is safe from conflict. Really, it could match on anything the user wants. If they're wrapping an API (like I am), they can add an adapter for the API they're wrapping, e.g.:

``` python
session.mount('https://api.github.com', FileAdapter)
```

This would allow the user to not need to specify `file://` when making their "mocked" requests.

My last comment was not meant to sell you Betamax.

Frankly here are the reasons I'm :-1:
- The code is likely to get lost in some future shuffle. This will break your adapter then and we'll have to rehash this issue.
- There are other ways that don't require this change
- There are other libraries that seem to satisfy your needs and other people's needs
  - Betamax
  - requestions
  - vcrpy
  - and the list goes on
- Historically we as trio have been wont to encourage people to use the `file://` scheme and to support it


Let me see if I can come up with a less offensive patch to achieve the same thing.


Okay, here's a new patch.  This one is significantly less hacky than the previous.  Highlights:
- says exactly what it's doing
- self-contained in a way that is highly unlikely to be shuffled around
- universally handles the core problem, HTTP-like URL handling breaks non-HTTP URLs (and, thus, other protocols adapters)
- has a test to document what's going on
- makes a very tiny fallback change to urllib3 that catches yet-another-bit-of-HTTP-specific URL handling

To see this change in action, check out jvantuyl/requests-file@5c0f287 and now jvantuyl/requests-data@1886c8a (which supports the imminently useful data:... URIs).

Objections?


Hmm, I feel like we're getting into interesting territory here. If I undrestand you correctly, PreparedRequest objects are intended to be used for the more complex things you're trying to do. Could you use a PreparedRequest to do this today, or is this change neccessary (I'd test myself, but I'm on an iPad at the moment :)


noooo, i hit the wrong button


Awwwww, you got my hopes up.  :(


@kennethreitz That's basically what this change does, makes the PreparedRequest a little less paranoid, so I can subsume this type of handling inside the adapter.  Without this change any PreparedRequest bombs out when it tries to handle the non-native URL.


I guess my question is: does the current problem occur when creating a new PreparedRequest? Or when sending it. 


@kennethreitz Creating?  Well, sort of.  When you create a Request, it automatically creates a PreparedResponse and prepares it.  I _could_ create one manually, not run any of the sanity checks, and then use it for this.  Though, that makes something that's otherwise fairly transparent into something that's significantly more complicated.


This change includes a change in urllib3, which we don't make here, we make upstream. Why did you need it?


URL schemes without mandatory //.


That said, I'll send that upstream and see what I get.


Oh, sorry, I meant why did this hit urllib3 at all? urllib3 is _definitely_ a HTTP-only library.


Unmerging, sorry about the accidental merge :)


Too bad I can't re-open this. Anyway, carry on :)


I'll re-open after I see about the urllib3 thing.