mirror of
https://github.com/navidrome/navidrome.git
synced 2026-06-19 07:37:15 +00:00
fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields (#5585)
* fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields ReplayGain values are stored in dedicated nullable columns (rg_album_gain, rg_album_peak, rg_track_gain, rg_track_peak) rather than in the media_file.tags JSON blob. The isMissing/isPresent operators previously only supported tag and role fields, causing two failure modes: 1. Using the documented alias names (replaygain_album_gain etc.) from PR #5256: these got registered as JSON tags from mappings.yaml, so isMissing queried json_tree(media_file.tags, '$.replaygain_album_gain') which is always empty -> the playlist matched ALL songs. 2. Using the canonical field names (rgalbumgain etc.): not a tag/role, so SQL generation returned an error. Because refreshSmartPlaylist deletes old tracks before regenerating, the abort left the playlist empty. Fix: add Nullable bool to FieldInfo and mark the four ReplayGain fields. Add static alias entries (replaygain_album_gain -> rgalbumgain etc.) with Numeric+Nullable set; because AddTagNames skips names already in the field map, these static entries take precedence over the mappings.yaml tag registration. missingExpr now emits IS NULL / IS NOT NULL for nullable column fields instead of the json_tree lookup. Fixes #5584 * chore(smartplaylist): address code review feedback - Simplify the isMissing/isPresent unsupported-field error message, removing the internal "nullable fields" jargon - Standardize comments on mappings.yaml (the actual filename) - Clarify the alias precedence comment in the LookupField test
This commit is contained in:
+20
-11
@@ -4,13 +4,14 @@ import "strings"
|
||||
|
||||
// FieldInfo contains semantic metadata about a criteria field.
|
||||
type FieldInfo struct {
|
||||
Alias string // If set, this field is a backward-compat alias for another canonical name
|
||||
IsTag bool
|
||||
IsRole bool
|
||||
Numeric bool
|
||||
Boolean bool
|
||||
Alias string // If set, this field is a backward-compat alias for another canonical name
|
||||
IsTag bool
|
||||
IsRole bool
|
||||
Numeric bool
|
||||
Boolean bool
|
||||
Nullable bool // If set, this column field can be NULL, so isMissing/isPresent are supported on it
|
||||
|
||||
tagAlias string // If set, a tag name from mappings.yml that resolves to this field
|
||||
tagAlias string // If set, a tag name from mappings.yaml that resolves to this field
|
||||
name string // Canonical name, populated by LookupField from the map key
|
||||
}
|
||||
|
||||
@@ -80,15 +81,23 @@ var fieldMap = map[string]FieldInfo{
|
||||
"mbz_recording_id": {},
|
||||
"mbz_release_track_id": {},
|
||||
"mbz_release_group_id": {},
|
||||
"rgalbumgain": {Numeric: true},
|
||||
"rgalbumpeak": {Numeric: true},
|
||||
"rgtrackgain": {Numeric: true},
|
||||
"rgtrackpeak": {Numeric: true},
|
||||
"rgalbumgain": {Numeric: true, Nullable: true},
|
||||
"rgalbumpeak": {Numeric: true, Nullable: true},
|
||||
"rgtrackgain": {Numeric: true, Nullable: true},
|
||||
"rgtrackpeak": {Numeric: true, Nullable: true},
|
||||
"library_id": {Numeric: true},
|
||||
|
||||
// Backward compatibility: albumtype is an alias for the releasetype tag.
|
||||
"albumtype": {Alias: "releasetype", IsTag: true},
|
||||
|
||||
// Backward compatibility: the replaygain_* tag names (as written in metadata and in the
|
||||
// PR #5256 example) are aliases for the canonical rg* column fields. Without these, the tag
|
||||
// names would be registered as empty tags from mappings.yaml and isMissing would always match.
|
||||
"replaygain_album_gain": {Alias: "rgalbumgain", Numeric: true, Nullable: true},
|
||||
"replaygain_album_peak": {Alias: "rgalbumpeak", Numeric: true, Nullable: true},
|
||||
"replaygain_track_gain": {Alias: "rgtrackgain", Numeric: true, Nullable: true},
|
||||
"replaygain_track_peak": {Alias: "rgtrackpeak", Numeric: true, Nullable: true},
|
||||
|
||||
// Pseudo-field for random sorting
|
||||
"random": {},
|
||||
}
|
||||
@@ -128,7 +137,7 @@ func AddRoles(roles []string) {
|
||||
}
|
||||
}
|
||||
|
||||
// AddTagNames adds tag names to the field map. This is used to add all tags mapped in the `mappings.yml`
|
||||
// AddTagNames adds tag names to the field map. This is used to add all tags mapped in the `mappings.yaml`
|
||||
// configuration file.
|
||||
func AddTagNames(tagNames []string) {
|
||||
for _, tagName := range tagNames {
|
||||
|
||||
@@ -53,5 +53,27 @@ var _ = Describe("fields", func() {
|
||||
gomega.Expect(field.IsRole).To(gomega.BeTrue())
|
||||
})
|
||||
|
||||
It("marks ReplayGain column fields as nullable", func() {
|
||||
field, ok := LookupField("rgAlbumGain")
|
||||
|
||||
gomega.Expect(ok).To(gomega.BeTrue())
|
||||
gomega.Expect(field.Name()).To(gomega.Equal("rgalbumgain"))
|
||||
gomega.Expect(field.Nullable).To(gomega.BeTrue())
|
||||
gomega.Expect(field.IsTag).To(gomega.BeFalse())
|
||||
})
|
||||
|
||||
It("resolves replaygain_* tag names as aliases to nullable column fields", func() {
|
||||
// AddTagNames skips names already in the field map, so the startup tag registration
|
||||
// (from mappings.yaml) must not convert the pre-registered alias into a tag field.
|
||||
AddTagNames([]string{"replaygain_album_gain"})
|
||||
|
||||
field, ok := LookupField("replaygain_album_gain")
|
||||
|
||||
gomega.Expect(ok).To(gomega.BeTrue())
|
||||
gomega.Expect(field.Name()).To(gomega.Equal("rgalbumgain"))
|
||||
gomega.Expect(field.Nullable).To(gomega.BeTrue())
|
||||
gomega.Expect(field.IsTag).To(gomega.BeFalse())
|
||||
})
|
||||
|
||||
})
|
||||
})
|
||||
|
||||
@@ -218,8 +218,8 @@ func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, er
|
||||
}
|
||||
return nil, fmt.Errorf("invalid field in criteria: %s", field)
|
||||
}
|
||||
if !info.IsTag && !info.IsRole {
|
||||
return nil, fmt.Errorf("isMissing/isPresent operator is only supported for tag and role fields, got: %s", field)
|
||||
if !info.IsTag && !info.IsRole && !info.Nullable {
|
||||
return nil, fmt.Errorf("isMissing/isPresent operator is not supported for field: %s", field)
|
||||
}
|
||||
|
||||
b, ok := value.(bool)
|
||||
@@ -227,6 +227,20 @@ func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, er
|
||||
return nil, fmt.Errorf("invalid boolean value for 'missing' expression: %s: %v", field, value)
|
||||
}
|
||||
negate := checkAbsence == b
|
||||
|
||||
// Nullable column fields (e.g. ReplayGain) are stored in dedicated columns, not in the tags
|
||||
// JSON, so "missing" maps to a NULL check on the column rather than a json_tree lookup.
|
||||
if info.Nullable && !info.IsTag && !info.IsRole {
|
||||
col, ok := fieldExpr(info.Name())
|
||||
if !ok || col == "" {
|
||||
return nil, fmt.Errorf("invalid field in criteria: %s", field)
|
||||
}
|
||||
if negate {
|
||||
return squirrel.Eq{col: nil}, nil
|
||||
}
|
||||
return squirrel.NotEq{col: nil}, nil
|
||||
}
|
||||
|
||||
return jsonExpr(info, nil, negate), nil
|
||||
}
|
||||
|
||||
|
||||
@@ -14,7 +14,7 @@ import (
|
||||
var _ = Describe("Smart playlist criteria SQL", func() {
|
||||
BeforeEach(func() {
|
||||
criteria.AddRoles([]string{"artist", "composer", "producer"})
|
||||
criteria.AddTagNames([]string{"genre", "mood", "releasetype", "recordingdate"})
|
||||
criteria.AddTagNames([]string{"genre", "mood", "releasetype", "recordingdate", "replaygain_album_gain"})
|
||||
criteria.AddNumericTags([]string{"rate"})
|
||||
})
|
||||
|
||||
@@ -85,6 +85,20 @@ var _ = Describe("Smart playlist criteria SQL", func() {
|
||||
"exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "composer"),
|
||||
Entry("isPresent role [false]", criteria.IsPresent{"composer": false},
|
||||
"not exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "composer"),
|
||||
// isMissing/isPresent — nullable column fields (ReplayGain)
|
||||
Entry("isMissing rgAlbumGain [true]", criteria.IsMissing{"rgAlbumGain": true},
|
||||
"media_file.rg_album_gain IS NULL"),
|
||||
Entry("isMissing rgAlbumGain [false]", criteria.IsMissing{"rgAlbumGain": false},
|
||||
"media_file.rg_album_gain IS NOT NULL"),
|
||||
Entry("isPresent rgTrackPeak [true]", criteria.IsPresent{"rgTrackPeak": true},
|
||||
"media_file.rg_track_peak IS NOT NULL"),
|
||||
Entry("isPresent rgTrackPeak [false]", criteria.IsPresent{"rgTrackPeak": false},
|
||||
"media_file.rg_track_peak IS NULL"),
|
||||
// isMissing — replaygain_* tag-name alias resolves to the nullable column (issue #5584)
|
||||
Entry("isMissing replaygain_album_gain alias [true]", criteria.IsMissing{"replaygain_album_gain": true},
|
||||
"media_file.rg_album_gain IS NULL"),
|
||||
Entry("isPresent replaygain_album_gain alias [true]", criteria.IsPresent{"replaygain_album_gain": true},
|
||||
"media_file.rg_album_gain IS NOT NULL"),
|
||||
)
|
||||
|
||||
Describe("playlist permissions", func() {
|
||||
@@ -143,12 +157,12 @@ var _ = Describe("Smart playlist criteria SQL", func() {
|
||||
|
||||
It("returns an error when isMissing is used with a regular field", func() {
|
||||
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"year": true}}).Where()
|
||||
Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is only supported for tag and role fields")))
|
||||
Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field")))
|
||||
})
|
||||
|
||||
It("returns an error when isPresent is used with a regular field", func() {
|
||||
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsPresent{"title": true}}).Where()
|
||||
Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is only supported for tag and role fields")))
|
||||
Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field")))
|
||||
})
|
||||
|
||||
It("returns an error when isMissing has a non-boolean value", func() {
|
||||
|
||||
@@ -70,11 +70,13 @@ var (
|
||||
|
||||
func buildTestFS() {
|
||||
abbeyRoad := template(_t{
|
||||
"albumartist": "The Beatles",
|
||||
"artist": "The Beatles",
|
||||
"album": "Abbey Road",
|
||||
"year": 1969,
|
||||
"genre": "Rock;Blues",
|
||||
"albumartist": "The Beatles",
|
||||
"artist": "The Beatles",
|
||||
"album": "Abbey Road",
|
||||
"year": 1969,
|
||||
"genre": "Rock;Blues",
|
||||
"replaygain_album_gain": "-6.5 dB",
|
||||
"replaygain_album_peak": "0.98",
|
||||
})
|
||||
ledZepIV := template(_t{
|
||||
"albumartist": "Led Zeppelin",
|
||||
@@ -116,12 +118,16 @@ func buildTestFS() {
|
||||
fs := storagetest.FakeFS{}
|
||||
fs.SetFiles(fstest.MapFS{
|
||||
"Rock/The Beatles/Abbey Road/01 - Come Together.mp3": abbeyRoad(track(1, "Come Together",
|
||||
_t{"genre": "Rock;Blues", "composer": "Lennon/McCartney", "bpm": 120, "grouping": "Beatles Tracks"})),
|
||||
_t{"genre": "Rock;Blues", "composer": "Lennon/McCartney", "bpm": 120, "grouping": "Beatles Tracks",
|
||||
"replaygain_track_gain": "-7.1 dB", "replaygain_track_peak": "0.95"})),
|
||||
"Rock/The Beatles/Abbey Road/02 - Something.mp3": abbeyRoad(track(2, "Something",
|
||||
_t{"genre": "Rock", "composer": "Harrison", "bpm": 100, "grouping": "Beatles Tracks"})),
|
||||
_t{"genre": "Rock", "composer": "Harrison", "bpm": 100, "grouping": "Beatles Tracks",
|
||||
"replaygain_track_gain": "-6.0 dB", "replaygain_track_peak": "0.92"})),
|
||||
// Stairway To Heaven has track gain but no album gain, to distinguish the two fields
|
||||
"Rock/Led Zeppelin/IV/01 - Stairway To Heaven.flac": ledZepIV(track(1, "Stairway To Heaven",
|
||||
_t{"genre": "Rock;Folk", "composer": "Page/Plant", "bpm": 82, "suffix": "flac",
|
||||
"bitrate": 900, "samplerate": 44100, "bitdepth": 16})),
|
||||
"bitrate": 900, "samplerate": 44100, "bitdepth": 16,
|
||||
"replaygain_track_gain": "-8.25 dB", "replaygain_track_peak": "0.99"})),
|
||||
"Rock/Led Zeppelin/IV/02 - Black Dog.flac": ledZepIV(track(2, "Black Dog",
|
||||
_t{"genre": "Rock;Blues", "composer": "Page/Plant/Jones", "bpm": 150, "suffix": "flac",
|
||||
"bitrate": 900, "samplerate": 44100, "bitdepth": 16})),
|
||||
|
||||
@@ -371,4 +371,61 @@ var _ = Describe("Smart Playlists", func() {
|
||||
Expect(results).To(ConsistOf("Black Dog", "All Along the Watchtower"))
|
||||
})
|
||||
})
|
||||
|
||||
// ReplayGain values are stored in nullable media_file columns (not in the tags JSON), so
|
||||
// isMissing/isPresent translate to IS [NOT] NULL checks on those columns (issue #5584).
|
||||
Describe("isMissing/isPresent on ReplayGain fields", func() {
|
||||
It("isMissing finds tracks without album gain", func() {
|
||||
results := evaluateRule(`{"all":[{"isMissing":{"rgalbumgain":true}}]}`)
|
||||
Expect(results).To(ConsistOf("Stairway To Heaven", "Black Dog", "So What",
|
||||
"Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions"))
|
||||
})
|
||||
|
||||
It("isMissing false finds tracks with album gain", func() {
|
||||
results := evaluateRule(`{"all":[{"isMissing":{"rgalbumgain":false}}]}`)
|
||||
Expect(results).To(ConsistOf("Come Together", "Something"))
|
||||
})
|
||||
|
||||
It("isPresent finds tracks with album gain", func() {
|
||||
results := evaluateRule(`{"all":[{"isPresent":{"rgalbumgain":true}}]}`)
|
||||
Expect(results).To(ConsistOf("Come Together", "Something"))
|
||||
})
|
||||
|
||||
It("isPresent finds tracks with album peak", func() {
|
||||
results := evaluateRule(`{"all":[{"isPresent":{"rgalbumpeak":true}}]}`)
|
||||
Expect(results).To(ConsistOf("Come Together", "Something"))
|
||||
})
|
||||
|
||||
It("isMissing distinguishes track gain from album gain", func() {
|
||||
results := evaluateRule(`{"all":[{"isMissing":{"rgtrackgain":true}}]}`)
|
||||
Expect(results).To(ConsistOf("Black Dog", "So What", "Bohemian Rhapsody",
|
||||
"All Along the Watchtower", "We Are the Champions"))
|
||||
})
|
||||
|
||||
It("isPresent finds tracks with track gain", func() {
|
||||
results := evaluateRule(`{"all":[{"isPresent":{"rgtrackgain":true}}]}`)
|
||||
Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven"))
|
||||
})
|
||||
|
||||
It("resolves the replaygain_album_gain alias to the rgalbumgain column", func() {
|
||||
results := evaluateRule(`{"all":[{"isMissing":{"replaygain_album_gain":true}}]}`)
|
||||
Expect(results).To(ConsistOf("Stairway To Heaven", "Black Dog", "So What",
|
||||
"Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions"))
|
||||
})
|
||||
|
||||
It("resolves the replaygain_track_gain alias to the rgtrackgain column", func() {
|
||||
results := evaluateRule(`{"all":[{"isPresent":{"replaygain_track_gain":true}}]}`)
|
||||
Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven"))
|
||||
})
|
||||
|
||||
It("supports numeric comparisons through the replaygain_* alias", func() {
|
||||
results := evaluateRule(`{"all":[{"gt":{"replaygain_track_gain":-7.5}}]}`)
|
||||
Expect(results).To(ConsistOf("Come Together", "Something"))
|
||||
})
|
||||
|
||||
It("combines isMissing on ReplayGain with other operators", func() {
|
||||
results := evaluateRule(`{"all":[{"isMissing":{"rgalbumgain":true}},{"is":{"genre":"Blues"}}]}`)
|
||||
Expect(results).To(ConsistOf("Black Dog", "All Along the Watchtower"))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user