Network body is wrongly formatted

RESOLVED FIXED in Firefox 64

Status

P3
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: natim, Assigned: asaba90, Mentored)

Tracking

({good-first-bug})

60 Branch
Firefox 64
good-first-bug

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: good-first-bug)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

5 months ago
Created attachment 8986146 [details]
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"}

Updated

5 months ago
Component: Untriaged → Netmonitor
Product: Firefox → DevTools
Created attachment 8986999 [details]
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)
Priority: -- → P5
(Reporter)

Comment 2

5 months 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

5 months ago
Created attachment 8987057 [details]
reproduce.png
(Reporter)

Comment 4

5 months ago
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

Comment 6

4 months 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?
(Assignee)

Comment 7

4 months ago
Created attachment 8989678 [details] [diff] [review]
network.patch

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-
(Assignee)

Comment 10

4 months 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
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
(Assignee)

Comment 12

4 months ago
Created attachment 8989994 [details] [diff] [review]
network.patch

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)
(Assignee)

Comment 14

4 months ago
Created attachment 8990490 [details] [diff] [review]
network-v3.patch

(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

4 months 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)
(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

4 months ago
Created attachment 8992873 [details] [diff] [review]
network-v4.patch

(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)
(Assignee)

Comment 19

2 months 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)
(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)
(Assignee)

Updated

2 months ago
Attachment #8989678 - Attachment is obsolete: true
Flags: needinfo?(asaba90)
(Assignee)

Updated

2 months ago
Attachment #8989994 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8990490 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8992873 - Attachment is obsolete: true
(Assignee)

Comment 22

2 months ago
Created attachment 9012114 [details] [diff] [review]
network-v5.patch

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+
Keywords: checkin-needed

Comment 25

2 months 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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5baad77e684
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months 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.