Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get coverage back to 100% #1726

Merged
merged 3 commits into from Nov 22, 2019
Merged

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Oct 31, 2019

This means we should probably refactor the connection classes, but that would change the public API and I think it should be discussed in another pull request.

@codecov-io
Copy link

codecov-io commented Oct 31, 2019

Codecov Report

Merging #1726 into master will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1726      +/-   ##
=========================================
+ Coverage    99.7%    100%   +0.29%     
=========================================
  Files          22      22              
  Lines        2003    1976      -27     
=========================================
- Hits         1997    1976      -21     
+ Misses          6       0       -6
Impacted Files Coverage Δ
src/urllib3/util/url.py 100% <ø> (+0.51%) ⬆️
src/urllib3/connection.py 100% <ø> (+2.05%) ⬆️
src/urllib3/util/timeout.py 100% <0%> (ø) ⬆️
src/urllib3/response.py 100% <0%> (ø) ⬆️
src/urllib3/contrib/_appengine_environ.py 100% <0%> (+20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 109c70f...011d857. Read the comment docs.

@pquentin pquentin changed the title Improve coverage Get coverage back to 100% Oct 31, 2019
@pquentin pquentin requested a review from sethmlarson Oct 31, 2019
Copy link
Member

@sethmlarson sethmlarson left a comment

Few questions!

src/urllib3/util/url.py Outdated Show resolved Hide resolved
test/with_dummyserver/test_connectionpool.py Outdated Show resolved Hide resolved
@@ -251,40 +251,6 @@ def __init__(
# HTTPS requests to go out as HTTP. (See Issue #356)
self._protocol = "https"

def connect(self):
Copy link
Member

@sethmlarson sethmlarson Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably shouldn't remove this, what happens if a user has an unverified HTTPS connection, is this class used?

Copy link
Member Author

@pquentin pquentin Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not used, unverified connections go through VerifiedHTTPSConnection.

Copy link
Member

@sethmlarson sethmlarson Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, huh. I wonder what the impact of nuking the class and rebinding the name would do to downstream. 🤷‍♂️

Copy link
Member Author

@pquentin pquentin Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anybody is using this class: connect() referenced variables that don't exist, so could not work. An additional wrinkle here is the bottom of the file:

if ssl:
    # Make a copy for testing.
    UnverifiedHTTPSConnection = HTTPSConnection
    HTTPSConnection = VerifiedHTTPSConnection

So one option is to merge VerifiedHTTPSConnection into HTTPSConnection, and add UnverifiedHTTPSConnection = HTTPSConnection at the end of the file.

Copy link
Member Author

@pquentin pquentin Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sethmlarson I opened #1730 to unblock this pull request and talk about our options here, including the upstream impact.

Copy link
Member

@sethmlarson sethmlarson Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zogzog Sorry for causing an issue. I've edited your above comment as I read it as sarcastic which isn't constructive. Hope you agree with my edit.

Copy link
Member Author

@pquentin pquentin Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aurélien, je comprends ta colère, je sais ce que c'est de tomber sur un commit quelque part sur GitHub qui casse ton code. Je te présente d'ailleurs mes excuses. Maintenant que ça c'est dit, on apprécierait des interventions plus courtoises !

Indeed I had to add [...] 6 months ago because someone removed them or something

We would have appreciated a heads-up at this point! This would have avoided the whole issue.

I'd be happy to accept a pull request that restores a working and covered connect().

People should really understand that, even though it is generally considered a bad thing and should be discouraged, there is a need for unverified https connections that don't spit warnings on the console.

Can you use our fingerprinting feature if the certificate does not change too often?

Is silencing the warning an option?

By the way, it looks like the TLS certificate of https://pythonian.fr/ is currently expired.

Copy link
Member Author

@pquentin pquentin Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, why don't you just silence the warning? Isn't this working for you?

import urllib3

urllib3.disable_warnings(urllib3.exceptions.SecurityWarning)
http = urllib3.PoolManager(cert_reqs="CERT_NONE")
r = http.request("GET", "https://pythonian.fr/webfiles/css.css")
print(r.status)

Copy link
Member Author

@pquentin pquentin Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for what it's worth, here are the thoughts of Seth on verifying HTTPS: https://sethmlarson.dev/blog/2019-11-26/designing-for-real-world-https. I happen to fully agree.

Copy link
Member

@sethmlarson sethmlarson Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also this method (using certificate pinning as mentioned above) that doesn't require you to disable security warnings. :)

http = urllib3.PoolManager(
    cert_reqs="CERT_NONE",
    assert_fingerprint="e677022f45ad8ecfc7a66dbb8d57e9c7b01a52c9f880ecb104af6ee67ee9f18a"
)
resp = http.request("GET", "https://pythonian.fr/webfiles/css.css")

Just remember this PoolManager will only work for the one website. Might be worth even using a ConnectionPool instead.

@pquentin
Copy link
Member Author

pquentin commented Nov 3, 2019

This is ready for another round of review!

@pquentin
Copy link
Member Author

pquentin commented Nov 5, 2019

Travis fails because of #1733, I'll rebase this PR when it gets merged, that will fix the issue. AppVeyor fails because my test for #1715 is not fully reliable. I plan to fix this soon too. [edit: fixed in #1734]

@pquentin
Copy link
Member Author

pquentin commented Nov 8, 2019

test_preserve_chunked_on_retry is a test that I recently added and it failed again. It was retried due to our flaky integration, but we got two different exceptions, both unexpected. The test is definitely not reliable on Windows, and I decided to change tactics to test this code path: an explicit Retry-After header. Opened #1743 for this.

pquentin added 3 commits Nov 10, 2019
They always return False in our case, but at least we test that they
exist and don't crash.
Indeed, the only _encode_target only calls it if the url starts with a
slash.
Unverified connections also go through VerifiedHTTPSConnection.
@sethmlarson
Copy link
Member

sethmlarson commented Nov 11, 2019

I'd like to release 1.25.7 before making large changes, will return to it after the release has been made. :)

@pquentin
Copy link
Member Author

pquentin commented Nov 11, 2019

Works for me 👍

@pquentin
Copy link
Member Author

pquentin commented Nov 20, 2019

@sethmlarson Is there anything I can do here? I understand that it's frightening, but I'm only removing code that was already broken anyway.

Copy link
Member

@sethmlarson sethmlarson left a comment

This seems fine, if all downstream test suites are passing I've got a feeling we'll be okay.

@sethmlarson sethmlarson merged commit 063d888 into urllib3:master Nov 22, 2019
2 checks passed
@pquentin pquentin deleted the improve-coverage branch Nov 22, 2019
@pquentin
Copy link
Member Author

pquentin commented Nov 22, 2019

Thanks!

@zogzog zogzog mentioned this pull request Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants