pg error code helpers

[jmhodges]

I've written something like

const uniquenessViolation = pq.ErrorCode("23505")
...
if pgerr, ok := err.(*pq.Error); ok {
   if pgerr.Code == uniquenessViolation {
       return ErrSomeSpecificErrorForAPINiceness
   }
}

a few times in various packages.

Maybe I'm doing something wrong by doing this, but it might be nice to have some funcs named things like pq.IsUniquenessViolation(error) to help out on this sort of thing. It seems like we could generate them once from the errorCodeNames maps (while handling the "000" class cases by ignoring them or checking for the class or whichever way you'd like).

Does this sound like a patch you'd accept?

[johto]

Maybe I'm doing something wrong by doing this, but it might be nice to have some funcs named things like pq.IsUniquenessViolation(error) to help out on this sort of thing.

I don't like that idea very much.

What I'd consider instead is something like:

func IsPqErrorCode(err error, errcode string) bool

and then you'd do:

if pq.IsPqErrorCode(err, "unique_violation") {
    // whatever
}

It might also make sense to add a variant which returns a *pq.Error or nil depending on whether it's that error code or not; the boolean version is trivial to build on top of that.

[jmhodges]

Ah, shoot. I'm not a fan of those options. Being stringly-typed like that would make it too easy for me to typo. There's precedent with the boolean version in os.IsExists and such.

Thanks for responding, anyhow!

[freeformz]

@johto I agree with @jmhodges There is precedence with such things as os.IsExists and os.IsNoExist. If the strings in your example were expressed as constants, maybe I wouldn't be -1.

Another option is to possibly add methods to the errors and then you could assert against an interface (either defined by pg or the developer using pq).

[johto]

@johto I agree with @jmhodges There is precedence with such things as os.IsExists and os.IsNoExist

Yeah, there appears to be around three of them in the "os" package. But there's what, a hundred different error codes and/or classes? Surely at that point you'll realize that this wasn't such a good idea. Now maybe you could restrict it to a subset of the error codes and/or classes, but which subset? Who are we going to disappoint?

If you're really so scared of typos, you could make them obvious by generating a reverse errorCodeNames map and panicing if the input string is not a part of that map. It would still be a runtime error instead of a compile-time error, but it'd at least be obvious if you tested your code at all.

One other option would be to put all codes into a separate package as an enum; that way you'd get compile time errors as well, if you wanted. I don't think that's a bad idea, but I'm not sure it's worth it, either.

[freeformz]

I didn't realize there would be a hundred, so yeah, generating methods for each isn't all that nice.

We could do something like (which is what you are suggesting more or less):

// errors.go
package pq

const (
  ...
  UniqueViolationError = pq.ErrorCode("23505") // 'unique_violation'
  ...
  SchemaAndDataStatementMixingNotSupportedError = pq.ErrorCode("25007") // 'schema_and_data_statement_mixing_not_supported'
  ...
)

// IsErrorCode a specific postgres specific error as defined by 
// https://www.postgresql.org/docs/9.5/static/errcodes-appendix.html
func IsErrorCode(err error, errcode pq.ErrorCode) bool {
  if pgerr, ok := err.(*pq.Error); ok {
     return pgerr.Code == errcode
  }
  return false
}

// User code
if pq.IsErrorCode(err, pq.UniqueViolationError) {
    // do something app specific
}

[johto]

We could do something like (which is what you are suggesting more or less):

Yeah. Though I'd personally prefer a separate package instead of the Error suffix, unless there's a really good reason not to do that.

[freeformz]

Re: package/errors suffix

Mostly because github.com/lib/pq/errors ends up being imported and used as "errors", which means it will almost always be aliased. It could be pqerrors, but then there is stutter in the full name: github.com/lib/pq/pqerrors. I also feel that the IsErrorCode function should be in the same package as the enums, although I'm not really sure why I feel that. Also seems a little strange for the enums to be in a different package than pq.Error.

[freeformz]

They should also probably be ErrUniqueViolation and Err SchemaAndDataStatementMixingNotSupported instead as per general go naming standards.

[omeid]

I have just created a package to handle error codes, it takes care of multiple error codes for the same error "message/type", I am happy to contribute it upstream if it seems fit.

The package is here pgerror.

[arp242]

#1275 adds a list of error codes in a new pqerror package. This also adds pqerror.As():

pqErr := pqerror.As(err, pqerror.UniqueViolation)
if pqErr != nil {
	log.Fatalf("email %q already exsts", email)
}

Although you can als use pqErr, ok := errors.AsType[*pq.Error](err), but this is a bit less of a mouthful.

It could be pqerrors, but then there is stutter in the full name

I agree this isn't great, but adding all the error codes to the pq package really clutters things like code completion and godoc. So not a fan of that either.

I'm not sure that putting everything under pqerror like #1275 does is right either, which is why I haven't merged it. Maybe it should just be a pqerror or pqcode package with the error codes. That's what I started with but then I didn't like how pq.As() isn't all that obvious (pqerror.As() is nicer). But to make pqerror.As() work you need to put everything in pqerror to prevent import cycles.

Need to think about it. Suggestions welcome.