diff --git a/internal/db/error.go b/internal/db/error.go index 556814fa6..d1af2af6b 100644 --- a/internal/db/error.go +++ b/internal/db/error.go @@ -18,5 +18,9 @@ var ( // ErrNilParameter is returned when a required parameter is nil. ErrNilParameter = errors.New("nil parameter") -) + // 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 = errors.New("record not found") +) diff --git a/internal/db/read_writer.go b/internal/db/read_writer.go index 5a540f27d..2c8b2a003 100644 --- a/internal/db/read_writer.go +++ b/internal/db/read_writer.go @@ -14,13 +14,6 @@ import ( "google.golang.org/protobuf/proto" ) -var ( - // 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 = errors.New("record not found") -) - const NoRowsAffected = 0 // Reader interface defines lookups/searching for resources @@ -127,18 +120,22 @@ func New(underlying *gorm.DB) *Db { // DB returns the sql.DB func (rw *Db) DB() (*sql.DB, error) { if rw.underlying == nil { - return nil, errors.New("underlying db is nil") + return nil, fmt.Errorf("missing underlying db: %w", ErrNilParameter) } return rw.underlying.DB(), nil } // Scan rows will scan the rows into the interface func (rw *Db) ScanRows(rows *sql.Rows, result interface{}) error { + if rw.underlying == nil { + return fmt.Errorf("scan rows: missing underlying db %w", ErrNilParameter) + } + if result == nil { + return fmt.Errorf("scan rows: result is missing %w", ErrNilParameter) + } return rw.underlying.ScanRows(rows, result) } -var ErrNotResourceWithId = errors.New("not a resource with an id") - func (rw *Db) lookupAfterWrite(ctx context.Context, i interface{}, opt ...Option) error { opts := GetOpts(opt...) withLookup := opts.withLookup @@ -148,17 +145,20 @@ func (rw *Db) lookupAfterWrite(ctx context.Context, i interface{}, opt ...Option } if _, ok := i.(ResourcePublicIder); ok { if err := rw.LookupByPublicId(ctx, i.(ResourcePublicIder), opt...); err != nil { - return err + return fmt.Errorf("lookup after write: failed %w", err) } return nil } - return ErrNotResourceWithId + return errors.New("not a resource with an id") } // Create an object in the db with options: WithOplog and WithLookup (to force a lookup after create)) func (rw *Db) Create(ctx context.Context, i interface{}, opt ...Option) error { if rw.underlying == nil { - return errors.New("create underlying db is nil") + return fmt.Errorf("create: missing underlying db %w", ErrNilParameter) + } + if i == nil { + return fmt.Errorf("create: interface is missing %w", ErrNilParameter) } opts := GetOpts(opt...) withOplog := opts.withOplog @@ -167,7 +167,7 @@ func (rw *Db) Create(ctx context.Context, i interface{}, opt ...Option) error { // let's validate oplog options before we start writing to the database _, err := validateOplogArgs(i, opts) if err != nil { - return err + return fmt.Errorf("create: oplog validation failed %w", err) } } if withDebug { @@ -175,15 +175,19 @@ func (rw *Db) Create(ctx context.Context, i interface{}, opt ...Option) error { defer rw.underlying.LogMode(false) } if i == nil { - return errors.New("create interface is nil") + return fmt.Errorf("create: missing interface %w", ErrNilParameter) } + // these fields should be nil, since they are not writeable and we want the + // db to manage them + setFieldsToNil(i, []string{"CreateTime", "UpdateTime"}) + if vetter, ok := i.(VetForWriter); ok { if err := vetter.VetForWrite(ctx, rw, CreateOp); err != nil { - return fmt.Errorf("error on create %w", err) + return fmt.Errorf("create: vet for write failed %w", err) } } if err := rw.underlying.Create(i).Error; err != nil { - return fmt.Errorf("error creating: %w", err) + return fmt.Errorf("create: failed %w", err) } if withOplog { if err := rw.addOplog(ctx, CreateOp, opts, i); err != nil { @@ -191,7 +195,7 @@ func (rw *Db) Create(ctx context.Context, i interface{}, opt ...Option) error { } } if err := rw.lookupAfterWrite(ctx, i, opt...); err != nil { - return fmt.Errorf("lookup error after create: %w", err) + return fmt.Errorf("create: lookup error %w", err) } return nil } @@ -202,8 +206,22 @@ func (rw *Db) Create(ctx context.Context, i interface{}, opt ...Option) error { // updating. Update returns the number of rows updated and any errors. func (rw *Db) Update(ctx context.Context, i interface{}, fieldMaskPaths []string, opt ...Option) (int, error) { if rw.underlying == nil { - return NoRowsAffected, errors.New("update underlying db is nil") + return NoRowsAffected, fmt.Errorf("update: missing underlying db %w", ErrNilParameter) } + if i == nil { + return NoRowsAffected, fmt.Errorf("update: interface is missing %w", ErrNilParameter) + } + if len(fieldMaskPaths) == 0 { + return NoRowsAffected, fmt.Errorf("update: missing fieldMaskPaths %w", ErrNilParameter) + } + + // This is not a watchtower scope, but rather a gorm Scope: + // https://godoc.org/github.com/jinzhu/gorm#DB.NewScope + scope := rw.underlying.NewScope(i) + if scope.PrimaryKeyZero() { + return NoRowsAffected, fmt.Errorf("update: primary key is not set") + } + opts := GetOpts(opt...) withDebug := opts.withDebug withOplog := opts.withOplog @@ -211,26 +229,25 @@ func (rw *Db) Update(ctx context.Context, i interface{}, fieldMaskPaths []string // let's validate oplog options before we start writing to the database _, err := validateOplogArgs(i, opts) if err != nil { - return NoRowsAffected, err + return NoRowsAffected, fmt.Errorf("update: oplog validation failed %w", err) } } if withDebug { rw.underlying.LogMode(true) defer rw.underlying.LogMode(false) } - if i == nil { - return NoRowsAffected, errors.New("update interface is nil") - } if vetter, ok := i.(VetForWriter); ok { if err := vetter.VetForWrite(ctx, rw, UpdateOp, WithFieldMaskPaths(fieldMaskPaths)); err != nil { - return NoRowsAffected, fmt.Errorf("error on update %w", err) + return NoRowsAffected, fmt.Errorf("update: vet for write failed %w", err) } } + + // we need to filter out some non-updatable fields (like: CreateTime, etc) + fieldMaskPaths = filterFieldMask(fieldMaskPaths) if len(fieldMaskPaths) == 0 { - if err := rw.underlying.Save(i).Error; err != nil { - return NoRowsAffected, fmt.Errorf("error updating: %w", err) - } + return NoRowsAffected, fmt.Errorf("update: after filtering non-updated fields, there are no fields left in fieldMaskPaths: %s", fieldMaskPaths) } + updateFields := map[string]interface{}{} val := reflect.Indirect(reflect.ValueOf(i)) @@ -266,23 +283,23 @@ func (rw *Db) Update(ctx context.Context, i interface{}, fieldMaskPaths []string } } if len(updateFields) == 0 { - return NoRowsAffected, fmt.Errorf("error no update fields matched using fieldMaskPaths: %s", fieldMaskPaths) + return NoRowsAffected, fmt.Errorf("update: no fields matched using fieldMaskPaths %s", fieldMaskPaths) } underlying := rw.underlying.Model(i).Updates(updateFields) if underlying.Error != nil { - return NoRowsAffected, fmt.Errorf("error updating: %w", underlying.Error) + return NoRowsAffected, fmt.Errorf("update: failed %w", underlying.Error) } rowsUpdated := int(underlying.RowsAffected) if withOplog && rowsUpdated > 0 { if err := rw.addOplog(ctx, UpdateOp, opts, i); err != nil { - return rowsUpdated, err + return rowsUpdated, fmt.Errorf("update: add oplog failed %w", err) } } // we need to force a lookupAfterWrite so the resource returned is correctly initialized // from the db opt = append(opt, WithLookup(true)) if err := rw.lookupAfterWrite(ctx, i, opt...); err != nil { - return NoRowsAffected, fmt.Errorf("lookup error after update: %w", err) + return NoRowsAffected, fmt.Errorf("update: lookup error %w", err) } return rowsUpdated, nil } @@ -292,10 +309,16 @@ func (rw *Db) Update(ctx context.Context, i interface{}, fieldMaskPaths []string // any errors. func (rw *Db) Delete(ctx context.Context, i interface{}, opt ...Option) (int, error) { if rw.underlying == nil { - return NoRowsAffected, errors.New("delete underlying db is nil") + return NoRowsAffected, fmt.Errorf("delete: missing underlying db %w", ErrNilParameter) } if i == nil { - return NoRowsAffected, errors.New("delete interface is nil") + return NoRowsAffected, fmt.Errorf("delete: interface is missing %w", ErrNilParameter) + } + // This is not a watchtower scope, but rather a gorm Scope: + // https://godoc.org/github.com/jinzhu/gorm#DB.NewScope + scope := rw.underlying.NewScope(i) + if scope.PrimaryKeyZero() { + return NoRowsAffected, fmt.Errorf("delete: primary key is not set") } opts := GetOpts(opt...) withDebug := opts.withDebug @@ -303,7 +326,7 @@ func (rw *Db) Delete(ctx context.Context, i interface{}, opt ...Option) (int, er if withOplog { _, err := validateOplogArgs(i, opts) if err != nil { - return NoRowsAffected, err + return NoRowsAffected, fmt.Errorf("delete: oplog validation failed %w", err) } } if withDebug { @@ -312,12 +335,12 @@ func (rw *Db) Delete(ctx context.Context, i interface{}, opt ...Option) (int, er } underlying := rw.underlying.Delete(i) if underlying.Error != nil { - return NoRowsAffected, fmt.Errorf("error deleting: %w", underlying.Error) + return NoRowsAffected, fmt.Errorf("delete: failed %w", underlying.Error) } rowsDeleted := int(underlying.RowsAffected) if withOplog && rowsDeleted > 0 { if err := rw.addOplog(ctx, DeleteOp, opts, i); err != nil { - return rowsDeleted, err + return rowsDeleted, fmt.Errorf("delete: add oplog failed %w", err) } } return rowsDeleted, nil @@ -515,3 +538,64 @@ func (rw *Db) SearchWhere(ctx context.Context, resources interface{}, where stri } return nil } + +// filterFieldMasks will filter out non-updatable fields +func filterFieldMask(fieldMaskPaths []string) []string { + if len(fieldMaskPaths) == 0 { + return nil + } + filtered := []string{} + for _, p := range fieldMaskPaths { + switch { + case strings.EqualFold(p, "CreateTime"): + continue + case strings.EqualFold(p, "UpdateTime"): + continue + case strings.EqualFold("PublicId"): + continue + default: + filtered = append(filtered, p) + } + } + return filtered +} + +// setFieldsToNil will set the fieldNames to nil. It supports embedding a struct +// (or structPtr) one level deep. +func setFieldsToNil(i interface{}, fieldNames []string) { + val := reflect.Indirect(reflect.ValueOf(i)) + structTyp := val.Type() + for _, field := range fieldNames { + for i := 0; i < structTyp.NumField(); i++ { + kind := structTyp.Field(i).Type.Kind() + if kind == reflect.Struct || kind == reflect.Ptr { + // check if the embedded field is exported via CanInterface() + if val.Field(i).CanInterface() { + embVal := reflect.Indirect(reflect.ValueOf(val.Field(i).Interface())) + // if it's a ptr to a struct, then we need a few more bits before proceeding. + if kind == reflect.Ptr { + embVal = val.Field(i).Elem() + embType := embVal.Type() + if embType.Kind() != reflect.Struct { + continue + } + } + f := embVal.FieldByName(field) + if f.IsValid() { + if f.CanSet() { + f.Set(reflect.Zero(f.Type())) + } + } + continue + } + } + // it's not an embedded type, so check if the field name matches + f := val.FieldByName(field) + if f.IsValid() { + if f.CanSet() { + f.Set(reflect.Zero(f.Type())) + } + } + } + } +} diff --git a/internal/db/read_writer_test.go b/internal/db/read_writer_test.go index b2cf23689..f72b0e94a 100644 --- a/internal/db/read_writer_test.go +++ b/internal/db/read_writer_test.go @@ -6,6 +6,7 @@ import ( "fmt" "testing" + "github.com/golang/protobuf/ptypes" "github.com/hashicorp/go-uuid" "github.com/hashicorp/watchtower/internal/db/db_test" "github.com/hashicorp/watchtower/internal/oplog" @@ -50,6 +51,50 @@ func TestDb_Update(t *testing.T) { assert.NoError(err) assert.Equal(foundUser.Name, user.Name) }) + t.Run("non-updatable-fields", func(t *testing.T) { + w := Db{underlying: db} + id, err := uuid.GenerateUUID() + assert.NoError(err) + user, err := db_test.NewTestUser() + assert.NoError(err) + user.Name = "foo-" + id + err = w.Create(context.Background(), user) + db.LogMode(false) + assert.NoError(err) + assert.NotEmpty(user.Id) + + foundUser, err := db_test.NewTestUser() + assert.NoError(err) + foundUser.PublicId = user.PublicId + err = w.LookupByPublicId(context.Background(), foundUser) + assert.NoError(err) + assert.Equal(foundUser.Id, user.Id) + + user.Name = "friendly-" + id + ts := &db_test.Timestamp{Timestamp: ptypes.TimestampNow()} + user.CreateTime = ts + user.UpdateTime = ts + rowsUpdated, err := w.Update(context.Background(), user, []string{"Name", "CreateTime", "UpdateTime"}) + assert.NoError(err) + assert.Equal(1, rowsUpdated) + + err = w.LookupByPublicId(context.Background(), foundUser) + assert.NoError(err) + assert.Equal(foundUser.Name, user.Name) + assert.NotEqual(foundUser.CreateTime, ts) + assert.NotEqual(foundUser.UpdateTime, ts) + + ts = &db_test.Timestamp{Timestamp: ptypes.TimestampNow()} + user.Name = id + user.CreateTime = ts + user.UpdateTime = ts + rowsUpdated, err = w.Update(context.Background(), user, nil) + assert.Error(err) + assert.Equal(0, rowsUpdated) + assert.Equal("update: missing fieldMaskPaths nil parameter", err.Error()) + assert.NotEqual(foundUser.CreateTime, ts) + assert.NotEqual(foundUser.UpdateTime, ts) + }) t.Run("valid-WithOplog", func(t *testing.T) { w := Db{underlying: db} id, err := uuid.GenerateUUID() @@ -105,7 +150,7 @@ func TestDb_Update(t *testing.T) { rowsUpdated, err := w.Update(context.Background(), user, nil) assert.Error(err) assert.Equal(0, rowsUpdated) - assert.Equal("update underlying db is nil", err.Error()) + assert.Equal("update: missing underlying db nil parameter", err.Error()) }) t.Run("no-wrapper-WithOplog", func(t *testing.T) { w := Db{underlying: db} @@ -137,7 +182,7 @@ func TestDb_Update(t *testing.T) { ) assert.Error(err) assert.Equal(0, rowsUpdated) - assert.Equal("error no wrapper WithOplog", err.Error()) + assert.Equal("update: oplog validation failed error no wrapper WithOplog", err.Error()) }) t.Run("no-metadata-WithOplog", func(t *testing.T) { w := Db{underlying: db} @@ -166,7 +211,7 @@ func TestDb_Update(t *testing.T) { ) assert.Error(err) assert.Equal(0, rowsUpdated) - assert.Equal("error no metadata for WithOplog", err.Error()) + assert.Equal("update: oplog validation failed error no metadata for WithOplog", err.Error()) }) } @@ -186,12 +231,16 @@ func TestDb_Create(t *testing.T) { assert.NoError(err) user, err := db_test.NewTestUser() assert.NoError(err) + ts := &db_test.Timestamp{Timestamp: ptypes.TimestampNow()} + user.CreateTime = ts + user.UpdateTime = ts user.Name = "foo-" + id err = w.Create(context.Background(), user) assert.NoError(err) assert.NotEmpty(user.Id) - assert.NotEmpty(user.GetCreateTime()) - assert.NotEmpty(user.GetUpdateTime()) + // make sure the database controlled the timestamp values + assert.NotEqual(ts, user.GetCreateTime()) + assert.NotEqual(ts, user.GetUpdateTime()) foundUser, err := db_test.NewTestUser() assert.NoError(err) @@ -249,7 +298,7 @@ func TestDb_Create(t *testing.T) { ), ) assert.Error(err) - assert.Equal("error no wrapper WithOplog", err.Error()) + assert.Equal("create: oplog validation failed error no wrapper WithOplog", err.Error()) }) t.Run("no-metadata-WithOplog", func(t *testing.T) { w := Db{underlying: db} @@ -267,7 +316,7 @@ func TestDb_Create(t *testing.T) { ), ) assert.Error(err) - assert.Equal("error no metadata for WithOplog", err.Error()) + assert.Equal("create: oplog validation failed error no metadata for WithOplog", err.Error()) }) t.Run("nil-tx", func(t *testing.T) { w := Db{underlying: nil} @@ -278,7 +327,7 @@ func TestDb_Create(t *testing.T) { user.Name = "foo-" + id err = w.Create(context.Background(), user) assert.Error(err) - assert.Equal("create underlying db is nil", err.Error()) + assert.Equal("create: missing underlying db nil parameter", err.Error()) }) } @@ -532,7 +581,7 @@ func TestDb_DB(t *testing.T) { d, err := w.DB() assert.Error(err) assert.Nil(d) - assert.Equal("underlying db is nil", err.Error()) + assert.Equal("missing underlying db: nil parameter", err.Error()) }) } @@ -834,7 +883,7 @@ func TestDb_Delete(t *testing.T) { ) assert.Error(err) assert.Equal(0, rowsDeleted) - assert.Equal("error no wrapper WithOplog", err.Error()) + assert.Equal("delete: oplog validation failed error no wrapper WithOplog", err.Error()) }) t.Run("no-metadata-WithOplog", func(t *testing.T) { w := Db{underlying: db} @@ -875,7 +924,7 @@ func TestDb_Delete(t *testing.T) { ) assert.Error(err) assert.Equal(0, rowsDeleted) - assert.Equal("error no metadata for WithOplog", err.Error()) + assert.Equal("delete: oplog validation failed error no metadata for WithOplog", err.Error()) }) t.Run("nil-tx", func(t *testing.T) { w := Db{underlying: nil} @@ -886,7 +935,7 @@ func TestDb_Delete(t *testing.T) { user.Name = "foo-" + id err = w.Create(context.Background(), user) assert.Error(err) - assert.Equal("create underlying db is nil", err.Error()) + assert.Equal("create: missing underlying db nil parameter", err.Error()) }) } diff --git a/internal/iam/repository_scope_test.go b/internal/iam/repository_scope_test.go index 145c238c6..9f94317e7 100644 --- a/internal/iam/repository_scope_test.go +++ b/internal/iam/repository_scope_test.go @@ -176,7 +176,7 @@ func Test_Repository_UpdateScope(t *testing.T) { assert.Error(err) assert.Nil(project) assert.Equal(0, updatedRows) - assert.Equal("failed to update scope: error on update you cannot change a scope's parent", err.Error()) + assert.Equal("failed to update scope: update: vet for write failed you cannot change a scope's parent", err.Error()) }) }