JSONP requests with the same callback name

[ricardohbin]

Instead of use JSON_CALLBACK to jsonp requests (and angular.callbacks_X callbacks), using JSON_SINGLE_CALLBACK to use always the same callback name (useful to caching)

[petebacondarwin]

Hi Ricardo,
Before you can get a pull request merged there are a few things you need to do. Check out the contributors document here: http://docs.angularjs.org/misc/contribute.
In particular you need to sign the CLA agreement (if you haven't already), write unit tests for your change and format your commit message appropriately.
Pete

[ricardohbin]

Thanks for the advices @petebacondarwin , i didn't know that Git Commit Messages Convections doc.

I think its all right now. :)

PS: I already signed the CLA.

[petebacondarwin]

Hi Ricardo,
Thanks for doing this. Git can be a pain!

Normally you should rebase your commits off the master so that they can be fast-forward merged and also you have written these commits on top of the old ones and accidentally merged in another branch to this one.

I think, also, that you should put your tests into the "feature" commit and not have a separate commit for them. "test" commits are only for adding missing tests when you are not adding anything else.

I suspect, though, that Igor will be able to cherry-pick/squash the two commits onto master when he gets to this PR. I have added it to the Triaged milestone.

Now, looking at the code again, I have to add that I am a little concerned about this change. I assume that the callbacks counter is to provide a level of security? @IgorMinar is this the case? If so then you are undermining the security by making this change.

[ricardohbin]

Hi @petebacondarwin,

Agree with your git points. If Igor can squash it to a better format, it would be nice.

But back to the code: I made this change to compatibility with caching requests.

I had problems with angular callbacks and varnish because its counter always make the requests hits the backend.

With JSON_SINGLE_CALLBACK, a jsonp request will be cached (you can even make other cache techniques, like expires, etc).

About the counter, I think it is just an angular's internal control to handle parallel jsonps requests, to avoid one callback overwrite others.

[petebacondarwin]

OK, I will leave this in the hands of the core team. They should look at
it in the next few days.
Pete

On 11 November 2012 23:40, Ricardo Bin [email protected] wrote:

Hi @petebacondarwin https://github.com/petebacondarwin,

Agree with your git points. If Igor can squash it to a better format, it
would be nice.

But back to the code: I made this change to compatibility with caching
requests.

I had problems with angular callbacks and varnish because its counter
always make the requests hits the backend.

With JSON_SINGLE_CALLBACK, a jsonp request will be cached (you can even
make other cache techniques, like expires, etc).

About the counter, I think it is just an angular's internal control to
handle parallel jsonps requests, to avoid one callback overwrite others.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1548#issuecomment-10274111.

[pkozlowski-opensource]

@ricardohbin Ricardo, just looking into the JSONP-related stuff in AngularJS and had a second look at your PR and realized one thing: introducing the "single" callback would mean that AngularJS would "remember" only one single callback for requests done with the JSON_SINGLE_CALLBACK.

I think that it might be very problematic in the async world: what would happen if you would do more than one request with the JSON_SINGLE_CALLBACK option and supply different callbacks. I think that in practice it could happen quite easily, especially that you would have one and only one callback for all the URLs if I'm not mistaken.

So, with your change people would have to know that only one request with the JSON_SINGLE_CALLBACK could be done at a time. Otherwise callbacks would be mixed / wrong callbacks called. I would say that this might be really confusing to users.

@ricardohbin Do you agree with the above reasoning? It might be that I'm badly interpreting your code change but if I'm not I would be concerned with this change as it could be confusing.

[ricardohbin]

@pkozlowski-opensource Pawel, you are absolutely right and this is an expected behavior when i made this PR.

When use JSON_SINGLE_CALLBACK you must use it to one single request at a time, otherwise the callbacks will overwrite each other.

[pkozlowski-opensource]

@ricardohbin thnx for clarifying.

In this case I'm afraid that I'm not in favor of merging this PR as-is. It is just way to easy to get unexpected behavior of callbacks overriding each other.

Maybe we should explore a different approach, where people could specify a callback name instead of relaying on the auto-generated one? This would also solve #1551

WDYT?

[ricardohbin]

@pkozlowski-opensource sounds nice.

What if we make a kind of convention like "JSON_CALLBACK:foo"?

[mhevery]

Hi Ricardo,

I see the value of having URLs, which are cacheable, so I think you are solving a good goal, unfortunately your current implementation has a serious flaw, which prevents us from merging it.

The issue is that if you make two jsonp requests with 'single' mode their callback functions would get overridden, which would crate inconsistant behavior.

A better approach would be to generate a SHA from a URL, and use that as the SHA name. That way different URLs have different, but stable callback functions. But event with this two JSONP requests to the same URL, would cause a collision, which would need to be resolved in some way.

I am going to close the PR for now, as we are trying to clean up the list. But once you have a fix for this issue please reopen it.

[cm325]

Having two or more requests in single mode is a problem with this patch. As a temporary way to take advantage of this, without going down the SHA route (which would be great), we can mimic that by providing the defaultParams with an "@callback" and provide the callback name from a parameter, like:

  SiteImage.query({
              find:"byMostRecentAndId",
              siteId:siteDescriptions[i].locationId,
              callback:"JSON_CALLBACK(siteimage" + siteDescriptions[i].locationId + ")"
            }, etc

as long as the params are unique, then you don't get caching collisions-

[ricardohbin]

@cm325 updated to #3073

[cm325]

@ricardohbin thanks-