Update existing links when adding a new note (#219)

CHANGELOG.md

@@ -8,6 +8,10 @@ All notable changes to this project will be documented in this file.
 
 * Removed the dependency on `libicu`.
 
+### Fixed
+
+* Indexed links are now automatically updated when adding a new note, if it is a better match than the previous link target.
+
 
 ## 0.10.0
 

internal/adapter/sqlite/link_dao.go

@@ -15,8 +15,8 @@ type LinkDAO struct {
 
 	// Prepared SQL statements
 	addLinkStmt        *LazyStmt
-	setLinksTargetStmt *LazyStmt
 	removeLinksStmt    *LazyStmt
+	updateTargetIDStmt *LazyStmt
 }
 
 // NewLinkDAO creates a new instance of a DAO working on the given database
@@ -32,19 +32,17 @@ func NewLinkDAO(tx Transaction, logger util.Logger) *LinkDAO {
 			VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
 		`),
 
-		// Set links matching a given href and missing a target ID to the given
-		// target ID.
-		setLinksTargetStmt: tx.PrepareLazy(`
-			UPDATE links
-			   SET target_id = ?
-			 WHERE target_id IS NULL AND external = 0 AND ? LIKE href || '%'
-		`),
-
 		// Remove all the outbound links of a note.
 		removeLinksStmt: tx.PrepareLazy(`
 			DELETE FROM links
 			 WHERE source_id = ?
 		`),
+
+		updateTargetIDStmt: tx.PrepareLazy(`
+			UPDATE links
+			   SET target_id = ?
+			 WHERE id = ?
+		`),
 	}
 }
 
@@ -69,10 +67,9 @@ func (d *LinkDAO) RemoveAll(id core.NoteID) error {
 	return err
 }
 
-// SetTargetID updates the missing target_id for links matching the given href.
-// FIXME: Probably doesn't work for all type of href (partial, wikilinks, etc.)
-func (d *LinkDAO) SetTargetID(href string, id core.NoteID) error {
-	_, err := d.setLinksTargetStmt.Exec(int64(id), href)
+// SetTargetID updates the target note of a link.
+func (d *LinkDAO) SetTargetID(id core.LinkID, targetID core.NoteID) error {
+	_, err := d.updateTargetIDStmt.Exec(noteIDToSQL(targetID), linkIDToSQL(id))
 	return err
 }
 
@@ -90,15 +87,31 @@ func joinLinkRels(rels []core.LinkRelation) string {
 	return res
 }
 
+// FindInternal returns all the links internal to the notebook.
+func (d *LinkDAO) FindInternal() ([]core.ResolvedLink, error) {
+	return d.findWhere("external = 0")
+}
+
+// FindBetweenNotes returns all the links existing between the given notes.
 func (d *LinkDAO) FindBetweenNotes(ids []core.NoteID) ([]core.ResolvedLink, error) {
+	idsString := joinNoteIDs(ids, ",")
+	return d.findWhere(fmt.Sprintf("source_id IN (%s) AND target_id IN (%s)", idsString, idsString))
+}
+
+// findWhere returns all the links, filtered by the given where query.
+func (d *LinkDAO) findWhere(where string) ([]core.ResolvedLink, error) {
 	links := make([]core.ResolvedLink, 0)
 
-	idsString := joinNoteIDs(ids, ",")
-	rows, err := d.tx.Query(fmt.Sprintf(`
+	query := `
 		SELECT id, source_id, source_path, target_id, target_path, title, href, type, external, rels, snippet, snippet_start, snippet_end
 		  FROM resolved_links
-		 WHERE source_id IN (%s) AND target_id IN (%s)
-	`, idsString, idsString))
+	`
+
+	if where != "" {
+		query += "\nWHERE " + where
+	}
+
+	rows, err := d.tx.Query(query)
 	if err != nil {
 		return links, err
 	}
@@ -120,10 +133,11 @@ func (d *LinkDAO) FindBetweenNotes(ids []core.NoteID) ([]core.ResolvedLink, erro
 
 func (d *LinkDAO) scanLink(row RowScanner) (*core.ResolvedLink, error) {
 	var (
-		id, sourceID, targetID, snippetStart, snippetEnd       int
-		sourcePath, targetPath, title, href, linkType, snippet string
+		id, sourceID, snippetStart, snippetEnd     int
+		targetID                                   sql.NullInt64
+		sourcePath, title, href, linkType, snippet string
 		external                                   bool
-		rels                                                   sql.NullString
+		targetPath, rels                           sql.NullString
 	)
 
 	err := row.Scan(
@@ -137,10 +151,11 @@ func (d *LinkDAO) scanLink(row RowScanner) (*core.ResolvedLink, error) {
 		return nil, err
 	default:
 		return &core.ResolvedLink{
+			ID:         core.LinkID(id),
 			SourceID:   core.NoteID(sourceID),
 			SourcePath: sourcePath,
-			TargetID:   core.NoteID(targetID),
-			TargetPath: targetPath,
+			TargetID:   core.NoteID(targetID.Int64),
+			TargetPath: targetPath.String,
 			Link: core.Link{
 				Title:        title,
 				Href:         href,

internal/adapter/sqlite/note_dao.go

@@ -278,6 +278,7 @@ func (d *NoteDAO) findIdsByHrefs(hrefs []string, allowPartialHrefs bool) ([]core
 	return ids, nil
 }
 
+// FIXME: This logic is duplicated in NoteIndex.linkMatchesPath(). Maybe there's a way to share it using a custom SQLite function?
 func (d *NoteDAO) FindIdsByHref(href string, allowPartialHref bool) ([]core.NoteID, error) {
 	// Remove any anchor at the end of the HREF, since it's most likely
 	// matching a sub-section in the note.

internal/adapter/sqlite/note_index.go

@@ -1,15 +1,21 @@
 package sqlite
 
 import (
+	"path/filepath"
+	"regexp"
+	"strings"
+
 	"github.com/mickael-menu/zk/internal/core"
 	"github.com/mickael-menu/zk/internal/util"
 	"github.com/mickael-menu/zk/internal/util/errors"
 	"github.com/mickael-menu/zk/internal/util/paths"
+	strutil "github.com/mickael-menu/zk/internal/util/strings"
 )
 
 // NoteIndex persists note indexing results in the SQLite database.
 // It implements the port core.NoteIndex and acts as a facade to the DAOs.
 type NoteIndex struct {
+	notebookPath string
 	db           *DB
 	dao          *dao
 	logger       util.Logger
@@ -22,8 +28,9 @@ type dao struct {
 	metadata    *MetadataDAO
 }
 
-func NewNoteIndex(db *DB, logger util.Logger) *NoteIndex {
+func NewNoteIndex(notebookPath string, db *DB, logger util.Logger) *NoteIndex {
 	return &NoteIndex{
+		notebookPath: notebookPath,
 		db:           db,
 		logger:       logger,
 	}
@@ -47,6 +54,37 @@ func (ni *NoteIndex) FindMinimal(opts core.NoteFindOpts) (notes []core.MinimalNo
 	return
 }
 
+// FindLinkMatch implements core.NoteIndex.
+func (ni *NoteIndex) FindLinkMatch(baseDir string, href string, linkType core.LinkType) (id core.NoteID, err error) {
+	err = ni.commit(func(dao *dao) error {
+		id, err = ni.findLinkMatch(dao, baseDir, href, linkType)
+		return err
+	})
+	return
+}
+
+func (ni *NoteIndex) findLinkMatch(dao *dao, baseDir string, href string, linkType core.LinkType) (core.NoteID, error) {
+	if strutil.IsURL(href) {
+		return 0, nil
+	}
+
+	id, _ := ni.findPathMatch(dao, baseDir, href)
+	if id.IsValid() {
+		return id, nil
+	}
+
+	allowPartialMatch := (linkType == core.LinkTypeWikiLink)
+	return dao.notes.FindIdByHref(href, allowPartialMatch)
+}
+
+func (ni *NoteIndex) findPathMatch(dao *dao, baseDir string, href string) (core.NoteID, error) {
+	href, err := ni.relNotebookPath(baseDir, href)
+	if err != nil {
+		return 0, err
+	}
+	return dao.notes.FindIdByHref(href, false)
+}
+
 // FindLinksBetweenNotes implements core.NoteIndex.
 func (ni *NoteIndex) FindLinksBetweenNotes(ids []core.NoteID) (links []core.ResolvedLink, err error) {
 	err = ni.commit(func(dao *dao) error {
@@ -82,8 +120,14 @@ func (ni *NoteIndex) Add(note core.Note) (id core.NoteID, err error) {
 		if err != nil {
 			return err
 		}
+		note.ID = id
+
+		err = ni.addLinks(dao, id, note.Links)
+		if err != nil {
+			return err
+		}
 
-		err = ni.addLinks(dao, id, note)
+		err = ni.fixExistingLinks(dao, note.ID, note.Path)
 		if err != nil {
 			return err
 		}
@@ -95,6 +139,81 @@ func (ni *NoteIndex) Add(note core.Note) (id core.NoteID, err error) {
 	return
 }
 
+// fixExistingLinks will go over all indexed links and update their target to
+// the given id if they match the given path better than their current
+// targetPath.
+func (ni *NoteIndex) fixExistingLinks(dao *dao, id core.NoteID, path string) error {
+	links, err := dao.links.FindInternal()
+	if err != nil {
+		return err
+	}
+
+	for _, link := range links {
+		// To find the best match possible, shortest paths take precedence.
+		// See https://github.com/mickael-menu/zk/issues/23
+		if link.TargetPath != "" && len(link.TargetPath) < len(path) {
+			continue
+		}
+
+		if matches, err := ni.linkMatchesPath(link, path); matches && err == nil {
+			err = dao.links.SetTargetID(link.ID, id)
+		}
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// linkMatchesPath returns whether the given link can be used to reach the
+// given note path.
+func (ni *NoteIndex) linkMatchesPath(link core.ResolvedLink, path string) (bool, error) {
+	// Remove any anchor at the end of the HREF, since it's most likely
+	// matching a sub-section in the note.
+	href := strings.SplitN(link.Href, "#", 2)[0]
+
+	matchString := func(pattern string, s string) bool {
+		reg := regexp.MustCompile(pattern)
+		return reg.MatchString(s)
+	}
+
+	matches := func(href string, allowPartialHref bool) bool {
+		href = regexp.QuoteMeta(href)
+
+		if allowPartialHref {
+			if matchString("^(.*/)?[^/]*"+href+"[^/]*$", path) {
+				return true
+			}
+			if matchString(".*"+href+".*", path) {
+				return true
+			}
+		}
+
+		return matchString("^(?:"+href+"[^/]*|"+href+"/.+)$", path)
+	}
+
+	baseDir := filepath.Dir(link.SourcePath)
+	if relHref, err := ni.relNotebookPath(baseDir, href); err != nil {
+		if matches(relHref, false) {
+			return true, nil
+		}
+	}
+
+	allowPartialMatch := (link.Type == core.LinkTypeWikiLink)
+	return matches(href, allowPartialMatch), nil
+}
+
+// relNotebookHref makes the given href (which is relative to baseDir) relative
+// to the notebook root instead.
+func (ni *NoteIndex) relNotebookPath(baseDir string, href string) (string, error) {
+	path := filepath.Clean(filepath.Join(baseDir, href))
+	path, err := filepath.Rel(ni.notebookPath, path)
+
+	return path,
+		errors.Wrapf(err, "failed to make href relative to the notebook: %s", href)
+}
+
 // Update implements core.NoteIndex.
 func (ni *NoteIndex) Update(note core.Note) error {
 	err := ni.commit(func(dao *dao) error {
@@ -108,7 +227,7 @@ func (ni *NoteIndex) Update(note core.Note) error {
 		if err != nil {
 			return err
 		}
-		err = ni.addLinks(dao, id, note)
+		err = ni.addLinks(dao, id, note.Links)
 		if err != nil {
 			return err
 		}
@@ -139,26 +258,19 @@ func (ni *NoteIndex) associateTags(collections *CollectionDAO, noteId core.NoteI
 	return nil
 }
 
-func (ni *NoteIndex) addLinks(dao *dao, id core.NoteID, note core.Note) error {
-	links, err := ni.resolveLinkNoteIDs(dao, id, note.Links)
+func (ni *NoteIndex) addLinks(dao *dao, id core.NoteID, links []core.Link) error {
+	resolvedLinks, err := ni.resolveLinkNoteIDs(dao, id, links)
 	if err != nil {
 		return err
 	}
-
-	err = dao.links.Add(links)
-	if err != nil {
-		return err
-	}
-
-	return dao.links.SetTargetID(note.Path, id)
+	return dao.links.Add(resolvedLinks)
 }
 
 func (ni *NoteIndex) resolveLinkNoteIDs(dao *dao, sourceID core.NoteID, links []core.Link) ([]core.ResolvedLink, error) {
 	resolvedLinks := []core.ResolvedLink{}
 
 	for _, link := range links {
-		allowPartialMatch := (link.Type == core.LinkTypeWikiLink)
-		targetID, err := dao.notes.FindIdByHref(link.Href, allowPartialMatch)
+		targetID, err := ni.findLinkMatch(dao, "" /* base dir */, link.Href, link.Type)
 		if err != nil {
 			return resolvedLinks, err
 		}

internal/adapter/sqlite/note_index_test.go

@@ -220,7 +220,7 @@ func TestNoteIndexUpdateWithTags(t *testing.T) {
 
 func testNoteIndex(t *testing.T) (*DB, *NoteIndex) {
 	db := testDB(t)
-	return db, NewNoteIndex(db, &util.NullLogger)
+	return db, NewNoteIndex("", db, &util.NullLogger)
 }
 
 func assertTagExistsOrNot(t *testing.T, db *DB, shouldExist bool, tag string) {

internal/adapter/sqlite/util.go

@@ -30,6 +30,14 @@ func escapeLikeTerm(term string, escapeChar rune) string {
 	return escape(escape(escape(term, string(escapeChar)), "%"), "_")
 }
 
+func linkIDToSQL(id core.LinkID) sql.NullInt64 {
+	if id.IsValid() {
+		return sql.NullInt64{Int64: int64(id), Valid: true}
+	} else {
+		return sql.NullInt64{}
+	}
+}
+
 func noteIDToSQL(id core.NoteID) sql.NullInt64 {
 	if id.IsValid() {
 		return sql.NullInt64{Int64: int64(id), Valid: true}

internal/cli/container.go

@@ -90,7 +90,7 @@ func NewContainer(version string) (*Container, error) {
 				}
 
 				notebook := core.NewNotebook(path, config, core.NotebookPorts{
-					NoteIndex: sqlite.NewNoteIndex(db, logger),
+					NoteIndex: sqlite.NewNoteIndex(path, db, logger),
 					NoteContentParser: markdown.NewParser(
 						markdown.ParserOpts{
 							HashtagEnabled:      config.Format.Markdown.Hashtags,

internal/core/link.go

@@ -1,5 +1,13 @@
 package core
 
+// LinkID represents the unique ID of a note link relative to a given
+// NoteIndex implementation.
+type LinkID int64
+
+func (id LinkID) IsValid() bool {
+	return id > 0
+}
+
 // Link represents a link in a note to another note or an external resource.
 type Link struct {
 	// Label of the link.
@@ -23,6 +31,7 @@ type Link struct {
 // ResolvedLink represents a link between two indexed notes.
 type ResolvedLink struct {
 	Link
+	ID         LinkID `json:"-"`
 	SourceID   NoteID `json:"sourceId"`
 	SourcePath string `json:"sourcePath"`
 	TargetID   NoteID `json:"targetId"`

internal/core/note_index.go

@@ -20,6 +20,10 @@ type NoteIndex interface {
 	// given filtering and sorting criteria.
 	FindMinimal(opts NoteFindOpts) ([]MinimalNote, error)
 
+	// Find link match returns the best note match for a given link href,
+	// relative to baseDir.
+	FindLinkMatch(baseDir string, href string, linkType LinkType) (NoteID, error)
+
 	// FindLinksBetweenNotes retrieves the links between the given notes.
 	FindLinksBetweenNotes(ids []NoteID) ([]ResolvedLink, error)
 

internal/core/note_new_test.go

@@ -504,6 +504,9 @@ type noteIndexAddMock struct {
 
 func (m *noteIndexAddMock) Find(opts NoteFindOpts) ([]ContextualNote, error)     { return nil, nil }
 func (m *noteIndexAddMock) FindMinimal(opts NoteFindOpts) ([]MinimalNote, error) { return nil, nil }
+func (m *noteIndexAddMock) FindLinkMatch(baseDir string, href string, linkType LinkType) (NoteID, error) {
+	return 0, nil
+}
 func (m *noteIndexAddMock) FindLinksBetweenNotes(ids []NoteID) ([]ResolvedLink, error) {
 	return nil, nil
 }

tests/cmd-init-defaults.tesh

@@ -1,4 +1,6 @@
-$ zk init --no-input > /dev/null
+$ zk init --no-input 2> /dev/null
+>
+>Initialized a notebook in {{working-dir}}
 
 # Test default config.
 $ cat .zk/config.toml

tests/cmd-init.tesh

@@ -16,7 +16,7 @@ $ zk init --help
 >      --no-input             Never prompt or ask for confirmation.
 
 # Creates a new notebook in a new directory.
-$ zk init --no-input new-dir
+$ zk init --no-input new-dir 2> /dev/null
 >
 >Initialized a notebook in {{working-dir}}/new-dir
 
@@ -25,7 +25,7 @@ $ test -f new-dir/.zk/config.toml
 # Creates a new notebook in an existing directory.
 $ mkdir existing-dir
 
-$ zk init --no-input existing-dir
+$ zk init --no-input existing-dir 2> /dev/null
 >
 >Initialized a notebook in {{working-dir}}/existing-dir
 
@@ -34,7 +34,7 @@ $ test -f existing-dir/.zk/config.toml
 # Creates a new notebook in the current directory.
 $ mkdir cur-dir && cd cur-dir
 
-$ zk init --no-input
+$ zk init --no-input 2> /dev/null
 >
 >Initialized a notebook in {{working-dir}}
 

tests/fixtures/issue-23/template.md

@@ -1 +0,0 @@
-# Template

tests/issue-16.tesh

@@ -0,0 +1,11 @@
+# Links with anchors are broken.
+# https://github.com/mickael-menu/zk/issues/16
+
+$ cd blank
+
+$ touch note.md
+$ echo "[[note#section]]" > index.md
+
+$ zk list -qfpath --linked-by index.md
+>note.md
+

tests/issue-20.tesh

@@ -6,5 +6,5 @@
 
 $ mkdir .zk
 
-$ zk index -q
+$ zk index -q 2> /dev/null
 

tests/issue-23.tesh

@@ -0,0 +1,13 @@
+# Link hrefs are not finding the best match
+# https://github.com/mickael-menu/zk/issues/23
+
+$ cd issue-23
+
+$ zk list -qfpath --linked-by index.md
+>template-creation.md
+
+# Add a note with a shorter filename, it should be a better match.
+$ echo "# Template" > template.md
+
+$ zk list -qfpath --linked-by index.md
+>template.md