fix: add 'hasOption' function to prevent 'null' or 'undefined' option values #1542

fedeci · 2020-11-30T20:19:24Z

This fixes #1532

Checklist

PR contains only changes related; no stray files, etc.
README updated (where applicable)
Tests written (where applicable)

codecov · 2020-12-01T02:35:46Z

Codecov Report

Merging #1542 (68ecc1b) into master (13651ea) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 68ecc1b differs from pull request most recent head d03e4bb. Consider uploading reports for the commit d03e4bb to get more accurate results

@@            Coverage Diff            @@
##            master     #1542   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          102       103    +1     
  Lines         2029      2033    +4     
  Branches       457       458    +1     
=========================================
+ Hits          2029      2033    +4

Impacted Files	Coverage Δ
src/lib/isFloat.js	`100.00% <100.00%> (ø)`
src/lib/isInt.js	`100.00% <100.00%> (ø)`
src/lib/util/hasOption.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13651ea...d03e4bb. Read the comment docs.

tux-tn

I am a little bit skeptical about this one. We don't do type checking on options for other validators, why should we for this one?

fedeci · 2020-12-19T20:05:51Z

It could undoubtedly be extended to other validators too.
I think @timo92 could provide more details than me on his use case of isInt

timo92 · 2020-12-20T16:09:29Z

As mentioned in my issue I was (still am) confused that the library seems to differentiate between passing

validator.isInt("2", {})

and

validator.isInt("2", { max: undefined })

I expected both invocations to result in the same output as options.max === undefined in both cases and the typescript types from @types/validator mark min and max as optional (i.e. not declared or undefined).
Now to me the types don't match the current implementation and one can argue which of the two implemenations/definitions is desired. To me as an end user of the library I can easily omit non required options from the argument but would have expected this handling to be performed within the library. I don't know what the initial intention behind using hasOwnProperty vs == null or the like was though.
The two possible solutions that would satisfy me personally would be to either allow explicit passing of undefined as the value of an option within validator.js or to change the typescript types to explicitly state that e.g. min must be a number or not present on the options object.

tux-tn

Hello @fedeci
I'm trying to check old and stale PRs, can you please fix merge conflicts here?

fedeci · 2021-10-02T10:39:50Z

I will in a couple of hours!

tux-tn

Thank you @fedeci !
I added two suggestions, can you check them and give me a feedback/your opinion?

tux-tn · 2021-10-02T20:44:30Z

src/lib/isInt.js

+    hasOption(options, 'allow_leading_zeroes') && !options.allow_leading_zeroes ?
      int : intLeadingZeroes
  );


Is hasOption needed here? Can't we just do this since both undefined and null will be converted to false?

let regex = options.allow_leading_zeroes ? intLeadingZeroes : int;

tux-tn · 2021-10-02T20:51:46Z

src/lib/isFloat.js

-    (!options.hasOwnProperty('max') || value <= options.max) &&
-    (!options.hasOwnProperty('lt') || value < options.lt) &&
-    (!options.hasOwnProperty('gt') || value > options.gt);
+    (!hasOption(options, 'min') || value >= options.min) &&


Was thinking of an alternative here since this function will only be needed in isFloat and isInt, can't we just use nullish coalescing operator? It will return null instead of undefined and any value > null will resolve to true

Suggested change

(!hasOption(options, 'min') || value >= options.min) &&

value >= (options.min ?? null) &&

I suppose that back when I wrote this I didn't know about the nullish coalescing op😄. Definitely what we are searching, though.

… values

fedeci · 2021-10-06T11:04:33Z

Oh, maybe ?? is not supported by eslint? Probably I should fix babel-parser config🤔

fedeci · 2021-10-06T11:46:53Z

I need to update some deps, including eslint itself reference. Do you prefer a new PR?

tux-tn · 2021-10-06T11:53:18Z

I need to update some deps, including eslint itself reference. Do you prefer a new PR?

You probably don't want to update eslint right now, i'm already working on a PR to update dev deps including rollup and eslint and you will have issues with eslint (some rules have been changed between versions)

The screenshot is yarn lint result with updated eslint dependencies and without changing the eslint config 😅

fedeci · 2021-10-06T13:09:39Z

I'll wait for that PR to mark this as ready for review, in the meanwhile I will work with my locally update deps. Remember to update babel-eslint to @babel/eslint-parser too.

fedeci · 2021-10-06T13:52:28Z

src/lib/isFloat.js

    return false;
  }
-  const value = parseFloat(str.replace(',', '.'));
+  const value = parseFloat(str.replace(splittingChar, '.'));


This is another bug fixed. I don't know why it could not be detected before.

fedeci · 2021-10-06T13:53:32Z

src/lib/isFloat.js

+    value >= (options.min ?? -Infinity) &&
+    value <= (options.max ?? Infinity) &&
+    value < (options.lt ?? Infinity) &&
+    value > (options.gt ?? -Infinity);


null returns true only for min and max. Otherwise is false. I had to use this kind of identity instead.

tux-tn · 2021-10-06T16:50:34Z

I'll wait for that PR to mark this as ready for review, in the meanwhile I will work with my locally update deps. Remember to update babel-eslint to @babel/eslint-parser too.

Thank you ! Already did 😅 I will probably finish working on it this week end

profnandaa

LGTM, thanks for fixing this.

And sorry for taking long for the review. We're now closing in, for the Jan release. Thanks @fedeci ! 🎉

profnandaa · 2022-12-12T13:51:06Z

Check the failing tests and we should be good to go.

fedeci force-pushed the option-availability branch from 3351ff8 to dad8af5 Compare December 1, 2020 20:46

profnandaa added needs-more-review 🕑 to-be-reviewed labels Dec 3, 2020

ezkemboi requested review from ezkemboi and tux-tn December 10, 2020 04:30

tux-tn reviewed Dec 15, 2020

View reviewed changes

tux-tn requested changes Oct 2, 2021

View reviewed changes

fedeci force-pushed the option-availability branch from dad8af5 to a6181a6 Compare October 2, 2021 11:52

tux-tn reviewed Oct 2, 2021

View reviewed changes

fedeci added 2 commits October 6, 2021 12:40

fix: add 'hasOption' function to prevent 'null' or 'undefined' option…

90267db

… values

test: add tests

68ecc1b

fedeci force-pushed the option-availability branch from a6181a6 to 68ecc1b Compare October 6, 2021 10:45

fedeci marked this pull request as draft October 6, 2021 10:46

fedeci force-pushed the option-availability branch from 153d641 to 3425af1 Compare October 6, 2021 13:27

Correctly update

d03e4bb

fedeci force-pushed the option-availability branch from 3425af1 to d03e4bb Compare October 6, 2021 13:51

fedeci commented Oct 6, 2021

View reviewed changes

fedeci marked this pull request as ready for review October 6, 2021 13:53

tux-tn mentioned this pull request Nov 16, 2021

Update build tooling and exports #1869

Draft

3 tasks

profnandaa approved these changes Dec 12, 2022

View reviewed changes

profnandaa added ready-to-land For PRs that are reviewed and ready to be landed and removed 🕑 to-be-reviewed labels Dec 12, 2022

profnandaa added 🧹 needs-update For PRs that need to be updated before landing and removed ready-to-land For PRs that are reviewed and ready to be landed labels Feb 1, 2023

	(!hasOption(options, 'min') \|\| value >= options.min) &&
	value >= (options.min ?? null) &&

Uh oh!

fix: add 'hasOption' function to prevent 'null' or 'undefined' option values #1542

Are you sure you want to change the base?

fix: add 'hasOption' function to prevent 'null' or 'undefined' option values #1542

Conversation

fedeci commented Nov 30, 2020

Checklist

Uh oh!

codecov bot commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tux-tn left a comment

Choose a reason for hiding this comment

Uh oh!

fedeci commented Dec 19, 2020

Uh oh!

timo92 commented Dec 20, 2020

Uh oh!

tux-tn left a comment

Choose a reason for hiding this comment

Uh oh!

fedeci commented Oct 2, 2021

Uh oh!

tux-tn left a comment

Choose a reason for hiding this comment

Uh oh!

tux-tn Oct 2, 2021

Choose a reason for hiding this comment

Uh oh!

fedeci Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

tux-tn Oct 2, 2021

Choose a reason for hiding this comment

Uh oh!

fedeci Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

fedeci commented Oct 6, 2021

Uh oh!

fedeci commented Oct 6, 2021

Uh oh!

tux-tn commented Oct 6, 2021

Uh oh!

fedeci commented Oct 6, 2021

Uh oh!

fedeci Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

fedeci Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

tux-tn commented Oct 6, 2021

Uh oh!

profnandaa left a comment

Choose a reason for hiding this comment

Uh oh!

profnandaa commented Dec 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 1, 2020 •

edited

Loading