-
Notifications
You must be signed in to change notification settings - Fork 301
Numeric options should be converted to Number
before being used
#622
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
Labels
enhancement
New feature or request
Comments
Hey, that's a great find! I'll get that fixed - Thank you |
porsager
added a commit
that referenced
this issue
Jul 2, 2023
porsager
added a commit
that referenced
this issue
Jul 5, 2023
* Initial support for cloudflare * Types here are not needed * Include cloudflare in npm * Allow crypto to be async to support WebCrypto polyfills * Polyfill crypto with WebCrypto for cloudflare * Use crypto polyfill for cloudflare * Not ready for tests on CF yet * build * build cf * build * README.md - improve the "Multiple statements in one query" section - add links for the official documentation - escape the backtick character - change the subtitle to "await sql``.simple()" instead of "await sql`select 1; select 2;`.simple()" (to be coherent with the other subtitles) - add a small example below * Ensure number options are coerced from string - fixes #622 * Add sql.reserve method * build * create beginPrepared function (#628) * create beginPrepared function * change implementation to new method * add prepare method type to TransactionSql * add documentations and test * fix test * enable prepared transactions in the bootstrap script * enable prepared transactions in the github actions setup file * fix github actions * fix github actions yml file * Please the linter * build * Fix for using compatibility_flags = [ "nodejs_compat" ] instead * build * please eslint * draft: Cloudflare works ! 🎉 (#618) * Reworked from source cloudflare branch feat: reran transpile fix linter feat: final touches + test files squashed 2 commits fix: Polyfills bulk (to please linter) fix: Removed MD5 + put back SHA in the digest() squashed 5 commits fix: cloudflare workers deployment feat: fixed auth fix: encrypt not found in worker :( fix: postgres SASL fix: linting * fix: merge cleanup --------- Co-authored-by: wackfx <[email protected]> * Switch to performance.now * Please the linter * Don't collect polyfills (keep line numbers similar to src) * Simplify manual test script * build --------- Co-authored-by: Paulo Vieira <[email protected]> Co-authored-by: Shayan Shojaei <[email protected]> Co-authored-by: Wack <[email protected]> Co-authored-by: wackfx <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Example 1:
Supposed I have
and PGMAXCONNECTIONS is set as
Then we don't get 20 connections as expected, but only 1! The problem is that
options.max
will be the string"20"
instead of the number20
, so the logic to create the array of connections won't work:Example 2:
This kind of issue might also happen with the
idle_timeout
,max_lifetime
orconnect_timeout
options. They end up being used in thetimer
utility, inconnection.js
(which callssetTimeout
). A numeric string will work well becausesetTimeout
will coerce the string to a number. However suppose there is some typo like this:# notice the comma after 5 PGIDLE_TIMEOUT=60, node index
When the numeric string can't be coerced to a number,
setTimeout
will still work but using 0 instead! This will obviously cause unexpected behaviors. In this case we would never see any idle connections when inspectingpg_stat_activity
.There might be other options where this sort of problems happen, I didn't check all of them.
Overall I think this kind of issues could be easily catched if some simple validations were done when parsing the options.
Thanks.
The text was updated successfully, but these errors were encountered: