Skip to content

Commit

Permalink
dns: deprecate passing falsy hostname to dns.lookup
Browse files Browse the repository at this point in the history
We can `dns.lookup` a falsy `hostname` like `dns.lookup(false)`
for the reason of backwards compatibility long before(see nodejs#13119
for detail). This behavior is undocumented and seems useless in
real world apps.

We could also make invalid `hostname` throw in the future and the
change might be semver-major.

Fixes: nodejs#13119
  • Loading branch information
oyyd committed Sep 30, 2018
1 parent 70ab310 commit 0ae969f
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 6 deletions.
6 changes: 4 additions & 2 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ const {
getDefaultResolver,
setDefaultResolver,
Resolver,
validateHints
validateHints,
emitInvalidHostnameWarning,
} = require('internal/dns/utils');
const {
ERR_INVALID_ARG_TYPE,
Expand Down Expand Up @@ -93,7 +94,7 @@ function lookup(hostname, options, callback) {

// Parse arguments
if (hostname && typeof hostname !== 'string') {
throw new ERR_INVALID_ARG_TYPE('hostname', ['string', 'falsy'], hostname);
throw new ERR_INVALID_ARG_TYPE('hostname', 'string', hostname);
} else if (typeof options === 'function') {
callback = options;
family = 0;
Expand All @@ -114,6 +115,7 @@ function lookup(hostname, options, callback) {
throw new ERR_INVALID_OPT_VALUE('family', family);

if (!hostname) {
emitInvalidHostnameWarning(hostname);
if (all) {
process.nextTick(callback, null, []);
} else {
Expand Down
6 changes: 4 additions & 2 deletions lib/internal/dns/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
const {
bindDefaultResolver,
Resolver: CallbackResolver,
validateHints
validateHints,
emitInvalidHostnameWarning,
} = require('internal/dns/utils');
const { codes, dnsException } = require('internal/errors');
const { isIP, isIPv4, isLegalPort } = require('internal/net');
Expand Down Expand Up @@ -56,6 +57,7 @@ function onlookupall(err, addresses) {
function createLookupPromise(family, hostname, all, hints, verbatim) {
return new Promise((resolve, reject) => {
if (!hostname) {
emitInvalidHostnameWarning(hostname);
if (all)
resolve([]);
else
Expand Down Expand Up @@ -100,7 +102,7 @@ function lookup(hostname, options) {

// Parse arguments
if (hostname && typeof hostname !== 'string') {
throw new ERR_INVALID_ARG_TYPE('hostname', ['string', 'falsy'], hostname);
throw new ERR_INVALID_ARG_TYPE('hostname', 'string', hostname);
} else if (options !== null && typeof options === 'object') {
hints = options.hints >>> 0;
family = options.family >>> 0;
Expand Down
11 changes: 10 additions & 1 deletion lib/internal/dns/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,19 @@ function validateHints(hints) {
}
}

function emitInvalidHostnameWarning(hostname) {
process.emitWarning(
`The provided hostname "${hostname}" is not a valid ` +
'hostname, and is supported in the dns module solely for compatibility.',
'DeprecationWarning',
);
}

module.exports = {
bindDefaultResolver,
getDefaultResolver,
setDefaultResolver,
validateHints,
Resolver
Resolver,
emitInvalidHostnameWarning,
};
25 changes: 24 additions & 1 deletion test/parallel/test-dns-lookup.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,36 @@ cares.getaddrinfo = () => internalBinding('uv').UV_ENOENT;
const err = {
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: /^The "hostname" argument must be one of type string or falsy/
message: /^The "hostname" argument must be of type string\. Received type number/
};

common.expectsError(() => dns.lookup(1, {}), err);
common.expectsError(() => dnsPromises.lookup(1, {}), err);
}

common.expectWarning({
// For 'internal/test/binding' module.
'internal/test/binding': [
'These APIs are exposed only for testing and are not ' +
'tracked by any versioning system or deprecation process.'
],
// For dns.promises.
'ExperimentalWarning': [
'The dns.promises API is experimental'
],
// For call `dns.lookup` with falsy `hostname`, twice.
'DeprecationWarning': [
[
'The provided hostname "false" is not a valid ' +
'hostname, and is supported in the dns module solely for compatibility.',
],
[
'The provided hostname "false" is not a valid ' +
'hostname, and is supported in the dns module solely for compatibility.',
]
],
});

common.expectsError(() => {
dns.lookup(false, 'cb');
}, {
Expand Down

0 comments on commit 0ae969f

Please sign in to comment.