diff --git a/core/scrobbler/play_tracker.go b/core/scrobbler/play_tracker.go index e21db42d2..63397f8f6 100644 --- a/core/scrobbler/play_tracker.go +++ b/core/scrobbler/play_tracker.go @@ -2,6 +2,7 @@ package scrobbler import ( "context" + "encoding/json" "maps" "slices" "sync" @@ -11,6 +12,7 @@ import ( "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/utils/cache" @@ -43,6 +45,10 @@ type PlaybackSession struct { PositionMs int64 PlaybackRate float64 LastReport time.Time + + // Verdict from the last report, for the expiry callback: its context carries only + // a stub user, so it cannot evaluate the filter itself. + filtered bool } type Submission struct { @@ -68,8 +74,9 @@ type nowPlayingEntry struct { } type playbackReportEntry struct { - ctx context.Context - info PlaybackSession + ctx context.Context + info PlaybackSession + filtered bool } type PlayTracker interface { @@ -145,7 +152,7 @@ func newPlayTracker(ds model.DataStore, broker events.Broker, pluginManager Plug log.Trace("Enqueueing PlaybackReport for expired session", "session", info) info.State = StateExpired info.LastReport = time.Now() - p.enqueuePlaybackReport(ctx, info) + p.enqueuePlaybackReport(ctx, info, info.filtered) } }) @@ -273,6 +280,10 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP now := time.Now() + // One verdict per report, reused by every dispatch below, so a filter reading + // annotations cannot decide differently on either side of incPlay. + var filtered bool + switch params.State { case StateStarting: // Clients may send starting/playing unordered; a late "starting" must not downgrade @@ -285,8 +296,10 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP if err != nil { return err } + filtered = p.isFilteredOut(ctx, mf) info := PlaybackSession{ MediaFile: *mf, + filtered: filtered, Start: now, UserId: user.ID, Username: user.UserName, @@ -309,7 +322,7 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP if err != nil { log.Warn(ctx, "Error adding PlaybackSession to cache", "clientId", clientId, "mediaId", params.MediaId, "state", params.State, err) } - p.enqueuePlaybackReport(ctx, info) + p.enqueuePlaybackReport(ctx, info, filtered) case StatePlaying, StatePaused: info, getErr := p.playMap.Get(clientId) @@ -331,6 +344,8 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP info.PositionMs = params.PositionMs info.PlaybackRate = params.PlaybackRate info.LastReport = now + filtered = p.isFilteredOut(ctx, &info.MediaFile) + info.filtered = filtered ttl := 30 * time.Minute if params.State == StatePlaying { ttl = remainingTTL(info.MediaFile.Duration, params.PositionMs, params.PlaybackRate) @@ -342,16 +357,19 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP if err != nil { log.Warn(ctx, "Error updating PlaybackSession in cache", "clientId", clientId, "mediaId", params.MediaId, "state", params.State, err) } - p.enqueuePlaybackReport(ctx, info) + p.enqueuePlaybackReport(ctx, info, filtered) case StateStopped: var loadedMF *model.MediaFile + haveVerdict := false if !params.IgnoreScrobble && player.ScrobbleEnabled { mf, err := p.ds.MediaFile(ctx).GetWithParticipants(params.MediaId) if err != nil { return err } loadedMF = mf + filtered = p.isFilteredOut(ctx, mf) + haveVerdict = true trackDurationMs := int64(mf.Duration * 1000) threshold := min(trackDurationMs*50/100, 240_000) if params.PositionMs >= threshold { @@ -359,7 +377,7 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP if err != nil { log.Warn(ctx, "Error updating play counts", "id", mf.ID, "track", mf.Title, "user", user.UserName, err) } - p.dispatchScrobble(ctx, mf, now) + p.dispatchScrobble(ctx, mf, now, filtered) } } p.sessionsMu.Lock() @@ -397,7 +415,10 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP } stoppedInfo.MediaFile = *mf } - p.enqueuePlaybackReport(ctx, stoppedInfo) + if !haveVerdict { + filtered = p.isFilteredOut(ctx, &stoppedInfo.MediaFile) + } + p.enqueuePlaybackReport(ctx, stoppedInfo, filtered) } if conf.Server.EnableNowPlaying { @@ -413,7 +434,9 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP // scrobbler plugins) returned by getActiveScrobblers; see dispatchNowPlaying. if player.ScrobbleEnabled && (params.State == StateStarting || params.State == StatePlaying) { - if info, err := p.playMap.Get(clientId); err == nil { + if filtered { + log.Debug(ctx, "Ignoring external NowPlaying update for filtered track", "mediaId", params.MediaId) + } else if info, err := p.playMap.Get(clientId); err == nil { p.enqueueNowPlaying(ctx, clientId, user.ID, &info.MediaFile, int(params.PositionMs/1000)) } } @@ -452,6 +475,7 @@ func (p *playTracker) Submit(ctx context.Context, submissions []Submission) erro log.Error(ctx, "Cannot find track for scrobbling", "id", s.TrackID, "user", username, err) continue } + filtered := p.isFilteredOut(ctx, mf) err = p.incPlay(ctx, mf, s.Timestamp) if err != nil { log.Error(ctx, "Error updating play counts", "id", mf.ID, "track", mf.Title, "user", username, err) @@ -460,7 +484,7 @@ func (p *playTracker) Submit(ctx context.Context, submissions []Submission) erro event.With("song", mf.ID).With("album", mf.AlbumID).With("artist", mf.AlbumArtistID) log.Info(ctx, "Scrobbled", "title", mf.Title, "artist", mf.Artist, "user", username, "timestamp", s.Timestamp) if player.ScrobbleEnabled { - p.dispatchScrobble(ctx, mf, s.Timestamp) + p.dispatchScrobble(ctx, mf, s.Timestamp, filtered) } } } @@ -494,11 +518,36 @@ func (p *playTracker) incPlay(ctx context.Context, track *model.MediaFile, times }) } -func (p *playTracker) dispatchScrobble(ctx context.Context, t *model.MediaFile, playTime time.Time) { +// Take this verdict before incPlay mutates what a filter reads, and independently of +// which scrobblers are active: it can be stored on a session and dispatched much later. +// Any parse or query failure fails open, because filtering must not break scrobbling. +func (p *playTracker) isFilteredOut(ctx context.Context, t *model.MediaFile) bool { + u, _ := request.UserFrom(ctx) + if u.ScrobbleFilter == "" { + return false + } + var c criteria.Criteria + if err := json.Unmarshal([]byte(u.ScrobbleFilter), &c); err != nil { + log.Warn(ctx, "Invalid scrobble filter, ignoring", "user", u.UserName, err) + return false + } + match, err := p.ds.MediaFile(ctx).MatchesCriteria(t.ID, c) + if err != nil { + log.Warn(ctx, "Error evaluating scrobble filter, ignoring", "user", u.UserName, "track", t.Title, err) + return false + } + return match +} + +func (p *playTracker) dispatchScrobble(ctx context.Context, t *model.MediaFile, playTime time.Time, filtered bool) { if t.Artist == consts.UnknownArtist { log.Debug(ctx, "Ignoring external Scrobble for track with unknown artist", "track", t.Title, "artist", t.Artist) return } + if filtered { + log.Debug(ctx, "Ignoring external Scrobble for filtered track", "track", t.Title, "artist", t.Artist) + return + } allScrobblers := p.getActiveScrobblers() u, _ := request.UserFrom(ctx) diff --git a/core/scrobbler/play_tracker_test.go b/core/scrobbler/play_tracker_test.go index f49d9a0bf..72f802c2e 100644 --- a/core/scrobbler/play_tracker_test.go +++ b/core/scrobbler/play_tracker_test.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/tests" @@ -46,6 +47,26 @@ func (m *mockPluginLoader) LoadScrobbler(name string) (Scrobbler, bool) { return s, ok } +// flipOnPlayRepo reports one filter verdict before the play is counted and another +// after, reproducing a filter that reads annotations incPlay mutates. +type flipOnPlayRepo struct { + model.MediaFileRepository + before, after bool + played atomic.Bool +} + +func (r *flipOnPlayRepo) IncPlayCount(id string, ts time.Time) error { + r.played.Store(true) + return r.MediaFileRepository.IncPlayCount(id, ts) +} + +func (r *flipOnPlayRepo) MatchesCriteria(string, criteria.Criteria) (bool, error) { + if r.played.Load() { + return r.after, nil + } + return r.before, nil +} + // slowMediaFileRepo widens the window between a report's session check and its // write, making check-then-write races reproducible. type slowMediaFileRepo struct { @@ -320,6 +341,185 @@ var _ = Describe("PlayTracker", func() { }) }) + Describe("Scrobble filter", func() { + var repo *tests.MockMediaFileRepo + + BeforeEach(func() { + ctx = request.WithUser(ctx, model.User{ID: "u-1", UserName: "user-1", + ScrobbleFilter: `{"all":[{"contains":{"title":"Track"}}]}`}) + repo = ds.MediaFile(ctx).(*tests.MockMediaFileRepo) + }) + + It("does not send a matching track to the agent", func() { + repo.MatchesCriteriaValue = true + + err := tracker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(fake.ScrobbleCalled.Load()).To(BeFalse()) + }) + + It("still increments play counts for a filtered track", func() { + repo.MatchesCriteriaValue = true + + err := tracker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(1))) + Expect(album.PlayCount).To(Equal(int64(1))) + }) + + It("sends a non-matching track to the agent", func() { + repo.MatchesCriteriaValue = false + + err := tracker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(fake.ScrobbleCalled.Load()).To(BeTrue()) + }) + + It("fails open when evaluation errors", func() { + repo.MatchesCriteriaErr = errors.New("boom") + + err := tracker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(fake.ScrobbleCalled.Load()).To(BeTrue()) + }) + + It("fails open when the stored filter is not valid JSON", func() { + ctx = request.WithUser(ctx, model.User{ID: "u-1", UserName: "user-1", ScrobbleFilter: `{broken`}) + repo.MatchesCriteriaValue = true + + err := tracker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(fake.ScrobbleCalled.Load()).To(BeTrue()) + }) + + It("does not send now-playing for a filtered track", func() { + repo.MatchesCriteriaValue = true + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", State: StateStarting, ClientId: "player-1", ClientName: "player"}) + + Expect(err).ToNot(HaveOccurred()) + Consistently(func() bool { return fake.GetNowPlayingCalled() }).Should(BeFalse()) + }) + + It("does not send playback reports for a filtered track", func() { + repo.MatchesCriteriaValue = true + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", State: StateStarting, ClientId: "player-1", ClientName: "player"}) + + Expect(err).ToNot(HaveOccurred()) + Consistently(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeFalse()) + }) + + It("sends playback reports for a non-matching track", func() { + repo.MatchesCriteriaValue = false + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", State: StateStarting, ClientId: "player-1", ClientName: "player"}) + + Expect(err).ToNot(HaveOccurred()) + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + }) + + It("evaluates the filter even when no scrobbler is active yet", func() { + // The verdict is stored on the session and dispatched at expiry, by which + // time a plugin scrobbler may have been enabled. + tracker.builtinScrobblers = map[string]Scrobbler{} + repo.MatchesCriteriaValue = true + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", State: StatePlaying, ClientId: "player-12", ClientName: "player"}) + + Expect(err).ToNot(HaveOccurred()) + stored, getErr := tracker.playMap.Get("player-12") + Expect(getErr).ToNot(HaveOccurred()) + Expect(stored.filtered).To(BeTrue()) + }) + + Context("when incPlay itself flips the filter", func() { + // A filter on playCount or lastPlayed changes verdict the moment incPlay + // commits, so the verdict has to be taken before it, not at dispatch time. + var flip *flipOnPlayRepo + + install := func(before, after bool) { + flip = &flipOnPlayRepo{MediaFileRepository: ds.MediaFile(ctx), before: before, after: after} + ds.(*tests.MockDataStore).MockedMediaFile = flip + } + + It("does not scrobble a track the filter matched before the play was counted", func() { + install(true, false) + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", State: StateStopped, PositionMs: 120_000, ClientId: "player-1", ClientName: "player"}) + + Expect(err).ToNot(HaveOccurred()) + Expect(flip.played.Load()).To(BeTrue(), "incPlay must still have run") + Expect(fake.ScrobbleCalled.Load()).To(BeFalse()) + }) + + It("still sends the stopped report when the filter only starts matching after the play", func() { + install(false, true) + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", State: StateStopped, PositionMs: 120_000, ClientId: "player-1", ClientName: "player"}) + + Expect(err).ToNot(HaveOccurred()) + Expect(flip.played.Load()).To(BeTrue(), "incPlay must still have run") + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + }) + + It("does not report an expired session for a filtered track", func() { + // The expiry callback runs with a stub user, so it cannot evaluate the + // filter itself and must reuse the verdict stored on the session. + info := PlaybackSession{ + MediaFile: track, Start: time.Now(), UserId: "u-1", Username: "user-1", + PlayerId: "player-9", PlayerName: "test-player", State: StatePlaying, filtered: true, + } + _ = tracker.playMap.AddWithTTL("player-9", info, 10*time.Millisecond) + + Consistently(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeFalse()) + }) + + It("still reports an expired session for a track that is not filtered", func() { + info := PlaybackSession{ + MediaFile: track, Start: time.Now(), UserId: "u-1", Username: "user-1", + PlayerId: "player-10", PlayerName: "test-player", State: StatePlaying, filtered: false, + } + _ = tracker.playMap.AddWithTTL("player-10", info, 10*time.Millisecond) + + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + }) + + It("stores the verdict on the session so expiry can reuse it", func() { + repo.MatchesCriteriaValue = true + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", State: StatePlaying, ClientId: "player-11", ClientName: "player"}) + + Expect(err).ToNot(HaveOccurred()) + stored, getErr := tracker.playMap.Get("player-11") + Expect(getErr).ToNot(HaveOccurred()) + Expect(stored.filtered).To(BeTrue()) + }) + + It("does not scrobble a Submit whose filter matched before the play was counted", func() { + install(true, false) + + err := tracker.Submit(ctx, []Submission{{TrackID: "123", Timestamp: time.Now()}}) + + Expect(err).ToNot(HaveOccurred()) + Expect(flip.played.Load()).To(BeTrue(), "incPlay must still have run") + Expect(fake.ScrobbleCalled.Load()).To(BeFalse()) + }) + }) + }) + Describe("ReportPlayback", func() { const defaultClientId = "client-1" diff --git a/core/scrobbler/playbackreport_worker.go b/core/scrobbler/playbackreport_worker.go index 78ca6e0f7..799dac616 100644 --- a/core/scrobbler/playbackreport_worker.go +++ b/core/scrobbler/playbackreport_worker.go @@ -6,13 +6,14 @@ import ( "github.com/navidrome/navidrome/log" ) -func (p *playTracker) enqueuePlaybackReport(ctx context.Context, info PlaybackSession) { +func (p *playTracker) enqueuePlaybackReport(ctx context.Context, info PlaybackSession, filtered bool) { p.prMu.Lock() defer p.prMu.Unlock() ctx = context.WithoutCancel(ctx) p.prQueue = append(p.prQueue, playbackReportEntry{ - ctx: ctx, - info: info, + ctx: ctx, + info: info, + filtered: filtered, }) p.sendPlaybackReportSignal() } @@ -44,12 +45,16 @@ func (p *playTracker) playbackReportWorker() { allScrobblers := p.getActiveScrobblers() for _, entry := range entries { - p.dispatchPlaybackReport(entry.ctx, entry.info, allScrobblers) + p.dispatchPlaybackReport(entry.ctx, entry.info, allScrobblers, entry.filtered) } } } -func (p *playTracker) dispatchPlaybackReport(ctx context.Context, info PlaybackSession, allScrobblers map[string]Scrobbler) { +func (p *playTracker) dispatchPlaybackReport(ctx context.Context, info PlaybackSession, allScrobblers map[string]Scrobbler, filtered bool) { + if filtered { + log.Debug(ctx, "Ignoring external PlaybackReport for filtered track", "track", info.MediaFile.Title, "state", info.State) + return + } for name, s := range allScrobblers { if !s.IsAuthorized(ctx, info.UserId) { continue diff --git a/db/migrations/20260815015320_add_user_scrobble_filter.sql b/db/migrations/20260815015320_add_user_scrobble_filter.sql new file mode 100644 index 000000000..82ed95f3d --- /dev/null +++ b/db/migrations/20260815015320_add_user_scrobble_filter.sql @@ -0,0 +1,5 @@ +-- +goose Up +alter table user add column scrobble_filter varchar default '' not null; + +-- +goose Down +alter table user drop column scrobble_filter; diff --git a/model/mediafile.go b/model/mediafile.go index ac1fad67b..888425d07 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -15,6 +15,7 @@ import ( "github.com/gohugoio/hashstructure" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/number" @@ -548,6 +549,9 @@ type MediaFileRepository interface { // filters as GetAll. Sort/Order are ignored. GetRandom(options ...QueryOptions) (MediaFiles, error) GetAllByTags(tag TagName, values []string, options ...QueryOptions) (MediaFiles, error) + // MatchesCriteria reports whether the media file matches the criteria's rule + // expression, using the logged user's annotations. Limit and offset are ignored. + MatchesCriteria(id string, c criteria.Criteria) (bool, error) GetCursor(options ...QueryOptions) (MediaFileCursor, error) // GetAllIDs returns just the media_file IDs for the same row set as GetAll. GetAllIDs(options ...QueryOptions) ([]string, error) diff --git a/model/user.go b/model/user.go index 1c8541ccf..b6f792c9a 100644 --- a/model/user.go +++ b/model/user.go @@ -14,6 +14,8 @@ type User struct { LastAccessAt *time.Time `structs:"last_access_at" json:"lastAccessAt"` CreatedAt time.Time `structs:"created_at" json:"createdAt"` UpdatedAt time.Time `structs:"updated_at" json:"updatedAt"` + // Smart-playlist criteria JSON; matching songs are not sent to external scrobblers + ScrobbleFilter string `structs:"scrobble_filter" json:"scrobbleFilter"` // Library associations (many-to-many relationship) Libraries Libraries `structs:"-" json:"libraries,omitempty"` diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 70f9a1fb5..890f757d7 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -129,7 +129,7 @@ var smartPlaylistFields = map[string]smartPlaylistField{ "random": {order: "random()"}, } -func (c smartPlaylistCriteria) Where() (squirrel.Sqlizer, error) { +func (c smartPlaylistCriteria) where() (squirrel.Sqlizer, error) { if c.Criteria.Expression == nil { return squirrel.Expr("1 = 1"), nil } @@ -786,7 +786,7 @@ func fieldJoinType(name string) smartPlaylistJoinType { return field.joinType } -func (c smartPlaylistCriteria) ExpressionJoins() smartPlaylistJoinType { +func (c smartPlaylistCriteria) expressionJoins() smartPlaylistJoinType { var joins smartPlaylistJoinType _ = criteria.Walk(c.Criteria.Expression, func(expr criteria.Expression) error { for field := range criteria.Fields(expr) { @@ -797,15 +797,50 @@ func (c smartPlaylistCriteria) ExpressionJoins() smartPlaylistJoinType { return joins } -func (c smartPlaylistCriteria) RequiredJoins() smartPlaylistJoinType { - joins := c.ExpressionJoins() +func (c smartPlaylistCriteria) requiredJoins() smartPlaylistJoinType { + joins := c.expressionJoins() for _, name := range c.Criteria.SortFieldNames() { joins |= fieldJoinType(name) } return joins } -func (c smartPlaylistCriteria) OrderBy() string { +// applyExpressionJoins adds every join the criteria's WHERE clause resolves against. +func (c smartPlaylistCriteria) applyExpressionJoins(sq squirrel.SelectBuilder, userID string) squirrel.SelectBuilder { + return c.applyJoins(sq, c.expressionJoins(), userID) +} + +// applyRequiredJoins adds the WHERE joins plus any the ORDER BY resolves against. +func (c smartPlaylistCriteria) applyRequiredJoins(sq squirrel.SelectBuilder, userID string) squirrel.SelectBuilder { + return c.applyJoins(sq, c.requiredJoins(), userID) +} + +// applyJoins joins the media_file annotation unconditionally — annotation fields +// COALESCE a missing row to a default, so the row has to be reachable to be absent. +func (c smartPlaylistCriteria) applyJoins(sq squirrel.SelectBuilder, joins smartPlaylistJoinType, userID string) squirrel.SelectBuilder { + sq = sq.LeftJoin("annotation on ("+ + "annotation.item_id = media_file.id"+ + " AND annotation.item_type = 'media_file'"+ + " AND annotation.user_id = ?)", userID) + if joins.has(smartPlaylistJoinAlbumAnnotation) { + sq = sq.LeftJoin("annotation AS album_annotation ON ("+ + "album_annotation.item_id = media_file.album_id"+ + " AND album_annotation.item_type = 'album'"+ + " AND album_annotation.user_id = ?)", userID) + } + if joins.has(smartPlaylistJoinArtistAnnotation) { + sq = sq.LeftJoin("annotation AS artist_annotation ON ("+ + "artist_annotation.item_id = media_file.artist_id"+ + " AND artist_annotation.item_type = 'artist'"+ + " AND artist_annotation.user_id = ?)", userID) + } + if joins.has(smartPlaylistJoinAlbum) { + sq = sq.LeftJoin("album ON album.id = media_file.album_id") + } + return sq +} + +func (c smartPlaylistCriteria) orderBy() string { sortFields := c.Criteria.OrderByFields() parts := make([]string, 0, len(sortFields)) for _, sf := range sortFields { diff --git a/persistence/criteria_sql_benchmark_test.go b/persistence/criteria_sql_benchmark_test.go index 1dcf97871..523a5d825 100644 --- a/persistence/criteria_sql_benchmark_test.go +++ b/persistence/criteria_sql_benchmark_test.go @@ -124,7 +124,7 @@ func benchmarkCriteriaPipeline(b *testing.B, ctx context.Context, expr criteria. // Build the full query matching buildSmartPlaylistQuery + addCriteria sq := squirrel.Select("media_file.id").From("media_file") - cond, err := cSQL.Where() + cond, err := cSQL.where() if err != nil { b.Fatal(err) } @@ -132,7 +132,7 @@ func benchmarkCriteriaPipeline(b *testing.B, ctx context.Context, expr criteria. if expr.Limit > 0 { sq = sq.Limit(uint64(expr.Limit)) } - if order := cSQL.OrderBy(); order != "" { + if order := cSQL.orderBy(); order != "" { sq = sq.OrderBy(order) } diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 0cd7187c5..8d0069b0d 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -20,7 +20,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { DescribeTable("expressions", func(expr criteria.Expression, expectedSQL string, expectedArgs ...any) { - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -204,7 +204,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { sqlizer, err := newSmartPlaylistCriteria( criteria.Criteria{Expression: criteria.InPlaylist{"id": "deadbeef-dead-beef"}}, withSmartPlaylistOwner(model.User{ID: "owner-id", IsAdmin: false}), - ).Where() + ).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -217,7 +217,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { sqlizer, err := newSmartPlaylistCriteria( criteria.Criteria{Expression: criteria.InPlaylist{"id": "deadbeef-dead-beef"}}, withSmartPlaylistOwner(model.User{ID: "admin-id", IsAdmin: true}), - ).Where() + ).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -228,7 +228,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { }) It("builds relative date expressions", func() { - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheLast{"lastPlayed": 30}}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheLast{"lastPlayed": 30}}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -238,7 +238,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { }) It("builds negated relative date expressions", func() { - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.NotInTheLast{"lastPlayed": 30}}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.NotInTheLast{"lastPlayed": 30}}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -248,76 +248,76 @@ var _ = Describe("Smart playlist criteria SQL", func() { }) It("returns an error for unknown fields", func() { - _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.EndsWith{"unknown": "value"}}).Where() + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.EndsWith{"unknown": "value"}}).where() Expect(err).To(MatchError("invalid field in criteria: unknown")) }) It("returns an error when isMissing is used with a regular field", func() { - _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"year": true}}).Where() + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"year": true}}).where() Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field"))) }) It("returns an error when isPresent is used with a regular field", func() { - _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsPresent{"title": true}}).Where() + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsPresent{"title": true}}).where() Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field"))) }) It("returns an error when isMissing has a non-boolean value", func() { - _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"genre": "hello"}}).Where() + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"genre": "hello"}}).where() Expect(err).To(MatchError(ContainSubstring("invalid boolean value for 'missing' expression"))) }) It("returns an error for a range over a tag/role field", func() { - _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"rate": []int{1, 5}}}).Where() + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"rate": []int{1, 5}}}).where() Expect(err).To(MatchError(ContainSubstring("range operator not supported for tag/role field"))) }) It("returns a clear error for a malformed range value", func() { - _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"playCount": []int{1, 2, 3}}}).Where() + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"playCount": []int{1, 2, 3}}}).where() Expect(err).To(MatchError(ContainSubstring("must be a [min, max] pair"))) }) Describe("sort", func() { It("sorts by regular fields", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title"}).OrderBy()).To(Equal("media_file.title asc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title"}).orderBy()).To(Equal("media_file.title asc")) }) It("sorts by tag fields", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "genre"}).OrderBy()).To(Equal("COALESCE(json_extract(media_file.tags, '$.genre[0].value'), '') asc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "genre"}).orderBy()).To(Equal("COALESCE(json_extract(media_file.tags, '$.genre[0].value'), '') asc")) }) It("sorts by role fields", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "artist"}).OrderBy()).To(Equal("COALESCE(json_extract(media_file.participants, '$.artist[0].name'), '') asc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "artist"}).orderBy()).To(Equal("COALESCE(json_extract(media_file.participants, '$.artist[0].name'), '') asc")) }) It("casts numeric tags when sorting", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "rate"}).OrderBy()).To(Equal("CAST(COALESCE(json_extract(media_file.tags, '$.rate[0].value'), '') AS REAL) asc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "rate"}).orderBy()).To(Equal("CAST(COALESCE(json_extract(media_file.tags, '$.rate[0].value'), '') AS REAL) asc")) }) It("sorts by albumtype alias", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "albumtype"}).OrderBy()).To(Equal("COALESCE(json_extract(media_file.tags, '$.releasetype[0].value'), '') asc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "albumtype"}).orderBy()).To(Equal("COALESCE(json_extract(media_file.tags, '$.releasetype[0].value'), '') asc")) }) It("sorts by random", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "random"}).OrderBy()).To(Equal("random() asc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "random"}).orderBy()).To(Equal("random() asc")) }) It("sorts by album columns bare, with no COALESCE default", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-albumDateAdded,trackNumber"}).OrderBy()). + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-albumDateAdded,trackNumber"}).orderBy()). To(Equal("album.created_at desc, media_file.track_number asc")) }) It("sorts by multiple fields", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title,-rating"}).OrderBy()).To(Equal("media_file.title asc, COALESCE(annotation.rating, 0) desc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title,-rating"}).orderBy()).To(Equal("media_file.title asc, COALESCE(annotation.rating, 0) desc")) }) It("reverts order when order is desc", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-date,artist", Order: "desc"}).OrderBy()).To(Equal("media_file.date asc, COALESCE(json_extract(media_file.participants, '$.artist[0].name'), '') desc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-date,artist", Order: "desc"}).orderBy()).To(Equal("media_file.date asc, COALESCE(json_extract(media_file.participants, '$.artist[0].name'), '') desc")) }) It("ignores invalid sort fields", func() { - Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "bogus,title"}).OrderBy()).To(Equal("media_file.title asc")) + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "bogus,title"}).orderBy()).To(Equal("media_file.title asc")) }) }) @@ -362,7 +362,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Contains{"artist": "Kraftwerk"}, criteria.Contains{"artist": "Pink Floyd"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -376,7 +376,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Contains{"artist": "Beatles"}, criteria.Contains{"composer": "Lennon"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, _, err := sqlizer.ToSql() @@ -391,7 +391,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.NotContains{"artist": "Beatles"}, criteria.NotContains{"artist": "Kraftwerk"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, _, err := sqlizer.ToSql() @@ -406,7 +406,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { for i := range anyExprs { anyExprs[i] = criteria.Contains{"artist": fmt.Sprintf("Artist%d", i)} } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: anyExprs}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: anyExprs}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -424,7 +424,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Contains{"artist": "Beatles"}, criteria.Contains{"artist": "Kraftwerk"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -440,7 +440,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Contains{"genre": "Metal"}, criteria.Contains{"genre": "Punk"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -454,7 +454,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Contains{"genre": "Rock"}, criteria.Contains{"mood": "Happy"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, _, err := sqlizer.ToSql() @@ -467,7 +467,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.NotContains{"genre": "Rock"}, criteria.NotContains{"genre": "Metal"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, _, err := sqlizer.ToSql() @@ -482,7 +482,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Contains{"genre": "Rock"}, criteria.Contains{"genre": "Metal"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -499,7 +499,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.IsNot{"artist": "Beatles"}, criteria.IsNot{"artist": "Kraftwerk"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -515,7 +515,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.NotContains{"artist": "Beatles"}, criteria.NotContains{"artist": "Kraftwerk"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -530,7 +530,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.NotContains{"genre": "Rock"}, criteria.NotContains{"genre": "Metal"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -547,7 +547,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Contains{"artist": "Beatles"}, criteria.IsNot{"artist": "Kraftwerk"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, _, err := sqlizer.ToSql() @@ -562,7 +562,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.IsNot{"artist": "Beatles"}, criteria.IsNot{"composer": "Lennon"}, } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where() Expect(err).ToNot(HaveOccurred()) sql, _, err := sqlizer.ToSql() @@ -575,7 +575,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { for i := range allExprs { allExprs[i] = criteria.IsNot{"artist": fmt.Sprintf("Artist%d", i)} } - sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: allExprs}).Where() + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: allExprs}).where() Expect(err).ToNot(HaveOccurred()) sql, args, err := sqlizer.ToSql() @@ -591,14 +591,14 @@ var _ = Describe("Smart playlist criteria SQL", func() { c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "albumRating"} cSQL := newSmartPlaylistCriteria(c) - Expect(cSQL.ExpressionJoins()).To(Equal(smartPlaylistJoinNone)) - Expect(cSQL.RequiredJoins().has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue()) + Expect(cSQL.expressionJoins()).To(Equal(smartPlaylistJoinNone)) + Expect(cSQL.requiredJoins().has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue()) }) It("includes expression-based joins", func() { c := criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumRating": 3}}} - Expect(newSmartPlaylistCriteria(c).ExpressionJoins().has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue()) + Expect(newSmartPlaylistCriteria(c).expressionJoins().has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue()) }) It("detects nested album and artist joins", func() { @@ -607,7 +607,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { criteria.Any{criteria.Gt{"artistPlayCount": 10}}, }} - joins := newSmartPlaylistCriteria(c).RequiredJoins() + joins := newSmartPlaylistCriteria(c).requiredJoins() Expect(joins.has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue()) Expect(joins.has(smartPlaylistJoinArtistAnnotation)).To(BeTrue()) }) @@ -615,20 +615,20 @@ var _ = Describe("Smart playlist criteria SQL", func() { It("detects join types from sort fields with direction prefixes", func() { c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "-artistRating"} - Expect(newSmartPlaylistCriteria(c).RequiredJoins().has(smartPlaylistJoinArtistAnnotation)).To(BeTrue()) + Expect(newSmartPlaylistCriteria(c).requiredJoins().has(smartPlaylistJoinArtistAnnotation)).To(BeTrue()) }) It("keeps a sort-only album join out of the expression joins", func() { c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "-albumDateAdded"} cSQL := newSmartPlaylistCriteria(c) - Expect(cSQL.ExpressionJoins()).To(Equal(smartPlaylistJoinNone)) - Expect(cSQL.RequiredJoins().has(smartPlaylistJoinAlbum)).To(BeTrue()) + Expect(cSQL.expressionJoins()).To(Equal(smartPlaylistJoinNone)) + Expect(cSQL.requiredJoins().has(smartPlaylistJoinAlbum)).To(BeTrue()) }) It("distinguishes the album join from the album annotation join", func() { c := criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumRating": 3}}} - joins := newSmartPlaylistCriteria(c).RequiredJoins() + joins := newSmartPlaylistCriteria(c).requiredJoins() Expect(joins.has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue()) Expect(joins.has(smartPlaylistJoinAlbum)).To(BeFalse()) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index f37e394cc..5f0addc5e 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -15,6 +15,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/utils/slice" "github.com/navidrome/navidrome/utils/str" "github.com/pocketbase/dbx" @@ -517,6 +518,23 @@ var mediaFileSearchConfig = searchConfig{ MBIDFields: []string{"mbz_recording_id", "mbz_release_track_id"}, } +func (r *mediaFileRepository) MatchesCriteria(id string, c criteria.Criteria) (bool, error) { + usr := loggedUser(r.ctx) + rulesSQL := newSmartPlaylistCriteria(c, withSmartPlaylistOwner(*usr)) + cond, err := rulesSQL.where() + if err != nil { + return false, err + } + sq := Select("count(*) as count").From("media_file") + sq = rulesSQL.applyExpressionJoins(sq, usr.ID) + sq = sq.Where(And{Eq{"media_file.id": id}, cond}) + var res struct{ Count int64 } + if err := r.queryOne(sq, &res); err != nil { + return false, err + } + return res.Count > 0, nil +} + func (r *mediaFileRepository) Search(q string, options ...model.QueryOptions) (model.MediaFiles, error) { var opts model.QueryOptions if len(options) > 0 { diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index b9cba03ff..ba89b5def 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -13,6 +13,7 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/model/request" . "github.com/onsi/ginkgo/v2" @@ -1129,4 +1130,31 @@ var _ = Describe("MediaRepository", func() { Expect(NewMediaFileRepository(rctx, GetDBXBuilder()).Exists(songAntenna.ID)).To(BeFalse()) }) }) + + Describe("MatchesCriteria", func() { + It("returns true when the track matches", func() { + c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "Day"}}} + match, err := mr.MatchesCriteria(songDayInALife.ID, c) + Expect(err).ToNot(HaveOccurred()) + Expect(match).To(BeTrue()) + }) + It("returns false when the track does not match", func() { + c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "Nickelback"}}} + match, err := mr.MatchesCriteria(songDayInALife.ID, c) + Expect(err).ToNot(HaveOccurred()) + Expect(match).To(BeFalse()) + }) + It("treats missing annotations as their COALESCE default", func() { + // unrated track: rating coalesces to 0, so "rating < 4" matches + c := criteria.Criteria{Expression: criteria.All{criteria.Lt{"rating": 4}}} + match, err := mr.MatchesCriteria(songDayInALife.ID, c) + Expect(err).ToNot(HaveOccurred()) + Expect(match).To(BeTrue()) + }) + It("returns an error for an invalid field", func() { + c := criteria.Criteria{Expression: criteria.All{criteria.Is{"bogusfield": 1}}} + _, err := mr.MatchesCriteria(songDayInALife.ID, c) + Expect(err).To(HaveOccurred()) + }) + }) }) diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 5f84981b3..65ae4656b 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -119,13 +119,11 @@ func (r *playlistRepository) resolvePercentageLimit(pls *model.Playlist, rulesSQ return nil } - exprJoins := rulesSQL.ExpressionJoins() countSq := Select("count(*) as count").From("media_file") - countSq = r.addMediaFileAnnotationJoin(countSq, userID) - countSq = r.addSmartPlaylistJoins(countSq, exprJoins, userID) + countSq = rulesSQL.applyExpressionJoins(countSq, userID) countSq = r.applyLibraryFilter(countSq, "media_file") - cond, err := rulesSQL.Where() + cond, err := rulesSQL.where() if err != nil { log.Error(r.ctx, "Error building smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err) return err @@ -146,50 +144,18 @@ func (r *playlistRepository) resolvePercentageLimit(pls *model.Playlist, rulesSQ // buildSmartPlaylistQuery constructs the SQL query to select media files matching the smart playlist criteria, // including the joins its fields require and library filtering. func (r *playlistRepository) buildSmartPlaylistQuery(pls *model.Playlist, rulesSQL smartPlaylistCriteria, userID string) SelectBuilder { - orderBy := rulesSQL.OrderBy() + orderBy := rulesSQL.orderBy() sq := Select("row_number() over (order by "+orderBy+") as id", "'"+pls.ID+"' as playlist_id", "media_file.id as media_file_id"). From("media_file") - sq = r.addMediaFileAnnotationJoin(sq, userID) - - requiredJoins := rulesSQL.RequiredJoins() - sq = r.addSmartPlaylistJoins(sq, requiredJoins, userID) + sq = rulesSQL.applyRequiredJoins(sq, userID) sq = r.applyLibraryFilter(sq, "media_file") return sq } -// addMediaFileAnnotationJoin adds a left join to the annotation table for media files, filtering by user ID to include -// user-specific annotations in the smart playlist criteria evaluation. -func (r *playlistRepository) addMediaFileAnnotationJoin(sq SelectBuilder, userID string) SelectBuilder { - return sq.LeftJoin("annotation on ("+ - "annotation.item_id = media_file.id"+ - " AND annotation.item_type = 'media_file'"+ - " AND annotation.user_id = ?)", userID) -} - -// addSmartPlaylistJoins adds the left joins required by the criteria's fields. -func (r *playlistRepository) addSmartPlaylistJoins(sq SelectBuilder, joins smartPlaylistJoinType, userID string) SelectBuilder { - if joins.has(smartPlaylistJoinAlbumAnnotation) { - sq = sq.LeftJoin("annotation AS album_annotation ON ("+ - "album_annotation.item_id = media_file.album_id"+ - " AND album_annotation.item_type = 'album'"+ - " AND album_annotation.user_id = ?)", userID) - } - if joins.has(smartPlaylistJoinArtistAnnotation) { - sq = sq.LeftJoin("annotation AS artist_annotation ON ("+ - "artist_annotation.item_id = media_file.artist_id"+ - " AND artist_annotation.item_type = 'artist'"+ - " AND artist_annotation.user_id = ?)", userID) - } - if joins.has(smartPlaylistJoinAlbum) { - sq = sq.LeftJoin("album ON album.id = media_file.album_id") - } - return sq -} - // addCriteria applies the where conditions, limit, offset, and order by clauses to the SQL query based on the // smart playlist criteria. func (r *playlistRepository) addCriteria(sql SelectBuilder, cSQL smartPlaylistCriteria) (SelectBuilder, error) { - cond, err := cSQL.Where() + cond, err := cSQL.where() if err != nil { return sql, err } @@ -197,7 +163,7 @@ func (r *playlistRepository) addCriteria(sql SelectBuilder, cSQL smartPlaylistCr if cSQL.Criteria.Limit > 0 { sql = sql.Limit(uint64(cSQL.Criteria.Limit)).Offset(uint64(cSQL.Criteria.Offset)) } - if order := cSQL.OrderBy(); order != "" { + if order := cSQL.orderBy(); order != "" { sql = sql.OrderBy(order) } return sql, nil diff --git a/persistence/user_repository.go b/persistence/user_repository.go index 9decff4e5..3c030a640 100644 --- a/persistence/user_repository.go +++ b/persistence/user_repository.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/slice" @@ -252,6 +253,9 @@ func (r *userRepository) Save(entity any) (string, error) { if err := validateUsernameUnique(r, u); err != nil { return "", err } + if err := validateScrobbleFilter(u); err != nil { + return "", err + } err := r.Put(u) if err != nil { return "", err @@ -284,6 +288,9 @@ func (r *userRepository) Update(id string, entity any, _ ...string) error { if err := validateUsernameUnique(r, u); err != nil { return err } + if err := validateScrobbleFilter(u); err != nil { + return err + } err := r.Put(u) if errors.Is(err, model.ErrNotFound) { return rest.ErrNotFound @@ -331,6 +338,33 @@ func validateUsernameUnique(r model.UserRepository, u *model.User) error { return nil } +func validateScrobbleFilter(u *model.User) error { + u.ScrobbleFilter = strings.TrimSpace(u.ScrobbleFilter) + if u.ScrobbleFilter == "" { + return nil + } + var c criteria.Criteria + if err := json.Unmarshal([]byte(u.ScrobbleFilter), &c); err != nil { + return invalidScrobbleFilter() + } + // A filter is a per-track test, so a result-set size means nothing here. Reject it + // rather than silently ignoring part of a rule copied from a smart playlist. + if c.Limit > 0 || c.LimitPercent > 0 || c.Offset > 0 || c.RefreshDelay > 0 { + return invalidScrobbleFilter() + } + // Building the WHERE clause is what validates field names and operators + if _, err := newSmartPlaylistCriteria(c).where(); err != nil { + return invalidScrobbleFilter() + } + return nil +} + +func invalidScrobbleFilter() error { + return &rest.ValidationError{Errors: map[string]string{ + "scrobbleFilter": "resources.user.validation.invalidScrobbleFilter", + }} +} + func (r *userRepository) Delete(id string) error { usr := loggedUser(r.ctx) if !usr.IsAdmin { diff --git a/persistence/user_repository_test.go b/persistence/user_repository_test.go index 6f8ab9161..ec417c193 100644 --- a/persistence/user_repository_test.go +++ b/persistence/user_repository_test.go @@ -70,6 +70,26 @@ var _ = Describe("UserRepository", func() { Expect(err).ToNot(HaveOccurred()) Expect(actual.Password).To(Equal("newpass")) }) + It("persists and reads back the scrobble filter", func() { + usr := model.User{ID: "u-filter", UserName: "u-filter", Name: "Filter User", + ScrobbleFilter: `{"all":[{"contains":{"title":"????"}}]}`} + Expect(repo.Put(&usr)).To(Succeed()) + + saved, err := repo.Get("u-filter") + Expect(err).ToNot(HaveOccurred()) + Expect(saved.ScrobbleFilter).To(Equal(`{"all":[{"contains":{"title":"????"}}]}`)) + }) + It("reads back a user row inserted without scrobble_filter", func() { + // Guards the column's NOT NULL DEFAULT '': rows predating the migration must stay scannable + _, err := GetDBXBuilder().NewQuery( + "insert into user (id, user_name, name, email, password, created_at, updated_at) " + + "values ('u-rawsql', 'u-rawsql', 'Raw', '', '', datetime('now'), datetime('now'))").Execute() + Expect(err).ToNot(HaveOccurred()) + + saved, err := repo.Get("u-rawsql") + Expect(err).ToNot(HaveOccurred()) + Expect(saved.ScrobbleFilter).To(Equal("")) + }) }) Describe("validatePasswordChange", func() { @@ -607,6 +627,52 @@ var _ = Describe("UserRepository", func() { }) }) + Describe("validateScrobbleFilter", func() { + It("accepts an empty filter", func() { + u := &model.User{} + Expect(validateScrobbleFilter(u)).To(Succeed()) + }) + It("trims a whitespace-only filter to empty", func() { + u := &model.User{ScrobbleFilter: " "} + Expect(validateScrobbleFilter(u)).To(Succeed()) + Expect(u.ScrobbleFilter).To(Equal("")) + }) + It("accepts valid criteria JSON", func() { + u := &model.User{ScrobbleFilter: `{"all":[{"lt":{"rating":4}}]}`} + Expect(validateScrobbleFilter(u)).To(Succeed()) + }) + It("rejects malformed JSON", func() { + u := &model.User{ScrobbleFilter: `{not json`} + var vErr *rest.ValidationError + err := validateScrobbleFilter(u) + Expect(errors.As(err, &vErr)).To(BeTrue()) + Expect(vErr.Errors).To(HaveKey("scrobbleFilter")) + }) + It("rejects criteria without rules", func() { + u := &model.User{ScrobbleFilter: `{"sort":"title"}`} + Expect(validateScrobbleFilter(u)).ToNot(Succeed()) + }) + It("rejects selection options that mean nothing for a single track", func() { + for _, f := range []string{ + `{"all":[{"lt":{"rating":4}}],"limit":100}`, + `{"all":[{"lt":{"rating":4}}],"limitPercent":10}`, + `{"all":[{"lt":{"rating":4}}],"offset":5}`, + `{"all":[{"lt":{"rating":4}}],"refreshDelay":"1h"}`, + } { + u := &model.User{ScrobbleFilter: f} + Expect(validateScrobbleFilter(u)).ToNot(Succeed(), f) + } + }) + It("accepts a sort, which cannot change a single-track match", func() { + u := &model.User{ScrobbleFilter: `{"all":[{"lt":{"rating":4}}],"sort":"title"}`} + Expect(validateScrobbleFilter(u)).To(Succeed()) + }) + It("rejects unknown fields", func() { + u := &model.User{ScrobbleFilter: `{"all":[{"is":{"bogusfield":1}}]}`} + Expect(validateScrobbleFilter(u)).ToNot(Succeed()) + }) + }) + Describe("filters", func() { It("qualifies id filter with table name", func() { r := repo.(*userRepository) diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index b3b3bab2f..9a6338ae1 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -155,11 +155,13 @@ "newPassword": "Nova Senha", "token": "Token", "lastAccessAt": "Últ. Acesso", - "libraries": "Bibliotecas" + "libraries": "Bibliotecas", + "scrobbleFilter": "Filtro de scrobble" }, "helperTexts": { "name": "Alterações no seu nome só serão refletidas no próximo login", - "libraries": "Selecione bibliotecas específicas para este usuário, ou deixe vazio para usar bibliotecas padrão" + "libraries": "Selecione bibliotecas específicas para este usuário, ou deixe vazio para usar bibliotecas padrão", + "scrobbleFilter": "Músicas que correspondem a estas regras de playlist inteligente não são enviadas ao Last.fm, ListenBrainz ou plugins de scrobble. Usa a mesma sintaxe JSON e o mesmo comportamento das playlists inteligentes. Exemplo: {\"all\":[{\"lt\":{\"rating\":4}}]}. Deixe vazio para enviar scrobbles de todas as músicas. As contagens de reprodução locais não são afetadas." }, "notifications": { "created": "Novo usuário criado", @@ -173,7 +175,8 @@ "adminAutoLibraries": "Usuários administradores têm acesso automático a todas as bibliotecas" }, "validation": { - "librariesRequired": "Pelo menos uma biblioteca deve ser selecionada para usuários não-administradores" + "librariesRequired": "Pelo menos uma biblioteca deve ser selecionada para usuários não-administradores", + "invalidScrobbleFilter": "Devem ser regras válidas de playlist inteligente. Limite, deslocamento e intervalo de atualização não são suportados." } }, "player": { diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index d993af1a8..58e84785b 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -9,6 +9,7 @@ import ( "github.com/deluan/rest" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/utils/slice" ) @@ -31,6 +32,8 @@ type MockMediaFileRepo struct { // Add fields for cross-library move detection tests FindRecentFilesByMBZTrackIDFunc func(missing model.MediaFile, since time.Time) (model.MediaFiles, error) FindRecentFilesByPropertiesFunc func(missing model.MediaFile, since time.Time) (model.MediaFiles, error) + MatchesCriteriaValue bool + MatchesCriteriaErr error } func (m *MockMediaFileRepo) SetError(err bool) { @@ -369,5 +372,12 @@ func (m *MockMediaFileRepo) FindRecentFilesByProperties(missing model.MediaFile, return result, nil } +func (m *MockMediaFileRepo) MatchesCriteria(string, criteria.Criteria) (bool, error) { + if m.MatchesCriteriaErr != nil { + return false, m.MatchesCriteriaErr + } + return m.MatchesCriteriaValue, nil +} + var _ model.MediaFileRepository = (*MockMediaFileRepo)(nil) var _ model.ResourceRepository = (*MockMediaFileRepo)(nil) diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index c0e226453..12968afbd 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -155,11 +155,13 @@ "currentPassword": "Current Password", "newPassword": "New Password", "token": "Token", - "libraries": "Libraries" + "libraries": "Libraries", + "scrobbleFilter": "Scrobble filter" }, "helperTexts": { "name": "Changes to your name will only be reflected on next login", - "libraries": "Select specific libraries for this user, or leave empty to use default libraries" + "libraries": "Select specific libraries for this user, or leave empty to use default libraries", + "scrobbleFilter": "Songs matching these smart playlist rules are not sent to Last.fm, ListenBrainz or scrobbler plugins. Uses the same JSON syntax and behavior as smart playlists. Example: {\"all\":[{\"lt\":{\"rating\":4}}]}. Leave empty to scrobble everything. Local play counts are not affected." }, "notifications": { "created": "User created", @@ -167,7 +169,8 @@ "deleted": "User deleted" }, "validation": { - "librariesRequired": "At least one library must be selected for non-admin users" + "librariesRequired": "At least one library must be selected for non-admin users", + "invalidScrobbleFilter": "Must be valid smart playlist rules. Limit, offset and refresh delay are not supported." }, "message": { "listenBrainzToken": "Enter your ListenBrainz user token.", diff --git a/ui/src/user/UserEdit.jsx b/ui/src/user/UserEdit.jsx index d8302a9f9..c5d9c75a4 100644 --- a/ui/src/user/UserEdit.jsx +++ b/ui/src/user/UserEdit.jsx @@ -172,6 +172,17 @@ const UserEdit = (props) => { )} + + diff --git a/ui/src/user/UserEdit.test.jsx b/ui/src/user/UserEdit.test.jsx index 1d8290569..74405cc13 100644 --- a/ui/src/user/UserEdit.test.jsx +++ b/ui/src/user/UserEdit.test.jsx @@ -127,6 +127,12 @@ describe('', () => { expect(screen.getByTestId('date-field-createdAt')).toBeInTheDocument() }) + it('should render the scrobble filter input', () => { + render() + + expect(screen.getByTestId('text-input-scrobbleFilter')).toBeInTheDocument() + }) + it('should not render username input for non-admin users', () => { render() diff --git a/ui/src/user/userValidation.js b/ui/src/user/userValidation.js index e90fd2acb..9eb1b8f05 100644 --- a/ui/src/user/userValidation.js +++ b/ui/src/user/userValidation.js @@ -15,5 +15,15 @@ export const validateUserForm = (values, translate) => { } } + if (values.scrobbleFilter && values.scrobbleFilter.trim() !== '') { + try { + JSON.parse(values.scrobbleFilter) + } catch { + errors.scrobbleFilter = translate( + 'resources.user.validation.invalidScrobbleFilter', + ) + } + } + return errors } diff --git a/ui/src/user/userValidation.test.js b/ui/src/user/userValidation.test.js index 2ee473910..de52c17df 100644 --- a/ui/src/user/userValidation.test.js +++ b/ui/src/user/userValidation.test.js @@ -67,4 +67,27 @@ describe('User Validation Utilities', () => { ) }) }) + + describe('scrobbleFilter validation', () => { + it('accepts an empty filter', () => { + const errors = validateUserForm({ isAdmin: true }, mockTranslate) + expect(errors.scrobbleFilter).toBeUndefined() + }) + it('accepts valid JSON', () => { + const errors = validateUserForm( + { isAdmin: true, scrobbleFilter: '{"all":[{"lt":{"rating":4}}]}' }, + mockTranslate, + ) + expect(errors.scrobbleFilter).toBeUndefined() + }) + it('rejects malformed JSON', () => { + const errors = validateUserForm( + { isAdmin: true, scrobbleFilter: '{broken' }, + mockTranslate, + ) + expect(errors.scrobbleFilter).toEqual( + 'resources.user.validation.invalidScrobbleFilter', + ) + }) + }) })