Refactor domain error to use errorId (#790)

* Refactor domain error to use errorId

* Wrap accepts options and uses New internally
pull/793/head
Louis Ruch 5 years ago committed by GitHub
parent b8a5b9adf1
commit d30dd0ff64
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -8,12 +8,12 @@ import (
"github.com/lib/pq"
)
// Op represents an operation (package.function).
// For example iam.CreateRole
type Op string
// ErrorId is an id that is unique to the error (not the instance of the error).
// The id should be generated using `make rand`, and used as the `errorid` param when calling `errors.New`
type ErrorId string
// Err provides the ability to specify a Msg, Op, Code and Wrapped error.
// Errs must have a Code and all other fields are optional. We've chosen Err
// Err provides the ability to specify a Msg, ErrorId, Code and Wrapped error.
// Errs must have a Code and ErrorId all other fields are optional. We've chosen Err
// over Error for the identifier to support the easy embedding of Errs. Errs
// can be embedded without a conflict between the embedded Err and Err.Error().
type Err struct {
@ -21,63 +21,78 @@ type Err struct {
// errorCodeInfo, which contains the error's Kind and Message
Code Code
// Unique ID used to identify the error being returned
ErrorId ErrorId
// Msg for the error
Msg string
// Op represents the operation raising/propagating an error and is optional
Op Op
// Wrapped is the error which this Err wraps and will be nil if there's no
// error to wrap.
Wrapped error
}
// New creates a new Err and supports the options of:
// WithOp - allows you to specify an optional Op (operation)
// WithMsg() - allows you to specify an optional error msg, if the default
// msg for the error Code is not sufficient.
// WithWrap() - allows you to specify
// an error to wrap
func New(c Code, opt ...Option) error {
func New(c Code, errorid ErrorId, opt ...Option) error {
opts := GetOpts(opt...)
return &Err{
Code: c,
Op: opts.withOp,
ErrorId: errorid,
Wrapped: opts.withErrWrapped,
Msg: opts.withErrMsg,
}
}
// Wrap creates a new Err, but preserves the Code of the original error being wrapped
func Wrap(e error, errorid ErrorId, opt ...Option) error {
var code Code
if err, ok := e.(*Err); ok {
// get code from wrapped error
code = err.Code
}
opt = append(opt, WithWrap(e))
return New(code, errorid, opt...)
}
// Convert will convert the error to a Boundary *Err (returning it as an error)
// and attempt to add a helpful error msg as well. If that's not possible, it
// will return nil
func Convert(e error) *Err {
func Convert(e error, errorid ErrorId) *Err {
if e == nil {
return nil
}
if err, ok := e.(*Err); ok {
return err
}
var pqError *pq.Error
if As(e, &pqError) {
if pqError.Code.Class() == "23" { // class of integrity constraint violations
switch pqError.Code {
case "23505": // unique_violation
return New(NotUnique, WithMsg(pqError.Detail), WithWrap(ErrNotUnique)).(*Err)
return New(NotUnique, errorid, WithMsg(pqError.Detail), WithWrap(ErrNotUnique)).(*Err)
case "23502": // not_null_violation
msg := fmt.Sprintf("%s must not be empty", pqError.Column)
return New(NotNull, WithMsg(msg), WithWrap(ErrNotNull)).(*Err)
return New(NotNull, errorid, WithMsg(msg), WithWrap(ErrNotNull)).(*Err)
case "23514": // check_violation
msg := fmt.Sprintf("%s constraint failed", pqError.Constraint)
return New(CheckConstraint, WithMsg(msg), WithWrap(ErrCheckConstraint)).(*Err)
return New(CheckConstraint, errorid, WithMsg(msg), WithWrap(ErrCheckConstraint)).(*Err)
default:
return New(NotSpecificIntegrity, WithMsg(pqError.Message)).(*Err)
return New(NotSpecificIntegrity, errorid, WithMsg(pqError.Message)).(*Err)
}
}
if pqError.Code == "42P01" {
return New(MissingTable, WithMsg(pqError.Message)).(*Err)
return New(MissingTable, errorid, WithMsg(pqError.Message)).(*Err)
}
}
if errors.Is(e, ErrRecordNotFound) {
return New(RecordNotFound, errorid, WithMsg(e.Error())).(*Err)
}
// unfortunately, we can't help.
return nil
}
@ -97,8 +112,8 @@ func (e *Err) Error() string {
return ""
}
var s strings.Builder
if e.Op != "" {
join(&s, ": ", string(e.Op))
if e.ErrorId != "" {
join(&s, ": ", string(e.ErrorId))
}
if e.Msg != "" {
join(&s, ": ", e.Msg)

@ -3,6 +3,7 @@ package errors_test
import (
"context"
stderrors "errors"
"fmt"
"testing"
"github.com/hashicorp/boundary/internal/db"
@ -14,6 +15,7 @@ import (
func Test_NewError(t *testing.T) {
t.Parallel()
testId := errors.ErrorId("testid")
tests := []struct {
name string
code errors.Code
@ -24,13 +26,12 @@ func Test_NewError(t *testing.T) {
name: "all-options",
code: errors.InvalidParameter,
opt: []errors.Option{
errors.WithOp("alice.Bob"),
errors.WithWrap(errors.ErrRecordNotFound),
errors.WithMsg("test msg"),
},
want: &errors.Err{
Op: "alice.Bob",
Wrapped: errors.ErrRecordNotFound,
ErrorId: testId,
Msg: "test msg",
Code: errors.InvalidParameter,
},
@ -39,14 +40,79 @@ func Test_NewError(t *testing.T) {
name: "no-options",
opt: nil,
want: &errors.Err{
Code: errors.Unknown,
ErrorId: testId,
Code: errors.Unknown,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert, require := assert.New(t), require.New(t)
err := errors.New(tt.code, tt.opt...)
err := errors.New(tt.code, testId, tt.opt...)
require.Error(err)
assert.Equal(tt.want, err)
})
}
}
func Test_WrapError(t *testing.T) {
t.Parallel()
testId := errors.ErrorId("testid")
testErr := errors.New(errors.InvalidParameter, "uniqueId")
tests := []struct {
name string
opt []errors.Option
err error
want error
}{
{
name: "boundary-error",
err: testErr,
opt: []errors.Option{
errors.WithMsg("test msg"),
},
want: &errors.Err{
Wrapped: testErr,
ErrorId: testId,
Msg: "test msg",
Code: errors.InvalidParameter,
},
},
{
name: "boundary-error-no-msg",
err: testErr,
want: &errors.Err{
Wrapped: testErr,
ErrorId: testId,
Code: errors.InvalidParameter,
},
},
{
name: "std-error",
err: fmt.Errorf("std error"),
want: &errors.Err{
Wrapped: fmt.Errorf("std error"),
ErrorId: testId,
Code: errors.Unknown,
},
},
{
name: "conflicting-with-wrap",
err: testErr,
opt: []errors.Option{
errors.WithWrap(fmt.Errorf("dont wrap this error")),
},
want: &errors.Err{
Wrapped: testErr,
ErrorId: testId,
Code: errors.InvalidParameter,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert, require := assert.New(t), require.New(t)
err := errors.Wrap(tt.err, testId, tt.opt...)
require.Error(err)
assert.Equal(tt.want, err)
})
@ -67,12 +133,12 @@ func TestError_Info(t *testing.T) {
},
{
name: "Unknown",
err: errors.New(errors.Unknown).(*errors.Err),
err: errors.New(errors.Unknown, "").(*errors.Err),
want: errors.Unknown,
},
{
name: "InvalidParameter",
err: errors.New(errors.InvalidParameter).(*errors.Err),
err: errors.New(errors.InvalidParameter, "").(*errors.Err),
want: errors.InvalidParameter,
},
}
@ -93,22 +159,27 @@ func TestError_Error(t *testing.T) {
}{
{
name: "msg",
err: errors.New(errors.Unknown, errors.WithMsg("test msg")),
err: errors.New(errors.Unknown, "", errors.WithMsg("test msg")),
want: "test msg: unknown: error #0",
},
{
name: "code",
err: errors.New(errors.CheckConstraint),
err: errors.New(errors.CheckConstraint, ""),
want: "constraint check failed, integrity violation: error #1000",
},
{
name: "op-msg-and-code",
err: errors.New(errors.CheckConstraint, errors.WithOp("alice.bob"), errors.WithMsg("test msg")),
want: "alice.bob: test msg: integrity violation: error #1000",
name: "id",
err: errors.New(errors.Unknown, "uniqueId"),
want: "uniqueId: unknown, unknown: error #0",
},
{
name: "id-msg-and-code",
err: errors.New(errors.CheckConstraint, "uniqueId", errors.WithMsg("test msg")),
want: "uniqueId: test msg: integrity violation: error #1000",
},
{
name: "unknown",
err: errors.New(errors.Unknown),
err: errors.New(errors.Unknown, ""),
want: "unknown, unknown: error #0",
},
}
@ -129,7 +200,8 @@ func TestError_Error(t *testing.T) {
func TestError_Unwrap(t *testing.T) {
t.Parallel()
testErr := errors.New(errors.Unknown, errors.WithMsg("test error"))
testId := errors.ErrorId("testid")
testErr := errors.New(errors.Unknown, testId, errors.WithMsg("test error"))
tests := []struct {
name string
@ -138,8 +210,14 @@ func TestError_Unwrap(t *testing.T) {
wantIsErr error
}{
{
name: "ErrInvalidParameter",
err: errors.New(errors.InvalidParameter, errors.WithWrap(errors.ErrInvalidParameter)),
name: "ErrInvalidParameterWithWrap",
err: errors.New(errors.InvalidParameter, testId, errors.WithWrap(errors.ErrInvalidParameter)),
want: errors.ErrInvalidParameter,
wantIsErr: errors.ErrInvalidParameter,
},
{
name: "ErrInvalidParameterWrap",
err: errors.Wrap(errors.ErrInvalidParameter, testId),
want: errors.ErrInvalidParameter,
wantIsErr: errors.ErrInvalidParameter,
},
@ -170,6 +248,7 @@ func TestError_Unwrap(t *testing.T) {
func TestConvertError(t *testing.T) {
t.Parallel()
testId := errors.ErrorId("testid")
const (
createTable = `
create table if not exists test_table (
@ -210,13 +289,13 @@ func TestConvertError(t *testing.T) {
e: &pq.Error{
Code: pq.ErrorCode("23001"),
},
wantErr: errors.New(errors.NotSpecificIntegrity),
wantErr: errors.New(errors.NotSpecificIntegrity, testId),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert, require := assert.New(t), require.New(t)
err := errors.Convert(tt.e)
err := errors.Convert(tt.e, testId)
if tt.wantErr == nil {
assert.Nil(err)
return
@ -234,7 +313,7 @@ func TestConvertError(t *testing.T) {
_, err = rw.Exec(ctx, insert, []interface{}{"alice", "dup coworker", nil})
require.Error(err)
e := errors.Convert(err)
e := errors.Convert(err, "")
require.NotNil(e)
assert.True(errors.Is(e, errors.ErrNotUnique))
assert.Equal("Key (name)=(alice) already exists.: integrity violation: error #1002: \nunique constraint violation: integrity violation: error #1002", e.Error())
@ -246,7 +325,7 @@ func TestConvertError(t *testing.T) {
_, err = rw.Exec(ctx, insert, []interface{}{"alice", nil, nil})
require.Error(err)
e := errors.Convert(err)
e := errors.Convert(err, "")
require.NotNil(e)
assert.True(errors.Is(e, errors.ErrNotNull))
assert.Equal("description must not be empty: integrity violation: error #1001: \nnot null constraint violated: integrity violation: error #1001", e.Error())
@ -258,7 +337,7 @@ func TestConvertError(t *testing.T) {
_, err = rw.Exec(ctx, insert, []interface{}{"alice", "coworker", "one"})
require.Error(err)
e := errors.Convert(err)
e := errors.Convert(err, "")
require.NotNil(e)
assert.True(errors.Is(e, errors.ErrCheckConstraint))
assert.Equal("test_table_five_check constraint failed: integrity violation: error #1000: \ncheck constraint violated: integrity violation: error #1000", e.Error())
@ -267,7 +346,7 @@ func TestConvertError(t *testing.T) {
assert, require := assert.New(t), require.New(t)
_, err := rw.Exec(ctx, missingTable, nil)
require.Error(err)
e := errors.Convert(err)
e := errors.Convert(err, "")
require.NotNil(e)
assert.True(errors.Match(errors.T(errors.MissingTable), e))
assert.Equal("relation \"not_a_defined_table\" does not exist: integrity violation: error #1004", e.Error())

@ -44,7 +44,9 @@ func TestError_IsUnique(t *testing.T) {
},
{
name: "wrapped-pq-is-unique",
in: errors.New(errors.NotUnique,
in: errors.New(
errors.NotUnique,
"",
errors.WithWrap(&pq.Error{
Code: pq.ErrorCode("23505"),
}),
@ -96,12 +98,14 @@ func TestError_IsCheckConstraint(t *testing.T) {
},
{
name: "ErrCodeCheckConstraint",
in: errors.New(errors.CheckConstraint),
in: errors.New(errors.CheckConstraint, ""),
want: true,
},
{
name: "wrapped-pq-is-check-constraint",
in: errors.New(errors.CheckConstraint,
in: errors.New(
errors.CheckConstraint,
"",
errors.WithWrap(&pq.Error{
Code: pq.ErrorCode("23514"),
}),
@ -160,12 +164,14 @@ func TestError_IsNotNullError(t *testing.T) {
},
{
name: "ErrCodeNotNull",
in: errors.New(errors.NotNull),
in: errors.New(errors.NotNull, ""),
want: true,
},
{
name: "wrapped-pq-is-not-null",
in: errors.New(errors.NotNull,
in: errors.New(
errors.NotNull,
"",
errors.WithWrap(&pq.Error{
Code: pq.ErrorCode("23502"),
}),

@ -16,10 +16,10 @@ func T(args ...interface{}) *Template {
switch arg := a.(type) {
case Code:
t.Code = arg
case ErrorId:
t.ErrorId = arg
case string:
t.Msg = arg
case Op:
t.Op = arg
case *Err: // order is important, this match must before "case error:"
c := *arg
t.Wrapped = &c
@ -78,7 +78,7 @@ func Match(t *Template, err error) bool {
if t.Msg != "" && t.Msg != e.Msg {
return false
}
if t.Op != "" && t.Op != e.Op {
if t.ErrorId != "" && t.ErrorId != e.ErrorId {
return false
}
if t.Kind != Other && t.Info().Kind != e.Info().Kind {

@ -10,6 +10,7 @@ import (
func TestT(t *testing.T) {
t.Parallel()
stdErr := stderrors.New("test error")
testId := ErrorId("testid")
tests := []struct {
name string
args []interface{}
@ -19,7 +20,7 @@ func TestT(t *testing.T) {
name: "all fields",
args: []interface{}{
"test error msg",
Op("alice.Bob"),
testId,
InvalidParameter,
stdErr,
Integrity,
@ -27,8 +28,8 @@ func TestT(t *testing.T) {
want: &Template{
Err: Err{
Code: InvalidParameter,
ErrorId: testId,
Msg: "test error msg",
Op: "alice.Bob",
Wrapped: stdErr,
},
Kind: Integrity,
@ -109,6 +110,7 @@ func TestTemplate_Info(t *testing.T) {
func TestTemplate_Error(t *testing.T) {
t.Parallel()
stdErr := stderrors.New("test error")
testId := ErrorId("testid")
tests := []struct {
name string
template *Template
@ -121,7 +123,7 @@ func TestTemplate_Error(t *testing.T) {
name: "all params",
template: T(
"test error msg",
Op("alice.Bob"),
testId,
InvalidParameter,
stdErr,
Integrity,
@ -140,6 +142,7 @@ func TestTemplate_Error(t *testing.T) {
func TestMatch(t *testing.T) {
t.Parallel()
stdErr := stderrors.New("test error")
testId := ErrorId("testid")
tests := []struct {
name string
template *Template
@ -149,7 +152,7 @@ func TestMatch(t *testing.T) {
{
name: "nil template",
template: nil,
err: New(NotUnique, WithMsg("this thing was must be unique")),
err: New(NotUnique, testId, WithMsg("this thing was must be unique")),
want: false,
},
{
@ -163,8 +166,8 @@ func TestMatch(t *testing.T) {
template: T(Integrity),
err: New(
NotUnique,
testId,
WithMsg("this thing must be unique"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: true,
@ -174,8 +177,8 @@ func TestMatch(t *testing.T) {
template: T(Integrity),
err: New(
RecordNotFound,
testId,
WithMsg("this thing is missing"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: false,
@ -185,8 +188,8 @@ func TestMatch(t *testing.T) {
template: T(NotUnique),
err: New(
NotUnique,
testId,
WithMsg("this thing must be unique"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: true,
@ -196,30 +199,30 @@ func TestMatch(t *testing.T) {
template: T(NotUnique),
err: New(
RecordNotFound,
testId,
WithMsg("this thing is missing"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: false,
},
{
name: "match on Op only",
template: T(Op("alice.Bob")),
template: T(ErrorId("unique")),
err: New(
NotUnique,
"unique",
WithMsg("this thing must be unique"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: true,
},
{
name: "no match on Op only",
template: T(Op("alice.Alice")),
template: T(ErrorId("unique")),
err: New(
RecordNotFound,
testId,
WithMsg("this thing is missing"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: false,
@ -231,12 +234,12 @@ func TestMatch(t *testing.T) {
Integrity,
InvalidParameter,
ErrInvalidFieldMask,
Op("alice.Bob"),
ErrorId("unique"),
),
err: New(
InvalidParameter,
"unique",
WithMsg("this thing must be unique"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: true,
@ -246,8 +249,8 @@ func TestMatch(t *testing.T) {
template: T(ErrInvalidFieldMask),
err: New(
NotUnique,
testId,
WithMsg("this thing must be unique"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: true,
@ -257,8 +260,8 @@ func TestMatch(t *testing.T) {
template: T(ErrNotUnique),
err: New(
RecordNotFound,
testId,
WithMsg("this thing is missing"),
WithOp("alice.Bob"),
WithWrap(ErrInvalidFieldMask),
),
want: false,
@ -268,8 +271,8 @@ func TestMatch(t *testing.T) {
template: T(stdErr),
err: New(
NotUnique,
testId,
WithMsg("this thing must be unique"),
WithOp("alice.Bob"),
WithWrap(stdErr),
),
want: true,
@ -279,8 +282,8 @@ func TestMatch(t *testing.T) {
template: T(stderrors.New("no match")),
err: New(
RecordNotFound,
testId,
WithMsg("this thing is missing"),
WithOp("alice.Bob"),
WithWrap(stdErr),
),
want: false,

@ -16,7 +16,6 @@ type Option func(*Options)
type Options struct {
withErrWrapped error
withErrMsg string
withOp Op
}
func getDefaultOptions() Options {
@ -38,11 +37,3 @@ func WithMsg(msg string) Option {
o.withErrMsg = msg
}
}
// WithOp provides an option to provide the operation that's raising/propagating
// the error.
func WithOp(op Op) Option {
return func(o *Options) {
o.withOp = op
}
}

@ -1,6 +1,7 @@
package errors
import (
"fmt"
"testing"
"github.com/stretchr/testify/assert"
@ -31,21 +32,9 @@ func Test_getOpts(t *testing.T) {
assert.Equal(opts, testOpts)
// try setting it
opts = GetOpts(WithWrap(ErrInvalidParameter))
testOpts.withErrWrapped = ErrInvalidParameter
assert.Equal(opts, testOpts)
})
t.Run("WithOp", func(t *testing.T) {
assert := assert.New(t)
// test default
opts := GetOpts()
testOpts := getDefaultOptions()
testOpts.withOp = ""
assert.Equal(opts, testOpts)
// try setting it
opts = GetOpts(WithOp("alice.bob"))
testOpts.withOp = "alice.bob"
err := fmt.Errorf("test error")
opts = GetOpts(WithWrap(err))
testOpts.withErrWrapped = err
assert.Equal(opts, testOpts)
})
}

@ -1,44 +1,42 @@
package errors
// Errors returned from this package may be tested against these errors
// with errors.Is. Creating new Sentinel type errors like these should be
// deprecated in favor of the new Err type that includes unique Codes and a
// Matching function.
// Errors returned from this package may be tested against these errors with errors.Is.
// TODO: Deprecate sentinel errors in favor of the new Err type that includes unique ErrorId and Code.
var (
// ErrInvalidPublicId indicates an invalid PublicId.
ErrInvalidPublicId = New(InvalidParameter, WithMsg("invalid publicId"))
ErrInvalidPublicId = New(InvalidParameter, "", WithMsg("invalid publicId"))
// ErrInvalidParameter is returned by create and update methods if
// an attribute on a struct contains illegal or invalid values.
ErrInvalidParameter = New(InvalidParameter, WithMsg("invalid parameter"))
ErrInvalidParameter = New(InvalidParameter, "", WithMsg("invalid parameter"))
// ErrInvalidFieldMask is returned by update methods if the field mask
// contains unknown fields or fields that cannot be updated.
ErrInvalidFieldMask = New(InvalidParameter, WithMsg("invalid field mask"))
ErrInvalidFieldMask = New(InvalidParameter, "", WithMsg("invalid field mask"))
// ErrEmptyFieldMask is returned by update methods if the field mask is
// empty.
ErrEmptyFieldMask = New(InvalidParameter, WithMsg("empty field mask"))
ErrEmptyFieldMask = New(InvalidParameter, "", WithMsg("empty field mask"))
// ErrNotUnique is returned by create and update methods when a write
// to the repository resulted in a unique constraint violation.
ErrNotUnique = New(NotUnique, WithMsg("unique constraint violation"))
ErrNotUnique = New(NotUnique, "", WithMsg("unique constraint violation"))
// ErrNotNull is returned by methods when a write to the repository resulted
// in a check constraint violation
ErrCheckConstraint = New(CheckConstraint, WithMsg("check constraint violated"))
ErrCheckConstraint = New(CheckConstraint, "", WithMsg("check constraint violated"))
// ErrNotNull is returned by methods when a write to the repository resulted
// in a not null constraint violation
ErrNotNull = New(NotNull, WithMsg("not null constraint violated"))
ErrNotNull = New(NotNull, "", WithMsg("not null constraint violated"))
// ErrRecordNotFound returns a "record not found" error and it only occurs
// when attempting to read from the database into struct.
// When reading into a slice it won't return this error.
ErrRecordNotFound = New(RecordNotFound, WithMsg("record not found"))
ErrRecordNotFound = New(RecordNotFound, "", WithMsg("record not found"))
// ErrMultipleRecords is returned by update and delete methods when a
// write to the repository would result in more than one record being
// changed resulting in the transaction being rolled back.
ErrMultipleRecords = New(MultipleRecords, WithMsg("multiple records"))
ErrMultipleRecords = New(MultipleRecords, "", WithMsg("multiple records"))
)

Loading…
Cancel
Save