fix(house): redirect lookup survives store failures
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
{
|
||||
|
||||
@@ -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 <c>articles/_plan.json</c> at the catalog's commit.
|
||||
/// </summary>
|
||||
public sealed class ArticleQueries(IDocumentCatalog catalog, INoteStore store)
|
||||
public sealed class ArticleQueries(IDocumentCatalog catalog, INoteStore store, ILogger<ArticleQueries> log)
|
||||
{
|
||||
public static readonly IReadOnlyList<string> Languages = ["de", "en"];
|
||||
|
||||
@@ -18,17 +19,31 @@ public sealed class ArticleQueries(IDocumentCatalog catalog, INoteStore store)
|
||||
|
||||
private static readonly IReadOnlyDictionary<string, string> NoRedirects = new Dictionary<string, string>();
|
||||
|
||||
private (string Commit, IReadOnlyDictionary<string, string> Redirects)? _redirects;
|
||||
private sealed record Cached(string Commit, IReadOnlyDictionary<string, string> Redirects);
|
||||
|
||||
/// <summary>Retired slug → slug that took over; empty when the plan is missing or unreadable. Cached per commit.</summary>
|
||||
private volatile Cached? _redirects;
|
||||
|
||||
/// <summary>
|
||||
/// 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).
|
||||
/// </summary>
|
||||
public async Task<IReadOnlyDictionary<string, string>> 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<string, string> 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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
/// <summary>The slug a retired <see cref="Slug"/> resolves to when that article exists in <see cref="Lang"/> and this reader may see it; else null.</summary>
|
||||
private async Task<string?> 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;
|
||||
}
|
||||
|
||||
|
||||
@@ -76,7 +76,7 @@ public static partial class ArticleMarkdown
|
||||
/// <param name="visibleTitles">slug → title in <paramref name="lang"/> of every article this reader may see.</param>
|
||||
/// <param name="redirects">retired slug → slug that took over; a link to a slug outside <paramref name="visibleTitles"/> follows them.</param>
|
||||
public static RenderedArticle Render(string body, string lang, string slug, bool signedIn,
|
||||
IReadOnlyDictionary<string, string> visibleTitles, IReadOnlyDictionary<string, string>? redirects = null, ILogger? logger = null) =>
|
||||
IReadOnlyDictionary<string, string> visibleTitles, IReadOnlyDictionary<string, string> redirects, ILogger? logger = null) =>
|
||||
RenderVisible(signedIn ? body : ArticleBody.RedactPrivate(body), lang, slug, signedIn, visibleTitles, redirects, logger);
|
||||
|
||||
/// <summary>
|
||||
@@ -85,15 +85,15 @@ public static partial class ArticleMarkdown
|
||||
/// </summary>
|
||||
public static IReadOnlyList<string> VisibleImagePaths(string body, string lang, string slug, bool signedIn,
|
||||
IReadOnlyDictionary<string, string> 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;
|
||||
|
||||
/// <summary>Renders a body that is already what this reader may see.</summary>
|
||||
internal static RenderedArticle RenderVisible(string visibleBody, string lang, string slug, bool signedIn,
|
||||
IReadOnlyDictionary<string, string> visibleTitles, IReadOnlyDictionary<string, string>? redirects = null, ILogger? logger = null) =>
|
||||
IReadOnlyDictionary<string, string> visibleTitles, IReadOnlyDictionary<string, string> redirects, ILogger? logger = null) =>
|
||||
Process(visibleBody, lang, slug, signedIn, visibleTitles, redirects, logger).Article;
|
||||
|
||||
private static (RenderedArticle Article, IReadOnlyList<string> Images) Process(string visibleBody, string lang, string slug,
|
||||
bool signedIn, IReadOnlyDictionary<string, string> visibleTitles, IReadOnlyDictionary<string, string>? redirects, ILogger? logger)
|
||||
bool signedIn, IReadOnlyDictionary<string, string> visibleTitles, IReadOnlyDictionary<string, string> redirects, ILogger? logger)
|
||||
{
|
||||
var slugs = new List<string>();
|
||||
var doc = Parse(visibleBody, slugs);
|
||||
@@ -110,7 +110,7 @@ public static partial class ArticleMarkdown
|
||||
|
||||
var images = new List<string>();
|
||||
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<LeafBlock>().Where(HasLeftover).ToList();
|
||||
if (leftovers.Count > 0)
|
||||
|
||||
@@ -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"]}"""));
|
||||
|
||||
@@ -11,6 +11,8 @@ public class ArticleMarkdownTests
|
||||
["blinds"] = "Rollläden",
|
||||
};
|
||||
|
||||
private static readonly Dictionary<string, string> NoRedirects = new();
|
||||
|
||||
private static readonly Dictionary<string, string> 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("<div class=\"private-locked\">", 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("<div class=\"private-locked\">", 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);
|
||||
}
|
||||
|
||||
@@ -69,8 +69,22 @@ public sealed class FakeStore : INoteStore
|
||||
return (await GetHeadAsync(ct)).Commit;
|
||||
}
|
||||
|
||||
public Task<StoredText?> ReadTextAsync(string path, string commit, CancellationToken ct) =>
|
||||
Task.FromResult(Files.TryGetValue(path, out var text) ? new StoredText(text, BlobSha(text)) : null);
|
||||
/// <summary>Paths whose text read throws, as an unreachable remote does.</summary>
|
||||
public HashSet<string> FailReads { get; } = [];
|
||||
|
||||
private readonly Dictionary<string, int> _textReads = new();
|
||||
|
||||
public int TextReads(string path)
|
||||
{
|
||||
lock (_textReads) return _textReads.GetValueOrDefault(path);
|
||||
}
|
||||
|
||||
public Task<StoredText?> 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);
|
||||
}
|
||||
|
||||
/// <summary>Every path <see cref="ReadBytesAsync"/> was asked for, in order.</summary>
|
||||
public List<string> ByteReads { get; } = [];
|
||||
|
||||
@@ -48,8 +48,10 @@ public class HouseTests(WikiHostFactory factory) : IClassFixture<WikiHostFactory
|
||||
"""
|
||||
{"articles":[],"assignments":{},"redirects":{
|
||||
"old-switch":"schalter","old-net":"network-internals","chain-a":"chain-b","chain-b":"schalter",
|
||||
"loop-a":"loop-b","loop-b":"loop-a","old-gone":"deleted-article"}}
|
||||
"loop-a":"loop-b","loop-b":"loop-a","old-gone":"deleted-article","old-nur-de":"nur-de",
|
||||
"hop-1":"hop-2","hop-2":"hop-3","hop-3":"hop-4","hop-4":"hop-5","hop-5":"hop-6","hop-6":"schalter"}}
|
||||
""";
|
||||
factory.Store.Files["articles/de/nur-de.md"] = ArticleText("de", "nur-de", "Nur Deutsch", "public", "Text.");
|
||||
factory.Store.Binaries["raw/taster.png"] = TasterPng;
|
||||
factory.Store.Binaries["raw/geheim.png"] = GeheimPng;
|
||||
factory.Store.Binaries["raw/cache.png"] = [1, 2, 3];
|
||||
@@ -237,14 +239,77 @@ public class HouseTests(WikiHostFactory factory) : IClassFixture<WikiHostFactory
|
||||
[Theory]
|
||||
[InlineData("loop-a")]
|
||||
[InlineData("old-gone")]
|
||||
public async Task Redirect_CycleOrMissingTarget_Is404(string old)
|
||||
[InlineData("hop-1")]
|
||||
public async Task Redirect_CycleMissingTargetOrTooLong_Is404_LikeMissing(string old)
|
||||
{
|
||||
await SeedAsync();
|
||||
|
||||
var page = await Browser().GetAsync($"/house/de/{old}");
|
||||
var missing = await Browser().GetAsync("/house/de/does-not-exist");
|
||||
|
||||
Assert.Equal(HttpStatusCode.NotFound, page.StatusCode);
|
||||
Assert.Null(page.Headers.Location);
|
||||
Assert.Equal(WithoutComponentState(await Html(missing)).Replace("does-not-exist", "{slug}", StringComparison.Ordinal),
|
||||
WithoutComponentState(await Html(page)).Replace(old, "{slug}", StringComparison.Ordinal));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Redirect_FiveHops_Is301()
|
||||
{
|
||||
await SeedAsync();
|
||||
|
||||
var page = await Browser().GetAsync("/house/de/hop-2");
|
||||
|
||||
Assert.Equal(HttpStatusCode.MovedPermanently, page.StatusCode);
|
||||
Assert.Equal("/house/de/schalter", page.Headers.Location!.ToString());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Redirect_TargetOnlyInOtherLanguage_Is404()
|
||||
{
|
||||
await SeedAsync();
|
||||
var browser = await SignedInAsync();
|
||||
|
||||
Assert.Equal(HttpStatusCode.MovedPermanently, (await browser.GetAsync("/house/de/old-nur-de")).StatusCode);
|
||||
Assert.Equal(HttpStatusCode.NotFound, (await browser.GetAsync("/house/en/old-nur-de")).StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Redirect_Response_NoStore_NoIndex()
|
||||
{
|
||||
await SeedAsync();
|
||||
|
||||
var page = await Browser().GetAsync("/house/de/old-switch");
|
||||
|
||||
Assert.Equal(HttpStatusCode.MovedPermanently, page.StatusCode);
|
||||
Assert.Equal("no-store", page.Headers.CacheControl!.ToString());
|
||||
Assert.Equal("noindex", page.Headers.GetValues("X-Robots-Tag").Single());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task PlanReadFails_ArticleStillRenders_PlanReadOncePerCommit()
|
||||
{
|
||||
await SeedAsync();
|
||||
var failing = factory.WithWebHostBuilder(_ => { });
|
||||
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]
|
||||
|
||||
Reference in New Issue
Block a user