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 | `