encore icon indicating copy to clipboard operation
encore copied to clipboard

encore.dev/storage/sqldb does not expose PostgreSQL error codes

Open eandre opened this issue 2 years ago • 9 comments

In order to detect various PostgreSQL errors it would be great if Encore's sqldb package exposed the underlying error code.

Workarounds exist (see #117) but they rely on the underlying database driver being jackc/pgx. Encore should expose these error codes natively through the sqldb package so that applications do not need to poke at the underlying database driver.

eandre avatar Sep 23 '22 13:09 eandre

@eandre I'm poking around looking for a potential hacktoberfest issue. I've read the contribution guidelines but, I didn't see a section on how your team assigns issues to contributors. Do you request that we just open a PR and tag the issue or is there an assignment process?

ElliottDenlinger avatar Sep 28 '22 16:09 ElliottDenlinger

Hey @ElliottDenlinger, great question! For this issue, since it requires some design work (designing what the API should look like, before coding up the implementation) it's probably easiest to first assign the issue so we don't have multiple people scrambling to take on the same task.

If you're interested in working on this let me know and we can assign this to you :) Thank for your expressing interest!

eandre avatar Sep 28 '22 16:09 eandre

@eandre I'd love to at least take a crack at it. I've not read any of the encore source other than a quick skim this morning so we'll see if I'm in over my head or not :D.

I'm not too familiar with your current strategy for chaining errors. Did you have any immediate thoughts on what you were hoping to see as a way to expose the codes? I'll go spelunking through the way you are currently wrapping / converting errors and see if anything comes to mind.

ElliottDenlinger avatar Sep 28 '22 21:09 ElliottDenlinger

I've had a change of plans come up for the next few weeks in the time since we started discussing my taking the issue. Others should feel free to ask for this to be assigned to them.

ElliottDenlinger avatar Oct 14 '22 19:10 ElliottDenlinger

@eandre I'll take a look at this over the weekend if no one else is currently working on it.

ronaudinho avatar Oct 15 '22 09:10 ronaudinho

Sure, thanks @ronaudinho!

eandre avatar Oct 15 '22 11:10 eandre

did a bit of digging and so far I see several ways to go about this:

  1. wrapping pgconn.Error as an error type that implements errs.ErrDetails interface -> would require two types assertion to unwrap as I see it, which of course can be done in a new convenient function
  2. if it is for internal use, perhaps in can be included Meta field inside errs.Error -> as I understand it, the underlying error code is to be exposed, so this does not seem right
  3. add a field in errs.Error for this -> simplest, but feels weird to me whichever chosen process would be done in convertErr and personally, I lean towards option 1, but it seems rather complex and perhaps I am missing something, thoughts?

in terms of exposing the error codes, the options i can think of are:

  1. adding them in the errs package or perhaps in this sqldb (with some codegen to avoid repetitive works of adding new error codes perhaps)
  2. let user get them from pgerrcode
  3. leaving it as is and let user do what they want with it

ronaudinho avatar Oct 19 '22 16:10 ronaudinho

The errs package is primarily for API errors, and while the sqldb errors currently return *errs.Error objects I think that was a design mistake.

My gut feeling is that we should add an *sqldb.PgError type similar to https://pkg.go.dev/github.com/jackc/pgconn#PgError (but we can leave out a bunch of the details for now, I think). I don't think we need to define all the error code constants; they're already well-documented and won't change.

Thoughts on this approach?

eandre avatar Oct 19 '22 16:10 eandre

sounds about right to me. to clarify, does this mean changing the type of error returned by convertErr function from *errs.Error into this new *sqldb.PgError?

ronaudinho avatar Oct 19 '22 17:10 ronaudinho