classification
Title: httplib client/server status refactor
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: barry, berker.peksag, demian.brecht, ethan.furman, martin.panter, orsenthil, python-dev, r.david.murray, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-06-17 19:02 by demian.brecht, last changed 2015-03-07 10:33 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
refactor_http_status_codes.patch demian.brecht, 2014-06-17 19:02 review
refactor_http_status_codes_1.patch demian.brecht, 2014-06-18 07:07 review
issue21793.patch demian.brecht, 2014-06-20 15:18 review
issue21793_1.patch demian.brecht, 2014-06-20 18:48 review
issue21793_2.patch demian.brecht, 2014-07-21 19:48 review
issue21793_3.patch demian.brecht, 2014-12-05 16:38 review
issue21793_4.patch demian.brecht, 2014-12-05 18:45 review
issue21793_5.patch demian.brecht, 2014-12-12 05:42 review
issue21793_6.patch demian.brecht, 2014-12-13 02:41 review
issue21793_logfix.patch demian.brecht, 2015-02-09 22:01 review
issue21793_logfix_1.patch demian.brecht, 2015-02-10 15:19 review
issue21793_logfix_2.patch demian.brecht, 2015-02-20 16:31 review
issue21793_logfix_3.patch demian.brecht, 2015-02-21 01:40 review
issue21793_logfix_4.patch demian.brecht, 2015-02-21 16:03 review
Messages (32)
msg220860 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-06-17 19:02
This patch is a follow up to an out of scope comment made by R. David Murray in #20898 (http://bugs.python.org/issue20898#msg213771).

In a nutshell, there is some redundancy between http.client and http.server insofar as the definition of http status code, names and descriptions go. The included patch is a stab at cleaning some of this up while remaining backwards compatible and is intended to solicit feedback before finishing work.

TODOs:
* Populate descriptions for status codes
* Documentation
* Tests (?)
msg220890 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-06-17 21:02
Left comments in reitvald about modifying the Enum used -- let me know if you have any questions about that.
msg220924 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-06-18 07:07
Attached new patch with changes from review and a stab at an update to the docs.
msg221097 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-06-20 15:18
New patch attached addressing review comments.
msg221110 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-06-20 18:48
Updated patch with silly missed doc update.
msg223088 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-07-15 05:30
Bump for a follow-up review or merge
msg223603 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-07-21 19:48
Removed draft status code, removed S from VARIANTS_
msg232171 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-05 01:21
Bump (again) for hopeful merge.
msg232176 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-12-05 07:11
I left a couple of comments on Rietveld. Thanks for the patch, Demian.
msg232198 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-05 16:38
Updated patch addressing review comments. Thanks for the review.
msg232509 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-12 05:42
Self review/update: removed two errant breakpoints and updated the what's new section (missed in my previous patch).
msg232588 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-13 02:41
Updated patch addressing further reviews
msg233046 - (view) Author: Roundup Robot (python-dev) Date: 2014-12-23 14:30
New changeset edf669b13482 by Serhiy Storchaka in branch 'default':
Issue #21793: Added http.HTTPStatus enums (i.e. HTTPStatus.OK,
https://hg.python.org/cpython/rev/edf669b13482
msg233047 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-12-23 14:31
Thank you for your contribution Demian.
msg235547 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-08 05:07
Currently the log output includes the new HTTPStatus codes. I don’t care much for the log output, but perhaps this wasn’t part of the plan? Before:

$ python3.4 -m http.server
Serving HTTP on 0.0.0.0 port 8000 ...
127.0.0.1 - - [08/Feb/2015 05:05:28] "GET / HTTP/1.1" 200 -

After:

$ ./python -m http.server
Serving HTTP on 0.0.0.0 port 8000 ...
127.0.0.1 - - [08/Feb/2015 05:05:40] "GET / HTTP/1.1" HTTPStatus.OK -
msg235560 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-02-08 17:40
Without having looked at the code I would guess the fix is as simple as changing a %s to a %d.

Having said that, it would be nice if the name was also in the log -- something like:

  127.0.0.1 - - [08/Feb/2015 05:05:28] "GET / HTTP/1.1" 200 (OK) -
msg235638 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-09 22:01
Thanks for the catch Martin, it definitely wasn't intended. I've added a patch to fix the issue and add the extra description as suggested by Ethan.
msg235651 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-10 00:26
If changing the log format, you might also want to update the comment at the top of Lib/http/server.py:53. It seems the original format was imitating <https://en.wikipedia.org/wiki/Common_Log_Format>, except the timestamp is slightly different.
msg235690 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-10 15:19
I’ve reverted the patch to use the old format. The main reason being that plain ints can still be used in most cases as values for code, in which case logging will be inconsistent with cases using the enum.
msg235712 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-10 21:54
New logfix patch looks good. I would have written format(self) or format(self, 'd') instead of '{:d}'.format(self), but that’s no big deal.
msg235738 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-11 11:47
FYI I opened Issue 23442 for a separate regression to do with the REQUEST_HEADER_FIELDS_TOO_LARGE name
msg236281 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-02-20 13:24
LGTM. I left a comment for Serhiy on Rietveld.
msg236284 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-20 13:49
Does not changing __str__ to decimal representation (and in this case __str__ = int.__str__ may be better) lost a part of the point of converting HTTP status codes to enums?
msg236304 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-20 16:31
The updated patch addresses comments which I’d somehow missed previously, but keeps the log fix to the __str__ implementation of HTTPStatus (using int.__str__ rather than format()).

> Does not changing __str__ to decimal representation (and in this case __str__ = int.__str__ may be better) lost a part of the point of converting HTTP status codes to enums?

I don’t think so. In the case of HTTPStatus in general, I think that the optimal string representation of an element of the enum is the stringified version of the status code. If nothing else, it’s consistent with the other type of status code that can be used (ints).

That does lead me to something that I think is a little odd about IntEnums in general but I’ll ask that question in python-dev rather than here as to not conflate this issue.
msg236340 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-20 22:39
One option might be changing log_request() from

self.log_message('"%s" %s %s',
                 self.requestline, str(code), str(size))

to

self.log_message('"%s" %s %s',
                 self.requestline, format(code), size)

Using str() is redundant with %s, and using format() instead invokes the int base class’s __format__() rather than the enum’s __repr__().
msg236341 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-20 22:50
I would write just:

if isinstance(code, HTTPStatus):
    code = '%d' % code
msg236342 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-20 22:56
> On Feb 20, 2015, at 2:50 PM, Serhiy Storchaka <report@bugs.python.org> wrote:
> 
> if isinstance(code, HTTPStatus):
>    code = '%d' % code

That’s what I’m intending on doing. It’s definitely not as contained as changing the __str__ implementation of HTTPStatus. That said, I understand the reasoning behind not doing so and this path is the next clearest.
msg236347 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-21 01:40
Latest patch should address all comments. It also fixes the same issue in error logging which wasn’t previously accounted for. The test file has also been updated with using HTTPStatus where possible rather than hard coded ints. This is consistent with other areas in the http package.
msg236349 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-21 04:11
I think you will find the error logging was already fine, since it already uses %d:

127.0.0.1 - - [21/Feb/2015 04:02:06] code 404, message File not found
127.0.0.1 - - [21/Feb/2015 04:02:06] "GET /nonexistant HTTP/1.1" HTTPStatus.NOT_FOUND -

The new codes in the tests look okay though.
msg236370 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-21 16:03
Well, I’m not entirely sure how I came to the conclusion that errors were a problem (other than not spending enough time walking through it) and of course you’re right about the coercion handling it just fine. I’ve removed the explicit conversion in the code but have left the tests as they’re not tested elsewhere. Hopefully that should now wrap this up.
msg237431 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-07 09:52
New changeset ad64b6a7c0e2 by Serhiy Storchaka in branch 'default':
Issue #21793: BaseHTTPRequestHandler again logs response code as numeric,
https://hg.python.org/cpython/rev/ad64b6a7c0e2
msg237432 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-07 09:55
Thank you Martin for noticing a logging issue.
History
Date User Action Args
2015-03-07 10:33:57berker.peksagsetstage: commit review -> resolved
2015-03-07 09:55:30serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg237432
2015-03-07 09:52:41python-devsetmessages: + msg237431
2015-02-21 16:03:16demian.brechtsetfiles: + issue21793_logfix_4.patch

messages: + msg236370
2015-02-21 04:11:37martin.pantersetmessages: + msg236349
2015-02-21 01:40:22demian.brechtsetfiles: + issue21793_logfix_3.patch

messages: + msg236347
2015-02-20 22:56:16demian.brechtsetmessages: + msg236342
2015-02-20 22:50:31serhiy.storchakasetmessages: + msg236341
2015-02-20 22:39:19martin.pantersetmessages: + msg236340
2015-02-20 16:31:35demian.brechtsetfiles: + issue21793_logfix_2.patch

messages: + msg236304
2015-02-20 13:49:00serhiy.storchakasetmessages: + msg236284
2015-02-20 13:24:08berker.peksagsetmessages: + msg236281
stage: needs patch -> commit review
2015-02-11 11:47:17martin.pantersetmessages: + msg235738
2015-02-10 21:54:27martin.pantersetmessages: + msg235712
2015-02-10 19:16:26eli.benderskysetnosy: - eli.bendersky
2015-02-10 15:19:07demian.brechtsetfiles: + issue21793_logfix_1.patch

messages: + msg235690
2015-02-10 00:26:51martin.pantersetmessages: + msg235651
2015-02-09 22:01:04demian.brechtsetfiles: + issue21793_logfix.patch

messages: + msg235638
2015-02-08 17:40:39ethan.furmansetmessages: + msg235560
2015-02-08 07:07:20serhiy.storchakasetstatus: closed -> open
resolution: fixed -> (no value)
stage: resolved -> needs patch
2015-02-08 05:07:23martin.pantersetmessages: + msg235547
2014-12-23 14:31:42serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg233047

stage: commit review -> resolved
2014-12-23 14:30:27python-devsetnosy: + python-dev
messages: + msg233046
2014-12-13 02:41:05demian.brechtsetfiles: + issue21793_6.patch

messages: + msg232588
2014-12-12 08:43:25serhiy.storchakasetassignee: serhiy.storchaka

nosy: + serhiy.storchaka
2014-12-12 07:59:53rhettingersetassignee: rhettinger -> (no value)
2014-12-12 05:42:37demian.brechtsetfiles: + issue21793_5.patch

messages: + msg232509
2014-12-05 18:45:46demian.brechtsetfiles: + issue21793_4.patch
2014-12-05 16:38:20demian.brechtsetfiles: + issue21793_3.patch

messages: + msg232198
2014-12-05 07:11:43berker.peksagsetmessages: + msg232176
2014-12-05 02:32:19r.david.murraysetstage: patch review -> commit review
2014-12-05 01:21:00demian.brechtsetmessages: + msg232171
2014-07-22 03:06:37rhettingersetassignee: rhettinger

nosy: + rhettinger
2014-07-21 19:48:46demian.brechtsetfiles: + issue21793_2.patch

messages: + msg223603
2014-07-16 00:43:46martin.pantersetnosy: + martin.panter
2014-07-15 15:59:49berker.peksagsetnosy: + berker.peksag

stage: patch review
2014-07-15 05:30:19demian.brechtsetmessages: + msg223088
2014-06-20 18:48:04demian.brechtsetfiles: + issue21793_1.patch

messages: + msg221110
2014-06-20 15:18:28demian.brechtsetfiles: + issue21793.patch

messages: + msg221097
2014-06-18 07:07:49demian.brechtsetfiles: + refactor_http_status_codes_1.patch

messages: + msg220924
2014-06-17 21:02:33ethan.furmansetmessages: + msg220890
2014-06-17 20:08:41r.david.murraysetnosy: + barry, orsenthil, eli.bendersky, ethan.furman
2014-06-17 19:02:36demian.brechtcreate