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

Numeric options should be converted to Number before being used #622

Closed
paulovieira opened this issue Jun 27, 2023 · 1 comment
Closed
Labels
enhancement New feature or request

Comments

@paulovieira
Copy link
Contributor

Example 1:

Supposed I have

// at index.js

let sql = Postgres({ max: process.env.PGMAXCONNECTIONS });

and PGMAXCONNECTIONS is set as

PGMAXCONNECTIONS=20 node index.js

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 number 20, so the logic to create the array of connections won't work:

const connections = [...Array(options.max)].map(...)

Example 2:

This kind of issue might also happen with the idle_timeout, max_lifetime or connect_timeout options. They end up being used in the timer utility, in connection.js (which calls setTimeout). A numeric string will work well because setTimeout 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 inspecting pg_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.

@porsager
Copy link
Owner

porsager commented Jul 1, 2023

Hey, that's a great find! I'll get that fixed - Thank you

@porsager porsager added the enhancement New feature or request label Jul 1, 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 <hello@wackfx.com>

* 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 <paulovieira@gmail.com>
Co-authored-by: Shayan Shojaei <68788931+shayan-shojaei@users.noreply.github.com>
Co-authored-by: Wack <135170502+wackfx@users.noreply.github.com>
Co-authored-by: wackfx <hello@wackfx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants