feat(subsonic): add ISRC support for OpenSubsonic Child (#4088)

* docs: add testing and logging guidelines to AGENTS.md Signed-off-by: Deluan <deluan@navidrome.org> * Introduce TagISRC and update ISRC handling * fix: update .gitignore to exclude executable files and bin directory Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2025-12-23 23:18:05 -05:00 · 2025-05-20 12:37:27 -04:00
parent 1e4e3eac6e
commit ba7fd13724
12 changed files with 130 additions and 2 deletions
--- a/.gitignore
+++ b/.gitignore
@@ -24,4 +24,5 @@ docker-compose.yml
 !contrib/docker-compose.yml
 binaries
 navidrome-master
-*.exe
+*.exe
+bin/
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -0,0 +1,110 @@
+# Testing Instructions
+
+- **No implementation task is considered complete until it includes thorough, passing tests that cover the new or
+  changed functionality. All new code must be accompanied by Ginkgo/Gomega tests, and PRs/commits without tests should
+  be considered incomplete.**
+- All Go tests in this project **MUST** be written using the **Ginkgo v2** and **Gomega** frameworks.
+- To run all tests, use `make test`.
+- To run tests for a specific package, use `make test PKG=./pkgname/...`
+- Do not run tests in parallel
+- Don't use `--fail-on-pending`
+
+## Mocking Convention
+
+- Always try to use the mocks provided in the `tests` package before creating a new mock implementation.
+- Only create a new mock if the required functionality is not covered by the existing mocks in `tests`.
+- Never mock a real implementation when testing. Remember: there is no value in testing an interface, only the real implementation.
+
+## Example
+
+Every package that you write tests for, should have a `*_suite_test.go` file, to hook up the Ginkgo test suite. Example:
+```
+package core
+
+import (
+	"testing"
+
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestCore(t *testing.T) {
+	tests.Init(t, false)
+	log.SetLevel(log.LevelFatal)
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Core Suite")
+}
+```
+Never put a `func Test*` in regular *_test.go files, only in `*_suite_test.go` files.
+
+Refer to existing test suites for examples of proper setup and usage, such as the one defined in @core_suite_test.go
+
+## Exceptions
+
+There should be no exceptions to this rule. If you encounter tests written with the standard `testing` package or other frameworks, they should be refactored to use Ginkgo/Gomega. If you need a new mock, first confirm that it does not already exist in the `tests` package.
+
+### Configuration
+
+You can set config values in the BeforeEach/BeforeAll blocks. If you do so, remember to add `DeferCleanup(configtest.SetupConfig())` to reset the values. Example:
+
+```go
+BeforeEach(func() {
+    DeferCleanup(configtest.SetupConfig())
+    conf.Server.EnableDownloads = true
+})
+```
+
+# Logging System Usage Guide
+
+This project uses a custom logging system built on top of logrus, `log/log.go`. Follow these conventions for all logging:
+
+## Logging API
+- Use the provided functions for logging at different levels:
+    - `Error(...)`, `Warn(...)`, `Info(...)`, `Debug(...)`, `Trace(...)`, `Fatal(...)`
+- These functions accept flexible arguments:
+    - The first argument can be a context (`context.Context`), an HTTP request, or `nil`.
+    - The next argument is the log message (string or error).
+    - Additional arguments are key-value pairs (e.g., `"key", value`).
+    - If the last argument is an error, it is logged under the `error` key.
+
+**Examples:**
+```go
+log.Error("A message")
+log.Error(ctx, "A message with context")
+log.Error("Failed to save", "id", 123, err)
+log.Info(req, "Request received", "user", userID)
+```
+
+## Logging errors
+- You don't need to add "err" key when logging an error, it is automatically added.
+- Error must always be the last parameter in the log call.
+  Examples:
+```go
+log.Error("Failed to save", "id", 123, err) // GOOD
+log.Error("Failed to save", "id", 123, "err", err) // BAD
+log.Error("Failed to save", err, "id", 123) // BAD
+```
+
+## Context and Request Logging
+- If a context or HTTP request is passed as the first argument, any logger fields in the context are included in the log entry.
+- Use `log.NewContext(ctx, "key", value, ...)` to add fields to a context for logging.
+
+## Log Levels
+- Set the global log level with `log.SetLevel(log.LevelInfo)` or `log.SetLevelString("info")`.
+- Per-path log levels can be set with `log.SetLogLevels(map[string]string{"path": "level"})`.
+- Use `log.IsGreaterOrEqualTo(level)` to check if a log level is enabled for the current code path.
+
+## Source Line Logging
+- Enable source file/line logging with `log.SetLogSourceLine(true)`.
+
+## Best Practices
+- Always use the logging API, never log directly with logrus or fmt.
+- Prefer structured logging (key-value pairs) for important data.
+- Use context/request logging for traceability in web handlers.
+- For tests, use Ginkgo/Gomega and set up a test logger as in `log/log_test.go`.
+
+## See Also
+- `log/log.go` for implementation details
+- `log/log_test.go` for usage examples and test patterns
--- a/model/tag.go
+++ b/model/tag.go
@@ -191,6 +191,7 @@ const (
 	TagReleaseCountry TagName = "releasecountry"
 	TagMedia          TagName = "media"
 	TagCatalogNumber  TagName = "catalognumber"
+	TagISRC           TagName = "isrc"
 	TagBPM            TagName = "bpm"
 	TagExplicitStatus TagName = "explicitstatus"

--- a/server/subsonic/helpers.go
+++ b/server/subsonic/helpers.go
@@ -224,6 +224,7 @@ func osChildFromMediaFile(ctx context.Context, mf model.MediaFile) *responses.Op
 	child.BPM = int32(mf.BPM)
 	child.MediaType = responses.MediaTypeSong
 	child.MusicBrainzId = mf.MbzRecordingID
+	child.Isrc = mf.Tags.Values(model.TagISRC)
 	child.ReplayGain = responses.ReplayGain{
 		TrackGain: mf.RGTrackGain,
 		AlbumGain: mf.RGAlbumGain,
--- a/server/subsonic/responses/.snapshots/Responses
+++ b/server/subsonic/responses/.snapshots/Responses
@@ -15,6 +15,7 @@
        "sortName": "sort name",
        "mediaType": "album",
        "musicBrainzId": "00000000-0000-0000-0000-000000000000",
+        "isrc": [],
        "genres": [
          {
            "name": "Genre 1"
--- a/server/subsonic/responses/.snapshots/Responses
+++ b/server/subsonic/responses/.snapshots/Responses
@@ -99,6 +99,9 @@
        "sortName": "sorted song",
        "mediaType": "song",
        "musicBrainzId": "4321",
+        "isrc": [
+          "ISRC-1"
+        ],
        "genres": [
          {
            "name": "rock"
--- a/server/subsonic/responses/.snapshots/Responses
+++ b/server/subsonic/responses/.snapshots/Responses
@@ -16,6 +16,7 @@
    <artists id="1" name="artist1"></artists>
    <artists id="2" name="artist2"></artists>
    <song id="1" isDir="true" title="title" album="album" artist="artist" track="1" year="1985" genre="Rock" coverArt="1" size="8421341" contentType="audio/flac" suffix="flac" starred="2016-03-02T20:30:00Z" transcodedContentType="audio/mpeg" transcodedSuffix="mp3" duration="146" bitRate="320" isVideo="false" bpm="127" comment="a comment" sortName="sorted song" mediaType="song" musicBrainzId="4321" channelCount="2" samplingRate="44100" bitDepth="16" displayArtist="artist1 &amp; artist2" displayAlbumArtist="album artist1 &amp; album artist2" displayComposer="composer 1 &amp; composer 2" explicitStatus="clean">
+      <isrc>ISRC-1</isrc>
      <genres name="rock"></genres>
      <genres name="progressive"></genres>
      <replayGain trackGain="1" albumGain="2" trackPeak="3" albumPeak="4" baseGain="5" fallbackGain="6"></replayGain>
--- a/server/subsonic/responses/.snapshots/Responses
+++ b/server/subsonic/responses/.snapshots/Responses
@@ -30,6 +30,10 @@
        "sortName": "sorted title",
        "mediaType": "song",
        "musicBrainzId": "4321",
+        "isrc": [
+          "ISRC-1",
+          "ISRC-2"
+        ],
        "genres": [
          {
            "name": "rock"
--- a/server/subsonic/responses/.snapshots/Responses
+++ b/server/subsonic/responses/.snapshots/Responses
@@ -1,6 +1,8 @@
 <subsonic-response xmlns="http://subsonic.org/restapi" status="ok" version="1.16.1" type="navidrome" serverVersion="v0.55.0" openSubsonic="true">
  <directory id="1" name="N">
    <child id="1" isDir="true" title="title" album="album" artist="artist" track="1" year="1985" genre="Rock" coverArt="1" size="8421341" contentType="audio/flac" suffix="flac" starred="2016-03-02T20:30:00Z" transcodedContentType="audio/mpeg" transcodedSuffix="mp3" duration="146" bitRate="320" isVideo="false" bpm="127" comment="a comment" sortName="sorted title" mediaType="song" musicBrainzId="4321" channelCount="2" samplingRate="44100" bitDepth="16" displayArtist="artist 1 &amp; artist 2" displayAlbumArtist="album artist 1 &amp; album artist 2" displayComposer="composer 1 &amp; composer 2" explicitStatus="clean">
+      <isrc>ISRC-1</isrc>
+      <isrc>ISRC-2</isrc>
      <genres name="rock"></genres>
      <genres name="progressive"></genres>
      <replayGain trackGain="1" albumGain="2" trackPeak="3" albumPeak="4" baseGain="5" fallbackGain="6"></replayGain>
--- a/server/subsonic/responses/.snapshots/Responses
+++ b/server/subsonic/responses/.snapshots/Responses
@@ -15,6 +15,7 @@
        "sortName": "",
        "mediaType": "",
        "musicBrainzId": "",
+        "isrc": [],
        "genres": [],
        "replayGain": {},
        "channelCount": 0,
--- a/server/subsonic/responses/responses.go
+++ b/server/subsonic/responses/responses.go
@@ -176,6 +176,7 @@ type OpenSubsonicChild struct {
 	SortName           string              `xml:"sortName,attr,omitempty"           json:"sortName"`
 	MediaType          MediaType           `xml:"mediaType,attr,omitempty"          json:"mediaType"`
 	MusicBrainzId      string              `xml:"musicBrainzId,attr,omitempty"      json:"musicBrainzId"`
+	Isrc               Array[string]       `xml:"isrc,omitempty"                  json:"isrc"`
 	Genres             Array[ItemGenre]    `xml:"genres,omitempty"                  json:"genres"`
 	ReplayGain         ReplayGain          `xml:"replayGain,omitempty"              json:"replayGain"`
 	ChannelCount       int32               `xml:"channelCount,attr,omitempty"       json:"channelCount"`
--- a/server/subsonic/responses/responses_test.go
+++ b/server/subsonic/responses/responses_test.go
@@ -224,7 +224,8 @@ var _ = Describe("Responses", func() {
 				child[0].OpenSubsonicChild = &OpenSubsonicChild{
 					Genres:  []ItemGenre{{Name: "rock"}, {Name: "progressive"}},
 					Comment: "a comment", MediaType: MediaTypeSong, MusicBrainzId: "4321", SortName: "sorted title",
-					BPM: 127, ChannelCount: 2, SamplingRate: 44100, BitDepth: 16,
+					Isrc: []string{"ISRC-1", "ISRC-2"},
+					BPM:  127, ChannelCount: 2, SamplingRate: 44100, BitDepth: 16,
 					Moods:         []string{"happy", "sad"},
 					ReplayGain:    ReplayGain{TrackGain: 1, AlbumGain: 2, TrackPeak: 3, AlbumPeak: 4, BaseGain: 5, FallbackGain: 6},
 					DisplayArtist: "artist 1 & artist 2",
@@ -312,6 +313,7 @@ var _ = Describe("Responses", func() {
 				songs[0].OpenSubsonicChild = &OpenSubsonicChild{
 					Genres:  []ItemGenre{{Name: "rock"}, {Name: "progressive"}},
 					Comment: "a comment", MediaType: MediaTypeSong, MusicBrainzId: "4321", SortName: "sorted song",
+					Isrc:       []string{"ISRC-1"},
 					Moods:      []string{"happy", "sad"},
 					ReplayGain: ReplayGain{TrackGain: 1, AlbumGain: 2, TrackPeak: 3, AlbumPeak: 4, BaseGain: 5, FallbackGain: 6},
 					BPM:        127, ChannelCount: 2, SamplingRate: 44100, BitDepth: 16,