diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 3844d0fa0..d310165e7 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -50,8 +50,8 @@ func CreateSubsonicAPIRouter() *subsonic.Router { externalMetadata := core.NewExternalMetadata(dataStore, agentsAgents) scanner := GetScanner() broker := events.GetBroker() - scrobblerBroker := scrobbler.GetBroker(dataStore) - router := subsonic.New(dataStore, artwork, mediaStreamer, archiver, players, externalMetadata, scanner, broker, scrobblerBroker) + playTracker := scrobbler.GetPlayTracker(dataStore, broker) + router := subsonic.New(dataStore, artwork, mediaStreamer, archiver, players, externalMetadata, scanner, broker, playTracker) return router } diff --git a/core/scrobbler/broker.go b/core/scrobbler/play_tracker.go similarity index 50% rename from core/scrobbler/broker.go rename to core/scrobbler/play_tracker.go index 0f0830724..268440781 100644 --- a/core/scrobbler/broker.go +++ b/core/scrobbler/play_tracker.go @@ -5,6 +5,8 @@ import ( "sort" "time" + "github.com/navidrome/navidrome/server/events" + "github.com/navidrome/navidrome/log" "github.com/ReneKroon/ttlcache/v2" @@ -23,28 +25,34 @@ type NowPlayingInfo struct { PlayerName string } -type Broker interface { - NowPlaying(ctx context.Context, playerId string, playerName string, trackId string) error - GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) - Submit(ctx context.Context, trackId string, playTime time.Time) error +type Submission struct { + TrackID string + Timestamp time.Time } -type broker struct { +type PlayTracker interface { + NowPlaying(ctx context.Context, playerId string, playerName string, trackId string) error + GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) + Submit(ctx context.Context, submissions []Submission) error +} + +type playTracker struct { ds model.DataStore + broker events.Broker playMap *ttlcache.Cache } -func GetBroker(ds model.DataStore) Broker { - instance := singleton.Get(broker{}, func() interface{} { +func GetPlayTracker(ds model.DataStore, broker events.Broker) PlayTracker { + instance := singleton.Get(playTracker{}, func() interface{} { m := ttlcache.NewCache() m.SkipTTLExtensionOnHit(true) _ = m.SetTTL(nowPlayingExpire) - return &broker{ds: ds, playMap: m} + return &playTracker{ds: ds, playMap: m, broker: broker} }) - return instance.(*broker) + return instance.(*playTracker) } -func (s *broker) NowPlaying(ctx context.Context, playerId string, playerName string, trackId string) error { +func (p *playTracker) NowPlaying(ctx context.Context, playerId string, playerName string, trackId string) error { user, _ := request.UserFrom(ctx) info := NowPlayingInfo{ TrackID: trackId, @@ -53,13 +61,13 @@ func (s *broker) NowPlaying(ctx context.Context, playerId string, playerName str PlayerId: playerId, PlayerName: playerName, } - _ = s.playMap.Set(playerId, info) - s.dispatchNowPlaying(ctx, user.ID, trackId) + _ = p.playMap.Set(playerId, info) + p.dispatchNowPlaying(ctx, user.ID, trackId) return nil } -func (s *broker) dispatchNowPlaying(ctx context.Context, userId string, trackId string) { - t, err := s.ds.MediaFile(ctx).Get(trackId) +func (p *playTracker) dispatchNowPlaying(ctx context.Context, userId string, trackId string) { + t, err := p.ds.MediaFile(ctx).Get(trackId) if err != nil { log.Error(ctx, "Error retrieving mediaFile", "id", trackId, err) return @@ -67,7 +75,7 @@ func (s *broker) dispatchNowPlaying(ctx context.Context, userId string, trackId // TODO Parallelize for name, constructor := range scrobblers { err := func() error { - s := constructor(s.ds) + s := constructor(p.ds) if !s.IsAuthorized(ctx, userId) { return nil } @@ -81,10 +89,10 @@ func (s *broker) dispatchNowPlaying(ctx context.Context, userId string, trackId } } -func (s *broker) GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) { +func (p *playTracker) GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) { var res []NowPlayingInfo - for _, playerId := range s.playMap.GetKeys() { - value, err := s.playMap.Get(playerId) + for _, playerId := range p.playMap.GetKeys() { + value, err := p.playMap.Get(playerId) if err != nil { continue } @@ -97,18 +105,56 @@ func (s *broker) GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) { return res, nil } -func (s *broker) Submit(ctx context.Context, trackId string, playTime time.Time) error { - u, _ := request.UserFrom(ctx) - t, err := s.ds.MediaFile(ctx).Get(trackId) - if err != nil { - log.Error(ctx, "Error retrieving mediaFile", "id", trackId, err) - return err +func (p *playTracker) Submit(ctx context.Context, submissions []Submission) error { + username, _ := request.UsernameFrom(ctx) + event := &events.RefreshResource{} + success := 0 + + for _, s := range submissions { + mf, err := p.ds.MediaFile(ctx).Get(s.TrackID) + if err != nil { + log.Error("Cannot find track for scrobbling", "id", s.TrackID, "user", username, err) + continue + } + err = p.incPlay(ctx, mf, s.Timestamp) + if err != nil { + log.Error("Error updating play counts", "id", mf.ID, "track", mf.Title, "user", username, err) + } else { + success++ + event.With("song", mf.ID).With("album", mf.AlbumID).With("artist", mf.AlbumArtistID) + log.Info("Scrobbled", "title", mf.Title, "artist", mf.Artist, "user", username) + _ = p.dispatchScrobble(ctx, mf, s.Timestamp) + } } + + if success > 0 { + p.broker.SendMessage(ctx, event) + } + return nil +} + +func (p *playTracker) incPlay(ctx context.Context, track *model.MediaFile, timestamp time.Time) error { + return p.ds.WithTx(func(tx model.DataStore) error { + err := p.ds.MediaFile(ctx).IncPlayCount(track.ID, timestamp) + if err != nil { + return err + } + err = p.ds.Album(ctx).IncPlayCount(track.AlbumID, timestamp) + if err != nil { + return err + } + err = p.ds.Artist(ctx).IncPlayCount(track.ArtistID, timestamp) + return err + }) +} + +func (p *playTracker) dispatchScrobble(ctx context.Context, t *model.MediaFile, playTime time.Time) error { + u, _ := request.UserFrom(ctx) scrobbles := []Scrobble{{MediaFile: *t, TimeStamp: playTime}} // TODO Parallelize for name, constructor := range scrobblers { err := func() error { - s := constructor(s.ds) + s := constructor(p.ds) if !s.IsAuthorized(ctx, u.ID) { return nil } diff --git a/core/scrobbler/broker_test.go b/core/scrobbler/play_tracker_test.go similarity index 70% rename from core/scrobbler/broker_test.go rename to core/scrobbler/play_tracker_test.go index 82faa05e5..5da80047b 100644 --- a/core/scrobbler/broker_test.go +++ b/core/scrobbler/play_tracker_test.go @@ -2,8 +2,11 @@ package scrobbler import ( "context" + "errors" "time" + "github.com/navidrome/navidrome/server/events" + "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" @@ -11,17 +14,19 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("Broker", func() { +var _ = Describe("PlayTracker", func() { var ctx context.Context var ds model.DataStore - var broker Broker + var broker PlayTracker var track model.MediaFile + var album model.Album + var artist model.Artist var fake *fakeScrobbler BeforeEach(func() { ctx = context.Background() ctx = request.WithUser(ctx, model.User{ID: "u-1"}) ds = &tests.MockDataStore{} - broker = GetBroker(ds) + broker = GetPlayTracker(ds, events.GetBroker()) fake = &fakeScrobbler{Authorized: true} Register("fake", func(ds model.DataStore) Scrobbler { return fake @@ -31,13 +36,19 @@ var _ = Describe("Broker", func() { ID: "123", Title: "Track Title", Album: "Track Album", + AlbumID: "al-1", Artist: "Track Artist", + ArtistID: "ar-1", AlbumArtist: "Track AlbumArtist", TrackNumber: 1, Duration: 180, MbzTrackID: "mbz-123", } _ = ds.MediaFile(ctx).Put(&track) + artist = model.Artist{ID: "ar-1"} + _ = ds.Artist(ctx).Put(&artist) + album = model.Album{ID: "al-1"} + _ = ds.Album(ctx).(*tests.MockAlbumRepo).Put(&album) }) Describe("NowPlaying", func() { @@ -50,9 +61,11 @@ var _ = Describe("Broker", func() { }) It("does not send track to agent if user has not authorized", func() { fake.Authorized = false + err := broker.NowPlaying(ctx, "player-1", "player-one", "123") + Expect(err).ToNot(HaveOccurred()) - Expect(fake.NowPlayingCalled).ToNot(BeTrue()) + Expect(fake.NowPlayingCalled).To(BeFalse()) }) }) @@ -90,7 +103,7 @@ var _ = Describe("Broker", func() { ctx = request.WithUser(ctx, model.User{ID: "u-1", UserName: "user-1"}) ts := time.Now() - err := broker.Submit(ctx, "123", ts) + err := broker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: ts}}) Expect(err).ToNot(HaveOccurred()) Expect(fake.ScrobbleCalled).To(BeTrue()) @@ -98,11 +111,38 @@ var _ = Describe("Broker", func() { Expect(fake.Scrobbles[0].ID).To(Equal("123")) }) + It("increments play counts in the DB", func() { + ctx = request.WithUser(ctx, model.User{ID: "u-1", UserName: "user-1"}) + ts := time.Now() + + err := broker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: ts}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(1))) + Expect(album.PlayCount).To(Equal(int64(1))) + Expect(artist.PlayCount).To(Equal(int64(1))) + }) + It("does not send track to agent if user has not authorized", func() { fake.Authorized = false - err := broker.Submit(ctx, "123", time.Now()) + + err := broker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + Expect(err).ToNot(HaveOccurred()) - Expect(fake.ScrobbleCalled).ToNot(BeTrue()) + Expect(fake.ScrobbleCalled).To(BeFalse()) + }) + + It("increments play counts even if it cannot scrobble", func() { + fake.Error = errors.New("error") + + err := broker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(fake.ScrobbleCalled).To(BeFalse()) + + Expect(track.PlayCount).To(Equal(int64(1))) + Expect(album.PlayCount).To(Equal(int64(1))) + Expect(artist.PlayCount).To(Equal(int64(1))) }) }) diff --git a/core/wire_providers.go b/core/wire_providers.go index 99fa050fe..b5b60cf8f 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -18,6 +18,6 @@ var Set = wire.NewSet( NewPlayers, agents.New, transcoder.New, - scrobbler.GetBroker, + scrobbler.GetPlayTracker, NewShare, ) diff --git a/server/subsonic/album_lists.go b/server/subsonic/album_lists.go index bf75ded22..8b1e5fef5 100644 --- a/server/subsonic/album_lists.go +++ b/server/subsonic/album_lists.go @@ -16,10 +16,10 @@ import ( type AlbumListController struct { ds model.DataStore - scrobbler scrobbler.Broker + scrobbler scrobbler.PlayTracker } -func NewAlbumListController(ds model.DataStore, scrobbler scrobbler.Broker) *AlbumListController { +func NewAlbumListController(ds model.DataStore, scrobbler scrobbler.PlayTracker) *AlbumListController { c := &AlbumListController{ ds: ds, scrobbler: scrobbler, diff --git a/server/subsonic/api.go b/server/subsonic/api.go index bfeb31c26..78e21e4df 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -34,11 +34,11 @@ type Router struct { ExternalMetadata core.ExternalMetadata Scanner scanner.Scanner Broker events.Broker - Scrobbler scrobbler.Broker + Scrobbler scrobbler.PlayTracker } func New(ds model.DataStore, artwork core.Artwork, streamer core.MediaStreamer, archiver core.Archiver, players core.Players, - externalMetadata core.ExternalMetadata, scanner scanner.Scanner, broker events.Broker, scrobbler scrobbler.Broker) *Router { + externalMetadata core.ExternalMetadata, scanner scanner.Scanner, broker events.Broker, scrobbler scrobbler.PlayTracker) *Router { r := &Router{ DataStore: ds, Artwork: artwork, diff --git a/server/subsonic/media_annotation.go b/server/subsonic/media_annotation.go index 9888e805b..a5aa106a2 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -18,11 +18,11 @@ import ( type MediaAnnotationController struct { ds model.DataStore - scrobbler scrobbler.Broker + scrobbler scrobbler.PlayTracker broker events.Broker } -func NewMediaAnnotationController(ds model.DataStore, scrobbler scrobbler.Broker, broker events.Broker) *MediaAnnotationController { +func NewMediaAnnotationController(ds model.DataStore, scrobbler scrobbler.PlayTracker, broker events.Broker) *MediaAnnotationController { return &MediaAnnotationController{ds: ds, scrobbler: scrobbler, broker: broker} } @@ -126,10 +126,25 @@ func (c *MediaAnnotationController) Scrobble(w http.ResponseWriter, r *http.Requ } submission := utils.ParamBool(r, "submission", true) ctx := r.Context() - event := &events.RefreshResource{} - submissions := 0 - log.Debug(r, "Scrobbling tracks", "ids", ids, "times", times, "submission", submission) + if submission { + err := c.scrobblerSubmit(ctx, ids, times) + if err != nil { + log.Error(ctx, "Error registering scrobbles", "ids", ids, "times", times, err) + } + } else { + err := c.scrobblerNowPlaying(ctx, ids[0]) + if err != nil { + log.Error(ctx, "Error setting NowPlaying", "id", ids[0], err) + } + } + + return newResponse(), nil +} + +func (c *MediaAnnotationController) scrobblerSubmit(ctx context.Context, ids []string, times []time.Time) error { + var submissions []scrobbler.Submission + log.Debug(ctx, "Scrobbling tracks", "ids", ids, "times", times) for i, id := range ids { var t time.Time if len(times) > 0 { @@ -137,57 +152,10 @@ func (c *MediaAnnotationController) Scrobble(w http.ResponseWriter, r *http.Requ } else { t = time.Now() } - if submission { - mf, err := c.scrobblerRegister(ctx, id, t) - if err != nil { - log.Error(r, "Error scrobbling track", "id", id, err) - continue - } - submissions++ - event.With("song", mf.ID).With("album", mf.AlbumID).With("artist", mf.AlbumArtistID) - } - if !submission || len(times) == 0 { - err := c.scrobblerNowPlaying(ctx, id) - if err != nil { - log.Error(r, "Error setting current song", "id", id, err) - continue - } - } + submissions = append(submissions, scrobbler.Submission{TrackID: id, Timestamp: t}) } - if submissions > 0 { - c.broker.SendMessage(ctx, event) - } - return newResponse(), nil -} -func (c *MediaAnnotationController) scrobblerRegister(ctx context.Context, trackId string, playTime time.Time) (*model.MediaFile, error) { - var mf *model.MediaFile - var err error - err = c.ds.WithTx(func(tx model.DataStore) error { - mf, err = c.ds.MediaFile(ctx).Get(trackId) - if err != nil { - return err - } - err = c.ds.MediaFile(ctx).IncPlayCount(trackId, playTime) - if err != nil { - return err - } - err = c.ds.Album(ctx).IncPlayCount(mf.AlbumID, playTime) - if err != nil { - return err - } - err = c.ds.Artist(ctx).IncPlayCount(mf.ArtistID, playTime) - return err - }) - - username, _ := request.UsernameFrom(ctx) - if err != nil { - log.Error("Error while scrobbling", "trackId", trackId, "user", username, err) - } else { - log.Info("Scrobbled", "title", mf.Title, "artist", mf.Artist, "user", username) - } - _ = c.scrobbler.Submit(ctx, trackId, playTime) - return mf, err + return c.scrobbler.Submit(ctx, submissions) } func (c *MediaAnnotationController) scrobblerNowPlaying(ctx context.Context, trackId string) error { diff --git a/server/subsonic/wire_gen.go b/server/subsonic/wire_gen.go index 44d651f89..8d76add06 100644 --- a/server/subsonic/wire_gen.go +++ b/server/subsonic/wire_gen.go @@ -25,16 +25,16 @@ func initBrowsingController(router *Router) *BrowsingController { func initAlbumListController(router *Router) *AlbumListController { dataStore := router.DataStore - broker := router.Scrobbler - albumListController := NewAlbumListController(dataStore, broker) + playTracker := router.Scrobbler + albumListController := NewAlbumListController(dataStore, playTracker) return albumListController } func initMediaAnnotationController(router *Router) *MediaAnnotationController { dataStore := router.DataStore - broker := router.Scrobbler - eventsBroker := router.Broker - mediaAnnotationController := NewMediaAnnotationController(dataStore, broker, eventsBroker) + playTracker := router.Scrobbler + broker := router.Broker + mediaAnnotationController := NewMediaAnnotationController(dataStore, playTracker, broker) return mediaAnnotationController } diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index 401dabc80..5f04ef572 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -2,17 +2,22 @@ package tests import ( "errors" + "time" + + "github.com/google/uuid" "github.com/navidrome/navidrome/model" ) func CreateMockAlbumRepo() *MockAlbumRepo { - return &MockAlbumRepo{} + return &MockAlbumRepo{ + data: make(map[string]*model.Album), + } } type MockAlbumRepo struct { model.AlbumRepository - data map[string]model.Album + data map[string]*model.Album all model.Albums err bool Options model.QueryOptions @@ -23,10 +28,10 @@ func (m *MockAlbumRepo) SetError(err bool) { } func (m *MockAlbumRepo) SetData(albums model.Albums) { - m.data = make(map[string]model.Album) + m.data = make(map[string]*model.Album) m.all = albums - for _, a := range m.all { - m.data[a.ID] = a + for i, a := range m.all { + m.data[a.ID] = &m.all[i] } } @@ -43,11 +48,22 @@ func (m *MockAlbumRepo) Get(id string) (*model.Album, error) { return nil, errors.New("Error!") } if d, ok := m.data[id]; ok { - return &d, nil + return d, nil } return nil, model.ErrNotFound } +func (m *MockAlbumRepo) Put(al *model.Album) error { + if m.err { + return errors.New("error") + } + if al.ID == "" { + al.ID = uuid.NewString() + } + m.data[al.ID] = al + return nil +} + func (m *MockAlbumRepo) GetAll(qo ...model.QueryOptions) (model.Albums, error) { if len(qo) > 0 { m.Options = qo[0] @@ -58,6 +74,18 @@ func (m *MockAlbumRepo) GetAll(qo ...model.QueryOptions) (model.Albums, error) { return m.all, nil } +func (m *MockAlbumRepo) IncPlayCount(id string, timestamp time.Time) error { + if m.err { + return errors.New("error") + } + if d, ok := m.data[id]; ok { + d.PlayCount++ + d.PlayDate = timestamp + return nil + } + return model.ErrNotFound +} + func (m *MockAlbumRepo) FindByArtist(artistId string) (model.Albums, error) { if m.err { return nil, errors.New("Error!") @@ -66,7 +94,7 @@ func (m *MockAlbumRepo) FindByArtist(artistId string) (model.Albums, error) { i := 0 for _, a := range m.data { if a.AlbumArtistID == artistId { - res[i] = a + res[i] = *a i++ } } diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index aa715bc04..6f30679fb 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -2,17 +2,22 @@ package tests import ( "errors" + "time" + + "github.com/google/uuid" "github.com/navidrome/navidrome/model" ) func CreateMockArtistRepo() *MockArtistRepo { - return &MockArtistRepo{} + return &MockArtistRepo{ + data: make(map[string]*model.Artist), + } } type MockArtistRepo struct { model.ArtistRepository - data map[string]model.Artist + data map[string]*model.Artist err bool } @@ -21,9 +26,9 @@ func (m *MockArtistRepo) SetError(err bool) { } func (m *MockArtistRepo) SetData(artists model.Artists) { - m.data = make(map[string]model.Artist) - for _, a := range artists { - m.data[a.ID] = a + m.data = make(map[string]*model.Artist) + for i, a := range artists { + m.data[a.ID] = &artists[i] } } @@ -40,9 +45,32 @@ func (m *MockArtistRepo) Get(id string) (*model.Artist, error) { return nil, errors.New("Error!") } if d, ok := m.data[id]; ok { - return &d, nil + return d, nil } return nil, model.ErrNotFound } +func (m *MockArtistRepo) Put(ar *model.Artist) error { + if m.err { + return errors.New("error") + } + if ar.ID == "" { + ar.ID = uuid.NewString() + } + m.data[ar.ID] = ar + return nil +} + +func (m *MockArtistRepo) IncPlayCount(id string, timestamp time.Time) error { + if m.err { + return errors.New("error") + } + if d, ok := m.data[id]; ok { + d.PlayCount++ + d.PlayDate = timestamp + return nil + } + return model.ErrNotFound +} + var _ model.ArtistRepository = (*MockArtistRepo)(nil) diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index 84ac4a2c9..7e4bb198c 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -2,6 +2,7 @@ package tests import ( "errors" + "time" "github.com/google/uuid" @@ -10,13 +11,13 @@ import ( func CreateMockMediaFileRepo() *MockMediaFileRepo { return &MockMediaFileRepo{ - data: make(map[string]model.MediaFile), + data: make(map[string]*model.MediaFile), } } type MockMediaFileRepo struct { model.MediaFileRepository - data map[string]model.MediaFile + data map[string]*model.MediaFile err bool } @@ -25,9 +26,9 @@ func (m *MockMediaFileRepo) SetError(err bool) { } func (m *MockMediaFileRepo) SetData(mfs model.MediaFiles) { - m.data = make(map[string]model.MediaFile) - for _, mf := range mfs { - m.data[mf.ID] = mf + m.data = make(map[string]*model.MediaFile) + for i, mf := range mfs { + m.data[mf.ID] = &mfs[i] } } @@ -44,22 +45,34 @@ func (m *MockMediaFileRepo) Get(id string) (*model.MediaFile, error) { return nil, errors.New("Error!") } if d, ok := m.data[id]; ok { - return &d, nil + return d, nil } return nil, model.ErrNotFound } func (m *MockMediaFileRepo) Put(mf *model.MediaFile) error { if m.err { - return errors.New("error!") + return errors.New("error") } if mf.ID == "" { mf.ID = uuid.NewString() } - m.data[mf.ID] = *mf + m.data[mf.ID] = mf return nil } +func (m *MockMediaFileRepo) IncPlayCount(id string, timestamp time.Time) error { + if m.err { + return errors.New("error") + } + if d, ok := m.data[id]; ok { + d.PlayCount++ + d.PlayDate = timestamp + return nil + } + return model.ErrNotFound +} + func (m *MockMediaFileRepo) FindByAlbum(artistId string) (model.MediaFiles, error) { if m.err { return nil, errors.New("Error!") @@ -68,7 +81,7 @@ func (m *MockMediaFileRepo) FindByAlbum(artistId string) (model.MediaFiles, erro i := 0 for _, a := range m.data { if a.AlbumID == artistId { - res[i] = a + res[i] = *a i++ } }