lib: allow to validate enums with validateOneOf by lundibundi · Pull Request #34070 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

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

We should benchmark this. I tried making some similar types of changes in here and the performance hit was significant due to issues with the way v8 optimizes these. I've been considering taking a different approach with the validators that should allow the optimizer to be more effective.

@jasnell what subsystems do you have in mind so we could start the benchmark CI, I don't think these 2 are extensively used in our codebase (maybe path, buffer, tls, http2 are most affected by the quick grep of these functions)?
I would definitely like to hear of a better approach to these as the current one is not really efficient or extensible.

I would do four benchmarks, they are a bit artificial but should be useful.

A basic microbenchmark that calls each of these as quickly as possible but with different inputs to validate each time. This should work the optimizer a bit since the argument being validated will change shape.
A basic microbenchmark that calls each of these as quickly as possible with identical valid inputs each time.
Create a function that uses each of these to validate arguments and call it as quickly as possible with different inputs to validate each time.
The same function with valid inputs.

The approach that I'm considering as an alternative would use a builder pattern to create validators rather than using options... for instance:

const { makeStringValidator } = require('internal/validators');

const fooValidator = makeStringValidator('foo', { oneOf: ['a', 'b'] });

stringValidator.fooValidator('c');

The builder would generate a more optimized version of the validator that, hopefully, takes less of a performance hit each time. I'm hoping but I'm not certain that the v8 optimizer will be capable of handling it more effectively also, but I haven't tested any of that.

Just finally was able to get through some testing here and it (thankfully) looks like there's no issue around opt/dept in the validators. So, yay :-) ...

btw, another PR to compare with here: #33288 (comment)

I'm sorry if this is an ignorant question, but am I correct that this adds an argument to existing internal-only functions that we then don't use in any lib code? Why do we want to make this change? (There's probably a good reason. I just don't know what it is.)

I presume the reason would be to start using it in various locations. Here's an example of where it could be useful: #34023 (comment)

I presume the reason would be to start using it in various locations. Here's an example of where it could be useful: #34023 (comment)

I would prefer this be bundled with a commit that actually uses it.

If we do include something like this (and a use case), I think this should be a separate function validateOneOf.

It does essentially the same in both cases (except formatting, which can be slow since it's only on the error path).
If all possible values are specified anyway, it does not matter what type they are.
It prevents performance hits of APIs that use the existing functions (validateNumber or validateString).
It removes the need for ArrayIsArray.

(Requesting changes to prevent it from landing, sorry.)

@Trott @tniessen @jasnell finally got time for this, changed the implementation to introduce new validator validateOneOf and used it in some of the places I could find.
I don't really like the option = false thingy but there is no other way to support both ERR_INVALID_ARG_VALUE and ERR_INVALID_OPT_VALUE (we could add separate functions though).

This also depends on #34671 for proper error formatting.

FWIW, I generally ignore ERR_INVALID_OPT_VALUE and instead use the options.${name} pattern instead with ERR_INVALID_ARG_VALUE.

As a semver-major follow-up to this PR, it may be worth moving the existing uses of ERR_INVALID_OPT_VALUE over and we can discontinue use of ERR_INVALID_OPT_VALUE

I'd love to see that. I always found different error codes for OPT and ARG weird since they are not really useful imo.
I will work on it 😄 .

@tniessen @jasnell @Trott @himself65 this is now ready.

The OPT_VALUE is still here since removing it is semver-major anyway.

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

Landed in 6726246

PR-URL: #34070 Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

This will be a start to generalize all argument validation errors. As currently we throw ARG/OPT, OUT_OF_RANGE, and other more specific errors. The OPT errors didn't bring much to the errors as it's just another variant of ARG error which is sometimes more confusing (some of our code used OPT errors to denote just argument validation errors presumably because of similarity of OPT to 'option' and not 'options-object') and they don't specify the name of the options object where the invalid value is located. Much better approach would be to just specify path to the invalid value in the name of the value as it is done in this PR (i.e. 'options.format', 'options.publicKey.type' etc) Also since this decreases a variety of errors we have it'd be easier to reuse validation code across the codebase. Refs: nodejs#31251 Refs: nodejs#34070 (comment) Signed-off-by: Denys Otrishko <shishugi@gmail.com>

PR-URL: #34070 Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

This will be a start to generalize all argument validation errors. As currently we throw ARG/OPT, OUT_OF_RANGE, and other more specific errors. The OPT errors didn't bring much to the errors as it's just another variant of ARG error which is sometimes more confusing (some of our code used OPT errors to denote just argument validation errors presumably because of similarity of OPT to 'option' and not 'options-object') and they don't specify the name of the options object where the invalid value is located. Much better approach would be to just specify path to the invalid value in the name of the value as it is done in this PR (i.e. 'options.format', 'options.publicKey.type' etc) Also since this decreases a variety of errors we have it'd be easier to reuse validation code across the codebase. Refs: #31251 Refs: #34070 (comment) Signed-off-by: Denys Otrishko <shishugi@gmail.com> PR-URL: #34682 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This will be a start to generalize all argument validation errors. As currently we throw ARG/OPT, OUT_OF_RANGE, and other more specific errors. The OPT errors didn't bring much to the errors as it's just another variant of ARG error which is sometimes more confusing (some of our code used OPT errors to denote just argument validation errors presumably because of similarity of OPT to 'option' and not 'options-object') and they don't specify the name of the options object where the invalid value is located. Much better approach would be to just specify path to the invalid value in the name of the value as it is done in this PR (i.e. 'options.format', 'options.publicKey.type' etc) Also since this decreases a variety of errors we have it'd be easier to reuse validation code across the codebase. Refs: nodejs#31251 Refs: nodejs#34070 (comment) Signed-off-by: Denys Otrishko <shishugi@gmail.com> PR-URL: nodejs#34682 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

lundibundi added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 26, 2020

lundibundi mentioned this pull request Jun 26, 2020

vm: add run-after-evaluate microtask mode #34023

Closed

4 tasks

lundibundi added the needs-benchmark-ci PR that need a benchmark CI run. label Jun 26, 2020

himself65 approved these changes Jun 26, 2020

View reviewed changes

tniessen requested changes Jul 17, 2020

View reviewed changes

lundibundi force-pushed the validators-allow-enum branch from e58c2b4 to 7c15424 Compare August 7, 2020 20:04

lundibundi changed the title lib: allow to validate enums in validateNumber/validateString lib: allow to validate enums with validateOneOf Aug 7, 2020

lundibundi removed the needs-benchmark-ci PR that need a benchmark CI run. label Aug 7, 2020

lundibundi added the blocked PRs that are blocked by other issues or PRs. label Aug 7, 2020

jasnell reviewed Aug 7, 2020

View reviewed changes

lundibundi mentioned this pull request Aug 7, 2020

errors: add ERR_INVALID_OPT_TYPE and support options in validators.js #31251

Closed

4 tasks

lundibundi mentioned this pull request Aug 8, 2020

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

Closed

4 tasks

lundibundi requested a review from a team as a code owner August 10, 2020 16:06

lundibundi removed the blocked PRs that are blocked by other issues or PRs. label Aug 15, 2020

lib: allow to validate enums with validateOneOf

38d603f

lundibundi force-pushed the validators-allow-enum branch from 7c15424 to 38d603f Compare August 16, 2020 07:29

jasnell approved these changes Aug 17, 2020

View reviewed changes

tniessen approved these changes Aug 17, 2020

View reviewed changes

jasnell closed this Aug 17, 2020

jasnell pushed a commit that referenced this pull request Aug 17, 2020

lib: allow to validate enums with validateOneOf

6726246

PR-URL: #34070 Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Aug 17, 2020

lib: allow to validate enums with validateOneOf

3b3a70e

PR-URL: #34070 Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

BethGriggs pushed a commit that referenced this pull request Aug 20, 2020

lib: allow to validate enums with validateOneOf

04defba

PR-URL: #34070 Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

lib: allow to validate enums with validateOneOf by lundibundi · Pull Request #34070 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: allow to validate enums with validateOneOf - #34070

lib: allow to validate enums with validateOneOf#34070lundibundi wants to merge 1 commit intonodejs:masternodejs/node:masterfrom lundibundi:validators-allow-enumlundibundi/node:validators-allow-enumCopy head branch name to clipboard

Conversation

lundibundi commented Jun 26, 2020

Checklist

Uh oh!

nodejs-github-bot commented Jun 26, 2020

Uh oh!

jasnell commented Jun 26, 2020

Uh oh!

lundibundi commented Jun 26, 2020

Uh oh!

jasnell commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jun 26, 2020

Uh oh!

jasnell commented Jun 26, 2020

Uh oh!

Trott commented Jul 2, 2020

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Aug 16, 2020

Uh oh!

nodejs-github-bot commented Aug 16, 2020

Uh oh!

jasnell commented Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lib: allow to validate enums with validateOneOf#34070
lundibundi wants to merge 1 commit into
nodejs:masterfrom
lundibundi:validators-allow-enum

jasnell commented Jun 26, 2020 •
edited
Loading

jasnell commented Jun 26, 2020 •
edited
Loading

lundibundi commented Aug 7, 2020 •
edited
Loading

jasnell Aug 7, 2020 •
edited
Loading