Allow xhr errors to parse json responses. (#3669)

ampproject · Jun 21, 2016 · a9d4c6b · a9d4c6b
1 parent fb559df
commit a9d4c6b
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 22 deletions.
diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js
@@ -157,8 +157,8 @@ export class Xhr {
     setupJson_(init);
 
     return this.fetchAmpCors_(input, init).then(response => {
-      return assertSuccess(response).json();
-    });
+      return assertSuccess(response);
+    }).then(response => response.json());
   }
 
   /**
@@ -177,8 +177,8 @@ export class Xhr {
     init.headers['Accept'] = 'text/html';
 
     return this.fetchAmpCors_(input, init).then(response => {
-      return assertSuccess(response).document_();
-    });
+      return assertSuccess(response);
+    }).then(response => response.document_());
   }
 
   /**
@@ -194,7 +194,7 @@ export class Xhr {
    */
   sendSignal(input, opt_init) {
     return this.fetchAmpCors_(input, opt_init).then(response => {
-      assertSuccess(response);
+      return assertSuccess(response);
     });
   }
 }
@@ -344,17 +344,31 @@ function isRetriable(status) {
 /**
  * Returns the response if successful or otherwise throws an error.
  * @paran {!FetchResponse} response
- * @return {!FetchResponse}
+ * @return {!Promise<!FetchResponse>}
+ * @private Visible for testing
  */
-function assertSuccess(response) {
-  if (!(response.status >= 200 && response.status < 300)) {
-    const err = user.createError(`HTTP error ${response.status}`);
-    if (isRetriable(response.status)) {
-      err.retriable = true;
+export function assertSuccess(response) {
+  return new Promise((resolve, reject) => {
+    if (response.status < 200 || response.status >= 300) {
+      const err = user.createError(`HTTP error ${response.status}`);
+      if (isRetriable(response.status)) {
+        err.retriable = true;
+      }
+      if (response.headers.get('Content-Type') == 'application/json') {
+        response.json().then(json => {
+          err.responseJson = json;
+          reject(err);
+        }).catch(() => {
+          // Ignore a failed json parsing and just throw the error without
+          // setting responseJson.
+          reject(err);
+        });
+      } else {
+        reject(err);
+      }
     }
-    throw err;
-  }
-  return response;
+    resolve(response);
+  });
 }
 
 

diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js
@@ -19,6 +19,7 @@ import {
   installXhrService,
   fetchPolyfill,
   FetchResponse,
+  assertSuccess,
 } from '../../src/service/xhr-impl';
 
 describe('XHR', function() {
@@ -29,14 +30,19 @@ describe('XHR', function() {
   this.timeout(5000);
 
   const scenarios = [
-    {xhr: installXhrService({
-      fetch: window.fetch,
-      location: {href: 'https://acme.com/path'},
-    }), desc: 'Native'},
-    {xhr: installXhrService({
-      fetch: fetchPolyfill,
-      location: {href: 'https://acme.com/path'},
-    }), desc: 'Polyfill'},
+    {
+      xhr: installXhrService({
+        fetch: window.fetch,
+        location: {href: 'https://acme.com/path'},
+      }),
+      desc: 'Native',
+    }, {
+      xhr: installXhrService({
+        fetch: fetchPolyfill,
+        location: {href: 'https://acme.com/path'},
+      }),
+      desc: 'Polyfill',
+    },
   ];
 
   function setupMockXhr() {
@@ -217,6 +223,63 @@ describe('XHR', function() {
 
     describe(test.desc, () => {
 
+      describe('assertSuccess', () => {
+        function createResponseInstance(body, init) {
+          if (test.desc == 'Native') {
+            return new Response(body, init);
+          } else {
+            init.responseText = body;
+            return new FetchResponse(init);
+          }
+        }
+        const mockXhr = {
+          status: 200,
+          headers: {
+            'Content-Type': 'plain/text',
+          },
+          getResponseHeader: () => '',
+        };
+
+        it('should resolve if success', () => {
+          mockXhr.status = 200;
+          return assertSuccess(createResponseInstance('', mockXhr))
+              .then(response => {
+                expect(response.status).to.equal(200);
+              }).should.not.be.rejected;
+        });
+
+        it('should reject if error', () => {
+          mockXhr.status = 500;
+          return assertSuccess(createResponseInstance('', mockXhr))
+              .then(response => {
+                expect(response.status).to.equal(500);
+              }).should.be.rejectedWith(/HTTP error 500/);
+        });
+
+        it('should parse json content when error', () => {
+          mockXhr.status = 500;
+          mockXhr.responseText = '{"a": "hello"}';
+          mockXhr.headers['Content-Type'] = 'application/json';
+          mockXhr.getResponseHeader = () => 'application/json';
+          return assertSuccess(createResponseInstance('{"a": 2}', mockXhr))
+              .catch(error => {
+                expect(error.responseJson).to.be.defined;
+                expect(error.responseJson.a).to.equal(2);
+              });
+        });
+
+        it('should not return a resolved indented promise', () => {
+          mockXhr.status = 500;
+          mockXhr.responseText = '{"a": "hello"}';
+          mockXhr.headers['Content-Type'] = 'application/json';
+          mockXhr.getResponseHeader = () => 'application/json';
+          const promise = assertSuccess(
+              createResponseInstance('{"a": 2}', mockXhr));
+          promise.should.be.rejected;
+          return promise;
+        });
+      });
+
       it('should do simple JSON fetch', () => {
         return xhr.fetchJson('https://httpbin.org/get?k=v1').then(res => {
           expect(res).to.exist;