url.format strips query in 0.11.15

[pierre-elie]

In 0.11.15, url.format(urlObj) seems to remove query from urlObj and does not include it in the formatted url.

0.11.14 does not have this issue.

[misterdjules]

@pierre-elie Thank you for reporting this issue!

Just because it's a bit easier to copy/paste, here's a repro of this issue as text:

var url = require('url');
var assert = require('assert');

var parsed = url.parse('http://example.com', false);
parsed.query = {key: 'value'};

url.format(parsed, true);

assert.ok(parsed.query, 'query property should exist');
assert.equal(parsed.query.key, 'value', "query,key should equal 'value'");

The breaking change is d312b6d. @chrisdickinson Do you have some time to take a look at it?

[chrisdickinson]

path overrides query, per the following diagram:

+-----------------------------------------------------------------------------------+
| href                                                                              |
+----------+--+-----------+--------------------+-----------------------------+------+
| protocol |* | auth      | host               | path                        | hash |
|          |  |           +-------------+------+----------+------------------+      |
|          |  |           | hostname    | port | pathname | search           |      |
|          |  |           |             |      |          +-+----------------+      |
|          |  |           |             |      |          | | query          |      |
" http:     //   user:pass@some.host.com:65535 /path/name ? search=1&continues#hash "
|          |  |           |             |      |          | |                |      |
+----------+--+-----------+-------------+------+----------+-+----------------+------+
(all spaces in the "" line should be ignored -- they're purely for formatting)
*: given by "slashes" key

path is to query, search, and pathname as host is to hostname and port.

[chrisdickinson]

Closing this issue as "works as expected."

[misterdjules]

I find it very confusing that the code mentioned above silently swallows the query property.

It seems that the reason for having a path property that overlaps pathname and search was to make writing http.request(url.parse(someUrl)) easier. Maybe we could revisit how it was implemented and make the url module's API more robust or at least more explicit when it's not used as intended?

[misterdjules]

Reopening to continue the discussion.

[chrisdickinson]

For my part, I'm okay with higher level keys overriding lower level keys (even silently), and -0 on making .format throw on duplicate keys. The confusion in these APIs stems from the fact that both functions interact with the same object, but have different expectations of use: url.parse gives multiple levels of keys for informational purposes, because it's often easier to deal with host than hostname + port. On the other hand, .format (correctly, I think) treats lower level keys as subordinate to higher level keys. Problems crop up when trying to take the output of the one, slightly modify it, and pass it to the other. This has never worked well, thanks to host and hostname, and now works even less well thanks to path. Worse yet, from a user perspective it seems very easy to DWIM, but from the format side it's impossible difficult to tell which properties were "affected" last.

Some potential paths out of this:

1. Make .format treat "own" properties higher in priority than inherited properties, retaining all other key relationships. Make .parse return an object inheriting from the parse object. User modifications now "win" vs. higher level keys from parse.
2. Add an option to parse that forces "lowest level of keys"-only output.
3. Throw an exception on conflicting keys.
4. Retain current behavior and document it heavily.

3 and 4 leave us (and users) in the same state – leaning on documentation, and without the ability to pass the output of parse to the input of format. Solutions 1 and 2 get us closer to that ability, but have their own drawbacks: 1 seems very convoluted (if it works, it works well, but if it doesn't do what's expected, then it's hard to see why), and 2 is hard to discover.

[jasnell]

Definitely understand the logic but it's certainly non-obvious. At the very least, this needs to be clearly described in the api docs.

[tjfontaine]

1 -- doesn't solve the problem of user creating invalid input
2 -- an additional api tweaking here means that the default case will still be the predominate usage and we still don't know what that is yet
3 -- fits more in line with what I prefer, but not just when conflicting keys appear but only when their contents cannot result in the same data.
4 -- I'm not sure if documentation is enough, as this can lead to very subtle bugs.

In the event of throwing users at least have recourse to read the documentation, with silent action things are happening and they may not be aware of it.

I think this would be a good conversation to have during the call tomorrow morning.

[jasnell]

There is perhaps a 5th option... it breaks api compatibility but perhaps that is ok: Have the result of parse() return an object that has it's own format() method, the input of which allows a developer to pass in overriding values that are taken before the parse object's own.. e.g.

var parsed = url.parse('http://www.example.org/path?a=b');
var formatted = parsed.format({path:'/alternative/path?c=d'});

Here the override would have to be explicit and the original parsed object remains untouched by the overrides. We can throw as suggested if the input to format contains conflicting values (e.g. {path:'/foo?y=z', query:'a?b'})

[chrisdickinson]

Recording the results of the meeting: In the future we will throw on all conflicting keys – that is, if path and pathname are both present, but would produce different results, we will throw an exception. This would extend to host (+hostname/port) and search (+query.) For the time being (v0.12), we will only present this behavior for the path key, which is a new key for url.format. I will be handling this change and its associated documentation.

[misterdjules]

@chrisdickinson @tjfontaine Sorry to continue on a discussion after we agreed on a decision, but I'd like to express one concern that I didn't mention during the meeting.

It seems to me that throwing in url.format could/would happen much later after setting conflicting properties, making tracking the cause of issues difficult, and thus in my opinion defeating the purpose of throwing because the context would be lost.

[jasnell]

Sorry I couldn't make the call earlier and pitch in on the discussion around this but I share a bit of the same concern as @misterdjules. When the throw actually happens, it may be somewhat difficult to back track to pinpoint the actual problem but I'm not entirely certain we'll be able to do much about that. My key concern is with the part, "if path and pathname are both present, but would produce different results"... what's the plan on determining if they "produce different results". If pathname is /jasnell@example.org/a and path is /jasnell%40example.org/a?a=b, do you throw?

[misterdjules]

Fixed by 3b392d3. Thank you @pierre-elie and all for your help!