From 2926740d033b61d823b64c4843259dac614097f9 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Fri, 31 Jul 2026 09:18:22 +0200 Subject: [PATCH] JR-023: correct the missing-dependency logs, and test them Both failure paths still told admins the overlay was "falling back to patching index.html on disk". JR-021 deleted that fallback, so the message was false -- and it was false in the worst place, since an admin reads it precisely when debugging a missing overlay and would go hunting for a patch that no longer exists. They now name the install URL and say the overlay is disabled while every other feature is unaffected. The not-found case is a Warning, not Information: a headline feature being off should not sit among startup chatter. UT-012..015 cover the branch. The File Transformation assembly is genuinely absent from the test host, so TryRegister exercises its real not-found path rather than a seam invented for the test. UT-015 pins that a null payload returns empty rather than throwing -- this callback runs inside another plugin's request path on every page served, so throwing would break the web client itself, not just JRay's overlay. Making those runnable needed the test project to reference Jellyfin.Controller and Jellyfin.Model without the plugin's ExcludeAssets=runtime. My earlier claim that DisableTransitiveFrameworkReferences alone sufficed was too narrow: it drops the demand for the web *framework*, but ILogger and MediaBrowser.Common live in the excluded assets, so anything past dependency-free logic failed at run time with FileNotFoundException. Both settings are needed, and the register now says so. Second mutation check: downgrading the warning to Information fails UT-013 and nothing else. Source restored and re-verified. JR-023 reaches Done for the detection half. The config-page banner stays T4 -- verifiable only against a live server. TRACES: UT-012, UT-013, UT-014, UT-015 | JR-023 Co-Authored-By: Claude Opus 5 --- .../FileTransformationRegistrationTests.cs | 96 +++++++++++++++++++ .../Jellyfin.Plugin.JRay.Tests.csproj | 9 ++ .../FileTransformationRegistration.cs | 20 +++- SPEC.md | 20 ++-- docs/requirements.md | 48 ++++++---- 5 files changed, 165 insertions(+), 28 deletions(-) create mode 100644 Jellyfin.Plugin.JRay.Tests/FileTransformationRegistrationTests.cs diff --git a/Jellyfin.Plugin.JRay.Tests/FileTransformationRegistrationTests.cs b/Jellyfin.Plugin.JRay.Tests/FileTransformationRegistrationTests.cs new file mode 100644 index 0000000..ae173fc --- /dev/null +++ b/Jellyfin.Plugin.JRay.Tests/FileTransformationRegistrationTests.cs @@ -0,0 +1,96 @@ +using System; +using System.Collections.Generic; +using Jellyfin.Plugin.JRay.Models; +using Jellyfin.Plugin.JRay.Services; +using Microsoft.Extensions.Logging; +using Xunit; + +namespace Jellyfin.Plugin.JRay.Tests; + +/// +/// JR-023 — absent the File Transformation plugin, disable *only* the overlay +/// and say so. There is no on-disk fallback (JR-021), so the failure path is +/// the one an admin will actually meet, and it has to be legible: silently +/// serving no overlay is indistinguishable from a broken install. +/// +/// The File Transformation assembly is not loaded in the test host, so +/// TryRegister exercises its not-found branch for real rather than through a +/// seam invented for the test. +/// +/// TRACES: UT-012, UT-013, UT-014, UT-015 | JR-023 +/// +public class FileTransformationRegistrationTests +{ + // UT-012 + [Fact] + public void TryRegister_WithPluginAbsent_ReturnsFalse() + { + Assert.False(FileTransformationRegistration.TryRegister(new CapturingLogger())); + } + + // UT-013 + [Fact] + public void TryRegister_WithPluginAbsent_WarnsNamingThePluginAndHowToInstallIt() + { + var logger = new CapturingLogger(); + + FileTransformationRegistration.TryRegister(logger); + + var entry = Assert.Single(logger.Entries); + + // Warning, not Information: the overlay is a headline feature and it is + // off. Logging this at Information buries it among startup chatter. + Assert.Equal(LogLevel.Warning, entry.Level); + + // The message has to carry the install URL, and must not promise the + // on-disk fallback that JR-021 removed. + Assert.Contains(FileTransformationRegistration.ManifestUrl, entry.Message, StringComparison.Ordinal); + Assert.DoesNotContain("falling back", entry.Message, StringComparison.OrdinalIgnoreCase); + } + + // UT-014 + [Fact] + public void TransformIndexHtml_WithOverlayDisabled_ReturnsContentsUnchanged() + { + // No Plugin instance exists in the test host, so OverlayEnabled is + // false — the same branch taken when an admin switches the overlay off. + var html = "
x
\n"; + + var result = FileTransformationRegistration.TransformIndexHtml( + new TransformationPayload { Contents = html }); + + Assert.Equal(html, result); + } + + // UT-015 + [Fact] + public void TransformIndexHtml_WithNullContents_ReturnsEmptyRatherThanThrowing() + { + // This callback is invoked by another plugin's code on every page + // served. Throwing here would break the web client itself, not just + // JRay's overlay. + var result = FileTransformationRegistration.TransformIndexHtml(new TransformationPayload()); + + Assert.Equal(string.Empty, result); + } + + private sealed class CapturingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = new(); + + public IDisposable? BeginScope(TState state) + where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Entries.Add((logLevel, formatter(state, exception))); + } + } +} diff --git a/Jellyfin.Plugin.JRay.Tests/Jellyfin.Plugin.JRay.Tests.csproj b/Jellyfin.Plugin.JRay.Tests/Jellyfin.Plugin.JRay.Tests.csproj index dbffd1c..eeeb369 100644 --- a/Jellyfin.Plugin.JRay.Tests/Jellyfin.Plugin.JRay.Tests.csproj +++ b/Jellyfin.Plugin.JRay.Tests/Jellyfin.Plugin.JRay.Tests.csproj @@ -35,6 +35,15 @@ + + + diff --git a/Jellyfin.Plugin.JRay/Services/FileTransformationRegistration.cs b/Jellyfin.Plugin.JRay/Services/FileTransformationRegistration.cs index 99da27f..455bcc3 100644 --- a/Jellyfin.Plugin.JRay/Services/FileTransformationRegistration.cs +++ b/Jellyfin.Plugin.JRay/Services/FileTransformationRegistration.cs @@ -23,9 +23,10 @@ namespace Jellyfin.Plugin.JRay.Services; /// AssemblyLoadContext from the real one, which is precisely the failure /// the reflection integration exists to avoid. /// -/// This is the mechanism JR-021 requires, but it does not by itself satisfy -/// JR-021 — that requirement is a prohibition, and it stays unmet while -/// can still write to disk. +/// This is the only route by which JRay reaches the web client. There is no +/// on-disk fallback (JR-021), so when registration fails the overlay is simply +/// disabled — which is why both failure paths log a warning naming the missing +/// plugin rather than quietly degrading. /// // TRACES: JR-020, JR-023 | PR-004 public static class FileTransformationRegistration @@ -72,7 +73,12 @@ public static class FileTransformationRegistration var registerMethod = ResolveRegisterMethod(); if (registerMethod is null) { - logger.LogInformation("JRay: File Transformation plugin not found; falling back to patching index.html on disk."); + logger.LogWarning( + "JRay: the File Transformation plugin was not found, so the pause overlay is " + + "disabled. JRay does not modify index.html on disk and has no fallback. " + + "Install it from {ManifestUrl} to enable the overlay; every other JRay " + + "feature is unaffected.", + ManifestUrl); return false; } @@ -84,7 +90,11 @@ public static class FileTransformationRegistration } catch (Exception ex) when (ex is TargetInvocationException or InvalidOperationException or JsonException or MissingMethodException) { - logger.LogWarning(ex, "JRay: failed to register with the File Transformation plugin; falling back to patching index.html on disk."); + logger.LogWarning( + ex, + "JRay: the File Transformation plugin is present but registration failed, so the " + + "pause overlay is disabled. JRay does not modify index.html on disk and has no " + + "fallback. Every other JRay feature is unaffected."); return false; } } diff --git a/SPEC.md b/SPEC.md index 7a77005..c77d9f6 100644 --- a/SPEC.md +++ b/SPEC.md @@ -477,13 +477,19 @@ Optionally, publish jRay through a repository manifest that also lists File Transformation, so one repository URL surfaces both. This is not a dependency mechanism; it removes a step and the chance of installing the wrong thing. -**Current:** all three implemented. Startup detection and registration, a warning -naming the plugin and its install URL, and a status banner on the configuration -page fed by `GET /Plugins/JRay/Status/Dependencies` — satisfied, or missing with -the manifest URL and what to do with it. The README now states the dependency -before the install step rather than after it. **Gap:** no test executes the -detection branch, so this stays `In Progress`; the config-page half is T4 and -verifiable only against a live server. +**Current:** all three implemented, and the detection half is tested +(UT-012…015). Startup detection and registration, a warning naming the plugin +and its install URL, and a status banner on the configuration page fed by +`GET /Plugins/JRay/Status/Dependencies`. The README states the dependency before +the install step rather than after it. + +Both failure-path log messages previously said the overlay was "falling back to +patching index.html on disk" — a claim JR-021 made false, and the worst place to +leave one: an admin reading it while debugging a missing overlay would go hunting +for a patch that no longer exists. UT-013 asserts the message names the install +URL and does *not* claim a fallback. + +**Gap:** the config-page banner is T4, verifiable only against a live server. ### JR-024 — Names render as text, never markup diff --git a/docs/requirements.md b/docs/requirements.md index 2c117cf..21cdf95 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -31,11 +31,17 @@ Tag code with `// TRACES: JR-012 | SR-002`. | UT-009 | **Prioritised series inside an ignored genre — series wins** | JR-016 | **Passing** | | UT-010 | Genre matching is case-insensitive | JR-016 | **Passing** | | UT-011 | A Series rule valued `Guid.Empty` does not swallow every movie | JR-016 | **Passing** | +| UT-012 | `TryRegister` returns `false` when File Transformation is absent | JR-023 | **Passing** | +| UT-013 | …and **warns** naming the install URL, with no "falling back" claim | JR-023 | **Passing** | +| UT-014 | Overlay disabled ⇒ `index.html` returned unchanged | JR-023 | **Passing** | +| UT-015 | Null contents return empty rather than throwing — this callback runs on every page another plugin serves | JR-023 | **Passing** | -All 11 execute and pass. The suite was also checked to **fail** on a mutation — -removing the newline-stripping from `RemoveInjection` fails UT-001 and nothing -else — because a suite that has only ever passed is not evidence that it tests -anything. +All 15 execute and pass. The suite was also checked to **fail** on two separate +mutations, because a suite that has only ever passed is not evidence that it +tests anything: removing the newline-stripping from `RemoveInjection` fails +UT-001 alone, and downgrading the missing-dependency warning to `Information` +fails UT-013 alone. In both cases the blast radius was one test, and the source +was restored and re-verified. `JR` is flat rather than split by theme. The plugin is one deployable with one audience, and the thematic grouping lives in the section headings below, where it @@ -95,7 +101,7 @@ coordinated `schema_version` bumps (SR-003). | JR-020 | Pause overlay: injected client script queries `jray?t=` and renders the scene's cast | **PR-001** | High | Done | | JR-021 | **jRay never injects into `index.html` on disk.** File Transformation is a hard dependency; there is no on-disk fallback. The only permitted write is JR-022's removal | PR-004 | High | **Done** | | JR-022 | Migration: remove any on-disk patch left by an earlier jRay, identified by the `` marker | PR-004 | High | **Done** (UT-001…006) | -| JR-023 | Absent the dependency, disable **only** the overlay and say so in the log and the config page; never bundle the assembly | PR-004 | Medium | In Progress | +| JR-023 | Absent the dependency, disable **only** the overlay and say so in the log and the config page; never bundle the assembly | PR-004 | Medium | **Done** (UT-012…015; config page is T4) | | JR-024 | Actor names and all server-supplied strings render as **text, never markup** | SR-004 | High | Done | ## Manifest exchange client (JR-025 … JR-037) @@ -188,7 +194,7 @@ thing when read across repos; reusing T3 for a live tier here would make a cross-repo reader count live-only requirements as covered. `Jellyfin.Plugin.JRay.Tests` (xUnit, in the solution) carries the T1 tier. It -builds clean alongside the plugin and all 11 tests pass. +builds clean alongside the plugin and all 15 tests pass. ### Running the suite on a box without the web runtime @@ -201,16 +207,26 @@ work anywhere, and both are load-bearing rather than incidental: runtime the plugin does not otherwise need. - **`DisableTransitiveFrameworkReferences=true`.** The plugin framework-references `Microsoft.AspNetCore.App` through `Jellyfin.Controller`, - and that flows into anything referencing it — so the test host would demand a - web runtime even for tests that touch no web type. .NET resolves assemblies - lazily, so pure-logic types load fine without it. + and that flows into anything referencing it — so the test host would otherwise + demand a web runtime that no version of exists in the Arch repositories for + .NET 9 (8 and 10 only). +- **Explicit `Jellyfin.Controller` / `Jellyfin.Model` references.** The plugin + sets `ExcludeAssets=runtime` on both, because at run time the *server* supplies + them and shipping copies would risk loading a second, different + `MediaBrowser.Common`. The test host is not the server, so it must bring its + own — hence the same packages without that exclusion, and only in the test + project. -**This is a T1-tier decision, not a workaround to unwind.** These tests exercise -string and rule logic; requiring an ASP.NET Core runtime to run them would be -incidental coupling. **T2 — controllers and authorisation — genuinely needs that -runtime**, and those tests belong in a second project that keeps the reference. -Note that no ASP.NET Core 9 exists in the Arch repositories (8 and 10 only), so -that project will lean on `RollForward` too. +Those two together are what make it work: the first drops the demand for the web +*framework*, the second supplies the Jellyfin *assemblies*. Cutting the framework +reference alone is not enough — `ILogger` and `MediaBrowser.Common` live in the +excluded assets, so anything beyond genuinely dependency-free logic fails to load +with `FileNotFoundException` at run time rather than at build. + +**T2 — controllers and authorisation — is still a separate matter**, since +instantiating MVC types needs the ASP.NET Core runtime itself, not just its +reference assemblies. Those tests belong in a second project that keeps the +framework reference and leans on `RollForward` to reach the 10.0 runtime. ### Per-requirement verification plan @@ -238,7 +254,7 @@ that project will lean on `RollForward` too. | JR-018 | T1 | `covered / (total - ignored)` | Item carrying two genres counts in both rows | | JR-021 | **static** | No code path *adds* the script tag to `index.html` | `scripts/checks/no-index-injection.sh`. Removal (JR-022) is the one permitted write, so the check is on injection, not on writing. Verified to **fail** on a reintroduced `Apply()` and on reintroduced `ReplaceLast` injection, not merely to pass today | | JR-022 | T1 | A marked legacy patch is removed; unmarked content untouched | Foreign plugin's injection left intact | -| JR-023 | T1 + **T4** | Absent dependency disables only the overlay | Detection unit-testable; config-page display is live | +| JR-023 | T1 + **T4** | Absent dependency disables only the overlay, and the log says so | Detection and log content covered by UT-012…015; the config-page banner is T4, verifiable only against a live server | | JR-024 | T1 | A name containing markup renders escaped | `