fix(ngResource): do not eat exceptions in success callback #15628

kylewuolle · 2017-01-19T20:19:40Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
Addresses bug identified in #15624.

No error message is output in the console:

var User = $resource('users/:id', {id: '@id'});
var user User.get({id: 123}, function (res) {
    y  = x + z; // this line not log any exception on the console
}, function () {
    console.log(arguments);
});

Error message is output in the console:

var User = $resource('users/:id', {id: '@id'});
var user User.get({id: 123}, function (res) {
    y  = x + z; // and this log the exception in the console
});

What is the new behavior (if this is a feature change)?
Now any exceptions thrown in the success callback will be thrown and not eaten.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

move the promise catch noop call inside of error callback handler so that errors inside of the success callback are not eaten added two tests to prove functionality No breaking changes

gkalpak

The implementation looks good. I've left a comment regarding the tests.

gkalpak · 2017-01-26T21:28:30Z

test/ngResource/resourceSpec.js

@@ -1978,4 +1978,39 @@ describe('configuring `cancellable` on the provider', function() {
    expect(creditCard3.$cancelRequest).toBeDefined();
  });
 });
+
+  describe('exception handling in callbacks', function() {


These tests should be incorporated into the errors block. Also, can you add tests for all 4 combinations (no errorCallback/no responseErrorInterceptor, no errorCallback/yes responseErrorInterceptor, yes errorCallback/no responseErrorInterceptor, yes errorCallback/yes responseErrorInterceptor).

thanks I made the changes requested!

…ded two more cases to cover all the scenarios. No breaking changes

kylewuolle · 2017-02-03T18:24:35Z

Thanks I made the changes

gkalpak · 2017-02-04T09:46:39Z

test/ngResource/resourceSpec.js

+
+  it('should throw exception in success callback when error callback provided and no responseErrorInterceptor', function() {
+    $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' });
+    var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' });


Remove the unnecessary bits (here and below). E.g.:

$httpBackend.expect('GET', '/CreditCard/123').respond(null); var CreditCard = $resource('/CreditCard/:id');

gkalpak · 2017-02-04T09:50:24Z

test/ngResource/resourceSpec.js

@@ -1709,6 +1709,79 @@ describe('handling rejections', function() {
      expect($exceptionHandler.errors[0]).toMatch(/^Possibly unhandled rejection/);
    })
  );
+
+
+  it('should throw exception in success callback when error callback provided and no responseErrorInterceptor', function() {


Change the tests descriptions to something like:

should not swallow exception in success callback...

gkalpak · 2017-02-04T09:52:28Z

test/ngResource/resourceSpec.js

+      var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, {
+        'get': {
+          method: 'GET',
+          interceptor: { responseError: function() { return $q.reject({}); } }


Make this a non-rejecting interceptor.

gkalpak · 2017-02-04T09:52:36Z

test/ngResource/resourceSpec.js

+      var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, {
+        'get': {
+          method: 'GET',
+          interceptor: { responseError: function() { return $q.reject({}); } }


Make this a non-rejecting interceptor.

…interceptors to non rejecting ones

kylewuolle · 2017-02-04T17:27:55Z

Thanks I fixed up the tests etc

gkalpak · 2017-02-04T18:42:31Z

LGTM 👍
_{I'll clean up the tests a bit more (e.g. remove unused default params ({ id: '@id.key' })) and merge.}

Previously, errors thrown inside the `success` callback would be swallowed by a noop `catch()` handler. The `catch()` handler was added in order to avoid an unnecessary "Possibly Unhandled Rejection" error, in case the user provided an `error` callback (which would handle request errors). The handler was added too "eagrly" and as a result would swallow errors thrown in the `success` callback, despite the fact that those errors would _not_ be handled by the `error` callback. This commit fixes this, by adding the `catch()` handler "lazily", only when it is certain that a rejection will be handled by the `error` callback. Fixes #15624 Closes #15628

Previously, errors thrown inside the `success` callback would be swallowed by a noop `catch()` handler. The `catch()` handler was added in order to avoid an unnecessary "Possibly Unhandled Rejection" error, in case the user provided an `error` callback (which would handle request errors). The handler was added too "eagrly" and as a result would swallow errors thrown in the `success` callback, despite the fact that those errors would _not_ be handled by the `error` callback. This commit fixes this, by adding the `catch()` handler "lazily", only when it is certain that a rejection will be handled by the `error` callback. Fixes angular#15624 Closes angular#15628

fix(ngResource): do not eat exceptions in success callback

982a063

move the promise catch noop call inside of error callback handler so that errors inside of the success callback are not eaten added two tests to prove functionality No breaking changes

googlebot added the cla: yes label Jan 19, 2017

kylewuolle mentioned this pull request Jan 19, 2017

angular 1.6.1 - $resource: no exception in console #15624

Closed

gkalpak added component: ngResource frequency: low severity: confusing type: bug labels Jan 26, 2017

gkalpak added this to the Backlog milestone Jan 26, 2017

gkalpak suggested changes Jan 26, 2017

View reviewed changes

kylewuolle added 2 commits February 3, 2017 09:20

* fix(ngResource): moved the unit tests to the proper location and ad…

c94d3d7

…ded two more cases to cover all the scenarios. No breaking changes

* fix(ngResource): fix up the linting issues

2c7280d

gkalpak suggested changes Feb 4, 2017

View reviewed changes

fix(ngResource): clean up some test descriptions and change response …

d7520b3

…interceptors to non rejecting ones

gkalpak approved these changes Feb 4, 2017

View reviewed changes

gkalpak closed this in edfb691 Feb 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngResource): do not eat exceptions in success callback #15628

fix(ngResource): do not eat exceptions in success callback #15628

kylewuolle commented Jan 19, 2017 •

edited by gkalpak

Loading

gkalpak left a comment

gkalpak Jan 26, 2017

kylewuolle Feb 3, 2017

kylewuolle commented Feb 3, 2017

gkalpak Feb 4, 2017

gkalpak Feb 4, 2017

gkalpak Feb 4, 2017

gkalpak Feb 4, 2017

kylewuolle commented Feb 4, 2017

gkalpak commented Feb 4, 2017

fix(ngResource): do not eat exceptions in success callback #15628

fix(ngResource): do not eat exceptions in success callback #15628

Conversation

kylewuolle commented Jan 19, 2017 • edited by gkalpak Loading

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Jan 26, 2017

Choose a reason for hiding this comment

kylewuolle Feb 3, 2017

Choose a reason for hiding this comment

kylewuolle commented Feb 3, 2017

gkalpak Feb 4, 2017

Choose a reason for hiding this comment

gkalpak Feb 4, 2017

Choose a reason for hiding this comment

gkalpak Feb 4, 2017

Choose a reason for hiding this comment

gkalpak Feb 4, 2017

Choose a reason for hiding this comment

kylewuolle commented Feb 4, 2017

gkalpak commented Feb 4, 2017

kylewuolle commented Jan 19, 2017 •

edited by gkalpak

Loading