Bug Tracker

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#8979 closed bug (invalid)

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

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

Change History (5)

comment:1 Changed 8 years ago by Rick Waldron

Component: unfiledcore
Priority: undecidedlow
Resolution: invalid
Status: newclosed

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

comment:2 in reply to:  1 ; Changed 8 years ago by danielh@…

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

comment:3 Changed 8 years ago by timmywil

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.

comment:4 in reply to:  2 Changed 8 years ago by jaubourg

Replying to danielh@…:

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

comment:5 Changed 8 years ago by jaubourg

Cc: jaubourg added
Note: See TracTickets for help on using tickets.