fix($httpParamSerializerJQLike): call functions as jQuery does #16139

frederikprijck · 2017-07-31T18:09:34Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
Previously, httpParamSerializerJQLike did not handle properties of type function.

What is the new behavior (if this is a feature change)?
This commit ensures function properties are executed and the return value is used.

Does this PR introduce a breaking change?
See: #16138 (comment)

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

gkalpak

There are no tests with functions inside arrays 😞

Can you also change the commit message to make it more clear that previously function were stringified (not ignored). This will make it easier for people understand what differences to expect.

gkalpak · 2017-08-01T09:16:25Z

src/ng/http.js

@@ -122,8 +122,12 @@ function $HttpParamSerializerJQLikeProvider() {
                (topLevel ? '' : ']'));
          });
        } else {
-          parts.push(encodeUriQuery(prefix) + '=' +
-              (toSerialize == null ? '' : encodeUriQuery(serializeValue(toSerialize))));
+          if (isFunction(toSerialize)) {


EDIT: This comment is now obsolete.

Merge with the else above for consistency:

... } else if (isFunction(toSerialize)) { ... } else { ... }

gkalpak · 2017-08-01T09:20:34Z

test/ng/httpSpec.js

+      expect(jqrSer({a: function() { return { b: 'c' }; }, foo: {'bar': 'barv', 'baz': 'bazv'}})).toEqual(
+        'a%5Bb%5D=c&foo%5Bbar%5D=barv&foo%5Bbaz%5D=bazv');
+        //a[b]=c&foo[bar]=barv&foo[baz]=bazv
+    });


These two tests are testing more (independent) things than necessary. They should focus on the function part. The rest is irrelevant.

gkalpak · 2017-08-01T09:21:12Z

test/ng/httpSpec.js

+        //a=b&foo[bar]=barv&foo[baz]=bazv
+    });
+
+    it('should serialize nested objects with function properties returning an object by repeating param name with [key] suffix', function() {


gkalpak · 2017-08-01T09:33:12Z

src/ng/http.js

-          parts.push(encodeUriQuery(prefix) + '=' +
-              (toSerialize == null ? '' : encodeUriQuery(serializeValue(toSerialize))));
+          if (isFunction(toSerialize)) {
+            serialize(toSerialize(), prefix);


Actually, this is not how jQuery does it. They do not recursively serialize a returned object. They just use the returned value.

frederikprijck · 2017-08-01T10:46:50Z

Updated the PR, added a test for a function in an array aswell!

gkalpak

Please add some test for array inside object (e.g. {foo: {bar: [valueFn('baz')]}}).

gkalpak · 2017-08-02T09:08:34Z

src/ng/http.js

        } else {
          parts.push(encodeUriQuery(prefix) + '=' +
-              (toSerialize == null ? '' : encodeUriQuery(serializeValue(toSerialize))));
+                (toSerialize == null ? '' : encodeUriQuery(serializeValue(toSerialize))));


Wrong indentation 😁

Actually, you can avoid code duplication with:

... } else { if (isFunction(toSerialize)) { toSerialize = toSerialize(); } parts.push(encodeUriQuery(prefix) + '=' + (toSerialize == null ? '' : encodeUriQuery(serializeValue(toSerialize)))); }

gkalpak · 2017-08-02T09:12:18Z

test/ng/httpSpec.js

@@ -2364,17 +2364,45 @@ describe('$http param serializers', function() {
      expect(decodeURIComponent(jqrSer({a: 'b', foo: ['bar', 'baz']}))).toEqual('a=b&foo[]=bar&foo[]=baz');
    });

+    it('should serialize arrays with functions', function() {
+      expect(jqrSer({foo:[function() { return 'bar'; }]})).toEqual('foo%5B%5D=bar');


Missing space after :. 😁
BTW, you an probably use valueFn('bar') to save some typing 😎

gkalpak · 2017-08-02T09:12:59Z

test/ng/httpSpec.js

    it('should serialize objects by repeating param name with [key] suffix', function() {
      expect(jqrSer({a: 'b', foo: {'bar': 'barv', 'baz': 'bazv'}})).toEqual('a=b&foo%5Bbar%5D=barv&foo%5Bbaz%5D=bazv');
                                                                           //a=b&foo[bar]=barv&foo[baz]=bazv
    });

+    it('should serialize objects with function properties by repeating param name with [key] suffix', function() {
+      expect(jqrSer({a: function() { return 'b'; }})).toEqual(
+        'a=b');


Remove the "by repeating param name with [key] suffix" part, which is irrelevant (here and below).
Weird line-splitting (here and below) 😕

gkalpak · 2017-08-02T09:14:07Z

test/ng/httpSpec.js

+    it('should serialize objects with function properties returning an object by repeating param name with [key] suffix', function() {
+      expect(jqrSer({a: function() { return { b: 'c' }; }})).toEqual(
+        'a=%7B%22b%22:%22c%22%7D');
+        //a={"b": "c"}


There is no space after :.

gkalpak · 2017-08-02T09:15:31Z

test/ng/httpSpec.js

+
+    it('should serialize objects with function properties returning an object by repeating param name with [key] suffix', function() {
+      expect(jqrSer({a: function() { return { b: 'c' }; }})).toEqual(
+        'a=%7B%22b%22:%22c%22%7D');


This is not exactly how jQuery does it, but that is not related to this PR 😁

gkalpak · 2017-08-02T09:16:01Z

test/ng/httpSpec.js

+    it('should serialize nested objects with function properties returning an object by repeating param name with [key] suffix', function() {
+      expect(jqrSer({foo: {'bar': function() { return { bav: 'barv' }; }}})).toEqual(
+        'foo%5Bbar%5D=%7B%22bav%22:%22barv%22%7D');
+        //foo[bar]={"bav": "barv"}


No space after :.

gkalpak · 2017-08-02T09:17:03Z

test/ng/httpSpec.js

    it('should serialize nested objects by repeating param name with [key] suffix', function() {
      expect(jqrSer({a: ['b', {c: 'd'}], e: {f: 'g', 'h': ['i', 'j']}})).toEqual(
         'a%5B%5D=b&a%5B1%5D%5Bc%5D=d&e%5Bf%5D=g&e%5Bh%5D%5B%5D=i&e%5Bh%5D%5B%5D=j');
         //a[]=b&a[1][c]=d&e[f]=g&e[h][]=i&e[h][]=j
    });

+    it('should serialize nested objects with function properties by repeating param name with [key] suffix', function() {
+      expect(jqrSer({foo: {'bar': function() { return 'barv'; }}})).toEqual(


No need for quoting bar (here and below).

frederikprijck · 2017-08-02T10:29:40Z

Updated!

gkalpak · 2017-08-02T10:29:48Z

src/ng/http.js

          parts.push(encodeUriQuery(prefix) + '=' +
-                (toSerialize == null ? '' : encodeUriQuery(serializeValue(toSerialize))));
+            (toSerialize == null ? '' : encodeUriQuery(serializeValue(toSerialize))));


Wrong indentation. 😞

gkalpak · 2017-08-02T10:30:51Z

test/ng/httpSpec.js

-    it('should serialize nested objects with function properties by repeating param name with [key] suffix', function() {
-      expect(jqrSer({foo: {'bar': function() { return 'barv'; }}})).toEqual(
+    it('should serialize nested objects with function properties', function() {
+      expect(jqrSer({foo: {bar: valueFn('barv')}})).toEqual(
        'foo%5Bbar%5D=barv');


Weird line break (here and below) 😕

gkalpak · 2017-08-02T10:33:03Z

test/ng/httpSpec.js

-    it('should serialize nested objects with function properties returning an object by repeating param name with [key] suffix', function() {
-      expect(jqrSer({foo: {'bar': function() { return { bav: 'barv' }; }}})).toEqual(
+    it('should serialize nested objects with function properties returning an object', function() {
+      expect(jqrSer({foo: {bar: valueFn({ bav: 'barv' })}})).toEqual(


Inconsistent whitespace after { and before } 😁

Previously, `httpParamSerializerJQLike` stringified function properties without executing them. This commit ensures function properties are executed and the return value is used. Closes #16138

gkalpak · 2017-08-03T09:37:32Z

test/ng/httpSpec.js

@@ -2364,17 +2364,41 @@ describe('$http param serializers', function() {
      expect(decodeURIComponent(jqrSer({a: 'b', foo: ['bar', 'baz']}))).toEqual('a=b&foo[]=bar&foo[]=baz');
    });

+    it('should serialize arrays with functions', function() {
+      expect(jqrSer({foo: [valueFn('bar')]})).toEqual('foo%5B%5D=bar');


Where's the decoded comment (foo[]=bar)? 😁

gkalpak · 2017-08-03T09:38:35Z

test/ng/httpSpec.js

+      expect(jqrSer({a: valueFn('b')})).toEqual('a=b');
+    });
+
+    it('should serialize objects with function properties returning an object by repeating param name with [key] suffix', function() {


Inaccurate description.

gkalpak · 2017-08-03T09:39:25Z

test/ng/httpSpec.js

+    });
+
+    it('should serialize nested objects with function properties returning an object', function() {
+      expect(jqrSer({foo: {bar: valueFn({ bav: 'barv'})}})).toEqual('foo%5Bbar%5D=%7B%22bav%22:%22barv%22%7D'); //foo[bar]={"bav":"barv"}


Extra whitespace in { bav. 😱

gkalpak · 2017-08-03T18:05:45Z

Merging to master only, since it has already deviated from 1.6.x (not converting empty strings to null).

googlebot added the cla: yes label Jul 31, 2017

frederikprijck closed this Jul 31, 2017

frederikprijck reopened this Jul 31, 2017

gkalpak suggested changes Aug 1, 2017

View reviewed changes

gkalpak reviewed Aug 1, 2017

View reviewed changes

gkalpak mentioned this pull request Aug 1, 2017

feat($httpParamSerializerJQLike): honor object.toJSON function if present #12330

Closed

gkalpak suggested changes Aug 2, 2017

View reviewed changes

fix($httpParamSerializerJQLike): call functions as jQuery does

a3d9f94

Previously, `httpParamSerializerJQLike` stringified function properties without executing them. This commit ensures function properties are executed and the return value is used. Closes #16138

frederikprijck closed this Aug 2, 2017

frederikprijck reopened this Aug 2, 2017

gkalpak suggested changes Aug 3, 2017

View reviewed changes

frederikprijck added 2 commits August 3, 2017 12:08

fixup! fix($httpParamSerializerJQLike): call functions as jQuery does

5d561bd

fixup! fix($httpParamSerializerJQLike): call functions as jQuery does

64c0a65

gkalpak approved these changes Aug 3, 2017

View reviewed changes

gkalpak closed this in a784fab Aug 3, 2017

This was referenced Aug 4, 2017

fix($httpParamSerializerJQLike): call functions as jQuery does angular-indonesia/angular.js#58

Merged

fix($httpParamSerializerJQLike): call functions as jQuery does GulajavaMinistudio/angular.js#54

Merged

dependencies bot mentioned this pull request May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($httpParamSerializerJQLike): call functions as jQuery does #16139

fix($httpParamSerializerJQLike): call functions as jQuery does #16139

frederikprijck commented Jul 31, 2017

gkalpak left a comment

gkalpak Aug 1, 2017 •

edited

Loading

gkalpak Aug 1, 2017

gkalpak Aug 1, 2017

gkalpak Aug 1, 2017

frederikprijck commented Aug 1, 2017

gkalpak left a comment

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

frederikprijck commented Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 2, 2017

gkalpak Aug 3, 2017

gkalpak Aug 3, 2017

gkalpak Aug 3, 2017

gkalpak commented Aug 3, 2017

fix($httpParamSerializerJQLike): call functions as jQuery does #16139

fix($httpParamSerializerJQLike): call functions as jQuery does #16139

Conversation

frederikprijck commented Jul 31, 2017

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck commented Aug 1, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Aug 3, 2017

gkalpak Aug 1, 2017 •

edited

Loading