diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go index bd7636706..26f14393e 100644 --- a/model/criteria/criteria_test.go +++ b/model/criteria/criteria_test.go @@ -369,6 +369,14 @@ var _ = Describe("Criteria", func() { ids := goObj.ChildPlaylistPaths() gomega.Expect(ids).To(gomega.ConsistOf(topLevelInPlaylistPath, nestedAnyNotInPlaylistPath)) }) + It("ignores empty child playlist paths", func() { + c := Criteria{Expression: All{InPlaylist{"path": ""}, NotInPlaylist{"path": ""}}} + gomega.Expect(c.ChildPlaylistPaths()).To(gomega.BeEmpty()) + }) + It("ignores empty child playlist ids", func() { + c := Criteria{Expression: All{InPlaylist{"id": ""}, NotInPlaylist{"id": ""}}} + gomega.Expect(c.ChildPlaylistIds()).To(gomega.BeEmpty()) + }) It("extracts child smart playlist IDs from deeply nested expression", func() { goObj = Criteria{ Expression: Any{ diff --git a/model/criteria/operators.go b/model/criteria/operators.go index c41d9f692..d49eb1f1e 100644 --- a/model/criteria/operators.go +++ b/model/criteria/operators.go @@ -191,7 +191,7 @@ func extractPlaylistField(inputRule any, field string) (values []string) { values = append(values, extractPlaylistField(rules, field)...) } case InPlaylist, NotInPlaylist: - if value, ok := rule.(Expression).fields()[field].(string); ok { + if value, ok := rule.(Expression).fields()[field].(string); ok && value != "" { values = append(values, value) } } diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index fc89d09b9..c61d9b3bf 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -345,7 +345,7 @@ func startOfPeriod(numDays int64, from time.Time) string { func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squirrel.Sqlizer, error) { var condition squirrel.Sqlizer - if playlistId, ok := values["id"].(string); ok { + if playlistId, ok := values["id"].(string); ok && playlistId != "" { condition = squirrel.Eq{"pl.playlist_id": playlistId} } else if playlistPath, ok := values["path"].(string); ok && playlistPath != "" { condition = squirrel.Eq{"playlist.path": playlistPath} diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 4b1df2dd0..905edfded 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -49,6 +49,7 @@ var _ = Describe("Smart playlist criteria SQL", func() { Entry("after", criteria.After{"lastPlayed": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "annotation.play_date > ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)), Entry("in playlist [path]", criteria.InPlaylist{"path": "lacuslacus.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path = ? AND playlist.public = ?))", "lacuslacus.nsp", 1), Entry("in playlist [id]", criteria.InPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), + Entry("in playlist [empty id falls back to path]", criteria.InPlaylist{"id": "", "path": "/music/x.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path = ? AND playlist.public = ?))", "/music/x.nsp", 1), Entry("not in playlist", criteria.NotInPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id NOT IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), Entry("album annotation", criteria.Gt{"albumRating": 3}, "album_annotation.rating > ?", 3), Entry("artist annotation", criteria.Is{"artistLoved": true}, "artist_annotation.starred = ?", true), diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index c359b3f2e..ea28e1563 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -97,9 +97,17 @@ func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL return true } - childPlaylists, err := r.GetAll(model.QueryOptions{Filters: Or{Eq{"playlist.id": childPlaylistIds}, Eq{"playlist.path": childPlaylistPaths}}}) + var conditions Or + if len(childPlaylistIds) > 0 { + conditions = append(conditions, Eq{"playlist.id": childPlaylistIds}) + } + if len(childPlaylistPaths) > 0 { + conditions = append(conditions, Eq{"playlist.path": childPlaylistPaths}) + } + + childPlaylists, err := r.GetAll(model.QueryOptions{Filters: conditions}) if err != nil { - log.Error(r.ctx, "Error loading child playlists for smart playlist refresh", "playlist", pls.Name, "id", pls.ID, "childIds", childPlaylistIds, err) + log.Error(r.ctx, "Error loading child playlists for smart playlist refresh", "playlist", pls.Name, "id", pls.ID, "childIds", childPlaylistIds, "childPaths", childPlaylistPaths, err) return false } diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 5f21b5376..b462191bd 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -174,6 +174,29 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { }) }) + It("does not treat an empty path as a reference to every playlist without a path", func() { + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + + bystander := model.Playlist{Name: "Bystander", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.Contains{"title": "Day"}}, + }} + Expect(repo.Put(&bystander)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(bystander.ID) }) + + parent := model.Playlist{Name: "Empty Path", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.InPlaylist{"path": ""}}, + }} + Expect(repo.Put(&parent)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(parent.ID) }) + + _, err := repo.GetWithTracks(parent.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + + reloaded, err := repo.Get(bystander.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(reloaded.EvaluatedAt).To(BeNil()) + }) + When("refresh delay has not expired", func() { It("should NOT refresh tracks for smart playlist referenced in parent smart playlist criteria", func() { conf.Server.SmartPlaylistRefreshDelay = 1 * time.Hour