From c86cf1263836cbd92d8d10771933c482d5b401fe Mon Sep 17 00:00:00 2001 From: Felipe Martin <812088+fmartingr@users.noreply.github.com> Date: Sat, 15 Oct 2022 23:01:52 +0200 Subject: [PATCH] fix: remove createnewid usages (#520) * remove CreateNewID usages * remove CreateNewID implementations and definition * added missing sqlite envvar to compose file --- docker-compose.yaml | 1 + internal/cmd/add.go | 36 ++++++------- internal/cmd/import.go | 9 ---- internal/cmd/pocket.go | 9 ---- internal/database/database.go | 3 -- internal/database/database_test.go | 82 ++++++++++++++++++++++++++++++ internal/database/mysql.go | 14 ----- internal/database/pg.go | 13 ----- internal/database/sqlite.go | 21 -------- 9 files changed, 102 insertions(+), 86 deletions(-) diff --git a/docker-compose.yaml b/docker-compose.yaml index 657347cc1..b31a225c0 100644 --- a/docker-compose.yaml +++ b/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 diff --git a/internal/cmd/add.go b/internal/cmd/add.go index a044bb7fe..80ccab9e1 100644 --- a/internal/cmd/add.go +++ b/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 diff --git a/internal/cmd/import.go b/internal/cmd/import.go index ad55d31a8..a623b2b6f 100644 --- a/internal/cmd/import.go +++ b/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) }) diff --git a/internal/cmd/pocket.go b/internal/cmd/pocket.go index 794fa71e0..4471d2118 100644 --- a/internal/cmd/pocket.go +++ b/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) }) diff --git a/internal/database/database.go b/internal/database/database.go index 8221a4fdd..12e7bd25d 100644 --- a/internal/database/database.go +++ b/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 { diff --git a/internal/database/database_test.go b/internal/database/database_test.go index 4082ecd2c..1a7bba193 100644 --- a/internal/database/database_test.go +++ b/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() diff --git a/internal/database/mysql.go b/internal/database/mysql.go index c3581a48a..9f9119af9 100644 --- a/internal/database/mysql.go +++ b/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 -} diff --git a/internal/database/pg.go b/internal/database/pg.go index f04b7bb8e..b749d8443 100644 --- a/internal/database/pg.go +++ b/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 -} diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 9e4e0849e..f3198728f 100644 --- a/internal/database/sqlite.go +++ b/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 -}