fix: remove createnewid usages (#520)

* remove CreateNewID usages

* remove CreateNewID implementations and definition

* added missing sqlite envvar to compose file

docker-compose.yaml

@@ -17,6 +17,7 @@ services:
       - "mariadb"
     environment:
       SHIORI_DBMS: mysql
+      SHIORI_DIR: /srv/shiori
       SHIORI_PG_USER: shiori
       SHIORI_PG_PASS: shiori
       SHIORI_PG_NAME: shiori

internal/cmd/add.go

@@ -56,21 +56,28 @@ func addHandler(cmd *cobra.Command, args []string) {
 		book.Tags[i].Name = strings.TrimSpace(tag)
 	}
 
-	// Create bookmark ID
+	// Clean up bookmark URL
 	var err error
-	book.ID, err = db.CreateNewID(cmd.Context(), "bookmark")
+	book.URL, err = core.RemoveUTMParams(book.URL)
 	if err != nil {
-		cError.Printf("Failed to create ID: %v\n", err)
+		cError.Printf("Failed to clean URL: %v\n", err)
 		os.Exit(1)
 	}
 
-	// Clean up bookmark URL
-	book.URL, err = core.RemoveUTMParams(book.URL)
+	// Make sure bookmark's title not empty
+	if book.Title == "" {
+		book.Title = book.URL
+	}
+
+	// Save bookmark to database
+	books, err := db.SaveBookmarks(cmd.Context(), true, book)
 	if err != nil {
-		cError.Printf("Failed to clean URL: %v\n", err)
+		cError.Printf("Failed to save bookmark: %v\n", err)
 		os.Exit(1)
 	}
 
+	book = books[0]
+
 	// If it's not offline mode, fetch data from internet.
 	if !offline {
 		cInfo.Println("Downloading article...")
@@ -103,18 +110,13 @@ func addHandler(cmd *cobra.Command, args []string) {
 				os.Exit(1)
 			}
 		}
-	}
-
-	// Make sure bookmark's title not empty
-	if book.Title == "" {
-		book.Title = book.URL
-	}
 
-	// Save bookmark to database
-	_, err = db.SaveBookmarks(cmd.Context(), true, book)
-	if err != nil {
-		cError.Printf("Failed to save bookmark: %v\n", err)
-		os.Exit(1)
+		// Save bookmark to database
+		_, err = db.SaveBookmarks(cmd.Context(), false, book)
+		if err != nil {
+			cError.Printf("Failed to save bookmark with content: %v\n", err)
+			os.Exit(1)
+		}
 	}
 
 	// Print added bookmark

internal/cmd/import.go

@@ -41,13 +41,6 @@ func importHandler(cmd *cobra.Command, args []string) {
 		generateTag = submit == "y"
 	}
 
-	// Prepare bookmark's ID
-	bookID, err := db.CreateNewID(cmd.Context(), "bookmark")
-	if err != nil {
-		cError.Printf("Failed to create ID: %v\n", err)
-		os.Exit(1)
-	}
-
 	// Open bookmark's file
 	srcFile, err := os.Open(args[0])
 	if err != nil {
@@ -141,14 +134,12 @@ func importHandler(cmd *cobra.Command, args []string) {
 
 		// Add item to list
 		bookmark := model.Bookmark{
-			ID:       bookID,
 			URL:      url,
 			Title:    title,
 			Tags:     tags,
 			Modified: modifiedDate.Format(model.DatabaseDateFormat),
 		}
 
-		bookID++
 		mapURL[url] = struct{}{}
 		bookmarks = append(bookmarks, bookmark)
 	})

internal/cmd/pocket.go

@@ -25,13 +25,6 @@ func pocketCmd() *cobra.Command {
 }
 
 func pocketHandler(cmd *cobra.Command, args []string) {
-	// Prepare bookmark's ID
-	bookID, err := db.CreateNewID(cmd.Context(), "bookmark")
-	if err != nil {
-		cError.Printf("Failed to create ID: %v\n", err)
-		return
-	}
-
 	// Open pocket's file
 	srcFile, err := os.Open(args[0])
 	if err != nil {
@@ -99,14 +92,12 @@ func pocketHandler(cmd *cobra.Command, args []string) {
 
 		// Add item to list
 		bookmark := model.Bookmark{
-			ID:       bookID,
 			URL:      url,
 			Title:    title,
 			Modified: modified.Format(model.DatabaseDateFormat),
 			Tags:     tags,
 		}
 
-		bookID++
 		mapURL[url] = struct{}{}
 		bookmarks = append(bookmarks, bookmark)
 	})

internal/database/database.go

@@ -80,9 +80,6 @@ type DB interface {
 
 	// RenameTag change the name of a tag.
 	RenameTag(ctx context.Context, id int, newName string) error
-
-	// CreateNewID creates new id for specified table.
-	CreateNewID(ctx context.Context, table string) (int, error)
 }
 
 type dbbase struct {

internal/database/database_test.go

@@ -13,11 +13,14 @@ type testDatabaseFactory func(ctx context.Context) (DB, error)
 
 func testDatabase(t *testing.T, dbFactory testDatabaseFactory) {
 	tests := map[string]databaseTestCase{
+		"testBookmarkAutoIncrement":       testBookmarkAutoIncrement,
 		"testCreateBookmark":              testCreateBookmark,
+		"testCreateBookmarkWithContent":   testCreateBookmarkWithContent,
 		"testCreateBookmarkTwice":         testCreateBookmarkTwice,
 		"testCreateBookmarkWithTag":       testCreateBookmarkWithTag,
 		"testCreateTwoDifferentBookmarks": testCreateTwoDifferentBookmarks,
 		"testUpdateBookmark":              testUpdateBookmark,
+		"testUpdateBookmarkWithContent":   testUpdateBookmarkWithContent,
 		"testGetBookmark":                 testGetBookmark,
 		"testGetBookmarkNotExistant":      testGetBookmarkNotExistant,
 		"testGetBookmarks":                testGetBookmarks,
@@ -34,6 +37,28 @@ func testDatabase(t *testing.T, dbFactory testDatabaseFactory) {
 	}
 }
 
+func testBookmarkAutoIncrement(t *testing.T, db DB) {
+	ctx := context.TODO()
+
+	book := model.Bookmark{
+		URL:   "https://github.com/go-shiori/shiori",
+		Title: "shiori",
+	}
+
+	result, err := db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+	assert.Equal(t, 1, result[0].ID, "Saved bookmark must have ID %d", 1)
+
+	book = model.Bookmark{
+		URL:   "https://github.com/go-shiori/obelisk",
+		Title: "obelisk",
+	}
+
+	result, err = db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+	assert.Equal(t, 2, result[0].ID, "Saved bookmark must have ID %d", 2)
+}
+
 func testCreateBookmark(t *testing.T, db DB) {
 	ctx := context.TODO()
 
@@ -48,6 +73,31 @@ func testCreateBookmark(t *testing.T, db DB) {
 	assert.Equal(t, 1, result[0].ID, "Saved bookmark must have an ID set")
 }
 
+func testCreateBookmarkWithContent(t *testing.T, db DB) {
+	ctx := context.TODO()
+
+	book := model.Bookmark{
+		URL:     "https://github.com/go-shiori/obelisk",
+		Title:   "shiori",
+		Content: "Some content",
+		HTML:    "Some HTML content",
+	}
+
+	result, err := db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+
+	books, err := db.GetBookmarks(ctx, GetBookmarksOptions{
+		IDs:         []int{result[0].ID},
+		WithContent: true,
+	})
+	assert.NoError(t, err, "Get bookmarks must not fail")
+	assert.Len(t, books, 1)
+
+	assert.Equal(t, 1, books[0].ID, "Saved bookmark must have an ID set")
+	assert.Equal(t, book.Content, books[0].Content, "Saved bookmark must have content")
+	assert.Equal(t, book.HTML, books[0].HTML, "Saved bookmark must have HTML")
+}
+
 func testCreateBookmarkWithTag(t *testing.T, db DB) {
 	ctx := context.TODO()
 
@@ -126,6 +176,38 @@ func testUpdateBookmark(t *testing.T, db DB) {
 	assert.Equal(t, savedBookmark.ID, result[0].ID)
 }
 
+func testUpdateBookmarkWithContent(t *testing.T, db DB) {
+	ctx := context.TODO()
+
+	book := model.Bookmark{
+		URL:     "https://github.com/go-shiori/obelisk",
+		Title:   "shiori",
+		Content: "Some content",
+		HTML:    "Some HTML content",
+	}
+
+	result, err := db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+
+	updatedBook := result[0]
+	updatedBook.Content = "Some updated content"
+	updatedBook.HTML = "Some updated HTML content"
+
+	_, err = db.SaveBookmarks(ctx, false, updatedBook)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+
+	books, err := db.GetBookmarks(ctx, GetBookmarksOptions{
+		IDs:         []int{result[0].ID},
+		WithContent: true,
+	})
+	assert.NoError(t, err, "Get bookmarks must not fail")
+	assert.Len(t, books, 1)
+
+	assert.Equal(t, 1, books[0].ID, "Saved bookmark must have an ID set")
+	assert.Equal(t, updatedBook.Content, books[0].Content, "Saved bookmark must have updated content")
+	assert.Equal(t, updatedBook.HTML, books[0].HTML, "Saved bookmark must have updated HTML")
+}
+
 func testGetBookmark(t *testing.T, db DB) {
 	ctx := context.TODO()
 

internal/database/mysql.go

@@ -3,7 +3,6 @@ package database
 import (
 	"context"
 	"database/sql"
-	"fmt"
 	"strings"
 	"time"
 
@@ -624,16 +623,3 @@ func (db *MySQLDatabase) RenameTag(ctx context.Context, id int, newName string)
 
 	return errors.WithStack(err)
 }
-
-// CreateNewID creates new ID for specified table
-func (db *MySQLDatabase) CreateNewID(ctx context.Context, table string) (int, error) {
-	var tableID int
-	query := fmt.Sprintf(`SELECT IFNULL(MAX(id) + 1, 1) FROM %s`, table)
-
-	err := db.GetContext(ctx, &tableID, query)
-	if err != nil && err != sql.ErrNoRows {
-		return -1, errors.WithStack(err)
-	}
-
-	return tableID, nil
-}

internal/database/pg.go

@@ -634,16 +634,3 @@ func (db *PGDatabase) RenameTag(ctx context.Context, id int, newName string) err
 
 	return nil
 }
-
-// CreateNewID creates new ID for specified table
-func (db *PGDatabase) CreateNewID(ctx context.Context, table string) (int, error) {
-	var tableID int
-	query := fmt.Sprintf(`SELECT last_value from %s_id_seq;`, table)
-
-	err := db.GetContext(ctx, &tableID, query)
-	if err != nil && err != sql.ErrNoRows {
-		return -1, err
-	}
-
-	return tableID, nil
-}

internal/database/sqlite.go

@@ -3,7 +3,6 @@ package database
 import (
 	"context"
 	"database/sql"
-	"fmt"
 	"log"
 	"strings"
 	"time"
@@ -177,13 +176,6 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma
 				return errors.WithStack(err)
 			}
 
-			bookID, err := res.LastInsertId()
-			if err != nil {
-				return errors.WithStack(err)
-			}
-
-			book.ID = int(bookID)
-
 			if rows == 0 {
 				_, err = stmtInsertBookContent.ExecContext(ctx, book.ID, book.Title, book.Content, book.HTML)
 				if err != nil {
@@ -758,16 +750,3 @@ func (db *SQLiteDatabase) RenameTag(ctx context.Context, id int, newName string)
 
 	return nil
 }
-
-// CreateNewID creates new ID for specified table
-func (db *SQLiteDatabase) CreateNewID(ctx context.Context, table string) (int, error) {
-	var tableID int
-	query := fmt.Sprintf(`SELECT IFNULL(MAX(id) + 1, 1) FROM %s`, table)
-
-	err := db.GetContext(ctx, &tableID, query)
-	if err != nil && err != sql.ErrNoRows {
-		return -1, errors.WithStack(err)
-	}
-
-	return tableID, nil
-}