Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/adapter/lsp/document.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ var documentParser = goldmark.New(
goldmark.WithExtensions(
gmext.Footnote,
extensions.WikiLinkExt,
extensions.MarkdownLinkWithSpacesExt,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only just got rid of our own custom markdown parser: it had way too many edge cases which were broken, and each time we fixed one, another showed up.

I'm very hesitant to re-introduce a custom parser for markdown links.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is very understandable! Actually, I did not know this, but the commonmark standard allows for spaces in markdown links if wrapped with <>. E.g. [Link name](<projects/Research Paper/review.md>)

So there is no need for this PR. But maybe this would be something worth adding to the documentation on links?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spec explicitly allows for it, and the parser is rejecting it, we should report this to https://github.com/yuin/goldmark. The author is quite responsive.

),
)

Expand Down
31 changes: 24 additions & 7 deletions internal/adapter/lsp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
75 changes: 75 additions & 0 deletions internal/adapter/lsp/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
64 changes: 64 additions & 0 deletions internal/adapter/markdown/extensions/link_spaces.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package extensions

import (
"regexp"

"github.com/yuin/goldmark"
"github.com/yuin/goldmark/ast"
"github.com/yuin/goldmark/parser"
"github.com/yuin/goldmark/text"
"github.com/yuin/goldmark/util"
)

// MarkdownLinkWithSpacesExt is an extension parsing Markdown links with spaces in the destination.
// For example, [label](file name.md).
var MarkdownLinkWithSpacesExt = &markdownLinkWithSpaces{}

type markdownLinkWithSpaces struct{}

func (e *markdownLinkWithSpaces) Extend(m goldmark.Markdown) {
m.Parser().AddOptions(
parser.WithInlineParsers(
util.Prioritized(&mlwsParser{}, 199),
),
)
}

type mlwsParser struct{}

func (p *mlwsParser) Trigger() []byte {
return []byte{'['}
}

var mlwsRegex = regexp.MustCompile(`^\[([^\]]+)\]\(\s*([^)"]*?)\s*(?:\s+"([^"]+)")?\s*\)`)

func (p *mlwsParser) Parse(parent ast.Node, block text.Reader, pc parser.Context) ast.Node {
line, segment := block.PeekLine()
match := mlwsRegex.FindSubmatch(line)
if match == nil {
return nil
}

label := match[1]
destination := match[2]
var title []byte
if len(match) > 3 {
title = match[3]
}

block.Advance(len(match[0]))

link := ast.NewLink()
link.Destination = destination
if len(title) > 0 {
link.Title = title
}

// Use ast.Text so that LinkPosition can find the text segments
txt := ast.NewText()
start := segment.Start + 1
txt.Segment = text.NewSegment(start, start+len(label))
link.AppendChild(link, txt)

return link
}
1 change: 1 addition & 0 deletions internal/adapter/markdown/markdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ func NewParser(options ParserOpts, logger util.Logger) *Parser {
),
),
extensions.WikiLinkExt,
extensions.MarkdownLinkWithSpacesExt,
&extensions.TagExt{
HashtagEnabled: options.HashtagEnabled,
MultiWordTagEnabled: options.MultiWordTagEnabled,
Expand Down
53 changes: 39 additions & 14 deletions internal/adapter/sqlite/note_dao.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -528,17 +545,25 @@ 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",
linksSrc, idsList,
))
}
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)"
Expand Down
Loading