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

dns.lookup allows a "falsy" hostname, but behaviour is undocumented and appears useless #13119

Closed
sam-github opened this issue May 19, 2017 · 9 comments
Labels
dns Issues and PRs related to the dns subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@sam-github
Copy link
Contributor

I assumed this was a bug, but on looking into it, it seems deliberate:

node/lib/dns.js

Line 132 in 6f21671

'hostname must be a string or falsey');

@cjihrig you added this, I think, in 5086d6e (missing PR metadata), but it looks like you were cleaning up something from an earlier commit. Any ideas why falsey is allowed?

Its even tested:

assert.doesNotThrow(() => {
dns.lookup(false, {
hints: 0,
family: 0,
all: true
}, common.mustCall((error, result, addressType) => {
assert.ifError(error);
assert.deepStrictEqual(result, []);
assert.strictEqual(addressType, undefined);
}));
});
, but I'm not sure if the test assertions were just to add coverage, or were because that was the expected output, @abouthiroppy added them in #10844, do you have any comments?

My guess was that the intention is that if hostname is falsy, there may be enough information in options to return a useful value, in which case options should be mandatory if hostname is missing.

Except... I don't see how that can be, it looks to me you will always get no results no matter what flags you put into options, and the tests even assert that, so... why is a falsey hostname allowed?

  • Version: *
  • Platform: *
  • Subsystem: dns
% ./node
> dns.lookup(false, console.log)
{}
> null null 4
@sam-github sam-github changed the title dns.lookup allows a "falsy" hostname, but behaviour is undocumented dns.lookup allows a "falsy" hostname, but behaviour is undocumented and appears useless May 19, 2017
@sam-github sam-github added the dns Issues and PRs related to the dns subsystem. label May 19, 2017
@cjihrig
Copy link
Contributor

cjihrig commented May 19, 2017

I have no clue. It looks like this behavior goes back at least 6 years. My changes look to have just been building on top of the existing quirks.

@sam-github
Copy link
Contributor Author

@bnoordhuis do you remember the history of any of this?

@bnoordhuis
Copy link
Member

There is no real backstory. It was introduced in commit fd3cd75 from 2010 as an optimization of sorts.

I think the idea was that an empty hostname string (which is falsy) never returns any results so you might as well take a shortcut.

@sam-github
Copy link
Contributor Author

@bnoordhuis Is there any reason not to call lookup of an empty string an error? I'm tempted to make everything falsy throw, like

% dig ""
dig: '' is not a legal name (unexpected end of input)
% nslookup ""
nslookup: '' is not in legal name syntax (unexpected end of input)

@bnoordhuis
Copy link
Member

Is there any reason not to call lookup of an empty string an error?

No reason except backwards compatibility. :-)

@targos
Copy link
Member

targos commented Sep 3, 2017

So what should we do:

  • Deprecate falsy argument?
  • Document that?
  • Nothing and close?

@sam-github
Copy link
Contributor Author

node is getting stricter about non-sensical arguments over time, I think that's a good trend, and it we should start to reject arguments that are invalid: false, null, undefined, and the empty string, too, IMO.

@apapirovski apapirovski added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 12, 2018
@sohailrajdev97
Copy link
Contributor

Is there someone working to change this behavior ? I would like to contribute to this issue and any leads would be appreciated.

oyyd added a commit to oyyd/node that referenced this issue Sep 30, 2018
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
@oyyd
Copy link
Contributor

oyyd commented Sep 30, 2018

Try to resolve this issue in #23173. Please comment if there are any inappropriate changes.

jasnell pushed a commit that referenced this issue Oct 17, 2018
We can `dns.lookup` a falsy `hostname` like `dns.lookup(false)`
for the reason of backwards compatibility long before(see #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: #13119

PR-URL: #23173
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants