require fieldMaskPaths in Update and refactor errors (#71)

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")
+)

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()))
+				}
+			}
+		}
+	}
+}

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())
 	})
 }
 

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())
 	})
 }