Address review comments

- don't blindly proxy all HTTPRequestExceptions
- log unexpected exceptions at error
- avoid `isinstance`
- improve docs on `from_http_response_exception`
This commit is contained in:
Richard van der Hoff 2017-03-14 13:36:06 +00:00
parent 7f237800e9
commit 1d09586599
2 changed files with 30 additions and 19 deletions

View File

@ -94,6 +94,17 @@ class SynapseError(CodeMessageException):
def from_http_response_exception(cls, err): def from_http_response_exception(cls, err):
"""Make a SynapseError based on an HTTPResponseException """Make a SynapseError based on an HTTPResponseException
This is useful when a proxied request has failed, and we need to
decide how to map the failure onto a matrix error to send back to the
client.
An attempt is made to parse the body of the http response as a matrix
error. If that succeeds, the errcode and error message from the body
are used as the errcode and error message in the new synapse error.
Otherwise, the errcode is set to M_UNKNOWN, and the error message is
set to the reason code from the HTTP response.
Args: Args:
err (HttpResponseException): err (HttpResponseException):
@ -137,13 +148,11 @@ class UnrecognizedRequestError(SynapseError):
class NotFoundError(SynapseError): class NotFoundError(SynapseError):
"""An error indicating we can't find the thing you asked for""" """An error indicating we can't find the thing you asked for"""
def __init__(self, *args, **kwargs): def __init__(self, msg="Not found", errcode=Codes.NOT_FOUND):
if "errcode" not in kwargs:
kwargs["errcode"] = Codes.NOT_FOUND
super(NotFoundError, self).__init__( super(NotFoundError, self).__init__(
404, 404,
"Not found", msg,
**kwargs errcode=errcode
) )

View File

@ -12,7 +12,11 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.
from twisted.internet import defer, threads
import twisted.internet.error import twisted.internet.error
import twisted.web.http
from twisted.web.resource import Resource
from .upload_resource import UploadResource from .upload_resource import UploadResource
from .download_resource import DownloadResource from .download_resource import DownloadResource
@ -20,9 +24,6 @@ from .thumbnail_resource import ThumbnailResource
from .identicon_resource import IdenticonResource from .identicon_resource import IdenticonResource
from .preview_url_resource import PreviewUrlResource from .preview_url_resource import PreviewUrlResource
from .filepath import MediaFilePaths from .filepath import MediaFilePaths
from twisted.web.resource import Resource
from .thumbnailer import Thumbnailer from .thumbnailer import Thumbnailer
from synapse.http.matrixfederationclient import MatrixFederationHttpClient from synapse.http.matrixfederationclient import MatrixFederationHttpClient
@ -30,8 +31,6 @@ from synapse.util.stringutils import random_string
from synapse.api.errors import SynapseError, HttpResponseException, \ from synapse.api.errors import SynapseError, HttpResponseException, \
NotFoundError NotFoundError
from twisted.internet import defer, threads
from synapse.util.async import Linearizer from synapse.util.async import Linearizer
from synapse.util.stringutils import is_ascii from synapse.util.stringutils import is_ascii
from synapse.util.logcontext import preserve_context_over_fn from synapse.util.logcontext import preserve_context_over_fn
@ -174,15 +173,18 @@ class MediaRepository(object):
except HttpResponseException as e: except HttpResponseException as e:
logger.warn("HTTP error fetching remote media %s/%s: %s", logger.warn("HTTP error fetching remote media %s/%s: %s",
server_name, media_id, e.response) server_name, media_id, e.response)
if e.code == twisted.web.http.NOT_FOUND:
raise SynapseError.from_http_response_exception(e) raise SynapseError.from_http_response_exception(e)
raise SynapseError(502, "Failed to fetch remote media")
except Exception as e: except SynapseError:
logger.warn("Failed to fetch remote media %s/%s", logger.exception("Failed to fetch remote media %s/%s",
server_name, media_id, server_name, media_id)
exc_info=True) raise
if isinstance(e, SynapseError):
raise e except Exception:
else: logger.exception("Failed to fetch remote media %s/%s",
server_name, media_id)
raise SynapseError(502, "Failed to fetch remote media") raise SynapseError(502, "Failed to fetch remote media")
media_type = headers["Content-Type"][0] media_type = headers["Content-Type"][0]