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

httplib client/server status refactor #65992

Closed
demianbrecht mannequin opened this issue Jun 17, 2014 · 32 comments
Closed

httplib client/server status refactor #65992

demianbrecht mannequin opened this issue Jun 17, 2014 · 32 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@demianbrecht
Copy link
Mannequin

demianbrecht mannequin commented Jun 17, 2014

BPO 21793
Nosy @warsaw, @rhettinger, @orsenthil, @bitdancer, @ethanfurman, @berkerpeksag, @vadmium, @serhiy-storchaka, @demianbrecht
Files
  • refactor_http_status_codes.patch
  • refactor_http_status_codes_1.patch
  • issue21793.patch
  • issue21793_1.patch
  • issue21793_2.patch
  • issue21793_3.patch
  • issue21793_4.patch
  • issue21793_5.patch
  • issue21793_6.patch
  • issue21793_logfix.patch
  • issue21793_logfix_1.patch
  • issue21793_logfix_2.patch
  • issue21793_logfix_3.patch
  • issue21793_logfix_4.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2015-03-07.09:55:30.642>
    created_at = <Date 2014-06-17.19:02:36.681>
    labels = ['type-feature', 'library']
    title = 'httplib client/server status refactor'
    updated_at = <Date 2015-03-07.10:33:57.116>
    user = 'https://github.com/demianbrecht'

    bugs.python.org fields:

    activity = <Date 2015-03-07.10:33:57.116>
    actor = 'berker.peksag'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-03-07.09:55:30.642>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2014-06-17.19:02:36.681>
    creator = 'demian.brecht'
    dependencies = []
    files = ['35672', '35680', '35708', '35711', '36012', '37368', '37370', '37420', '37434', '38068', '38084', '38191', '38193', '38197']
    hgrepos = []
    issue_num = 21793
    keywords = ['patch']
    message_count = 32.0
    messages = ['220860', '220890', '220924', '221097', '221110', '223088', '223603', '232171', '232176', '232198', '232509', '232588', '233046', '233047', '235547', '235560', '235638', '235651', '235690', '235712', '235738', '236281', '236284', '236304', '236340', '236341', '236342', '236347', '236349', '236370', '237431', '237432']
    nosy_count = 10.0
    nosy_names = ['barry', 'rhettinger', 'orsenthil', 'r.david.murray', 'ethan.furman', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'demian.brecht']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21793'
    versions = ['Python 3.5']

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jun 17, 2014

    This patch is a follow up to an out of scope comment made by R. David Murray in bpo-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 (?)

    @demianbrecht demianbrecht mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jun 17, 2014
    @ethanfurman
    Copy link
    Member

    ethanfurman commented Jun 17, 2014

    Left comments in reitvald about modifying the Enum used -- let me know if you have any questions about that.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jun 18, 2014

    Attached new patch with changes from review and a stab at an update to the docs.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jun 20, 2014

    New patch attached addressing review comments.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jun 20, 2014

    Updated patch with silly missed doc update.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jul 15, 2014

    Bump for a follow-up review or merge

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Jul 21, 2014

    Removed draft status code, removed S from VARIANTS_

    @rhettinger rhettinger self-assigned this Jul 22, 2014
    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Dec 5, 2014

    Bump (again) for hopeful merge.

    @berkerpeksag
    Copy link
    Member

    berkerpeksag commented Dec 5, 2014

    I left a couple of comments on Rietveld. Thanks for the patch, Demian.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Dec 5, 2014

    Updated patch addressing review comments. Thanks for the review.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Dec 12, 2014

    Self review/update: removed two errant breakpoints and updated the what's new section (missed in my previous patch).

    @rhettinger rhettinger removed their assignment Dec 12, 2014
    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 12, 2014
    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Dec 13, 2014

    Updated patch addressing further reviews

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 23, 2014

    New changeset edf669b13482 by Serhiy Storchaka in branch 'default':
    Issue bpo-21793: Added http.HTTPStatus enums (i.e. HTTPStatus.OK,
    https://hg.python.org/cpython/rev/edf669b13482

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Dec 23, 2014

    Thank you for your contribution Demian.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 8, 2015

    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 -

    @ethanfurman
    Copy link
    Member

    ethanfurman commented Feb 8, 2015

    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) -

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Feb 9, 2015

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 10, 2015

    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.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Feb 10, 2015

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 10, 2015

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 11, 2015

    FYI I opened bpo-23442 for a separate regression to do with the REQUEST_HEADER_FIELDS_TOO_LARGE name

    @berkerpeksag
    Copy link
    Member

    berkerpeksag commented Feb 20, 2015

    LGTM. I left a comment for Serhiy on Rietveld.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Feb 20, 2015

    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?

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Feb 20, 2015

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 20, 2015

    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__().

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Feb 20, 2015

    I would write just:

    if isinstance(code, HTTPStatus):
        code = '%d' % code

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Feb 20, 2015

    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.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Feb 21, 2015

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 21, 2015

    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.

    @demianbrecht
    Copy link
    Mannequin Author

    demianbrecht mannequin commented Feb 21, 2015

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 7, 2015

    New changeset ad64b6a7c0e2 by Serhiy Storchaka in branch 'default':
    Issue bpo-21793: BaseHTTPRequestHandler again logs response code as numeric,
    https://hg.python.org/cpython/rev/ad64b6a7c0e2

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 7, 2015

    Thank you Martin for noticing a logging issue.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants