Implement a PoolError with dedicated codes

[rmedaer]

Previously the Pool was throwing built-in JS Error. It's therefore difficult to identify each kind of error.
This patch allow the developer to identify each type of error raised by the Pool with a PoolError.

Each PoolError has its own code and a message (original message for backward compatibility).

Codes and errors are documented in README file.

[charmander]

• How about meaningful text like the error codes in the Node standard library?
• I’m not sure if most of these deserve their own code. Nobody should be handling library misuse like multiple release, multiple end, use after end, or type errors programmatically. “Timeout exceeded when trying to connect” makes sense, though.
• throwed → thrown

[rmedaer]

How about meaningful text like the error codes in the Node standard library?

Current implementation mimic the NoticeOrError interface from pg-protocol, cf this message

Nobody should be handling library misuse like multiple release, multiple end, use after end, or type errors programmatically.

That's why it raises exceptions errors.

“Timeout exceeded when trying to connect” makes sense, though.

That's actually the only one I want to catch for now. For instance in a HTTP API, my goal is to return (a more) meaningful message to the client (in this case a 503 which means that the service is unavailable for now).

[charmander]

I don’t think mimicking the format of PostgreSQL error codes (with values that don’t come from PostgreSQL) is useful. https://nodejs.org/api/errors.html#nodejs-error-codes

[charmander]

Putting Pool on itself doesn’t feel right pre-ESM, and it’s not easy to use via pg. Would this even need to be exported with non-colliding error codes like ERR_PG_POOL_…?

[rmedaer]

Do you mean: replacing PoolError by constants (for instance ERR_PG_POOL_CONNECT_TIMEOUT, ERR_PG_POOL_UNEXPECTED_CONDITION, ...) ?

My goal was to implement the same kind of structure than NoticeOrError interface from pg-protocol. What would be the PROs/CONs ?

[matthieusieben]

I believe what @charmander is trying to say, is that if, instead of error codes like Z0001, you used NodeJS like error codes, you would not need to do instanceof PoolError and you could simply perform a check like:

catch (err) {
  if (err.code === 'ERR_PG_POOL_UNEXPECTED_STATE') throw err // something's wrong
  else if (err.code === 'ERR_PG_POOL_CONNECTION_TIMEOUT') {} // handle overloaded database
  else {} // Probably not a pg-pool error
}

In this case, you would not even need to export PoolError at all.

Note that because of the way node_modules are structured, checking for instanceof might yield false negative in case pg-pool ends up twice in you node_modules hierarchy (blame this on NodeJS's ecosystem).

This also explain his other comment explaining that you don't need to explicit every possible error case. ERR_PG_POOL_UNEXPECTED_STATE + ERR_PG_POOL_CONNECTION_TIMEOUT (please find better names 😅) are probably enough.

I believe that his first comment is that this feels right in ESM:

export default Pool
export { Pool, PoolError }

but the following is weird in common-js (pre-ESM)

module.exports = Pool
module.exports.Pool = Pool

because it is the same as doing Pool.Pool = Pool which feels odd.

[sehrope]

If we're going to have custom Error types, it'd be nice to have a single hierarchy for all things pg and PoolError (or maybe "PgPoolError"?) would be a child of a more generic PgError.

It makes testing easier too as you can check for a specific type (ex: "PgScramHandshakeError") rather than patterns of message text and have generic error handling for all things DB related in app code.

[rmedaer]

If we're going to have custom Error types, it'd be nice to have a single hierarchy for all things pg and PoolError (or maybe "PgPoolError"?) would be a child of a more generic PgError.

Indeed, currently the PoolError I implemented inherits std Error. The errors coming from pg-protocol are also inheriting std Error. However they implement interface NoticeOrError which is really specific to postgresql protocol and its backend. I do not have so much information and btw the Pool is should not be so specific to postgresql backend. Indeed, as far as I know, postgresql is not aware if you use a Pool or not. I'm not sure we should "mix" errors from postgresql vs internal library errors.

Having said that, you maybe saw that I'm using the same code keyword in PoolError with the same pattern as postgresql error codes. As far as I know there is no reserved codes. I had to choose a "prefix": Z.

As a conclusion, I understand the goal of "a single hierarchy for all things pg and PoolError" and I would enjoy to implement it even if I think it's not really required. Could you help us on this @brianc ?