Skip to main content

Bug Tracker

Side navigation

#10202 closed bug (invalid)

Opened September 04, 2011 09:41PM UTC

Closed October 12, 2011 12:47AM UTC

Last modified March 14, 2012 09:22AM UTC

`data` attribute disappears from settings for ajax GET requests

Reported by: steve@alfajango.com Owned by:
Priority: low Milestone: 1.next
Component: ajax Version: 1.6.3
Keywords: Cc: jaubourg
Blocked by: Blocking:
Description

This is specifically for v1.6.3, everything worked prior to that, from at least 1.4 onward.

If you create an ajax request with jQuery.ajax(type: 'GET', data: someData), and then bind handler to the beforeSubmit event, the data attribute disappears from the settings object. This only happens for GET requests in 1.6.3. The data attribute does not disappear for POST requests, or for any request types in 1.6.2 and earlier.

I don't know if this was intensional or not (since GET requests also happen to serialize the data attribute into the URL), but I didn't see it in the changelog, and it messed up the jquery-ujs in Rails.

See http://jsfiddle.net/HnVjX/1/

If you change the JS in the above example to be a non-GET type it works, or alternatively if you change it to jquery 1.6.2, it works.

Attachments (0)
Change History (18)

Changed September 05, 2011 12:35AM UTC by rwaldron comment:1

component: unfiledajax
priority: undecidedlow
resolution: → worksforme
status: newclosed

Changed September 05, 2011 12:54AM UTC by anonymous comment:2

Please read the ticket description. You changed the type to "POST". From the original ticket description:

This only happens for GET requests in 1.6.3.

Please re-open this ticket. It is not fixed, and it broke our well-tested jquery-ujs library in core rails.

Changed September 05, 2011 01:02AM UTC by rwaldron comment:3

resolution: worksforme
status: closedreopened

Whoops! Thanks for noting and thanks for your patience and persistence - greatly appreciated.

Changed September 05, 2011 01:06AM UTC by steve@alfajango.com comment:4

No prob, I know how it goes ;-) If I can get some time, and if nobody else can get to it, I could probably come up with a test case and fix in the next week or so.

Changed September 05, 2011 01:14AM UTC by rwaldron comment:5

cc: → jaubourg

I'll cc jaubourg on this, maybe he has some insight, in the meantime, yes a test case would be awesome.

Changed September 05, 2011 10:42AM UTC by jaubourg comment:6

http://bugs.jquery.com/ticket/9682

So, yes, data is removed on purpose for requests with no body.

Changed September 05, 2011 02:26PM UTC by steve@alfajango.com comment:7

Hmm, so it looks like data was purposely removed from the settings object, so that if you retry the ajax request with $.ajax(this), it won't re-append the data to the url.

I can definitely see that use-case, but it seems at least equally likely (if not more) that we have some handler bound to *any* of the event hooks that needs access to settings.data. I'm not sure how breaking one situation to fix the other is a good solution.

Also, in order to workaround the bug in that other scenario, all we had to do is: delete this.data; $.ajax(this). But now, in order to workaround *this* bug, and access the data, we need an entire script to deserialize the url?

Changed September 05, 2011 02:30PM UTC by steve@alfajango.com comment:8

For that other bug, if we're going to alter the settings object anyway, wouldn't have made way more sense to change the processData attribute to false rather than delete the data attribute?

Changed September 05, 2011 02:33PM UTC by Steve Schwartz <steve@alfajango.com> comment:9

Replying to [comment:8 steve@…]:

For that other bug, if we're going to alter the settings object anyway, wouldn't have made way more sense to change the processData attribute to false rather than delete the data attribute?

Nope, nevermind, that just affects translating the data from an array to a string, it doesn't actually change whether or not it gets appended to the query string.

Changed September 21, 2011 12:32AM UTC by timmywil comment:10

milestone: None1.next
status: reopenedopen

I'll mark this as a valid regression.

Changed September 21, 2011 07:42AM UTC by jaubourg comment:11

Replying to [comment:10 timmywil]:

I'll mark this as a valid regression.

I completly disagree. This is not a regression.

{
    url: "mypath",
    data: {
        a: 1
    }
}

and

{
    url: "mypath?a=1"
}

are stricly equivalent from an ajax point of view.

If one need to keep track of the original data then:

1. use the original options map (never changed)

2. create a prefilter that "saves" the data in the complete options map

We're just paying the price for giving access to the internal options map in the callbacks, knowing that the exact behaviour of such and such options in that scenario has never ever been documented.

Changed September 21, 2011 02:20PM UTC by Steve Schwartz <steve@alfajango.com> comment:12

It seems easy to avoid this problem in the first place. Right now, jQuery ''always'' appends the serialized data attribute to url. So, to fix that other ticket, jQuery was modified to remove the data attribute after the first time through, so that there would not be any data left to append on subsequent attempts.

Rather than deleting the data attribute (which seems destructive), why not just check to see if it's already been done?

// If data is available, append data to url
if ( s.data && s.url.indexOf(s.data) === -1 ) {
	s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.data;
}

Changed September 21, 2011 02:54PM UTC by jaubourg comment:13

Replying to [comment:12 Steve Schwartz <steve@…>]:

It seems easy to avoid this problem in the first place. Right now, jQuery ''always'' appends the serialized data attribute to url. So, to fix that other ticket, jQuery was modified to remove the data attribute after the first time through, so that there would not be any data left to append on subsequent attempts. Rather than deleting the data attribute (which seems destructive), why not just check to see if it's already been done?
> // If data is available, append data to url
> if ( s.data && s.url.indexOf(s.data) === -1 ) {
> 	s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.data;
> }
> 

Because I could very well do something like this:

$.ajax({
    url: "myPath?hello=world",
    data: {
        field: value
    }
});

Changed September 21, 2011 04:07PM UTC by Steve Schwartz <steve@alfajango.com> comment:14

I don't understand. That would result in url: myPath?hello=world&field=value the first time around, and then it wouldn't change on subsequent tries. This is exactly as jQuery currently does, except that the code I posted doesn't delete the data attribute.

Changed October 11, 2011 10:55AM UTC by niyazpk comment:15

_comment0: Replying to [comment:14 Steve Schwartz <steve@…>]: \ > I don't understand. That would result in `url: myPath?hello=world&field=value` the first time around, and then it wouldn't change on subsequent tries. This is exactly as jQuery currently does, except that the code I posted doesn't delete the `data` attribute. \ \ What about: \ \ \ {{{ \ $.ajax({ \ url: "myPath?hello=world", \ data: { \ ello: worl \ } \ }); \ }}} \ \ 1318330547330610

Replying to [comment:14 Steve Schwartz <steve@…>]:

I don't understand. That would result in url: myPath?hello=world&field=value the first time around, and then it wouldn't change on subsequent tries. This is exactly as jQuery currently does, except that the code I posted doesn't delete the data attribute.

What about:

$.ajax({
    url: "myPath?hello=world",
    data: {
        ello: worl
    }
});

?

Changed October 11, 2011 08:32PM UTC by Steve Schwartz <steve@alfajango.com> comment:16

Ah, that's a good point. Replace my original code example with this:

var dataAppendedToUrl = (new RegExp("(&|\\\\?)" + s.data + "(&|$)")).test(s.url);
if ( s.data && !dataAppendedToUrl ) {
	s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.data;
}

I have this coded up, complete with test, and all tests passing. See pull request 544: https://github.com/jquery/jquery/pull/544.

Changed October 12, 2011 12:47AM UTC by jaubourg comment:17

resolution: → invalid
status: openclosed

This is not a regression. Rationale and possible adaptation to the now correct behaviour ajax exhibits are given in http://bugs.jquery.com/ticket/10202#comment:11 (I'd go with a prefilter personally).

Changed October 12, 2011 01:55AM UTC by Steve Schwartz <steve@alfajango.com> comment:18

I posted this on the github ticket, but I'll repost here, in response to comment 11 and why {p: 6} and "p=6" are not technically equivalent, and thus why this is still needed.

I don't think it's quite as simple as saying they're equivalent and thus it's not a regression. The problem is this real-world scenario: we have a handler that does something with xhr.data, expecting the object that we *just* passed to .ajax() in the first place. This has been the case since forever, up until 1.6.3. Do {p: 6} and "p=6" contain the same data? Yes. However, jQuery makes it incredibly easy to convert the former to the latter, but *no way* to convert the latter to the former. We'd have to use our own function to convert back (I've done this and it's about 30 lines of code).

>

Since conversion process between the two forms ({p: 6} and "p=6") is trivial one way and difficult the other way, then they're not equivalent in practice.

>

If adding 30 lines of code to every plugin and app that needs this can be easily avoided by changing one already-existing line of code in here, why not do it? I understand not wanting to use this solution, rightfully so; but there are a number of other ways to go about it, and I'm more than willing to figure it out.