feat: include pg message in errors

[mmkal]

Fixes #233

[mmkal]

Any thoughts on getting rid of the "Query violates a unique integrity constraint" part completely? We'd still get the type safety by having the UniqueIntegrityConstraintViolationError class, and the other part conveys the same information, but a bit more helpfully/specifically.

[gajus]

I am of the inverse opinion that we shouldn't include: duplicate key value violates unique constraint "person_pkey" part.

I am generally of opinion that error messages should be static, and that details such as "which constraint" belong as properties of the error.

[coveralls]

Coverage decreased (-0.07%) to 87.749% when pulling 47050e6 on mmkal:original-error-message into b06f72b on gajus:master.

[gajus]

What's the reason for wanting to include the original error message?

[gajus]

I've set a reminder to comeback to this PR tomorrow.

My main concern is that there is no guarantees that pg will remain the underlying driver for Slonik. Therefore, exposing errors like this creates unnecessary tight coupling and (future) breaking change.

[mmkal]

There's always a tradeoff between error specificity and stability. I think of Error messages as mostly for humans. So they should be short, predictable and understandable, but also deliver an appropriate amount of information. In general I find postgres does a good job finding that balance.

Right now slonik abstracts away the slightly more precise postgres message. To take the example from the linked issue:

let slonik = createPool(...)
await slonik.query(sql`create table t1(id int unique, name text unique)`)
await slonik.query(sql`insert into t1 values(1, 'a')`)
await slonik.query(sql`insert into t1 values(2, 'a')`).catch(e => console.log(e.message))

In the above code, we'll only ever see "Query violates a unique integrity constraint.", whereas with the original message, we'd see "duplicate key value violates unique constraint "t1_name_key". With the original error message, you can see at a glance which table and column is the root of the problem. Of course, e.originalError already has that information, but e.originalError is often not present:

• At 3am when a developer gets paged. The alert says "Query violates a unique integrity constraint.". Where do they start looking?
• At 3:05am when they look at Sentry. All of the "Query violates a unique integrity constraint." errors are grouped together, including totally unrelated ones going back days, weeks or months, because Sentry uses a fairly naive algorithm for grouping. The increasingly panicked developer now doesn't know when this issue started happening and needs to dive deep into logs to (hopefully) get a better sense.
• When using command-line tools like ts-command-line. By default, these only show the error message (no stack, and no attached properties). If you're lucky, when you rerun the command with some extra logging, the error will occur again. If it was a race condition or intermittent bug, you might be out of luck.
• In code like the above, where something is non-critical, so the team write an overly-permissive .catch and only log the error message, not all the properties associated (in this case, they shouldn't do that. But when they do, the only clue to go on will be the generic error).
• Jest. The default reporter only shows the top line of the error message. It looks like this for me. It'd be much quicker to debug if it had the original error message:

As for coupling - you're right, there's somewhat increased coupling - but isn't it to postgres itself, rather than pg? I believe pg is just surfacing postgres error messages. I wouldn't necessarily advocate for using the original error message unless postgres was such a solid, battle-tested system with very well-designed error messages. And if properly documented, a driver change wouldn't need to be considered breaking - using Error subclasses enables if (e instanceof UniqueConstraintError) { ... }, and .toString() will include the class name in the stack/message, so if the convention was to rely on that, it'd still be possible to correlate errors from another driver if desired.

[mmkal]

@gajus bump on this. If you want to keep the errors as they are, what if I made it configurable? Right now there's no way as a user for me to switch to the original error message. I could add a function WrappedPGError.getMessage(originalError, message) which just returns message in slonik, but a downstream user would be able to override the implementation of getMessage to return originalError.message, or originalError.code + originalError.message, or whatever they wanted, at their own risk.

Open to that idea?

[gajus]

Thank you for bumping the conversation.

Your arguments make sense.

I have pushed the update.

[gajus]

🎉 This PR is included in version 23.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀