From c356b7bd003ca5b02978b8717167a2e767f1b13a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Menu?= Date: Sun, 22 May 2022 16:39:13 +0200 Subject: [PATCH] Update existing links when adding a new note (#219) --- CHANGELOG.md | 4 + internal/adapter/sqlite/link_dao.go | 61 +++++---- internal/adapter/sqlite/note_dao.go | 1 + internal/adapter/sqlite/note_index.go | 150 ++++++++++++++++++--- internal/adapter/sqlite/note_index_test.go | 2 +- internal/adapter/sqlite/util.go | 8 ++ internal/cli/container.go | 2 +- internal/core/link.go | 9 ++ internal/core/note_index.go | 4 + internal/core/note_new_test.go | 3 + tests/cmd-init-defaults.tesh | 4 +- tests/cmd-init.tesh | 6 +- tests/fixtures/issue-23/template.md | 1 - tests/issue-16.tesh | 11 ++ tests/issue-20.tesh | 2 +- tests/issue-23.tesh | 13 ++ 16 files changed, 231 insertions(+), 50 deletions(-) delete mode 100644 tests/fixtures/issue-23/template.md create mode 100644 tests/issue-16.tesh create mode 100644 tests/issue-23.tesh diff --git a/CHANGELOG.md b/CHANGELOG.md index 2763a09..3d783bf 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/adapter/sqlite/link_dao.go b/internal/adapter/sqlite/link_dao.go index 4746bef..7985796 100644 --- a/internal/adapter/sqlite/link_dao.go +++ b/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 - external bool - rels sql.NullString + id, sourceID, snippetStart, snippetEnd int + targetID sql.NullInt64 + sourcePath, title, href, linkType, snippet string + external bool + 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, diff --git a/internal/adapter/sqlite/note_dao.go b/internal/adapter/sqlite/note_dao.go index 0ce3c8b..bb7687c 100644 --- a/internal/adapter/sqlite/note_dao.go +++ b/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. diff --git a/internal/adapter/sqlite/note_index.go b/internal/adapter/sqlite/note_index.go index 6d65d68..294eb9c 100644 --- a/internal/adapter/sqlite/note_index.go +++ b/internal/adapter/sqlite/note_index.go @@ -1,18 +1,24 @@ 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 { - db *DB - dao *dao - logger util.Logger + notebookPath string + db *DB + dao *dao + logger util.Logger } type dao struct { @@ -22,10 +28,11 @@ 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{ - db: db, - logger: logger, + 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 } diff --git a/internal/adapter/sqlite/note_index_test.go b/internal/adapter/sqlite/note_index_test.go index cf3efa7..453de28 100644 --- a/internal/adapter/sqlite/note_index_test.go +++ b/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) { diff --git a/internal/adapter/sqlite/util.go b/internal/adapter/sqlite/util.go index 9aa7b1c..da5bbde 100644 --- a/internal/adapter/sqlite/util.go +++ b/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} diff --git a/internal/cli/container.go b/internal/cli/container.go index f9af809..26e41e4 100644 --- a/internal/cli/container.go +++ b/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, diff --git a/internal/core/link.go b/internal/core/link.go index 5eb5441..4f6ba36 100644 --- a/internal/core/link.go +++ b/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"` diff --git a/internal/core/note_index.go b/internal/core/note_index.go index c192b77..45b2fcd 100644 --- a/internal/core/note_index.go +++ b/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) diff --git a/internal/core/note_new_test.go b/internal/core/note_new_test.go index 7f64c63..7009f51 100644 --- a/internal/core/note_new_test.go +++ b/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 } diff --git a/tests/cmd-init-defaults.tesh b/tests/cmd-init-defaults.tesh index 5469082..b20af5a 100644 --- a/tests/cmd-init-defaults.tesh +++ b/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 diff --git a/tests/cmd-init.tesh b/tests/cmd-init.tesh index c543cdc..29cfee5 100644 --- a/tests/cmd-init.tesh +++ b/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}} diff --git a/tests/fixtures/issue-23/template.md b/tests/fixtures/issue-23/template.md deleted file mode 100644 index c296a5f..0000000 --- a/tests/fixtures/issue-23/template.md +++ /dev/null @@ -1 +0,0 @@ -# Template diff --git a/tests/issue-16.tesh b/tests/issue-16.tesh new file mode 100644 index 0000000..277c264 --- /dev/null +++ b/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 + diff --git a/tests/issue-20.tesh b/tests/issue-20.tesh index 840bbdb..2a0391d 100644 --- a/tests/issue-20.tesh +++ b/tests/issue-20.tesh @@ -6,5 +6,5 @@ $ mkdir .zk -$ zk index -q +$ zk index -q 2> /dev/null diff --git a/tests/issue-23.tesh b/tests/issue-23.tesh new file mode 100644 index 0000000..0af2664 --- /dev/null +++ b/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