Skip to content

Commit

Permalink
url: fix treatment of some values as non-empty
Browse files Browse the repository at this point in the history
In addition to null, undefined and the empty string
are treated as empty (removing the component from the url).

The string '#' is treated same as empty values when
setting .hash.

The string '?' is treated same as empty values when
setting .search.

Fixes nodejs#1588
  • Loading branch information
petkaantonov committed May 2, 2015
1 parent 7702b0d commit e274b92
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 22 deletions.
24 changes: 14 additions & 10 deletions lib/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -879,7 +879,7 @@ Object.defineProperty(Url.prototype, 'port', {
return null;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._port = -1;
if (this._host)
this._host = null;
Expand Down Expand Up @@ -941,7 +941,7 @@ Object.defineProperty(Url.prototype, 'path', {
return (p == null && s) ? ('/' + s) : null;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._pathname = this._search = null;
return;
}
Expand Down Expand Up @@ -973,7 +973,7 @@ Object.defineProperty(Url.prototype, 'protocol', {
return proto;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._protocol = null;
} else {
var proto = '' + v;
Expand Down Expand Up @@ -1001,7 +1001,7 @@ Object.defineProperty(Url.prototype, 'href', {
var parsesQueryStrings = this._parsesQueryStrings;
// Reset properties.
Url.call(this);
if (v !== null && v !== '')
if (!isConsideredEmpty(v))
this.parse('' + v, parsesQueryStrings, false);
},
enumerable: true,
Expand All @@ -1013,7 +1013,7 @@ Object.defineProperty(Url.prototype, 'auth', {
return this._auth;
},
set: function(v) {
this._auth = v === null ? null : '' + v;
this._auth = isConsideredEmpty(v) ? null : '' + v;
this._href = '';
},
enumerable: true,
Expand All @@ -1026,7 +1026,7 @@ Object.defineProperty(Url.prototype, 'host', {
return this._host;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._port = -1;
this._hostname = this._host = null;
} else {
Expand All @@ -1053,7 +1053,7 @@ Object.defineProperty(Url.prototype, 'hostname', {
return this._hostname;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._hostname = null;

if (this._hasValidPort())
Expand All @@ -1080,7 +1080,7 @@ Object.defineProperty(Url.prototype, 'hash', {
return this._hash;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v) || v === '#') {
this._hash = null;
} else {
var hash = '' + v;
Expand All @@ -1100,7 +1100,7 @@ Object.defineProperty(Url.prototype, 'search', {
return this._search;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v) || v === "?") {
this._search = this._query = null;
} else {
var search = escapeSearch('' + v);
Expand All @@ -1125,7 +1125,7 @@ Object.defineProperty(Url.prototype, 'pathname', {
return this._pathname;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._pathname = null;
} else {
var pathname = escapePathName('' + v).replace(/\\/g, '/');
Expand All @@ -1144,6 +1144,10 @@ Object.defineProperty(Url.prototype, 'pathname', {
configurable: true
});

function isConsideredEmpty(value) {
return value === null || value === undefined || value === '';
}

// Search `char1` (integer code for a character) in `string`
// starting from `fromIndex` and ending at `string.length - 1`
// or when a stop character is found.
Expand Down
59 changes: 47 additions & 12 deletions test/parallel/test-url-accessors.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ const accessorTests = [{
}, {
// Setting href to non-null non-string coerces to string
url: 'google',
set: {href: undefined},

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 2, 2015

Does that mean there is no longer a test for undefined?

This comment has been minimized.

Copy link
@petkaantonov

petkaantonov May 2, 2015

Author Owner

undefined is treated same as null or '' after this patch.

Edit: the test's purpose is to have a falsy value that is not considered 'empty'.

set: {href: 0},
test: {
path: 'undefined',
href: 'undefined'
path: '0',
href: '0'
}
}, {
// Setting port is reflected in host
Expand Down Expand Up @@ -180,8 +180,8 @@ const accessorTests = [{
url: 'http://www.google.com',
set: {search: ''},
test: {
search: '?',
path: '/?'
search: null,
path: '/'
}
}, {

Expand All @@ -203,10 +203,11 @@ const accessorTests = [{
}, {

// Empty hash is ok
url: 'http://www.google.com',
url: 'http://www.google.com#hash',
set: {hash: ''},
test: {
hash: '#'
hash: null,
href: 'http://www.google.com/'
}
}, {

Expand Down Expand Up @@ -252,7 +253,8 @@ const accessorTests = [{
url: 'http://www.google.com',
set: {pathname: ''},
test: {
pathname: '/'
pathname: null,
href: 'http://www.google.com'
}
}, {
// Null path is ok
Expand Down Expand Up @@ -290,11 +292,12 @@ const accessorTests = [{
protocol: null
}
}, {
// Empty protocol is invalid
// Empty protocol is ok
url: 'http://www.google.com/path',
set: {protocol: ''},
test: {
protocol: 'http:'
protocol: null,
href: '//www.google.com/path'
}
}, {
// Set query to an object
Expand Down Expand Up @@ -327,9 +330,9 @@ const accessorTests = [{
url: 'http://www.google.com/path?key=value',
set: {path: '?key2=value2'},
test: {
pathname: '/',
pathname: null,
search: '?key2=value2',
href: 'http://www.google.com/?key2=value2'
href: 'http://www.google.com?key2=value2'
}
}, {
// path is reflected in search and pathname 3
Expand All @@ -349,6 +352,38 @@ const accessorTests = [{
search: null,
href: 'http://www.google.com'
}
}, {
// setting hash to '' removes any hash
url: 'http://www.google.com/#hash',
set: {hash: ''},
test: {
hash: null,
href: 'http://www.google.com/'
}
}, {
// setting hash to '#' removes any hash
url: 'http://www.google.com/#hash',
set: {hash: '#'},
test: {
hash: null,
href: 'http://www.google.com/'
}
}, {
// setting search to '' removes any search
url: 'http://www.google.com/?search',
set: {search: ''},
test: {
search: null,
href: 'http://www.google.com/'
}
}, {
// setting search to '?' removes any search
url: 'http://www.google.com/?search',
set: {search: '?'},
test: {
search: null,
href: 'http://www.google.com/'
}
}

];
Expand Down

0 comments on commit e274b92

Please sign in to comment.