Closed Bug 1469533 Opened 6 years ago Closed 6 years ago

Network body is wrongly formatted

Categories

(DevTools :: Netmonitor, defect, P3)

60 Branch
defect

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)

Attached image bug.png
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"}
Component: Untriaged → Netmonitor
Product: Firefox → DevTools
Attached image json-postdata.png
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)
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)
Attached image reproduce.png
Don't worry about the API key in the screenshot it is a readonly one already public :)
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
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?
Attached patch network.patch (obsolete) — Splinter Review
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+
(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 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-
(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
Summary: Network body is wrongly formated → Network body is wrongly formatted
(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
Attached patch network.patch (obsolete) — Splinter Review
Two tests were failing, because they were expecting parameters to get sorted. I fixed the tests.
@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)
Attached patch network-v3.patch (obsolete) — Splinter Review
(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?
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)
(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
Attached patch network-v4.patch (obsolete) — Splinter Review
(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.
@Amir: any progress on this bug?

Honza
Flags: needinfo?(asaba90)
(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)
(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
Attached patch network-v5.patchSplinter Review
I added the comment. But some of the test were failing due to failed network requests.
Flags: needinfo?(odvarko)
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+
https://hg.mozilla.org/mozilla-central/rev/f5baad77e684
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: