Added better error and OAuth support in xhr.js.

remoting/webapp/crd/js/xhr.js

Index: remoting/webapp/crd/js/xhr.js
diff --git a/remoting/webapp/crd/js/xhr.js b/remoting/webapp/crd/js/xhr.js
index cdefb0aa4d81d45d2dee8763a2d4f19c3404c651..5280e44d6d4dcafa986d7489441b3f7054bd2ca3 100644
--- a/remoting/webapp/crd/js/xhr.js
+++ b/remoting/webapp/crd/js/xhr.js
@@ -17,13 +17,10 @@ var remoting = remoting || {};
  * @param {remoting.Xhr.Params} params
  */
 remoting.Xhr = function(params) {
-  /** @private @const {!XMLHttpRequest} */
-  this.nativeXhr_ = new XMLHttpRequest();
-  this.nativeXhr_.onreadystatechange = this.onReadyStateChange_.bind(this);
-  this.nativeXhr_.withCredentials = params.withCredentials || false;
+  remoting.Xhr.checkParams_(params);
 
   /** @private @const */
-  this.responseType_ = params.responseType || remoting.Xhr.ResponseType.TEXT;
+  this.ignoreErrors_ = params.ignoreErrors || null;
 
   // Apply URL parameters.
   var url = params.url;
@@ -35,92 +32,91 @@ remoting.Xhr = function(params) {
         remoting.Xhr.removeNullFields_(params.urlParams));
   }
   if (parameterString) {
-    base.debug.assert(url.indexOf('?') == -1);
     url += '?' + parameterString;
   }
 
-  // Check that the content spec is consistent.
-  if ((Number(params.textContent !== undefined) +
-       Number(params.formContent !== undefined) +
-       Number(params.jsonContent !== undefined)) > 1) {
-    throw new Error(
-        'may only specify one of textContent, formContent, and jsonContent');
-  }
-
   // Prepare the build modified headers.
-  var headers = remoting.Xhr.removeNullFields_(params.headers);
+  /** @const */
+  this.headers_ = remoting.Xhr.removeNullFields_(params.headers);
 
   // Convert the content fields to a single text content variable.
   /** @private {?string} */
   this.content_ = null;
   if (params.textContent !== undefined) {
+    this.maybeSetContentType_('text/plain');
     this.content_ = params.textContent;
   } else if (params.formContent !== undefined) {
-    if (!('Content-type' in headers)) {
-      headers['Content-type'] = 'application/x-www-form-urlencoded';
-    }
+    this.maybeSetContentType_('application/x-www-form-urlencoded');
     this.content_ = remoting.Xhr.urlencodeParamHash(params.formContent);
   } else if (params.jsonContent !== undefined) {
-    if (!('Content-type' in headers)) {
-      headers['Content-type'] = 'application/json; charset=UTF-8';
-    }
+    this.maybeSetContentType_('application/json');
     this.content_ = JSON.stringify(params.jsonContent);
   }
 
   // Apply the oauthToken field.
   if (params.oauthToken !== undefined) {
-    base.debug.assert(!('Authorization' in headers));
-    headers['Authorization'] = 'Bearer ' + params.oauthToken;
+    this.setAuthToken_(params.oauthToken);
   }
 
-  this.nativeXhr_.open(params.method, url, true);
-  for (var key in headers) {
-    this.nativeXhr_.setRequestHeader(key, headers[key]);
+  /** @private @const {boolean} */
+  this.acceptJson_ = params.acceptJson || false;
+  if (this.acceptJson_) {
+    this.maybeSetHeader_('Accept', 'application/json');
   }
 
+  // Apply useIdentity field.
+  /** @const {boolean} */
+  this.useIdentity_ = params.useIdentity || false;
+
+  /** @private @const {!XMLHttpRequest} */
+  this.nativeXhr_ = new XMLHttpRequest();
+  this.nativeXhr_.onreadystatechange = this.onReadyStateChange_.bind(this);
+  this.nativeXhr_.withCredentials = params.withCredentials || false;
+  this.nativeXhr_.open(params.method, url, true);
+
   /** @private {base.Deferred<!remoting.Xhr.Response>} */
   this.deferred_ = null;
-};
 
-/**
- * @enum {string}
- */
-remoting.Xhr.ResponseType = {
-  TEXT: 'TEXT',  // Request a plain text response (default).
-  JSON: 'JSON',  // Request a JSON response.
-  NONE: 'NONE'   // Don't request any response.
+  /** @private {boolean} True if the request has been aborted. */
+  this.aborted_ = false;

[Jamie, 2015/03/23 23:11:33]

Do you need to distinguish between this case and |deferred_| == null?

[John Williams, 2015/03/24 00:52:12]

Yes, when useIdentity is true.  Although I didn't realize until just now that,
in the one place where abort() is used, useIdentity is false.

[Jamie, 2015/03/24 19:44:11]

I don't understand why you need it when useIdentity is true. deferred_ is set in
start() and if abort() is intended to prevent the corresponding promise from
being fulfilled, it seems like it should null it. That said, I'm hopeful that
abort() can be removed altogether (see below).

[John Williams, 2015/03/24 23:34:40]

It's moot since I got rid of the method, but be case I'm trying to avoid looks
like this:

1. User calls start.
2. A token is requested from the identity API.
3. User calls abort().
4. The native XHR's abort() method is called, resulting in an exception, which
is caught and ignored.
5. The identity API returns a token.
6. The native XHR is sent, even though it should have been aborted.

+
 };
 
 /**
- * Parameters for the 'start' function.
+ * Parameters for the 'start' function.  Unless otherwise noted, all
+ * parameters are optional.
+ *
+ * method: (required) The HTTP method to use.
  *
- * method: The HTTP method to use.
+ * url: (required) The URL to request.
  *
- * url: The URL to request.
+ * urlParams: Parameters to be appended to the URL.  Null-valued
+ *     parameters are omitted.
  *
- * urlParams: (optional) Parameters to be appended to the URL.
- *     Null-valued parameters are omitted.
+ * textContent: Text to be sent as the request body.
  *
- * textContent: (optional) Text to be sent as the request body.
+ * formContent: Data to be URL-encoded and sent as the request body.
+ *     Causes Content-type header to be set appropriately.
  *
- * formContent: (optional) Data to be URL-encoded and sent as the
- *     request body.  Causes Content-type header to be set
- *     appropriately.
+ * jsonContent: Data to be JSON-encoded and sent as the request body.
+ *     Causes Content-type header to be set appropriately.
  *
- * jsonContent: (optional) Data to be JSON-encoded and sent as the
- *     request body.  Causes Content-type header to be set
- *     appropriately.
+ * headers: Additional request headers to be sent.  Null-valued
+ *     headers are omitted.
  *
- * headers: (optional) Additional request headers to be sent.
- *     Null-valued headers are omitted.
+ * withCredentials: Value of the XHR's withCredentials field.
  *
- * withCredentials: (optional) Value of the XHR's withCredentials field.
+ * oauthToken: An OAuth2 token used to construct an Authentication
+ *     header.
  *
- * oauthToken: (optional) An OAuth2 token used to construct an
- *     Authentication header.
+ * useIdentity: Use identity API to get an OAuth2 token.
  *
- * responseType: (optional) Request a response of a specific
- *    type. Default: TEXT.
+ * ignoreErrors: List of error types (arising from HTTP result codes)
+ *     to ignore.  If null (the default) no HTTP status code is
+ *     treated as an error.

[Jamie, 2015/03/23 23:11:34]

This seems like an odd thing to put into a general-purpose API, and it's only
used by a couple of callers. I think it would be better to remove it and have
those sites handle the expected errors explicitly.

[John Williams, 2015/03/24 00:52:12]

Done.

+ *
+ * acceptJson: If true, send an Accept header indicating that a JSON
+ *     response is expected.
  *
  * @typedef {{
  *   method: string,
@@ -132,17 +128,27 @@ remoting.Xhr.ResponseType = {
  *   headers:(Object<string,?string>|undefined),
  *   withCredentials:(boolean|undefined),
  *   oauthToken:(string|undefined),
- *   responseType:(remoting.Xhr.ResponseType|undefined)
+ *   useIdentity:(boolean|undefined),
+ *   ignoreErrors:(Array<remoting.Error.Tag>|undefined),
+ *   acceptJson:(boolean|undefined)
  * }}
  */
 remoting.Xhr.Params;
 
 /**
- * Aborts the HTTP request.  Does nothing is the request has finished
- * already.
+ * Aborts the HTTP request.  Does nothing if the request has finished
+ * already.  Prevents the promise returned by start() from ever being
+ * resolved or rejected.

[Jamie, 2015/03/23 23:11:34]

Is that the right contract for this call? Generally, the call that returned the
original Promise is waiting for it to be fulfilled (at which point it may, for
example, remove a UI spinner). By stating that abort() bypasses this
fulfillment, any call to abort() needs to know what that code would have done
and duplicate it. Rejecting the Promise with CANCELLED would seem like a more
consistent contract.

[John Williams, 2015/03/24 00:52:12]

So far, this method has exactly one call site, and changing the semantics as you
suggest would either complicate the call site or cause a spurious error message
to be logged.

Basically I see this method as a necessary evil, so I'm not keen on adding more
features to it.

[Jamie, 2015/03/24 19:44:11]

What's the call-site? I don't see it in the diffs, but that may simply be
because the new code is API-compatible with the old. Could we remove it and
simply ignore the result? Something like abort() is inherently racy, so the
caller should not be relying on it for cancelling a request before it takes
effect anyway.

[John Williams, 2015/03/24 23:34:40]

It was API compatible, but you should see it now that I removed it :)

  */
 remoting.Xhr.prototype.abort = function() {
-  this.nativeXhr_.abort();
+  this.aborted_ = true;
+  try {
+    this.nativeXhr_.abort();
+  } catch (error) {
+    // This abort method throws an exception if called before the
+    // request is sent.  This isn't an error for our purposes, so we
+    // just ignore the exception.
+  }
 };
 
 /**
@@ -160,22 +166,128 @@ remoting.Xhr.prototype.abort = function() {
  */
 remoting.Xhr.prototype.start = function() {
   if (this.deferred_ == null) {
-    var xhr = this.nativeXhr_;
-    xhr.send(this.content_);
-    this.content_ = null;  // for gc
     this.deferred_ = new base.Deferred();
+
+    // Send the XHR, possibly after getting an OAuth token.
+    var self = this;

[Jamie, 2015/03/23 23:11:33]

We've tended to use |that|, rather than |self| when we have to use this pattern.

[John Williams, 2015/03/24 00:52:12]

Done, but I'm not a big fan because in ordinary usage, "this" and "that"
necessarily refer to different things.

+    if (this.useIdentity_) {
+      remoting.identity.getToken().then(function(token) {
+        base.debug.assert(self.nativeXhr_.readyState == 1);
+        self.setAuthToken_(token);
+        self.sendXhr_();
+      }, function(error) {

[Jamie, 2015/03/23 23:11:33]

Please use the more explicit catch() method, rather than a second parameter to
then().

[John Williams, 2015/03/24 00:52:12]

OK.  It makes no difference here but I should point out that this is a semantic
change.  With P.then(A).catch(B), any errors generate by A will be handled by B,
but with P.then(A,B), B is only run when P itself is rejected, and it never sees
any errors that might be generated by A.

[Jamie, 2015/03/24 19:44:11]

Thanks for the clarification. I wasn't aware of that nuance.

+        if (!self.aborted_) {
+          self.deferred_.reject(error);
+        }
+      });
+    } else {
+      this.sendXhr_();
+    }
   }
   return this.deferred_.promise();
 };
 
 /**
+ * @param {remoting.Xhr.Params} params
+ * @throws {Error} if params are invalid
+ */
+remoting.Xhr.checkParams_ = function(params) {
+  if (params.urlParams) {
+    if (params.url.indexOf('?') != -1) {
+      throw new Error('URL may not contain "?" when urlParams is set');
+    }
+    if (params.url.indexOf('#') != -1) {
+      throw new Error('URL may not contain "#" when urlParams is set');
+    }
+  }
+
+  if ((Number(params.textContent !== undefined) +
+       Number(params.formContent !== undefined) +
+       Number(params.jsonContent !== undefined)) > 1) {
+    throw new Error(
+        'may only specify one of textContent, formContent, and jsonContent');
+  }
+
+  if (params.useIdentity && params.oauthToken !== undefined) {
+    throw new Error('may not specify both useIdentity and oauthToken');
+  }
+
+  if ((params.useIdentity || params.oauthToken !== undefined) &&
+      params.headers &&
+      params.headers['Authorization'] != null) {
+    throw new Error(
+        'may not specify useIdentity or oauthToken ' +
+        'with an Authorization header');
+  }
+};
+
+/**
+ * @param {string} token
+ * @private
+ */
+remoting.Xhr.prototype.setAuthToken_ = function(token) {
+  this.setHeader_('Authorization', 'Bearer ' + token);
+};
+
+/**
+ * @param {string} type
+ * @private
+ */
+remoting.Xhr.prototype.maybeSetContentType_ = function(type) {
+  this.maybeSetHeader_('Content-type', type + '; charset=UTF-8');
+};
+
+/**
+ * @param {string} key
+ * @param {string} value
+ * @private
+ */
+remoting.Xhr.prototype.setHeader_ = function(key, value) {
+  var wasSet = this.maybeSetHeader_(key, value);
+  base.debug.assert(wasSet);
+};
+
+/**
+ * @param {string} key
+ * @param {string} value
+ * @return {boolean}
+ * @private
+ */
+remoting.Xhr.prototype.maybeSetHeader_ = function(key, value) {
+  if (!(key in this.headers_)) {
+    this.headers_[key] = value;
+    return true;
+  }
+  return false;
+};
+
+/** @private */
+remoting.Xhr.prototype.sendXhr_ = function() {
+  if (!this.aborted_) {
+    for (var key in this.headers_) {
+      this.nativeXhr_.setRequestHeader(key, this.headers_[key]);
+    }
+    this.nativeXhr_.send(this.content_);
+  }
+  this.content_ = null;  // for gc
+};
+
+/**
  * @private
  */
 remoting.Xhr.prototype.onReadyStateChange_ = function() {
   var xhr = this.nativeXhr_;
-  if (xhr.readyState == 4) {
-    // See comments at remoting.Xhr.Response.
-    this.deferred_.resolve(new remoting.Xhr.Response(xhr, this.responseType_));
+  if (xhr.readyState == 4 && !this.aborted_) {
+    var error = remoting.Error.fromHttpStatus(xhr.status);

[Jamie, 2015/03/23 23:11:34]

I don't think that this is the right place to do this translation.
remoting.Error is essentially a set of user-facing error conditions used mostly
for translation purposes. It has a mapping from HTTP status, but that mapping is
somewhat open to interpretation by the call-site, and is lossy. I think it would
be better to handle 2xx as resolve and anything else as reject, but just pass
the status code to the caller and let them decide what to do with it, otherwise
you're throwing away information that might be useful.

[John Williams, 2015/03/24 00:52:12]

I'm not sure I agree, but I'll compromise and just remove the error handling
entirely since there aren't many places that use it.

+    if (error.isNone() ||
+        this.ignoreErrors_ == null ||
+        error.hasTag.apply(error, this.ignoreErrors_)) {
+      // See comments at remoting.Xhr.Response.
+      this.deferred_.resolve(new remoting.Xhr.Response(
+          xhr, this.acceptJson_));
+    } else {
+      this.deferred_.reject(error);
+    }
   }
 };
 
@@ -189,11 +301,11 @@ remoting.Xhr.prototype.onReadyStateChange_ = function() {
  *
  * @constructor
  * @param {!XMLHttpRequest} xhr
- * @param {remoting.Xhr.ResponseType} type
+ * @param {boolean} allowJson
  */
-remoting.Xhr.Response = function(xhr, type) {
+remoting.Xhr.Response = function(xhr, allowJson) {
   /** @private @const */
-  this.type_ = type;
+  this.allowJson_ = allowJson;
 
   /**
    * The HTTP status code.
@@ -215,6 +327,9 @@ remoting.Xhr.Response = function(xhr, type) {
 
   /** @private {string} */
   this.text_ = xhr.responseText || '';
+
+  /** @private {*|undefined}  */
+  this.json_ = undefined;
 };
 
 /**
@@ -225,11 +340,16 @@ remoting.Xhr.Response.prototype.getText = function() {
 };
 
 /**
+ * Get the JSON content of the response.  Requires acceptJson to have
+ * been true in the request.
  * @return {*} The parsed JSON content of the response.
  */
 remoting.Xhr.Response.prototype.getJson = function() {
-  base.debug.assert(this.type_ == remoting.Xhr.ResponseType.JSON);
-  return JSON.parse(this.text_);
+  base.debug.assert(this.allowJson_);
+  if (this.json_ === undefined) {
+    this.json_ = JSON.parse(this.text_);
+  }
+  return this.json_;
 };
 
 /**
@@ -271,33 +391,3 @@ remoting.Xhr.urlencodeParamHash = function(paramHash) {
   }
   return '';
 };
-
-/**
- * Generic success/failure response proxy.
- *
- * TODO(jrw): Stop using this and move default error handling directly
- * into Xhr class.
- *
- * @param {function():void} onDone
- * @param {function(!remoting.Error):void} onError
- * @param {Array<remoting.Error.Tag>=} opt_ignoreErrors
- * @return {function(!remoting.Xhr.Response):void}
- */
-remoting.Xhr.defaultResponse = function(onDone, onError, opt_ignoreErrors) {
-  /** @param {!remoting.Xhr.Response} response */
-  var result = function(response) {
-    var error = remoting.Error.fromHttpStatus(response.status);
-    if (error.isNone()) {
-      onDone();
-      return;
-    }
-
-    if (opt_ignoreErrors && error.hasTag.apply(error, opt_ignoreErrors)) {
-      onDone();
-      return;
-    }
-
-    onError(error);
-  };
-  return result;
-};