diff --git a/model/criteria/fields.go b/model/criteria/fields.go index 9eafff7ab..36206712f 100644 --- a/model/criteria/fields.go +++ b/model/criteria/fields.go @@ -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 { diff --git a/model/criteria/fields_test.go b/model/criteria/fields_test.go index 5b6f53341..3dc0c7b90 100644 --- a/model/criteria/fields_test.go +++ b/model/criteria/fields_test.go @@ -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()) + }) + }) }) diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 37e4ae340..ee0baca18 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -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 } diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 5c8909e1c..8e801a703 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -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() { diff --git a/persistence/e2e/e2e_suite_test.go b/persistence/e2e/e2e_suite_test.go index f42292f02..1ff2139e6 100644 --- a/persistence/e2e/e2e_suite_test.go +++ b/persistence/e2e/e2e_suite_test.go @@ -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})), diff --git a/persistence/e2e/smartplaylist_test.go b/persistence/e2e/smartplaylist_test.go index a844dc982..4966d3b8a 100644 --- a/persistence/e2e/smartplaylist_test.go +++ b/persistence/e2e/smartplaylist_test.go @@ -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")) + }) + }) })