Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Convert http.HTTPStatus objects to their int equivalent #7188

Merged
merged 6 commits into from
Apr 3, 2020
Merged

Convert http.HTTPStatus objects to their int equivalent #7188

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d85b9d
Convert http.HTTPStatus objects to their int equivalent
anoadragon453 Mar 31, 2020
61add4a
Add changelog
anoadragon453 Mar 31, 2020
6a0a1de
Don't special-case HTTPStatus. Expand comments
anoadragon453 Apr 3, 2020
4b36aa4
Clean up changelog and remove extra space
anoadragon453 Apr 3, 2020
395feb3
grammar
anoadragon453 Apr 3, 2020
9c43ab0
Remove TODO
anoadragon453 Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/7188.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix consistency of HTTP status codes reported in log lines. Instances of `HTTPStatus.*` are replaced with their integer equivalent.
6 changes: 6 additions & 0 deletions synapse/api/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"""Contains exceptions and error codes."""

import logging
from http import HTTPStatus
from typing import Dict, List

from six import iteritems
Expand Down Expand Up @@ -86,6 +87,11 @@ class CodeMessageException(RuntimeError):

def __init__(self, code, msg):
super(CodeMessageException, self).__init__("%d: %s" % (code, msg))

# Convert http.HTTPStatus objects to their int status code equivalents
Copy link
Member

Choose a reason for hiding this comment

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

ohh gosh, this is gnarly.

so it turns out that HTTPStatus is a subclass of int:

>>> import http
>>> isinstance(http.HTTPStatus.FORBIDDEN, int)
True

... so the docstring is correct. Indeed everything works correctly except for the access logger, where it does str(code). Twisted does b"%d" % code which does the right thing too.

Incidentally, the change of http.client.<code> from int to IntEnum happened in python 3.5.

It feels like there are multiple potential solutions here:

  • go around replacing every HTTPStatus reference with <code>.value (which may be tricky, because there are many different ways of getting hold of an HTTPStatus reference - which is a separate problem; we should be consistent.)
  • do this and special-case HTTPStatus here
  • do code = int(code) here
  • do code = "%d" % (self.code,) in the access logger code

I'm not sure I have strong feelings. I'm slightly uneasy about the last option, which feels like it might be a bit fragile if Twisted ever decides to switch from b"%d" % code to str(code).encode() or something.

Either way I'd suggest expanding the comments to explain that HTTPStatus instances are ints but with magic __str__ methods which mess us up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the detailed comment! It looks like we're leaning towards option 3, bearing in mind to aim for option 1 in the long term.

I'll go ahead and change the PR to reflect that and update the comments.

if isinstance(code, HTTPStatus):
code = code.value

self.code = code
self.msg = msg

Expand Down