Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

$http.jsonp fails when the response body is empty #4987

Closed
tkrotoff opened this issue Nov 17, 2013 · 3 comments
Closed

$http.jsonp fails when the response body is empty #4987

tkrotoff opened this issue Nov 17, 2013 · 3 comments

Comments

@tkrotoff
Copy link

If the response to a JSONP request is empty, AngularJS $http.jsonp thinks an error occurred (version 1.2.1 tested).

jQuery $.ajax works fine in this case.

Investigation

This is due to the following lines inside function createHttpBackend() (see comments inside code snippet):

var jsonpDone = jsonpReq(url.replace('JSON_CALLBACK', 'angular.callbacks.' + callbackId),
    function() {
  if (callbacks[callbackId].data) {
    completeRequest(callback, 200, callbacks[callbackId].data);
  } else {
    // callbacks[callbackId].data is undefined thus the if test fails
    completeRequest(callback, status || -2);
  }
  delete callbacks[callbackId];
});

Later on transformResponse() is performed:

function transformResponse(response) {
  var resp = extend({}, response, {
    data: transformData(response.data, response.headers, config.transformResponse)
  });
  return (isSuccess(response.status))
    ? resp
    : $q.reject(resp);
}

And isSuccess() checks for the HTTP status code:

function isSuccess(status) {
  return 200 <= status && status < 300;
} 

And of course returns false since status value is 0.

What jQuery does?

https://github.com/jquery/jquery/blob/2.0.3/src/ajax/script.js#L28
See comments inside code snippet

jQuery.ajaxTransport( "script", function( s ) {
    if ( s.crossDomain ) {
        var script, callback;
        return {
            send: function( _, complete ) {
                script = jQuery("<script>").prop({
                    async: true,
                    charset: s.scriptCharset,
                    src: s.url
                }).on(
                    "load error",
                    callback = function( evt ) {
                        script.remove();
                        callback = null;
                        if ( evt ) {
                            complete( evt.type === "error" ? 404 : 200, evt.type );
                            // Whatever the HTTP response is, HTTP status is always 200 OK
                            // FYI in our case evt.type value is "load"
                        }
                    }
                );
                document.head.appendChild( script[ 0 ] );
            },
            abort: function() {
                if ( callback ) {
                    callback();
                }
            }
        };
    }
});

(btw jQuery coding conventions are really ugly, spaces everywhere WTF, that's why AngularJS is so much better :) )

What should be done instead?

One cannot get JSONP responses HTTP status code, thus whenever a JSONP response is received, HTTP status code should be 200.

Considering this, the following if test from createHttpBackend() is useless in the case of JSONP:

if (callbacks[callbackId].data) {
  completeRequest(callback, 200, callbacks[callbackId].data);
} else {
  completeRequest(callback, status || -2);
}

and should be instead:

  completeRequest(callback, 200, callbacks[callbackId].data);

Why an empty JSONP response?

Google OAuth2 implementation allows to revoke a token by calling https://accounts.google.com/o/oauth2/revoke?token=0001
See https://developers.google.com/accounts/docs/OAuth2WebServer#tokenrevoke
See https://developers.google.com/+/web/signin/#revoking_access_tokens_and_disconnecting_the_app

Google does not allow CORS to perform this call, it must be JSONP.

If the call succeed and the token is valid, /oauth2/revoke returns HTTP 200 OK without any data inside the HTTP body response.

Example/Plunker

I've written an example to test with Google OAuth2: http://plnkr.co/edit/1fl7zi?p=preview

Unfortunately, a OAuth2 token expires and when playing with this example you need to generate your own token.
I don't have a public server of my own that returns JSONP responses.

I've also tried to implement a Jasmine test using $httpBackend to reproduce the case without success.

@ghost ghost assigned caitp Jan 6, 2014
@caitp
Copy link
Contributor

caitp commented Jan 8, 2014

Hmm, the jQuery code you've posted only registers onload and onerror event handlers... Do they have some magic for dealing with older IE browsers somewhere?

I don't think it's quite right to treat all of these responses as success (although you're right, it may be possible to not get a response at all), which is what we'd end up doing.

With the current strategy, with browsers that do support onload/onerror, we don't actually know if we've got an error or not (using jqlite events would make this easier to sort out), but it still seems to be impossible to know for IE8 and under. But yes, an empty payload should not be the same as an error.

Hmm.

@tkrotoff
Copy link
Author

tkrotoff commented Mar 6, 2014

the jQuery code you've posted only registers onload and onerror event handlers... Do they have some magic for dealing with older IE browsers somewhere?

@caitp The "magic" is named jQuery < 2 :-)
The copy-pasted code above is from jQuery 2.0.3 which does not support old IE :)

Here the interesting part from jQuery 1.11.0: https://github.com/jquery/jquery/blob/1.11.0/src/ajax/script.js#L34

When I wrote the bug report, I've only experiment with modern browsers, and Plunker (where the demo is hosted) does not even support IE < 9.

@btford btford modified the milestones: 1.3.0-beta.2, 1.3.0-beta.1 Mar 10, 2014
@tbosch tbosch modified the milestones: 1.3.0-beta.3, 1.3.0-beta.2 Mar 14, 2014
@caitp
Copy link
Contributor

caitp commented Mar 18, 2014

Well, since this is in the 1.3 milestone now, IE8 is not really a priority here. Still need to figure out how to make this work nicely though

caitp added a commit to caitp/angular.js that referenced this issue Mar 18, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
caitp added a commit to caitp/angular.js that referenced this issue Mar 18, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
caitp added a commit to caitp/angular.js that referenced this issue Mar 18, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
caitp added a commit to caitp/angular.js that referenced this issue Mar 19, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
caitp added a commit to caitp/angular.js that referenced this issue Mar 19, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
caitp added a commit to caitp/angular.js that referenced this issue Mar 19, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
caitp added a commit to caitp/angular.js that referenced this issue Mar 19, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
caitp added a commit to caitp/angular.js that referenced this issue Mar 19, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
@caitp caitp closed this as completed in 6680b7b Mar 19, 2014
caitp added a commit that referenced this issue Mar 21, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes #4987
Closes #6735
caitp added a commit to caitp/angular.js that referenced this issue Apr 7, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
Closes angular#6735
caitp added a commit to caitp/angular.js that referenced this issue Apr 7, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
Closes angular#6735
caitp added a commit to caitp/angular.js that referenced this issue Apr 8, 2014
…ameter

This change brings Angular's JSONP behaviour closer in line with jQuery's. It will no longer treat
a callback called with no data as an error, and will no longer support IE8 via the onreadystatechanged
event.

BREAKING CHANGE:

Previously, the JSONP backend code would support IE8 by relying on the readystatechanged events. This
is no longer the case, as these events do not provide adequate useful information for deeming whether
or not a response is an error.

Previously, a JSONP response which did not pass data into the callback would be given a status of -2,
and treated as an error. Now, this situation will instead be given a status of 200, despite the lack
of data. This is useful for interaction with certain APIs.

Previously, the onload and onerror callbacks were added to the JSONP script tag. These have been
replaced with jQuery events, in order to gain access to the event object. This means that it is now
difficult to test if the callbacks are registered or not. This is possible with jQuery, using the
$.data("events") method, however it is currently impossible with jqLite. This is not expected to
break applications.

Closes angular#4987
Closes angular#6735
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants