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

bugfix: env min/max input validation #465

Merged
merged 9 commits into from
Oct 7, 2020
Merged

bugfix: env min/max input validation #465

merged 9 commits into from
Oct 7, 2020

Conversation

xninjax
Copy link
Contributor

@xninjax xninjax commented Oct 7, 2020

Description

This was originally going to be a bug report, instead submitting PR with possible solution.

ISSUE
When supplying only one of a min/max pair from .env, undesirable results can occur in code.
I discovered this when setting only PAGE_SLEEP_MIN="60000" ... resulting in the page hits to occur more often than 60sec.

image
image

Why?
The culprit
getSleepTime() { return config.browser.minSleep + (Math.random() * (config.browser.maxSleep - config.browser.minSleep)); }

Since PAGE_SLEEP_MAX defaults to "10000"

getSleepTime() { return 60000 + (Math.random() * (10000 - 60000)); }

We're applying a negative number to the Math.random result ☝️ . Thus subtracting from the original 60sec delay.

Possible solution

... Math.random() * Math.abs(config.browser.maxSleep - config.browser.minSleep)) ...

Math.abs... is one possible fix, but doesn't really get the job done properly IMO. Would still need to think about how to address this for PAGE_BACKOFF_MIN/MAX. And again for all future min/max pairs.

Instead, I propose MIN/MAX input validation (subject of this PR).

function envOrNumberMinMax
  Returns environment variable, given number, or default number,
  while handling .env input errors for a Min/Max pair.
  .env errors handled:
  - Min/Max swapped (Min larger than Max, Max smaller than Min)
  - Min larger than default Max when no Max defined
  - Max smaller than default Min when no Min defined

Testing

Input validation applied...
With PAGE_SLEEP_MIN="60000" entered as before, and appling input validation, results are now as expected.
image
image

Other Scenario's
image
image

The following works because input validation swaps min/max first (not well illustrated, but was verified)
image
image

@xninjax xninjax requested a review from jef as a code owner October 7, 2020 01:13
@xninjax
Copy link
Contributor Author

xninjax commented Oct 7, 2020

linter is not liking the single line 'if' statements, and I knew it would complain, since local checks warned me too.

I can expand, but will look more of a mess 🤷‍♂️

@jef
Copy link
Owner

jef commented Oct 7, 2020

getSleepTime() { return 60000 + (Math.random() * (10000 - 60000)); }

Ahhhhhh... Good find! Definitely want to bring this in.

Yeah, if you don't mind running npm run lint:fix. Keeps it consistent.

Thanks!

Comment on lines +69 to +80
case 'min':
if (enviromentMin && enviromentMax) {
return Number(Number(enviromentMin) < Number(enviromentMax) ? enviromentMin : enviromentMax);
}

if (enviromentMax) {
return Number(enviromentMax) < (number ?? 0) ? Number(enviromentMax) : (number ?? 0);
}

if (enviromentMin) {
return Number(enviromentMin);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All expanded so linter is happy 😄

Copy link
Owner

@jef jef left a comment

Choose a reason for hiding this comment

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

Great! Thank you very much for doing this. I really appreciate it. This will hopefully clear up some issues.

@jef jef merged commit 7274ead into jef:main Oct 7, 2020
@xninjax xninjax deleted the xninjax/min-max-bugfix branch October 7, 2020 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants