angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.85k stars 27.52k forks source link

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

Closed tkrotoff closed 10 years ago

tkrotoff commented 10 years ago

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.

caitp commented 10 years ago

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 commented 10 years ago

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.

caitp commented 10 years ago

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