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

v3 #259

Merged
merged 55 commits into from Mar 24, 2022
Merged

v3 #259

merged 55 commits into from Mar 24, 2022

Conversation

porsager
Copy link
Owner

@porsager porsager commented Jan 11, 2022

This is a complete rewrite to better support all the features that I was trying to get into v2. There are a few breaking changes from v2 beta , which some (myself included) are using in production, so I'm skipping a stable v2 release and going to v3.

Here are some of the new things available..

  • Converted to ESM with CJS support
  • Deno support
  • Dynamic query builder based on raw sql
  • Realtime subscribe to db changes through logical replication
  • Cursors as async iterators
  • Multi-host support for High Availability setups
  • Postgres input parameter types from ParameterDescription
  • .describe() to only get query input types and column definitions
  • Support for Large Objects
  • max_lifetime for connections
  • Cancellation of requests

Breaking changes from v2 -> v3

  • Cursors are always called with Result arrays (previously cursor 1 would return a row object, where > 1 would return an array of rows)
  • .writable() and .readable() is now async (returns a Promise that resolves to the stream)
  • Queries now returns a lazy promise instead of being executed immediately. This means the query won't be sent until awaited (.then, .catch, .finally is called) or until .execute() is manually called.
  • .stream() is renamed to .forEach
  • Returned results are now it's own Result class extending Array instead of an Array with extra properties (actually shouldn't be breaking unless you're doing something funny)
  • Parameters are now cast using the types returned from Postgres ParameterDescription with a fallback to the previously inferred types
  • Only tested with node v12 and up
  • Implicit array value to multiple parameter expansion removed (use sql([...]) instead)

Breaking changes from v1 -> v2 (v2 never moved on from beta)

  • All identifiers from sql() in queries are now always quoted
  • Undefined parameters are no longer allowed
  • Numbers that cannot be safely cast to JS Number are returned as string. This happens for eg, select count(*) because count() returns a 64 bit integer (int8), so if you know your count() won't be too big for a js number just cast in your query to int4 like select count(*)::int

Fixes #12, Fixes #30, Fixes #63, Fixes #65, Fixes #67, Fixes #89, Fixes #156, Fixes #159, Fixes #179, Fixes #201, Fixes #221, Fixes #230, Fixes #234, Fixes #248, Fixes #250, Fixes #252, Fixes #254
Closes #98, Closes #101, Closes #231, Closes #233

@porsager porsager marked this pull request as ready for review January 14, 2022 08:34
@porsager
Copy link
Owner Author

I think the only thing left now is some missing and better documentation for the new features. Would be really great if anyone wants to help with that, since I'm not gonna be able to find a lot of time the next couple of weeks..

@dilan-dio4
Copy link
Contributor

@porsager how does the new "dynamic query builder based on raw sql" work?

I'm very excited about that feature. Thanks!

@porsager
Copy link
Owner Author

porsager commented Feb 5, 2022

It's basically the most simply building block I could come up with for composing static sql safely into queries. I'm also very excited to see what people will come up with using it.

The sql`` instance can be used as a value inside a query to compose sql queries using safe static strings.

I've also chosen this way to stay as close as possible to actual SQL.

Here are some examples for what you can do:

// Using sql functions dynamically
sql` 
  update x set updated_at = ${ now ? sql`now()` : someDate }
`

// Dynamic filters (eg where, and / or usage)
sql` 
  select 
    * 
  from x ${ id 
    ? sql`where user_id = ${ id }` 
    : sql`` 
  }
`

// Complete queries using with 
const other = sql`
  select 1 as a
`

sql`
  with xs as (
    ${ other }
  )
  select * from xs
`

Now this should hopefully highlight that you can replace these with your own helper function(s) which should give you all the options you'd like. Now even though it does prevent sql injection of any kind, it doesn't mean you can't accidentally expose ways to access you database that was not intended, if you make any conditional query building available to users.

I hope this gives enough info to go on :)

@dilan-dio4
Copy link
Contributor

Okay this is great. I'm going to explore more later today.

One more thing. I was having trouble reverse engineering where in code this takes place. Could you point me to where a dynamic query is recognized and how it is parsed?

@porsager
Copy link
Owner Author

porsager commented Feb 5, 2022

Sure thing :) This recursive thing does the work

value instanceof Query ? fragment(string, value, parameters, types) :

@dilan-dio4
Copy link
Contributor

Awesome @porsager makes sense now.

@dilan-dio4
Copy link
Contributor

Here's what a few things needed for updated documentation: dynamic queries (!), .describe, and async cursors.

With that I think we can simplify the structure of the README. There's a lot of stuff at the top that would serve better later in the document as being for advanced usage. I think there's an opportunity to make it easier for people get started with a simplified structure.

I propose a structure like this:

  • Getting started
    • Installation
    • Usage
  • Basic connection options
  • Queries (this header will be for the basics of using sql``)
    • Select
    • Insert
      • Multiple inserts with array of objects
    • Update
    • Delete
  • Dynamic Queries
    • Usage
      • Select, Insert, Update, Delete
    • Identifier names (sql(name))
    • Helpers (.json, arrays)
  • Advanced iteration methods
    • .forEach
    • .cursor
    • .describe
    • .raw
    • .unsafe (?)
  • Advanced communication
    • Listen and notify
    • .file
    • .subscribe
    • Transactions
    • .begin
    • .savepoint
  • Custom types
  • postgres connection options
    • .postgres({ options }) full options object
    • SSL
    • Multi host connection - HA
    • Auto fetching of array types
    • Environment Variables for Options
    • The connection pool
  • Error handling
  • TypeScript support

What's your thoughts on something like this @porsager ?

@porsager
Copy link
Owner Author

porsager commented Feb 5, 2022

Wow! That looks like a great start @dilan-dio4 , and I think the order and grouping you've come up with looks very nice!

Some notes:

  • Perhaps "Advanced iteration methods" should be "Advanced query methods"
  • sql.json helper is almost useless in v3 since you can pass json directly
  • I like the position of unsafe, and then keep the collapsed version
  • .file and transactions should probably go under "Advanced query methods"

I'll try to write the updated section for the connection pool description.

* Cleaned up some language in README

* README update

* Moved sections to advanced query methods

* cursor to async iterators

* sql array removed parenthesis

* #264 (comment) 2 and 3
@porsager
Copy link
Owner Author

I feel the docs are ready for release now. If anyone is up for it, I would love some feedback or just if you could read through them at https://github.com/porsager/postgres/tree/rewrite and check for any errors or missing details.

@porsager
Copy link
Owner Author

Also just published an RC version to npm as well https://www.npmjs.com/package/postgres/v/3.0.0-rc.1

@s0xDk
Copy link

s0xDk commented Mar 21, 2022

bytea literals like '\\xDEADBEEF' are not working anymore.
Switched to Buffer.from('DEAFBEEF', 'hex'), but it's not quite succinctly.
Any other options? 🤔

BTW, thank you very much for crafting this beautiful piece of software! 🙏

@porsager
Copy link
Owner Author

@s0xDk Hey, thanks a lot!

Compared to v1?
Would you mind posting a quick sample how you used that before?

@s0xDk
Copy link

s0xDk commented Mar 21, 2022

@porsager

having, for example, hex-encoded binary data:

const data = 'BAD455'

In v1 and v2.0.0-beta.11 I used:

await sql`INSERT INTO t SET b = ${`\\x${data}`}` // or
await sql`INSERT INTO t SET b = ${'\\x'+data}`

In v3 that doesn't work. It's treating my already hex-encoded data as plain string \xBAD455, so I have to use:

await sql`INSERT INTO t SET b = ${Buffer.from(data, 'hex')}`

@s0xDk
Copy link

s0xDk commented Mar 21, 2022

Sometimes the following error throws up:

file://.../node_modules/postgres/src/connection.js:148
      return q.reject(Errors.connection('CONNECTION_DESTROYED', options))
                             ^

Error: write CONNECTION_DESTROYED /var/run/postgresql/.s.PGSQL.5432
    at Object.execute (file://.../node_modules/postgres/src/connection.js:148:30)
    at go (file://.../node_modules/postgres/src/index.js:348:14)
    at Promise.handler (file://.../node_modules/postgres/src/index.js:336:14)
    at Promise.handle (file://.../node_modules/postgres/src/query.js:128:65)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  code: 'CONNECTION_DESTROYED',
  errno: 'CONNECTION_DESTROYED',
  address: '/var/run/postgresql/.s.PGSQL.5432'
}

Node.js v17.7.2

But I don't use query timeouts, nor terminate any connections manually.

@porsager
Copy link
Owner Author

In v1 and v2.0.0-beta.11 I used:

await sql`INSERT INTO t SET b = ${`\\x${data}`}` // or
await sql`INSERT INTO t SET b = ${'\\x'+data}`

Right, that's because v1 and v2 didn't know b was of type bytea, so the default serialisation was used (string). In v3 types are used so the serialization defined as serialize: x => '\\x' + Buffer.from(x).toString('hex') for bytea will not work this way.

I unfortunately can't think of a nice way we can make both ways work. It's too lose to check if it's a string and if it starts with \\x, and it's also a bit weird if we expect strings to be hex.. I'm open to ideas...

@porsager
Copy link
Owner Author

Error: write CONNECTION_DESTROYED /var/run/postgresql/.s.PGSQL.5432

That is probably related to the new max lifetime, and the fact that a terminated connection is not removed immediately. Am I correct if this first comes after the service has run for 45 minutes?

Would you mind trying the latest in #rewrite ?

@s0xDk
Copy link

s0xDk commented Mar 21, 2022

I unfortunately can't think of a nice way we can make both ways work. It's too lose to check if it's a string and if it starts with \\x, and it's also a bit weird if we expect strings to be hex.. I'm open to ideas...

something like this, maybe?

serialize: x => x.startsWith('\\x') ? x : '\\x' + Buffer.from(x).toString('hex')

Or even more strict approach:

serialize: x => x.match(/^\\x[0-9a-f]+$/i) ? x : '\\x' + Buffer.from(x).toString('hex') // or
serialize: x => x.match(/^\\x\p{Hex_Digit}+$/u) ? x : '\\x' + Buffer.from(x).toString('hex')

@s0xDk
Copy link

s0xDk commented Mar 21, 2022

That is probably related to the new max lifetime, and the fact that a terminated connection is not removed immediately. Am I correct if this first comes after the service has run for 45 minutes?

Yes, it is. Sometimes it goes through that point without any issues, sometimes not.

Will try the #latest, thank you!

@porsager
Copy link
Owner Author

serialize: x => x.startsWith('\\x') ? x : Buffer.from(x).toString('hex')

Or even more strict approach:

serialize: x => x.match(/^\\x[0-9a-f]+$/i) ? x : Buffer.from(x).toString('hex') // or
serialize: x => x.match(/^\\x\p{Hex_Digit}+$/u) ? x : Buffer.from(x).toString('hex')

Yeah, that's what I think is too loose. There's a really good chance no one ever will be bitten by it, but I wouldn't want to be responsible for it if so..

@porsager
Copy link
Owner Author

Will try the #latest, thank you!

Cool.. Let me know if it's good / bad.

* fixed docs typos + minor cleanup

* image alignment fix

Co-authored-by: s13k <s13k@pm.me>
@s0xDk
Copy link

s0xDk commented Mar 21, 2022

Cool.. Let me know if it's good / bad.

Something new happened:

node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^
Error: connect EISCONN /var/run/postgresql/.s.PGSQL.5432
    at PipeConnectWrap.afterConnect [as oncomplete] (node:net:1187:16)
    at cachedError (file://.../node_modules/postgres/src/query.js:158:23)
    at new Query (file://.../node_modules/postgres/src/query.js:36:9)
    at sql (file://.../node_modules/postgres/src/index.js:98:11)
    at my_func (file://.../my_script.js:392:25)

Node.js v17.7.2

@porsager
Copy link
Owner Author

@s0xDk Really nice feedback! There was indeed a race condition on connection end for transactions. It should be fixed now.

@porsager porsager merged commit a9bfd19 into master Mar 24, 2022
@porsager porsager deleted the rewrite branch March 24, 2022 16:02
@karlhorky
Copy link
Contributor

karlhorky commented Mar 25, 2022

Thanks for merging and publishing v3 on npm @porsager, congratulations!! 🎉 🎊

Thanks again for your tireless work on this project, really amazing!! 🙌

@porsager
Copy link
Owner Author

Thanks @karlhorky ! :) I hope it'll be put to good use ;)

@arxpoetica
Copy link

AMAZING!!! YOU PUBLISHED v3!!! Can't wait to try it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment