fix(http): error on non-200 status codes

robwormald · robwormald · commit 201f189d0eaa · 2015-11-23T22:17:13.000Z
BREAKING CHANGE: previously http would only error on network errors to match the fetch specification. Now status codes less than 200 and greater than 299 will cause Http's Observable to error. Closes angular#5130.
diff --git a/modules/angular2/src/http/backends/xhr_backend.ts b/modules/angular2/src/http/backends/xhr_backend.ts
@@ -7,6 +7,7 @@ import {Injectable} from 'angular2/angular2';
 import {BrowserXhr} from './browser_xhr';
 import {isPresent} from 'angular2/src/facade/lang';
 import {Observable} from 'angular2/angular2';
+import {isSuccess} from '../http_utils';
 /**
 * Creates connections using `XMLHttpRequest`. Given a fully-qualified
 * request, an `XHRConnection` will immediately create an `XMLHttpRequest` object and send the
@@ -33,24 +34,30 @@ export class XHRConnection implements Connection {
         // responseText is the old-school way of retrieving response (supported by IE8 & 9)
         // response/responseType properties were introduced in XHR Level2 spec (supported by
         // IE10)
-        let response = isPresent(_xhr.response) ? _xhr.response : _xhr.responseText;
+        let xhrResponse = isPresent(_xhr.response) ? _xhr.response : _xhr.responseText;
+
 
         // normalize IE9 bug (http://bugs.jquery.com/ticket/1450)
-        let status = _xhr.status === 1223 ? 204 : _xhr.status;
+        let status: number = _xhr.status === 1223 ? 204 : _xhr.status;
 
         // fix status code when it is 0 (0 status is undocumented).
         // Occurs when accessing file resources or on Android 4.1 stock browser
         // while retrieving files from application cache.
         if (status === 0) {
-          status = response ? 200 : 0;
+          status = xhrResponse ? 200 : 0;
         }
-        var responseOptions = new ResponseOptions({body: response, status: status});
+        var responseOptions = new ResponseOptions({body: xhrResponse, status: status});
         if (isPresent(baseResponseOptions)) {
           responseOptions = baseResponseOptions.merge(responseOptions);
         }
-        responseObserver.next(new Response(responseOptions));
-        // TODO(gdi2290): defer complete if array buffer until done
-        responseObserver.complete();
+        let response = new Response(responseOptions);
+        if (isSuccess(status)) {
+          responseObserver.next(response);
+          // TODO(gdi2290): defer complete if array buffer until done
+          responseObserver.complete();
+          return;
+        }
+        responseObserver.error(response);
       };
       // error event handler
       let onError = (err) => {
diff --git a/modules/angular2/src/http/http_utils.ts b/modules/angular2/src/http/http_utils.ts
@@ -1,6 +1,7 @@
 import {isString} from 'angular2/src/facade/lang';
 import {RequestMethods} from './enums';
 import {makeTypeError} from 'angular2/src/facade/exceptions';
+import {Response} from './static_response';
 
 export function normalizeMethodName(method): RequestMethods {
   if (isString(method)) {
@@ -14,4 +15,6 @@ export function normalizeMethodName(method): RequestMethods {
   return method;
 }
 
+export const isSuccess = (status: number): boolean => (status >= 200 && status < 300);
+
 export {isJsObject} from 'angular2/src/facade/lang';
diff --git a/modules/angular2/test/http/backends/xhr_backend_spec.ts b/modules/angular2/test/http/backends/xhr_backend_spec.ts
@@ -99,6 +99,7 @@ export function main() {
              expect(res.type).toBe(ResponseTypes.Error);
              async.done();
            });
+           existingXHRs[0].setStatusCode(200);
            existingXHRs[0].dispatchEvent('load');
          }));
 
@@ -107,7 +108,7 @@ export function main() {
                                               new ResponseOptions({type: ResponseTypes.Error}));
            connection.response.subscribe(res => { expect(res.type).toBe(ResponseTypes.Error); },
                                          null, () => { async.done(); });
-
+           existingXHRs[0].setStatusCode(200);
            existingXHRs[0].dispatchEvent('load');
          }));
 
@@ -164,15 +165,57 @@ export function main() {
            var connection = new XHRConnection(sampleRequest, new MockBrowserXHR(),
                                               new ResponseOptions({status: statusCode}));
 
-           connection.response.subscribe(res => {
-             expect(res.status).toBe(statusCode);
-             async.done();
-           });
+           connection.response.subscribe(
+               res => {
+
+               },
+               errRes => {
+                 expect(errRes.status).toBe(statusCode);
+                 async.done();
+               });
+
+           existingXHRs[0].setStatusCode(statusCode);
+           existingXHRs[0].dispatchEvent('load');
+         }));
+
+      it('should call next and complete on 200 codes', inject([AsyncTestCompleter], async => {
+           var nextCalled = false;
+           var errorCalled = false;
+           var statusCode = 200;
+           var connection = new XHRConnection(sampleRequest, new MockBrowserXHR(),
+                                              new ResponseOptions({status: statusCode}));
+
+           connection.response.subscribe(
+               res => {
+                 nextCalled = true;
+                 expect(res.status).toBe(statusCode);
+               },
+               errRes => { errorCalled = true; }, () => {
+                 expect(nextCalled).toBe(true);
+                 expect(errorCalled).toBe(false);
+                 async.done();
+               });
 
            existingXHRs[0].setStatusCode(statusCode);
            existingXHRs[0].dispatchEvent('load');
          }));
 
+      it('should call error and not complete on 300+ codes', inject([AsyncTestCompleter], async => {
+           var nextCalled = false;
+           var errorCalled = false;
+           var statusCode = 301;
+           var connection = new XHRConnection(sampleRequest, new MockBrowserXHR(),
+                                              new ResponseOptions({status: statusCode}));
+
+           connection.response.subscribe(res => { nextCalled = true; }, errRes => {
+             expect(errRes.status).toBe(statusCode);
+             expect(nextCalled).toBe(false);
+             async.done();
+           }, () => { throw 'should not be called'; });
+
+           existingXHRs[0].setStatusCode(statusCode);
+           existingXHRs[0].dispatchEvent('load');
+         }));
       it('should normalize IE\'s 1223 status code into 204', inject([AsyncTestCompleter], async => {
            var statusCode = 1223;
            var normalizedCode = 204;
@@ -204,10 +247,11 @@ export function main() {
                expect(ress.text()).toBe(responseBody);
                async.done();
              });
+             existingXHRs[1].setStatusCode(200);
              existingXHRs[1].setResponse(responseBody);
              existingXHRs[1].dispatchEvent('load');
            });
-
+           existingXHRs[0].setStatusCode(200);
            existingXHRs[0].setResponseText(responseBody);
            existingXHRs[0].dispatchEvent('load');
          }));
