From 11e4aaed1ba3a1cc6420facb888cbbcf4589b445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 4 Mar 2026 22:42:49 -0500 Subject: [PATCH] feat(server): add percentage-based limits to smart playlists (#5144) * feat(playlists): add percentage-based limits to smart playlists Add a new `limitPercent` JSON field to Criteria that allows smart playlist limits to be expressed as a percentage of matching tracks rather than a fixed number. For example, a playlist matching 450 songs with a 10% limit returns 45 songs, scaling dynamically as the library grows. When `limitPercent` is set, refreshSmartPlaylist runs a COUNT query first to determine the total matching tracks, then resolves the percentage to an absolute LIMIT before executing the main query. The fixed `limit` field takes precedence when both are set. Values are clamped to [0, 100] during JSON unmarshaling. No database migration is needed since rules are stored as a JSON string. * fix(criteria): validate percentage limit range in IsPercentageLimit method Signed-off-by: Deluan * fix(criteria): ensure idempotency of ToSql method for expressions Signed-off-by: Deluan --------- Signed-off-by: Deluan --- core/playlists/parse_nsp_test.go | 15 +++ model/criteria/criteria.go | 91 ++++++++++++++---- model/criteria/criteria_test.go | 142 +++++++++++++++++++++++++++++ model/criteria/fields.go | 25 +++-- model/criteria/operators_test.go | 15 +++ persistence/playlist_repository.go | 54 ++++++++--- 6 files changed, 296 insertions(+), 46 deletions(-) diff --git a/core/playlists/parse_nsp_test.go b/core/playlists/parse_nsp_test.go index 0a7b2727..516a5355 100644 --- a/core/playlists/parse_nsp_test.go +++ b/core/playlists/parse_nsp_test.go @@ -122,6 +122,21 @@ var _ = Describe("parseNSP", func() { Expect(pls.Name).To(Equal("Original")) }) + It("parses limitPercent from NSP", func() { + nsp := `{ + "all": [{"is": {"loved": true}}], + "sort": "playCount", + "order": "desc", + "limitPercent": 25 + }` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Rules).ToNot(BeNil()) + Expect(pls.Rules.LimitPercent).To(Equal(25)) + Expect(pls.Rules.Limit).To(Equal(0)) + }) + It("parses criteria with multiple rules", func() { nsp := `{ "all": [ diff --git a/model/criteria/criteria.go b/model/criteria/criteria.go index bc3fe801..278acf34 100644 --- a/model/criteria/criteria.go +++ b/model/criteria/criteria.go @@ -15,10 +15,38 @@ type Expression = squirrel.Sqlizer type Criteria struct { Expression - Sort string - Order string - Limit int - Offset int + Sort string + Order string + Limit int + LimitPercent int + Offset int +} + +// EffectiveLimit resolves the effective limit for a query. If a fixed Limit is +// set it takes precedence. Otherwise, if LimitPercent is set, the limit is +// computed as a percentage of totalCount (minimum 1 when totalCount > 0). +// Returns 0 when no limit applies. +func (c Criteria) EffectiveLimit(totalCount int64) int { + if c.Limit > 0 { + return c.Limit + } + if c.LimitPercent > 0 && c.LimitPercent <= 100 { + if totalCount <= 0 { + return 0 + } + result := int(totalCount) * c.LimitPercent / 100 + if result < 1 { + return 1 + } + return result + } + return 0 +} + +// IsPercentageLimit returns true when the criteria uses a valid percentage-based +// limit (i.e. LimitPercent is in [1, 100] and no fixed Limit overrides it). +func (c Criteria) IsPercentageLimit() bool { + return c.Limit == 0 && c.LimitPercent > 0 && c.LimitPercent <= 100 } func (c Criteria) OrderBy() string { @@ -95,6 +123,16 @@ func (c Criteria) ToSql() (sql string, args []any, err error) { return c.Expression.ToSql() } +// ExpressionJoins returns only the JOINs needed by the WHERE-clause expression, +// excluding any JOINs required solely for sorting. This is useful for COUNT +// queries where sort order is irrelevant. +func (c Criteria) ExpressionJoins() JoinType { + if c.Expression == nil { + return JoinNone + } + return extractJoinTypes(c.Expression) +} + // RequiredJoins inspects the expression tree and Sort field to determine which // additional JOINs are needed when evaluating this criteria. func (c Criteria) RequiredJoins() JoinType { @@ -128,17 +166,19 @@ func (c Criteria) ChildPlaylistIds() []string { func (c Criteria) MarshalJSON() ([]byte, error) { aux := struct { - All []Expression `json:"all,omitempty"` - Any []Expression `json:"any,omitempty"` - Sort string `json:"sort,omitempty"` - Order string `json:"order,omitempty"` - Limit int `json:"limit,omitempty"` - Offset int `json:"offset,omitempty"` + All []Expression `json:"all,omitempty"` + Any []Expression `json:"any,omitempty"` + Sort string `json:"sort,omitempty"` + Order string `json:"order,omitempty"` + Limit int `json:"limit,omitempty"` + LimitPercent int `json:"limitPercent,omitempty"` + Offset int `json:"offset,omitempty"` }{ - Sort: c.Sort, - Order: c.Order, - Limit: c.Limit, - Offset: c.Offset, + Sort: c.Sort, + Order: c.Order, + Limit: c.Limit, + LimitPercent: c.LimitPercent, + Offset: c.Offset, } switch rules := c.Expression.(type) { case Any: @@ -153,12 +193,13 @@ func (c Criteria) MarshalJSON() ([]byte, error) { func (c *Criteria) UnmarshalJSON(data []byte) error { var aux struct { - All unmarshalConjunctionType `json:"all"` - Any unmarshalConjunctionType `json:"any"` - Sort string `json:"sort"` - Order string `json:"order"` - Limit int `json:"limit"` - Offset int `json:"offset"` + All unmarshalConjunctionType `json:"all"` + Any unmarshalConjunctionType `json:"any"` + Sort string `json:"sort"` + Order string `json:"order"` + Limit int `json:"limit"` + LimitPercent int `json:"limitPercent"` + Offset int `json:"offset"` } if err := json.Unmarshal(data, &aux); err != nil { return err @@ -174,5 +215,15 @@ func (c *Criteria) UnmarshalJSON(data []byte) error { c.Order = aux.Order c.Limit = aux.Limit c.Offset = aux.Offset + + // Clamp LimitPercent to [0, 100] + if aux.LimitPercent < 0 { + log.Warn("limitPercent value out of range, clamping to 0", "value", aux.LimitPercent) + aux.LimitPercent = 0 + } else if aux.LimitPercent > 100 { + log.Warn("limitPercent value out of range, clamping to 100", "value", aux.LimitPercent) + aux.LimitPercent = 100 + } + c.LimitPercent = aux.LimitPercent return nil } diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go index 9a4da360..a76b3fc1 100644 --- a/model/criteria/criteria_test.go +++ b/model/criteria/criteria_test.go @@ -181,6 +181,28 @@ var _ = Describe("Criteria", func() { }) }) + Describe("ExpressionJoins", func() { + It("excludes sort-only joins", func() { + c := Criteria{ + Expression: All{ + Contains{"title": "love"}, + }, + Sort: "albumRating", + } + gomega.Expect(c.ExpressionJoins()).To(gomega.Equal(JoinNone)) + gomega.Expect(c.RequiredJoins().Has(JoinAlbumAnnotation)).To(gomega.BeTrue()) + }) + + It("includes expression-based joins", func() { + c := Criteria{ + Expression: All{ + Gt{"albumRating": 3}, + }, + } + gomega.Expect(c.ExpressionJoins().Has(JoinAlbumAnnotation)).To(gomega.BeTrue()) + }) + }) + Describe("RequiredJoins", func() { It("returns JoinNone when no annotation fields are used", func() { c := Criteria{ @@ -263,6 +285,126 @@ var _ = Describe("Criteria", func() { }) }) + Describe("LimitPercent", func() { + Describe("JSON round-trip", func() { + It("marshals and unmarshals limitPercent", func() { + goObj := Criteria{ + Expression: All{Contains{"title": "love"}}, + Sort: "title", + Order: "asc", + LimitPercent: 10, + } + j, err := json.Marshal(goObj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(string(j)).To(gomega.ContainSubstring(`"limitPercent":10`)) + gomega.Expect(string(j)).ToNot(gomega.ContainSubstring(`"limit"`)) + + var newObj Criteria + err = json.Unmarshal(j, &newObj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(newObj.LimitPercent).To(gomega.Equal(10)) + gomega.Expect(newObj.Limit).To(gomega.Equal(0)) + }) + + It("does not include limitPercent when zero", func() { + goObj := Criteria{ + Expression: All{Contains{"title": "love"}}, + Limit: 50, + } + j, err := json.Marshal(goObj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(string(j)).To(gomega.ContainSubstring(`"limit":50`)) + gomega.Expect(string(j)).ToNot(gomega.ContainSubstring(`limitPercent`)) + }) + + It("backward compatible: JSON with only limit still works", func() { + jsonStr := `{"all":[{"contains":{"title":"love"}}],"limit":20}` + var c Criteria + err := json.Unmarshal([]byte(jsonStr), &c) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(c.Limit).To(gomega.Equal(20)) + gomega.Expect(c.LimitPercent).To(gomega.Equal(0)) + }) + }) + + Describe("UnmarshalJSON clamping", func() { + It("clamps values above 100 to 100", func() { + jsonStr := `{"all":[{"contains":{"title":"love"}}],"limitPercent":150}` + var c Criteria + err := json.Unmarshal([]byte(jsonStr), &c) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(c.LimitPercent).To(gomega.Equal(100)) + }) + + It("clamps negative values to 0", func() { + jsonStr := `{"all":[{"contains":{"title":"love"}}],"limitPercent":-5}` + var c Criteria + err := json.Unmarshal([]byte(jsonStr), &c) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(c.LimitPercent).To(gomega.Equal(0)) + }) + }) + + Describe("EffectiveLimit", func() { + It("returns fixed limit when Limit is set", func() { + c := Criteria{Limit: 50, LimitPercent: 10} + gomega.Expect(c.EffectiveLimit(1000)).To(gomega.Equal(50)) + }) + + It("returns percentage-based limit", func() { + c := Criteria{LimitPercent: 10} + gomega.Expect(c.EffectiveLimit(450)).To(gomega.Equal(45)) + }) + + It("returns minimum 1 when totalCount > 0 and percentage rounds to 0", func() { + c := Criteria{LimitPercent: 1} + gomega.Expect(c.EffectiveLimit(5)).To(gomega.Equal(1)) + }) + + It("returns 0 when totalCount is 0", func() { + c := Criteria{LimitPercent: 10} + gomega.Expect(c.EffectiveLimit(0)).To(gomega.Equal(0)) + }) + + It("returns 0 when no limit is set", func() { + c := Criteria{} + gomega.Expect(c.EffectiveLimit(1000)).To(gomega.Equal(0)) + }) + + It("returns full count for 100%", func() { + c := Criteria{LimitPercent: 100} + gomega.Expect(c.EffectiveLimit(450)).To(gomega.Equal(450)) + }) + + It("returns 1 for 1% of 50 items", func() { + c := Criteria{LimitPercent: 1} + gomega.Expect(c.EffectiveLimit(50)).To(gomega.Equal(1)) + }) + }) + + Describe("IsPercentageLimit", func() { + It("returns true when LimitPercent is set and Limit is 0", func() { + c := Criteria{LimitPercent: 10} + gomega.Expect(c.IsPercentageLimit()).To(gomega.BeTrue()) + }) + + It("returns false when Limit is set", func() { + c := Criteria{Limit: 50, LimitPercent: 10} + gomega.Expect(c.IsPercentageLimit()).To(gomega.BeFalse()) + }) + + It("returns false when neither is set", func() { + c := Criteria{} + gomega.Expect(c.IsPercentageLimit()).To(gomega.BeFalse()) + }) + + It("returns false when LimitPercent is out of range", func() { + c := Criteria{LimitPercent: 150} + gomega.Expect(c.IsPercentageLimit()).To(gomega.BeFalse()) + }) + }) + }) + Context("with child playlists", func() { var ( topLevelInPlaylistID string diff --git a/model/criteria/fields.go b/model/criteria/fields.go index ed73de9f..b9d91f08 100644 --- a/model/criteria/fields.go +++ b/model/criteria/fields.go @@ -122,27 +122,24 @@ func mapExpr(expr squirrel.Sqlizer, negate bool, exprFunc func(string, squirrel. log.Fatal(fmt.Sprintf("expr is not a map-based operator: %T", expr)) } - // Extract into a generic map + // Extract the field name and value, then build a new map keyed by "value" + // for the inner condition. The original map is left untouched so that + // ToSql can be called multiple times without corruption. var k string - m := make(map[string]any, rv.Len()) + var v any for _, key := range rv.MapKeys() { - // Save the key to build the expression, and use the provided keyName as the key k = key.String() - m["value"] = rv.MapIndex(key).Interface() + v = rv.MapIndex(key).Interface() break // only one key is expected (and supported) } - // Clear the original map - for _, key := range rv.MapKeys() { - rv.SetMapIndex(key, reflect.Value{}) - } + // Create a new map-based expression with "value" as the key, matching the + // column name inside json_tree subqueries. + newMap := reflect.MakeMap(rv.Type()) + newMap.SetMapIndex(reflect.ValueOf("value"), reflect.ValueOf(v)) + newExpr := newMap.Interface().(squirrel.Sqlizer) - // Write the updated map back into the original variable - for key, val := range m { - rv.SetMapIndex(reflect.ValueOf(key), reflect.ValueOf(val)) - } - - return exprFunc(k, expr, negate) + return exprFunc(k, newExpr, negate) } // mapTagExpr maps a normal field expression to a tag expression. diff --git a/model/criteria/operators_test.go b/model/criteria/operators_test.go index f0681af6..5f756f97 100644 --- a/model/criteria/operators_test.go +++ b/model/criteria/operators_test.go @@ -178,6 +178,21 @@ var _ = Describe("Operators", func() { }) }) + DescribeTable("ToSql idempotency", + func(expr Expression) { + sql1, args1, err1 := expr.ToSql() + sql2, args2, err2 := expr.ToSql() + + gomega.Expect(err1).ToNot(gomega.HaveOccurred()) + gomega.Expect(err2).ToNot(gomega.HaveOccurred()) + gomega.Expect(sql2).To(gomega.Equal(sql1)) + gomega.Expect(args2).To(gomega.Equal(args1)) + }, + Entry("tag expression", Is{"genre": "Rock"}), + Entry("role expression", Contains{"artist": "Beatles"}), + Entry("nested criteria", Criteria{Expression: All{Is{"genre": "Rock"}, Contains{"artist": "Beatles"}}}), + ) + DescribeTable("JSON Marshaling", func(op Expression, jsonString string) { obj := And{op} diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 11b9cd8b..8d1bbe0f 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -248,22 +248,36 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { // Conditionally join album/artist annotation tables only when referenced by criteria or sort requiredJoins := rules.RequiredJoins() - if requiredJoins.Has(criteria.JoinAlbumAnnotation) { - 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 = ?)", usr.ID) - } - if requiredJoins.Has(criteria.JoinArtistAnnotation) { - 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 = ?)", usr.ID) - } + sq = r.addSmartPlaylistAnnotationJoins(sq, requiredJoins, usr.ID) // Only include media files from libraries the user has access to sq = r.applyLibraryFilter(sq, "media_file") + // Resolve percentage-based limit to an absolute number before applying criteria + if rules.IsPercentageLimit() { + // Use only expression-based joins for the COUNT query (sort joins are unnecessary) + exprJoins := rules.ExpressionJoins() + countSq := Select("count(*) as count").From("media_file"). + LeftJoin("annotation on ("+ + "annotation.item_id = media_file.id"+ + " AND annotation.item_type = 'media_file'"+ + " AND annotation.user_id = ?)", usr.ID) + countSq = r.addSmartPlaylistAnnotationJoins(countSq, exprJoins, usr.ID) + countSq = r.applyLibraryFilter(countSq, "media_file") + countSq = countSq.Where(rules) + + var res struct{ Count int64 } + err = r.queryOne(countSq, &res) + if err != nil { + log.Error(r.ctx, "Error counting matching tracks for percentage limit", "playlist", pls.Name, "id", pls.ID, err) + return false + } + resolvedLimit := rules.EffectiveLimit(res.Count) + log.Debug(r.ctx, "Resolved percentage limit", "playlist", pls.Name, "percent", rules.LimitPercent, "totalMatching", res.Count, "resolvedLimit", resolvedLimit) + rules.Limit = resolvedLimit + rules.LimitPercent = 0 + } + // Apply the criteria rules sq = r.addCriteria(sq, rules) insSql := Insert("playlist_tracks").Columns("id", "playlist_id", "media_file_id").Select(sq) @@ -296,6 +310,22 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { return true } +func (r *playlistRepository) addSmartPlaylistAnnotationJoins(sq SelectBuilder, joins criteria.JoinType, userID string) SelectBuilder { + if joins.Has(criteria.JoinAlbumAnnotation) { + 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(criteria.JoinArtistAnnotation) { + 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) + } + return sq +} + func (r *playlistRepository) addCriteria(sql SelectBuilder, c criteria.Criteria) SelectBuilder { sql = sql.Where(c) if c.Limit > 0 {