From c90a50827a167cb61ed8f6a5982c6bcfa2ec6f40 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 18 Mar 2016 11:33:50 -0400 Subject: [PATCH] Get() methods from all repositories now return a ErrNotFound when the id is nonexistent --- api/stream.go | 11 +++++----- domain/base.go | 6 ++++++ engine/cover.go | 18 +++++++--------- engine/search.go | 17 ++++++++++++--- persistence/ledis_repository.go | 4 +++- persistence/ledis_repository_test.go | 31 +++++++++++++++------------- persistence/mock_album_repo.go | 5 ++++- persistence/mock_artist_repo.go | 5 ++++- persistence/mock_mediafile_repo.go | 6 ++++-- persistence/property_repository.go | 5 +++-- scanner/itunes_scanner.go | 4 ++-- 11 files changed, 70 insertions(+), 42 deletions(-) diff --git a/api/stream.go b/api/stream.go index 1893da3b0..174cec70a 100644 --- a/api/stream.go +++ b/api/stream.go @@ -21,14 +21,13 @@ func (c *StreamController) Prepare() { c.id = c.RequiredParamString("id", "id parameter required") mf, err := c.repo.Get(c.id) - if err != nil { - beego.Error("Error reading mediafile", c.id, "from the database", ":", err) - c.SendError(responses.ERROR_GENERIC, "Internal error") - } - - if mf == nil { + switch { + case err == domain.ErrNotFound: beego.Error("MediaFile", c.id, "not found!") c.SendError(responses.ERROR_DATA_NOT_FOUND) + case err != nil: + beego.Error("Error reading mediafile", c.id, "from the database", ":", err) + c.SendError(responses.ERROR_GENERIC, "Internal error") } c.mf = mf diff --git a/domain/base.go b/domain/base.go index 859da5b21..adb3798c7 100644 --- a/domain/base.go +++ b/domain/base.go @@ -1,11 +1,17 @@ package domain +import "errors" + type BaseRepository interface { NewId(fields ...string) string CountAll() (int64, error) Exists(id string) (bool, error) } +var ( + ErrNotFound = errors.New("Data not found") +) + type QueryOptions struct { SortBy string Alpha bool diff --git a/engine/cover.go b/engine/cover.go index 3492aa2cd..db93d9102 100644 --- a/engine/cover.go +++ b/engine/cover.go @@ -1,16 +1,13 @@ package engine import ( - "io" - "os" - + "bytes" "image" _ "image/gif" - _ "image/jpeg" - _ "image/png" - - "bytes" "image/jpeg" + _ "image/png" + "io" + "os" "github.com/deluan/gosonic/domain" "github.com/dhowden/tag" @@ -31,16 +28,17 @@ func NewCover(mr domain.MediaFileRepository) Cover { func (c cover) Get(id string, size int, out io.Writer) error { mf, err := c.mfileRepo.Get(id) - if err != nil { + if err != nil && err != domain.ErrNotFound { return err } var reader io.Reader - if mf != nil && mf.HasCoverArt { + if err == nil && mf.HasCoverArt { reader, err = readFromTag(mf.Path) } else { - f, err := os.Open("static/default_cover.jpg") + var f *os.File + f, err = os.Open("static/default_cover.jpg") if err == nil { defer f.Close() reader = f diff --git a/engine/search.go b/engine/search.go index 6c146fd84..109bf46dd 100644 --- a/engine/search.go +++ b/engine/search.go @@ -3,6 +3,7 @@ package engine import ( "strings" + "github.com/astaxie/beego" "github.com/deluan/gomate" "github.com/deluan/gosonic/domain" "github.com/kennygrant/sanitize" @@ -73,7 +74,7 @@ func (s search) SearchArtist(q string, offset int, size int) (*Results, error) { res := make(Results, len(resp)) for i, id := range resp { a, err := s.artistRepo.Get(id) - if err != nil { + if criticalError("Artist", id, err) { return nil, err } res[i] = Entry{Id: a.Id, Title: a.Name, IsDir: true} @@ -92,7 +93,7 @@ func (s search) SearchAlbum(q string, offset int, size int) (*Results, error) { res := make(Results, len(resp)) for i, id := range resp { al, err := s.albumRepo.Get(id) - if err != nil { + if criticalError("Album", id, err) { return nil, err } res[i] = FromAlbum(al) @@ -111,10 +112,20 @@ func (s search) SearchSong(q string, offset int, size int) (*Results, error) { res := make(Results, len(resp)) for i, id := range resp { mf, err := s.mfileRepo.Get(id) - if err != nil { + if criticalError("Song", id, err) { return nil, err } res[i] = FromMediaFile(mf) } return &res, nil } + +func criticalError(kind, id string, err error) bool { + switch { + case err != nil: + return true + case err == domain.ErrNotFound: + beego.Warn(kind, "Id", id, "not in the DB. Need a reindex?") + } + return false +} diff --git a/persistence/ledis_repository.go b/persistence/ledis_repository.go index 59d3fc538..40452b518 100644 --- a/persistence/ledis_repository.go +++ b/persistence/ledis_repository.go @@ -6,7 +6,6 @@ import ( "fmt" "reflect" "strings" - "time" "github.com/deluan/gosonic/domain" @@ -228,6 +227,9 @@ func (r *ledisRepository) readEntity(id string) (interface{}, error) { if err != nil { return nil, err } + if len(res[0]) == 0 { + return nil, domain.ErrNotFound + } err = r.toEntity(res, entity) return entity, err } diff --git a/persistence/ledis_repository_test.go b/persistence/ledis_repository_test.go index 91a2e21a7..2f4b7b47a 100644 --- a/persistence/ledis_repository_test.go +++ b/persistence/ledis_repository_test.go @@ -4,9 +4,9 @@ import ( "fmt" "strconv" "testing" - "time" + "github.com/deluan/gosonic/domain" "github.com/deluan/gosonic/tests" "github.com/deluan/gosonic/utils" . "github.com/smartystreets/goconvey/convey" @@ -27,7 +27,8 @@ func shouldBeEqual(actualStruct interface{}, expectedStruct ...interface{}) stri return ShouldEqual(actual, expected) } -func createRepo() *ledisRepository { +func createEmptyRepo() *ledisRepository { + dropDb() repo := &ledisRepository{} repo.init("test", &TestEntity{}) return repo @@ -37,7 +38,7 @@ func TestBaseRepository(t *testing.T) { tests.Init(t, false) Convey("Subject: Annotations", t, func() { - repo := createRepo() + repo := createEmptyRepo() Convey("It should parse the parent table definition", func() { So(repo.parentTable, ShouldEqual, "parent") So(repo.parentIdField, ShouldEqual, "ParentId") @@ -51,7 +52,7 @@ func TestBaseRepository(t *testing.T) { }) Convey("Subject: calcScore", t, func() { - repo := createRepo() + repo := createEmptyRepo() Convey("It should create an int score", func() { def := repo.indexes["ByCount"] @@ -86,7 +87,7 @@ func TestBaseRepository(t *testing.T) { }) Convey("Subject: NewId", t, func() { - repo := createRepo() + repo := createEmptyRepo() Convey("When I call NewId with a name", func() { Id := repo.NewId("a name") @@ -120,7 +121,14 @@ func TestBaseRepository(t *testing.T) { Convey("Subject: saveOrUpdate/loadEntity/CountAll", t, func() { Convey("Given an empty DB", func() { - repo := createRepo() + repo := createEmptyRepo() + + Convey("When I try to retrieve an nonexistent ID", func() { + _, err := repo.readEntity("NOT_FOUND") + Convey("Then I should get a NotFound error", func() { + So(err, ShouldEqual, domain.ErrNotFound) + }) + }) Convey("When I save a new entity and a parent", func() { entity := &TestEntity{Id: "123", Name: "My Name", ParentId: "ABC", Year: time.Now()} @@ -151,7 +159,7 @@ func TestBaseRepository(t *testing.T) { }) Convey("Given a table with one entity", func() { - repo := createRepo() + repo := createEmptyRepo() entity := &TestEntity{Id: "111", Name: "One Name", ParentId: "AAA"} repo.saveOrUpdate(entity.Id, entity) @@ -186,7 +194,7 @@ func TestBaseRepository(t *testing.T) { }) Convey("Given a table with 3 entities", func() { - repo := createRepo() + repo := createEmptyRepo() for i := 1; i <= 3; i++ { e := &TestEntity{Id: strconv.Itoa(i), Name: fmt.Sprintf("Name %d", i), ParentId: "AAA"} repo.saveOrUpdate(e.Id, e) @@ -216,7 +224,7 @@ func TestBaseRepository(t *testing.T) { }) Convey("And I get all saved ids", func() { So(len(ids), ShouldEqual, 3) - for k, _ := range ids { + for k := range ids { So(k, ShouldBeIn, []string{"1", "2", "3"}) } }) @@ -245,10 +253,5 @@ func TestBaseRepository(t *testing.T) { }) }) - - Reset(func() { - dropDb() - }) - }) } diff --git a/persistence/mock_album_repo.go b/persistence/mock_album_repo.go index c85d2c6a9..38d37c56c 100644 --- a/persistence/mock_album_repo.go +++ b/persistence/mock_album_repo.go @@ -45,7 +45,10 @@ func (m *MockAlbum) Get(id string) (*domain.Album, error) { if m.err { return nil, errors.New("Error!") } - return m.data[id], nil + if d, ok := m.data[id]; ok { + return d, nil + } + return nil, domain.ErrNotFound } func (m *MockAlbum) GetAll(qo domain.QueryOptions) (*domain.Albums, error) { diff --git a/persistence/mock_artist_repo.go b/persistence/mock_artist_repo.go index 5a45abe3f..fec229632 100644 --- a/persistence/mock_artist_repo.go +++ b/persistence/mock_artist_repo.go @@ -43,5 +43,8 @@ func (m *MockArtist) Get(id string) (*domain.Artist, error) { if m.err { return nil, errors.New("Error!") } - return m.data[id], nil + if d, ok := m.data[id]; ok { + return d, nil + } + return nil, domain.ErrNotFound } diff --git a/persistence/mock_mediafile_repo.go b/persistence/mock_mediafile_repo.go index 28f57b14c..f8485f293 100644 --- a/persistence/mock_mediafile_repo.go +++ b/persistence/mock_mediafile_repo.go @@ -46,8 +46,10 @@ func (m *MockMediaFile) Get(id string) (*domain.MediaFile, error) { if m.err { return nil, errors.New("Error!") } - mf := m.data[id] - return mf, nil + if d, ok := m.data[id]; ok { + return d, nil + } + return nil, domain.ErrNotFound } func (m *MockMediaFile) FindByAlbum(artistId string) (*domain.MediaFiles, error) { diff --git a/persistence/property_repository.go b/persistence/property_repository.go index f1cc5433f..be4db62ad 100644 --- a/persistence/property_repository.go +++ b/persistence/property_repository.go @@ -3,6 +3,7 @@ package persistence import ( "errors" + "github.com/deluan/gosonic/domain" "github.com/deluan/gosonic/engine" ) @@ -33,8 +34,8 @@ func (r *propertyRepository) Get(id string) (string, error) { func (r *propertyRepository) DefaultGet(id string, defaultValue string) (string, error) { v, err := r.Get(id) - if v == "" { - v = defaultValue + if err == domain.ErrNotFound { + return defaultValue, nil } return v, err diff --git a/scanner/itunes_scanner.go b/scanner/itunes_scanner.go index b8588cd9b..074cb8215 100644 --- a/scanner/itunes_scanner.go +++ b/scanner/itunes_scanner.go @@ -66,6 +66,8 @@ func (s *ItunesScanner) ScanLibrary(lastModifiedSince time.Time, path string) (i i := 0 for _, t := range l.Tracks { if !s.skipTrack(&t) { + s.calcCheckSum(&t) + ar := s.collectArtists(&t) mf := s.collectMediaFiles(&t) s.collectAlbums(&t, mf, ar) @@ -200,8 +202,6 @@ func (s *ItunesScanner) calcCheckSum(t *itl.Track) string { } func (s *ItunesScanner) collectMediaFiles(t *itl.Track) *domain.MediaFile { - s.calcCheckSum(t) - mf := &domain.MediaFile{} mf.Id = strconv.Itoa(t.TrackID) mf.Album = unescape(t.Album)