Merge pull request #229 from matrix-org/rav/fix_upload_error_parsing

Fix error handling on uploadContent

lib/http-api.js

@@ -495,6 +495,29 @@ module.exports.MatrixHttpApi.prototype = {
         return this.opts.baseUrl + prefix + path + queryString;
     },
 
+    /**
+     * @private
+     *
+     * @param {function} callback
+     * @param {string} method
+     * @param {string} uri
+     * @param {object} queryParams
+     * @param {object|string} data
+     * @param {object=} opts
+     *
+     * @param {boolean=true} opts.json Json-encode data before sending, and
+     *   decode response on receipt. (We will still json-decode error
+     *   responses, even if this is false.)
+     *
+     * @param {object=} opts.headers  extra request headers
+     *
+     * @param {number=} opts.localTimeoutMs client-side timeout for the
+     *    request. No timeout if undefined.
+     *
+     * @return {module:client.Promise} a promise which resolves to either the
+     * response object (if this.opts.onlyData is truthy), or the parsed
+     * body. Rejects
+     */
     _request: function(callback, method, uri, queryParams, data, opts) {
         if (callback !== undefined && !utils.isFunction(callback)) {
             throw Error(
@@ -510,6 +533,9 @@ module.exports.MatrixHttpApi.prototype = {
                 queryParams[key] = this.opts.extraParams[key];
             }
         }
+
+        var json = opts.json === undefined ? true : opts.json;
+
         var defer = q.defer();
 
         var timeoutId;
@@ -540,7 +566,7 @@ module.exports.MatrixHttpApi.prototype = {
                     withCredentials: false,
                     qs: queryParams,
                     body: data,
-                    json: opts.json === undefined ? true : opts.json,
+                    json: json,
                     timeout: localTimeoutMs,
                     headers: opts.headers || {},
                     _matrix_opts: this.opts
@@ -552,7 +578,14 @@ module.exports.MatrixHttpApi.prototype = {
                             return; // already rejected promise
                         }
                     }
-                    var handlerFn = requestCallback(defer, callback, self.opts.onlyData);
+
+                    // if json is falsy, we won't parse any error response, so need
+                    // to do so before turning it into a MatrixError
+                    var parseErrorJson = !json;
+                    var handlerFn = requestCallback(
+                        defer, callback, self.opts.onlyData,
+                        parseErrorJson
+                    );
                     handlerFn(err, response, body);
                 }
             );
@@ -579,14 +612,32 @@ module.exports.MatrixHttpApi.prototype = {
  *
  * If onlyData is true, the defer/callback is invoked with the body of the
  * response, otherwise the result code.
+ *
+ * If parseErrorJson is true, we will JSON.parse the body if we get a 4xx error.
+ *
  */
-var requestCallback = function(defer, userDefinedCallback, onlyData) {
+var requestCallback = function(
+    defer, userDefinedCallback, onlyData,
+    parseErrorJson
+) {
     userDefinedCallback = userDefinedCallback || function() {};
 
     return function(err, response, body) {
-        if (!err && response.statusCode >= 400) {
-            err = new module.exports.MatrixError(body);
-            err.httpStatus = response.statusCode;
+        if (!err) {
+            try {
+                if (response.statusCode >= 400) {
+                    if (parseErrorJson) {
+                        // we won't have json-decoded the response.
+                        body = JSON.parse(body);
+                    }
+                    err = new module.exports.MatrixError(body);
+                }
+            } catch (e) {
+                err = e;
+            }
+            if (err) {
+                err.httpStatus = response.statusCode;
+            }
         }
 
         if (err) {

spec/integ/matrix-client-methods.spec.js

@@ -43,17 +43,13 @@ describe("MatrixClient", function() {
             httpBackend.when(
                 "POST", "/_matrix/media/v1/upload"
             ).check(function(req) {
-                console.log("Request", req);
-
                 expect(req.data).toEqual(buf);
                 expect(req.queryParams.filename).toEqual("hi.txt");
                 expect(req.queryParams.access_token).toEqual(accessToken);
                 expect(req.headers["Content-Type"]).toEqual("text/plain");
                 expect(req.opts.json).toBeFalsy();
                 expect(req.opts.timeout).toBe(undefined);
-            }).respond(200, {
-                "content_uri": "uri"
-            });
+            }).respond(200, "content");
 
             var prom = client.uploadContent({
                 stream: buf,
@@ -69,8 +65,8 @@ describe("MatrixClient", function() {
             expect(uploads[0].loaded).toEqual(0);
 
             prom.then(function(response) {
-                console.log("Response", response);
-                expect(response.content_uri).toEqual("uri");
+                // for backwards compatibility, we return the raw JSON
+                expect(response).toEqual("content");
 
                 var uploads = client.getCurrentUploads();
                 expect(uploads.length).toEqual(0);
@@ -79,6 +75,33 @@ describe("MatrixClient", function() {
             httpBackend.flush();
         });
 
+        it("should parse errors into a MatrixError", function(done) {
+            // opts.json is false, so request returns unparsed json.
+            httpBackend.when(
+                "POST", "/_matrix/media/v1/upload"
+            ).check(function(req) {
+                expect(req.data).toEqual(buf);
+                expect(req.opts.json).toBeFalsy();
+            }).respond(400, JSON.stringify({
+                "errcode": "M_SNAFU",
+                "error": "broken",
+            }));
+
+            client.uploadContent({
+                stream: buf,
+                name: "hi.txt",
+                type: "text/plain",
+            }).then(function(response) {
+                throw Error("request not failed");
+            }, function(error) {
+                expect(error.httpStatus).toEqual(400);
+                expect(error.errcode).toEqual("M_SNAFU");
+                expect(error.message).toEqual("broken");
+            }).catch(utils.failTest).done(done);
+
+            httpBackend.flush();
+        });
+
         it("should return a promise which can be cancelled", function(done) {
             var prom = client.uploadContent({
                 stream: buf,