From 56f0518830bac54c7d29cd895de915268f7adf29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 24 Jun 2026 09:10:00 -0400 Subject: [PATCH] feat(subsonic): add OpenSubsonic work and movement attributes (#5659) * feat(subsonic): add Work/Movement response types and tag constants * feat(subsonic): surface works and movements in Child response * test(subsonic): verify works/movements JSON serialization Fix G109 lint: use strconv.ParseInt with bitSize=32 to avoid potential integer overflow; add JSON serialization test confirming omitempty on optional sub-fields. * refactor(subsonic): use number.ParseInt idiom in buildMovements * refactor(subsonic): move work/movement builders to MediaFile methods Introduces model.Work and model.Movement types with Works()/Movements() methods on MediaFile. The Subsonic layer maps them to response types inline via slice.Map, replacing the deleted buildWorks/buildMovements helpers. * test(subsonic): cover populated works/movements in response snapshots * test(subsonic): clarify empty-case name and assert JSON structurally --- model/mediafile.go | 50 +++++++++++++++ model/mediafile_test.go | 63 +++++++++++++++++++ model/tag.go | 5 ++ server/subsonic/helpers.go | 6 ++ server/subsonic/helpers_test.go | 47 ++++++++++++++ ... AlbumList with OS data should match .JSON | 4 +- ...mWithSongsID3 with data should match .JSON | 8 ++- ...sponses Child with data should match .JSON | 20 +++++- ...esponses Child with data should match .XML | 3 + ...thout data should match OpenSubsonic .JSON | 4 +- server/subsonic/responses/responses.go | 13 ++++ server/subsonic/responses/responses_test.go | 7 +++ 12 files changed, 225 insertions(+), 5 deletions(-) diff --git a/model/mediafile.go b/model/mediafile.go index d93060dba..0fd172cee 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/gg" + "github.com/navidrome/navidrome/utils/number" "github.com/navidrome/navidrome/utils/slice" ) @@ -151,6 +152,55 @@ func (mf MediaFile) String() string { return mf.Path } +type Work struct { + Name string + MbzWorkID string +} + +type Movement struct { + Name string + Number int32 + Count int32 +} + +func (mf MediaFile) Works() []Work { + names := mf.Tags.Values(TagWork) + if len(names) == 0 { + return nil + } + ids := mf.Tags.Values(TagMusicBrainzWorkID) + works := make([]Work, 0, len(names)) + for i, name := range names { + w := Work{Name: name} + if i < len(ids) { + w.MbzWorkID = ids[i] + } + works = append(works, w) + } + return works +} + +func (mf MediaFile) Movements() []Movement { + names := mf.Tags.Values(TagMovementName) + if len(names) == 0 { + return nil + } + numbers := mf.Tags.Values(TagMovementNumber) + counts := mf.Tags.Values(TagMovementTotal) + movements := make([]Movement, 0, len(names)) + for i, name := range names { + m := Movement{Name: name} + if i < len(numbers) { + m.Number = number.ParseInt[int32](numbers[i]) + } + if i < len(counts) { + m.Count = number.ParseInt[int32](counts[i]) + } + movements = append(movements, m) + } + return movements +} + // Hash returns a hash of the MediaFile based on its tags and audio properties func (mf MediaFile) Hash() string { opts := &hashstructure.HashOptions{ diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 65c5a0652..f070f4649 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -604,6 +604,69 @@ var _ = Describe("MediaFile", func() { }) +var _ = Describe("MediaFile.Works", func() { + It("returns nil when there are no work tags", func() { + mf := MediaFile{} + Expect(mf.Works()).To(BeNil()) + }) + + It("pairs a work name with its MbzWorkID", func() { + mf := MediaFile{Tags: Tags{ + TagWork: {"Symphony No. 5"}, + TagMusicBrainzWorkID: {"abc-123"}, + }} + Expect(mf.Works()).To(Equal([]Work{ + {Name: "Symphony No. 5", MbzWorkID: "abc-123"}, + })) + }) + + It("leaves MbzWorkID empty when no id is present", func() { + mf := MediaFile{Tags: Tags{TagWork: {"Symphony No. 5"}}} + Expect(mf.Works()).To(Equal([]Work{ + {Name: "Symphony No. 5"}, + })) + }) + + It("pairs by index and ignores extra ids", func() { + mf := MediaFile{Tags: Tags{ + TagWork: {"Work A", "Work B"}, + TagMusicBrainzWorkID: {"id-a"}, + }} + Expect(mf.Works()).To(Equal([]Work{ + {Name: "Work A", MbzWorkID: "id-a"}, + {Name: "Work B"}, + })) + }) +}) + +var _ = Describe("MediaFile.Movements", func() { + It("returns nil when there are no movement tags", func() { + mf := MediaFile{} + Expect(mf.Movements()).To(BeNil()) + }) + + It("builds a movement with name, number and count", func() { + mf := MediaFile{Tags: Tags{ + TagMovementName: {"I. Allegro"}, + TagMovementNumber: {"1"}, + TagMovementTotal: {"4"}, + }} + Expect(mf.Movements()).To(Equal([]Movement{ + {Name: "I. Allegro", Number: 1, Count: 4}, + })) + }) + + It("non-numeric number/count yields 0", func() { + mf := MediaFile{Tags: Tags{ + TagMovementName: {"I. Allegro"}, + TagMovementNumber: {"not-a-number"}, + }} + Expect(mf.Movements()).To(Equal([]Movement{ + {Name: "I. Allegro"}, + })) + }) +}) + var _ = Describe("MediaFile.Hash", func() { // Guards the upgrade guarantee: converting BPM/BitDepth from int to *int must not change hashes, // or every file would be spuriously re-imported on the next scan. diff --git a/model/tag.go b/model/tag.go index 1f6b24d21..02ccac05d 100644 --- a/model/tag.go +++ b/model/tag.go @@ -192,6 +192,10 @@ const ( TagISRC TagName = "isrc" TagBPM TagName = "bpm" TagExplicitStatus TagName = "explicitstatus" + TagWork TagName = "work" + TagMovementName TagName = "movementname" + TagMovementNumber TagName = "movement" + TagMovementTotal TagName = "movementtotal" // Dates and years @@ -240,6 +244,7 @@ const ( TagMusicBrainzAlbumArtistID TagName = "musicbrainz_albumartistid" TagMusicBrainzAlbumID TagName = "musicbrainz_albumid" TagMusicBrainzReleaseGroupID TagName = "musicbrainz_releasegroupid" + TagMusicBrainzWorkID TagName = "musicbrainz_workid" TagMusicBrainzComposerID TagName = "musicbrainz_composerid" TagMusicBrainzLyricistID TagName = "musicbrainz_lyricistid" diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index 4027ba8b6..a76165cc1 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -290,6 +290,12 @@ func osChildFromMediaFile(ctx context.Context, mf model.MediaFile) *responses.Op } child.Contributors = contributors child.ExplicitStatus = mapExplicitStatus(mf.ExplicitStatus) + child.Works = slice.Map(mf.Works(), func(w model.Work) responses.Work { + return responses.Work{Name: w.Name, MusicBrainzId: w.MbzWorkID} + }) + child.Movements = slice.Map(mf.Movements(), func(m model.Movement) responses.Movement { + return responses.Movement{Name: m.Name, Number: m.Number, Count: m.Count} + }) return &child } diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index 2ae6eb28e..ed8f257d1 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -2,6 +2,7 @@ package subsonic import ( "context" + "encoding/json" "net/http/httptest" "time" @@ -358,6 +359,52 @@ var _ = Describe("helpers", func() { Expect(osChild).ToNot(BeNil()) Expect(osChild.Comment).To(Equal("Test Comment")) }) + + It("populates works and movements from tags", func() { + mf.Tags = model.Tags{ + model.TagWork: {"Symphony No. 5"}, + model.TagMusicBrainzWorkID: {"abc-123"}, + model.TagMovementName: {"I. Allegro"}, + model.TagMovementNumber: {"1"}, + model.TagMovementTotal: {"4"}, + } + osChild := osChildFromMediaFile(ctx, mf) + Expect(osChild).ToNot(BeNil()) + Expect(osChild.Works).To(Equal(responses.Array[responses.Work]{ + {Name: "Symphony No. 5", MusicBrainzId: "abc-123"}, + })) + Expect(osChild.Movements).To(Equal(responses.Array[responses.Movement]{ + {Name: "I. Allegro", Number: 1, Count: 4}, + })) + }) + + It("returns empty works and movements when no classical tags are present", func() { + osChild := osChildFromMediaFile(ctx, mf) + Expect(osChild).ToNot(BeNil()) + Expect(osChild.Works).To(BeEmpty()) + Expect(osChild.Movements).To(BeEmpty()) + }) + + It("serializes works and movements to spec-compliant JSON", func() { + mf.Tags = model.Tags{ + model.TagWork: {"Symphony No. 5"}, + model.TagMovementName: {"I. Allegro"}, + model.TagMovementNumber: {"1"}, + } + osChild := osChildFromMediaFile(ctx, mf) + data, err := json.Marshal(osChild) + Expect(err).ToNot(HaveOccurred()) + + var got map[string]any + Expect(json.Unmarshal(data, &got)).To(Succeed()) + // Required name present; optional musicBrainzId/count omitted (omitempty); number present. + Expect(got).To(HaveKeyWithValue("works", []any{ + map[string]any{"name": "Symphony No. 5"}, + })) + Expect(got).To(HaveKeyWithValue("movements", []any{ + map[string]any{"name": "I. Allegro", "number": float64(1)}, + })) + }) }) Context("when legacy clients list is empty", func() { diff --git a/server/subsonic/responses/.snapshots/Responses AlbumList with OS data should match .JSON b/server/subsonic/responses/.snapshots/Responses AlbumList with OS data should match .JSON index 9d9ae2195..d6b195f58 100644 --- a/server/subsonic/responses/.snapshots/Responses AlbumList with OS data should match .JSON +++ b/server/subsonic/responses/.snapshots/Responses AlbumList with OS data should match .JSON @@ -59,7 +59,9 @@ "explicitStatus": "explicit", "groupings": [ "Soundtrack" - ] + ], + "works": [], + "movements": [] } ] } diff --git a/server/subsonic/responses/.snapshots/Responses AlbumWithSongsID3 with data should match .JSON b/server/subsonic/responses/.snapshots/Responses AlbumWithSongsID3 with data should match .JSON index bff0bd20c..f776c7535 100644 --- a/server/subsonic/responses/.snapshots/Responses AlbumWithSongsID3 with data should match .JSON +++ b/server/subsonic/responses/.snapshots/Responses AlbumWithSongsID3 with data should match .JSON @@ -171,7 +171,9 @@ "groupings": [ "Soundtrack", "Live" - ] + ], + "works": [], + "movements": [] }, { "id": "2", @@ -217,7 +219,9 @@ "contributors": [], "displayComposer": "", "explicitStatus": "", - "groupings": [] + "groupings": [], + "works": [], + "movements": [] } ] } diff --git a/server/subsonic/responses/.snapshots/Responses Child with data should match .JSON b/server/subsonic/responses/.snapshots/Responses Child with data should match .JSON index 4c0ea6c68..fef60c9b1 100644 --- a/server/subsonic/responses/.snapshots/Responses Child with data should match .JSON +++ b/server/subsonic/responses/.snapshots/Responses Child with data should match .JSON @@ -114,6 +114,22 @@ "groupings": [ "Soundtrack", "Live" + ], + "works": [ + { + "name": "Symphony No. 5", + "musicBrainzId": "mbz-work-1" + }, + { + "name": "Encore" + } + ], + "movements": [ + { + "name": "I. Allegro", + "number": 1, + "count": 4 + } ] }, { @@ -146,7 +162,9 @@ "contributors": [], "displayComposer": "", "explicitStatus": "", - "groupings": [] + "groupings": [], + "works": [], + "movements": [] } ], "id": "1", diff --git a/server/subsonic/responses/.snapshots/Responses Child with data should match .XML b/server/subsonic/responses/.snapshots/Responses Child with data should match .XML index ddceb67d4..b626fd6ea 100644 --- a/server/subsonic/responses/.snapshots/Responses Child with data should match .XML +++ b/server/subsonic/responses/.snapshots/Responses Child with data should match .XML @@ -26,6 +26,9 @@ Soundtrack Live + + + diff --git a/server/subsonic/responses/.snapshots/Responses Child without data should match OpenSubsonic .JSON b/server/subsonic/responses/.snapshots/Responses Child without data should match OpenSubsonic .JSON index 9a9ab1ff6..ea23dc5d6 100644 --- a/server/subsonic/responses/.snapshots/Responses Child without data should match OpenSubsonic .JSON +++ b/server/subsonic/responses/.snapshots/Responses Child without data should match OpenSubsonic .JSON @@ -29,7 +29,9 @@ "contributors": [], "displayComposer": "", "explicitStatus": "", - "groupings": [] + "groupings": [], + "works": [], + "movements": [] } ], "id": "", diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index 7e41a1daa..252eee4c6 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -190,6 +190,8 @@ type OpenSubsonicChild struct { DisplayComposer string `xml:"displayComposer,attr,omitempty" json:"displayComposer"` ExplicitStatus string `xml:"explicitStatus,attr,omitempty" json:"explicitStatus"` Groupings Array[string] `xml:"groupings,omitempty" json:"groupings"` + Works Array[Work] `xml:"works,omitempty" json:"works"` + Movements Array[Movement] `xml:"movements,omitempty" json:"movements"` } type Songs struct { @@ -598,6 +600,17 @@ type ItemGenre struct { Name string `xml:"name,attr" json:"name"` } +type Work struct { + Name string `xml:"name,attr" json:"name"` + MusicBrainzId string `xml:"musicBrainzId,attr,omitempty" json:"musicBrainzId,omitempty"` +} + +type Movement struct { + Name string `xml:"name,attr" json:"name"` + Number int32 `xml:"number,attr,omitempty" json:"number,omitempty"` + Count int32 `xml:"count,attr,omitempty" json:"count,omitempty"` +} + type ReplayGain struct { TrackGain *float64 `xml:"trackGain,omitempty,attr" json:"trackGain,omitempty"` AlbumGain *float64 `xml:"albumGain,omitempty,attr" json:"albumGain,omitempty"` diff --git a/server/subsonic/responses/responses_test.go b/server/subsonic/responses/responses_test.go index 3166df875..586e46b63 100644 --- a/server/subsonic/responses/responses_test.go +++ b/server/subsonic/responses/responses_test.go @@ -239,6 +239,13 @@ var _ = Describe("Responses", func() { {Role: "composer", Artist: ArtistID3Ref{Id: "4", Name: "composer2"}}, }, ExplicitStatus: "clean", + Works: []Work{ + {Name: "Symphony No. 5", MusicBrainzId: "mbz-work-1"}, + {Name: "Encore"}, + }, + Movements: []Movement{ + {Name: "I. Allegro", Number: 1, Count: 4}, + }, } child[1].OpenSubsonicChild = &OpenSubsonicChild{ ReplayGain: ReplayGain{TrackGain: new(0.0), AlbumGain: new(0.0), TrackPeak: new(0.0), AlbumPeak: new(0.0), BaseGain: new(0.0), FallbackGain: new(0.0)},