Skip to main content

Bug Tracker

Side navigation

#8979 closed bug (invalid)

Opened April 26, 2011 05:31PM UTC

Closed April 26, 2011 05:34PM UTC

Last modified April 26, 2011 10:30PM UTC

ajax 'data' arg stomped on by jQuery.extend()

Reported by: danielh@nffs.com Owned by:
Priority: low Milestone: 1.next
Component: core Version: 1.5.2
Keywords: Cc: jaubourg
Blocked by: Blocking:
Description

jQuery.ajax() merges the passed-in arguments with defaults previously configured via ajaxSetup(). This uses jQuery.extend() to merge the settings. This can be a problem for the data setting.

data may be an object ( { foo: 1, bar: 2 } ), or a string ("foo=1&bar=2"). Merging with extend() works properly if all of the data args are objects, but doesn't have the same behavior if any of the data args are strings.

For example:


 $.extend( true, { data: {foo:1} }, { data: {bar:2} } );
 // { data: {foo:1, bar:2} } - GOOD

 $.extend( true, { data: "foo=1" }, { data: "bar=2" } );
 // { data: "bar=2" } - BAD: foo is lost

Attachments (0)
Change History (5)

Changed April 26, 2011 05:34PM UTC by rwaldron comment:1

component: unfiledcore
priority: undecidedlow
resolution: → invalid
status: newclosed

This is the exact, 100% correct behavior of $.extend

Changed April 26, 2011 05:47PM UTC by danielh@nffs.com comment:2

Replying to [comment:1 rwaldron]:

This is the exact, 100% correct behavior of $.extend

I'm sorry, perhaps I wasn't clear. I'm not suggesting that extend is misbehaving. I agree that the extend behavior is correct. What I am suggesting is that because of this behavior, extend, on its own, is not suitable for merging the ajax data setting.

The bug is that data is allowed to be an object or a string, and this is clearly documented, but that the behavior is different in each case, which is not documented.

Consider this example:


$.ajaxSetup( { data: { foo: 1 } } )

// These requests should be identical, but they are not
$('#id1').load( url, {bar:2}, callback )
$('#id2').load( url, "bar=2", callback )

I don't see any way to attach a file (sorry if I'm missing something). Here is a proposed patch that solves the problem.


diff --git a/jquery-1.5.2.js b/jquery-1.5.2.js
--- a/jquery-1.5.2.js
+++ b/jquery-1.5.2.js
@@ -6383,14 +6383,31 @@
        // with both ajaxSettings and settings fields.
        // If target is omitted, writes into ajaxSettings.
        ajaxSetup: function ( target, settings ) {
+               var data_args = [];
                if ( !settings ) {
                        // Only one parameter, we extend ajaxSettings
                        settings = target;
                        target = jQuery.extend( true, jQuery.ajaxSettings, settings );
+                       data_args.push( settings.data, jQuery.ajaxSettings.data );
                } else {
                        // target was provided, we extend into it
                        jQuery.extend( true, target, jQuery.ajaxSettings, settings );
-               }
+                       data_args.push( settings.data, jQuery.ajaxSettings.data, target.data );
+               }
+
+               // Make sure already-serialized data gets merged correctly
+               if ( target.processData ) {
+                       var data_strings = jQuery.map( data_args, function( data, index ) {
+                               if ( data == null )
+                                       return null;
+                               else if ( typeof data === "string" )
+                                       return data;
+                               else
+                                       return jQuery.param( data, target.traditional );
+                       });
+                       target.data = data_strings.join( '&' )
+               }
+
                // Flatten fields we don't want deep extended
                for( var field in { context: 1, url: 1 } ) {
                        if ( field in settings ) {
@@ -6712,11 +6729,6 @@
                        );
                }
 
-               // Convert data if not already a string
-               if ( s.data && s.processData && typeof s.data !== "string" ) {
-                       s.data = jQuery.param( s.data, s.traditional );
-               }
-
                // Apply prefilters
                inspectPrefiltersOrTransports( prefilters, s, options, jqXHR );

Changed April 26, 2011 05:57PM UTC by timmywil comment:3

I don't think that's something we want to change. Data is correctly overwritten by the second.

Why not just do:

$.extend({}, { data: "foo=1"+"bar=2" });

or some equivalent.

Changed April 26, 2011 10:29PM UTC by jaubourg comment:4

Replying to [comment:2 danielh@…]:

Replying to [comment:1 rwaldron]: > This is the exact, 100% correct behavior of $.extend > I'm sorry, perhaps I wasn't clear. I'm not suggesting that extend is misbehaving. I agree that the extend behavior is correct. What I am suggesting is that because of this behavior, extend, on its own, is not suitable for merging the ajax data setting. The bug is that data is allowed to be an object or a string, and this is clearly documented, but that the behavior is different in each case, which is not documented. Consider this example:
> 
> $.ajaxSetup( { data: { foo: 1 } } )
> 
> // These requests should be identical, but they are not
> $('#id1').load( url, {bar:2}, callback )
> $('#id2').load( url, "bar=2", callback )
> 
> 
I don't see any way to attach a file (sorry if I'm missing something). Here is a proposed patch that solves the problem.
> 
> diff --git a/jquery-1.5.2.js b/jquery-1.5.2.js
> --- a/jquery-1.5.2.js
> +++ b/jquery-1.5.2.js
> @@ -6383,14 +6383,31 @@
>         // with both ajaxSettings and settings fields.
>         // If target is omitted, writes into ajaxSettings.
>         ajaxSetup: function ( target, settings ) {
> +               var data_args = [];
>                 if ( !settings ) {
>                         // Only one parameter, we extend ajaxSettings
>                         settings = target;
>                         target = jQuery.extend( true, jQuery.ajaxSettings, settings );
> +                       data_args.push( settings.data, jQuery.ajaxSettings.data );
>                 } else {
>                         // target was provided, we extend into it
>                         jQuery.extend( true, target, jQuery.ajaxSettings, settings );
> -               }
> +                       data_args.push( settings.data, jQuery.ajaxSettings.data, target.data );
> +               }
> +
> +               // Make sure already-serialized data gets merged correctly
> +               if ( target.processData ) {
> +                       var data_strings = jQuery.map( data_args, function( data, index ) {
> +                               if ( data == null )
> +                                       return null;
> +                               else if ( typeof data === "string" )
> +                                       return data;
> +                               else
> +                                       return jQuery.param( data, target.traditional );
> +                       });
> +                       target.data = data_strings.join( '&' )
> +               }
> +
>                 // Flatten fields we don't want deep extended
>                 for( var field in { context: 1, url: 1 } ) {
>                         if ( field in settings ) {
> @@ -6712,11 +6729,6 @@
>                         );
>                 }
>  
> -               // Convert data if not already a string
> -               if ( s.data && s.processData && typeof s.data !== "string" ) {
> -                       s.data = jQuery.param( s.data, s.traditional );
> -               }
> -
>                 // Apply prefilters
>                 inspectPrefiltersOrTransports( prefilters, s, options, jqXHR );
> 
> 

Your code assumes the data is url encoded. What if it's a json or xml document?

Truth is you can't know for sure what the intent is when data is given as string and serializing like you do is not a solution and will actually break existing applications.

I like the overall idea, mind you, but I'm unsure of its merits on a generality point of view.

Changed April 26, 2011 10:30PM UTC by jaubourg comment:5

cc: → jaubourg