classification
Title: httplib is using a method that doesn't exist
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: orsenthil Nosy List: BreamoreBoy, benjamin.peterson, berker.peksag, brett.cannon, demian.brecht, guohua, nikratio, orsenthil, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-01-22 11:50 by guohua, last changed 2015-01-25 03:27 by orsenthil. This issue is now closed.

Files
File name Uploaded Description Edit
23000.patch orsenthil, 2015-01-23 09:13
23000-v2.patch orsenthil, 2015-01-23 18:08
23300-3.3.patch orsenthil, 2015-01-25 00:43
23300-3.3-v2.patch orsenthil, 2015-01-25 01:38
Messages (23)
msg234488 - (view) Author: Guohua Ouyang (guohua) Date: 2015-01-22 11:50
Following the issue 7776, there is a patch for 2.7 version.

Which changes the method of class HTTPConnection from "_set_hostport" to "_get_hostport"[1], it seems introduce in some incompatibility issues.

On my system, the file "/usr/lib64/python2.7/site-packages/mercurial/url.py" from package mercurial-3.0-2.fc21 still use the old method "_set_hostport". I met an error "AttributeError: httpsconnection instance has no attribute '_set_hostport'" when use this package. I only see this incompatibility issue so far.

And in the file httplib.py itself, [2] still use "self._conn._set_hostport(host, port)", which should be "self._conn._get_hostport(host, port)" if it's settled to rename "_set_hostport" to "_get_hostport".

[1] https://github.com/python/cpython/blob/2.7/Lib/httplib.py#L743
[2] https://github.com/python/cpython/blob/2.7/Lib/httplib.py#L1132
msg234494 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-01-22 14:38
That leading underscore in the method name means it is not a public API and thus changes are allowed without any backwards-compatibility guarantees. Mercurial will need to update their code to handle this if they want to continue to use the method.
msg234499 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-01-22 16:52
This is a bug first reported here https://mail.python.org/pipermail/python-list/2015-January/697228.html.  The problem is that in #7776 r90728 568041fd8090 _set_hostport was renamed to _get_hostport but there is still a call to the former at line 1132 in httplib.py.
msg234504 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-01-22 17:45
There's definitely a bug because httplib is now using a method that doesn't exist.
msg234505 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-22 18:14
Ouch. Will fix this today. Strange, I think, no coverage exists for that and it has escaped our testing.
msg234508 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-01-22 18:41
@Senthil: If you're fixing this today, can you also correct the typo here: https://hg.python.org/cpython/rev/568041fd8090/#l1.27 (s/HTML/HTTP)? Just caught my eye going through the review that Mark linked.
msg234509 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-22 18:43
Demian, sure, will do.
msg234532 - (view) Author: Nikolaus Rath (nikratio) * Date: 2015-01-23 01:21
Serhiy, did you add me to Cc just for information, or is there anything I should be doing (having written the patch that introduced this bug)?
msg234536 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-23 01:47
Nikolaus, nothing might be required from your end. Just a good protocol to keep the interested parties in CC.
msg234546 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-23 09:13
Here is the patch to fix this. I have added a test case covering this change. Please review this and if it is good to go, I will commit it. Thank you.
msg234549 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-01-23 09:36
+class TestServer(TestCase):

This can be a mixin.

+    def testHTTPWithConnectHostPost(self):

Post -> Port?

+        self.conn = httplib.HTTP(host=constructor_host, port=constructor_port)

or to make the test simpler,

    self.conn = httplib.HTTP()
msg234559 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-01-23 15:15
Other than Berker's comments, LGTM.
msg234568 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-23 18:08
Addressed Berker's review comments.
1) Made the TestServer a Mixin. (Thanks, that's the correct to do).
2) Changed Post to Port.
3) I went with still using a testdomain and port in the constructor. My idea of the test is to demonstrate that the connect(host,port) is used when given rather than HTTP(host,port).
msg234569 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-01-23 18:31
LGTM.
msg234636 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-01-24 20:59
New changeset c2bba3c9424b by Senthil Kumaran in branch '2.7':
Fix Issue23300 : httplib.HTTP classe's connect method should use _get_hostport
https://hg.python.org/cpython/rev/c2bba3c9424b
msg234637 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-24 21:00
Fixed in 2.7. Other versions do not have this bug.
msg234638 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-01-24 21:01
On Sat, Jan 24, 2015, at 16:00, Senthil Kumaran wrote:
> 
> Senthil Kumaran added the comment:
> 
> Fixed in 2.7. Other versions do not have this bug.

Thank you! Would porting the tests be useful, though?
msg234639 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-24 21:05
> Would porting the tests be useful, though?

Yes, it will be useful to port the tests to 3.4 and default branch.
I will do that.
msg234642 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-25 00:43
Actually, the same tests cannot be ported to 3.4 or default. httplib.HTTP class has been merged to HTTPConnection in 3.x and we don't allow a host, and port in connect().

There is good test coverage for verifying host, port in test_hort_port test in 3.x

However, I saw that TunnelTests could see some refactor and a test addition to cover the _tunnel_host, _tunnel_port attributes (which are set using _get_hostport method). 

Attaching a change to client.test_httplib which refactors TunnelTest and adds a new test called test_set_tunnel_host_port_headers. Please review if this change is Okay. I can push this in 3.4 and default branches.

Thanks!
msg234645 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-01-25 01:34
+        self.assertDictEqual(self.conn._tunnel_headers, tunnel_headers)

You could just use assertEqual. Quoting from docs:

    "The list of type-specific methods automatically used by assertEqual() are summarized in the following table. Note that it’s usually not necessary to invoke these methods directly."

https://docs.python.org/3/library/unittest.html#unittest.TestCase.addTypeEqualityFunc

+        self.assertTrue(b'CONNECT destination.com' in self.conn.sock.data)
+        self.assertTrue(b'Host: destination.com' in self.conn.sock.data)

You could use assertIn instead.
msg234646 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-25 01:38
Thanks for the review comments. Updated the patch with the suggested changes.
msg234649 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-01-25 02:54
LGTM.
msg234650 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2015-01-25 03:27
Committed the test changes in  fcab9c106f2f (3.4) and  a858cde113f2 (3.5)
History
Date User Action Args
2015-01-25 03:27:37orsenthilsetstatus: open -> closed

messages: + msg234650
2015-01-25 02:54:17berker.peksagsetmessages: + msg234649
2015-01-25 01:38:03orsenthilsetfiles: + 23300-3.3-v2.patch

messages: + msg234646
2015-01-25 01:34:22berker.peksagsetmessages: + msg234645
2015-01-25 00:43:07orsenthilsetfiles: + 23300-3.3.patch

messages: + msg234642
2015-01-24 21:05:01orsenthilsetstatus: closed -> open

messages: + msg234639
2015-01-24 21:01:47benjamin.petersonsetmessages: + msg234638
2015-01-24 21:00:31orsenthilsetstatus: open -> closed
resolution: fixed
messages: + msg234637

stage: commit review -> resolved
2015-01-24 20:59:05python-devsetnosy: + python-dev
messages: + msg234636
2015-01-23 18:31:06berker.peksagsetmessages: + msg234569
stage: patch review -> commit review
2015-01-23 18:08:57orsenthilsetfiles: + 23000-v2.patch

messages: + msg234568
2015-01-23 15:15:14demian.brechtsetmessages: + msg234559
2015-01-23 09:36:07berker.peksagsetnosy: + berker.peksag
messages: + msg234549
2015-01-23 09:13:16orsenthilsetfiles: + 23000.patch
keywords: + patch
messages: + msg234546

stage: needs patch -> patch review
2015-01-23 01:47:45orsenthilsetmessages: + msg234536
2015-01-23 01:21:54nikratiosetmessages: + msg234532
2015-01-22 18:43:42orsenthilsetmessages: + msg234509
2015-01-22 18:41:44demian.brechtsetnosy: + demian.brecht
messages: + msg234508
2015-01-22 18:14:36orsenthilsetassignee: orsenthil
messages: + msg234505
2015-01-22 18:09:25serhiy.storchakasetnosy: + nikratio, serhiy.storchaka

type: enhancement -> behavior
stage: needs patch
title: An improper change in httplib.py -> httplib is using a method that doesn't exist
2015-01-22 17:45:42benjamin.petersonsetstatus: closed -> open

nosy: + benjamin.peterson, orsenthil
messages: + msg234504

resolution: wont fix -> (no value)
2015-01-22 16:52:56BreamoreBoysetnosy: + BreamoreBoy
messages: + msg234499
2015-01-22 14:38:23brett.cannonsetstatus: open -> closed

nosy: + brett.cannon
messages: + msg234494

resolution: wont fix
2015-01-22 11:50:08guohuacreate