[server] Improved bookmarks

server/internal/api/v1/my/bookmarks.go

@@ -0,0 +1,80 @@
+package my
+
+import (
+	"strconv"
+	"time"
+
+	"github.com/gin-gonic/gin"
+	"github.com/google/uuid"
+	"github.com/shroff/phylum/server/internal/api/auth"
+	"github.com/shroff/phylum/server/internal/core/user"
+)
+
+type bookmarksResponse struct {
+	Bookmarks []user.Bookmark `json:"bookmarks"`
+	Until     int64           `json:"until"`
+}
+
+func setupBookmarksRoutes(r *gin.RouterGroup) {
+	group := r.Group("/bookmarks")
+	group.POST("/add/:id", handleBookmarksAddRoute)
+	group.POST("/remove/:id", handleBookmarksRemoveRoute)
+	group.GET("/list", handleBookmarksGetRoute)
+}
+
+func handleBookmarksGetRoute(c *gin.Context) {
+	var since *time.Time
+	sinceStr := c.Query("since")
+	if sinceStr != "" {
+		sinceInt, err := strconv.ParseInt(sinceStr, 10, 64)
+		if err != nil {
+			panic(err)
+		}
+		t := time.UnixMilli(sinceInt - 1000).UTC()
+		since = &t
+	}
+	u := auth.GetUser(c)
+	bookmarks, err := user.CreateManager(c.Request.Context()).ListBookmarks(u, since)
+	if err != nil {
+		panic(err)
+	}
+	c.JSON(200, bookmarksResponse{
+		Bookmarks: bookmarks,
+		Until:     time.Now().UnixMilli(),
+	})
+}
+
+func handleBookmarksAddRoute(c *gin.Context) {
+	idStr, ok := c.Params.Get("id")
+	name := c.Query("name")
+	if !ok {
+		panic(errInvalidParams)
+	}
+	id := uuid.MustParse(idStr)
+
+	u := auth.GetUser(c)
+	r, err := auth.GetFileSystem(c).ResourceByID(id)
+	if err != nil {
+		panic(err)
+	}
+	err = user.CreateManager(c.Request.Context()).AddBookmark(u, r, name)
+	if err != nil {
+		panic(err)
+	}
+	c.JSON(200, gin.H{})
+}
+
+func handleBookmarksRemoveRoute(c *gin.Context) {
+	idStr, ok := c.Params.Get("id")
+	if !ok {
+		panic(errInvalidParams)
+	}
+	id := uuid.MustParse(idStr)
+
+	u := auth.GetUser(c)
+	err := user.CreateManager(c.Request.Context()).RemoveBookmark(u, id)
+	if err != nil {
+		panic(err)
+	}
+	c.JSON(200, gin.H{})
+}

server/internal/api/v1/my/routes.go

@@ -4,7 +4,6 @@ import (
 	"net/http"
 
 	"github.com/gin-gonic/gin"
-	"github.com/google/uuid"
 	"github.com/shroff/phylum/server/internal/api/auth"
 	fsapi "github.com/shroff/phylum/server/internal/api/v1/fs"
 	"github.com/shroff/phylum/server/internal/core/errors"
@@ -23,20 +22,11 @@ type sharedResponse struct {
 	Shared []fs.ResourceInfo `json:"shared"`
 }
 
-type bookmarksResponse struct {
-	Bookmarks []fs.ResourceInfo `json:"bookmarks"`
-}
-type bookmarkIDParams struct {
-	ID uuid.UUID `json:"id" binding:"required"`
-}
-
 func SetupRoutes(r *gin.RouterGroup) {
 	group := r.Group("/my")
 	group.GET("/home", handleHomeRoute)
 	group.GET("/shared", handleSharedRoute)
-	group.GET("/bookmarks/list", handleBookmarksGetRoute)
-	group.POST("/bookmarks/add", handleBookmarksAddRoute)
-	group.POST("/bookmarks/remove", handleBookmarksRemoveRoute)
+	setupBookmarksRoutes(group)
 }
 
 func handleHomeRoute(c *gin.Context) {
@@ -61,46 +51,3 @@ func handleSharedRoute(c *gin.Context) {
 	}
 	c.JSON(200, sharedResponse{Shared: shared})
 }
-
-func handleBookmarksGetRoute(c *gin.Context) {
-	u := auth.GetUser(c)
-	bookmarks, err := user.CreateManager(c.Request.Context()).Bookmarks(u)
-	if err != nil {
-		panic(err)
-	}
-	c.JSON(200, bookmarksResponse{Bookmarks: bookmarks})
-}
-
-func handleBookmarksAddRoute(c *gin.Context) {
-	var params bookmarkIDParams
-	err := c.ShouldBindJSON(&params)
-	if err != nil || params.ID == uuid.Nil {
-		panic(errInvalidParams)
-	}
-
-	u := auth.GetUser(c)
-	r, err := auth.GetFileSystem(c).ResourceByID(params.ID)
-	if err != nil {
-		panic(err)
-	}
-	err = user.CreateManager(c.Request.Context()).AddBookmark(u, r)
-	if err != nil {
-		panic(err)
-	}
-	c.JSON(200, gin.H{})
-}
-
-func handleBookmarksRemoveRoute(c *gin.Context) {
-	var params bookmarkIDParams
-	err := c.ShouldBindJSON(&params)
-	if err != nil || params.ID == uuid.Nil {
-		panic(errInvalidParams)
-	}
-
-	u := auth.GetUser(c)
-	err = user.CreateManager(c.Request.Context()).RemoveBookmark(u, params.ID)
-	if err != nil {
-		panic(err)
-	}
-	c.JSON(200, gin.H{})
-}

server/internal/command/user/bookmarks.go

@@ -32,12 +32,12 @@ func setupBookmarksListCommand() *cobra.Command {
 		Short: "List Bookmarks",
 		Args:  cobra.ExactArgs(0),
 		Run: func(cmd *cobra.Command, args []string) {
-			if bookmarks, err := user.CreateManager(context.Background()).Bookmarks(common.User(cmd)); err != nil {
+			if bookmarks, err := user.CreateManager(context.Background()).ListBookmarks(common.User(cmd), nil); err != nil {
 				fmt.Println("unable to list bookmark: " + err.Error())
 				os.Exit(1)
 			} else {
 				for _, b := range bookmarks {
-					fmt.Printf("%s %s %s\n", b.ID().String(), b.Name(), b.PermissionsString())
+					fmt.Printf("%s %s %t\n", b.ResourceID.String(), b.Name, b.Deleted == nil)
 				}
 			}
 		},
@@ -68,17 +68,21 @@ func setupBookmarksRemoveCommand() *cobra.Command {
 
 func setupBookmarksAddCommand() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "add [<path>|<uuid>]",
+		Use:   "add (path | uuid) [name]",
 		Short: "Add Bookmark",
-		Args:  cobra.ExactArgs(1),
+		Args:  cobra.RangeArgs(1, 2),
 		Run: func(cmd *cobra.Command, args []string) {
 			r, err := common.UserFileSystem(cmd).ResourceByPathOrUUID(args[0])
 			if err != nil {
 				fmt.Println("unable to add bookmark: " + err.Error())
 				os.Exit(1)
 			}
+			name := ""
+			if len(args) == 2 {
+				name = args[1]
+			}
 
-			if err := user.CreateManager(context.Background()).AddBookmark(common.User(cmd), r); err != nil {
+			if err := user.CreateManager(context.Background()).AddBookmark(common.User(cmd), r, name); err != nil {
 				fmt.Println("unable to add bookmark: " + err.Error())
 				os.Exit(1)
 			}

server/internal/core/db/bookmarks.sql.go

@@ -0,0 +1,90 @@
+// Code generated by sqlc. DO NOT EDIT.
+// versions:
+//   sqlc v1.26.0
+// source: bookmarks.sql
+
+package db
+
+import (
+	"context"
+
+	"github.com/google/uuid"
+	"github.com/jackc/pgx/v5/pgtype"
+)
+
+const addBookmark = `-- name: AddBookmark :exec
+INSERT INTO bookmarks(
+    username,
+    resource_id,
+    name
+) VALUES (
+    $1::TEXT,
+    $2::UUID,
+    $3::TEXT
+) ON CONFLICT(username, resource_id) DO UPDATE SET
+    created = CASE WHEN bookmarks.deleted IS NULL THEN CURRENT_TIMESTAMP ELSE bookmarks.created END,
+    modified = CURRENT_TIMESTAMP,
+    deleted = NULL,
+    name = $3::TEXT
+`
+
+type AddBookmarkParams struct {
+	Username   string
+	ResourceID uuid.UUID
+	Name       string
+}
+
+func (q *Queries) AddBookmark(ctx context.Context, arg AddBookmarkParams) error {
+	_, err := q.db.Exec(ctx, addBookmark, arg.Username, arg.ResourceID, arg.Name)
+	return err
+}
+
+const deleteBookmarkByUsernameResourceID = `-- name: DeleteBookmarkByUsernameResourceID :exec
+DELETE FROM bookmarks WHERE username = $1::TEXT AND resource_id = $2::UUID
+`
+
+type DeleteBookmarkByUsernameResourceIDParams struct {
+	Username   string
+	ResourceID uuid.UUID
+}
+
+func (q *Queries) DeleteBookmarkByUsernameResourceID(ctx context.Context, arg DeleteBookmarkByUsernameResourceIDParams) error {
+	_, err := q.db.Exec(ctx, deleteBookmarkByUsernameResourceID, arg.Username, arg.ResourceID)
+	return err
+}
+
+const listBookmarks = `-- name: ListBookmarks :many
+SELECT username, resource_id, name, created, modified, deleted FROM bookmarks b WHERE username = $1::TEXT AND modified > $2::TIMESTAMP
+`
+
+type ListBookmarksParams struct {
+	Username string
+	Since    pgtype.Timestamp
+}
+
+func (q *Queries) ListBookmarks(ctx context.Context, arg ListBookmarksParams) ([]Bookmark, error) {
+	rows, err := q.db.Query(ctx, listBookmarks, arg.Username, arg.Since)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+	var items []Bookmark
+	for rows.Next() {
+		var i Bookmark
+		if err := rows.Scan(
+			&i.Username,
+			&i.ResourceID,
+			&i.Name,
+			&i.Created,
+			&i.Modified,
+			&i.Deleted,
+		); err != nil {
+			return nil, err
+		}
+		items = append(items, i)
+	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
+	return items, nil
+}

server/internal/core/db/db_handler.go

@@ -42,6 +42,7 @@ func Get() *DbHandler {
 func Close() {
 	if d != nil {
 		d.pool.Close()
+		d = nil
 	}
 }
 

server/internal/core/db/migrations/data/002_users.sql

@@ -2,12 +2,11 @@ CREATE TABLE users(
   username      TEXT        PRIMARY KEY,
   created       TIMESTAMP   NOT NULL DEFAULT CURRENT_TIMESTAMP,
   modified      TIMESTAMP   NOT NULL DEFAULT CURRENT_TIMESTAMP,
+  deleted       TIMESTAMP,
   display_name  TEXT        NOT NULL,
   password_hash TEXT        NOT NULL,
-  deleted       TIMESTAMP,
   root          uuid        NOT NULL REFERENCES resources(id),
-  home          uuid        NOT NULL REFERENCES resources(id),
-  bookmarks     JSONB       NOT NULL DEFAULT '[]'
+  home          uuid        NOT NULL REFERENCES resources(id)
 );
 
 ---- create above / drop below ----

server/internal/core/db/migrations/data/009_bookmarks.sql

@@ -0,0 +1,17 @@
+CREATE TABLE bookmarks(
+    username        TEXT        NOT NULL REFERENCES users(username),
+    resource_id     UUID        NOT NULL REFERENCES resources(id),
+    name            TEXT        NOT NULL,
+    created         TIMESTAMP   NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    modified        TIMESTAMP   NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    deleted         TIMESTAMP,
+    PRIMARY KEY(username, resource_id)
+);
+
+CREATE INDEX modified_bookmarks ON bookmarks(username, modified);
+
+---- create above / drop below ----
+
+DROP INDEX modified_bookmarks;
+
+DROP TABLE bookmarks;

server/internal/core/db/models.go

@@ -16,6 +16,15 @@ type AccessToken struct {
 	Username string
 }
 
+type Bookmark struct {
+	Username   string
+	ResourceID uuid.UUID
+	Name       string
+	Created    pgtype.Timestamp
+	Modified   pgtype.Timestamp
+	Deleted    pgtype.Timestamp
+}
+
 type Publink struct {
 	ID           int32
 	Name         string
@@ -68,10 +77,9 @@ type User struct {
 	Username     string
 	Created      pgtype.Timestamp
 	Modified     pgtype.Timestamp
+	Deleted      pgtype.Timestamp
 	DisplayName  string
 	PasswordHash string
-	Deleted      pgtype.Timestamp
 	Root         uuid.UUID
 	Home         uuid.UUID
-	Bookmarks    []byte
 }

server/internal/core/db/user.sql.go

@@ -11,24 +11,6 @@ import (
 	"github.com/google/uuid"
 )
 
-const addBookmark = `-- name: AddBookmark :exec
-UPDATE users
-SET
-    bookmarks = CASE WHEN bookmarks ? ($1::uuid)::text THEN bookmarks ELSE bookmarks || to_jsonb(array[$1::uuid]) END,
-    modified = NOW()
-WHERE username = $2::text
-`
-
-type AddBookmarkParams struct {
-	ID       uuid.UUID
-	Username string
-}
-
-func (q *Queries) AddBookmark(ctx context.Context, arg AddBookmarkParams) error {
-	_, err := q.db.Exec(ctx, addBookmark, arg.ID, arg.Username)
-	return err
-}
-
 const allUsers = `-- name: AllUsers :many
 SELECT username, display_name, root, home from users WHERE deleted IS NULL
 `
@@ -70,7 +52,7 @@ INSERT INTO users(
     username, display_name, password_hash, root, home
 ) VALUES (
     $1, $2, $3, $4, $5
-) RETURNING username, created, modified, display_name, password_hash, deleted, root, home, bookmarks
+) RETURNING username, created, modified, deleted, display_name, password_hash, root, home
 `
 
 type CreateUserParams struct {
@@ -94,77 +76,15 @@ func (q *Queries) CreateUser(ctx context.Context, arg CreateUserParams) (User, e
 		&i.Username,
 		&i.Created,
 		&i.Modified,
+		&i.Deleted,
 		&i.DisplayName,
 		&i.PasswordHash,
-		&i.Deleted,
 		&i.Root,
 		&i.Home,
-		&i.Bookmarks,
 	)
 	return i, err
 }
 
-const listBookmarks = `-- name: ListBookmarks :many
-WITH ids(id) AS(
-  SELECT jsonb_array_elements_text(bookmarks)::uuid
-  FROM users
-  WHERE username = $1::text
-) SELECT r.id, r.permissions, r.name, r.parent, r.dir, r.created, r.modified, r.deleted, r.content_size, r.content_type, r.content_sha256
-FROM ids i
-JOIN resources r
-ON i.id = r.id
-`
-
-func (q *Queries) ListBookmarks(ctx context.Context, username string) ([]Resource, error) {
-	rows, err := q.db.Query(ctx, listBookmarks, username)
-	if err != nil {
-		return nil, err
-	}
-	defer rows.Close()
-	var items []Resource
-	for rows.Next() {
-		var i Resource
-		if err := rows.Scan(
-			&i.ID,
-			&i.Permissions,
-			&i.Name,
-			&i.Parent,
-			&i.Dir,
-			&i.Created,
-			&i.Modified,
-			&i.Deleted,
-			&i.ContentSize,
-			&i.ContentType,
-			&i.ContentSha256,
-		); err != nil {
-			return nil, err
-		}
-		items = append(items, i)
-	}
-	if err := rows.Err(); err != nil {
-		return nil, err
-	}
-	return items, nil
-}
-
-const removeBookmark = `-- name: RemoveBookmark :exec
-UPDATE users
-SET
-    bookmarks = bookmarks - ($1::uuid)::text,
-    modified = NOW()
-WHERE username = $2::text
-`
-
-type RemoveBookmarkParams struct {
-	ID       uuid.UUID
-	Username string
-}
-
-func (q *Queries) RemoveBookmark(ctx context.Context, arg RemoveBookmarkParams) error {
-	_, err := q.db.Exec(ctx, removeBookmark, arg.ID, arg.Username)
-	return err
-}
-
 const updateUserDisplayName = `-- name: UpdateUserDisplayName :exec
 UPDATE users
 SET

server/internal/core/user/bookmarks.go

@@ -0,0 +1,62 @@
+package user
+
+import (
+	"time"
+
+	"github.com/google/uuid"
+	"github.com/jackc/pgx/v5/pgtype"
+	"github.com/shroff/phylum/server/internal/core/db"
+	"github.com/shroff/phylum/server/internal/core/fs"
+)
+
+type Bookmark struct {
+	ResourceID uuid.UUID  `json:"resource_id"`
+	Name       string     `json:"name"`
+	Deleted    *time.Time `json:"deleted"`
+}
+
+func (m manager) AddBookmark(u User, resource fs.Resource, name string) error {
+	if name == "" {
+		name = resource.Name()
+	}
+	if name == "" || fs.CheckNameInvalid(name) {
+		return fs.ErrResourceNameInvalid
+	}
+	return m.db.Queries.AddBookmark(m.ctx, db.AddBookmarkParams{
+		Username:   u.Username,
+		ResourceID: resource.ID(),
+		Name:       name,
+	})
+}
+
+func (m manager) RemoveBookmark(u User, id uuid.UUID) error {
+	return m.db.Queries.DeleteBookmarkByUsernameResourceID(m.ctx, db.DeleteBookmarkByUsernameResourceIDParams{Username: u.Username, ResourceID: id})
+}
+
+func (m manager) ListBookmarks(u User, since *time.Time) ([]Bookmark, error) {
+	s := pgtype.Timestamp{
+		Valid: true,
+	}
+	if since != nil {
+		s.Time = *since
+	}
+	// TODO: #permissions This doesn't take permissions into account. is this okay?
+	res, err := m.db.Queries.ListBookmarks(m.ctx, db.ListBookmarksParams{Username: u.Username, Since: s})
+	if err != nil {
+		return nil, err
+	}
+
+	result := make([]Bookmark, len(res))
+	for i, r := range res {
+		var deleted *time.Time
+		if r.Deleted.Valid {
+			deleted = &r.Deleted.Time
+		}
+		result[i] = Bookmark{
+			ResourceID: r.ResourceID,
+			Name:       r.Name,
+			Deleted:    deleted,
+		}
+	}
+	return result, err
+}

server/internal/core/user/shared.go

@@ -0,0 +1,20 @@
+package user
+
+import (
+	"github.com/shroff/phylum/server/internal/core/db"
+	"github.com/shroff/phylum/server/internal/core/fs"
+)
+
+func (m manager) SharedResources(u User) ([]fs.ResourceInfo, error) {
+	// TODO: #permissions This doesn't take permissions into account. is this okay?
+	res, err := m.db.Queries.SharedResources(m.ctx, db.SharedResourcesParams{Username: u.Username, UserHome: u.Home})
+	if err != nil {
+		return nil, err
+	}
+
+	result := make([]fs.ResourceInfo, len(res))
+	for i, r := range res {
+		result[i] = fs.ResourceInfoFromDBResource(r)
+	}
+	return result, err
+}

server/internal/core/user/user.go

@@ -2,6 +2,7 @@ package user
 
 import (
 	"context"
+	"time"
 
 	"github.com/google/uuid"
 	"github.com/shroff/phylum/server/internal/core/db"
@@ -40,9 +41,11 @@ type Manager interface {
 	CreateAccessToken(username string) (string, error)
 	ReadAccessToken(accessToken string) (User, error)
 
-	// user_lists.go
-	SharedResources(u User) (result []fs.ResourceInfo, err error)
-	Bookmarks(u User) ([]fs.ResourceInfo, error)
-	AddBookmark(u User, resource fs.Resource) error
+	// bookmarks.go
+	AddBookmark(u User, resource fs.Resource, name string) error
 	RemoveBookmark(u User, id uuid.UUID) error
+	ListBookmarks(u User, since *time.Time) ([]Bookmark, error)
+
+	// shared.go
+	SharedResources(u User) (result []fs.ResourceInfo, err error)
 }

server/internal/core/user/user_lists.go

@@ -1,43 +0,0 @@
-package user
-
-import (
-	"github.com/google/uuid"
-	"github.com/shroff/phylum/server/internal/core/db"
-	"github.com/shroff/phylum/server/internal/core/fs"
-)
-
-func (m manager) SharedResources(u User) ([]fs.ResourceInfo, error) {
-	// TODO: #permissions This doesn't take permissions into account. is this okay?
-	res, err := m.db.Queries.SharedResources(m.ctx, db.SharedResourcesParams{Username: u.Username, UserHome: u.Home})
-	if err != nil {
-		return nil, err
-	}
-
-	result := make([]fs.ResourceInfo, len(res))
-	for i, r := range res {
-		result[i] = fs.ResourceInfoFromDBResource(r)
-	}
-	return result, err
-}
-
-func (m manager) AddBookmark(u User, resource fs.Resource) error {
-	return m.db.Queries.AddBookmark(m.ctx, db.AddBookmarkParams{Username: u.Username, ID: resource.ID()})
-}
-
-func (m manager) RemoveBookmark(u User, id uuid.UUID) error {
-	return m.db.Queries.RemoveBookmark(m.ctx, db.RemoveBookmarkParams{Username: u.Username, ID: id})
-}
-
-func (m manager) Bookmarks(u User) ([]fs.ResourceInfo, error) {
-	// TODO: #permissions This doesn't take permissions into account. is this okay?
-	res, err := m.db.Queries.ListBookmarks(m.ctx, u.Username)
-	if err != nil {
-		return nil, err
-	}
-
-	result := make([]fs.ResourceInfo, len(res))
-	for i, r := range res {
-		result[i] = fs.ResourceInfoFromDBResource(r)
-	}
-	return result, err
-}

server/sql/queries/bookmarks.sql

@@ -0,0 +1,20 @@
+-- name: AddBookmark :exec
+INSERT INTO bookmarks(
+    username,
+    resource_id,
+    name
+) VALUES (
+    @username::TEXT,
+    @resource_id::UUID,
+    @name::TEXT
+) ON CONFLICT(username, resource_id) DO UPDATE SET
+    created = CASE WHEN bookmarks.deleted IS NULL THEN CURRENT_TIMESTAMP ELSE bookmarks.created END,
+    modified = CURRENT_TIMESTAMP,
+    deleted = NULL,
+    name = @name::TEXT;
+
+-- name: DeleteBookmarkByUsernameResourceID :exec
+DELETE FROM bookmarks WHERE username = @username::TEXT AND resource_id = @resource_id::UUID;
+
+-- name: ListBookmarks :many
+SELECT * FROM bookmarks b WHERE username = @username::TEXT AND modified > @since::TIMESTAMP;

server/sql/queries/user.sql

@@ -38,27 +38,3 @@ SET
     home = $1,
     modified = NOW()
 WHERE username = $2;
-
--- name: ListBookmarks :many
-WITH ids(id) AS(
-  SELECT jsonb_array_elements_text(bookmarks)::uuid
-  FROM users
-  WHERE username = @username::text
-) SELECT r.*
-FROM ids i
-JOIN resources r
-ON i.id = r.id;
-
--- name: AddBookmark :exec
-UPDATE users
-SET
-    bookmarks = CASE WHEN bookmarks ? (@id::uuid)::text THEN bookmarks ELSE bookmarks || to_jsonb(array[@id::uuid]) END,
-    modified = NOW()
-WHERE username = @username::text;
-
--- name: RemoveBookmark :exec
-UPDATE users
-SET
-    bookmarks = bookmarks - (@id::uuid)::text,
-    modified = NOW()
-WHERE username = @username::text;