Add native MP3 metadata writer and prefer it over ffmpeg for .mp3 files

http/resource.go

@@ -2,6 +2,7 @@ package fbhttp
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -10,9 +11,11 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"os/exec"
 	"path"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"github.com/shirou/gopsutil/v4/disk"
 	"github.com/spf13/afero"
@@ -54,6 +57,13 @@ var resourceGetHandler = withUser(func(w http.ResponseWriter, r *http.Request, d
 		file.Content = ""
 	}
 
+	// If metadata requested, attempt to read audio tags and attach them
+	if r.URL.Query().Get("metadata") == "1" {
+		if err := file.ReadAudioTags(); err != nil {
+			log.Printf("warning: failed reading audio tags for %s: %v", file.Path, err)
+		}
+	}
+
 	return renderJSON(w, r, file)
 })
 
@@ -201,14 +211,20 @@ func resourcePatchHandler(fileCache FileCache) handleFunc {
 			return http.StatusForbidden, nil
 		}
 
-		err = checkParent(src, dst)
-		if err != nil {
-			return http.StatusBadRequest, err
+		// Only check parent relationship when a destination was provided.
+		// Some actions (like metadata) don't provide a destination and calling
+		// filepath.Rel with an empty dst returns an error "can't make relative to ...".
+		if dst != "" {
+			err = checkParent(src, dst)
+			if err != nil {
+				return http.StatusBadRequest, err
+			}
 		}
 
 		override := r.URL.Query().Get("override") == "true"
 		rename := r.URL.Query().Get("rename") == "true"
-		if !override && !rename {
+		// Only check destination existence when a destination was provided.
+		if dst != "" && !override && !rename {
 			if _, err = d.user.Fs.Stat(dst); err == nil {
 				return http.StatusConflict, nil
 			}
@@ -222,6 +238,28 @@ func resourcePatchHandler(fileCache FileCache) handleFunc {
 			return http.StatusForbidden, nil
 		}
 
+		// Special-case metadata action because it needs the request body
+		if action == "metadata" {
+			if !d.user.Perm.Modify {
+				return http.StatusForbidden, nil
+			}
+
+			var tags map[string]string
+			body, err := io.ReadAll(r.Body)
+			if err != nil {
+				return http.StatusBadRequest, err
+			}
+			if err := json.Unmarshal(body, &tags); err != nil {
+				return http.StatusBadRequest, err
+			}
+
+			err = d.RunHook(func() error {
+				return applyMetadataWithFFmpeg(r.Context(), d, src, tags)
+			}, action, src, dst, d.user)
+
+			return errToStatus(err), err
+		}
+
 		err = d.RunHook(func() error {
 			return patchAction(r.Context(), action, src, dst, d, fileCache)
 		}, action, src, dst, d.user)
@@ -230,6 +268,103 @@ 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()).
+func applyMetadataWithFFmpeg(ctx context.Context, d *data, src string, tags map[string]string) error {
+	fi, err := files.NewFileInfo(&files.FileOptions{
+		Fs:         d.user.Fs,
+		Path:       src,
+		Modify:     d.user.Perm.Modify,
+		Expand:     false,
+		ReadHeader: false,
+		Checker:    d,
+	})
+	if err != nil {
+		return err
+	}
+
+	real := fi.RealPath()
+	// If RealPath returns the same virtual path, we cannot run native writers
+	// or ffmpeg on it.
+	if real == "" || real == fi.Path {
+		return fmt.Errorf("unable to obtain underlying real file path for %s: %w", fi.Path, fberrors.ErrInvalidRequestParams)
+	}
+
+	// If it's an MP3, try the native writer first to avoid ffmpeg and preserve
+	// existing tags accurately.
+	if strings.EqualFold(filepath.Ext(real), ".mp3") {
+		if err := files.applyMP3Tags(real, tags); err == nil {
+			return nil
+		} else {
+			// log and fall back to ffmpeg
+			log.Printf("applyMetadataWithFFmpeg: native mp3 writer failed: %v", err)
+		}
+	}
+
+	// Ensure ffmpeg is available
+	if _, err := exec.LookPath("ffmpeg"); err != nil {
+		return fmt.Errorf("ffmpeg not found: %w", err)
+	}
+
+	dir := filepath.Dir(real)
+
+	// Create a unique temporary filename in the same directory and keep the
+	// same extension as the original so ffmpeg can infer the output format.
+	ext := filepath.Ext(real)
+	tmp := filepath.Join(dir, fmt.Sprintf(".metadata_tmp_%d%s", time.Now().UnixNano(), ext))
+
+	// Ensure the temp file is removed on error
+	defer func() {
+		if _, statErr := os.Stat(tmp); statErr == nil {
+			_ = os.Remove(tmp)
+		}
+	}()
+
+	// Build metadata pairs but only include non-empty values so we don't
+	// overwrite existing tags with empty values coming from the client.
+	// Important: do NOT strip all metadata with -map_metadata -1 here —
+	// that would remove unmodified tags. Instead, keep existing metadata
+	// and only pass the -metadata pairs for the keys we want to change.
+	var metaPairs []string
+	for k, v := range tags {
+		if strings.TrimSpace(v) == "" {
+			continue
+		}
+		metaPairs = append(metaPairs, "-metadata", fmt.Sprintf("%s=%s", k, v))
+	}
+
+	// If there are no metadata pairs to set, there's nothing to do.
+	if len(metaPairs) == 0 {
+		return nil
+	}
+
+	args := []string{"-y", "-i", real, "-c", "copy"}
+	args = append(args, metaPairs...)
+	args = append(args, tmp)
+
+	cmd := exec.CommandContext(ctx, "ffmpeg", args...)
+	// Log the command (args) so we can debug unexpected metadata overwrites.
+	log.Printf("applyMetadataWithFFmpeg: ffmpeg %v", args)
+
+	// Capture combined output to provide actionable errors and log it for
+	// successful runs as well so we can inspect what ffmpeg actually wrote.
+	out, err := cmd.CombinedOutput()
+	if len(out) > 0 {
+		log.Printf("applyMetadataWithFFmpeg: ffmpeg output: %s", strings.TrimSpace(string(out)))
+	}
+	if err != nil {
+		return fmt.Errorf("ffmpeg error: %w: %s", err, strings.TrimSpace(string(out)))
+	}
+
+	// replace original
+	if err := os.Rename(tmp, real); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func checkParent(src, dst string) error {
 	rel, err := filepath.Rel(src, dst)
 	if err != nil {