navidrome/persistence/playlist_repository_test.go
Deluan Quintão 1bd736dae9
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: update FieldInfo structure and simplify fieldMap initialization

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: move sort parsing logic from persistence to criteria package

Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.

* refactor: standardize field access in smartPlaylistCriteria structure

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: add ResolveLimit method to Criteria

Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.

* refactor: improve child playlist loading and error handling in refresh logic

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: extract smart playlist logic to dedicated files

Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.

* refactor: break refreshSmartPlaylist into smaller focused methods

Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.

* refactor: deduplicate child playlist IDs in Criteria

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: simplify withSmartPlaylistOwner to accept model.User

Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.

* fix: handle empty sort fields and propagate child playlist load errors

OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.

* refactor: log warning when no valid sort fields are found

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00

197 lines
6.5 KiB
Go

package persistence
import (
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)
var _ = Describe("PlaylistRepository", func() {
var repo model.PlaylistRepository
BeforeEach(func() {
ctx := log.NewContext(GinkgoT().Context())
ctx = request.WithUser(ctx, model.User{ID: "userid", UserName: "userid", IsAdmin: true})
repo = NewPlaylistRepository(ctx, GetDBXBuilder())
})
Describe("Count", func() {
It("returns the number of playlists in the DB", func() {
Expect(repo.CountAll()).To(Equal(int64(2)))
})
})
Describe("Exists", func() {
It("returns true for an existing playlist", func() {
Expect(repo.Exists(plsCool.ID)).To(BeTrue())
})
It("returns false for a non-existing playlist", func() {
Expect(repo.Exists("666")).To(BeFalse())
})
})
Describe("Get", func() {
It("returns an existing playlist", func() {
p, err := repo.Get(plsBest.ID)
Expect(err).To(BeNil())
// Compare all but Tracks and timestamps
p2 := *p
p2.Tracks = plsBest.Tracks
p2.UpdatedAt = plsBest.UpdatedAt
p2.CreatedAt = plsBest.CreatedAt
Expect(p2).To(Equal(plsBest))
// Compare tracks
for i := range p.Tracks {
Expect(p.Tracks[i].ID).To(Equal(plsBest.Tracks[i].ID))
}
})
It("returns ErrNotFound for a non-existing playlist", func() {
_, err := repo.Get("666")
Expect(err).To(MatchError(model.ErrNotFound))
})
It("returns all tracks", func() {
pls, err := repo.GetWithTracks(plsBest.ID, true, false)
Expect(err).ToNot(HaveOccurred())
Expect(pls.Name).To(Equal(plsBest.Name))
Expect(pls.Tracks).To(HaveLen(2))
Expect(pls.Tracks[0].ID).To(Equal("1"))
Expect(pls.Tracks[0].PlaylistID).To(Equal(plsBest.ID))
Expect(pls.Tracks[0].MediaFileID).To(Equal(songDayInALife.ID))
Expect(pls.Tracks[0].MediaFile.ID).To(Equal(songDayInALife.ID))
Expect(pls.Tracks[1].ID).To(Equal("2"))
Expect(pls.Tracks[1].PlaylistID).To(Equal(plsBest.ID))
Expect(pls.Tracks[1].MediaFileID).To(Equal(songRadioactivity.ID))
Expect(pls.Tracks[1].MediaFile.ID).To(Equal(songRadioactivity.ID))
mfs := pls.MediaFiles()
Expect(mfs).To(HaveLen(2))
Expect(mfs[0].ID).To(Equal(songDayInALife.ID))
Expect(mfs[1].ID).To(Equal(songRadioactivity.ID))
})
})
It("Put/Exists/Delete", func() {
By("saves the playlist to the DB")
newPls := model.Playlist{Name: "Great!", OwnerID: "userid"}
newPls.AddMediaFilesByID([]string{"1004", "1003"})
By("saves the playlist to the DB")
Expect(repo.Put(&newPls)).To(BeNil())
By("adds repeated songs to a playlist and keeps the order")
newPls.AddMediaFilesByID([]string{"1004"})
Expect(repo.Put(&newPls)).To(BeNil())
saved, _ := repo.GetWithTracks(newPls.ID, true, false)
Expect(saved.Tracks).To(HaveLen(3))
Expect(saved.Tracks[0].MediaFileID).To(Equal("1004"))
Expect(saved.Tracks[1].MediaFileID).To(Equal("1003"))
Expect(saved.Tracks[2].MediaFileID).To(Equal("1004"))
By("returns the newly created playlist")
Expect(repo.Exists(newPls.ID)).To(BeTrue())
By("returns deletes the playlist")
Expect(repo.Delete(newPls.ID)).To(BeNil())
By("returns error if tries to retrieve the deleted playlist")
Expect(repo.Exists(newPls.ID)).To(BeFalse())
})
Describe("GetAll", func() {
It("returns all playlists from DB", func() {
all, err := repo.GetAll()
Expect(err).To(BeNil())
Expect(all[0].ID).To(Equal(plsBest.ID))
Expect(all[1].ID).To(Equal(plsCool.ID))
})
})
Describe("GetPlaylists", func() {
It("returns playlists for a track", func() {
pls, err := repo.GetPlaylists(songRadioactivity.ID)
Expect(err).ToNot(HaveOccurred())
Expect(pls).To(HaveLen(1))
Expect(pls[0].ID).To(Equal(plsBest.ID))
})
It("returns empty when none", func() {
pls, err := repo.GetPlaylists("9999")
Expect(err).ToNot(HaveOccurred())
Expect(pls).To(HaveLen(0))
})
})
Describe("Track Deletion and Renumbering", func() {
var testPlaylistID string
AfterEach(func() {
if testPlaylistID != "" {
Expect(repo.Delete(testPlaylistID)).To(BeNil())
testPlaylistID = ""
}
})
// helper to get track positions and media file IDs
getTrackInfo := func(playlistID string) (ids []string, mediaFileIDs []string) {
pls, err := repo.GetWithTracks(playlistID, false, false)
Expect(err).ToNot(HaveOccurred())
for _, t := range pls.Tracks {
ids = append(ids, t.ID)
mediaFileIDs = append(mediaFileIDs, t.MediaFileID)
}
return
}
It("renumbers correctly after deleting a track from the middle", func() {
By("creating a playlist with 4 tracks")
newPls := model.Playlist{Name: "Renumber Test Middle", OwnerID: "userid"}
newPls.AddMediaFilesByID([]string{"1001", "1002", "1003", "1004"})
Expect(repo.Put(&newPls)).To(Succeed())
testPlaylistID = newPls.ID
By("deleting the second track (position 2)")
tracksRepo := repo.Tracks(newPls.ID, false)
Expect(tracksRepo.Delete("2")).To(Succeed())
By("verifying remaining tracks are renumbered sequentially")
ids, mediaFileIDs := getTrackInfo(newPls.ID)
Expect(ids).To(Equal([]string{"1", "2", "3"}))
Expect(mediaFileIDs).To(Equal([]string{"1001", "1003", "1004"}))
})
It("renumbers correctly after deleting the first track", func() {
By("creating a playlist with 3 tracks")
newPls := model.Playlist{Name: "Renumber Test First", OwnerID: "userid"}
newPls.AddMediaFilesByID([]string{"1001", "1002", "1003"})
Expect(repo.Put(&newPls)).To(Succeed())
testPlaylistID = newPls.ID
By("deleting the first track (position 1)")
tracksRepo := repo.Tracks(newPls.ID, false)
Expect(tracksRepo.Delete("1")).To(Succeed())
By("verifying remaining tracks are renumbered sequentially")
ids, mediaFileIDs := getTrackInfo(newPls.ID)
Expect(ids).To(Equal([]string{"1", "2"}))
Expect(mediaFileIDs).To(Equal([]string{"1002", "1003"}))
})
It("renumbers correctly after deleting the last track", func() {
By("creating a playlist with 3 tracks")
newPls := model.Playlist{Name: "Renumber Test Last", OwnerID: "userid"}
newPls.AddMediaFilesByID([]string{"1001", "1002", "1003"})
Expect(repo.Put(&newPls)).To(Succeed())
testPlaylistID = newPls.ID
By("deleting the last track (position 3)")
tracksRepo := repo.Tracks(newPls.ID, false)
Expect(tracksRepo.Delete("3")).To(Succeed())
By("verifying remaining tracks are renumbered sequentially")
ids, mediaFileIDs := getTrackInfo(newPls.ID)
Expect(ids).To(Equal([]string{"1", "2"}))
Expect(mediaFileIDs).To(Equal([]string{"1001", "1002"}))
})
})
})