From 1d09586599a495e01bfb6b887b1a59419673600a Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 14 Mar 2017 13:36:06 +0000
Subject: [PATCH] Address review comments

- don't blindly proxy all HTTPRequestExceptions
- log unexpected exceptions at error
- avoid `isinstance`
- improve docs on `from_http_response_exception`
---
 synapse/api/errors.py                     | 19 ++++++++++----
 synapse/rest/media/v1/media_repository.py | 30 ++++++++++++-----------
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/synapse/api/errors.py b/synapse/api/errors.py
index d5391a80c..6fbd5d687 100644
--- a/synapse/api/errors.py
+++ b/synapse/api/errors.py
@@ -94,6 +94,17 @@ class SynapseError(CodeMessageException):
     def from_http_response_exception(cls, err):
         """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:
             err (HttpResponseException):
 
@@ -137,13 +148,11 @@ class UnrecognizedRequestError(SynapseError):
 
 class NotFoundError(SynapseError):
     """An error indicating we can't find the thing you asked for"""
-    def __init__(self, *args, **kwargs):
-        if "errcode" not in kwargs:
-            kwargs["errcode"] = Codes.NOT_FOUND
+    def __init__(self, msg="Not found", errcode=Codes.NOT_FOUND):
         super(NotFoundError, self).__init__(
             404,
-            "Not found",
-            **kwargs
+            msg,
+            errcode=errcode
         )
 
 
diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py
index 66464cfe6..c43b185e0 100644
--- a/synapse/rest/media/v1/media_repository.py
+++ b/synapse/rest/media/v1/media_repository.py
@@ -12,7 +12,11 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+
+from twisted.internet import defer, threads
 import twisted.internet.error
+import twisted.web.http
+from twisted.web.resource import Resource
 
 from .upload_resource import UploadResource
 from .download_resource import DownloadResource
@@ -20,9 +24,6 @@ from .thumbnail_resource import ThumbnailResource
 from .identicon_resource import IdenticonResource
 from .preview_url_resource import PreviewUrlResource
 from .filepath import MediaFilePaths
-
-from twisted.web.resource import Resource
-
 from .thumbnailer import Thumbnailer
 
 from synapse.http.matrixfederationclient import MatrixFederationHttpClient
@@ -30,8 +31,6 @@ from synapse.util.stringutils import random_string
 from synapse.api.errors import SynapseError, HttpResponseException, \
     NotFoundError
 
-from twisted.internet import defer, threads
-
 from synapse.util.async import Linearizer
 from synapse.util.stringutils import is_ascii
 from synapse.util.logcontext import preserve_context_over_fn
@@ -174,16 +173,19 @@ class MediaRepository(object):
                 except HttpResponseException as e:
                     logger.warn("HTTP error fetching remote media %s/%s: %s",
                                 server_name, media_id, e.response)
-                    raise SynapseError.from_http_response_exception(e)
+                    if e.code == twisted.web.http.NOT_FOUND:
+                        raise SynapseError.from_http_response_exception(e)
+                    raise SynapseError(502, "Failed to fetch remote media")
 
-                except Exception as e:
-                    logger.warn("Failed to fetch remote media %s/%s",
-                                server_name, media_id,
-                                exc_info=True)
-                    if isinstance(e, SynapseError):
-                        raise e
-                    else:
-                        raise SynapseError(502, "Failed to fetch remote media")
+                except SynapseError:
+                    logger.exception("Failed to fetch remote media %s/%s",
+                                     server_name, media_id)
+                    raise
+
+                except Exception:
+                    logger.exception("Failed to fetch remote media %s/%s",
+                                     server_name, media_id)
+                    raise SynapseError(502, "Failed to fetch remote media")
 
             media_type = headers["Content-Type"][0]
             time_now_ms = self.clock.time_msec()