fix(psql): save bookmarks not using passed bookmark id for the insert (#484)

* fix(psql): get last inserted id from insert query

book.ID was not being used, so inserts were failing.
the check for book.ID was removed and it is filled with the returning
id from the insert query

* test(psql): added save bookmarks simple test

* ci: added postgresql service

* fix(typo): QueryRow -> QueryRowContext

* ci: explicit postgresql port

* ci(test): 1.19 only

* ci: bind psql to localhost

* test(pg): migrate before test

* test(pg): migrate database before test

* fix(pg): check no rows error on get query

.github/workflows/_test.yml

@@ -8,6 +8,19 @@ jobs:
     strategy:
       matrix:
         go: [1.19]
+    services:
+      postgres:
+        image: postgres
+        env:
+          POSTGRES_PASSWORD: shiori
+          POSTGRES_USER: shiori
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+        ports:
+          - 5432:5432
     name: Go ${{ matrix.go }} unit tests
     steps:
       - uses: actions/checkout@v2
@@ -25,4 +38,6 @@ jobs:
             golangci-lint.cache-{interval_number}-
             golangci-lint.cache-
       - run: go test ./...
+        env:
+          SHIORI_TEST_PG_URL: "postgres://shiori:shiori@localhost:5432/shiori?sslmode=disable"
       - run: CGO_ENABLED=0 go build -tags osusergo,netgo -ldflags="-s -w -X main.version=$(git describe --tags) -X main.date=$(date --iso-8601=seconds)"

go.mod

@@ -21,6 +21,7 @@ require (
 	github.com/shurcooL/vfsgen v0.0.0-20200824052919-0d455de96546
 	github.com/sirupsen/logrus v1.9.0
 	github.com/spf13/cobra v1.5.0
+	github.com/stretchr/testify v1.7.0
 	golang.org/x/crypto v0.0.0-20220722155217-630584e8d5aa
 	golang.org/x/net v0.0.0-20220425223048-2871e0cb64e4
 	golang.org/x/term v0.0.0-20220722155259-a9ba230a4035
@@ -29,6 +30,7 @@ require (
 
 require (
 	github.com/andybalholm/cascadia v1.3.1 // indirect
+	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/go-shiori/dom v0.0.0-20210627111528-4e4722cd0d65 // indirect
 	github.com/gogs/chardet v0.0.0-20211120154057-b7413eaefb8f // indirect
 	github.com/google/uuid v1.3.0 // indirect
@@ -39,6 +41,7 @@ require (
 	github.com/mattn/go-colorable v0.1.12 // indirect
 	github.com/mattn/go-isatty v0.0.14 // indirect
 	github.com/mitchellh/go-homedir v1.1.0 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0 // indirect
 	github.com/shurcooL/httpfs v0.0.0-20190707220628-8d4bc4ba7749 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
@@ -51,6 +54,7 @@ require (
 	golang.org/x/text v0.3.7 // indirect
 	golang.org/x/tools v0.1.10 // indirect
 	golang.org/x/xerrors v0.0.0-20220411194840-2f41105eb62f // indirect
+	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
 	lukechampine.com/uint128 v1.2.0 // indirect
 	modernc.org/cc/v3 v3.36.0 // indirect
 	modernc.org/ccgo/v3 v3.16.8 // indirect

go.sum

@@ -772,11 +772,13 @@ github.com/kr/fs v0.1.0/go.mod h1:FFnZGqtBN9Gxj7eW1uZ42v5BccTP0vu6NEaFoC2HwRg=
 github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc=
 github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
 github.com/kr/pretty v0.2.0/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
+github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI=
 github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
 github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/pty v1.1.5/go.mod h1:9r2w37qlBe7rQ6e1fg1S/9xpWHSnaqNdHD3WcMdbPDA=
 github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
+github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
 github.com/ktrysmt/go-bitbucket v0.6.4/go.mod h1:9u0v3hsd2rqCHRIpbir1oP7F58uo5dq19sBYvuMoyQ4=
 github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
@@ -1772,6 +1774,7 @@ gopkg.in/check.v1 v1.0.0-20141024133853-64131543e789/go.mod h1:Co6ibVJAznAaIkqp8
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=
 gopkg.in/cheggaaa/pb.v1 v1.0.25/go.mod h1:V/YB90LKu/1FcN3WVnfiiE5oMCibMjukxqG/qStrOgw=
 gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI=

internal/database/pg.go

@@ -79,7 +79,8 @@ func (db *PGDatabase) SaveBookmarks(ctx context.Context, bookmarks ...model.Book
 			public   = $5,
 			content  = $6,
 			html     = $7,
-			modified = $8`)
+			modified = $8
+		RETURNING id`)
 		if err != nil {
 			return errors.WithStack(err)
 		}
@@ -112,11 +113,7 @@ func (db *PGDatabase) SaveBookmarks(ctx context.Context, bookmarks ...model.Book
 		// Execute statements
 		result = []model.Bookmark{}
 		for _, book := range bookmarks {
-			// Check ID, URL and title
+			// URL and title
-			if book.ID == 0 {
-				return errors.New("ID must not be empty")
-			}
-
 			if book.URL == "" {
 				return errors.New("URL must not be empty")
 			}
@@ -129,9 +126,9 @@ func (db *PGDatabase) SaveBookmarks(ctx context.Context, bookmarks ...model.Book
 			book.Modified = modifiedTime
 
 			// Save bookmark
-			_, err := stmtInsertBook.ExecContext(ctx,
+			err := stmtInsertBook.QueryRowContext(ctx,
 				book.URL, book.Title, book.Excerpt, book.Author,
-				book.Public, book.Content, book.HTML, book.Modified)
+				book.Public, book.Content, book.HTML, book.Modified).Scan(&book.ID)
 			if err != nil {
 				return errors.WithStack(err)
 			}
@@ -154,15 +151,15 @@ func (db *PGDatabase) SaveBookmarks(ctx context.Context, bookmarks ...model.Book
 
 				// If tag doesn't have any ID, fetch it from database
 				if tag.ID == 0 {
-					err = stmtGetTag.Get(&tag.ID, tagName)
+					err = stmtGetTag.GetContext(ctx, &tag.ID, tagName)
-					if err != nil {
+					if err != nil && !errors.Is(err, sql.ErrNoRows) {
 						return errors.WithStack(err)
 					}
 
 					// If tag doesn't exist in database, save it
 					if tag.ID == 0 {
 						var tagID64 int64
-						err = stmtInsertTag.Get(&tagID64, tagName)
+						err = stmtInsertTag.GetContext(ctx, &tagID64, tagName)
 						if err != nil {
 							return errors.WithStack(err)
 						}
@@ -170,7 +167,7 @@ func (db *PGDatabase) SaveBookmarks(ctx context.Context, bookmarks ...model.Book
 						tag.ID = int(tagID64)
 					}
 
-					if _, err := stmtInsertBookTag.Exec(tag.ID, book.ID); err != nil {
+					if _, err := stmtInsertBookTag.ExecContext(ctx, tag.ID, book.ID); err != nil {
 						return errors.WithStack(err)
 					}
 				}

internal/database/pg_test.go

@@ -0,0 +1,48 @@
+package database
+
+import (
+	"context"
+	"errors"
+	"log"
+	"os"
+	"testing"
+
+	"github.com/go-shiori/shiori/internal/model"
+	"github.com/golang-migrate/migrate/v4"
+	"github.com/stretchr/testify/assert"
+)
+
+func init() {
+	testPsqlURL := os.Getenv("SHIORI_TEST_PG_URL")
+	if testPsqlURL == "" {
+		log.Fatal("psql tests can't run without a PSQL database")
+	}
+}
+
+func TestPsqlSaveBookmarkWithTag(t *testing.T) {
+	ctx := context.TODO()
+	pgDB, err := OpenPGDatabase(ctx, os.Getenv("SHIORI_TEST_PG_URL"))
+	if err != nil {
+		t.Error(err)
+	}
+
+	if err := pgDB.Migrate(); err != nil && !errors.Is(migrate.ErrNoChange, err) {
+		t.Error(err)
+	}
+
+	book := model.Bookmark{
+		URL:   "https://github.com/go-shiori/obelisk",
+		Title: "shiori",
+		Tags: []model.Tag{
+			{
+				Name: "test-tag",
+			},
+		},
+	}
+
+	result, err := pgDB.SaveBookmarks(ctx, book)
+
+	assert.NoError(t, err, "Save bookmarks must not fail")
+	assert.Equal(t, book.URL, result[0].URL)
+	assert.Equal(t, book.Tags[0].Name, result[0].Tags[0].Name)
+}