Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url: throw for invalid values to url.format #1036

Merged
merged 1 commit into from
Mar 4, 2015
Merged

Conversation

tellnes
Copy link
Contributor

@tellnes tellnes commented Mar 3, 2015

(Original title: "url: fix passing undefined to url.format")

'use strict' changes the behavior for Function.prototype.call when
the context is undefined. In earlier versions of node the value
undefined would make url.format look for fields in the global scope.

Fixes: #1033

@vkurchatkin
Copy link
Contributor

In earlier versions of node the value undefined would make url.format look for fields in the global scope.

That's actually a bug isn't it? Current behaviour seems to be better

@cjihrig
Copy link
Contributor

cjihrig commented Mar 3, 2015

Why not throw if the input isn't a string or object? That seems more predictable.

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

Yes, that should be classified as a bug in earlier node versions. Including v0.12.0.

This changes removes the TypeError we started getting after introducing strict mode and always return an empty string for falsey values. That is probably how people expect it to behave. I don't think anyone expect it to look for values in global scope or throw.

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

@cjihrig To throw is maybe a better solution, but I think that will break more code than just return an empty string.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 3, 2015

An argument could be made against breaking code, but IMO this is still a bug fix. Code will be broken if people are relying on incorrect behavior. The fix also only handles falsey values. There are many truthy values that are incorrect here. I'm also working under the assumption that core's philosophy has been to throw immediately on invalid inputs (although this is not enforced 100% of the time).

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

Actually there is only a problem with null and undefined. As fare as I know, all other values will be converted to an Object when we are calling Function.prototype.call.

I do agree that throwing is a better solution, but if we are going to change it to throw for everything which is not a string or an object, then this will be a breaking change and needs a semver-major.

@brendanashworth
Copy link
Contributor

+1 for throwing a TypeError.
I don't think it'd be semver-major, as the first few words of the url.format documentation are:

Take a parsed URL object ...

That its taking any other value would be a bug clear to any developer who read the docs.

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

Ok, maybe we should get some comments from others about if we should throw or just return an empty string.

cc @iojs/collaborators

@mscdex
Copy link
Contributor

mscdex commented Mar 3, 2015

If we're strictly expecting an object, I'd think a better check would be something like typeof obj !== 'object' || obj === null

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

@mscdex Also see the code commit below my change.

@julianduque
Copy link
Contributor

I'm +1 on throwing TypeError rather than return an empty string

@chrisdickinson
Copy link
Contributor

Throw a TypeError. This really only stands a chance to break already broken code (either it was already throwing a TypeError due to strict mode, or it was looking up properties off of global scope!) I'd put it down as semver-patch.

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

I suppose this should be left intact

  // ensure it's an object, and not a string url.
  // If it's an obj, this is a no-op.
  // this way, you can call url_format() on strings
  // to clean up potentially wonky urls.
  if (typeof obj === 'string') obj = urlParse(obj);

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

Ok, I've changed it to throw for all input values except object excluding null and strings.

@@ -353,6 +353,13 @@ function urlFormat(obj) {
// this way, you can call url_format() on strings
// to clean up potentially wonky urls.
if (typeof obj === 'string') obj = urlParse(obj);

if (typeof obj !== 'object')
throw new TypeError('Parameter `urlObj` must be an object, not ' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use quotes instead of backticks. :)

@trevnorris
Copy link
Contributor

Two minor things, but LGTM otherwise.

@tellnes
Copy link
Contributor Author

tellnes commented Mar 3, 2015

Minor things fixed

@trevnorris
Copy link
Contributor

anyone else have anything to say?

@tellnes tellnes changed the title url: fix passing undefined to url.format url: throw for invalid values to url.format Mar 3, 2015
@@ -353,6 +353,13 @@ function urlFormat(obj) {
// this way, you can call url_format() on strings
// to clean up potentially wonky urls.
if (typeof obj === 'string') obj = urlParse(obj);

if (typeof obj !== 'object')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: If urlParse() guarantees to return an object, then you could avoid these two checks by making them else ifs. If you opt for a simpler error message, you could combine the two into one else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did choose to use only if for readability, but yes, I should probably change it to else if for speed.
Also, I did prefer the explicit error message over one else if.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd suggest a third way, a ternary statement:

if (typeof obj !== 'object' || obj === null)
    throw new TypeError("Parameter 'urlObj' must be an object, not " +
                        obj === null ? "null" : typeof obj);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend saving typeof obj above. There are several repetitions. The ternary on the error looks like a fine solution otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V8 has pattern-matching logic for if (typeof(value) === 'type') guards. Storing the result of typeof(value) in a local before comparing kills that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Does that also match for situations like eg. how the code is now which is else if (typeof obj !== 'object' || obj === null)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tellnes Yes, that's fine.

@tellnes
Copy link
Contributor Author

tellnes commented Mar 4, 2015

Combined the two ifs and also changed the following if to an else if.

@trevnorris
Copy link
Contributor

/cc @iojs/collaborators

@mscdex
Copy link
Contributor

mscdex commented Mar 4, 2015

LGTM

1 similar comment
@julianduque
Copy link
Contributor

LGTM

`'use strict'` changes the behavior for `Function.prototype.call` when
the context is `undefined`. In earlier versions of node the value
`undefined` would make `url.format` look for fields in the global scope.

The docs states that `url.format` takes a parsed URL object and returns
a formatted URL string. So with this change it will now throw for other
values.

The exception is if the input is a string. Then it will call `url.parse`
on the string and then format it. The reason for that is that you can
call `url.format` on strings to clean up potentially wonky urls.

Fixes: nodejs#1033
PR-URL: nodejs#1036
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@tellnes tellnes merged commit abb00cc into nodejs:v1.x Mar 4, 2015
@tellnes tellnes deleted the gh-1033 branch March 4, 2015 21:06
@rvagg rvagg mentioned this pull request Mar 4, 2015
@ChALkeR ChALkeR added the url Issues and PRs related to the legacy built-in url module. label Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants