Merge pull request #51 from mozilla-services/improved-fetch-err-messages

Fixes #12 - Improved Api#fetchChangesSince error messages.
Kinto · Jul 7, 2015 · e98f15a · e98f15a
2 parents 627bd2e + 010b903
commit e98f15a
Show file tree

Hide file tree

Showing 3 changed files with 96 additions and 16 deletions.
diff --git a/src/api.js b/src/api.js
@@ -1,6 +1,7 @@
 "use strict";
 
 import { quote, unquote } from "./utils.js";
+import ERROR_CODES from "./errors.js";
 
 const RECORD_FIELDS_TO_CLEAN = ["_status", "last_modified"];
 
@@ -19,6 +20,15 @@ const DEFAULT_REQUEST_HEADERS = {
   "Content-Type": "application/json",
 };
 
+function _handleFetchError(response, json) {
+  var message = `Fetching changes failed: HTTP ${response.status} `;
+  if (json.errno && ERROR_CODES.hasOwnProperty(json.errno))
+    message += ERROR_CODES[json.errno];
+  const err = new Error(message.trim());
+  err.data = json;
+  throw err;
+}
+
 export default class Api {
   constructor(remote, options={headers: {}}) {
     if (typeof(remote) !== "string" || !remote.length)
@@ -64,8 +74,8 @@ export default class Api {
    */
   fetchChangesSince(bucketName, collName, options={lastModified: null, headers: {}}) {
     const recordsUrl = this.endpoints().records(bucketName, collName);
-    var newLastModified;
-    var queryString = "";
+    const errPrefix = "Fetching changes failed";
+    var newLastModified, response, queryString = "";
     var headers = Object.assign({},
       DEFAULT_REQUEST_HEADERS,
       this.optionHeaders,
@@ -79,21 +89,27 @@ export default class Api {
 
     return fetch(recordsUrl + queryString, {headers})
       .then(res => {
+        response = res;
         // If HTTP 304, nothing has changed
-        if (res.status === 304) {
+        if (response.status === 304) {
           newLastModified = options.lastModified;
           return {data: []};
-        } else if (res.status >= 400) {
-          // TODO: attach better error reporting
-          throw new Error("Fetching changes failed: HTTP " + res.status);
         } else {
-          const etag = res.headers.get("ETag");  // e.g. '"42"'
-          // XXX: ETag are supposed to be opaque and stored «as-is».
-          newLastModified = parseInt(unquote(etag), 10);
-          return res.json();
+          return response.json();
         }
       })
+      .catch(err => {
+        throw new Error(`${errPrefix}: HTTP ${response.status}; ${err}`);
+      })
       .then(json => {
+        if (response.status >= 400) {
+          _handleFetchError(response, json);
+        } else {
+          const etag = response.headers.get("ETag");  // e.g. '"42"'
+          // XXX: ETag are supposed to be opaque and stored «as-is».
+          if (etag)
+            newLastModified = parseInt(unquote(etag), 10);
+        }
         return {
           lastModified: newLastModified,
           changes: json.data

diff --git a/src/errors.js b/src/errors.js
@@ -0,0 +1,21 @@
+export default {
+  104: "Missing Authorization Token",
+  105: "Invalid Authorization Token",
+  106: "Request body was not valid JSON",
+  107: "Invalid request parameter",
+  108: "Missing request parameter",
+  109: "Invalid posted data",
+  110: "Invalid Token / id",
+  111: "Missing Token / id",
+  112: "Content-Length header was not provided",
+  113: "Request body too large",
+  114: "Resource was modified meanwhile",
+  115: "Method not allowed on this end point",
+  116: "Requested version not available on this server",
+  117: "Client has sent too many requests",
+  121: "Resource access is forbidden for this user",
+  122: "Another resource violates constraint",
+  201: "Service Temporary unavailable due to high load",
+  202: "Service deprecated",
+  999: "Internal Server Error",
+};
diff --git a/test/api_test.js b/test/api_test.js
@@ -134,15 +134,17 @@ describe("Api", () => {
 
   describe("#fetchChangesSince", () => {
     it("should request server for latest changes", () => {
-      sandbox.stub(root, "fetch").returns(Promise.resolve());
+      sandbox.stub(root, "fetch").returns(
+        fakeServerResponse(200, { data: [] }, { }));
 
       api.fetchChangesSince("blog", "articles");
 
       sinon.assert.calledOnce(fetch);
     });
 
     it("should merge instance option headers", () => {
-      sandbox.stub(root, "fetch").returns(Promise.resolve());
+      sandbox.stub(root, "fetch").returns(
+        fakeServerResponse(200, { data: [] }, { }));
       api.optionHeaders = {Foo: "Bar"};
 
       api.fetchChangesSince("blog", "articles");
@@ -154,7 +156,8 @@ describe("Api", () => {
     });
 
     it("should request server changes since last modified", () =>{
-      sandbox.stub(root, "fetch").returns(Promise.resolve());
+      sandbox.stub(root, "fetch").returns(
+        fakeServerResponse(200, { data: [] }, { }));
 
       api.fetchChangesSince("blog", "articles", {lastModified: 42});
 
@@ -163,7 +166,8 @@ describe("Api", () => {
     });
 
     it("should attach an If-None-Match header if lastModified is provided", () =>{
-      sandbox.stub(root, "fetch").returns(Promise.resolve());
+      sandbox.stub(root, "fetch").returns(
+        fakeServerResponse(200, { data: [] }, { }));
       api.fetchChangesSince("blog", "articles", {lastModified: 42});
 
       sinon.assert.calledOnce(fetch);
@@ -184,7 +188,8 @@ describe("Api", () => {
     });
 
     it("should merge provided headers with default ones", () => {
-      sandbox.stub(root, "fetch").returns(Promise.resolve());
+      sandbox.stub(root, "fetch").returns(
+        fakeServerResponse(200, { data: [] }, { }));
 
       const options = {lastModified: 42, headers: {Foo: "bar"}};
       api.fetchChangesSince("blog", "articles", options);
@@ -209,9 +214,47 @@ describe("Api", () => {
     it("should reject on any HTTP status >= 400", () => {
       sandbox.stub(root, "fetch").returns(fakeServerResponse(401, {}));
 
-      return api.fetchChangesSince("blog", "articles", {lastModified: 42})
+      return api.fetchChangesSince("blog", "articles")
         .should.eventually.be.rejectedWith(Error, /failed: HTTP 401/);
     });
+
+    it("should reject with detailed error message", () => {
+      sandbox.stub(root, "fetch").returns(fakeServerResponse(401, {
+        errno: 105
+      }));
+
+      return api.fetchChangesSince("blog", "articles")
+        .should.eventually.be.rejectedWith(Error, /failed: HTTP 401 Invalid Authorization Token/);
+    });
+
+    it("should reject with fallback error message", () => {
+      sandbox.stub(root, "fetch").returns(fakeServerResponse(401, {}));
+
+      return api.fetchChangesSince("blog", "articles")
+        .should.eventually.be.rejectedWith(Error, /failed: HTTP 401$/);
+    });
+
+    it("should expose json response body to err object on rejection", () => {
+      const response = {errno: 105, message: "Dude."};
+
+      sandbox.stub(root, "fetch").returns(fakeServerResponse(401, response));
+
+      return api.fetchChangesSince("blog", "articles")
+        .catch(err => err.data)
+        .should.eventually.become(response);
+    });
+
+    it("should reject on on invalid json response body", () => {
+      sandbox.stub(root, "fetch").returns(Promise.resolve({
+        status: 500,
+        json() {
+          return Promise.reject("JSON Error");
+        }
+      }));
+
+      return api.fetchChangesSince("blog", "articles")
+        .should.eventually.be.rejectedWith(Error, /HTTP 500; JSON Error/);
+    });
   });
 
   describe("#batch", () => {