Closed
Bug 1469533
Opened 7 years ago
Closed 6 years ago
Network body is wrongly formatted
Categories
(DevTools :: Netmonitor, defect, P3)
Tracking
(firefox64 fixed)
RESOLVED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: natim, Assigned: asaba90, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: good-first-bug)
Attachments
(4 files, 4 obsolete files)
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180607190419
Steps to reproduce:
Try a POST request with the following parameters:
{"params":"query=La bûche cheeseburger cheeseburger
filters=language:fr
removeWordsIfNoResults=allOptional
hitsPerPage=10"}
Look at the developer console network panel.
Actual results:
The body shown looks like this:
{"params":"query=La bûche cheeseburger cheeseburger
filters=language:fr
hitsPerPage=10"}
removeWordsIfNoResults=allOptional
Expected results:
It should display the following:
{"params":"query=La bûche cheeseburger cheeseburger
filters=language:fr
removeWordsIfNoResults=allOptional
hitsPerPage=10"}
Updated•7 years ago
|
Component: Untriaged → Netmonitor
Product: Firefox → DevTools
Comment 1•7 years ago
|
||
I can't reproduce the problem on my machine, Win10, testing Fx60 and Fx62 (Nightly), en-US. Attaching screenshot.
Can you please try Firefox Nightly?
https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly
Perhaps it's language dependent bug? Does it work for you if you use English Firefox (Nightly)?
Honza
Flags: needinfo?(hubscher.remy)
Updated•7 years ago
|
Priority: -- → P5
Reporter | ||
Comment 2•7 years ago
|
||
I was able to reproduce on a fresh account with Firefox Nightly EN.
You can reproduce by looking at the POST query to Algolia.net here for instance: https://www.chefclub.tv/fr/recettes/original/cd00692c-3bfe-5926-7d5e-cfac5310286c/pommes-de-terre-tourbillon-elles-vont-faire-tourner-des-tetes/
Flags: needinfo?(hubscher.remy)
Reporter | ||
Comment 3•7 years ago
|
||
Reporter | ||
Comment 4•7 years ago
|
||
Don't worry about the API key in the screenshot it is a readonly one already public :)
Comment 5•7 years ago
|
||
Thanks, I see the problem now!
Some info for anyone interested in fixing this issue:
1) The postBody in the test case from comment #2 is:
[ "{\"params\":\"query=POMMES%20DE%20TERRE%20TOURBILLON%20pomme%20de%20terre%20patate%20pur%C3%A9e%20emmental%20friture%20oignon%20'pomme%20de%20terre'&filters=language%3Afr%20AND%20NOT%20objectID%3Acd00692c-3bfe-5926-7d5e-cfac5310286c&removeWordsIfNoResults=allOptional&advancedSyntax=true&hitsPerPage=4\"}" ]
content-type: application/x-www-form-urlencoded
2) Parsing of form data (post body) is done here:
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/devtools/client/netmonitor/src/components/ParamsPanel.js#125
3) `parseFormData` helper returns something like as follows:
0: Object { name: "{\"params\":\"query", value: "POMMES DE TERRE TOURBILLON pomme de terre patate purée emmental friture oignon 'pomme de terre'" }
1: Object { name: "filters", value: "language:fr AND NOT objectID:cd00692c-3bfe-5926-7d5e-cfac5310286c" }
2: Object { name: "removeWordsIfNoResults", value: "allOptional" }
3: Object { name: "advancedSyntax", value: "true" }
4: Object { name: "hitsPerPage", value: "4\"}" }
length: 5
4) And it looks like that `getProperties` is sorting the result and producing wrongly formatted result.
So, we need to make sure that the parsing does expected thing here.
---
I am marking this one as good-first-bug since it might be relatively simple to fix.
Proper test coverage needed for this one (new test or updating an existing).
Honza
Mentor: odvarko
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: P5 → P3
Whiteboard: good-first-bug
Comment 6•7 years ago
|
||
Hi,
I'm interested in working on this.
Please correct me if I misunderstood the issue here.
The expected formatting is to display query params in the same order they are sent ( to preserve braces? ) and that order is changed(sorted) when we use getProperties because it calls sortObjectKeys internally .
Is this correct?
Hi, I have already started working on this (Sorry Pavan).
Now I need feedback.
Flags: needinfo?(odvarko)
Attachment #8989678 -
Flags: review+
Attachment #8989678 -
Flags: feedback+
Comment 8•7 years ago
|
||
(In reply to Pavan Veginati from comment #6)
So, I am going to assign the bug to Amir since he just attached a patch.
But, please see this list for other available bugs for Network panel
(all good first bugs):
https://bugs.firefox-dev.tools/?easy&tool=network
Feel free to ask me for any help!
(In reply to Amir from comment #7)
> Created attachment 8989678 [details] [diff] [review]
> network.patch
>
> Hi, I have already started working on this (Sorry Pavan).
> Now I need feedback.
Thanks for the patch, but you should ask for the bug first
and wait till its assigned to you before claiming its yours.
We want to avoid collisions like this.
Also, you are not supposed to set review and feedback flags to +
You should set them to ? and wait for response that can be + or -
Honza
Assignee: nobody → asaba90
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Comment 9•7 years ago
|
||
Comment on attachment 8989678 [details] [diff] [review]
network.patch
Review of attachment 8989678 [details] [diff] [review]:
-----------------------------------------------------------------
Some eslint errors need to be fixed yet.
Honza
Attachment #8989678 -
Flags: review+ → review-
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> (In reply to Amir from comment #7)
> > Created attachment 8989678 [details] [diff] [review]
> > network.patch
> >
> > Hi, I have already started working on this (Sorry Pavan).
> > Now I need feedback.
> Thanks for the patch, but you should ask for the bug first
> and wait till its assigned to you before claiming its yours.
> We want to avoid collisions like this.
>
> Also, you are not supposed to set review and feedback flags to +
> You should set them to ? and wait for response that can be + or -
>
> Honza
Sorry. I didn't know the procedure. I'll first ask for assignment next time.
(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> Comment on attachment 8989678 [details] [diff] [review]
> network.patch
>
> Review of attachment 8989678 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Some eslint errors need to be fixed yet.
>
> Honza
I get 0 errors and 0 warnings by running ./mach eslint devtools/client/netmonitor/src/components/
Amir
Updated•7 years ago
|
Summary: Network body is wrongly formated → Network body is wrongly formatted
Comment 11•7 years ago
|
||
(In reply to Amir from comment #10)
> I get 0 errors and 0 warnings by running ./mach eslint
> devtools/client/netmonitor/src/components/
True (that was different patch I see failing with eslint)
I pushed the patch to try, let's first see how that goes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce18ff2c9d6a11bb7356759264ffce957f4d1411
Honza
Assignee | ||
Comment 12•7 years ago
|
||
Two tests were failing, because they were expecting parameters to get sorted. I fixed the tests.
Comment 13•7 years ago
|
||
@Rémy: what is actually your expectation? The test case is posting JSON data,
but using wrong content type: application/x-www-form-urlencoded
So, the panel is trying to parse (as URL encoded string) and sort alphabetically.
If you use correct content type for posted data, e.g. application/json it nicely
shows JSON tree.
Honza
Flags: needinfo?(hubscher.remy)
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> @Rémy: what is actually your expectation? The test case is posting JSON data,
> but using wrong content type: application/x-www-form-urlencoded
>
> So, the panel is trying to parse (as URL encoded string) and sort
> alphabetically.
>
> If you use correct content type for posted data, e.g. application/json it
> nicely
> shows JSON tree.
>
> Honza
The input does not seem to be JSON. It is valid URL encoded form data.
I also fixed a typo in my last patch. I attached the correct one.
How can I run the tests locally by the way?
Reporter | ||
Comment 15•7 years ago
|
||
As Amir says it looks like JSON but it is not JSON.
I'd like to say that I am a user of this API and probably wouldn't have it designed that way...
Flags: needinfo?(hubscher.remy)
Comment 16•7 years ago
|
||
(In reply to Amir from comment #14)
> The input does not seem to be JSON. It is valid URL encoded form data.
>
> I also fixed a typo in my last patch. I attached the correct one.
>
> How can I run the tests locally by the way?
Run all net monitor tests:
./mach test devtools/client/netmonitor/test
Run specific test
./mach test devtools/client/netmonitor/test/browser_net_api-calls.js
Honza
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> Run all net monitor tests:
> ./mach test devtools/client/netmonitor/test
>
> Run specific test
> ./mach test devtools/client/netmonitor/test/browser_net_api-calls.js
2 more tests were failing. I attached the new patch.
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> @Amir: any progress on this bug?
>
> Honza
I have fixed the tests. My latest patch works fine. Please test it and tell if I need to do anything.
Flags: needinfo?(asaba90)
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
(In reply to Amir from comment #19)
> I have fixed the tests. My latest patch works fine. Please test it and tell
> if I need to do anything.
I just pushed to try, let's see if it's green.
TODO:
1) Can you please mark the old patches as obsolete (patch details -> edit details -> obsolete checkbox)
2) Can you please add a comment in front of the `getProperties` method explaining why we are not sorting the props? Something like:
/**
* Mapping array to dict for TreeView usage.
* Since TreeView only support Object(dict) format.
* This function also deal with duplicate key case
* (for multiple selection and query params with same keys)
*
* This function is not sorting result properties since it can
* results in unexpected order of params. See bug 1469533
*
* @param {Object[]} arr - key-value pair array like query or form params
* @returns {Object} Rep compatible object
*/
Also, please always need-info me otherwise your question can easily be lost in the bugmail.
Thanks!
Honza
Flags: needinfo?(asaba90)
Attachment #8989678 -
Attachment is obsolete: true
Flags: needinfo?(asaba90)
Attachment #8989994 -
Attachment is obsolete: true
Attachment #8990490 -
Attachment is obsolete: true
Attachment #8992873 -
Attachment is obsolete: true
Assignee | ||
Comment 22•6 years ago
|
||
I added the comment. But some of the test were failing due to failed network requests.
Flags: needinfo?(odvarko)
Comment 23•6 years ago
|
||
Comment on attachment 9012114 [details] [diff] [review]
network-v5.patch
Review of attachment 9012114 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the update.
R+
Honza
Attachment #9012114 -
Flags: review+
Comment 24•6 years ago
|
||
Flags: needinfo?(odvarko)
Updated•6 years ago
|
Keywords: checkin-needed
Comment 25•6 years ago
|
||
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5baad77e684
Remove sorting parameters r=Honza
Keywords: checkin-needed
Comment 26•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in
before you can comment on or make changes to this bug.
Description
•