Skip to content

Commit 5bae611

Browse files
MpdreamzMpdreamz
authored
Refactor ImageBlock URL handling and file resolution logic. (#1219)
* 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 * restore old logic that got rewritten
1 parent a9c0a08 commit 5bae611

File tree

8 files changed

+160
-75
lines changed

8 files changed

+160
-75
lines changed

‎src/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cs‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
using System.Diagnostics.CodeAnalysis;
66
using Elastic.Markdown.Diagnostics;
7+
using Elastic.Markdown.Myst.InlineParsers;
78
using Elastic.Markdown.Myst.InlineParsers.Substitution;
89
using Elastic.Markdown.Myst.Settings;
910
using Elastic.Markdown.Slices.Directives;
@@ -85,23 +86,7 @@ protected override void Write(HtmlRenderer renderer, DirectiveBlock directiveBlo
8586
private static void WriteImage(HtmlRenderer renderer, ImageBlock block)
8687
{
8788
var imageUrl = block.ImageUrl;
88-
if (!string.IsNullOrEmpty(block.ImageUrl))
89-
{
90-
if (block.ImageUrl.StartsWith('/') || block.ImageUrl.StartsWith("_static"))
91-
imageUrl = $"{block.Build.UrlPathPrefix}/{block.ImageUrl.TrimStart('/')}";
92-
else
93-
{
94-
// `block.Build.ConfigurationPath.DirectoryName` is the directory of the docset.yml file
95-
// which is the root of the documentation source
96-
// e.g. `/User/username/Projects/docs-builder/docs`
97-
// `block.CurrentFile.DirectoryName` is the directory of the current markdown file where the image is referenced
98-
// e.g. `/User/username/Projects/docs-builder/docs/page/with/image`
99-
// `Path.GetRelativePath` will return the relative path to the docset.yml directory
100-
// e.g. `page/with/image`
101-
// Hence the full imageUrl will be something like /path-prefix/page/with/image/image.png
102-
imageUrl = block.Build.UrlPathPrefix + '/' + Path.GetRelativePath(block.Build.ConfigurationPath.DirectoryName!, block.CurrentFile.DirectoryName!) + "/" + block.ImageUrl;
103-
}
104-
}
89+
10590
var slice = Image.Create(new ImageViewModel
10691
{
10792
Label = block.Label,

‎src/Elastic.Markdown/Myst/Directives/ImageBlock.cs‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
using Elastic.Markdown.Diagnostics;
66
using Elastic.Markdown.IO;
7+
using Elastic.Markdown.Myst.InlineParsers;
78

89
namespace Elastic.Markdown.Myst.Directives;
910

@@ -96,17 +97,14 @@ private void ExtractImageUrl(ParserContext context)
9697
return;
9798
}
9899

99-
var includeFrom = context.MarkdownSourcePath.Directory!.FullName;
100-
if (imageUrl.StartsWith('/'))
101-
includeFrom = context.Build.DocumentationSourceDirectory.FullName;
100+
ImageUrl = DiagnosticLinkInlineParser.UpdateRelativeUrl(context, imageUrl);
102101

103-
ImageUrl = imageUrl;
104-
var imagePath = Path.Combine(includeFrom, imageUrl.TrimStart('/'));
105-
var file = context.Build.ReadFileSystem.FileInfo.New(imagePath);
102+
103+
var file = DiagnosticLinkInlineParser.ResolveFile(context, imageUrl);
106104
if (file.Exists)
107105
Found = true;
108106
else
109-
this.EmitError($"`{imageUrl}` does not exist. resolved to `{imagePath}");
107+
this.EmitError($"`{imageUrl}` does not exist. resolved to `{file}");
110108

111109
if (context.DocumentationFileLookup(context.MarkdownSourcePath) is MarkdownFile currentMarkdown)
112110
{

‎src/Elastic.Markdown/Myst/InlineParsers/DiagnosticLinkInlineParser.cs‎

Lines changed: 48 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ private static void ProcessLinkText(InlineProcessor processor, LinkInline link,
279279
_ = link.AppendChild(new LiteralInline(title));
280280
}
281281

282-
private static IFileInfo ResolveFile(ParserContext context, string url) =>
282+
public static IFileInfo ResolveFile(ParserContext context, string url) =>
283283
string.IsNullOrWhiteSpace(url)
284284
? context.MarkdownSourcePath
285285
: url.StartsWith('/')
@@ -302,49 +302,8 @@ private static void UpdateLinkUrl(LinkInline link, MarkdownFile? linkMarkdown, s
302302
newUrl = linkMarkdown.Url;
303303
}
304304
else
305-
{
306-
// TODO revisit when we refactor our documentation set graph
307-
// This method grew too complex, we need to revisit our documentation set graph generation so we can ask these questions
308-
// on `DocumentationFile` that are mostly precomputed
309-
var urlPathPrefix = context.Build.UrlPathPrefix ?? string.Empty;
310-
311-
if (!newUrl.StartsWith('/') && !string.IsNullOrEmpty(newUrl))
312-
{
313-
// eat overall path prefix since its gets appended later
314-
var subPrefix = context.CurrentUrlPath.Length >= urlPathPrefix.Length
315-
? context.CurrentUrlPath[urlPathPrefix.Length..]
316-
: urlPathPrefix;
317-
318-
// if we are trying to resolve a relative url from a _snippet folder ensure we eat the _snippet folder
319-
// as it's not part of url by chopping of the extra parent navigation
320-
if (newUrl.StartsWith("../") && context.DocumentationFileLookup(context.MarkdownSourcePath) is SnippetFile snippetFile)
321-
newUrl = url.Substring(3);
322-
323-
// TODO check through context.DocumentationFileLookup if file is index vs "index.md" check
324-
var markdownPath = context.MarkdownSourcePath;
325-
// if the current path is an index e.g /reference/cloud-k8s/
326-
// './' current path lookups should be relative to sub-path.
327-
// If it's not e.g /reference/cloud-k8s/api-docs/ these links should resolve on folder up.
328-
var lastIndexPath = subPrefix.LastIndexOf('/');
329-
if (lastIndexPath >= 0 && markdownPath.Name != "index.md")
330-
subPrefix = subPrefix[..lastIndexPath];
331-
var combined = '/' + Path.Combine(subPrefix, newUrl).TrimStart('/');
332-
newUrl = Path.GetFullPath(combined);
333-
}
334-
335-
// When running on Windows, path traversal results must be normalized prior to being used in a URL
336-
// Path.GetFullPath() will result in the drive letter being appended to the path, which needs to be pruned back.
337-
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
338-
{
339-
newUrl = newUrl.Replace('\\', '/');
340-
if (newUrl.Length > 2 && newUrl[1] == ':')
341-
newUrl = newUrl[2..];
342-
}
305+
newUrl = UpdateRelativeUrl(context, url);
343306

344-
if (!string.IsNullOrWhiteSpace(newUrl) && !string.IsNullOrWhiteSpace(urlPathPrefix))
345-
newUrl = $"{urlPathPrefix.TrimEnd('/')}{newUrl}";
346-
347-
}
348307

349308
if (newUrl.EndsWith(".md"))
350309
{
@@ -364,6 +323,52 @@ private static void UpdateLinkUrl(LinkInline link, MarkdownFile? linkMarkdown, s
364323
: newUrl;
365324
}
366325

326+
// TODO revisit when we refactor our documentation set graph
327+
// This method grew too complex, we need to revisit our documentation set graph generation so we can ask these questions
328+
// on `DocumentationFile` that are mostly precomputed
329+
public static string UpdateRelativeUrl(ParserContext context, string url)
330+
{
331+
var urlPathPrefix = context.Build.UrlPathPrefix ?? string.Empty;
332+
var newUrl = url;
333+
if (!newUrl.StartsWith('/') && !string.IsNullOrEmpty(newUrl))
334+
{
335+
var subPrefix = context.CurrentUrlPath.Length >= urlPathPrefix.Length
336+
? context.CurrentUrlPath[urlPathPrefix.Length..]
337+
: urlPathPrefix;
338+
339+
// if we are trying to resolve a relative url from a _snippet folder ensure we eat the _snippet folder
340+
// as it's not part of url by chopping of the extra parent navigation
341+
if (newUrl.StartsWith("../") && context.DocumentationFileLookup(context.MarkdownSourcePath) is SnippetFile)
342+
newUrl = url[3..];
343+
344+
// TODO check through context.DocumentationFileLookup if file is index vs "index.md" check
345+
var markdownPath = context.MarkdownSourcePath;
346+
// if the current path is an index e.g /reference/cloud-k8s/
347+
// './' current path lookups should be relative to sub-path.
348+
// If it's not e.g /reference/cloud-k8s/api-docs/ these links should resolve on folder up.
349+
var lastIndexPath = subPrefix.LastIndexOf('/');
350+
if (lastIndexPath >= 0 && markdownPath.Name != "index.md")
351+
subPrefix = subPrefix[..lastIndexPath];
352+
var combined = '/' + Path.Combine(subPrefix, newUrl).TrimStart('/');
353+
newUrl = Path.GetFullPath(combined);
354+
355+
}
356+
// When running on Windows, path traversal results must be normalized prior to being used in a URL
357+
// Path.GetFullPath() will result in the drive letter being appended to the path, which needs to be pruned back.
358+
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
359+
{
360+
newUrl = newUrl.Replace('\\', '/');
361+
if (newUrl.Length > 2 && newUrl[1] == ':')
362+
newUrl = newUrl[2..];
363+
}
364+
365+
if (!string.IsNullOrWhiteSpace(newUrl) && !string.IsNullOrWhiteSpace(urlPathPrefix))
366+
newUrl = $"{urlPathPrefix.TrimEnd('/')}{newUrl}";
367+
368+
// eat overall path prefix since its gets appended later
369+
return newUrl;
370+
}
371+
367372
private static bool IsCrossLink([NotNullWhen(true)] Uri? uri) =>
368373
uri != null // This means it's not a local
369374
&& !ExcludedSchemes.Contains(uri.Scheme)

‎tests/Elastic.Markdown.Tests/Directives/ImageTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public void ParsesBreakPoint()
3131
{
3232
Block!.Alt.Should().Be("Elasticsearch");
3333
Block!.Width.Should().Be("250px");
34-
Block!.ImageUrl.Should().Be("img/observability.png");
34+
Block!.ImageUrl.Should().Be("/img/observability.png");
3535
Block!.Screenshot.Should().Be("screenshot");
3636
}
3737

‎tests/authoring/Blocks/ImageBlocks.fs‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Licensed to Elasticsearch B.V under one or more agreements.
2+
// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
3+
// See the LICENSE file in the project root for more information
4+
module ``block elements``.``image blocks``
5+
6+
open Xunit
7+
open authoring
8+
9+
type ``static path to image`` () =
10+
static let markdown = Setup.Markdown """
11+
:::{image} img/observability.png
12+
:alt: Elasticsearch
13+
:width: 250px
14+
:screenshot:
15+
:::
16+
"""
17+
18+
[<Fact>]
19+
let ``validate src is anchored`` () =
20+
markdown |> convertsToContainingHtml """
21+
<img loading="lazy" alt="Elasticsearch" src="/img/observability.png" style="width: 250px;" class="screenshot">
22+
"""
23+
24+
type ``supports --url-path-prefix`` () =
25+
static let docs = Setup.GenerateWithOptions { UrlPathPrefix = Some "/docs" } [
26+
Static "img/observability.png"
27+
28+
Index """# Testing nested inline anchors
29+
:::{image} img/observability.png
30+
:alt: Elasticsearch
31+
:width: 250px
32+
:screenshot:
33+
:::
34+
"""
35+
36+
Markdown "folder/relative.md" """
37+
:::{image} ../img/observability.png
38+
:alt: Elasticsearch
39+
:width: 250px
40+
:screenshot:
41+
:::
42+
"""
43+
]
44+
45+
[<Fact>]
46+
let ``validate image src contains prefix`` () =
47+
docs |> convertsToContainingHtml """
48+
<img loading="lazy" alt="Elasticsearch" src="/docs/img/observability.png" style="width: 250px;" class="screenshot">
49+
"""
50+
51+
[<Fact>]
52+
let ``validate image src contains prefix when referenced relatively`` () =
53+
docs |> converts "folder/relative.md" |> containsHtml """
54+
<img loading="lazy" alt="Elasticsearch" src="/docs/img/observability.png" style="width: 250px;" class="screenshot">
55+
"""
56+
57+
[<Fact>]
58+
let ``has no errors`` () = docs |> hasNoErrors
59+
60+
type ``image ref out of scope`` () =
61+
static let docs = Setup.GenerateWithOptions { UrlPathPrefix = Some "/docs" } [
62+
Static "img/observability.png"
63+
64+
Index """# Testing nested inline anchors
65+
:::{image} ../img/observability.png
66+
:alt: Elasticsearch
67+
:width: 250px
68+
:screenshot:
69+
:::
70+
"""
71+
]
72+
73+
[<Fact>]
74+
let ``validate image src contains prefix and is anchored to documentation scope root`` () =
75+
docs |> convertsToContainingHtml """
76+
<img loading="lazy" alt="Elasticsearch" src="/docs/img/observability.png" style="width: 250px;" class="screenshot">
77+
"""
78+
79+
[<Fact>]
80+
let ``emits an error image reference is outside of documentation scope`` () =
81+
docs |> hasError "./img/observability.png` does not exist. resolved to"

‎tests/authoring/Container/DefinitionLists.fs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ This is my `definition`
6767
</dd>
6868
</dl>
6969
"""
70+
7071
[<Fact>]
7172
let ``has no errors 2`` () = markdown |> hasNoErrors
7273

‎tests/authoring/Framework/Setup.fs‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,25 @@ type TestFile =
2525
| File of name: string * contents: string
2626
| MarkdownFile of name: string * markdown: Markdown
2727
| SnippetFile of name: string * markdown: Markdown
28+
| StaticFile of name: string
2829

2930
static member Index ([<LanguageInjection("markdown")>] m) =
3031
MarkdownFile("index.md" , m)
3132

3233
static member Markdown path ([<LanguageInjection("markdown")>] m) =
3334
MarkdownFile(path , m)
3435

36+
static member Static path = StaticFile(path)
37+
3538
static member Snippet path ([<LanguageInjection("markdown")>] m) =
3639
SnippetFile(path , m)
3740

41+
type SetupOptions =
42+
{ UrlPathPrefix: string option }
43+
static member Empty = {
44+
UrlPathPrefix = None
45+
}
46+
3847
type Setup =
3948

4049
static let GenerateDocSetYaml(
@@ -61,7 +70,7 @@ type Setup =
6170
yaml.WriteLine($" - file: {relative}")
6271
let fullPath = Path.Combine(root.FullName, relative)
6372
let contents = File.ReadAllText fullPath
64-
fileSystem.AddFile(fullPath, new MockFileData(contents))
73+
fileSystem.AddFile(fullPath, MockFileData(contents))
6574
)
6675

6776
match globalVariables with
@@ -79,14 +88,16 @@ type Setup =
7988
let redirectYaml = File.ReadAllText(Path.Combine(root.FullName, "_redirects.yml"))
8089
fileSystem.AddFile(Path.Combine(root.FullName, redirectsName), MockFileData(redirectYaml))
8190

82-
static member Generator (files: TestFile seq) : Task<GeneratorResults> =
91+
static member Generator (files: TestFile seq) (options: SetupOptions option) : Task<GeneratorResults> =
92+
let options = options |> Option.defaultValue SetupOptions.Empty
8393

8494
let d = files
8595
|> Seq.map (fun f ->
8696
match f with
8797
| File(name, contents) -> ($"docs/{name}", MockFileData(contents))
8898
| SnippetFile(name, markdown) -> ($"docs/{name}", MockFileData(markdown))
8999
| MarkdownFile(name, markdown) -> ($"docs/{name}", MockFileData(markdown))
100+
| StaticFile(name) -> ($"docs/{name}", MockFileData(""))
90101
)
91102
|> Map.ofSeq
92103

@@ -95,8 +106,8 @@ type Setup =
95106

96107
GenerateDocSetYaml (fileSystem, None)
97108

98-
let collector = TestDiagnosticsCollector();
99-
let context = BuildContext(collector, fileSystem)
109+
let collector = TestDiagnosticsCollector()
110+
let context = BuildContext(collector, fileSystem, UrlPathPrefix=(options.UrlPathPrefix |> Option.defaultValue ""))
100111
let logger = new TestLoggerFactory()
101112
let conversionCollector = TestConversionCollector()
102113
let linkResolver = TestCrossLinkResolver(context.Configuration)
@@ -115,11 +126,14 @@ type Setup =
115126

116127
/// Pass several files to the test setup
117128
static member Generate files =
118-
lazy (task { return! Setup.Generator files } |> Async.AwaitTask |> Async.RunSynchronously)
129+
lazy (task { return! Setup.Generator files None } |> Async.AwaitTask |> Async.RunSynchronously)
130+
131+
static member GenerateWithOptions options files =
132+
lazy (task { return! Setup.Generator files (Some options) } |> Async.AwaitTask |> Async.RunSynchronously)
119133

120134
/// Pass a full documentation page to the test setup
121135
static member Document ([<LanguageInjection("markdown")>]m: string) =
122-
lazy (task { return! Setup.Generator [Index m] } |> Async.AwaitTask |> Async.RunSynchronously)
136+
lazy (task { return! Setup.Generator [Index m] None } |> Async.AwaitTask |> Async.RunSynchronously)
123137

124138
/// Pass a markdown fragment to the test setup
125139
static member Markdown ([<LanguageInjection("markdown")>]m: string) =
@@ -128,6 +142,6 @@ type Setup =
128142
{m}
129143
"""
130144
lazy (
131-
task { return! Setup.Generator [Index m] }
145+
task { return! Setup.Generator [Index m] None }
132146
|> Async.AwaitTask |> Async.RunSynchronously
133147
)

‎tests/authoring/authoring.fsproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
<Compile Include="Blocks\CodeBlocks\CodeBlocks.fs"/>
5353
<Compile Include="Blocks\Lists.fs"/>
5454
<Compile Include="Blocks\Admonitions.fs"/>
55+
<Compile Include="Blocks\ImageBlocks.fs" />
5556
<Compile Include="Applicability\AppliesToFrontMatter.fs" />
5657
<Compile Include="Applicability\AppliesToDirective.fs" />
5758
<Compile Include="Directives\IncludeBlocks.fs" />

0 commit comments

Comments
 (0)