From 61af59561493f0655a3ce4620008770feb65a02f Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Mon, 26 Oct 2026 09:57:07 +0100 Subject: [PATCH] fix(house): redirect lookup survives store failures Co-Authored-By: Claude Opus 5.5 --- .../Encyclopedia/ArticlePlan.cs | 2 +- .../Reading/ArticleQueries.cs | 27 ++++++-- .../Components/Pages/HouseArticle.razor | 13 ++-- src/Wiki.Host/Rendering/ArticleMarkdown.cs | 10 +-- .../ArticlePlanTests.cs | 9 +++ test/Wiki.Host.Tests/ArticleMarkdownTests.cs | 20 +++--- test/Wiki.Host.Tests/Fakes.cs | 18 ++++- test/Wiki.Host.Tests/HouseTests.cs | 69 ++++++++++++++++++- 8 files changed, 136 insertions(+), 32 deletions(-) diff --git a/src/Wiki.Application/Encyclopedia/ArticlePlan.cs b/src/Wiki.Application/Encyclopedia/ArticlePlan.cs index 0044b3e..01c940b 100644 --- a/src/Wiki.Application/Encyclopedia/ArticlePlan.cs +++ b/src/Wiki.Application/Encyclopedia/ArticlePlan.cs @@ -186,7 +186,7 @@ public static class ArticlePlanJson foreach (var (from, to) in redirects) { var target = PlanParsing.Text(to); - if (target is not null && PlanParsing.SlugPattern().IsMatch(from) && plan.Find(from) is null) plan.SetRedirect(from, target); + if (target is not null && PlanParsing.SlugPattern().IsMatch(target) && PlanParsing.SlugPattern().IsMatch(from) && plan.Find(from) is null) plan.SetRedirect(from, target); } foreach (var (note, value) in assignments) { diff --git a/src/Wiki.Application/Reading/ArticleQueries.cs b/src/Wiki.Application/Reading/ArticleQueries.cs index 017d66f..b76ba85 100644 --- a/src/Wiki.Application/Reading/ArticleQueries.cs +++ b/src/Wiki.Application/Reading/ArticleQueries.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging; using Wiki.Application.Encyclopedia; using Wiki.Core.Model; using Wiki.Core.Parsing; @@ -10,7 +11,7 @@ namespace Wiki.Application.Reading; /// counts as missing. Anonymous visibility is per subject: both language files must exist and be public. /// Redirects come from articles/_plan.json at the catalog's commit. /// -public sealed class ArticleQueries(IDocumentCatalog catalog, INoteStore store) +public sealed class ArticleQueries(IDocumentCatalog catalog, INoteStore store, ILogger log) { public static readonly IReadOnlyList Languages = ["de", "en"]; @@ -18,17 +19,31 @@ public sealed class ArticleQueries(IDocumentCatalog catalog, INoteStore store) private static readonly IReadOnlyDictionary NoRedirects = new Dictionary(); - private (string Commit, IReadOnlyDictionary Redirects)? _redirects; + private sealed record Cached(string Commit, IReadOnlyDictionary Redirects); - /// Retired slug → slug that took over; empty when the plan is missing or unreadable. Cached per commit. + private volatile Cached? _redirects; + + /// + /// Retired slug → slug that took over; empty when the plan is missing, unreadable or the store fails. + /// Cached per commit, a failure too (no store request per page view until the commit changes). + /// public async Task> RedirectsAsync(CancellationToken ct) { var commit = (await catalog.GetStateAsync(ct)).Commit; if (commit is null) return NoRedirects; if (_redirects is { } cached && cached.Commit == commit) return cached.Redirects; - var text = await store.ReadTextAsync(PlanPath, commit, ct); - var redirects = (text is null ? null : ArticlePlanJson.Read(text.Text))?.Redirects ?? NoRedirects; - _redirects = (commit, redirects); + IReadOnlyDictionary redirects; + try + { + var text = await store.ReadTextAsync(PlanPath, commit, ct); + redirects = (text is null ? null : ArticlePlanJson.Read(text.Text))?.Redirects ?? NoRedirects; + } + catch (Exception e) when (e is not OperationCanceledException) + { + log.LogWarning(e, "Cannot read {Path} at {Commit}; serving without redirects", PlanPath, commit); + redirects = NoRedirects; + } + _redirects = new Cached(commit, redirects); return redirects; } diff --git a/src/Wiki.Host/Components/Pages/HouseArticle.razor b/src/Wiki.Host/Components/Pages/HouseArticle.razor index 8754552..59bd502 100644 --- a/src/Wiki.Host/Components/Pages/HouseArticle.razor +++ b/src/Wiki.Host/Components/Pages/HouseArticle.razor @@ -79,18 +79,16 @@ else if (_article is not null && _rendered is not null) { var ct = HttpContext.RequestAborted; _signedIn = HouseEndpoints.IsHuman(HttpContext.User); - if (Rest is null && HouseEndpoints.IsLanguage(Lang) && HouseEndpoints.IsSlug(Slug) - && await Articles.GetAsync(Lang, Slug, ct) is null - && await RedirectTargetAsync(ct) is { } target) + var valid = Rest is null && HouseEndpoints.IsLanguage(Lang) && HouseEndpoints.IsSlug(Slug); + var article = valid ? await Articles.GetAsync(Lang, Slug, ct) : null; + if (valid && article is null && await RedirectTargetAsync(ct) is { } target) { // NavigateTo answers 302 in static SSR; a retired slug moved for good. HttpContext.Response.StatusCode = StatusCodes.Status301MovedPermanently; HttpContext.Response.Headers.Location = $"/house/{Lang}/{target}"; return; } - if (Rest is not null || !HouseEndpoints.IsLanguage(Lang) || !HouseEndpoints.IsSlug(Slug) - || await Articles.GetAsync(Lang, Slug, ct) is not { } article - || (!_signedIn && await Articles.IsPrivateAsync(Slug, ct))) + if (article is null || (!_signedIn && await Articles.IsPrivateAsync(Slug, ct))) { _notFound = true; HouseEndpoints.RespondNotFound(HttpContext.Response); @@ -107,8 +105,9 @@ else if (_article is not null && _rendered is not null) /// The slug a retired resolves to when that article exists in and this reader may see it; else null. private async Task RedirectTargetAsync(CancellationToken ct) { + if (!(await Articles.RedirectsAsync(ct)).ContainsKey(Slug)) return null; var target = await Articles.ResolveAsync(Slug, ct); - if (target is null || target == Slug || await Articles.GetAsync(Lang, target, ct) is null) return null; + if (target is null || target == Slug || !HouseEndpoints.IsSlug(target) || await Articles.GetAsync(Lang, target, ct) is null) return null; return !_signedIn && await Articles.IsPrivateAsync(target, ct) ? null : target; } diff --git a/src/Wiki.Host/Rendering/ArticleMarkdown.cs b/src/Wiki.Host/Rendering/ArticleMarkdown.cs index 7adc3a9..de1edb9 100644 --- a/src/Wiki.Host/Rendering/ArticleMarkdown.cs +++ b/src/Wiki.Host/Rendering/ArticleMarkdown.cs @@ -76,7 +76,7 @@ public static partial class ArticleMarkdown /// slug → title in of every article this reader may see. /// retired slug → slug that took over; a link to a slug outside follows them. public static RenderedArticle Render(string body, string lang, string slug, bool signedIn, - IReadOnlyDictionary visibleTitles, IReadOnlyDictionary? redirects = null, ILogger? logger = null) => + IReadOnlyDictionary visibleTitles, IReadOnlyDictionary redirects, ILogger? logger = null) => RenderVisible(signedIn ? body : ArticleBody.RedactPrivate(body), lang, slug, signedIn, visibleTitles, redirects, logger); /// @@ -85,15 +85,15 @@ public static partial class ArticleMarkdown /// public static IReadOnlyList VisibleImagePaths(string body, string lang, string slug, bool signedIn, IReadOnlyDictionary visibleTitles) => - Process(signedIn ? body : ArticleBody.RedactPrivate(body), lang, slug, signedIn, visibleTitles, redirects: null, logger: null).Images; + Process(signedIn ? body : ArticleBody.RedactPrivate(body), lang, slug, signedIn, visibleTitles, redirects: NoRedirects, logger: null).Images; /// Renders a body that is already what this reader may see. internal static RenderedArticle RenderVisible(string visibleBody, string lang, string slug, bool signedIn, - IReadOnlyDictionary visibleTitles, IReadOnlyDictionary? redirects = null, ILogger? logger = null) => + IReadOnlyDictionary visibleTitles, IReadOnlyDictionary redirects, ILogger? logger = null) => Process(visibleBody, lang, slug, signedIn, visibleTitles, redirects, logger).Article; private static (RenderedArticle Article, IReadOnlyList Images) Process(string visibleBody, string lang, string slug, - bool signedIn, IReadOnlyDictionary visibleTitles, IReadOnlyDictionary? redirects, ILogger? logger) + bool signedIn, IReadOnlyDictionary visibleTitles, IReadOnlyDictionary redirects, ILogger? logger) { var slugs = new List(); var doc = Parse(visibleBody, slugs); @@ -110,7 +110,7 @@ public static partial class ArticleMarkdown var images = new List(); SanitizeLinks(doc, lang, slug, visibleTitles, images); - var headings = ResolveWikiLinks(doc, lang, slugs, visibleTitles, redirects ?? NoRedirects); + var headings = ResolveWikiLinks(doc, lang, slugs, visibleTitles, redirects); var leftovers = doc.Descendants().Where(HasLeftover).ToList(); if (leftovers.Count > 0) diff --git a/test/Wiki.Application.Tests/ArticlePlanTests.cs b/test/Wiki.Application.Tests/ArticlePlanTests.cs index a925856..2c8a23f 100644 --- a/test/Wiki.Application.Tests/ArticlePlanTests.cs +++ b/test/Wiki.Application.Tests/ArticlePlanTests.cs @@ -158,6 +158,15 @@ public class ArticlePlanTests Assert.Equal("heating", read.Redirects["old"]); } + [Fact] + public void Json_RedirectWithBadTargetSlug_Dropped() + { + var read = ArticlePlanJson.Read("""{"articles":[],"assignments":{},"redirects":{"ok":"good-slug","bad":"../x","bad2":"A b"}}"""); + + Assert.NotNull(read); + Assert.Equal(["ok"], read.Redirects.Keys); + } + [Fact] public void Json_OldFormat_IsNull() => Assert.Null(ArticlePlanJson.Read("""{"articles":[{"slug":"a","category":"topic","sources":["x"],"related":[]}],"coveredNotes":["x"]}""")); diff --git a/test/Wiki.Host.Tests/ArticleMarkdownTests.cs b/test/Wiki.Host.Tests/ArticleMarkdownTests.cs index 5f12441..911e8ef 100644 --- a/test/Wiki.Host.Tests/ArticleMarkdownTests.cs +++ b/test/Wiki.Host.Tests/ArticleMarkdownTests.cs @@ -11,6 +11,8 @@ public class ArticleMarkdownTests ["blinds"] = "Rollläden", }; + private static readonly Dictionary NoRedirects = new(); + private static readonly Dictionary Redirects = new() { ["old-heating"] = "heating", @@ -39,9 +41,9 @@ public class ArticleMarkdownTests Assert.DoesNotContain("loop", html); } - private static RenderedArticle Anonymous(string body) => ArticleMarkdown.Render(body, "de", "switches", signedIn: false, Titles); + private static RenderedArticle Anonymous(string body) => ArticleMarkdown.Render(body, "de", "switches", signedIn: false, Titles, NoRedirects); - private static RenderedArticle SignedIn(string body) => ArticleMarkdown.Render(body, "de", "switches", signedIn: true, Titles); + private static RenderedArticle SignedIn(string body) => ArticleMarkdown.Render(body, "de", "switches", signedIn: true, Titles, NoRedirects); [Fact] public void Render_Anonymous_ContainsNoPrivateText() @@ -59,7 +61,7 @@ public class ArticleMarkdownTests [Fact] public void Render_Anonymous_LockedBlockIsLocalized() { - var article = ArticleMarkdown.Render("::: private\nSECRET\n:::", "en", "switches", signedIn: false, Titles); + var article = ArticleMarkdown.Render("::: private\nSECRET\n:::", "en", "switches", signedIn: false, Titles, NoRedirects); Assert.DoesNotContain("SECRET", article.Html); Assert.Contains("Private section — log in to read", article.Html); @@ -97,7 +99,7 @@ public class ArticleMarkdownTests var logger = new ListLogger(); var article = ArticleMarkdown.RenderVisible( - "## Teil\n\nÖffentlich.\n\n::: warning\nGEHEIM\n:::", "de", "switches", signedIn: false, Titles, logger: logger); + "## Teil\n\nÖffentlich.\n\n::: warning\nGEHEIM\n:::", "de", "switches", signedIn: false, Titles, NoRedirects, logger: logger); Assert.DoesNotContain("GEHEIM", article.Html); Assert.DoesNotContain("Öffentlich", article.Html); @@ -110,7 +112,7 @@ public class ArticleMarkdownTests [Fact] public void RenderVisible_Anonymous_PrivateContainer_LocksWholeBody() { - var article = ArticleMarkdown.RenderVisible("Öffentlich.\n\n::: private\nGEHEIM\n:::", "de", "switches", signedIn: false, Titles); + var article = ArticleMarkdown.RenderVisible("Öffentlich.\n\n::: private\nGEHEIM\n:::", "de", "switches", signedIn: false, Titles, NoRedirects); Assert.DoesNotContain("GEHEIM", article.Html); Assert.Contains("
", article.Html); @@ -280,7 +282,7 @@ public class ArticleMarkdownTests { foreach (var signedIn in new[] { false, true }) { - var html = ArticleMarkdown.Render(body, "de", "switches", signedIn, Titles).Html; + var html = ArticleMarkdown.Render(body, "de", "switches", signedIn, Titles, NoRedirects).Html; Assert.DoesNotContain("href=\"javascript", html, StringComparison.OrdinalIgnoreCase); Assert.DoesNotContain("href=\"java", html, StringComparison.OrdinalIgnoreCase); @@ -365,7 +367,7 @@ public class ArticleMarkdownTests { foreach (var signedIn in new[] { false, true }) { - var html = ArticleMarkdown.Render(body, "de", "switches", signedIn, Titles).Html; + var html = ArticleMarkdown.Render(body, "de", "switches", signedIn, Titles, NoRedirects).Html; Assert.DoesNotContain("network", html); Assert.DoesNotContain("href=\"https://e.com", html); @@ -446,12 +448,12 @@ public class ArticleMarkdownTests var body = "Öffentlich.\n\n\\[\\[network-internals]]"; var logger = new ListLogger(); - var anonymous = ArticleMarkdown.RenderVisible(body, "de", "switches", signedIn: false, Titles, logger: logger); + var anonymous = ArticleMarkdown.RenderVisible(body, "de", "switches", signedIn: false, Titles, NoRedirects, logger: logger); Assert.DoesNotContain("network", anonymous.Html); Assert.Contains("
", anonymous.Html); Assert.Contains(logger.Entries, e => e.Level == LogLevel.Warning); - var signedIn = ArticleMarkdown.RenderVisible(body, "de", "switches", signedIn: true, Titles); + var signedIn = ArticleMarkdown.RenderVisible(body, "de", "switches", signedIn: true, Titles, NoRedirects); Assert.DoesNotContain("network", signedIn.Html); Assert.Contains("Öffentlich.", signedIn.Html); } diff --git a/test/Wiki.Host.Tests/Fakes.cs b/test/Wiki.Host.Tests/Fakes.cs index f961085..ea607b4 100644 --- a/test/Wiki.Host.Tests/Fakes.cs +++ b/test/Wiki.Host.Tests/Fakes.cs @@ -69,8 +69,22 @@ public sealed class FakeStore : INoteStore return (await GetHeadAsync(ct)).Commit; } - public Task ReadTextAsync(string path, string commit, CancellationToken ct) => - Task.FromResult(Files.TryGetValue(path, out var text) ? new StoredText(text, BlobSha(text)) : null); + /// Paths whose text read throws, as an unreachable remote does. + public HashSet FailReads { get; } = []; + + private readonly Dictionary _textReads = new(); + + public int TextReads(string path) + { + lock (_textReads) return _textReads.GetValueOrDefault(path); + } + + public Task ReadTextAsync(string path, string commit, CancellationToken ct) + { + lock (_textReads) _textReads[path] = _textReads.GetValueOrDefault(path) + 1; + if (FailReads.Contains(path)) throw new IOException($"store unavailable: {path}"); + return Task.FromResult(Files.TryGetValue(path, out var text) ? new StoredText(text, BlobSha(text)) : null); + } /// Every path was asked for, in order. public List ByteReads { get; } = []; diff --git a/test/Wiki.Host.Tests/HouseTests.cs b/test/Wiki.Host.Tests/HouseTests.cs index b20bdda..12996b4 100644 --- a/test/Wiki.Host.Tests/HouseTests.cs +++ b/test/Wiki.Host.Tests/HouseTests.cs @@ -48,8 +48,10 @@ public class HouseTests(WikiHostFactory factory) : IClassFixture { }); + var sync = new HttpRequestMessage(HttpMethod.Post, "/api/v1/sync?force=true"); + sync.Headers.Add("X-Api-Key", WikiHostFactory.ApiKey); + Assert.Equal(HttpStatusCode.OK, (await failing.CreateClient().SendAsync(sync)).StatusCode); + factory.Store.FailReads.Add("articles/_plan.json"); + try + { + var before = factory.Store.TextReads("articles/_plan.json"); + var client = failing.CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false }); + + Assert.Equal(HttpStatusCode.OK, (await client.GetAsync("/house/de/schalter")).StatusCode); + Assert.Equal(HttpStatusCode.OK, (await client.GetAsync("/house/de/schalter")).StatusCode); + Assert.Equal(HttpStatusCode.NotFound, (await client.GetAsync("/house/de/old-switch")).StatusCode); + + Assert.Equal(before + 1, factory.Store.TextReads("articles/_plan.json")); + } + finally + { + factory.Store.FailReads.Clear(); + } } [Fact]