dgram: tighten address validation in socket.send

[VoltrexKeyva]

We don't mention a value being "falsy" in validation, its
better to use a validator here to keep consistency,
this change makes the address parameter only
accept a string, null or undefined.

[aduh95]

You'd need to update the docs here:

node/doc/api/dgram.md

Lines 518 to 521 in f4d0a6a

	The `address` argument is a string. If the value of `address` is a host name,
	DNS will be used to resolve the address of the host. If `address` is not
	provided or otherwise falsy, `'127.0.0.1'` (for `udp4` sockets) or `'::1'`
	(for `udp6` sockets) will be used by default.

Also doesn't that make the method throw if address is nullish? undefined and null should still be valid value IMHO.

[VoltrexKeyva]

You'd need to update the docs here:

node/doc/api/dgram.md

Lines 518 to 521 in f4d0a6a

	The `address` argument is a string. If the value of `address` is a host name,
	DNS will be used to resolve the address of the host. If `address` is not
	provided or otherwise falsy, `'127.0.0.1'` (for `udp4` sockets) or `'::1'`
	(for `udp6` sockets) will be used by default.

Also doesn't that make the method throw if address is nullish? undefined and null should still be valid value IMHO.

No, it doesn't make it throw if the value is "falsy" (or "nullish" if you will) as it only validates the value as a string if the value is not "falsy" or "nullish".

[aduh95]

How do you mean? Doesn't validateString simply check the typeof?

node/lib/internal/validators.js

Lines 117 to 120 in 46a7e61

	function validateString(value, name) {
	if (typeof value !== 'string')
	throw new ERR_INVALID_ARG_TYPE(name, 'string', value);
	}

[VoltrexKeyva]

@aduh95 Here's how:

The current code:

• Check if address is not falsy or nullish
  • Check if type of address doesn't equal to string
    • Throw
• Otherwise ignore

---

This change:

• Check if address is not falsy or nullish
  • Validate address to check if its a type of string with validateString()
    • validateString(): If type of the passed value is not string, then throw
• Otherwise ignore

---

Both leading to the exact same result but current change tends to keep consistency with the usage of validators and improve readability a little.

[aduh95]

Oh right, I guess I missed the else if part, my bad.
I think I'd prefer to stricken the validation to allow only nullish or strings, and making this a semver-major, to make this function more consistent with how Node.js usually validates arguments. wdyt?

[VoltrexKeyva]

@aduh95 I'm fine with making the validation more strict, if this actually becomes semver-major do we need to add any change entries to the method's documentation?

[aduh95]

@aduh95 I'm fine with making the validation more strict, if this actually becomes semver-major do we need to add any change entries to the method's documentation?

That's correct, we'd need also to update the documentation and add some tests.

[VoltrexKeyva]

@aduh95 I'm fine with making the validation more strict, if this actually becomes semver-major do we need to add any change entries to the method's documentation?

That's correct, we'd need also to update the documentation and add some tests.

Done, although I don't think we should add more tests because there's already a good one that exists; fixed that one in the changed files.

[targos]

Change LGTM but please improve the commit message. There was nothing "missing" and the message should reflect the change in behavior.

[aduh95]

Can you update the doc here (falsy -> nullish):

node/doc/api/dgram.md

Lines 518 to 521 in f4d0a6a

	The `address` argument is a string. If the value of `address` is a host name,
	DNS will be used to resolve the address of the host. If `address` is not
	provided or otherwise falsy, `'127.0.0.1'` (for `udp4` sockets) or `'::1'`
	(for `udp6` sockets) will be used by default.

Also, it'd be nice to have some tests that unsures a TypeError is thrown when a non-valid value is provided. You can use the following as an example:

node/test/parallel/test-buffer-readfloat.js

Lines 70 to 75 in a352a71

	['', '0', null, {}, [], () => {}, true, false].forEach((off) => {
	assert.throws(
	() => buffer[fn](off),
	{ code: 'ERR_INVALID_ARG_TYPE' }
	);
	});

[VoltrexKeyva]

Can you update the doc here (falsy -> nullish):

node/doc/api/dgram.md

Lines 518 to 521 in f4d0a6a

	The `address` argument is a string. If the value of `address` is a host name,
	DNS will be used to resolve the address of the host. If `address` is not
	provided or otherwise falsy, `'127.0.0.1'` (for `udp4` sockets) or `'::1'`
	(for `udp6` sockets) will be used by default.

Done.

Also, it'd be nice to have some tests that unsures a TypeError is thrown when a non-valid value is provided. You can use the following as an example:

node/test/parallel/test-buffer-readfloat.js

Lines 70 to 75 in a352a71

	['', '0', null, {}, [], () => {}, true, false].forEach((off) => {
	assert.throws(
	() => buffer[fn](off),
	{ code: 'ERR_INVALID_ARG_TYPE' }
	);
	});

Doesn't this already do that though?

node/test/parallel/test-dgram-send-address-types.js

Lines 24 to 32 in 245dd62

	[[], 1, true].forEach((invalidInput) => {
	const expectedError = {
	code: 'ERR_INVALID_ARG_TYPE',
	name: 'TypeError',
	message: 'The "address" argument must be of type string.' +
	`${common.invalidArgTypeHelper(invalidInput)}`
	};
	assert.throws(() => client.send(buf, port, invalidInput), expectedError);
	});

[VoltrexKeyva]

cc @aduh95

[aduh95]

Doesn't this already do that though?

Sure, can you add the missing cases then? Something like that:

[0, 1, 1n, {}, [], () => {}, true, false, Symbol()].forEach(...

[VoltrexKeyva]

@aduh95 done.

[VoltrexKeyva]

@aduh95 👍

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/39750/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/39756/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/39957/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/39962/

[targos]

Landed in 40c6e83