Closed Bug 1074902 Opened 10 years ago Closed 10 years ago

URLSearchParams.set() should remove duplicate pairs

Categories

(Core :: DOM: Core & HTML, defect)

34 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: crimsteam, Assigned: sciarp, Mentored)

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

Per URL spec for set() method:
https://url.spec.whatwg.org/#dom-urlsearchparams-set
"1. If there are any name-value pairs whose name is name, set the value of the first such name-value pair to value and remove the others."

but now Firefox doesn't remove others pair. 

Small test:

<script>

 var USP = new URLSearchParams("name1=value1&name1=value2&name1=value3");
 
 document.write("Before set:<br>" + USP);

 USP.set("name1", "firstPair")
 document.write("<br><br>After set:<br>" + USP);

</script>
Mentor: nsm.nikhil
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=C++]
I would like to take on this bug.

As far as I understand, this bug is related to bug 1074963 which might bring some modifications to the underling data structure. To proceed further I think some clarifications are required.
> I would like to take on this bug.

cool!
Just today I landed a patch that stores the parameters into an nsTArray. So you easily iterate between them with a for loop. Here the function you should change:

https://hg.mozilla.org/integration/mozilla-inbound/file/f5ccdeb36843/dom/base/URLSearchParams.cpp#l260
Attached patch bug1074902.patch (obsolete) — Splinter Review
The attached patch removes duplicate entries on Set() calls.
I think anyway there is a flaw in the specification or the current implementation  needs to be revisited or I am missing something here...
I see an inconsistency between the Set() method and the Constructor().
As from the specification the Get() method returns the first entry found in the "list". Say we initialize a URLSearchParams with an URL as from Comment 0. Why does not the parser remove duplicate entries as well? This could end up in a situation where we could have many name/values pairs (with the same name) in the "list" which cannot be Get(). Slowing down eventually the search...

Example:
u = new URLSearchParams("name1=value1&name1=value2&name1=value3");
is(u.get('name1'), 'value1', "URL.searchParams.get('name1')");
is(u.getAll('name1').length, 3, "URLSearchParams.getAll('name1').length");
IMO, the length should be 1 and not 3.

Of course, if Set() is invoked on the name, then duplicates entries are removed.
Attachment #8501314 - Flags: review?(amarchesini)
At now pairs with the same name are not forbbiden, this applies to URLSearchParams() and a method append(), only set() should remove duplicate. If we have more than one pair with the same name and want get other then we must use getAll(). Maybe Anne should confirm that this is a conscious behavior.
Comment on attachment 8501314 [details] [diff] [review]
bug1074902.patch

Review of attachment 8501314 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
It looks perfect but, please send a new version of this patch following these comments.

::: dom/base/URLSearchParams.cpp
@@ +257,5 @@
>  void
>  URLSearchParams::Set(const nsAString& aName, const nsAString& aValue)
>  {
>    Param* param = nullptr;
> +  for (uint32_t i = 0, len = mSearchParams.Length(); i < len; ) {

no extra space between ';' and ')'

@@ +258,5 @@
>  URLSearchParams::Set(const nsAString& aName, const nsAString& aValue)
>  {
>    Param* param = nullptr;
> +  for (uint32_t i = 0, len = mSearchParams.Length(); i < len; ) {
> +    if ( mSearchParams[i].mKey.Equals( aName ) ) {

if (mSearchParams[i].mKey.Equals(aName)) {

@@ +259,5 @@
>  {
>    Param* param = nullptr;
> +  for (uint32_t i = 0, len = mSearchParams.Length(); i < len; ) {
> +    if ( mSearchParams[i].mKey.Equals( aName ) ) {
> +      if ( !param ) {

if (!param) {

@@ +269,5 @@
> +        mSearchParams.RemoveElementAt(i);
> +        len = mSearchParams.Length();
> +      }
> +    }
> +    else{

} else {

but what about if we do this:

if (!mSearchParams[i].mKey.Equals(aName)) {
  ++i;
  continue;
}

...

::: dom/base/test/test_urlSearchParams.html
@@ +288,5 @@
>      runTest();
>    }
>  
> +  function testSet() {
> +      

extra spaces.

@@ +296,5 @@
> +    u.set('i','d');
> +    u.set('o','f');
> +    u.set('u','g');
> +
> +    is(u.get('a'), 'b', "URL.searchParams.get('a')");

write a better message here and in the other checks.
Attachment #8501314 - Flags: review?(amarchesini) → review+
Attached patch bug1074902.patch (obsolete) — Splinter Review
The new patch includes the proposed modifications. 
Thanks for the "if" suggestion! It does not change the function's behaviour but it does reduce the indentations and improves the readability. Thanks!
Attachment #8501314 - Attachment is obsolete: true
Attachment #8502054 - Flags: review?(amarchesini)
Comment on attachment 8502054 [details] [diff] [review]
bug1074902.patch

Review of attachment 8502054 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Can you push it to try or you want me to do it?

::: dom/base/URLSearchParams.cpp
@@ +270,4 @@
>      }
> +    // Remove duplicates.
> +    mSearchParams.RemoveElementAt(i);
> +    len = mSearchParams.Length();

--len; is enough.
Attachment #8502054 - Flags: review?(amarchesini) → review+
Attached patch bug1074902.patchSplinter Review
I don't have (yet) access to the try-server. It would be nice if you could push it for me. Thanks!
Attachment #8502054 - Attachment is obsolete: true
Attachment #8502603 - Flags: review?(amarchesini)
Comment on attachment 8502603 [details] [diff] [review]
bug1074902.patch

Review of attachment 8502603 [details] [diff] [review]:
-----------------------------------------------------------------

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0302ba93ff4e
Attachment #8502603 - Flags: review?(amarchesini) → review+
Donato, your patch is green on try. You can mark this bug as 'checkin-needed'.
Thanks a lot for your help.
If only the bug was assigned to me...
Flags: needinfo?(nsm.nikhil)
Assignee: nobody → sciarp
Flags: needinfo?(nsm.nikhil)
Keywords: checkin-needed
OS: Windows 7 → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/e5bc95c2b993
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.