feat(subsonic): update track play stats on scrobble instead of stream

cmd/gonic/gonic.go

@@ -32,6 +32,7 @@ import (
 	"go.senan.xyz/gonic/podcasts"
 	"go.senan.xyz/gonic/scanner"
 	"go.senan.xyz/gonic/scanner/tags"
+	"go.senan.xyz/gonic/scrobble"
 	"go.senan.xyz/gonic/server/ctrladmin"
 	"go.senan.xyz/gonic/server/ctrlbase"
 	"go.senan.xyz/gonic/server/ctrlsubsonic"
@@ -235,7 +236,7 @@ func main() {
 		CacheCoverPath:  cacheDirCovers,
 		LastFMClient:    lastfmClient,
 		ArtistInfoCache: artistInfoCache,
-		Scrobblers: []ctrlsubsonic.Scrobbler{
+		Scrobblers: []scrobble.Scrobbler{
 			lastfmClient,
 			listenbrainzClient,
 		},

go.mod

@@ -5,6 +5,7 @@ go 1.21
 require (
 	github.com/Masterminds/sprig v2.22.0+incompatible
 	github.com/andybalholm/cascadia v1.3.2
+	github.com/davecgh/go-spew v1.1.1
 	github.com/dexterlb/mpvipc v0.0.0-20230829142118-145d6eabdc37
 	github.com/disintegration/imaging v1.6.2
 	github.com/dustin/go-humanize v1.0.1
@@ -40,7 +41,6 @@ require (
 	github.com/Masterminds/goutils v1.1.1 // indirect
 	github.com/Masterminds/semver v1.5.0 // indirect
 	github.com/PuerkitoBio/goquery v1.8.1 // indirect
-	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/felixge/httpsnoop v1.0.3 // indirect
 	github.com/go-openapi/jsonpointer v0.19.5 // indirect
 	github.com/go-openapi/swag v0.21.1 // indirect

lastfm/client.go

@@ -10,12 +10,11 @@ import (
 	"net/url"
 	"sort"
 	"strconv"
-	"strings"
 	"time"
 
 	"github.com/andybalholm/cascadia"
-	"github.com/google/uuid"
 	"go.senan.xyz/gonic/db"
+	"go.senan.xyz/gonic/scrobble"
 	"golang.org/x/net/html"
 )
 
@@ -185,11 +184,11 @@ func (c *Client) StealArtistImage(artistURL string) (string, error) {
 	return imageURL, nil
 }
 
-func (c *Client) IsUserAuthenticated(user *db.User) bool {
+func (c *Client) IsUserAuthenticated(user db.User) bool {
 	return user.LastFMSession != ""
 }
 
-func (c *Client) Scrobble(user *db.User, track *db.Track, stamp time.Time, submission bool) error {
+func (c *Client) Scrobble(user db.User, track scrobble.Track, stamp time.Time, submission bool) error {
 	apiKey, secret, err := c.keySecret()
 	if err != nil {
 		return fmt.Errorf("get key and secret: %w", err)
@@ -198,33 +197,27 @@ func (c *Client) Scrobble(user *db.User, track *db.Track, stamp time.Time, submi
 		return ErrNoUserSession
 	}
 
-	if track.Album == nil || len(track.Album.Artists) == 0 {
-		return fmt.Errorf("track has no album artists")
-	}
-
 	params := url.Values{}
 	if submission {
 		params.Add("method", "track.Scrobble")
-		// last.fm wants the timestamp in seconds
+		params.Add("timestamp", strconv.Itoa(int(stamp.Unix()))) // last.fm wants the timestamp in seconds
-		params.Add("timestamp", strconv.Itoa(int(stamp.Unix())))
 	} else {
 		params.Add("method", "track.updateNowPlaying")
 	}
 
-	params.Add("api_key", apiKey)
+	params.Add("artist", track.Artist)
-	params.Add("sk", user.LastFMSession)
+	params.Add("track", track.Track)
-	params.Add("artist", track.TagTrackArtist)
+	params.Add("trackNumber", strconv.Itoa(int(track.TrackNumber)))
-	params.Add("track", track.TagTitle)
+	params.Add("album", track.Album)
-	params.Add("trackNumber", strconv.Itoa(track.TagTrackNumber))
+	params.Add("albumArtist", track.AlbumArtist)
-	params.Add("album", track.Album.TagTitle)
+	params.Add("duration", strconv.Itoa(int(track.Duration.Seconds())))
-	params.Add("albumArtist", strings.Join(track.Album.ArtistsStrings(), ", "))
-	params.Add("duration", strconv.Itoa(track.Length))
 
-	// make sure we provide a valid uuid, since some users may have an incorrect mbid in their tags
+	if track.MusicBrainzID != "" {
-	if _, err := uuid.Parse(track.TagBrainzID); err == nil {
+		params.Add("mbid", track.MusicBrainzID)
-		params.Add("mbid", track.TagBrainzID)
 	}
 
+	params.Add("sk", user.LastFMSession)
+	params.Add("api_key", apiKey)
 	params.Add("api_sig", GetParamSignature(params, secret))
 
 	_, err = c.makeRequest(http.MethodPost, params)
@@ -236,7 +229,7 @@ func (c *Client) LoveTrack(user *db.User, track *db.Track) error {
 	if err != nil {
 		return fmt.Errorf("get key and secret: %w", err)
 	}
-	if !c.IsUserAuthenticated(user) {
+	if !c.IsUserAuthenticated(*user) {
 		return ErrNoUserSession
 	}
 
@@ -257,7 +250,7 @@ func (c *Client) GetCurrentUser(user *db.User) (User, error) {
 	if err != nil {
 		return User{}, fmt.Errorf("get key and secret: %w", err)
 	}
-	if !c.IsUserAuthenticated(user) {
+	if !c.IsUserAuthenticated(*user) {
 		return User{}, ErrNoUserSession
 	}
 
@@ -277,6 +270,7 @@ func (c *Client) GetCurrentUser(user *db.User) (User, error) {
 func (c *Client) makeRequest(method string, params url.Values) (LastFM, error) {
 	req, _ := http.NewRequest(method, BaseURL, nil)
 	req.URL.RawQuery = params.Encode()
+
 	resp, err := c.httpClient.Do(req)
 	if err != nil {
 		return LastFM{}, fmt.Errorf("get: %w", err)

lastfm/client_test.go

@@ -14,6 +14,7 @@ import (
 	"go.senan.xyz/gonic/db"
 	"go.senan.xyz/gonic/lastfm"
 	"go.senan.xyz/gonic/lastfm/mockclient"
+	"go.senan.xyz/gonic/scrobble"
 )
 
 func TestArtistGetInfo(t *testing.T) {
@@ -485,26 +486,6 @@ func TestGetSessionClientRequestFails(t *testing.T) {
 func TestScrobble(t *testing.T) {
 	t.Parallel()
 
-	user := &db.User{
-		LastFMSession: "lastFMSession1",
-	}
-
-	track := &db.Track{
-		Album: &db.Album{
-			TagTitle: "album1",
-			Artists: []*db.Artist{{
-				Name: "artist1",
-			}},
-		},
-		Length:         100,
-		TagBrainzID:    "916b242d-d439-4ae4-a439-556eef99c06e",
-		TagTitle:       "title1",
-		TagTrackArtist: "trackArtist1",
-		TagTrackNumber: 1,
-	}
-
-	stamp := time.Date(2023, 8, 12, 12, 34, 1, 200, time.UTC)
-
 	client := lastfm.NewClientCustom(
 		mockclient.New(t, func(w http.ResponseWriter, r *http.Request) {
 			require.Equal(t, http.MethodPost, r.Method)
@@ -534,6 +515,21 @@ func TestScrobble(t *testing.T) {
 		},
 	)
 
+	user := db.User{
+		LastFMSession: "lastFMSession1",
+	}
+	track := scrobble.Track{
+		Track:         "title1",
+		Artist:        "trackArtist1",
+		Album:         "album1",
+		AlbumArtist:   "artist1",
+		TrackNumber:   1,
+		Duration:      100 * time.Second,
+		MusicBrainzID: "916b242d-d439-4ae4-a439-556eef99c06e",
+	}
+
+	stamp := time.Date(2023, 8, 12, 12, 34, 1, 200, time.UTC)
+
 	err := client.Scrobble(user, track, stamp, true)
 	require.NoError(t, err)
 }
@@ -545,14 +541,14 @@ func TestScrobbleErrorsWithoutLastFMSession(t *testing.T) {
 		return "", "", nil
 	})
 
-	err := client.Scrobble(&db.User{}, &db.Track{}, time.Now(), false)
+	err := client.Scrobble(db.User{}, scrobble.Track{}, time.Now(), false)
 	require.ErrorIs(t, err, lastfm.ErrNoUserSession)
 }
 
 func TestScrobbleFailsWithoutLastFMAPIKey(t *testing.T) {
 	t.Parallel()
 
-	user := &db.User{
+	user := db.User{
 		LastFMSession: "lastFMSession1",
 	}
 
@@ -560,7 +556,7 @@ func TestScrobbleFailsWithoutLastFMAPIKey(t *testing.T) {
 		return "", "", fmt.Errorf("no keys")
 	})
 
-	err := scrobbler.Scrobble(user, &db.Track{}, time.Now(), false)
+	err := scrobbler.Scrobble(user, scrobble.Track{}, time.Now(), false)
 
 	require.Error(t, err)
 }

listenbrainz/listenbrainz.go

@@ -10,8 +10,8 @@ import (
 	"net/http/httputil"
 	"time"
 
-	"github.com/google/uuid"
 	"go.senan.xyz/gonic/db"
+	"go.senan.xyz/gonic/scrobble"
 )
 
 const (
@@ -36,27 +36,21 @@ func NewClientCustom(httpClient *http.Client) *Client {
 	return &Client{httpClient: httpClient}
 }
 
-func (c *Client) IsUserAuthenticated(user *db.User) bool {
+func (c *Client) IsUserAuthenticated(user db.User) bool {
 	return user.ListenBrainzURL != "" && user.ListenBrainzToken != ""
 }
 
-func (c *Client) Scrobble(user *db.User, track *db.Track, stamp time.Time, submission bool) error {
+func (c *Client) Scrobble(user db.User, track scrobble.Track, stamp time.Time, submission bool) error {
-	// make sure we provide a valid uuid, since some users may have an incorrect mbid in their tags
-	var trackMBID string
-	if _, err := uuid.Parse(track.TagBrainzID); err == nil {
-		trackMBID = track.TagBrainzID
-	}
-
 	payload := &Payload{
 		TrackMetadata: &TrackMetadata{
 			AdditionalInfo: &AdditionalInfo{
-				TrackNumber:   track.TagTrackNumber,
+				TrackNumber:   int(track.TrackNumber),
-				RecordingMBID: trackMBID,
+				RecordingMBID: track.MusicBrainzID,
-				TrackLength:   track.Length,
+				TrackLength:   int(track.Duration.Seconds()),
 			},
-			ArtistName:  track.TagTrackArtist,
+			ArtistName:  track.Artist,
-			TrackName:   track.TagTitle,
+			TrackName:   track.Track,
-			ReleaseName: track.Album.TagTitle,
+			ReleaseName: track.Album,
 		},
 	}
 	scrobble := Scrobble{

listenbrainz/listenbrainz_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"go.senan.xyz/gonic/db"
 	"go.senan.xyz/gonic/listenbrainz"
+	"go.senan.xyz/gonic/scrobble"
 )
 
 func TestScrobble(t *testing.T) {
@@ -35,8 +36,8 @@ func TestScrobble(t *testing.T) {
 	)
 
 	err := client.Scrobble(
-		&db.User{ListenBrainzURL: "https://listenbrainz.org", ListenBrainzToken: "token1"},
+		db.User{ListenBrainzURL: "https://listenbrainz.org", ListenBrainzToken: "token1"},
-		&db.Track{Album: &db.Album{TagTitle: "album"}, TagTitle: "title", TagTrackArtist: "artist", TagTrackNumber: 1},
+		scrobble.Track{Track: "title", Artist: "artist", Album: "album", TrackNumber: 1},
 		time.Unix(1683804525, 0),
 		true,
 	)
@@ -59,8 +60,8 @@ func TestScrobbleUnauthorized(t *testing.T) {
 	)
 
 	err := client.Scrobble(
-		&db.User{ListenBrainzURL: "https://listenbrainz.org", ListenBrainzToken: "token1"},
+		db.User{ListenBrainzURL: "https://listenbrainz.org", ListenBrainzToken: "token1"},
-		&db.Track{Album: &db.Album{TagTitle: "album"}, TagTitle: "title", TagTrackArtist: "artist", TagTrackNumber: 1},
+		scrobble.Track{Track: "title", Artist: "artist", Album: "album", TrackNumber: 1},
 		time.Now(),
 		true,
 	)
@@ -83,8 +84,8 @@ func TestScrobbleServerError(t *testing.T) {
 	)
 
 	err := client.Scrobble(
-		&db.User{ListenBrainzURL: "https://listenbrainz.org", ListenBrainzToken: "token1"},
+		db.User{ListenBrainzURL: "https://listenbrainz.org", ListenBrainzToken: "token1"},
-		&db.Track{Album: &db.Album{TagTitle: "album"}, TagTitle: "title", TagTrackArtist: "artist", TagTrackNumber: 1},
+		scrobble.Track{Track: "title", Artist: "artist", Album: "album", TrackNumber: 1},
 		time.Now(),
 		true,
 	)

scrobble/scrobble.go

@@ -0,0 +1,22 @@
+package scrobble
+
+import (
+	"time"
+
+	"go.senan.xyz/gonic/db"
+)
+
+type Track struct {
+	Track         string
+	Artist        string
+	Album         string
+	AlbumArtist   string
+	TrackNumber   uint
+	Duration      time.Duration
+	MusicBrainzID string
+}
+
+type Scrobbler interface {
+	IsUserAuthenticated(user db.User) bool
+	Scrobble(user db.User, track Track, stamp time.Time, submission bool) error
+}

server/ctrlsubsonic/ctrl.go

@@ -7,12 +7,11 @@ import (
 	"io"
 	"log"
 	"net/http"
-	"time"
 
-	"go.senan.xyz/gonic/db"
 	"go.senan.xyz/gonic/jukebox"
 	"go.senan.xyz/gonic/lastfm"
 	"go.senan.xyz/gonic/podcasts"
+	"go.senan.xyz/gonic/scrobble"
 	"go.senan.xyz/gonic/server/ctrlbase"
 	"go.senan.xyz/gonic/server/ctrlsubsonic/artistinfocache"
 	"go.senan.xyz/gonic/server/ctrlsubsonic/params"
@@ -40,11 +39,6 @@ func PathsOf(paths []MusicPath) []string {
 	return r
 }
 
-type Scrobbler interface {
-	IsUserAuthenticated(user *db.User) bool
-	Scrobble(user *db.User, track *db.Track, stamp time.Time, submission bool) error
-}
-
 type Controller struct {
 	*ctrlbase.Controller
 	MusicPaths      []MusicPath
@@ -52,7 +46,7 @@ type Controller struct {
 	CacheAudioPath  string
 	CacheCoverPath  string
 	Jukebox         *jukebox.Jukebox
-	Scrobblers      []Scrobbler
+	Scrobblers      []scrobble.Scrobbler
 	Podcasts        *podcasts.Podcasts
 	Transcoder      transcode.Transcoder
 	LastFMClient    *lastfm.Client

server/ctrlsubsonic/handlers_common.go

@@ -7,38 +7,22 @@ import (
 	"math"
 	"net/http"
 	"path/filepath"
+	"strings"
 	"time"
 	"unicode"
 
+	"github.com/google/uuid"
 	"github.com/jinzhu/gorm"
 
 	"go.senan.xyz/gonic/db"
 	"go.senan.xyz/gonic/scanner"
+	"go.senan.xyz/gonic/scrobble"
 	"go.senan.xyz/gonic/server/ctrlsubsonic/params"
 	"go.senan.xyz/gonic/server/ctrlsubsonic/spec"
 	"go.senan.xyz/gonic/server/ctrlsubsonic/specid"
 	"go.senan.xyz/gonic/server/ctrlsubsonic/specidpaths"
 )
 
-func lowerUDecOrHash(in string) string {
-	lower := unicode.ToLower(rune(in[0]))
-	if !unicode.IsLetter(lower) {
-		return "#"
-	}
-	return string(lower)
-}
-
-func getMusicFolder(musicPaths []MusicPath, p params.Params) string {
-	idx, err := p.GetInt("musicFolderId")
-	if err != nil {
-		return ""
-	}
-	if idx < 0 || idx >= len(musicPaths) {
-		return ""
-	}
-	return musicPaths[idx].Path
-}
-
 func (c *Controller) ServeGetLicence(_ *http.Request) *spec.Response {
 	sub := spec.NewResponse()
 	sub.Licence = &spec.Licence{
@@ -56,28 +40,60 @@ func (c *Controller) ServeScrobble(r *http.Request) *spec.Response {
 	params := r.Context().Value(CtxParams).(params.Params)
 
 	id, err := params.GetID("id")
-	if err != nil || id.Type != specid.Track {
+	if err != nil {
-		return spec.NewError(10, "please provide a track `id` track parameter")
+		return spec.NewError(10, "please provide a `id` parameter")
-	}
-
-	track := &db.Track{}
-	if err := c.DB.Preload("Album").Preload("Album.Artists").First(track, id.Value).Error; err != nil {
-		return spec.NewError(0, "error finding track: %v", err)
 	}
 
 	optStamp := params.GetOrTime("time", time.Now())
 	optSubmission := params.GetOrBool("submission", true)
 
-	if err := streamUpdateStats(c.DB, user.ID, track, optStamp); err != nil {
+	var scrobbleTrack scrobble.Track
-		return spec.NewError(0, "error updating stats: %v", err)
+
+	switch id.Type {
+	case specid.Track:
+		var track db.Track
+		if err := c.DB.Preload("Album").Preload("Album.Artists").First(&track, id.Value).Error; err != nil {
+			return spec.NewError(0, "error finding track: %v", err)
+		}
+		if track.Album == nil {
+			return spec.NewError(0, "track has no album %d", track.ID)
+		}
+
+		scrobbleTrack.Track = track.TagTitle
+		scrobbleTrack.Artist = track.TagTrackArtist
+		scrobbleTrack.Album = track.Album.TagTitle
+		scrobbleTrack.AlbumArtist = strings.Join(track.Album.ArtistsStrings(), ", ")
+		scrobbleTrack.TrackNumber = uint(track.TagTrackNumber)
+		scrobbleTrack.Duration = time.Second * time.Duration(track.Length)
+		if _, err := uuid.Parse(track.TagBrainzID); err == nil {
+			scrobbleTrack.MusicBrainzID = track.TagBrainzID
+		}
+
+		if err := scrobbleStatsUpdateTrack(c.DB, &track, user.ID, optStamp); err != nil {
+			return spec.NewError(0, "error updating stats: %v", err)
+		}
+
+	case specid.PodcastEpisode:
+		var podcastEpisode db.PodcastEpisode
+		if err := c.DB.Preload("Podcast").First(&podcastEpisode, id.Value).Error; err != nil {
+			return spec.NewError(0, "error finding podcast episode: %v", err)
+		}
+
+		scrobbleTrack.Track = podcastEpisode.Title
+		scrobbleTrack.Artist = podcastEpisode.Podcast.Title
+		scrobbleTrack.Duration = time.Second * time.Duration(podcastEpisode.Length)
+
+		if err := scrobbleStatsUpdatePodcastEpisode(c.DB, id.Value); err != nil {
+			return spec.NewError(0, "error updating stats: %v", err)
+		}
 	}
 
 	var scrobbleErrs []error
 	for _, scrobbler := range c.Scrobblers {
-		if !scrobbler.IsUserAuthenticated(user) {
+		if !scrobbler.IsUserAuthenticated(*user) {
 			continue
 		}
-		if err := scrobbler.Scrobble(user, track, optStamp, optSubmission); err != nil {
+		if err := scrobbler.Scrobble(*user, scrobbleTrack, optStamp, optSubmission); err != nil {
 			scrobbleErrs = append(scrobbleErrs, err)
 		}
 	}
@@ -426,3 +442,56 @@ func (c *Controller) ServeGetLyrics(_ *http.Request) *spec.Response {
 	sub.Lyrics = &spec.Lyrics{}
 	return sub
 }
+
+func scrobbleStatsUpdateTrack(dbc *db.DB, track *db.Track, userID int, playTime time.Time) error {
+	var play db.Play
+	if err := dbc.Where("album_id=? AND user_id=?", track.AlbumID, userID).First(&play).Error; err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
+		return fmt.Errorf("find stat: %w", err)
+	}
+
+	play.AlbumID = track.AlbumID
+	play.UserID = userID
+	play.Count++ // for getAlbumList?type=frequent
+	play.Length += track.Length
+	if playTime.After(play.Time) {
+		play.Time = playTime // for getAlbumList?type=recent
+	}
+
+	if err := dbc.Save(&play).Error; err != nil {
+		return fmt.Errorf("save stat: %w", err)
+	}
+	return nil
+}
+
+func scrobbleStatsUpdatePodcastEpisode(dbc *db.DB, peID int) error {
+	var pe db.PodcastEpisode
+	if err := dbc.Where("id=?", peID).First(&pe).Error; err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
+		return fmt.Errorf("find podcast episode: %w", err)
+	}
+
+	pe.ModifiedAt = time.Now()
+
+	if err := dbc.Save(&pe).Error; err != nil {
+		return fmt.Errorf("save podcast episode: %w", err)
+	}
+	return nil
+}
+
+func getMusicFolder(musicPaths []MusicPath, p params.Params) string {
+	idx, err := p.GetInt("musicFolderId")
+	if err != nil {
+		return ""
+	}
+	if idx < 0 || idx >= len(musicPaths) {
+		return ""
+	}
+	return musicPaths[idx].Path
+}
+
+func lowerUDecOrHash(in string) string {
+	lower := unicode.ToLower(rune(in[0]))
+	if !unicode.IsLetter(lower) {
+		return "#"
+	}
+	return string(lower)
+}

server/ctrlsubsonic/handlers_raw.go

@@ -62,48 +62,6 @@ func streamGetTranscodeMeta(dbc *db.DB, userID int, client string) spec.Transcod
 	}
 }
 
-func streamUpdateStats(dbc *db.DB, userID int, track *db.Track, playTime time.Time) error {
-	var play db.Play
-	err := dbc.
-		Where("album_id=? AND user_id=?", track.AlbumID, userID).
-		First(&play).
-		Error
-	if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
-		return fmt.Errorf("find stat: %w", err)
-	}
-
-	play.AlbumID = track.AlbumID
-	play.UserID = userID
-	play.Count++ // for getAlbumList?type=frequent
-	play.Length += track.Length
-	if playTime.After(play.Time) {
-		play.Time = playTime // for getAlbumList?type=recent
-	}
-
-	if err := dbc.Save(&play).Error; err != nil {
-		return fmt.Errorf("save stat: %w", err)
-	}
-	return nil
-}
-
-func streamUpdatePodcastEpisodeStats(dbc *db.DB, peID int) error {
-	var pe db.PodcastEpisode
-	err := dbc.
-		Where("id=?", peID).
-		First(&pe).
-		Error
-	if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
-		return fmt.Errorf("find podcast episode: %w", err)
-	}
-
-	pe.ModifiedAt = time.Now()
-
-	if err := dbc.Save(&pe).Error; err != nil {
-		return fmt.Errorf("save podcast episode: %w", err)
-	}
-	return nil
-}
-
 const (
 	coverDefaultSize = 600
 	coverCacheFormat = "png"
@@ -258,22 +216,6 @@ func (c *Controller) ServeStream(w http.ResponseWriter, r *http.Request) *spec.R
 		return spec.NewError(0, "type of id does not contain audio")
 	}
 
-	if track, ok := audioFile.(*db.Track); ok && track.Album != nil {
-		defer func() {
-			if err := streamUpdateStats(c.DB, user.ID, track, time.Now()); err != nil {
-				log.Printf("error updating track status: %v", err)
-			}
-		}()
-	}
-
-	if pe, ok := audioFile.(*db.PodcastEpisode); ok {
-		defer func() {
-			if err := streamUpdatePodcastEpisodeStats(c.DB, pe.ID); err != nil {
-				log.Printf("error updating podcast episode status: %v", err)
-			}
-		}()
-	}
-
 	maxBitRate, _ := params.GetInt("maxBitRate")
 	format, _ := params.Get("format")