fix: multivalue tags

2026-02-08 16:50:29 +01:00
parent b59d30784f
commit 8993c8031a
7 changed files with 640 additions and 26 deletions
--- a/http/data.go
+++ b/http/data.go
@@ -48,9 +48,32 @@ func (d *data) Check(path string) bool {
 }

 func handle(fn handleFunc, prefix string, store *storage.Storage, server *settings.Server) http.Handler {
-	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+    // trackingWriter wraps the ResponseWriter to detect if headers/body were already written.
+    type trackingWriter struct {
+        http.ResponseWriter
+        wroteHeader bool
+        wroteBody   bool
+    }
+    func (tw *trackingWriter) WriteHeader(code int) {
+        if !tw.wroteHeader {
+            tw.wroteHeader = true
+            tw.ResponseWriter.WriteHeader(code)
+        }
+    }
+    func (tw *trackingWriter) Write(b []byte) (int, error) {
+        // Any Write implies headers committed (status 200 if none set)
+        if !tw.wroteHeader {
+            tw.wroteHeader = true
+        }
+        tw.wroteBody = true
+        return tw.ResponseWriter.Write(b)
+    }
+
+    handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+        // Wrap writer to prevent superfluous WriteHeader calls
+        tw := &trackingWriter{ResponseWriter: w}
 		for k, v := range globalHeaders {
-			w.Header().Set(k, v)
+			tw.Header().Set(k, v)
 		}

 		settings, err := store.Settings.Get()
@@ -59,7 +82,7 @@ func handle(fn handleFunc, prefix string, store *storage.Storage, server *settin
 			return
 		}

-		status, err := fn(w, r, &data{
+		status, err := fn(tw, r, &data{
 			Runner:   &runner.Runner{Enabled: server.EnableExec, Settings: settings},
 			store:    store,
 			settings: settings,
@@ -76,7 +99,10 @@ func handle(fn handleFunc, prefix string, store *storage.Storage, server *settin
 			if status == http.StatusBadRequest && err != nil {
 				txt += " (" + err.Error() + ")"
 			}
-			http.Error(w, strconv.Itoa(status)+" "+txt, status)
+			// Only write an error response if nothing was written yet.
+			if !tw.wroteHeader && !tw.wroteBody {
+				http.Error(tw, strconv.Itoa(status)+" "+txt, status)
+			}
 			return
 		}
 	})
--- a/http/metadata.go
+++ b/http/metadata.go
@@ -64,10 +64,8 @@ var canonicalMap = map[string]string{
 var ffmpegKey = map[string]string{
    "Title":       "title",
    "Artist":      "artist",
-    "Artists":     "artist",
    "Album":       "album",
    "AlbumArtist": "album_artist",
-    "AlbumArtists": "album_artist",
    "Composer":    "composer",
    "Track":       "track",
    "TrackNumber": "track",
@@ -80,12 +78,22 @@ var ffmpegKey = map[string]string{
    "Lyrics":      "lyrics",
 }

+// ffmpegMultiKey maps multi-valued canonical names to ffmpeg/Vorbis keys.
+// For FLAC/Vorbis, use uppercase ARTISTS/ALBUMARTISTS for proper multi-value support.
+var ffmpegMultiKey = map[string]string{
+    "Artists":      "ARTISTS",
+    "AlbumArtists": "ALBUMARTISTS",
+}
+
 // allowedCanonicals is the set of canonical tags we accept for writes.
 var allowedCanonicals = func() map[string]struct{} {
-    m := make(map[string]struct{}, len(ffmpegKey))
+    m := make(map[string]struct{}, len(ffmpegKey)+len(ffmpegMultiKey))
    for k := range ffmpegKey {
        m[k] = struct{}{}
    }
+    for k := range ffmpegMultiKey {
+        m[k] = struct{}{}
+    }
    return m
 }()

@@ -132,6 +140,7 @@ func normalizeAndMapToFFmpeg(in map[string]string) map[string]string {
    return out
 }
 // normalizeMultiToFFmpeg maps multi-valued canonical keys to ffmpeg keys, preserving arrays.
+// Uses ffmpegMultiKey for proper Vorbis comment names (ARTISTS, ALBUMARTISTS).
 func normalizeMultiToFFmpeg(in map[string][]string) map[string][]string {
    out := map[string][]string{}
    for k, arr := range in {
@@ -145,9 +154,14 @@ func normalizeMultiToFFmpeg(in map[string][]string) map[string][]string {
                continue
            }
        }
-        ffk, ok := ffmpegKey[canonical]
+        // First check if this is a multi-valued tag
+        ffk, ok := ffmpegMultiKey[canonical]
        if !ok {
-            continue
+            // Fall back to regular ffmpeg key
+            ffk, ok = ffmpegKey[canonical]
+            if !ok {
+                continue
+            }
        }
        vals := []string{}
        for _, v := range arr {
--- a/http/resource.go
+++ b/http/resource.go
@@ -298,7 +298,23 @@ func resourcePatchHandler(fileCache FileCache) handleFunc {
 				}
 			}

+			// If the source is a directory, apply changes to all audio files within.
+			// Otherwise, apply to the single file.
 			err = d.RunHook(func() error {
+				fi, statErr := files.NewFileInfo(&files.FileOptions{
+					Fs:         d.user.Fs,
+					Path:       src,
+					Modify:     d.user.Perm.Modify,
+					Expand:     false,
+					ReadHeader: false,
+					Checker:    d,
+				})
+				if statErr != nil {
+					return statErr
+				}
+				if fi.IsDir {
+					return applyMetadataToDir(r.Context(), d, src, tags, multi, clear)
+				}
 				return applyMetadataWithFFmpeg(r.Context(), d, src, tags, multi, clear)
 			}, action, src, dst, d.user)

@@ -316,6 +332,8 @@ func resourcePatchHandler(fileCache FileCache) handleFunc {
 // applyMetadataWithFFmpeg attempts to write metadata using ffmpeg by creating
 // a temporary file and replacing the original. This requires that the
 // underlying filesystem exposes a real path (see FileInfo.RealPath()).
+// For FLAC files with multi-valued tags (ARTISTS, ALBUMARTISTS), it uses
+// metaflac directly since ffmpeg doesn't support true multi-valued Vorbis comments.
 func applyMetadataWithFFmpeg(ctx context.Context, d *data, src string, tags map[string]string, multi map[string][]string, clear []string) error {
 	fi, err := files.NewFileInfo(&files.FileOptions{
 		Fs:         d.user.Fs,
@@ -362,10 +380,26 @@ func applyMetadataWithFFmpeg(ctx context.Context, d *data, src string, tags map[
 	isMP3 := strings.EqualFold(ext, ".mp3")
 	// Treat M4A/MP4 similarly for metadata handling specifics
 	isMP4 := strings.EqualFold(ext, ".m4a") || strings.EqualFold(ext, ".m4b") || strings.EqualFold(ext, ".mp4")
+	// FLAC supports true multi-valued Vorbis comments via metaflac
+	isFLAC := strings.EqualFold(ext, ".flac")
 	if isMP3 {
 		args = append(args, "-id3v2_version", "4")
 	}

+	// For FLAC files with multi-valued tags, use metaflac which properly supports
+	// multiple values for the same tag (Vorbis comments).
+	if isFLAC && len(multi) > 0 {
+		if _, err := exec.LookPath("metaflac"); err == nil {
+			// Use metaflac for multi-valued tags
+			if err := applyMultiValuedTagsWithMetaflac(ctx, real, tags, multi, clear); err != nil {
+				log.Printf("metaflac failed, falling back to ffmpeg: %v", err)
+				// Fall through to ffmpeg
+			} else {
+				return nil
+			}
+		}
+	}
+
 	// If MP3 and there are clears, perform ID3v2 in-place removal first.
 	// This avoids ffmpeg creating unwanted empty entries (e.g., TXXX artifacts).
 	didID3Clear := false
@@ -395,12 +429,11 @@ func applyMetadataWithFFmpeg(ctx context.Context, d *data, src string, tags map[
 		changes++
 		setKeys[k] = struct{}{}
 	}
-	// Handle multi-valued tags (e.g., Artists, AlbumArtists) by adding repeated metadata entries
+	// Handle multi-valued tags (e.g., Artists, AlbumArtists)
 	if len(multi) > 0 {
 		mm := normalizeMultiToFFmpeg(multi)
 		for ffk, vals := range mm {
-			// MP4/M4A generally expects a single value for artist/album_artist.
-			// Join multiple values for better compatibility with tag readers.
+			// MP4/M4A expects a single value; join for compatibility
 			if isMP4 && (ffk == "artist" || ffk == "album_artist") {
 				if len(vals) > 0 {
 					joined := strings.Join(vals, "; ")
@@ -410,6 +443,28 @@ func applyMetadataWithFFmpeg(ctx context.Context, d *data, src string, tags map[
 				}
 				continue
 			}
+			// MP3 (ID3): prefer writing consolidated artist/album_artist rather than TXXX ARTISTS
+			if isMP3 {
+				if ffk == "ARTISTS" {
+					if len(vals) > 0 {
+						joined := strings.Join(vals, "; ")
+						args = append(args, "-metadata", fmt.Sprintf("%s=%s", "artist", joined))
+						changes++
+						setKeys["artist"] = struct{}{}
+					}
+					continue
+				}
+				if ffk == "ALBUMARTISTS" {
+					if len(vals) > 0 {
+						joined := strings.Join(vals, "; ")
+						args = append(args, "-metadata", fmt.Sprintf("%s=%s", "album_artist", joined))
+						changes++
+						setKeys["album_artist"] = struct{}{}
+					}
+					continue
+				}
+			}
+			// Default: add repeated metadata entries for formats that support it (e.g., Vorbis)
 			for _, v := range vals {
 				args = append(args, "-metadata", fmt.Sprintf("%s=%s", ffk, v))
 				changes++
@@ -463,6 +518,68 @@ func applyMetadataWithFFmpeg(ctx context.Context, d *data, src string, tags map[
 	return nil
 }

+// applyMetadataToDir iterates all immediate files in a directory and applies
+// metadata changes to each supported audio file.
+func applyMetadataToDir(ctx context.Context, d *data, dir string, tags map[string]string, multi map[string][]string, clear []string) error {
+	// List directory entries
+	f, err := d.user.Fs.Open(dir)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+
+	names, err := f.Readdirnames(0)
+	if err != nil {
+		return err
+	}
+
+	// Supported audio extensions for metadata updates
+	isAudio := func(ext string) bool {
+		ext = strings.ToLower(ext)
+		switch ext {
+		case ".mp3", ".flac", ".m4a", ".mp4", ".ogg", ".wav":
+			return true
+		default:
+			return false
+		}
+	}
+
+	processed := 0
+	failed := 0
+	for _, name := range names {
+		p := filepath.Join(dir, name)
+		info, err := d.user.Fs.Stat(p)
+		if err != nil {
+			// skip entries we can't stat
+			log.Printf("metadata: skip %s: %v", p, err)
+			continue
+		}
+		if info.IsDir() {
+			continue
+		}
+		if !isAudio(filepath.Ext(name)) {
+			continue
+		}
+		if err := applyMetadataWithFFmpeg(ctx, d, p, tags, multi, clear); err != nil {
+			failed++
+			log.Printf("metadata: failed for %s: %v", p, err)
+			continue
+		}
+		processed++
+	}
+
+	if processed == 0 {
+		return fmt.Errorf("no audio files found in album: %w", fberrors.ErrInvalidRequestParams)
+	}
+	if failed > 0 {
+		// Do not fail entire album operation; return success and rely on logs
+		// to indicate which files failed. This avoids a 500 when some files
+		// are temporarily locked or have unsupported formats.
+		return nil
+	}
+	return nil
+}
+
 func checkParent(src, dst string) error {
 	rel, err := filepath.Rel(src, dst)
 	if err != nil {
@@ -613,3 +730,94 @@ var diskUsage = withUser(func(w http.ResponseWriter, r *http.Request, d *data) (
 		Used:  usage.Used,
 	})
 })
+
+// applyMultiValuedTagsWithMetaflac uses metaflac to write tags to FLAC files.
+// Unlike ffmpeg, metaflac properly supports multiple values for the same tag.
+func applyMultiValuedTagsWithMetaflac(ctx context.Context, filepath string, tags map[string]string, multi map[string][]string, clear []string) error {
+	// Normalize tags and multi to proper Vorbis comment names
+	norm := normalizeAndMapToFFmpeg(tags)
+	mm := normalizeMultiToFFmpeg(multi)
+
+	// Build metaflac arguments
+	// First, remove tags that we're going to set (to avoid duplicates)
+	removeArgs := []string{}
+	for k := range norm {
+		upperK := strings.ToUpper(k)
+		removeArgs = append(removeArgs, "--remove-tag="+upperK)
+		// Also handle ALBUM_ARTIST vs "ALBUM ARTIST" variants
+		if upperK == "ALBUM_ARTIST" {
+			removeArgs = append(removeArgs, "--remove-tag=ALBUM ARTIST")
+			removeArgs = append(removeArgs, "--remove-tag=ALBUMARTIST")
+		}
+	}
+	for k := range mm {
+		removeArgs = append(removeArgs, "--remove-tag="+k)
+	}
+	// Also remove cleared tags
+	for _, c := range clear {
+		token := normalizeKey(c)
+		canonical, ok := canonicalMap[token]
+		if !ok {
+			if _, allowed := allowedCanonicals[c]; allowed {
+				canonical = c
+			} else {
+				continue
+			}
+		}
+		// Check multi-valued tags first
+		if ffk, ok := ffmpegMultiKey[canonical]; ok {
+			removeArgs = append(removeArgs, "--remove-tag="+ffk)
+		}
+		// Also check regular tags
+		if ffk, ok := ffmpegKey[canonical]; ok {
+			upperK := strings.ToUpper(ffk)
+			removeArgs = append(removeArgs, "--remove-tag="+upperK)
+			// Handle all variants of album artist
+			if canonical == "AlbumArtist" {
+				removeArgs = append(removeArgs, "--remove-tag=ALBUM ARTIST")
+				removeArgs = append(removeArgs, "--remove-tag=ALBUMARTIST")
+				removeArgs = append(removeArgs, "--remove-tag=ALBUM_ARTIST")
+			}
+			// Handle artist variants (lowercase/uppercase)
+			if canonical == "Artist" {
+				removeArgs = append(removeArgs, "--remove-tag=artist")
+				removeArgs = append(removeArgs, "--remove-tag=ARTIST")
+			}
+		}
+	}
+
+	// Execute removal first if needed
+	if len(removeArgs) > 0 {
+		removeArgs = append(removeArgs, filepath)
+		cmd := exec.CommandContext(ctx, "metaflac", removeArgs...)
+		if out, err := cmd.CombinedOutput(); err != nil {
+			return fmt.Errorf("metaflac remove failed: %w: %s", err, string(out))
+		}
+	}
+
+	// Now set the new tags
+	setArgs := []string{}
+	for k, v := range norm {
+		// Use uppercase for Vorbis comments (standard convention)
+		setArgs = append(setArgs, fmt.Sprintf("--set-tag=%s=%s", strings.ToUpper(k), v))
+	}
+	// Add multi-valued tags (each value as a separate --set-tag)
+	for k, vals := range mm {
+		for _, v := range vals {
+			setArgs = append(setArgs, fmt.Sprintf("--set-tag=%s=%s", k, v))
+		}
+	}
+
+	// If nothing to set, we're done (removal was already executed above)
+	if len(setArgs) == 0 {
+		return nil
+	}
+
+	setArgs = append(setArgs, filepath)
+	cmd := exec.CommandContext(ctx, "metaflac", setArgs...)
+	if out, err := cmd.CombinedOutput(); err != nil {
+		return fmt.Errorf("metaflac set failed: %w: %s", err, string(out))
+	}
+
+	return nil
+}