fix(smartplaylists): coerce string booleans in smart playlist rules (#5450)

* fix(criteria): coerce string booleans in smart playlist rules - #4826

When clients (e.g. Feishin) send boolean values as strings ("true"/"false")
in smart playlist JSON rules, the SQL comparison fails because SQLite stores
booleans as 0/1 integers. For example, `COALESCE(annotation.starred, false) = 'true'`
never matches.

This adds a `boolean` flag to mapped fields and coerces string values to
native Go bools in `mapFields`, so squirrel generates correct SQL parameters.

Signed-off-by: mango766 <mango766@users.noreply.github.com>
Signed-off-by: easonysliu <easonysliu@tencent.com>

* fix(criteria): implement boolean string coercion for smart playlist rules

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

---------

Signed-off-by: mango766 <mango766@users.noreply.github.com>
Signed-off-by: easonysliu <easonysliu@tencent.com>
Signed-off-by: Deluan <deluan@navidrome.org>
Co-authored-by: easonysliu <easonysliu@tencent.com>

model/criteria/fields.go

@@ -8,6 +8,7 @@ type FieldInfo struct {
 	IsTag   bool
 	IsRole  bool
 	Numeric bool
+	Boolean bool
 
 	tagAlias string // If set, a tag name from mappings.yml that resolves to this field
 	name     string // Canonical name, populated by LookupField from the map key
@@ -21,7 +22,7 @@ func (f FieldInfo) Name() string {
 var fieldMap = map[string]FieldInfo{
 	"title":                {},
 	"album":                {},
-	"hascoverart":          {},
+	"hascoverart":          {Boolean: true},
 	"tracknumber":          {},
 	"discnumber":           {},
 	"year":                 {},
@@ -31,8 +32,8 @@ var fieldMap = map[string]FieldInfo{
 	"releaseyear":          {},
 	"releasedate":          {},
 	"size":                 {},
-	"compilation":          {},
-	"missing":              {},
+	"compilation":          {Boolean: true},
+	"missing":              {Boolean: true},
 	"explicitstatus":       {},
 	"dateadded":            {},
 	"datemodified":         {},
@@ -54,7 +55,7 @@ var fieldMap = map[string]FieldInfo{
 	"samplerate":           {},
 	"bpm":                  {},
 	"channels":             {},
-	"loved":                {},
+	"loved":                {Boolean: true},
 	"dateloved":            {},
 	"lastplayed":           {},
 	"daterated":            {},
@@ -62,13 +63,13 @@ var fieldMap = map[string]FieldInfo{
 	"rating":               {},
 	"averagerating":        {Numeric: true},
 	"albumrating":          {},
-	"albumloved":           {},
+	"albumloved":           {Boolean: true},
 	"albumplaycount":       {},
 	"albumlastplayed":      {},
 	"albumdateloved":       {},
 	"albumdaterated":       {},
 	"artistrating":         {},
-	"artistloved":          {},
+	"artistloved":          {Boolean: true},
 	"artistplaycount":      {},
 	"artistlastplayed":     {},
 	"artistdateloved":      {},

model/criteria/fields_test.go

@@ -52,5 +52,6 @@ var _ = Describe("fields", func() {
 			gomega.Expect(field.Name()).To(gomega.Equal("task3_producer"))
 			gomega.Expect(field.IsRole).To(gomega.BeTrue())
 		})
+
 	})
 })

model/criteria/json.go

@@ -3,6 +3,7 @@ package criteria
 import (
 	"encoding/json"
 	"fmt"
+	"strconv"
 	"strings"
 )
 
@@ -38,6 +39,7 @@ func unmarshalExpression(opName string, rawValue json.RawMessage) Expression {
 	if err != nil {
 		return nil
 	}
+	normalizeBoolFields(m)
 	switch opName {
 	case "is":
 		return Is(m)
@@ -70,13 +72,47 @@ func unmarshalExpression(opName string, rawValue json.RawMessage) Expression {
 	case "notinplaylist":
 		return NotInPlaylist(m)
 	case "ismissing":
+		normalizeAllBoolFields(m)
 		return IsMissing(m)
 	case "ispresent":
+		normalizeAllBoolFields(m)
 		return IsPresent(m)
 	}
 	return nil
 }
 
+func normalizeAllBoolFields(m map[string]any) {
+	for k, v := range m {
+		m[k] = normalizeBoolValue(v)
+	}
+}
+
+func normalizeBoolFields(m map[string]any) {
+	for field, value := range m {
+		info, ok := LookupField(field)
+		if ok && info.Boolean {
+			m[field] = normalizeBoolValue(value)
+		}
+	}
+}
+
+func normalizeBoolValue(v any) any {
+	switch val := v.(type) {
+	case string:
+		if b, err := strconv.ParseBool(val); err == nil {
+			return b
+		}
+	case float64:
+		if val == 1 {
+			return true
+		}
+		if val == 0 {
+			return false
+		}
+	}
+	return v
+}
+
 func unmarshalConjunction(conjName string, rawValue json.RawMessage) Expression {
 	var items unmarshalConjunctionType
 	err := json.Unmarshal(rawValue, &items)

model/criteria/operators.go

@@ -1,9 +1,6 @@
 package criteria
 
-import (
-	"strconv"
-	"time"
-)
+import "time"
 
 // Conjunctions need to implement this interface, to allow Criteria to extract child playlist IDs recursively
 type conjunction interface {
@@ -181,20 +178,6 @@ func (ip IsPresent) MarshalJSON() ([]byte, error) {
 
 func (ip IsPresent) fields() map[string]any { return ip }
 
-func IsTruthy(v any) bool {
-	switch val := v.(type) {
-	case bool:
-		return val
-	case float64:
-		return val != 0
-	case string:
-		b, err := strconv.ParseBool(val)
-		return err == nil && b
-	default:
-		return v != nil
-	}
-}
-
 func extractPlaylistIds(inputRule any) (ids []string) {
 	var id string
 	var ok bool

model/criteria/operators_test.go

@@ -31,6 +31,7 @@ var _ = Describe("Operators", func() {
 		},
 		Entry("is [string]", Is{"title": "Low Rider"}, `{"is":{"title":"Low Rider"}}`),
 		Entry("is [bool]", Is{"loved": false}, `{"is":{"loved":false}}`),
+		Entry("is [string does not coerce non-boolean field]", Is{"title": "true"}, `{"is":{"title":"true"}}`),
 		Entry("isNot", IsNot{"title": "Low Rider"}, `{"isNot":{"title":"Low Rider"}}`),
 		Entry("gt", Gt{"playCount": 10.0}, `{"gt":{"playCount":10}}`),
 		Entry("lt", Lt{"playCount": 10.0}, `{"lt":{"playCount":10}}`),
@@ -51,4 +52,78 @@ var _ = Describe("Operators", func() {
 		Entry("isPresent [true]", IsPresent{"genre": true}, `{"isPresent":{"genre":true}}`),
 		Entry("isPresent [false]", IsPresent{"genre": false}, `{"isPresent":{"genre":false}}`),
 	)
+
+	Describe("Boolean string coercion at unmarshal time (issue #4826)", func() {
+		It("coerces string 'true' to bool for boolean fields", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"is":{"loved":"true"}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": true}))
+		})
+
+		It("coerces string 'false' to bool for boolean fields", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"is":{"loved":"false"}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": false}))
+		})
+
+		It("does not coerce string values for non-boolean fields", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"is":{"title":"true"}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(Is{"title": "true"}))
+		})
+
+		It("coerces numeric 1 to bool true for boolean fields", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"is":{"loved":1}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": true}))
+		})
+
+		It("coerces numeric 0 to bool false for boolean fields", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"is":{"loved":0}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": false}))
+		})
+
+		It("coerces in nested any/all groups", func() {
+			var c Criteria
+			err := json.Unmarshal([]byte(`{"all":[{"contains":{"title":"love"}},{"any":[{"is":{"loved":"true"}}]}]}`), &c)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			all := c.Expression.(All)
+			nested := all[1].(Any)
+			gomega.Expect(nested[0]).To(gomega.Equal(Is{"loved": true}))
+		})
+
+		It("coerces isMissing string 'true' to bool", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"isMissing":{"genre":"true"}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(IsMissing{"genre": true}))
+		})
+
+		It("coerces isMissing numeric 0 to bool false", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"isMissing":{"genre":0}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(IsMissing{"genre": false}))
+		})
+
+		It("coerces isPresent string 'false' to bool", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"isPresent":{"genre":"false"}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(IsPresent{"genre": false}))
+		})
+
+		It("coerces isPresent numeric 1 to bool true", func() {
+			var obj UnmarshalConjunctionType
+			err := json.Unmarshal([]byte(`[{"isPresent":{"genre":1}}]`), &obj)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(obj[0]).To(gomega.Equal(IsPresent{"genre": true}))
+		})
+	})
 })

persistence/criteria_sql.go

@@ -220,7 +220,11 @@ func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, er
 		return nil, fmt.Errorf("isMissing/isPresent operator is only supported for tag and role fields, got: %s", field)
 	}
 
-	negate := checkAbsence == criteria.IsTruthy(value)
+	b, ok := value.(bool)
+	if !ok {
+		return nil, fmt.Errorf("invalid boolean value for 'missing' expression: %s: %v", field, value)
+	}
+	negate := checkAbsence == b
 	return jsonExpr(info, nil, negate), nil
 }
 

persistence/criteria_sql_test.go

@@ -149,6 +149,11 @@ var _ = Describe("Smart playlist criteria SQL", func() {
 		Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is only supported for tag and role fields")))
 	})
 
+	It("returns an error when isMissing has a non-boolean value", func() {
+		_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"genre": "hello"}}).Where()
+		Expect(err).To(MatchError(ContainSubstring("invalid boolean value for 'missing' expression")))
+	})
+
 	Describe("sort", func() {
 		It("sorts by regular fields", func() {
 			Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title"}).OrderBy()).To(Equal("media_file.title asc"))

persistence/smart_playlist_repository_test.go

@@ -281,6 +281,71 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() {
 
 			Expect(pls.Tracks).To(BeEmpty())
 		})
+
+		It("matches loved tracks when loved value is a string in nested group (issue #4826)", func() {
+			// songComeTogether (ID "1002") is starred in test fixtures
+			rules := &criteria.Criteria{
+				Expression: criteria.All{
+					criteria.Any{
+						criteria.Is{"loved": "true"},
+					},
+				},
+			}
+			newPls := model.Playlist{Name: "String Loved Nested", OwnerID: "userid", Rules: rules}
+			Expect(repo.Put(&newPls)).To(Succeed())
+			testPlaylistID = newPls.ID
+
+			conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second
+			pls, err := repo.GetWithTracks(newPls.ID, true, false)
+			Expect(err).ToNot(HaveOccurred())
+
+			trackIDs := make([]string, len(pls.Tracks))
+			for i, t := range pls.Tracks {
+				trackIDs[i] = t.MediaFileID
+			}
+			Expect(trackIDs).To(ContainElement("1002"))
+			Expect(len(pls.Tracks)).To(BeNumerically(">=", 1))
+		})
+
+		It("returns same results for string and bool loved values (issue #4826)", func() {
+			boolRules := &criteria.Criteria{
+				Expression: criteria.All{
+					criteria.Any{
+						criteria.Is{"loved": true},
+					},
+				},
+			}
+			boolPls := model.Playlist{Name: "Bool Loved", OwnerID: "userid", Rules: boolRules}
+			Expect(repo.Put(&boolPls)).To(Succeed())
+			DeferCleanup(func() { _ = repo.Delete(boolPls.ID) })
+
+			stringRules := &criteria.Criteria{
+				Expression: criteria.All{
+					criteria.Any{
+						criteria.Is{"loved": "true"},
+					},
+				},
+			}
+			stringPls := model.Playlist{Name: "String Loved", OwnerID: "userid", Rules: stringRules}
+			Expect(repo.Put(&stringPls)).To(Succeed())
+			testPlaylistID = stringPls.ID
+
+			conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second
+			boolResult, err := repo.GetWithTracks(boolPls.ID, true, false)
+			Expect(err).ToNot(HaveOccurred())
+			stringResult, err := repo.GetWithTracks(stringPls.ID, true, false)
+			Expect(err).ToNot(HaveOccurred())
+
+			boolIDs := make([]string, len(boolResult.Tracks))
+			for i, t := range boolResult.Tracks {
+				boolIDs[i] = t.MediaFileID
+			}
+			stringIDs := make([]string, len(stringResult.Tracks))
+			for i, t := range stringResult.Tracks {
+				stringIDs[i] = t.MediaFileID
+			}
+			Expect(stringIDs).To(ConsistOf(boolIDs))
+		})
 	})
 
 	Describe("Smart Playlists with Tag Criteria", func() {

server/e2e/subsonic_playlists_test.go

@@ -517,4 +517,134 @@ var _ = Describe("Playlist Endpoints", Ordered, func() {
 			Expect(resp.Status).To(Equal(responses.StatusFailed))
 		})
 	})
+
+	Describe("Smart Playlist Boolean String Normalization (issue #4826)", Ordered, func() {
+		var songID string
+		var boolPlaylistID, stringPlaylistID, nestedPlaylistID string
+
+		BeforeAll(func() {
+			setupTestDB()
+
+			songs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{Sort: "title", Max: 1})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(songs).ToNot(BeEmpty())
+			songID = songs[0].ID
+
+			// Star the song via the Subsonic API
+			resp := doReq("star", "id", songID)
+			Expect(resp.Status).To(Equal(responses.StatusOK))
+
+			// Force immediate refresh for all smart playlists
+			conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second
+
+			// Create smart playlist with boolean true
+			boolPls := &model.Playlist{
+				Name:    "Bool Loved",
+				OwnerID: adminUser.ID,
+				Rules:   &criteria.Criteria{Expression: criteria.All{criteria.Is{"loved": true}}},
+			}
+			Expect(ds.Playlist(ctx).Put(boolPls)).To(Succeed())
+			boolPlaylistID = boolPls.ID
+
+			// Create smart playlist with string "true"
+			stringPls := &model.Playlist{
+				Name:    "String Loved",
+				OwnerID: adminUser.ID,
+				Rules:   &criteria.Criteria{Expression: criteria.All{criteria.Is{"loved": "true"}}},
+			}
+			Expect(ds.Playlist(ctx).Put(stringPls)).To(Succeed())
+			stringPlaylistID = stringPls.ID
+
+			// Create smart playlist with string "true" in nested any group (exact issue #4826 scenario)
+			nestedPls := &model.Playlist{
+				Name:    "Nested String Loved",
+				OwnerID: adminUser.ID,
+				Rules: &criteria.Criteria{Expression: criteria.All{
+					criteria.Any{
+						criteria.Is{"loved": "true"},
+					},
+				}},
+			}
+			Expect(ds.Playlist(ctx).Put(nestedPls)).To(Succeed())
+			nestedPlaylistID = nestedPls.ID
+		})
+
+		It("smart playlist with bool loved=true returns starred song", func() {
+			resp := doReq("getPlaylist", "id", boolPlaylistID)
+
+			Expect(resp.Status).To(Equal(responses.StatusOK))
+			Expect(resp.Playlist.SongCount).To(BeNumerically(">=", int32(1)))
+			entryIDs := make([]string, len(resp.Playlist.Entry))
+			for i, e := range resp.Playlist.Entry {
+				entryIDs[i] = e.Id
+			}
+			Expect(entryIDs).To(ContainElement(songID))
+		})
+
+		It("smart playlist with string loved='true' returns same results as bool (issue #4826)", func() {
+			boolResp := doReq("getPlaylist", "id", boolPlaylistID)
+			stringResp := doReq("getPlaylist", "id", stringPlaylistID)
+
+			Expect(stringResp.Status).To(Equal(responses.StatusOK))
+			Expect(stringResp.Playlist.SongCount).To(Equal(boolResp.Playlist.SongCount))
+		})
+
+		It("nested any group with string loved='true' returns starred song (issue #4826)", func() {
+			resp := doReq("getPlaylist", "id", nestedPlaylistID)
+
+			Expect(resp.Status).To(Equal(responses.StatusOK))
+			Expect(resp.Playlist.SongCount).To(BeNumerically(">=", int32(1)))
+			entryIDs := make([]string, len(resp.Playlist.Entry))
+			for i, e := range resp.Playlist.Entry {
+				entryIDs[i] = e.Id
+			}
+			Expect(entryIDs).To(ContainElement(songID))
+		})
+
+		It("isPresent with string 'true' matches songs that have the tag", func() {
+			pls := &model.Playlist{
+				Name:    "Genre Present String",
+				OwnerID: adminUser.ID,
+				Rules:   &criteria.Criteria{Expression: criteria.All{criteria.IsPresent{"genre": "true"}}},
+			}
+			Expect(ds.Playlist(ctx).Put(pls)).To(Succeed())
+
+			resp := doReq("getPlaylist", "id", pls.ID)
+			Expect(resp.Status).To(Equal(responses.StatusOK))
+			Expect(resp.Playlist.SongCount).To(BeNumerically(">=", int32(1)))
+		})
+
+		It("isMissing with string 'true' excludes songs that have the tag", func() {
+			pls := &model.Playlist{
+				Name:    "Genre Missing String",
+				OwnerID: adminUser.ID,
+				Rules:   &criteria.Criteria{Expression: criteria.All{criteria.IsMissing{"genre": "true"}}},
+			}
+			Expect(ds.Playlist(ctx).Put(pls)).To(Succeed())
+
+			resp := doReq("getPlaylist", "id", pls.ID)
+			Expect(resp.Status).To(Equal(responses.StatusOK))
+			Expect(resp.Playlist.SongCount).To(Equal(int32(0)))
+		})
+
+		It("isMissing with string 'true' returns same results as bool true", func() {
+			boolPls := &model.Playlist{
+				Name:    "Genre Missing Bool",
+				OwnerID: adminUser.ID,
+				Rules:   &criteria.Criteria{Expression: criteria.All{criteria.IsMissing{"genre": true}}},
+			}
+			Expect(ds.Playlist(ctx).Put(boolPls)).To(Succeed())
+
+			stringPls := &model.Playlist{
+				Name:    "Genre Missing String2",
+				OwnerID: adminUser.ID,
+				Rules:   &criteria.Criteria{Expression: criteria.All{criteria.IsMissing{"genre": "true"}}},
+			}
+			Expect(ds.Playlist(ctx).Put(stringPls)).To(Succeed())
+
+			boolResp := doReq("getPlaylist", "id", boolPls.ID)
+			stringResp := doReq("getPlaylist", "id", stringPls.ID)
+			Expect(stringResp.Playlist.SongCount).To(Equal(boolResp.Playlist.SongCount))
+		})
+	})
 })