Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#10202 closed bug (invalid)

`data` attribute disappears from settings for ajax GET requests

Reported by: steve@… 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.

Change History (18)

comment:1 Changed 7 years ago by Rick Waldron

Component: unfiledajax
Priority: undecidedlow
Resolution: worksforme
Status: newclosed

comment:2 Changed 7 years ago by anonymous

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.

comment:3 Changed 7 years ago by Rick Waldron

Resolution: worksforme
Status: closedreopened

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

comment:4 Changed 7 years ago by steve@…

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.

comment:5 Changed 7 years ago by Rick Waldron

Cc: jaubourg added

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

comment:6 Changed 7 years ago by jaubourg

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

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

comment:7 Changed 7 years ago by steve@…

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?

comment:8 Changed 7 years ago by 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?

comment:9 in reply to:  8 Changed 7 years ago by Steve Schwartz <steve@…>

Replying to 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.

comment:10 Changed 7 years ago by timmywil

Milestone: None1.next
Status: reopenedopen

I'll mark this as a valid regression.

comment:11 in reply to:  10 Changed 7 years ago by jaubourg

Replying to 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.

comment:12 Changed 7 years ago by 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;
}

comment:13 in reply to:  12 Changed 7 years ago by jaubourg

Replying to 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
    }
});

comment:14 Changed 7 years ago by 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.

comment:15 in reply to:  14 Changed 7 years ago by Niyaz

Replying to 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
    }
});

?

Last edited 7 years ago by Niyaz (previous) (diff)

comment:16 Changed 7 years ago by Steve Schwartz <steve@…>

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.

comment:17 Changed 7 years ago by jaubourg

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).

comment:18 Changed 7 years ago by Steve Schwartz <steve@…>

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.

Note: See TracTickets for help on using tickets.