uploadContent: Attempt some consistency between browser and node

Previously, the API for uploadContent differed wildly depending on whether you
were on a browser with XMLHttpRequest or node.js with the HTTP system
library. This lead to great confusion, as well as making it hard to test the
browser behaviour.

The browser version expected a File, which could be sent straight to
XMLHttpRequest, whereas the node.js version expected an object with a `stream`
property. Now, we no longer recommend the `stream` property (though maintain it
for backwards compatibility) and instead expect the first argument to be the
thing to upload. To support the different ways of passing `type` and `name`,
they can now either be properties of the first argument (which will probably
suit browsers), or passed in as explicit `opts` (which will suit the node.js
users).

Even more crazily, the browser version returned the value of the `content_uri`
property of the result, while the node.js returned the raw JSON. Both flew in
the face of the convention of the js-sdk, which is to return the entire parsed
result object. Hence, add `rawResponse` and `onlyContentUri` options, which
grandfather in those behaviours.

lib/base-apis.js

@@ -574,13 +574,38 @@ MatrixBaseApis.prototype.setRoomDirectoryVisibility =
 
 /**
  * Upload a file to the media repository on the home server.
- * @param {File} file object
+ *
- * @param {module:client.callback} callback Optional.
+ * @param {object} file The object to upload. On a browser, something that
- * @return {module:client.Promise} Resolves: TODO
+ *   can be sent to XMLHttpRequest.send (typically a File).  Under node.js,
- * @return {module:http-api.MatrixError} Rejects: with an error response.
+ *   a a Buffer, String or ReadStream.
+ *
+ * @param {object} opts  options object
+ *
+ * @param {string=} opts.name   Name to give the file on the server. Defaults
+ *   to <tt>file.name</tt>.
+ *
+ * @param {string=} opts.type   Content-type for the upload. Defaults to
+ *   <tt>file.type</tt>, or <tt>applicaton/octet-stream</tt>.
+ *
+ * @param {boolean=} opts.rawResponse Return the raw body, rather than
+ *   parsing the JSON. Defaults to false (except on node.js, where it
+ *   defaults to true for backwards compatibility).
+ *
+ * @param {boolean=} opts.onlyContentUri Just return the content URI,
+ *   rather than the whole body. Defaults to false (except on browsers,
+ *   where it defaults to true for backwards compatibility). Ignored if
+ *   opts.rawResponse is true.
+ *
+ * @param {Function=} opts.callback Deprecated. Optional. The callback to
+ *    invoke on success/failure. See the promise return values for more
+ *    information.
+ *
+ * @return {module:client.Promise} Resolves to response object, as
+ *    determined by this.opts.onlyData, opts.rawResponse, and
+ *    opts.onlyContentUri.  Rejects with an error (usually a MatrixError).
  */
-MatrixBaseApis.prototype.uploadContent = function(file, callback) {
+MatrixBaseApis.prototype.uploadContent = function(file, opts) {
-    return this._http.uploadContent(file, callback);
+    return this._http.uploadContent(file, opts);
 };
 
 /**

lib/http-api.js

@@ -63,9 +63,11 @@ module.exports.PREFIX_MEDIA_R0 = "/_matrix/media/r0";
  * requests. This function must look like function(opts, callback){ ... }.
  * @param {string} opts.prefix Required. The matrix client prefix to use, e.g.
  * '/_matrix/client/r0'. See PREFIX_R0 and PREFIX_UNSTABLE for constants.
- * @param {bool} opts.onlyData True to return only the 'data' component of the
+ *
- * response (e.g. the parsed HTTP body). If false, requests will return status
+ * @param {bool=} opts.onlyData True to return only the 'data' component of the
- * codes and headers in addition to data. Default: false.
+ * response (e.g. the parsed HTTP body). If false, requests will return an
+ * object with the properties <tt>code</tt>, <tt>headers</tt> and <tt>data</tt>.
+ *
  * @param {string} opts.accessToken The access_token to send with requests. Can be
  * null to not send an access token.
  * @param {Object} opts.extraParams Optional. Extra query parameters to send on
@@ -99,20 +101,87 @@ module.exports.MatrixHttpApi.prototype = {
 
     /**
      * Upload content to the Home Server
-     * @param {File} file A File object (in a browser) or in Node,
+     *
-                                 an object with properties:
+     * @param {object} file The object to upload. On a browser, something that
-                                 name: The file's name
+     *   can be sent to XMLHttpRequest.send (typically a File).  Under node.js,
-                                 stream: A read stream
+     *   a a Buffer, String or ReadStream.
-     * @param {Function} callback Optional. The callback to invoke on
+     *
-     * success/failure. See the promise return values for more information.
+     * @param {object} opts  options object
-     * @return {module:client.Promise} Resolves to <code>{data: {Object},
+     *
+     * @param {string=} opts.name   Name to give the file on the server. Defaults
+     *   to <tt>file.name</tt>.
+     *
+     * @param {string=} opts.type   Content-type for the upload. Defaults to
+     *   <tt>file.type</tt>, or <tt>applicaton/octet-stream</tt>.
+     *
+     * @param {boolean=} opts.rawResponse Return the raw body, rather than
+     *   parsing the JSON. Defaults to false (except on node.js, where it
+     *   defaults to true for backwards compatibility).
+     *
+     * @param {boolean=} opts.onlyContentUri Just return the content URI,
+     *   rather than the whole body. Defaults to false (except on browsers,
+     *   where it defaults to true for backwards compatibility). Ignored if
+     *   opts.rawResponse is true.
+     *
+     * @param {Function=} opts.callback Deprecated. Optional. The callback to
+     *    invoke on success/failure. See the promise return values for more
+     *    information.
+     *
+     * @return {module:client.Promise} Resolves to response object, as
+     *    determined by this.opts.onlyData, opts.rawResponse, and
+     *    opts.onlyContentUri.  Rejects with an error (usually a MatrixError).
      */
-    uploadContent: function(file, callback) {
+    uploadContent: function(file, opts) {
-        if (callback !== undefined && !utils.isFunction(callback)) {
+        if (utils.isFunction(opts)) {
-            throw Error(
+            // opts used to be callback
-                "Expected callback to be a function but got " + typeof callback
+            opts = {
-            );
+                callback: opts,
+            };
+        } else if (opts === undefined) {
+            opts = {};
         }
+
+        // if the file doesn't have a mime type, use a default since
+        // the HS errors if we don't supply one.
+        var contentType = opts.type || file.type || 'application/octet-stream';
+        var fileName = opts.name || file.name;
+
+        // we used to recommend setting file.stream to the thing to upload on
+        // nodejs.
+        var body = file.stream ? file.stream : file;
+
+        // backwards-compatibility hacks where we used to do different things
+        // between browser and node.
+        var rawResponse = opts.rawResponse;
+        if (rawResponse === undefined) {
+            if (global.XMLHttpRequest) {
+                rawResponse = false;
+            } else {
+                console.warn(
+                    "Returning the raw JSON from uploadContent(). Future " +
+                    "versions of the js-sdk will change this default, to " +
+                    "return the parsed object. Set opts.rawResponse=false " +
+                    "to change this behaviour now."
+                );
+                rawResponse = true;
+            }
+        }
+
+        var onlyContentUri = opts.onlyContentUri;
+        if (!rawResponse && onlyContentUri === undefined) {
+            if (global.XMLHttpRequest) {
+                console.warn(
+                    "Returning only the content-uri from uploadContent(). " +
+                    "Future versions of the js-sdk will change this " +
+                    "default, to return the whole response object. Set " +
+                    "opts.onlyContentUri=false to change this behaviour now."
+                );
+                onlyContentUri = true;
+            } else {
+                onlyContentUri = false;
+            }
+        }
+
         // browser-request doesn't support File objects because it deep-copies
         // the options using JSON.parse(JSON.stringify(options)). Instead of
         // loading the whole file into memory as a string and letting
@@ -123,11 +192,30 @@ module.exports.MatrixHttpApi.prototype = {
 
         var upload = { loaded: 0, total: 0 };
         var promise;
+
+        // XMLHttpRequest doesn't parse JSON for us. request normally does, but
+        // we're setting opts.json=false so that it doesn't JSON-encode the
+        // request, which also means it doesn't JSON-decode the response. Either
+        // way, we have to JSON-parse the response ourselves.
+        var bodyParser = null;
+        if (!rawResponse) {
+            bodyParser = function(rawBody) {
+                var body = JSON.parse(rawBody);
+                if (onlyContentUri) {
+                    body = body.content_uri;
+                    if (body === undefined) {
+                        throw Error('Bad response');
+                    }
+                }
+                return body;
+            };
+        }
+
         if (global.XMLHttpRequest) {
             var defer = q.defer();
             var xhr = new global.XMLHttpRequest();
             upload.xhr = xhr;
-            var cb = requestCallback(defer, callback, this.opts.onlyData);
+            var cb = requestCallback(defer, opts.callback, this.opts.onlyData);
 
             var timeout_fn = function() {
                 xhr.abort();
@@ -142,23 +230,21 @@ module.exports.MatrixHttpApi.prototype = {
                 switch (xhr.readyState) {
                     case global.XMLHttpRequest.DONE:
                         callbacks.clearTimeout(xhr.timeout_timer);
-                        var err;
+                        var resp;
+                        try {
                             if (!xhr.responseText) {
-                            err = new Error('No response body.');
+                                throw new Error('No response body.');
+                            }
+                            resp = xhr.responseText;
+                            if (bodyParser) {
+                                resp = bodyParser(resp);
+                            }
+                        } catch (err) {
                             err.http_status = xhr.status;
                             cb(err);
                             return;
                         }
-
+                        cb(undefined, xhr, resp);
-                        var resp = JSON.parse(xhr.responseText);
-                        if (resp.content_uri === undefined) {
-                            err = Error('Bad response');
-                            err.http_status = xhr.status;
-                            cb(err);
-                            return;
-                        }
-
-                        cb(undefined, xhr, resp.content_uri);
                         break;
                 }
             };
@@ -171,30 +257,26 @@ module.exports.MatrixHttpApi.prototype = {
             });
             var url = this.opts.baseUrl + "/_matrix/media/v1/upload";
             url += "?access_token=" + encodeURIComponent(this.opts.accessToken);
-            url += "&filename=" + encodeURIComponent(file.name);
+            url += "&filename=" + encodeURIComponent(fileName);
 
             xhr.open("POST", url);
-            if (file.type) {
+            xhr.setRequestHeader("Content-Type", contentType);
-                xhr.setRequestHeader("Content-Type", file.type);
+            xhr.send(body);
-            } else {
-                // if the file doesn't have a mime type, use a default since
-                // the HS errors if we don't supply one.
-                xhr.setRequestHeader("Content-Type", 'application/octet-stream');
-            }
-            xhr.send(file);
             promise = defer.promise;
 
             // dirty hack (as per _request) to allow the upload to be cancelled.
             promise.abort = xhr.abort.bind(xhr);
         } else {
             var queryParams = {
-                filename: file.name,
+                filename: fileName,
             };
+
             promise = this.authedRequest(
-                callback, "POST", "/upload", queryParams, file.stream, {
+                opts.callback, "POST", "/upload", queryParams, body, {
                     prefix: "/_matrix/media/v1",
-                    headers: {"Content-Type": file.type},
+                    headers: {"Content-Type": contentType},
                     json: false,
+                    bodyParser: bodyParser,
                 }
             );
         }
@@ -514,6 +596,9 @@ module.exports.MatrixHttpApi.prototype = {
      * @param {number=} opts.localTimeoutMs client-side timeout for the
      *    request. No timeout if undefined.
      *
+     * @param {function=} opts.bodyParser function to parse the body of the
+     *    response before passing it to the promise and callback.
+     *
      * @return {module:client.Promise} a promise which resolves to either the
      * response object (if this.opts.onlyData is truthy), or the parsed
      * body. Rejects
@@ -584,7 +669,8 @@ module.exports.MatrixHttpApi.prototype = {
                     var parseErrorJson = !json;
                     var handlerFn = requestCallback(
                         defer, callback, self.opts.onlyData,
-                        parseErrorJson
+                        parseErrorJson,
+                        opts.bodyParser
                     );
                     handlerFn(err, response, body);
                 }
@@ -618,7 +704,7 @@ module.exports.MatrixHttpApi.prototype = {
  */
 var requestCallback = function(
     defer, userDefinedCallback, onlyData,
-    parseErrorJson
+    parseErrorJson, bodyParser
 ) {
     userDefinedCallback = userDefinedCallback || function() {};
 
@@ -631,6 +717,8 @@ var requestCallback = function(
                         body = JSON.parse(body);
                     }
                     err = new module.exports.MatrixError(body);
+                } else if (bodyParser) {
+                    body = bodyParser(body);
                 }
             } catch (e) {
                 err = e;

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

@@ -75,6 +75,26 @@ describe("MatrixClient", function() {
             httpBackend.flush();
         });
 
+        it("should parse the response if rawResponse=false", function(done) {
+            httpBackend.when(
+                "POST", "/_matrix/media/v1/upload"
+            ).check(function(req) {
+                expect(req.opts.json).toBeFalsy();
+            }).respond(200, JSON.stringify({ "content_uri": "uri" }));
+
+            client.uploadContent({
+                stream: buf,
+                name: "hi.txt",
+                type: "text/plain",
+            }, {
+                rawResponse: false,
+            }).then(function(response) {
+                expect(response.content_uri).toEqual("uri");
+            }).catch(utils.failTest).done(done);
+
+            httpBackend.flush();
+        });
+
         it("should parse errors into a MatrixError", function(done) {
             // opts.json is false, so request returns unparsed json.
             httpBackend.when(