Better implementation of Bookmarks, using its own table

db/migration/20200801101355_create_bookmark_table.go

@@ -0,0 +1,53 @@
+package migration
+
+import (
+	"database/sql"
+
+	"github.com/pressly/goose"
+)
+
+func init() {
+	goose.AddMigration(upCreateBookmarkTable, downCreateBookmarkTable)
+}
+
+func upCreateBookmarkTable(tx *sql.Tx) error {
+	_, err := tx.Exec(`
+create table bookmark
+(
+    user_id    varchar(255) not null
+        references user
+            on update cascade on delete cascade,
+    item_id    varchar(255) not null,
+    item_type  varchar(255) not null,
+    comment    varchar(255),
+    position   integer,
+    changed_by varchar(255),
+    created_at datetime,
+    updated_at datetime,
+    constraint bookmark_pk
+        unique (user_id, item_id, item_type)
+);
+
+create table playqueue_dg_tmp
+(
+	id varchar(255) not null,
+	user_id varchar(255) not null
+		references user
+			on update cascade on delete cascade,
+	current varchar(255),
+	position real,
+	changed_by varchar(255),
+	items varchar(255),
+	created_at datetime,
+	updated_at datetime
+);
+drop table playqueue;
+alter table playqueue_dg_tmp rename to playqueue;
+`)
+
+	return err
+}
+
+func downCreateBookmarkTable(tx *sql.Tx) error {
+	return nil
+}

engine/common.go

@@ -9,37 +9,37 @@ import (
 )
 
 type Entry struct {
-	Id          string
-	Title       string
-	IsDir       bool
-	Parent      string
-	Album       string
-	Year        int
-	Artist      string
-	Genre       string
-	CoverArt    string
-	Starred     time.Time
-	Track       int
-	Duration    int
-	Size        int64
-	Suffix      string
-	BitRate     int
-	ContentType string
-	Path        string
-	PlayCount   int32
-	DiscNumber  int
-	Created     time.Time
-	AlbumId     string
-	ArtistId    string
-	Type        string
-	UserRating  int
-	SongCount   int
-
-	UserName   string
-	MinutesAgo int
-	PlayerId   int
-	PlayerName string
-	AlbumCount int
+	Id               string
+	Title            string
+	IsDir            bool
+	Parent           string
+	Album            string
+	Year             int
+	Artist           string
+	Genre            string
+	CoverArt         string
+	Starred          time.Time
+	Track            int
+	Duration         int
+	Size             int64
+	Suffix           string
+	BitRate          int
+	ContentType      string
+	Path             string
+	PlayCount        int32
+	DiscNumber       int
+	Created          time.Time
+	AlbumId          string
+	ArtistId         string
+	Type             string
+	UserRating       int
+	SongCount        int
+	UserName         string
+	MinutesAgo       int
+	PlayerId         int
+	PlayerName       string
+	AlbumCount       int
+	BookmarkPosition int64
 }
 
 type Entries []Entry
@@ -112,6 +112,7 @@ func FromMediaFile(mf *model.MediaFile) Entry {
 		e.Starred = mf.StarredAt
 	}
 	e.UserRating = mf.Rating
+	e.BookmarkPosition = mf.BookmarkPosition
 	return e
 }
 

model/bookmark.go

@@ -0,0 +1,28 @@
+package model
+
+import "time"
+
+type Bookmarkable struct {
+	BookmarkPosition int64 `json:"bookmarkPosition"`
+}
+
+type BookmarkableRepository interface {
+	AddBookmark(id, comment string, position int64) error
+	DeleteBookmark(id string) error
+	GetBookmarks() (Bookmarks, error)
+}
+
+type Bookmark struct {
+	Item      MediaFile `json:"item"`
+	Comment   string    `json:"comment"`
+	Position  int64     `json:"position"`
+	ChangedBy string    `json:"changed_by"`
+	CreatedAt time.Time `json:"createdAt"`
+	UpdatedAt time.Time `json:"updatedAt"`
+}
+
+type Bookmarks []Bookmark
+
+// While I can't find a better way to make these fields optional in the models, I keep this list here
+// to be used in other packages
+var BookmarkFields = []string{"bookmarkPosition"}

model/mediafile.go

@@ -7,6 +7,7 @@ import (
 
 type MediaFile struct {
 	Annotations
+	Bookmarkable
 
 	ID                   string    `json:"id"            orm:"pk;column(id)"`
 	Path                 string    `json:"path"`
@@ -63,6 +64,7 @@ type MediaFileRepository interface {
 	DeleteByPath(path string) (int64, error)
 
 	AnnotatedRepository
+	BookmarkableRepository
 }
 
 func (mf MediaFile) GetAnnotations() Annotations {

model/playqueue.go

@@ -7,7 +7,6 @@ import (
 type PlayQueue struct {
 	ID        string     `json:"id"          orm:"column(id)"`
 	UserID    string     `json:"userId"      orm:"column(user_id)"`
-	Comment   string     `json:"comment"`
 	Current   string     `json:"current"`
 	Position  int64      `json:"position"`
 	ChangedBy string     `json:"changedBy"`
@@ -21,17 +20,4 @@ type PlayQueues []PlayQueue
 type PlayQueueRepository interface {
 	Store(queue *PlayQueue) error
 	Retrieve(userId string) (*PlayQueue, error)
-	AddBookmark(userId, id, comment string, position int64) error
-	GetBookmarks(userId string) (Bookmarks, error)
-	DeleteBookmark(userId, id string) error
 }
-
-type Bookmark struct {
-	Item      MediaFile `json:"item"`
-	Comment   string    `json:"comment"`
-	Position  int64     `json:"position"`
-	CreatedAt time.Time `json:"createdAt"`
-	UpdatedAt time.Time `json:"updatedAt"`
-}
-
-type Bookmarks []Bookmark

persistence/helpers.go

@@ -23,7 +23,9 @@ func toSqlArgs(rec interface{}) (map[string]interface{}, error) {
 	err = json.Unmarshal(b, &m)
 	r := make(map[string]interface{}, len(m))
 	for f, v := range m {
-		if !utils.StringInSlice(f, model.AnnotationFields) && v != nil {
+		isAnnotationField := utils.StringInSlice(f, model.AnnotationFields)
+		isBookmarkField := utils.StringInSlice(f, model.BookmarkFields)
+		if !isAnnotationField && !isBookmarkField && v != nil {
 			r[toSnakeCase(f)] = v
 		}
 	}

persistence/mediafile_repository.go

@@ -52,7 +52,8 @@ func (r mediaFileRepository) Put(m *model.MediaFile) error {
 }
 
 func (r mediaFileRepository) selectMediaFile(options ...model.QueryOptions) SelectBuilder {
-	return r.newSelectWithAnnotation("media_file.id", options...).Columns("media_file.*")
+	sql := r.newSelectWithAnnotation("media_file.id", options...).Columns("media_file.*")
+	return r.withBookmark(sql, "media_file.id")
 }
 
 func (r mediaFileRepository) Get(id string) (*model.MediaFile, error) {

persistence/persistence.go

@@ -137,6 +137,11 @@ func (s *SQLStore) GC(ctx context.Context) error {
 		log.Error(ctx, "Error removing orphan artist annotations", err)
 		return err
 	}
+	err = s.MediaFile(ctx).(*mediaFileRepository).cleanBookmarks()
+	if err != nil {
+		log.Error(ctx, "Error removing orphan bookmarks", err)
+		return err
+	}
 	err = s.Playlist(ctx).(*playlistRepository).removeOrphans()
 	if err != nil {
 		log.Error(ctx, "Error tidying up playlists", err)

persistence/playqueue_repository.go

@@ -9,7 +9,6 @@ import (
 	"github.com/astaxie/beego/orm"
 	"github.com/deluan/navidrome/log"
 	"github.com/deluan/navidrome/model"
-	"github.com/deluan/navidrome/model/request"
 )
 
 type playQueueRepository struct {
@@ -27,7 +26,6 @@ func NewPlayQueueRepository(ctx context.Context, o orm.Ormer) model.PlayQueueRep
 type playQueue struct {
 	ID        string `orm:"column(id)"`
 	UserID    string `orm:"column(user_id)"`
-	Comment   string
 	Current   string
 	Position  int64
 	ChangedBy string
@@ -64,79 +62,10 @@ func (r *playQueueRepository) Retrieve(userId string) (*model.PlayQueue, error)
 	return &pls, err
 }
 
-func (r *playQueueRepository) AddBookmark(userId, id, comment string, position int64) error {
-	u := loggedUser(r.ctx)
-	client, _ := request.ClientFrom(r.ctx)
-	bm := &playQueue{
-		UserID:    userId,
-		Comment:   comment,
-		Current:   id,
-		Position:  position,
-		ChangedBy: client,
-		CreatedAt: time.Now(),
-		UpdatedAt: time.Now(),
-	}
-
-	sel := r.newSelect().Column("*").Where(And{
-		Eq{"user_id": userId},
-		Eq{"items": ""},
-		Eq{"current": id},
-	})
-	var prev model.PlayQueue
-	err := r.queryOne(sel, &prev)
-	if err != nil && err != model.ErrNotFound {
-		log.Error(r.ctx, "Error retrieving previous bookmark", "user", u.UserName, err, "mediaFileId", id, err)
-		return err
-	}
-
-	// If there is a previous bookmark, override
-	if prev.ID != "" {
-		bm.ID = prev.ID
-		bm.CreatedAt = prev.CreatedAt
-	}
-
-	_, err = r.put(bm.ID, bm)
-	if err != nil {
-		log.Error(r.ctx, "Error saving bookmark", "user", u.UserName, err, "mediaFileId", id, err)
-		return err
-	}
-	return nil
-}
-
-func (r *playQueueRepository) GetBookmarks(userId string) (model.Bookmarks, error) {
-	u := loggedUser(r.ctx)
-	sel := r.newSelect().Column("*").Where(And{Eq{"user_id": userId}, Eq{"items": ""}})
-	var pqs model.PlayQueues
-	err := r.queryAll(sel, &pqs)
-	if err != nil {
-		log.Error(r.ctx, "Error retrieving bookmarks", "user", u.UserName, err)
-		return nil, err
-	}
-	bms := make(model.Bookmarks, len(pqs))
-	for i := range pqs {
-		items := r.loadTracks(model.MediaFiles{{ID: pqs[i].Current}})
-		bms[i].Item = items[0]
-		bms[i].Comment = pqs[i].Comment
-		bms[i].Position = pqs[i].Position
-		bms[i].CreatedAt = pqs[i].CreatedAt
-		bms[i].UpdatedAt = pqs[i].UpdatedAt
-	}
-	return bms, nil
-}
-
-func (r *playQueueRepository) DeleteBookmark(userId, id string) error {
-	return r.delete(And{
-		Eq{"user_id": userId},
-		Eq{"items": ""},
-		Eq{"current": id},
-	})
-}
-
 func (r *playQueueRepository) fromModel(q *model.PlayQueue) playQueue {
 	pq := playQueue{
 		ID:        q.ID,
 		UserID:    q.UserID,
-		Comment:   q.Comment,
 		Current:   q.Current,
 		Position:  q.Position,
 		ChangedBy: q.ChangedBy,
@@ -155,7 +84,6 @@ func (r *playQueueRepository) toModel(pq *playQueue) model.PlayQueue {
 	q := model.PlayQueue{
 		ID:        pq.ID,
 		UserID:    pq.UserID,
-		Comment:   pq.Comment,
 		Current:   pq.Current,
 		Position:  pq.Position,
 		ChangedBy: pq.ChangedBy,
@@ -219,7 +147,7 @@ func (r *playQueueRepository) loadTracks(tracks model.MediaFiles) model.MediaFil
 }
 
 func (r *playQueueRepository) clearPlayQueue(userId string) error {
-	return r.delete(And{Eq{"user_id": userId}, NotEq{"items": ""}})
+	return r.delete(Eq{"user_id": userId})
 }
 
 var _ model.PlayQueueRepository = (*playQueueRepository)(nil)