-
-
Notifications
You must be signed in to change notification settings - Fork 135
feat: error translator support #162
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
Conversation
error_translator.go
Outdated
} | ||
|
||
func (dialector Dialector) Translate(err error) error { | ||
parsedErr, marshalErr := json.Marshal(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to assert the error type for performance? e.g:
pgErr, ok := err.(*pgconn.PgError)
And use JSON marshal/unmarshal only when failing to assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe since it is driver related native error, only checking the PgError is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but it might be broken if using a different pg driver... but anyway, both are fine for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var p *pgconn.PgError
if errors.As(err, &p) && p.Code == "23505" {
if strings.Contains(p.Detail, "email") {
return nil, errno.DBError(errno.ErrEmailExist, "注册商户失败")
}
if strings.Contains(p.Detail, "phone") {
return nil, errno.DBError(errno.ErrMerchantPhoneExist, "注册商户失败")
}
}
How should I tell which key is conflicting in the new version ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact I think this has been breaking API, The original code cannot be reached after updating the new version,Resulting in users not knowing whether the mailbox is duplicated or the phone number is duplicated,So I use v1.4.8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can enhance the error details, @jinzhu what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 @Naist4869. Previously, I could parse pgConn.pgError for the constraint name and handle each constraint violation differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agree this is a breaking API, the good news we still haven't released a new version for GORM, so these changes won't affected users unless they are using the latest master.
Maybe we need to add a configuration TranslateError
to GORM Config, https://github.com/go-gorm/gorm/blob/master/gorm.go#L20, by default we don't translate the error.
@saeidee do you feel good to make the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can implement the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the PR go-gorm/gorm#6178
994883f
to
13e563b
Compare
What did this pull request do?
This PR adds support for translating Postgres errors like unique key violation to native gorm errors (ErrDuplicatedKey)
Related PR in gorm repo: go-gorm/gorm#6004