diff --git a/internal/adapter/lsp/server.go b/internal/adapter/lsp/server.go index 4c0f3b6e..9ab48ff9 100644 --- a/internal/adapter/lsp/server.go +++ b/internal/adapter/lsp/server.go @@ -325,7 +325,12 @@ func NewServer(opts ServerOpts) *Server { return nil, err } - if isTrue(clientCapabilities.TextDocument.Definition.LinkSupport) { + linkSupport := false + if clientCapabilities.TextDocument != nil && clientCapabilities.TextDocument.Definition != nil { + linkSupport = isTrue(clientCapabilities.TextDocument.Definition.LinkSupport) + } + + if linkSupport { return protocol.LocationLink{ OriginSelectionRange: &link.Range, TargetURI: target.URI, @@ -531,13 +536,25 @@ func (s *Server) notebookOf(doc *document) (*core.Notebook, error) { // 2. Find any occurrence of the href in a note path (substring) // 3. Match the href as a term in the note titles func (s *Server) noteForLink(link documentLink, notebook *core.Notebook) (*Note, error) { - note, err := s.noteForHref(link.Href, link.RelativeToDir, notebook) - if note == nil && err == nil && link.IsWikiLink { - // Try to find a partial href match. - note, err = notebook.FindByHref(link.Href, true) + if strutil.IsURL(link.Href) { + return nil, nil } - if note == nil || err != nil { - return nil, err + + // 1. Try relative to the current file (relativeToDir) + note, _ := s.noteForHref(link.Href, link.RelativeToDir, notebook) + + // 2. Try relative to vault root + if note == nil { + note, _ = notebook.FindByHref(link.Href, false) + } + + // 3. Fallback to partial matching for both link types + if note == nil { + note, _ = notebook.FindByHref(link.Href, true) + } + + if note == nil { + return nil, fmt.Errorf("failed to resolve href: %s", link.Href) } joinedPath := filepath.Join(notebook.Path, note.Path) diff --git a/internal/adapter/lsp/server_test.go b/internal/adapter/lsp/server_test.go index c9cd4c58..27c9334f 100644 --- a/internal/adapter/lsp/server_test.go +++ b/internal/adapter/lsp/server_test.go @@ -169,3 +169,78 @@ func TestServer_buildInvokedCompletionList(t *testing.T) { }) } } + +func TestDefinition_PanicProtection(t *testing.T) { + fixture := getNotebookFixture("completion") + + fs, err := fs.NewFileStorage(fixture.Path, &util.NullLogger) + if err != nil { + t.Fatal(err) + } + fixture.FS = fs + + db, err := sqlite.OpenInMemory() + if err != nil { + t.Fatal(err) + } + defer db.Close() + + config := core.NewDefaultConfig() + index := sqlite.NewNoteIndex(fixture.Path, db, &util.NullLogger, config.Note.Extension) + parser := markdown.NewParser(markdown.ParserOpts{}, &util.NullLogger) + + notebook := core.NewNotebook(fixture.Path, config, core.NotebookPorts{ + NoteIndex: index, + NoteContentParser: parser, + FS: fs, + TemplateLoaderFactory: func(lang string) (core.TemplateLoader, error) { + return &core.NullTemplateLoader, nil + }, + Logger: &util.NullLogger, + }) + notebook.Index(core.NoteIndexOpts{Force: true}) + + docStore := newDocumentStore(fs, &util.NullLogger) + server := &Server{ + documents: docStore, + } + + // Create note content with a link + noteContent := "Link to [[Item2]]" + doc, err := docStore.DidOpen(protocol.DidOpenTextDocumentParams{ + TextDocument: protocol.TextDocumentItem{ + URI: pathToURI(filepath.Join(fixture.Path, "Item1.md")), + LanguageID: "markdown", + Version: 1, + Text: noteContent, + }, + }, nil) + if err != nil { + t.Fatal(err) + } + + link, err := doc.DocumentLinkAt(protocol.Position{Line: 0, Character: 10}) + if err != nil { + t.Fatal(err) + } + if link == nil { + t.Fatal("Link not found at position") + } + + // 1. Test with NIL capabilities (the crash case) + var nilCaps protocol.ClientCapabilities + + target, err := server.noteForLink(*link, notebook) + assert.Nil(t, err) + assert.NotNil(t, target) + + // Logic from Definition handler: + linkSupport := false + if nilCaps.TextDocument != nil && nilCaps.TextDocument.Definition != nil { + linkSupport = isTrue(nilCaps.TextDocument.Definition.LinkSupport) + } + + if linkSupport { + t.Error("linkSupport should be false for nil capabilities") + } +} diff --git a/internal/adapter/sqlite/note_dao.go b/internal/adapter/sqlite/note_dao.go index fdf94c22..45d23bb1 100644 --- a/internal/adapter/sqlite/note_dao.go +++ b/internal/adapter/sqlite/note_dao.go @@ -296,31 +296,48 @@ func (d *NoteDAO) findIDsByHrefs(hrefs []string, allowPartialHrefs bool) ([]core // FindIdsByHref finds note IDs which match the given href string. // This implements logic similar to NoteIndex.linkMatchesPath. func (d *NoteDAO) FindIdsByHref(href string, allowPartialHref bool) ([]core.NoteID, error) { + var err 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(href, "#", 2)[0] href = strings.NewReplacer("%", "\\%", "_", "\\_").Replace(href) + + // Treat leading slash as relative to the notebook root. + rootHref := strings.TrimPrefix(href, "/") - // Prioritise exact match with extension. - id, err := d.FindIDByPath(href + "." + d.extension) - if err != nil { - return nil, err - } - if id.IsValid() { - return []core.NoteID{id}, nil + // 1. Prioritize exact match (either literal href or root-relative). + for _, h := range []string{href, rootHref} { + id, err := d.FindIDByPath(h) + if err != nil { + return nil, err + } + if id.IsValid() { + return []core.NoteID{id}, nil + } + + // Try with extension. + if !strings.HasSuffix(h, "."+d.extension) { + id, err = d.FindIDByPath(h + "." + d.extension) + if err != nil { + return nil, err + } + if id.IsValid() { + return []core.NoteID{id}, nil + } + } } var ids []core.NoteID if allowPartialHref { // Filename (not path) contains 'href' anywhere. - ids, err = d.findIDsByFilenameLike("%" + href + "%") + ids, err = d.findIDsByFilenameLike("%" + rootHref + "%") if len(ids) > 0 || err != nil { return ids, err } // Path contains 'href' anywhere. - ids, err = d.findIDsByPathLike("%" + href + "%") + ids, err = d.findIDsByPathLike("%" + rootHref + "%") if len(ids) > 0 || err != nil { return ids, err } @@ -329,7 +346,7 @@ func (d *NoteDAO) FindIdsByHref(href string, allowPartialHref bool) ([]core.Note // Path either: // 1. starts with 'href' and has no slash after href. // 2. starts with 'href/', followed by more content. - ids, err = d.findIDsByPathPrefix(href) + ids, err = d.findIDsByPathPrefix(rootHref) if len(ids) > 0 || err != nil { return ids, err } @@ -528,6 +545,7 @@ func (d *NoteDAO) findRows(opts core.NoteFindOpts, selection noteSelection) (*sq } idSelects := make([]string, 0) + hrefList := "(" + joinStrings(hrefs, ",", "'") + ")" if direction <= 0 { idSelects = append(idSelects, fmt.Sprintf( " SELECT target_id FROM %s WHERE target_id IS NOT NULL AND source_id IN %s", @@ -535,10 +553,17 @@ func (d *NoteDAO) findRows(opts core.NoteFindOpts, selection noteSelection) (*sq )) } if direction >= 0 { - idSelects = append(idSelects, fmt.Sprintf( - " SELECT source_id FROM %s WHERE target_id IS NOT NULL AND target_id IN %s", - linksSrc, idsList, - )) + if linksSrc == "links" { + idSelects = append(idSelects, fmt.Sprintf( + " SELECT source_id FROM %s WHERE (target_id IN %s OR (target_id IS NULL AND href IN %s))", + linksSrc, idsList, hrefList, + )) + } else { + idSelects = append(idSelects, fmt.Sprintf( + " SELECT source_id FROM %s WHERE target_id IS NOT NULL AND target_id IN %s", + linksSrc, idsList, + )) + } } idExpr += " IN (\n" + strings.Join(idSelects, "\n UNION\n") + "\n)" diff --git a/internal/adapter/sqlite/note_index.go b/internal/adapter/sqlite/note_index.go index b849b033..4adcdfc6 100644 --- a/internal/adapter/sqlite/note_index.go +++ b/internal/adapter/sqlite/note_index.go @@ -31,6 +31,12 @@ type dao struct { } func NewNoteIndex(notebookPath string, db *DB, logger util.Logger, extension string) *NoteIndex { + if notebookPath == "" { + notebookPath = "." + } + if abs, err := filepath.Abs(notebookPath); err == nil { + notebookPath = abs + } return &NoteIndex{ notebookPath: notebookPath, db: db, @@ -71,19 +77,30 @@ func (ni *NoteIndex) findLinkMatch(dao *dao, baseDir string, href string, linkTy return 0, nil } + // 1. Try relative to the current file (baseDir) id, _ := ni.findPathMatch(dao, baseDir, href) if id.IsValid() { return id, nil } - allowPartialMatch := (linkType == core.LinkTypeWikiLink) - return dao.notes.FindIDByHref(href, allowPartialMatch) + // 2. Try relative to vault root + id, _ = dao.notes.FindIDByHref(href, false) + if id.IsValid() { + return id, nil + } + + // 3. Fallback to partial matching for both link types + id, _ = dao.notes.FindIDByHref(href, true) + return id, nil } 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 + if baseDir != "" { + var err error + href, err = ni.relNotebookPath(baseDir, href) + if err != nil { + return 0, err + } } return dao.notes.FindIDByHref(href, false) } @@ -127,7 +144,7 @@ func (ni *NoteIndex) Add(note core.Note) (id core.NoteID, err error) { } note.ID = id - err = ni.addLinks(dao, id, note.Links) + err = ni.addLinks(dao, id, note.Path, note.Links) if err != nil { return err } @@ -156,24 +173,75 @@ func (ni *NoteIndex) fixExistingLinks(dao *dao, id core.NoteID, path string) err } for _, link := range links { - // To find the best match possible, shortest paths take precedence. - // See https://github.com/zk-org/zk/issues/23 - if link.TargetPath != "" && len(link.TargetPath) < len(path) { + matches, err := ni.linkMatchesPath(link, path) + if err != nil || !matches { continue } - matches, err := ni.linkMatchesPath(link, path) - if matches && err == nil { + // If the link has no target yet, use the current match. + if link.TargetID == 0 { err = dao.links.SetTargetID(link.ID, id) + if err != nil { + return err + } + continue } - if err != nil { - return err + + // If the link already has a target, only update if the new match is better. + newPriority := ni.linkMatchPriority(link, path) + currentPriority := ni.linkMatchPriority(link, link.TargetPath) + + if newPriority < currentPriority || (newPriority == currentPriority && len(path) < len(link.TargetPath)) { + err = dao.links.SetTargetID(link.ID, id) + if err != nil { + return err + } } } return nil } +// linkMatchPriority returns the priority level of a match (1, 2 or 3). +// Lower number means higher priority. +func (ni *NoteIndex) linkMatchPriority(link core.ResolvedLink, path string) int { + // 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] + + matches := func(h string) bool { + h = strings.TrimPrefix(h, "/") + h = filepath.Clean(h) + if h == "." { + h = "" + } + if h == path { + return true + } + // Try with extension if h doesn't have it + if h != "" && h+"."+ni.extension == path { + return true + } + return false + } + + // 1. Match relative to the current file. + baseDir := filepath.Dir(link.SourcePath) + if relHref, err := ni.relNotebookPath(baseDir, href); err == nil { + if matches(relHref) { + return 1 + } + } + + // 2. Exact match relative to the vault root. + if matches(href) { + return 2 + } + + // 3. Fallback to partial path matching. + return 3 +} + // 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) { @@ -204,13 +272,20 @@ func (ni *NoteIndex) linkMatchesPath(link core.ResolvedLink, path string) (bool, return matchString("^(?:"+href+"[^/]*|"+href+"/.+)$", path) } - baseDir := filepath.Join(ni.notebookPath, filepath.Dir(link.SourcePath)) - if relHref, err := ni.relNotebookPath(baseDir, href); err != nil { + // 1. Check if it matches relative to the source note. + baseDir := filepath.Dir(link.SourcePath) + if relHref, err := ni.relNotebookPath(baseDir, href); err == nil { if matches(relHref, false) { return true, nil } } + // 2. Check if it matches relative to the vault root. + if matches(href, false) { + return true, nil + } + + // 3. Fallback to partial match. allowPartialMatch := (link.Type == core.LinkTypeWikiLink) return matches(href, allowPartialMatch), nil } @@ -218,6 +293,12 @@ func (ni *NoteIndex) linkMatchesPath(link core.ResolvedLink, path string) (bool, // 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) { + if strings.HasPrefix(href, "/") { + return filepath.Clean(href[1:]), nil + } + if !filepath.IsAbs(baseDir) { + baseDir = filepath.Join(ni.notebookPath, baseDir) + } path := filepath.Clean(filepath.Join(baseDir, href)) path, err := filepath.Rel(ni.notebookPath, path) @@ -240,7 +321,7 @@ func (ni *NoteIndex) Update(note core.Note) error { if err != nil { return err } - err = ni.addLinks(dao, id, note.Links) + err = ni.addLinks(dao, id, note.Path, note.Links) if err != nil { return err } @@ -274,19 +355,20 @@ func (ni *NoteIndex) associateTags(collections *CollectionDAO, noteID core.NoteI return nil } -func (ni *NoteIndex) addLinks(dao *dao, id core.NoteID, links []core.Link) error { - resolvedLinks, err := ni.resolveLinkNoteIDs(dao, id, links) +func (ni *NoteIndex) addLinks(dao *dao, id core.NoteID, path string, links []core.Link) error { + resolvedLinks, err := ni.resolveLinkNoteIDs(dao, id, path, links) if err != nil { return err } return dao.links.Add(resolvedLinks) } -func (ni *NoteIndex) resolveLinkNoteIDs(dao *dao, sourceID core.NoteID, links []core.Link) ([]core.ResolvedLink, error) { +func (ni *NoteIndex) resolveLinkNoteIDs(dao *dao, sourceID core.NoteID, path string, links []core.Link) ([]core.ResolvedLink, error) { resolvedLinks := []core.ResolvedLink{} + baseDir := filepath.Dir(path) for _, link := range links { - targetID, err := ni.findLinkMatch(dao, "" /* base dir */, link.Href, link.Type) + targetID, err := ni.findLinkMatch(dao, baseDir, link.Href, link.Type) if err != nil { return resolvedLinks, err } diff --git a/internal/adapter/sqlite/util.go b/internal/adapter/sqlite/util.go index 22f0f96b..c089cd46 100644 --- a/internal/adapter/sqlite/util.go +++ b/internal/adapter/sqlite/util.go @@ -55,6 +55,14 @@ func joinNoteIDs(ids []core.NoteID, delimiter string) string { return strings.Join(strs, delimiter) } +func joinStrings(strs []string, sep string, quote string) string { + res := make([]string, len(strs)) + for i, s := range strs { + res[i] = quote + s + quote + } + return strings.Join(res, sep) +} + func unmarshalMetadata(metadataJSON string) (metadata map[string]any, err error) { err = json.Unmarshal([]byte(metadataJSON), &metadata) if err != nil { diff --git a/internal/core/note_parse.go b/internal/core/note_parse.go index c1392ae8..efac176d 100644 --- a/internal/core/note_parse.go +++ b/internal/core/note_parse.go @@ -3,13 +3,11 @@ package core import ( "crypto/sha256" "fmt" - "path/filepath" "strings" "time" "github.com/relvacode/iso8601" "github.com/zk-org/zk/internal/util/opt" - strutil "github.com/zk-org/zk/internal/util/strings" "gopkg.in/djherbis/times.v1" ) @@ -75,15 +73,6 @@ func (n *Notebook) ParseNoteWithContent(absPath string, content []byte) (*Note, } for _, link := range contentParts.Links { - if !strutil.IsURL(link.Href) && link.Type == LinkTypeMarkdown { - // Make the href relative to the notebook root. - href := filepath.Join(filepath.Dir(absPath), link.Href) - link.Href, err = n.RelPath(href) - if err != nil { - n.logger.Err(err) - continue - } - } note.Links = append(note.Links, link) } diff --git a/tests/cmd-list-filter-link.tesh b/tests/cmd-list-filter-link.tesh index cce4c6a8..1998c47b 100644 --- a/tests/cmd-list-filter-link.tesh +++ b/tests/cmd-list-filter-link.tesh @@ -10,6 +10,10 @@ $ zk list --debug-style -q --link-to 4oma.md > > - [Channel](fwsj) for a safe [message passing](4oma) approach. > +>Do not communicate by sharing memory; instead, share memory by communicating ref/7fto.md (just now) +> +> - Advocates for the use of [message passing](4oma) instead of shared state. +> # Follow backlinks of "Message passing" recursively. # FIXME: The snippets don't seem right. @@ -18,6 +22,10 @@ $ zk list --debug-style -q --link-to 4oma.md --recursive > > - It's an implementation of the [message passing](4oma) pattern. > +>Do not communicate by sharing memory; instead, share memory by communicating ref/7fto.md (just now) +> +> - Advocates for the use of [message passing](4oma) instead of shared state. +> >Concurrency in Rust g7qa.md (just now) > > - It's an implementation of the [message passing](4oma) pattern. @@ -38,6 +46,10 @@ $ zk list --debug-style -q --link-to 4oma.md --recursive --max-distance 1 > > - [Channel](fwsj) for a safe [message passing](4oma) approach. > +>Do not communicate by sharing memory; instead, share memory by communicating ref/7fto.md (just now) +> +> - Advocates for the use of [message passing](4oma) instead of shared state. +> # Find notes linked by "Mutex". # The snippet is an extract from the Mutex note. diff --git a/tests/fixtures/robust-links/.zk/config.toml b/tests/fixtures/robust-links/.zk/config.toml new file mode 100644 index 00000000..6cdf29aa --- /dev/null +++ b/tests/fixtures/robust-links/.zk/config.toml @@ -0,0 +1,2 @@ +[note] +extension = "md" diff --git a/tests/fixtures/robust-links/root-note.md b/tests/fixtures/robust-links/root-note.md new file mode 100644 index 00000000..10fe7440 --- /dev/null +++ b/tests/fixtures/robust-links/root-note.md @@ -0,0 +1 @@ +# Root Note diff --git a/tests/fixtures/robust-links/sub/another-note.md b/tests/fixtures/robust-links/sub/another-note.md new file mode 100644 index 00000000..1211d8d6 --- /dev/null +++ b/tests/fixtures/robust-links/sub/another-note.md @@ -0,0 +1 @@ +# Another Note diff --git a/tests/fixtures/robust-links/sub/priority-test.md b/tests/fixtures/robust-links/sub/priority-test.md new file mode 100644 index 00000000..ea290971 --- /dev/null +++ b/tests/fixtures/robust-links/sub/priority-test.md @@ -0,0 +1,2 @@ +# Priority Test +This links to [target](target.md) and [[target]]. diff --git a/tests/fixtures/robust-links/sub/sub-note.md b/tests/fixtures/robust-links/sub/sub-note.md new file mode 100644 index 00000000..91cb4115 --- /dev/null +++ b/tests/fixtures/robust-links/sub/sub-note.md @@ -0,0 +1,7 @@ +# Sub Note +Links: +- [[root-note]] +- [markdown root](../root-note.md) +- [[another-note]] +- [markdown same dir](another-note.md) +- [markdown root absolute](/root-note.md) diff --git a/tests/fixtures/robust-links/sub/target.md b/tests/fixtures/robust-links/sub/target.md new file mode 100644 index 00000000..1b476bdb --- /dev/null +++ b/tests/fixtures/robust-links/sub/target.md @@ -0,0 +1 @@ +# Target in Sub diff --git a/tests/fixtures/robust-links/target.md b/tests/fixtures/robust-links/target.md new file mode 100644 index 00000000..cd1102b5 --- /dev/null +++ b/tests/fixtures/robust-links/target.md @@ -0,0 +1 @@ +# Target at Root diff --git a/tests/robust-links.tesh b/tests/robust-links.tesh new file mode 100644 index 00000000..4453aecd --- /dev/null +++ b/tests/robust-links.tesh @@ -0,0 +1,42 @@ +$ cd robust-links + +# Check if all notes are indexed +$ zk list -s path -qf'\{{path}}' +>root-note.md +>sub/another-note.md +>sub/priority-test.md +>sub/sub-note.md +>sub/target.md +>target.md + +# Test links in sub/sub-note.md +# [[root-note]] +$ zk list -s path -qf'\{{path}}' --link-to root-note.md +>sub/sub-note.md + +# [markdown root](../root-note.md) +$ zk list -s path -qf'\{{path}}' --link-to root-note.md +>sub/sub-note.md + +# [[another-note]] +$ zk list -s path -qf'\{{path}}' --link-to sub/another-note.md +>sub/sub-note.md + +# [markdown same dir](another-note.md) +$ zk list -s path -qf'\{{path}}' --link-to sub/another-note.md +>sub/sub-note.md + +# Priority Test +# sub/priority-test.md links to [target](target.md) and [[target]] +# They should both point to sub/target.md, NOT target.md (root) +$ zk list -s path -qf'\{{path}}' --linked-by sub/priority-test.md +>sub/target.md + +# Verify that root target.md is NOT linked by priority-test.md +$ zk list -s path -qf'\{{path}}' --link-to target.md +> + +# Root absolute links +# [markdown root absolute](/root-note.md) +$ zk list -s path -qf'\{{path}}' --link-to root-note.md +>sub/sub-note.md