fix(db): avoid using dash in fts5 queries (#514)

* fix(sqlite): avoid using dash in fts5 queries

* test(db): get bookmarks and count with dash in keyword

* test(db): common and ad-hoc sqlite test case

internal/database/database_test.go

@@ -16,8 +16,10 @@ func testDatabase(t *testing.T, dbFactory testDatabaseFactory) {
 		"testCreateBookmark":              testCreateBookmark,
 		"testCreateBookmarkTwice":         testCreateBookmarkTwice,
 		"testCreateBookmarkWithTag":       testCreateBookmarkWithTag,
-		"testCreateTwoDifferentBookmarks": testCreateTwoDifferentBookmarks,
 		"testUpdateBookmark":              testUpdateBookmark,
+		"testGetBookmarks":                testGetBookmarks,
+		"testGetBookmarksCount":           testGetBookmarksCount,
+    "testCreateTwoDifferentBookmarks": testCreateTwoDifferentBookmarks,
 	}
 
 	for testName, testCase := range tests {
@@ -121,3 +123,44 @@ func testUpdateBookmark(t *testing.T, db DB) {
 	assert.Equal(t, "modified", result[0].Title)
 	assert.Equal(t, savedBookmark.ID, result[0].ID)
 }
+
+func testGetBookmarks(t *testing.T, db DB) {
+	ctx := context.TODO()
+
+	book := model.Bookmark{
+		URL:   "https://github.com/go-shiori/shiori",
+		Title: "shiori",
+	}
+
+	bookmarks, err := db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+
+	savedBookmark := bookmarks[0]
+
+	results, err := db.GetBookmarks(ctx, GetBookmarksOptions{
+		Keyword: "go-shiori",
+	})
+
+	assert.NoError(t, err, "Get bookmarks should not fail")
+	assert.Len(t, results, 1, "results should contain one item")
+	assert.Equal(t, savedBookmark.ID, results[0].ID, "bookmark should be the one saved")
+}
+
+func testGetBookmarksCount(t *testing.T, db DB) {
+	ctx := context.TODO()
+
+	expectedCount := 1
+	book := model.Bookmark{
+		URL:   "https://github.com/go-shiori/shiori",
+		Title: "shiori",
+	}
+
+	_, err := db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+
+	count, err := db.GetBookmarksCount(ctx, GetBookmarksOptions{
+		Keyword: "go-shiori",
+	})
+	assert.NoError(t, err, "Get bookmarks count should not fail")
+	assert.Equal(t, count, expectedCount, "count should be %d", expectedCount)
+}

internal/database/sqlite.go

@@ -281,9 +281,12 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt
 
 		args = append(args,
 			"%"+opts.Keyword+"%",
-			"%"+opts.Keyword+"%",
-			opts.Keyword,
-			opts.Keyword)
+			"%"+opts.Keyword+"%")
+
+		// Replace dash with spaces since FTS5 uses `-name` as column identifier
+		opts.Keyword = strings.Replace(opts.Keyword, "-", " ", -1)
+		args = append(args, opts.Keyword, opts.Keyword)
+
 	}
 
 	// Add where clause for tags.
@@ -470,8 +473,11 @@ func (db *SQLiteDatabase) GetBookmarksCount(ctx context.Context, opts GetBookmar
 		args = append(args,
 			"%"+opts.Keyword+"%",
 			"%"+opts.Keyword+"%",
-			opts.Keyword,
-			opts.Keyword)
+		)
+
+		// Replace dash with spaces since FTS5 uses `-name` as column identifier
+		opts.Keyword = strings.Replace(opts.Keyword, "-", " ", -1)
+		args = append(args, opts.Keyword, opts.Keyword)
 	}
 
 	// Add where clause for tags.

internal/database/sqlite_test.go

@@ -6,8 +6,10 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/go-shiori/shiori/internal/model"
 	"github.com/golang-migrate/migrate/v4"
 	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
 )
 
 var sqliteDatabaseTestPath string
@@ -33,4 +35,44 @@ func sqliteTestDatabaseFactory(ctx context.Context) (DB, error) {
 
 func TestSqliteDatabase(t *testing.T) {
 	testDatabase(t, sqliteTestDatabaseFactory)
+	testSqliteGetBookmarksWithDash(t)
+}
+
+// testSqliteGetBookmarksWithDash ad-hoc test for SQLite that checks that a match search against
+// the FTS5 engine does not fail by using dashes, making sqlite think that we are trying to avoid
+// matching a column name. This works in a fun way and it seems that it depends on the tokens
+// already scanned by the database, since trying to match for `go-shiori` with no bookmarks or only
+// the shiori bookmark does not fail, but it fails if we add any other bookmark to the database, hence
+// this test.
+func testSqliteGetBookmarksWithDash(t *testing.T) {
+	ctx := context.TODO()
+
+	db, err := sqliteTestDatabaseFactory(ctx)
+	assert.NoError(t, err)
+
+	book := model.Bookmark{
+		URL:   "https://github.com/go-shiori/shiori",
+		Title: "shiori",
+	}
+
+	_, err = db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+
+	book = model.Bookmark{
+		URL:   "https://github.com/jamiehannaford/what-happens-when-k8s",
+		Title: "what-happens-when-k8s",
+	}
+
+	result, err := db.SaveBookmarks(ctx, true, book)
+	assert.NoError(t, err, "Save bookmarks must not fail")
+	savedBookmark := result[0]
+
+	results, err := db.GetBookmarks(ctx, GetBookmarksOptions{
+		Keyword: "what-happens-when",
+	})
+
+	assert.NoError(t, err, "Get bookmarks should not fail")
+	assert.Len(t, results, 1, "results should contain one item")
+	assert.Equal(t, savedBookmark.ID, results[0].ID, "bookmark should be the one saved")
+
 }