New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: param serializes first element of an array without an index #3666

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@odedniv

odedniv commented May 11, 2017

Summary

jQuery.param will serialize the first element of an array without an index, this prevents ambiguity between an object with numeric keys and an array, and will therefor allow properly serializing an array of objects.

Before:

jQuery.param({ a: [{ b: 1 }, { c: 2 }] })
=> a[0][b]=1&a[1][c]=2

This could falsely be deserialized as: { a: { "0": { b: 1 }, "1": { c: 2 } } } (and it absolutely will).

After:

jQuery.param({ a: [{ b: 1 }, { c: 2 }] })
=> a[][b]=1&a[1][c]=2

This first element ensures a should be considered an array.

This follows this PR in rack: rack/rack#1165

Unfortunately this is not backward compatibly (would actually break rack before it pulls this request), but this is somewhat mandatory as you simply cannot properly serialize some scenarios (see the tests in rack's PR for more scenarios). Should this be a configuration option somewhere?

Checklist

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot May 11, 2017

@odedniv, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jaubourg, @markelog and @dmethvin to be potential reviewers.

@odedniv, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jaubourg, @markelog and @dmethvin to be potential reviewers.

@odedniv

This comment has been minimized.

Show comment
Hide comment
@odedniv

odedniv May 11, 2017

PS. I could not run the tests for some reason, they always succeeded even they were supposed to fail.

odedniv commented May 11, 2017

PS. I could not run the tests for some reason, they always succeeded even they were supposed to fail.

@gibson042

I approve of the goal, but shouldn't empty brackets only appear when introducing an array? This fix emits them for every subproperty of the first object: https://jsfiddle.net/215nvo0e/

Output: arr[][p1]=&arr[][p2]=
Expected: arr[][p1]=&arr[0][p2]=

@odedniv

This comment has been minimized.

Show comment
Hide comment
@odedniv

odedniv May 15, 2017

To do that you have to completely change how buildParams work, are you comfortable with that?

If so, do you expect non-object elements to include an index as well? Right now it only includes an index in object elements due to requiring to modify existing elements (which was ambiguous before this fix, as you couldn't know whether it's a new array element or not - and in rack for example you they had to guess according to existing keys in the object).

odedniv commented May 15, 2017

To do that you have to completely change how buildParams work, are you comfortable with that?

If so, do you expect non-object elements to include an index as well? Right now it only includes an index in object elements due to requiring to modify existing elements (which was ambiguous before this fix, as you couldn't know whether it's a new array element or not - and in rack for example you they had to guess according to existing keys in the object).

Core: param serializes first element of an array without an index
This prevents ambiguity between an object with numeric keys and an array.
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 16, 2017

Member

Current released behavior is correct but ambiguous when the first element of an array is not primitive. But the current PR behavior is incorrect when the first element of an array has more than one key, so we cannot accept it as-is.

Member

gibson042 commented May 16, 2017

Current released behavior is correct but ambiguous when the first element of an array is not primitive. But the current PR behavior is incorrect when the first element of an array has more than one key, so we cannot accept it as-is.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 17, 2017

@gibson042 the current PR behavior is not ambiguous, so I do not understand why it's incorrect. Once it's been established that the element is part of an array, and the element itself is an object, you can add more items to that object with [][...].

But my real question is this: Are you comfortable with a rewrite of the function in order to make the second attribute of an object indexed? If you are, I'll get right on it.

ghost commented May 17, 2017

@gibson042 the current PR behavior is not ambiguous, so I do not understand why it's incorrect. Once it's been established that the element is part of an array, and the element itself is an object, you can add more items to that object with [][...].

But my real question is this: Are you comfortable with a rewrite of the function in order to make the second attribute of an object indexed? If you are, I'll get right on it.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 17, 2017

Member

Once it's been established that the element is part of an array, and the element itself is an object, you can add more items to that object with [][...].

This doesn't seem right. a[] is referring to a new element pushed to the array so a[][p1]=2&a[][p2]=3 would produce [{p1: 2}, {p2: 3}] instead of the expected [{p1: 2, p2: 3}]. The latter result could be achieved via a[][p1]=2&a[0][p2]=3.

Member

mgol commented May 17, 2017

Once it's been established that the element is part of an array, and the element itself is an object, you can add more items to that object with [][...].

This doesn't seem right. a[] is referring to a new element pushed to the array so a[][p1]=2&a[][p2]=3 would produce [{p1: 2}, {p2: 3}] instead of the expected [{p1: 2, p2: 3}]. The latter result could be achieved via a[][p1]=2&a[0][p2]=3.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 17, 2017

Member

Are you comfortable with a rewrite of the function in order to make the second attribute of an object indexed? If you are, I'll get right on it.

Yes, provided it does not increase the size of jquery.min.gz by more than 10 bytes. Behavior has been stable for a long time, and in my opinion this edge case isn't worth a size investment.

Member

gibson042 commented May 17, 2017

Are you comfortable with a rewrite of the function in order to make the second attribute of an object indexed? If you are, I'll get right on it.

Yes, provided it does not increase the size of jquery.min.gz by more than 10 bytes. Behavior has been stable for a long time, and in my opinion this edge case isn't worth a size investment.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jun 5, 2017

Member

This seems to have gotten stale and I'm not sure it's worth the effort.

Member

timmywil commented Jun 5, 2017

This seems to have gotten stale and I'm not sure it's worth the effort.

@timmywil timmywil closed this Jun 5, 2017

@mgol mgol removed the Needs review label Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment