From 83e09169a0dc5c395a3923400804374802d8d4ae Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Tue, 6 May 2025 17:56:47 +0200 Subject: [PATCH 1/2] Refactor ImageBlock URL handling and file resolution logic. Streamline URL updates and file resolution by leveraging `DiagnosticLinkInlineParser`. This reduces redundancy and ensures consistent handling of relative URLs. Improved error messaging for nonexistent files. Fixes #1134 --- .../Myst/Directives/DirectiveHtmlRenderer.cs | 19 +--- .../Myst/Directives/ImageBlock.cs | 12 +-- .../DiagnosticLinkInlineParser.cs | 91 ++++++++++--------- tests/authoring/Blocks/ImageBlocks.fs | 81 +++++++++++++++++ tests/authoring/Container/DefinitionLists.fs | 1 + tests/authoring/Framework/Setup.fs | 28 ++++-- tests/authoring/authoring.fsproj | 1 + 7 files changed, 159 insertions(+), 74 deletions(-) create mode 100644 tests/authoring/Blocks/ImageBlocks.fs diff --git a/src/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cs b/src/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cs index 4cca0048d..7f06fd39b 100644 --- a/src/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cs +++ b/src/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cs @@ -4,6 +4,7 @@ using System.Diagnostics.CodeAnalysis; using Elastic.Markdown.Diagnostics; +using Elastic.Markdown.Myst.InlineParsers; using Elastic.Markdown.Myst.InlineParsers.Substitution; using Elastic.Markdown.Myst.Settings; using Elastic.Markdown.Slices.Directives; @@ -85,23 +86,7 @@ protected override void Write(HtmlRenderer renderer, DirectiveBlock directiveBlo private static void WriteImage(HtmlRenderer renderer, ImageBlock block) { var imageUrl = block.ImageUrl; - if (!string.IsNullOrEmpty(block.ImageUrl)) - { - if (block.ImageUrl.StartsWith('/') || block.ImageUrl.StartsWith("_static")) - imageUrl = $"{block.Build.UrlPathPrefix}/{block.ImageUrl.TrimStart('/')}"; - else - { - // `block.Build.ConfigurationPath.DirectoryName` is the directory of the docset.yml file - // which is the root of the documentation source - // e.g. `/User/username/Projects/docs-builder/docs` - // `block.CurrentFile.DirectoryName` is the directory of the current markdown file where the image is referenced - // e.g. `/User/username/Projects/docs-builder/docs/page/with/image` - // `Path.GetRelativePath` will return the relative path to the docset.yml directory - // e.g. `page/with/image` - // Hence the full imageUrl will be something like /path-prefix/page/with/image/image.png - imageUrl = block.Build.UrlPathPrefix + '/' + Path.GetRelativePath(block.Build.ConfigurationPath.DirectoryName!, block.CurrentFile.DirectoryName!) + "/" + block.ImageUrl; - } - } + var slice = Image.Create(new ImageViewModel { Label = block.Label, diff --git a/src/Elastic.Markdown/Myst/Directives/ImageBlock.cs b/src/Elastic.Markdown/Myst/Directives/ImageBlock.cs index 8883fec0e..448d7fab7 100644 --- a/src/Elastic.Markdown/Myst/Directives/ImageBlock.cs +++ b/src/Elastic.Markdown/Myst/Directives/ImageBlock.cs @@ -4,6 +4,7 @@ using Elastic.Markdown.Diagnostics; using Elastic.Markdown.IO; +using Elastic.Markdown.Myst.InlineParsers; namespace Elastic.Markdown.Myst.Directives; @@ -96,17 +97,14 @@ private void ExtractImageUrl(ParserContext context) return; } - var includeFrom = context.MarkdownSourcePath.Directory!.FullName; - if (imageUrl.StartsWith('/')) - includeFrom = context.Build.DocumentationSourceDirectory.FullName; + ImageUrl = DiagnosticLinkInlineParser.UpdateRelativeUrl(context, imageUrl); - ImageUrl = imageUrl; - var imagePath = Path.Combine(includeFrom, imageUrl.TrimStart('/')); - var file = context.Build.ReadFileSystem.FileInfo.New(imagePath); + + var file = DiagnosticLinkInlineParser.ResolveFile(context, imageUrl); if (file.Exists) Found = true; else - this.EmitError($"`{imageUrl}` does not exist. resolved to `{imagePath}"); + this.EmitError($"`{imageUrl}` does not exist. resolved to `{file}"); if (context.DocumentationFileLookup(context.MarkdownSourcePath) is MarkdownFile currentMarkdown) { diff --git a/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs b/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs index 6e7ee5c19..264945a6c 100644 --- a/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs +++ b/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs @@ -279,7 +279,7 @@ private static void ProcessLinkText(InlineProcessor processor, LinkInline link, _ = link.AppendChild(new LiteralInline(title)); } - private static IFileInfo ResolveFile(ParserContext context, string url) => + public static IFileInfo ResolveFile(ParserContext context, string url) => string.IsNullOrWhiteSpace(url) ? context.MarkdownSourcePath : url.StartsWith('/') @@ -302,49 +302,8 @@ private static void UpdateLinkUrl(LinkInline link, MarkdownFile? linkMarkdown, s newUrl = linkMarkdown.Url; } else - { - // TODO revisit when we refactor our documentation set graph - // This method grew too complex, we need to revisit our documentation set graph generation so we can ask these questions - // on `DocumentationFile` that are mostly precomputed - var urlPathPrefix = context.Build.UrlPathPrefix ?? string.Empty; + newUrl = UpdateRelativeUrl(context, url); - if (!newUrl.StartsWith('/') && !string.IsNullOrEmpty(newUrl)) - { - // eat overall path prefix since its gets appended later - var subPrefix = context.CurrentUrlPath.Length >= urlPathPrefix.Length - ? context.CurrentUrlPath[urlPathPrefix.Length..] - : urlPathPrefix; - - // if we are trying to resolve a relative url from a _snippet folder ensure we eat the _snippet folder - // as it's not part of url by chopping of the extra parent navigation - if (newUrl.StartsWith("../") && context.DocumentationFileLookup(context.MarkdownSourcePath) is SnippetFile snippetFile) - newUrl = url.Substring(3); - - // TODO check through context.DocumentationFileLookup if file is index vs "index.md" check - var markdownPath = context.MarkdownSourcePath; - // if the current path is an index e.g /reference/cloud-k8s/ - // './' current path lookups should be relative to sub-path. - // If it's not e.g /reference/cloud-k8s/api-docs/ these links should resolve on folder up. - var lastIndexPath = subPrefix.LastIndexOf('/'); - if (lastIndexPath >= 0 && markdownPath.Name != "index.md") - subPrefix = subPrefix[..lastIndexPath]; - var combined = '/' + Path.Combine(subPrefix, newUrl).TrimStart('/'); - newUrl = Path.GetFullPath(combined); - } - - // When running on Windows, path traversal results must be normalized prior to being used in a URL - // Path.GetFullPath() will result in the drive letter being appended to the path, which needs to be pruned back. - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - newUrl = newUrl.Replace('\\', '/'); - if (newUrl.Length > 2 && newUrl[1] == ':') - newUrl = newUrl[2..]; - } - - if (!string.IsNullOrWhiteSpace(newUrl) && !string.IsNullOrWhiteSpace(urlPathPrefix)) - newUrl = $"{urlPathPrefix.TrimEnd('/')}{newUrl}"; - - } if (newUrl.EndsWith(".md")) { @@ -364,6 +323,52 @@ private static void UpdateLinkUrl(LinkInline link, MarkdownFile? linkMarkdown, s : newUrl; } + // TODO revisit when we refactor our documentation set graph + // This method grew too complex, we need to revisit our documentation set graph generation so we can ask these questions + // on `DocumentationFile` that are mostly precomputed + public static string UpdateRelativeUrl(ParserContext context, string url) + { + var urlPathPrefix = context.Build.UrlPathPrefix ?? string.Empty; + var newUrl = url; + if (newUrl.StartsWith('/') || string.IsNullOrEmpty(newUrl)) + return newUrl; + + // eat overall path prefix since its gets appended later + var subPrefix = context.CurrentUrlPath.Length >= urlPathPrefix.Length + ? context.CurrentUrlPath[urlPathPrefix.Length..] + : urlPathPrefix; + + // if we are trying to resolve a relative url from a _snippet folder ensure we eat the _snippet folder + // as it's not part of url by chopping of the extra parent navigation + if (newUrl.StartsWith("../") && context.DocumentationFileLookup(context.MarkdownSourcePath) is SnippetFile) + newUrl = url[3..]; + + // TODO check through context.DocumentationFileLookup if file is index vs "index.md" check + var markdownPath = context.MarkdownSourcePath; + // if the current path is an index e.g /reference/cloud-k8s/ + // './' current path lookups should be relative to sub-path. + // If it's not e.g /reference/cloud-k8s/api-docs/ these links should resolve on folder up. + var lastIndexPath = subPrefix.LastIndexOf('/'); + if (lastIndexPath >= 0 && markdownPath.Name != "index.md") + subPrefix = subPrefix[..lastIndexPath]; + var combined = '/' + Path.Combine(subPrefix, newUrl).TrimStart('/'); + newUrl = Path.GetFullPath(combined); + + // When running on Windows, path traversal results must be normalized prior to being used in a URL + // Path.GetFullPath() will result in the drive letter being appended to the path, which needs to be pruned back. + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + newUrl = newUrl.Replace('\\', '/'); + if (newUrl.Length > 2 && newUrl[1] == ':') + newUrl = newUrl[2..]; + } + + if (!string.IsNullOrWhiteSpace(newUrl) && !string.IsNullOrWhiteSpace(urlPathPrefix)) + newUrl = $"{urlPathPrefix.TrimEnd('/')}{newUrl}"; + + return newUrl; + } + private static bool IsCrossLink([NotNullWhen(true)] Uri? uri) => uri != null // This means it's not a local && !ExcludedSchemes.Contains(uri.Scheme) diff --git a/tests/authoring/Blocks/ImageBlocks.fs b/tests/authoring/Blocks/ImageBlocks.fs new file mode 100644 index 000000000..ad492f68a --- /dev/null +++ b/tests/authoring/Blocks/ImageBlocks.fs @@ -0,0 +1,81 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// Elasticsearch B.V licenses this file to you under the Apache 2.0 License. +// See the LICENSE file in the project root for more information +module ``block elements``.``image blocks`` + +open Xunit +open authoring + +type ``static path to image`` () = + static let markdown = Setup.Markdown """ +:::{image} img/observability.png +:alt: Elasticsearch +:width: 250px +:screenshot: +::: +""" + + [] + let ``validate src is anchored`` () = + markdown |> convertsToContainingHtml """ + Elasticsearch + """ + +type ``supports --url-path-prefix`` () = + static let docs = Setup.GenerateWithOptions { UrlPathPrefix = Some "/docs" } [ + Static "img/observability.png" + + Index """# Testing nested inline anchors +:::{image} img/observability.png +:alt: Elasticsearch +:width: 250px +:screenshot: +::: +""" + + Markdown "folder/relative.md" """ +:::{image} ../img/observability.png +:alt: Elasticsearch +:width: 250px +:screenshot: +::: + """ + ] + + [] + let ``validate image src contains prefix`` () = + docs |> convertsToContainingHtml """ + Elasticsearch + """ + + [] + let ``validate image src contains prefix when referenced relatively`` () = + docs |> converts "folder/relative.md" |> containsHtml """ + Elasticsearch + """ + + [] + let ``has no errors`` () = docs |> hasNoErrors + +type ``image ref out of scope`` () = + static let docs = Setup.GenerateWithOptions { UrlPathPrefix = Some "/docs" } [ + Static "img/observability.png" + + Index """# Testing nested inline anchors +:::{image} ../img/observability.png +:alt: Elasticsearch +:width: 250px +:screenshot: +::: +""" + ] + + [] + let ``validate image src contains prefix and is anchored to documentation scope root`` () = + docs |> convertsToContainingHtml """ + Elasticsearch + """ + + [] + let ``emits an error image reference is outside of documentation scope`` () = + docs |> hasError "./img/observability.png` does not exist. resolved to" diff --git a/tests/authoring/Container/DefinitionLists.fs b/tests/authoring/Container/DefinitionLists.fs index dd8cee990..389f019d1 100644 --- a/tests/authoring/Container/DefinitionLists.fs +++ b/tests/authoring/Container/DefinitionLists.fs @@ -67,6 +67,7 @@ This is my `definition` """ + [] let ``has no errors 2`` () = markdown |> hasNoErrors diff --git a/tests/authoring/Framework/Setup.fs b/tests/authoring/Framework/Setup.fs index 682091fa8..66df0e955 100644 --- a/tests/authoring/Framework/Setup.fs +++ b/tests/authoring/Framework/Setup.fs @@ -25,6 +25,7 @@ type TestFile = | File of name: string * contents: string | MarkdownFile of name: string * markdown: Markdown | SnippetFile of name: string * markdown: Markdown + | StaticFile of name: string static member Index ([] m) = MarkdownFile("index.md" , m) @@ -32,9 +33,17 @@ type TestFile = static member Markdown path ([] m) = MarkdownFile(path , m) + static member Static path = StaticFile(path) + static member Snippet path ([] m) = SnippetFile(path , m) +type SetupOptions = + { UrlPathPrefix: string option } + static member Empty = { + UrlPathPrefix = None + } + type Setup = static let GenerateDocSetYaml( @@ -61,7 +70,7 @@ type Setup = yaml.WriteLine($" - file: {relative}") let fullPath = Path.Combine(root.FullName, relative) let contents = File.ReadAllText fullPath - fileSystem.AddFile(fullPath, new MockFileData(contents)) + fileSystem.AddFile(fullPath, MockFileData(contents)) ) match globalVariables with @@ -79,7 +88,8 @@ type Setup = let redirectYaml = File.ReadAllText(Path.Combine(root.FullName, "_redirects.yml")) fileSystem.AddFile(Path.Combine(root.FullName, redirectsName), MockFileData(redirectYaml)) - static member Generator (files: TestFile seq) : Task = + static member Generator (files: TestFile seq) (options: SetupOptions option) : Task = + let options = options |> Option.defaultValue SetupOptions.Empty let d = files |> Seq.map (fun f -> @@ -87,6 +97,7 @@ type Setup = | File(name, contents) -> ($"docs/{name}", MockFileData(contents)) | SnippetFile(name, markdown) -> ($"docs/{name}", MockFileData(markdown)) | MarkdownFile(name, markdown) -> ($"docs/{name}", MockFileData(markdown)) + | StaticFile(name) -> ($"docs/{name}", MockFileData("")) ) |> Map.ofSeq @@ -95,8 +106,8 @@ type Setup = GenerateDocSetYaml (fileSystem, None) - let collector = TestDiagnosticsCollector(); - let context = BuildContext(collector, fileSystem) + let collector = TestDiagnosticsCollector() + let context = BuildContext(collector, fileSystem, UrlPathPrefix=(options.UrlPathPrefix |> Option.defaultValue "")) let logger = new TestLoggerFactory() let conversionCollector = TestConversionCollector() let linkResolver = TestCrossLinkResolver(context.Configuration) @@ -115,11 +126,14 @@ type Setup = /// Pass several files to the test setup static member Generate files = - lazy (task { return! Setup.Generator files } |> Async.AwaitTask |> Async.RunSynchronously) + lazy (task { return! Setup.Generator files None } |> Async.AwaitTask |> Async.RunSynchronously) + + static member GenerateWithOptions options files = + lazy (task { return! Setup.Generator files (Some options) } |> Async.AwaitTask |> Async.RunSynchronously) /// Pass a full documentation page to the test setup static member Document ([]m: string) = - lazy (task { return! Setup.Generator [Index m] } |> Async.AwaitTask |> Async.RunSynchronously) + lazy (task { return! Setup.Generator [Index m] None } |> Async.AwaitTask |> Async.RunSynchronously) /// Pass a markdown fragment to the test setup static member Markdown ([]m: string) = @@ -128,6 +142,6 @@ type Setup = {m} """ lazy ( - task { return! Setup.Generator [Index m] } + task { return! Setup.Generator [Index m] None } |> Async.AwaitTask |> Async.RunSynchronously ) \ No newline at end of file diff --git a/tests/authoring/authoring.fsproj b/tests/authoring/authoring.fsproj index 7003f0aaa..aa289193f 100644 --- a/tests/authoring/authoring.fsproj +++ b/tests/authoring/authoring.fsproj @@ -52,6 +52,7 @@ + From 36f718ec0f79f5fe2e34dcb825676242e249b8e0 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Tue, 6 May 2025 18:07:14 +0200 Subject: [PATCH 2/2] restore old logic that got rewritten --- .../DiagnosticLinkInlineParser.cs | 46 +++++++++---------- .../Directives/ImageTests.cs | 2 +- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs b/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs index 264945a6c..f236ad4ed 100644 --- a/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs +++ b/src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs @@ -330,30 +330,29 @@ public static string UpdateRelativeUrl(ParserContext context, string url) { var urlPathPrefix = context.Build.UrlPathPrefix ?? string.Empty; var newUrl = url; - if (newUrl.StartsWith('/') || string.IsNullOrEmpty(newUrl)) - return newUrl; - - // eat overall path prefix since its gets appended later - var subPrefix = context.CurrentUrlPath.Length >= urlPathPrefix.Length - ? context.CurrentUrlPath[urlPathPrefix.Length..] - : urlPathPrefix; - - // if we are trying to resolve a relative url from a _snippet folder ensure we eat the _snippet folder - // as it's not part of url by chopping of the extra parent navigation - if (newUrl.StartsWith("../") && context.DocumentationFileLookup(context.MarkdownSourcePath) is SnippetFile) - newUrl = url[3..]; - - // TODO check through context.DocumentationFileLookup if file is index vs "index.md" check - var markdownPath = context.MarkdownSourcePath; - // if the current path is an index e.g /reference/cloud-k8s/ - // './' current path lookups should be relative to sub-path. - // If it's not e.g /reference/cloud-k8s/api-docs/ these links should resolve on folder up. - var lastIndexPath = subPrefix.LastIndexOf('/'); - if (lastIndexPath >= 0 && markdownPath.Name != "index.md") - subPrefix = subPrefix[..lastIndexPath]; - var combined = '/' + Path.Combine(subPrefix, newUrl).TrimStart('/'); - newUrl = Path.GetFullPath(combined); + if (!newUrl.StartsWith('/') && !string.IsNullOrEmpty(newUrl)) + { + var subPrefix = context.CurrentUrlPath.Length >= urlPathPrefix.Length + ? context.CurrentUrlPath[urlPathPrefix.Length..] + : urlPathPrefix; + + // if we are trying to resolve a relative url from a _snippet folder ensure we eat the _snippet folder + // as it's not part of url by chopping of the extra parent navigation + if (newUrl.StartsWith("../") && context.DocumentationFileLookup(context.MarkdownSourcePath) is SnippetFile) + newUrl = url[3..]; + + // TODO check through context.DocumentationFileLookup if file is index vs "index.md" check + var markdownPath = context.MarkdownSourcePath; + // if the current path is an index e.g /reference/cloud-k8s/ + // './' current path lookups should be relative to sub-path. + // If it's not e.g /reference/cloud-k8s/api-docs/ these links should resolve on folder up. + var lastIndexPath = subPrefix.LastIndexOf('/'); + if (lastIndexPath >= 0 && markdownPath.Name != "index.md") + subPrefix = subPrefix[..lastIndexPath]; + var combined = '/' + Path.Combine(subPrefix, newUrl).TrimStart('/'); + newUrl = Path.GetFullPath(combined); + } // When running on Windows, path traversal results must be normalized prior to being used in a URL // Path.GetFullPath() will result in the drive letter being appended to the path, which needs to be pruned back. if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) @@ -366,6 +365,7 @@ public static string UpdateRelativeUrl(ParserContext context, string url) if (!string.IsNullOrWhiteSpace(newUrl) && !string.IsNullOrWhiteSpace(urlPathPrefix)) newUrl = $"{urlPathPrefix.TrimEnd('/')}{newUrl}"; + // eat overall path prefix since its gets appended later return newUrl; } diff --git a/tests/Elastic.Markdown.Tests/Directives/ImageTests.cs b/tests/Elastic.Markdown.Tests/Directives/ImageTests.cs index 673e3eb9d..475675b09 100644 --- a/tests/Elastic.Markdown.Tests/Directives/ImageTests.cs +++ b/tests/Elastic.Markdown.Tests/Directives/ImageTests.cs @@ -31,7 +31,7 @@ public void ParsesBreakPoint() { Block!.Alt.Should().Be("Elasticsearch"); Block!.Width.Should().Be("250px"); - Block!.ImageUrl.Should().Be("img/observability.png"); + Block!.ImageUrl.Should().Be("/img/observability.png"); Block!.Screenshot.Should().Be("screenshot"); }