fix(server): fix numeric comparisons for float custom tags in smart playlists (#4116)
* Fix numeric comparisons for custom float tags * feat(criteria): cast numeric tags for sorting and comparisons Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
@@ -43,7 +43,7 @@ This is a music streaming server written in Go with a React frontend. The applic
|
|||||||
- Validate both backend and frontend interactions
|
- Validate both backend and frontend interactions
|
||||||
- Consider how changes will affect user experience and performance
|
- Consider how changes will affect user experience and performance
|
||||||
- Test with different music library sizes and configurations
|
- Test with different music library sizes and configurations
|
||||||
- Always run formatting and linting before committing changes
|
- Before committing, ALWAYS run `make format lint test`, and make sure there are no issues
|
||||||
|
|
||||||
## Important commands
|
## Important commands
|
||||||
- `make build`: Build the application
|
- `make build`: Build the application
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ package criteria
|
|||||||
import (
|
import (
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"errors"
|
"errors"
|
||||||
|
"fmt"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
"github.com/Masterminds/squirrel"
|
"github.com/Masterminds/squirrel"
|
||||||
@@ -40,6 +41,9 @@ func (c Criteria) OrderBy() string {
|
|||||||
} else {
|
} else {
|
||||||
mapped = f.field
|
mapped = f.field
|
||||||
}
|
}
|
||||||
|
if f.numeric {
|
||||||
|
mapped = fmt.Sprintf("CAST(%s AS REAL)", mapped)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
if c.Order != "" {
|
if c.Order != "" {
|
||||||
if strings.EqualFold(c.Order, "asc") || strings.EqualFold(c.Order, "desc") {
|
if strings.EqualFold(c.Order, "asc") || strings.EqualFold(c.Order, "desc") {
|
||||||
|
|||||||
@@ -109,6 +109,15 @@ var _ = Describe("Criteria", func() {
|
|||||||
)
|
)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
It("casts numeric tags when sorting", func() {
|
||||||
|
AddTagNames([]string{"rate"})
|
||||||
|
AddNumericTags([]string{"rate"})
|
||||||
|
goObj.Sort = "rate"
|
||||||
|
gomega.Expect(goObj.OrderBy()).To(
|
||||||
|
gomega.Equal("CAST(COALESCE(json_extract(media_file.tags, '$.rate[0].value'), '') AS REAL) asc"),
|
||||||
|
)
|
||||||
|
})
|
||||||
|
|
||||||
It("sorts by random", func() {
|
It("sorts by random", func() {
|
||||||
newObj := goObj
|
newObj := goObj
|
||||||
newObj.Sort = "random"
|
newObj.Sort = "random"
|
||||||
|
|||||||
@@ -54,11 +54,12 @@ var fieldMap = map[string]*mappedField{
|
|||||||
}
|
}
|
||||||
|
|
||||||
type mappedField struct {
|
type mappedField struct {
|
||||||
field string
|
field string
|
||||||
order string
|
order string
|
||||||
isRole bool // true if the field is a role (e.g. "artist", "composer", "conductor", etc.)
|
isRole bool // true if the field is a role (e.g. "artist", "composer", "conductor", etc.)
|
||||||
isTag bool // true if the field is a tag imported from the file metadata
|
isTag bool // true if the field is a tag imported from the file metadata
|
||||||
alias string // name from `mappings.yml` that may differ from the name used in the smart playlist
|
alias string // name from `mappings.yml` that may differ from the name used in the smart playlist
|
||||||
|
numeric bool // true if the field/tag should be treated as numeric
|
||||||
}
|
}
|
||||||
|
|
||||||
func mapFields(expr map[string]any) map[string]any {
|
func mapFields(expr map[string]any) map[string]any {
|
||||||
@@ -145,6 +146,12 @@ type tagCond struct {
|
|||||||
|
|
||||||
func (e tagCond) ToSql() (string, []any, error) {
|
func (e tagCond) ToSql() (string, []any, error) {
|
||||||
cond, args, err := e.cond.ToSql()
|
cond, args, err := e.cond.ToSql()
|
||||||
|
|
||||||
|
// Check if this tag is marked as numeric in the fieldMap
|
||||||
|
if fm, ok := fieldMap[e.tag]; ok && fm.numeric {
|
||||||
|
cond = strings.ReplaceAll(cond, "value", "CAST(value AS REAL)")
|
||||||
|
}
|
||||||
|
|
||||||
cond = fmt.Sprintf("exists (select 1 from json_tree(tags, '$.%s') where key='value' and %s)",
|
cond = fmt.Sprintf("exists (select 1 from json_tree(tags, '$.%s') where key='value' and %s)",
|
||||||
e.tag, cond)
|
e.tag, cond)
|
||||||
if e.not {
|
if e.not {
|
||||||
@@ -205,3 +212,16 @@ func AddTagNames(tagNames []string) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// AddNumericTags marks the given tag names as numeric so they can be cast
|
||||||
|
// when used in comparisons or sorting.
|
||||||
|
func AddNumericTags(tagNames []string) {
|
||||||
|
for _, name := range tagNames {
|
||||||
|
name := strings.ToLower(name)
|
||||||
|
if fm, ok := fieldMap[name]; ok {
|
||||||
|
fm.numeric = true
|
||||||
|
} else {
|
||||||
|
fieldMap[name] = &mappedField{field: name, isTag: true, numeric: true}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ import (
|
|||||||
var _ = BeforeSuite(func() {
|
var _ = BeforeSuite(func() {
|
||||||
AddRoles([]string{"artist", "composer"})
|
AddRoles([]string{"artist", "composer"})
|
||||||
AddTagNames([]string{"genre"})
|
AddTagNames([]string{"genre"})
|
||||||
|
AddNumericTags([]string{"rate"})
|
||||||
})
|
})
|
||||||
|
|
||||||
var _ = Describe("Operators", func() {
|
var _ = Describe("Operators", func() {
|
||||||
@@ -68,6 +69,15 @@ var _ = Describe("Operators", func() {
|
|||||||
Entry("role endsWith [string]", EndsWith{"composer": "Lennon"}, "exists (select 1 from json_tree(participants, '$.composer') where key='name' and value LIKE ?)", "%Lennon"),
|
Entry("role endsWith [string]", EndsWith{"composer": "Lennon"}, "exists (select 1 from json_tree(participants, '$.composer') where key='name' and value LIKE ?)", "%Lennon"),
|
||||||
)
|
)
|
||||||
|
|
||||||
|
// TODO Validate operators that are not valid for each field type.
|
||||||
|
XDescribeTable("ToSQL - Invalid Operators",
|
||||||
|
func(op Expression, expectedError string) {
|
||||||
|
_, _, err := op.ToSql()
|
||||||
|
gomega.Expect(err).To(gomega.MatchError(expectedError))
|
||||||
|
},
|
||||||
|
Entry("numeric tag contains", Contains{"rate": 5}, "numeric tag 'rate' cannot be used with Contains operator"),
|
||||||
|
)
|
||||||
|
|
||||||
Describe("Custom Tags", func() {
|
Describe("Custom Tags", func() {
|
||||||
It("generates valid SQL", func() {
|
It("generates valid SQL", func() {
|
||||||
AddTagNames([]string{"mood"})
|
AddTagNames([]string{"mood"})
|
||||||
@@ -77,6 +87,14 @@ var _ = Describe("Operators", func() {
|
|||||||
gomega.Expect(sql).To(gomega.Equal("exists (select 1 from json_tree(tags, '$.mood') where key='value' and value LIKE ?)"))
|
gomega.Expect(sql).To(gomega.Equal("exists (select 1 from json_tree(tags, '$.mood') where key='value' and value LIKE ?)"))
|
||||||
gomega.Expect(args).To(gomega.HaveExactElements("%Soft"))
|
gomega.Expect(args).To(gomega.HaveExactElements("%Soft"))
|
||||||
})
|
})
|
||||||
|
It("casts numeric comparisons", func() {
|
||||||
|
AddNumericTags([]string{"rate"})
|
||||||
|
op := Lt{"rate": 6}
|
||||||
|
sql, args, err := op.ToSql()
|
||||||
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
||||||
|
gomega.Expect(sql).To(gomega.Equal("exists (select 1 from json_tree(tags, '$.rate') where key='value' and CAST(value AS REAL) < ?)"))
|
||||||
|
gomega.Expect(args).To(gomega.HaveExactElements(6))
|
||||||
|
})
|
||||||
It("skips unknown tag names", func() {
|
It("skips unknown tag names", func() {
|
||||||
op := EndsWith{"unknown": "value"}
|
op := EndsWith{"unknown": "value"}
|
||||||
sql, args, _ := op.ToSql()
|
sql, args, _ := op.ToSql()
|
||||||
|
|||||||
@@ -162,6 +162,17 @@ func tagNames() []string {
|
|||||||
return names
|
return names
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func numericTagNames() []string {
|
||||||
|
mappings := TagMappings()
|
||||||
|
names := make([]string, 0)
|
||||||
|
for k, cfg := range mappings {
|
||||||
|
if cfg.Type == TagTypeInteger || cfg.Type == TagTypeFloat {
|
||||||
|
names = append(names, string(k))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return names
|
||||||
|
}
|
||||||
|
|
||||||
func loadTagMappings() {
|
func loadTagMappings() {
|
||||||
mappingsFile, err := resources.FS().Open("mappings.yaml")
|
mappingsFile, err := resources.FS().Open("mappings.yaml")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -228,5 +239,6 @@ func init() {
|
|||||||
// used in smart playlists
|
// used in smart playlists
|
||||||
criteria.AddRoles(slices.Collect(maps.Keys(AllRoles)))
|
criteria.AddRoles(slices.Collect(maps.Keys(AllRoles)))
|
||||||
criteria.AddTagNames(tagNames())
|
criteria.AddTagNames(tagNames())
|
||||||
|
criteria.AddNumericTags(numericTagNames())
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user