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

lib: allow to validate enums with validateOneOf #34070

Closed
wants to merge 1 commit into from

Conversation

lundibundi
Copy link
Member

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

@lundibundi lundibundi added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 26, 2020
@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

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.

@lundibundi
Copy link
Member Author

@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.

@lundibundi lundibundi added the needs-benchmark-ci PR that need a benchmark CI run. label Jun 26, 2020
@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

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

  1. 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.

  2. A basic microbenchmark that calls each of these as quickly as possible with identical valid inputs each time.

  3. 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.

  4. 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.

@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

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)

@Trott
Copy link
Member

Trott commented Jun 26, 2020

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.)

@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

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)

@Trott
Copy link
Member

Trott commented Jul 2, 2020

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.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

@lundibundi lundibundi changed the title lib: allow to validate enums in validateNumber/validateString lib: allow to validate enums with validateOneOf Aug 7, 2020
@lundibundi lundibundi removed the needs-benchmark-ci PR that need a benchmark CI run. label Aug 7, 2020
@lundibundi
Copy link
Member Author

lundibundi commented Aug 7, 2020

@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.

@lundibundi lundibundi added the blocked PRs that are blocked by other issues or PRs. label Aug 7, 2020
throw new ERR_INVALID_ARG_VALUE(name, value, reason);
} else {
throw new ERR_INVALID_OPT_VALUE(name, value, reason);
}
Copy link
Member

@jasnell jasnell Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄 .

@lundibundi lundibundi requested a review from a team as a code owner August 10, 2020 16:06
@lundibundi lundibundi removed the blocked PRs that are blocked by other issues or PRs. label Aug 15, 2020
@lundibundi
Copy link
Member Author

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

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

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Aug 17, 2020

Landed in 6726246

@jasnell jasnell closed this Aug 17, 2020
jasnell pushed a commit that referenced this pull request Aug 17, 2020
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
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>
lundibundi added a commit to lundibundi/node that referenced this pull request Aug 18, 2020
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>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
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>
lundibundi added a commit that referenced this pull request Sep 11, 2020
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>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants