From ee4a0d3f45e3950883b4c3e793a34a37195c9018 Mon Sep 17 00:00:00 2001 From: Jim Date: Mon, 11 May 2020 16:21:09 -0400 Subject: [PATCH] refactor lookups so we can easily check for not found and do the "right" thing in the repos (#42) --- internal/db/read_writer.go | 23 +++++++++++++-- internal/db/read_writer_test.go | 11 ++++--- internal/iam/repository_scope.go | 11 ++++++- internal/iam/repository_scope_test.go | 41 +++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/internal/db/read_writer.go b/internal/db/read_writer.go index 68cd5de5e..1818adb6a 100644 --- a/internal/db/read_writer.go +++ b/internal/db/read_writer.go @@ -14,6 +14,13 @@ 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") +) + // Reader interface defines lookups/searching for resources type Reader interface { // LookupByName will lookup resource by its friendly name which must be unique @@ -396,7 +403,13 @@ func (rw *GormReadWriter) LookupByName(ctx context.Context, resource ResourceNam if resource.GetName() == "" { return errors.New("error name empty string for lookup by name") } - return rw.Tx.Where("name = ?", resource.GetName()).First(resource).Error + if err := rw.Tx.Where("name = ?", resource.GetName()).First(resource).Error; err != nil { + if err == gorm.ErrRecordNotFound { + return ErrRecordNotFound + } + return err + } + return nil } // LookupByPublicId will lookup resource my its public_id which must be unique @@ -416,7 +429,13 @@ func (rw *GormReadWriter) LookupByPublicId(ctx context.Context, resource Resourc if resource.GetPublicId() == "" { return errors.New("error public id empty string for lookup by public id") } - return rw.Tx.Where("public_id = ?", resource.GetPublicId()).First(resource).Error + if err := rw.Tx.Where("public_id = ?", resource.GetPublicId()).First(resource).Error; err != nil { + if err == gorm.ErrRecordNotFound { + return ErrRecordNotFound + } + return err + } + return nil } // LookupWhere will lookup the first resource using a where clause with parameters (it only returns the first one) diff --git a/internal/db/read_writer_test.go b/internal/db/read_writer_test.go index 04ba3e1ec..c86e2e9b4 100644 --- a/internal/db/read_writer_test.go +++ b/internal/db/read_writer_test.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/watchtower/internal/db/db_test" "github.com/hashicorp/watchtower/internal/oplog" "github.com/hashicorp/watchtower/internal/oplog/store" - "github.com/jinzhu/gorm" "github.com/stretchr/testify/assert" ) @@ -320,7 +319,7 @@ func TestGormReadWriter_LookupByName(t *testing.T) { foundUser.Name = "fn-" + id err = w.LookupByName(context.Background(), foundUser) assert.True(err != nil) - assert.Equal(gorm.ErrRecordNotFound, err) + assert.Equal(ErrRecordNotFound, err) }) } @@ -375,7 +374,7 @@ func TestGormReadWriter_LookupByPublicId(t *testing.T) { foundUser.PublicId = id err = w.LookupByPublicId(context.Background(), foundUser) assert.True(err != nil) - assert.Equal(gorm.ErrRecordNotFound, err) + assert.Equal(ErrRecordNotFound, err) }) } @@ -416,7 +415,7 @@ func TestGormReadWriter_LookupWhere(t *testing.T) { var foundUser db_test.TestUser err = w.LookupWhere(context.Background(), &foundUser, "public_id = ?", id) assert.True(err != nil) - assert.Equal(gorm.ErrRecordNotFound, err) + assert.Equal(ErrRecordNotFound, err) }) t.Run("bad-where", func(t *testing.T) { w := GormReadWriter{Tx: db} @@ -633,7 +632,7 @@ func TestGormReadWriter_Delete(t *testing.T) { err = w.LookupByPublicId(context.Background(), foundUser) assert.True(err != nil) - assert.Equal(gorm.ErrRecordNotFound, err) + assert.Equal(ErrRecordNotFound, err) }) t.Run("valid-WithOplog", func(t *testing.T) { w := GormReadWriter{Tx: db} @@ -680,7 +679,7 @@ func TestGormReadWriter_Delete(t *testing.T) { err = w.LookupByPublicId(context.Background(), foundUser) assert.True(err != nil) - assert.Equal(gorm.ErrRecordNotFound, err) + assert.Equal(ErrRecordNotFound, err) }) t.Run("no-wrapper-WithOplog", func(t *testing.T) { w := GormReadWriter{Tx: db} diff --git a/internal/iam/repository_scope.go b/internal/iam/repository_scope.go index 753a6818b..7d444e288 100644 --- a/internal/iam/repository_scope.go +++ b/internal/iam/repository_scope.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + + "github.com/hashicorp/watchtower/internal/db" ) // CreateScope will create a scope in the repository and return the written scope @@ -30,7 +32,8 @@ func (r *Repository) UpdateScope(ctx context.Context, scope *Scope, fieldMaskPat return resource.(*Scope), err } -// LookupScope will look up a scope in the respository +// LookupScope will look up a scope in the repository. If the scope is not +// found, it will return nil, nil. func (r *Repository) LookupScope(ctx context.Context, opt ...Option) (*Scope, error) { opts := getOpts(opt...) withPublicId := opts.withPublicId @@ -40,6 +43,9 @@ func (r *Repository) LookupScope(ctx context.Context, opt ...Option) (*Scope, er scope := allocScope() scope.PublicId = withPublicId if err := r.reader.LookupByPublicId(ctx, &scope); err != nil { + if err == db.ErrRecordNotFound { + return nil, nil + } return nil, err } return &scope, nil @@ -48,6 +54,9 @@ func (r *Repository) LookupScope(ctx context.Context, opt ...Option) (*Scope, er scope := allocScope() scope.Name = withName if err := r.reader.LookupByName(ctx, &scope); err != nil { + if err == db.ErrRecordNotFound { + return nil, nil + } return nil, err } return &scope, nil diff --git a/internal/iam/repository_scope_test.go b/internal/iam/repository_scope_test.go index d562719f2..c56e20c01 100644 --- a/internal/iam/repository_scope_test.go +++ b/internal/iam/repository_scope_test.go @@ -133,3 +133,44 @@ func Test_Repository_UpdateScope(t *testing.T) { assert.Equal("failed to update scope: error on update you cannot change a scope's parent", err.Error()) }) } + +func Test_Repository_LookupScope(t *testing.T) { + t.Parallel() + cleanup, conn := db.TestSetup(t, "postgres") + defer cleanup() + assert := assert.New(t) + defer conn.Close() + + t.Run("found-and-not-found", func(t *testing.T) { + rw := &db.GormReadWriter{Tx: conn} + wrapper := db.TestWrapper(t) + repo, err := NewRepository(rw, rw, wrapper) + id, err := uuid.GenerateUUID() + assert.Nil(err) + + s, err := NewOrganization(WithName("fname-" + id)) + s, err = repo.CreateScope(context.Background(), s) + assert.Nil(err) + assert.True(s != nil) + assert.True(s.GetPublicId() != "") + assert.Equal(s.GetName(), "fname-"+id) + + foundScope, err := repo.LookupScope(context.Background(), WithPublicId(s.PublicId)) + assert.Nil(err) + assert.Equal(foundScope.GetPublicId(), s.GetPublicId()) + + foundScope, err = repo.LookupScope(context.Background(), WithName("fname-"+id)) + assert.Nil(err) + assert.Equal(foundScope.GetPublicId(), s.GetPublicId()) + + invalidId, err := uuid.GenerateUUID() + assert.Nil(err) + notFoundById, err := repo.LookupScope(context.Background(), WithPublicId(invalidId)) + assert.Nil(err) + assert.Nil(notFoundById) + + notFoundByName, err := repo.LookupScope(context.Background(), WithName(invalidId)) + assert.Nil(err) + assert.Nil(notFoundByName) + }) +}