docs(plan): external-logo download-on-save implementation plan
10 tasks, TDD, no schema change. Extracts RemoteImageDecodeBudget (Core) + RemoteImageValidator (Infra), adds IRemoteLogoCacher, wires the 3 channel handlers + UploadArtworkHandler, a startup migration for existing URL rows, and the SPA preview/copy changes. refs #525
This commit is contained in:
@@ -0,0 +1,877 @@
|
||||
# External Channel-Logo Download-On-Save Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Turn an external channel-logo URL into a download-on-save input method: on save the URL is fetched, decode-validated, and cached under a content-hash name so it becomes byte-identical to an uploaded logo; the render path never fetches a logo again.
|
||||
|
||||
**Architecture:** A pure arithmetic budget (`RemoteImageDecodeBudget`, Core) is shared by the render path and the new save path. An Infrastructure decoder (`RemoteImageValidator`) performs the ImageSharp identify/decode/validate step; a Core interface `IRemoteLogoCacher` (Infrastructure impl) composes fetch → validate → cache and returns the cache name or a `BaseError`. The three channel handlers call it; `UploadArtworkHandler` reuses the validator; a startup `BackgroundService` migrates existing URL rows.
|
||||
|
||||
**Tech Stack:** C#/.NET 10, MediatR CQRS, LanguageExt (`Either`/`Validation`/`Option`), EF Core (SQLite + MySql), SixLabors.ImageSharp 3.1.12, NUnit + Shouldly + NSubstitute, ChicoryTV React SPA (Vite + TS).
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- **Layering (enforced by `ErsatzTV.Architecture.Tests`):** `Core` may depend on `FFmpeg` only — no EF, no Infrastructure, no ImageSharp-in-a-way-that-breaks-purity. `Application` may depend on `Core` + `Infrastructure` abstractions. Put interfaces in `Core`, implementations in `Infrastructure`, DI wiring in `ErsatzTV/Startup.cs`.
|
||||
- **NUnit + Shouldly + NSubstitute only.** Never xUnit. Handler tests extend `ChannelHandlerTestBase` (`ErsatzTV.Tests/Support/ChannelHandlerTestBase.cs`) using `InMemoryTvContext`.
|
||||
- **Decode budgets (verbatim from #511, do not change the numbers):** `MaxRemoteDecodedPixels = 50_000_000`; `MaxRemoteFrames = 600`. Decode bound must be imposed on the DECODER (`DecoderOptions.MaxFrames`) and re-verified against the decoded image — header frame counts lie (APNG reports 0).
|
||||
- **Fix formatting as you touch it:** run `dotnet format ErsatzTV.sln --include <changed .cs>` under `bash -c` before committing; no UTF-8 BOM on any touched `.cs` (`head -c3 | xxd -p` must not be `efbbbf`). `charset=utf-8` in `.editorconfig`.
|
||||
- **Dual-provider migrations:** any `TvContext` model change needs `scripts/add-migration.sh <Name>` (SQLite + MySql). This plan adds **no** schema change (reuses `Artwork.Path`), so no migration is expected — if you find you need one, stop and reconsider.
|
||||
- **Docs-in-same-PR:** update `docs/decisions.md`, `docs/channels.md`, `docs/api-conventions.md` in the implementation PR (Task 9). Regenerate OpenAPI (`./scripts/update-openapi.sh` + `npm run generate:api`) only if a response shape changes — this plan changes only error status/messages, not shapes, so likely just the endpoint prose.
|
||||
- **Central Package Management:** no `Version=` on `<PackageReference>`; versions live in `Directory.Packages.props`.
|
||||
- **Content-hash name, not GUID:** reuse `IImageCache.SaveArtworkToCache` (MD5-of-bytes). No new naming scheme.
|
||||
- **Fixes #525.**
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Extract the pure decode budget into `RemoteImageDecodeBudget` (Core)
|
||||
|
||||
Lift the pure arithmetic budget out of `ImageElementBase` so both the render path and the save path share one implementation. No behavior change — this is a move + delegate.
|
||||
|
||||
**Files:**
|
||||
- Create: `ErsatzTV.Core/Images/RemoteImageDecodeBudget.cs`
|
||||
- Modify: `ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs` (delete the moved members, delegate to the new class)
|
||||
- Create: `ErsatzTV.Core.Tests/Images/RemoteImageDecodeBudgetTests.cs`
|
||||
- Move (into the test above): the budget-arithmetic cases from `ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs` (keep the ImageSharp-decode tests where they are)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces:
|
||||
- `RemoteImageDecodeBudget.MaxRemoteDecodedPixels` (`const long = 50_000_000`)
|
||||
- `RemoteImageDecodeBudget.MaxRemoteFrames` (`const int = 600`)
|
||||
- `static void EnsureDimensionsAffordable(int width, int height, Uri uri)`
|
||||
- `static int AffordableFrames(int width, int height)`
|
||||
- `static void EnsureDecodeAffordable(int width, int height, int frameCount, Uri uri)`
|
||||
- Consumes: nothing (pure).
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Create `ErsatzTV.Core.Tests/Images/RemoteImageDecodeBudgetTests.cs`:
|
||||
|
||||
```csharp
|
||||
using ErsatzTV.Core.Images;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Core.Tests.Images;
|
||||
|
||||
[TestFixture]
|
||||
public class RemoteImageDecodeBudgetTests
|
||||
{
|
||||
private static readonly Uri Uri = new("https://example.com/logo.png");
|
||||
|
||||
// the product is the real bound: 2500x2500 x600 is affordable on each axis alone but not together
|
||||
[Test]
|
||||
public void Should_Reject_Dimensions_And_Frames_Affordable_Alone_But_Not_Together()
|
||||
{
|
||||
((long)2500 * 2500).ShouldBeLessThanOrEqualTo(RemoteImageDecodeBudget.MaxRemoteDecodedPixels);
|
||||
600.ShouldBeLessThanOrEqualTo(RemoteImageDecodeBudget.MaxRemoteFrames);
|
||||
|
||||
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
|
||||
() => RemoteImageDecodeBudget.EnsureDecodeAffordable(2500, 2500, 600, Uri));
|
||||
ex.Message.ShouldContain("pixel limit");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Should_Reject_Too_Many_Frames_Even_When_Each_Is_Tiny() =>
|
||||
Should.Throw<InvalidOperationException>(
|
||||
() => RemoteImageDecodeBudget.EnsureDecodeAffordable(8, 8, RemoteImageDecodeBudget.MaxRemoteFrames + 1, Uri))
|
||||
.Message.ShouldContain("frame limit");
|
||||
|
||||
[Test]
|
||||
public void Should_Reject_A_Single_Oversized_Frame() =>
|
||||
Should.Throw<InvalidOperationException>(
|
||||
() => RemoteImageDecodeBudget.EnsureDimensionsAffordable(30000, 30000, Uri))
|
||||
.Message.ShouldContain("pixel limit");
|
||||
|
||||
[Test]
|
||||
public void Should_Allow_A_Single_Large_Still_Within_Budget() =>
|
||||
Should.NotThrow(() => RemoteImageDecodeBudget.EnsureDecodeAffordable(7680, 4320, 1, Uri));
|
||||
|
||||
[Test]
|
||||
public void Should_Charge_At_Least_One_Frame_When_Header_Reports_None() =>
|
||||
Should.Throw<InvalidOperationException>(
|
||||
() => RemoteImageDecodeBudget.EnsureDecodeAffordable(30000, 30000, 0, Uri));
|
||||
|
||||
[Test]
|
||||
public void Should_Afford_Fewer_Frames_As_Frames_Get_Larger()
|
||||
{
|
||||
RemoteImageDecodeBudget.AffordableFrames(8, 8).ShouldBe(RemoteImageDecodeBudget.MaxRemoteFrames);
|
||||
RemoteImageDecodeBudget.AffordableFrames(1000, 1000).ShouldBe(50);
|
||||
RemoteImageDecodeBudget.AffordableFrames(7000, 7000).ShouldBe(1);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Core.Tests/ErsatzTV.Core.Tests.csproj --filter "FullyQualifiedName~RemoteImageDecodeBudget"`
|
||||
Expected: FAIL — `RemoteImageDecodeBudget` does not exist.
|
||||
|
||||
- [ ] **Step 3: Create `RemoteImageDecodeBudget`**
|
||||
|
||||
Create `ErsatzTV.Core/Images/RemoteImageDecodeBudget.cs` (bodies copied verbatim from `ImageElementBase`, only the type moved):
|
||||
|
||||
```csharp
|
||||
namespace ErsatzTV.Core.Images;
|
||||
|
||||
/// <summary>
|
||||
/// The decode-budget policy for a remote image, as pure arithmetic so it can be enforced both at
|
||||
/// render time (graphics engine) and at save time (logo download) without materializing
|
||||
/// multi-gigabyte images. Extracted from ImageElementBase for reuse. (ersatztv#525, from #511.)
|
||||
/// </summary>
|
||||
public static class RemoteImageDecodeBudget
|
||||
{
|
||||
/// <summary>
|
||||
/// Ceiling on TOTAL decoded pixels — width x height x frames, as one product. Checking
|
||||
/// dimensions and frame count independently does not bound the decode: a 60 KiB 2500x2500 x600
|
||||
/// GIF passes both a 50 MP dimension check and a 600 frame check and costs ~14 GiB.
|
||||
/// </summary>
|
||||
public const long MaxRemoteDecodedPixels = 50_000_000;
|
||||
|
||||
/// <summary>Frame ceiling, a cheap legible guard against absurd counts of tiny frames.</summary>
|
||||
public const int MaxRemoteFrames = 600;
|
||||
|
||||
public static void EnsureDimensionsAffordable(int width, int height, Uri uri)
|
||||
{
|
||||
long pixels = (long)width * height;
|
||||
if (pixels > MaxRemoteDecodedPixels)
|
||||
{
|
||||
throw new InvalidOperationException(
|
||||
$"Remote image {uri} is {width}x{height} ({pixels} pixels), over the "
|
||||
+ $"{MaxRemoteDecodedPixels} pixel limit");
|
||||
}
|
||||
}
|
||||
|
||||
public static int AffordableFrames(int width, int height)
|
||||
{
|
||||
long perFrame = Math.Max((long)width * height, 1);
|
||||
return (int)Math.Clamp(MaxRemoteDecodedPixels / perFrame, 1, MaxRemoteFrames);
|
||||
}
|
||||
|
||||
public static void EnsureDecodeAffordable(int width, int height, int frameCount, Uri uri)
|
||||
{
|
||||
int frames = Math.Max(frameCount, 1);
|
||||
if (frames > MaxRemoteFrames)
|
||||
{
|
||||
throw new InvalidOperationException(
|
||||
$"Remote image {uri} has {frames} frames, over the {MaxRemoteFrames} frame limit");
|
||||
}
|
||||
|
||||
long totalPixels = (long)width * height * frames;
|
||||
if (totalPixels > MaxRemoteDecodedPixels)
|
||||
{
|
||||
throw new InvalidOperationException(
|
||||
$"Remote image {uri} decodes to {width}x{height} x{frames} frames "
|
||||
+ $"({totalPixels} pixels), over the {MaxRemoteDecodedPixels} pixel limit");
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Delegate from `ImageElementBase`**
|
||||
|
||||
In `ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs`: delete the `MaxRemoteDecodedPixels`, `MaxRemoteFrames`, `EnsureDimensionsAffordable`, `AffordableFrames`, `EnsureDecodeAffordable` members. Keep `MaxRemoteScaledPixels` + `EnsureScaledFramesAffordable` (retention budget — render-only). Add `using ErsatzTV.Core.Images;` and update the three call sites inside `DecodeRemoteImage`:
|
||||
|
||||
```csharp
|
||||
RemoteImageDecodeBudget.EnsureDimensionsAffordable(info.Width, info.Height, uri);
|
||||
int affordableFrames = RemoteImageDecodeBudget.AffordableFrames(info.Width, info.Height);
|
||||
// ... after decode:
|
||||
RemoteImageDecodeBudget.EnsureDecodeAffordable(image.Width, image.Height, image.Frames.Count, uri);
|
||||
```
|
||||
|
||||
Delete the now-duplicated arithmetic tests from `RemoteImageDecodeLimitTests.cs` (the `EnsureDecodeAffordable`/`AffordableFrames`/`EnsureDimensionsAffordable` cases moved to Task 1's test). KEEP its ImageSharp-decode tests (`DecodeRemoteImage`, APNG regression, CRC-crafted PNG) — those move to Task 2.
|
||||
|
||||
- [ ] **Step 5: Run tests to verify they pass**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Core.Tests/ErsatzTV.Core.Tests.csproj --filter "FullyQualifiedName~RemoteImageDecodeBudget"` → PASS
|
||||
Run: `dotnet build ErsatzTV.sln` → `Build succeeded`, 0 warnings (warnings are errors).
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
bash -c 'dotnet format ErsatzTV.sln --no-restore --include ErsatzTV.Core/Images/RemoteImageDecodeBudget.cs ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs ErsatzTV.Core.Tests/Images/RemoteImageDecodeBudgetTests.cs ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs'
|
||||
git add -A && git commit -m "refactor(525): extract RemoteImageDecodeBudget from ImageElementBase"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: `RemoteImageValidator` (Infrastructure) — decode + budget-validate a stream
|
||||
|
||||
Extract the ImageSharp identify/decode/validate step so both the render path and the save path share it. It returns the decoded `Image` (render needs it; save disposes it). This is the `DecodeRemoteImage` logic relocated behind an interface.
|
||||
|
||||
**Files:**
|
||||
- Create: `ErsatzTV.Core/Interfaces/Images/IRemoteImageValidator.cs`
|
||||
- Create: `ErsatzTV.Infrastructure/Images/RemoteImageValidator.cs`
|
||||
- Modify: `ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs` (delegate `DecodeRemoteImage` to the validator; it is constructed with `IRemoteImageFetcher` today — add `IRemoteImageValidator` alongside)
|
||||
- Move: the ImageSharp-decode tests from `RemoteImageDecodeLimitTests.cs` → `ErsatzTV.Infrastructure.Tests/Images/RemoteImageValidatorTests.cs`
|
||||
- Modify: `ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsEngine.cs`, `Image/ImageElement.cs`, `Image/WatermarkElement.cs` (thread the validator through, same pattern as `IRemoteImageFetcher`)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces:
|
||||
- `IRemoteImageValidator.DecodeAndValidate(Stream stream, Uri uri, CancellationToken) : Task<Image>` (SixLabors `Image`; throws `InvalidOperationException` on a budget violation, ImageSharp exceptions on a corrupt stream; caller owns the returned `Image`)
|
||||
- Consumes: `RemoteImageDecodeBudget` (Task 1).
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — move the existing decode tests and retarget them
|
||||
|
||||
Create `ErsatzTV.Infrastructure.Tests/Images/RemoteImageValidatorTests.cs` by moving the `DecodeRemoteImage` tests out of `RemoteImageDecodeLimitTests.cs` and calling the validator instead. Key cases (bodies come from the existing tests — reuse the crafted-PNG + APNG helpers verbatim):
|
||||
|
||||
```csharp
|
||||
using ErsatzTV.Infrastructure.Images;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
using SixLabors.ImageSharp;
|
||||
using Image = SixLabors.ImageSharp.Image;
|
||||
|
||||
namespace ErsatzTV.Infrastructure.Tests.Images;
|
||||
|
||||
[TestFixture]
|
||||
public class RemoteImageValidatorTests
|
||||
{
|
||||
private static readonly Uri Uri = new("https://example.com/logo.png");
|
||||
private readonly IRemoteImageValidator _validator = new RemoteImageValidator();
|
||||
|
||||
[Test]
|
||||
public async Task Should_Decode_A_Normal_Image()
|
||||
{
|
||||
await using MemoryStream stream = await RealPng(64, 32);
|
||||
using Image image = await _validator.DecodeAndValidate(stream, Uri, CancellationToken.None);
|
||||
image.Width.ShouldBe(64);
|
||||
image.Height.ShouldBe(32);
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Reject_A_Decompression_Bomb_By_Declared_Dimensions()
|
||||
{
|
||||
await using MemoryStream stream = PngHeaderDeclaring(30000, 30000);
|
||||
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
|
||||
() => _validator.DecodeAndValidate(stream, Uri, CancellationToken.None));
|
||||
ex.Message.ShouldContain("pixel limit");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Reject_An_Apng_Whose_Header_Under_Reports_Its_Frames()
|
||||
{
|
||||
await using MemoryStream stream = Apng(64, 64, RemoteImageDecodeBudgetFrames + 100);
|
||||
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
|
||||
() => _validator.DecodeAndValidate(stream, Uri, CancellationToken.None));
|
||||
ex.Message.ShouldContain("frame limit");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Decode_An_Apng_That_A_Default_Identify_Cannot_Read()
|
||||
{
|
||||
await using MemoryStream stream = Apng(288, 288, 60);
|
||||
stream.Position = 0;
|
||||
await Should.ThrowAsync<Exception>(() => Image.IdentifyAsync(stream));
|
||||
stream.Position = 0;
|
||||
using Image image = await _validator.DecodeAndValidate(stream, Uri, CancellationToken.None);
|
||||
image.Frames.Count.ShouldBe(60);
|
||||
}
|
||||
|
||||
// (move RealPng / PngHeaderDeclaring / Apng / Crc32 helpers here verbatim from RemoteImageDecodeLimitTests)
|
||||
private const int RemoteImageDecodeBudgetFrames = 600;
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Infrastructure.Tests/ErsatzTV.Infrastructure.Tests.csproj --filter "FullyQualifiedName~RemoteImageValidator"`
|
||||
Expected: FAIL — `RemoteImageValidator` / `IRemoteImageValidator` do not exist.
|
||||
|
||||
- [ ] **Step 3: Create the interface and implementation**
|
||||
|
||||
`ErsatzTV.Core/Interfaces/Images/IRemoteImageValidator.cs`:
|
||||
|
||||
```csharp
|
||||
using SixLabors.ImageSharp;
|
||||
|
||||
namespace ErsatzTV.Core.Interfaces.Images;
|
||||
|
||||
/// <summary>
|
||||
/// Decodes a remote image and enforces the decode budget on the decoder itself, returning the
|
||||
/// decoded image. Shared by the graphics engine (which composites the result) and the logo save
|
||||
/// path (which validates then discards it, caching the original bytes). (ersatztv#525)
|
||||
/// </summary>
|
||||
public interface IRemoteImageValidator
|
||||
{
|
||||
Task<Image> DecodeAndValidate(Stream stream, Uri uri, CancellationToken cancellationToken);
|
||||
}
|
||||
```
|
||||
|
||||
`ErsatzTV.Infrastructure/Images/RemoteImageValidator.cs` — move the body of `ImageElementBase.DecodeRemoteImage` here verbatim (the `!CanSeek` guard, the `MaxFrames = 1` Identify workaround, `RemoteImageDecodeBudget.*` calls, the `MaxFrames = affordable + 2` decode, the post-decode re-verify + dispose-on-throw). Class:
|
||||
|
||||
```csharp
|
||||
using ErsatzTV.Core.Images;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using SixLabors.ImageSharp;
|
||||
using SixLabors.ImageSharp.Formats;
|
||||
|
||||
namespace ErsatzTV.Infrastructure.Images;
|
||||
|
||||
public class RemoteImageValidator : IRemoteImageValidator
|
||||
{
|
||||
public async Task<Image> DecodeAndValidate(Stream stream, Uri uri, CancellationToken cancellationToken)
|
||||
{
|
||||
// <verbatim body of ImageElementBase.DecodeRemoteImage, RemoteImageDecodeBudget.* for the
|
||||
// three budget calls; see that method for the exact code and comments>
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Note (`ErsatzTV.Core` referencing `SixLabors.ImageSharp`): the interface's return type is `SixLabors.ImageSharp.Image`. `ErsatzTV.Core` already references ImageSharp transitively? VERIFY before relying on it: `grep -r "SixLabors.ImageSharp" ErsatzTV.Core/ErsatzTV.Core.csproj Directory.Packages.props`. If Core does NOT reference ImageSharp, change the interface to return the fetched bytes / a small `record RemoteImageInfo(int Width, int Height, int FrameCount)` instead of `Image`, keep the `Image` inside Infrastructure, and have the render path call a concrete Infrastructure method. Pick the byte/record option if in doubt — it keeps Core pure and the save path only needs "valid or not", not the `Image`.
|
||||
|
||||
- [ ] **Step 4: Delegate `DecodeRemoteImage` and thread the validator**
|
||||
|
||||
`ImageElementBase`: constructor becomes `(IRemoteImageFetcher remoteImageFetcher, IRemoteImageValidator remoteImageValidator)`; `DecodeRemoteImage` body becomes `remoteImageValidator.DecodeAndValidate(...)`. Thread the new dep through `GraphicsEngine` (inject `IRemoteImageValidator`, pass to `new WatermarkElement(...)` / `new ImageElement(...)`) exactly as `IRemoteImageFetcher` is threaded today (see the #511 diff for the pattern). Register in `Startup.cs`: `services.AddScoped<IRemoteImageValidator, RemoteImageValidator>();`.
|
||||
|
||||
- [ ] **Step 5: Run tests to verify they pass**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Infrastructure.Tests/ErsatzTV.Infrastructure.Tests.csproj --filter "FullyQualifiedName~Streaming|FullyQualifiedName~Images"` → PASS
|
||||
Run: `dotnet build ErsatzTV.sln` → `Build succeeded`.
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
bash -c 'dotnet format ErsatzTV.sln --no-restore --include <all touched .cs>'
|
||||
git add -A && git commit -m "refactor(525): extract RemoteImageValidator; render path delegates to it"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: `IRemoteLogoCacher` — fetch + validate + cache a URL to a cache name
|
||||
|
||||
The save-path primitive: given a URL, fetch (hardened, #511), validate (Task 2), and cache the original bytes (`IImageCache`), returning the content-hash name or a `BaseError`. This is what the handlers call.
|
||||
|
||||
**Files:**
|
||||
- Create: `ErsatzTV.Core/Interfaces/Images/IRemoteLogoCacher.cs`
|
||||
- Create: `ErsatzTV.Infrastructure/Images/RemoteLogoCacher.cs`
|
||||
- Create: `ErsatzTV.Infrastructure.Tests/Images/RemoteLogoCacherTests.cs`
|
||||
- Modify: `ErsatzTV/Startup.cs` (register)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: `IRemoteLogoCacher.CacheFromUrl(Uri uri, CancellationToken) : Task<Either<BaseError, string>>` — Right = bare cache file name (as `IImageCache.SaveArtworkToCache` returns), Left = a `BaseError` whose message names the failure (timeout / status / not-image / over-size / over-budget / cache write).
|
||||
- Consumes: `IRemoteImageFetcher.Fetch` (Task from #511), `IRemoteImageValidator.DecodeAndValidate` (Task 2), `IImageCache.SaveArtworkToCache`.
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Create `ErsatzTV.Infrastructure.Tests/Images/RemoteLogoCacherTests.cs`:
|
||||
|
||||
```csharp
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Streaming;
|
||||
using ErsatzTV.Infrastructure.Images;
|
||||
using LanguageExt;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
using SixLabors.ImageSharp;
|
||||
using SixLabors.ImageSharp.Formats.Png;
|
||||
using SixLabors.ImageSharp.PixelFormats;
|
||||
using Image = SixLabors.ImageSharp.Image;
|
||||
|
||||
namespace ErsatzTV.Infrastructure.Tests.Images;
|
||||
|
||||
[TestFixture]
|
||||
public class RemoteLogoCacherTests
|
||||
{
|
||||
private static readonly Uri Uri = new("https://example.com/logo.png");
|
||||
|
||||
[Test]
|
||||
public async Task Should_Fetch_Validate_And_Cache_Returning_The_Name()
|
||||
{
|
||||
MemoryStream png = await RealPng(64, 64);
|
||||
var fetcher = Substitute.For<IRemoteImageFetcher>();
|
||||
fetcher.Fetch(Uri, Arg.Any<CancellationToken>()).Returns(png);
|
||||
var validator = Substitute.For<IRemoteImageValidator>();
|
||||
validator.DecodeAndValidate(png, Uri, Arg.Any<CancellationToken>()).Returns(new Image<Rgba32>(64, 64));
|
||||
var cache = Substitute.For<IImageCache>();
|
||||
cache.SaveArtworkToCache(Arg.Any<Stream>(), ArtworkKind.Logo).Returns(Right<BaseError, string>("abc123"));
|
||||
|
||||
var cacher = new RemoteLogoCacher(fetcher, validator, cache);
|
||||
Either<BaseError, string> result = await cacher.CacheFromUrl(Uri, CancellationToken.None);
|
||||
|
||||
result.IsRight.ShouldBeTrue();
|
||||
result.IfRight(name => name.ShouldBe("abc123"));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Return_Left_When_The_Fetch_Throws()
|
||||
{
|
||||
var fetcher = Substitute.For<IRemoteImageFetcher>();
|
||||
fetcher.Fetch(Uri, Arg.Any<CancellationToken>()).Returns<Stream>(_ => throw new TimeoutException("timed out"));
|
||||
var cacher = new RemoteLogoCacher(fetcher, Substitute.For<IRemoteImageValidator>(), Substitute.For<IImageCache>());
|
||||
|
||||
Either<BaseError, string> result = await cacher.CacheFromUrl(Uri, CancellationToken.None);
|
||||
|
||||
result.IsLeft.ShouldBeTrue();
|
||||
result.IfLeft(e => e.Value.ShouldContain("timed out"));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Return_Left_When_Validation_Rejects_A_Bomb()
|
||||
{
|
||||
MemoryStream png = await RealPng(64, 64);
|
||||
var fetcher = Substitute.For<IRemoteImageFetcher>();
|
||||
fetcher.Fetch(Uri, Arg.Any<CancellationToken>()).Returns(png);
|
||||
var validator = Substitute.For<IRemoteImageValidator>();
|
||||
validator.DecodeAndValidate(png, Uri, Arg.Any<CancellationToken>())
|
||||
.Returns<Image>(_ => throw new InvalidOperationException("over the 50000000 pixel limit"));
|
||||
var cacher = new RemoteLogoCacher(fetcher, validator, Substitute.For<IImageCache>());
|
||||
|
||||
Either<BaseError, string> result = await cacher.CacheFromUrl(Uri, CancellationToken.None);
|
||||
|
||||
result.IsLeft.ShouldBeTrue();
|
||||
result.IfLeft(e => e.Value.ShouldContain("pixel limit"));
|
||||
}
|
||||
|
||||
private static async Task<MemoryStream> RealPng(int w, int h)
|
||||
{
|
||||
using var img = new Image<Rgba32>(w, h);
|
||||
var ms = new MemoryStream();
|
||||
await img.SaveAsync(ms, new PngEncoder());
|
||||
ms.Position = 0;
|
||||
return ms;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Infrastructure.Tests/ErsatzTV.Infrastructure.Tests.csproj --filter "FullyQualifiedName~RemoteLogoCacher"`
|
||||
Expected: FAIL — `RemoteLogoCacher` / `IRemoteLogoCacher` do not exist.
|
||||
|
||||
- [ ] **Step 3: Create the interface and implementation**
|
||||
|
||||
`ErsatzTV.Core/Interfaces/Images/IRemoteLogoCacher.cs`:
|
||||
|
||||
```csharp
|
||||
using ErsatzTV.Core;
|
||||
using LanguageExt;
|
||||
|
||||
namespace ErsatzTV.Core.Interfaces.Images;
|
||||
|
||||
/// <summary>
|
||||
/// Fetches an external logo URL, validates it against the decode budget, and stores it in the
|
||||
/// image cache — turning a URL into a cache name so it is thereafter identical to an uploaded
|
||||
/// logo. Errors are returned, not thrown, so a save handler can surface a 400. (ersatztv#525)
|
||||
/// </summary>
|
||||
public interface IRemoteLogoCacher
|
||||
{
|
||||
Task<Either<BaseError, string>> CacheFromUrl(Uri uri, CancellationToken cancellationToken);
|
||||
}
|
||||
```
|
||||
|
||||
`ErsatzTV.Infrastructure/Images/RemoteLogoCacher.cs`:
|
||||
|
||||
```csharp
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Streaming;
|
||||
using LanguageExt;
|
||||
using SixLabors.ImageSharp;
|
||||
|
||||
namespace ErsatzTV.Infrastructure.Images;
|
||||
|
||||
public class RemoteLogoCacher(
|
||||
IRemoteImageFetcher fetcher,
|
||||
IRemoteImageValidator validator,
|
||||
IImageCache imageCache) : IRemoteLogoCacher
|
||||
{
|
||||
public async Task<Either<BaseError, string>> CacheFromUrl(Uri uri, CancellationToken cancellationToken)
|
||||
{
|
||||
try
|
||||
{
|
||||
await using Stream stream = await fetcher.Fetch(uri, cancellationToken);
|
||||
|
||||
// validate by decoding under the budget; dispose the decoded image, we cache raw bytes
|
||||
using (Image _ = await validator.DecodeAndValidate(stream, uri, cancellationToken))
|
||||
{
|
||||
}
|
||||
|
||||
stream.Position = 0;
|
||||
return await imageCache.SaveArtworkToCache(stream, ArtworkKind.Logo);
|
||||
}
|
||||
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
|
||||
{
|
||||
throw;
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
return BaseError.New($"Could not download logo from {uri}: {ex.Message}");
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Note: `IRemoteImageFetcher.Fetch` returns a seekable, fully-buffered stream at position 0 (its contract), so `stream.Position = 0` after validation rewinds it for the cache write. If Task 2 changed the validator to return bytes/record instead of `Image`, adjust the `using` accordingly.
|
||||
|
||||
Register in `Startup.cs`: `services.AddScoped<IRemoteLogoCacher, RemoteLogoCacher>();`.
|
||||
|
||||
- [ ] **Step 4: Run tests to verify they pass**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Infrastructure.Tests/ErsatzTV.Infrastructure.Tests.csproj --filter "FullyQualifiedName~RemoteLogoCacher"` → PASS
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
bash -c 'dotnet format ErsatzTV.sln --no-restore --include <touched .cs>'
|
||||
git add -A && git commit -m "feat(525): add RemoteLogoCacher (fetch + validate + cache a logo URL)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: `UpdateChannelHandler` downloads a URL logo on save
|
||||
|
||||
Route an incoming external-URL logo through `IRemoteLogoCacher` before it reaches `Artwork.Path`, so a saved channel never stores a URL. A cacher failure fails the save.
|
||||
|
||||
**Files:**
|
||||
- Modify: `ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs` (inject `IRemoteLogoCacher`; resolve URL → cache name inside `ApplyUpdateRequest`; surface failure)
|
||||
- Modify: `ErsatzTV.Tests/Application/Channels/UpdateChannelHandlerTests.cs`
|
||||
- Modify: `ErsatzTV.Tests/Support/ChannelHandlerTestBase.cs` (add a substituted `IRemoteLogoCacher`)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `IRemoteLogoCacher.CacheFromUrl` (Task 3).
|
||||
- Produces: on an external-URL logo, `Artwork.Path` holds the cache name (not the URL); a cacher `Left` becomes a `Left<BaseError, ChannelViewModel>` from `Handle`.
|
||||
|
||||
Design note on error flow: `ApplyUpdateRequest` currently returns `Task<ChannelViewModel>` and is invoked via `validation.Apply(...)`. The download can fail, so it must be able to produce a `Left`. Change the logo resolution to happen in `Handle` *before* `ApplyUpdateRequest` (so the `Either` composes cleanly), OR change `ApplyUpdateRequest` to return `Task<Either<BaseError, ChannelViewModel>>` and `Bind` it. The plan uses the first (resolve-before-apply) to keep `ApplyUpdateRequest` synchronous-shaped.
|
||||
|
||||
- [ ] **Step 1: Write the failing tests**
|
||||
|
||||
Add to `UpdateChannelHandlerTests.cs`:
|
||||
|
||||
```csharp
|
||||
[Test]
|
||||
public async Task Should_Download_External_Url_Logo_And_Store_Cache_Name()
|
||||
{
|
||||
Channel channel = await SeedChannel(number: "5");
|
||||
RemoteLogoCacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, string>("cachedhash"));
|
||||
|
||||
Either<BaseError, ChannelViewModel> result = await MakeHandler().Handle(
|
||||
MakeUpdate(channel.Id, number: "5", logoPath: "https://example.com/logo.png"),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsRight.ShouldBeTrue();
|
||||
await using TvContext db = Db.CreateContext();
|
||||
Artwork logo = db.Channels.Include(c => c.Artwork).Single(c => c.Id == channel.Id)
|
||||
.Artwork.Single(a => a.ArtworkKind == ArtworkKind.Logo);
|
||||
logo.Path.ShouldBe("cachedhash");
|
||||
logo.IsExternalUrl().ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Fail_The_Save_When_The_Logo_Download_Fails()
|
||||
{
|
||||
Channel channel = await SeedChannel(number: "5");
|
||||
RemoteLogoCacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Left<BaseError, string>(BaseError.New("Could not download logo: host timed out")));
|
||||
|
||||
Either<BaseError, ChannelViewModel> result = await MakeHandler().Handle(
|
||||
MakeUpdate(channel.Id, number: "5", logoPath: "https://example.com/logo.png"),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsLeft.ShouldBeTrue();
|
||||
LeftOf(result).Value.ShouldContain("Could not download logo");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Not_Call_The_Cacher_For_An_Uploaded_Logo_Path()
|
||||
{
|
||||
Channel channel = await SeedChannel(number: "5");
|
||||
await MakeHandler().Handle(
|
||||
MakeUpdate(channel.Id, number: "5", logoPath: "iptv/logos/deadbeef"),
|
||||
CancellationToken.None);
|
||||
await RemoteLogoCacher.DidNotReceive().CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>());
|
||||
}
|
||||
```
|
||||
|
||||
(Add `RemoteLogoCacher` to `ChannelHandlerTestBase` as `protected IRemoteLogoCacher RemoteLogoCacher = Substitute.For<IRemoteLogoCacher>();` set in `BaseSetUp`, and to `MakeHandler()`/`MakeUpdate` a `logoPath` parameter. If `SeedChannel` doesn't exist, use the fixture's existing channel-seeding helper — check the file.)
|
||||
|
||||
- [ ] **Step 2: Run tests to verify they fail**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Tests/ErsatzTV.Tests.csproj --filter "FullyQualifiedName~UpdateChannelHandlerTests"`
|
||||
Expected: FAIL — handler does not download; `RemoteLogoCacher` not a ctor param.
|
||||
|
||||
- [ ] **Step 3: Implement**
|
||||
|
||||
`UpdateChannelHandler`: add `IRemoteLogoCacher remoteLogoCacher` to the primary constructor. In `Handle`, after validation passes and before `ApplyUpdateRequest`, if `request.Logo?.Path` is an external URL, call `remoteLogoCacher.CacheFromUrl`; on `Left` return it; on `Right` replace `request.Logo.Path` with the returned cache name (wrap the request or pass the resolved path into `ApplyUpdateRequest`). Then `ApplyUpdateRequest` stores the (now non-URL) path exactly as today — its existing `iptv/logos/` strip is a no-op for a bare cache name.
|
||||
|
||||
Concretely, change the `Handle` continuation:
|
||||
|
||||
```csharp
|
||||
return await maybeChannel.Match(
|
||||
Some: async channel =>
|
||||
{
|
||||
Validation<BaseError, Channel> validation = await Validate(dbContext, request, channel, cancellationToken);
|
||||
return await validation.Match(
|
||||
Succ: async c =>
|
||||
{
|
||||
Either<BaseError, string> resolvedLogo = await ResolveLogoPath(request, cancellationToken);
|
||||
return await resolvedLogo.Match(
|
||||
Right: async logoPath => Right<BaseError, ChannelViewModel>(
|
||||
await ApplyUpdateRequest(dbContext, c, request, logoPath, cancellationToken)),
|
||||
Left: e => Task.FromResult(Left<BaseError, ChannelViewModel>(e)));
|
||||
},
|
||||
Fail: errors => Task.FromResult(Left<BaseError, ChannelViewModel>(errors.Head)));
|
||||
},
|
||||
None: () => Task.FromResult(Left<BaseError, ChannelViewModel>(
|
||||
new NotFoundError($"Channel {request.ChannelId} does not exist."))));
|
||||
```
|
||||
|
||||
where `ResolveLogoPath` returns `Right(string.Empty)`/`Right(originalPath)` for empty/non-URL and `remoteLogoCacher.CacheFromUrl(...)` for a URL, and `ApplyUpdateRequest` takes the resolved `logoPath` instead of reading `update.Logo.Path`. (Keep `ContentType` handling as-is; a downloaded logo's content type can be left null — the serve route sniffs it, per #283.)
|
||||
|
||||
- [ ] **Step 4: Run tests to verify they pass**
|
||||
|
||||
Run: `dotnet test ErsatzTV.Tests/ErsatzTV.Tests.csproj --filter "FullyQualifiedName~UpdateChannelHandlerTests"` → PASS
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
bash -c 'dotnet format ErsatzTV.sln --no-restore --include <touched .cs>'
|
||||
git add -A && git commit -m "feat(525): download external-url logo on channel update"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: `CreateChannelHandler` + `CreateChannelFromLineupHandler` download on create
|
||||
|
||||
Same treatment for the two create paths, so a channel can never be created with a URL in `Artwork.Path`.
|
||||
|
||||
**Files:**
|
||||
- Modify: `ErsatzTV.Application/Channels/Commands/CreateChannelHandler.cs`
|
||||
- Modify: `ErsatzTV.Application/Channels/Commands/CreateChannelFromLineupHandler.cs`
|
||||
- Modify/Create: the corresponding `*HandlerTests` in `ErsatzTV.Tests/Application/Channels/`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `IRemoteLogoCacher.CacheFromUrl`.
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — mirror Task 4's download + fail cases for `CreateChannelHandler` (URL → cache name; cacher `Left` → save fails). Use that fixture's create helpers.
|
||||
|
||||
- [ ] **Step 2: Run to verify fail.**
|
||||
Run: `dotnet test ErsatzTV.Tests/ErsatzTV.Tests.csproj --filter "FullyQualifiedName~CreateChannelHandlerTests"` → FAIL
|
||||
|
||||
- [ ] **Step 3: Implement** — inject `IRemoteLogoCacher` into both handlers; resolve a URL logo → cache name before persisting `Artwork.Path`, propagating a `Left` as the handler result. `CreateChannelFromLineupHandler` (`:360-362`) builds logo artwork from the lineup — only channels whose lineup logo is a URL need the download; a lineup that already references a local/cached path is unchanged.
|
||||
|
||||
- [ ] **Step 4: Run to verify pass.** → PASS
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
```bash
|
||||
git add -A && git commit -m "feat(525): download external-url logo on channel create + create-from-lineup"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 6: Apply the decode budget to `UploadArtworkHandler`
|
||||
|
||||
Close the pre-existing gap: a direct upload is not budget-checked, and once URL logos become uploads that inconsistency is created by this feature. One rule: anything entering the logo cache is budget-checked.
|
||||
|
||||
**Files:**
|
||||
- Modify: `ErsatzTV.Application/Artworks/Commands/UploadArtworkHandler.cs` (validate the buffered bytes via `IRemoteImageValidator` before `SaveArtworkToCache`)
|
||||
- Modify: `ErsatzTV.Tests/Application/Artworks/UploadArtworkHandlerTests.cs` (create if absent)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `IRemoteImageValidator.DecodeAndValidate` (Task 2).
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
```csharp
|
||||
[Test]
|
||||
public async Task Should_Reject_An_Upload_That_Busts_The_Decode_Budget()
|
||||
{
|
||||
// craft a tiny PNG header declaring 30000x30000 (reuse PngHeaderDeclaring helper)
|
||||
await using MemoryStream bomb = PngHeaderDeclaring(30000, 30000);
|
||||
var handler = new UploadArtworkHandler(ImageCache, new RemoteImageValidator());
|
||||
Either<BaseError, ArtworkUploadResponseModel> result =
|
||||
await handler.Handle(new UploadArtwork(bomb, ArtworkKind.Logo), CancellationToken.None);
|
||||
result.IsLeft.ShouldBeTrue();
|
||||
result.IfLeft(e => e.Value.ShouldContain("pixel limit"));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Accept_A_Normal_Upload()
|
||||
{
|
||||
await using MemoryStream png = await RealPng(64, 64);
|
||||
var handler = new UploadArtworkHandler(ImageCache, new RemoteImageValidator());
|
||||
Either<BaseError, ArtworkUploadResponseModel> result =
|
||||
await handler.Handle(new UploadArtwork(png, ArtworkKind.Logo), CancellationToken.None);
|
||||
result.IsRight.ShouldBeTrue();
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run to verify fail.**
|
||||
Run: `dotnet test ErsatzTV.Tests/ErsatzTV.Tests.csproj --filter "FullyQualifiedName~UploadArtworkHandlerTests"` → FAIL (validator not a ctor param; bomb currently accepted)
|
||||
|
||||
- [ ] **Step 3: Implement** — add `IRemoteImageValidator validator` to `UploadArtworkHandler`'s constructor. After the content-type sniff and before `SaveArtworkToCache`, decode-validate the bytes:
|
||||
|
||||
```csharp
|
||||
using (var probe = new MemoryStream(bytes, writable: false))
|
||||
{
|
||||
try
|
||||
{
|
||||
using Image _ = await validator.DecodeAndValidate(
|
||||
probe, new Uri("upload://artwork"), cancellationToken);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
return BaseError.New($"Image cannot be used: {ex.Message}");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
(`upload://artwork` is a synthetic Uri for the message; the validator only uses it for text. If Task 2 made the validator return a record/bytes rather than `Image`, drop the `using`.)
|
||||
|
||||
- [ ] **Step 4: Run to verify pass.** → PASS. Also run the full `Artworks` + `Channels` test folders.
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
```bash
|
||||
git add -A && git commit -m "feat(525): budget-check direct artwork uploads (close the upload gap)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 7: `WatermarkSelector` stops treating a URL logo as renderable
|
||||
|
||||
After migration, a logo path is a URL only for a row that failed migration. Such a row must degrade to "no bug" with a warning, never fetch.
|
||||
|
||||
**Files:**
|
||||
- Modify: `ErsatzTV.Core/FFmpeg/WatermarkSelector.cs` (`ChannelLogoWatermarkOptions`, `:301-325`)
|
||||
- Modify: `ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorChannelLogoTests.cs`
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: for an external-URL logo path, `ChannelLogoWatermarkOptions` returns `None` and logs a warning (was: returned the URL as `ImagePath` for render-time fetch, added in #502).
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — extend `WatermarkSelectorChannelLogoTests`: a channel whose logo `Artwork.Path` is `https://example.com/logo.png` yields `None` (no watermark), and the existing cached-local-path case still renders. Assert the URL case does NOT produce a `WatermarkOptions` with the URL as `ImagePath`.
|
||||
|
||||
- [ ] **Step 2: Run to verify fail.**
|
||||
Run: `dotnet test ErsatzTV.Core.Tests/ErsatzTV.Core.Tests.csproj --filter "FullyQualifiedName~WatermarkSelectorChannelLogo"` → FAIL (URL still passed through)
|
||||
|
||||
- [ ] **Step 3: Implement** — in `ChannelLogoWatermarkOptions`, replace the `if (Artwork.IsExternalUrl(logoArtwork.Path)) return new WatermarkOptions(watermark, logoArtwork.Path, None);` branch with:
|
||||
|
||||
```csharp
|
||||
if (Artwork.IsExternalUrl(logoArtwork.Path))
|
||||
{
|
||||
// As of #525 an external-URL logo is downloaded and cached at save time, so a URL here
|
||||
// means a row that failed migration. Do not fetch at render time; degrade to no bug.
|
||||
logger.LogWarning(
|
||||
"Channel logo for channel {Channel} is still an un-downloaded URL {Url}; re-save the "
|
||||
+ "channel to download it. Rendering without an on-screen bug.",
|
||||
channel.Number,
|
||||
logoArtwork.Path);
|
||||
return None;
|
||||
}
|
||||
```
|
||||
|
||||
(Confirm `logger` and `channel` are in scope in that method; the recon shows `logger` is injected and `channel` is the parameter.)
|
||||
|
||||
- [ ] **Step 4: Run to verify pass.** → PASS. Also run `ChannelPlaylistGoldenTests` + `ChannelGuideGoldenTests` (M3U/XMLTV still emit the raw URL for a not-yet-migrated row — those consumers are unchanged; goldens should be green).
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
```bash
|
||||
git add -A && git commit -m "feat(525): render path no longer fetches a URL logo; degrades to no bug"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 8: One-time startup migration of existing URL logo rows
|
||||
|
||||
Convert `Artwork` rows whose `Path` is an `http(s)` URL and kind `Logo` into cached rows. Failures leave the row + warn. Idempotent.
|
||||
|
||||
**Files:**
|
||||
- Create: `ErsatzTV/Services/RunOnce/ExternalLogoMigratorService.cs`
|
||||
- Modify: `ErsatzTV/Startup.cs` (register in the run-once block)
|
||||
- Create: `ErsatzTV.Tests/Services/ExternalLogoMigratorTests.cs` (test the migration method against `InMemoryTvContext`)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `IRemoteLogoCacher.CacheFromUrl`, `TvContext`, `SystemStartup.WaitForDatabase`.
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — extract the migration body into an internal static/instance method `MigrateAsync(TvContext db, IRemoteLogoCacher cacher, ILogger, CancellationToken)` so it is testable without hosting. Tests:
|
||||
- a row with a URL path is converted to the cache name (cacher returns `Right`), `IsExternalUrl()` false afterward;
|
||||
- a row whose cacher returns `Left` is left unchanged (still the URL) and a warning is logged (assert via a substituted `ILogger` `Received` or just that the path is unchanged);
|
||||
- a second run over already-migrated rows calls the cacher zero times (idempotent — only URL rows are selected).
|
||||
|
||||
- [ ] **Step 2: Run to verify fail.**
|
||||
Run: `dotnet test ErsatzTV.Tests/ErsatzTV.Tests.csproj --filter "FullyQualifiedName~ExternalLogoMigrator"` → FAIL (type absent)
|
||||
|
||||
- [ ] **Step 3: Implement** — mirror `DatabaseCleanerService` (primary-ctor `IServiceScopeFactory` + `ILogger<>` + `SystemStartup`; `Task.Yield()`; `await systemStartup.WaitForDatabase`; scope → `TvContext`; resolve `IRemoteLogoCacher` from the scope). Selection: EF-side filter is awkward (`IsExternalUrl` is C#), so load logo artwork and filter in memory: `db.Artwork.Where(a => a.ArtworkKind == ArtworkKind.Logo)` → `.Where(a => a.IsExternalUrl())`. For each: `CacheFromUrl(new Uri(a.Path))` → on `Right` set `a.Path = name; a.DateUpdated = DateTime.UtcNow;` on `Left` log a warning naming the row/channel; `SaveChangesAsync` once at the end. Register after `DatabaseMigratorService` / `DatabaseCleanerService` so the schema exists.
|
||||
|
||||
- [ ] **Step 4: Run to verify pass.** → PASS
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
```bash
|
||||
git add -A && git commit -m "feat(525): startup migration converts existing URL logo rows to cache"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 9: SPA — preview a saved logo, drop the stale copy, inline error on rejected save
|
||||
|
||||
**Files:**
|
||||
- Modify: `web/src/screens/ChannelEditScreen.tsx` (remove `&& !externalUrlLogo` preview suppression; delete the "cannot drive the bug" help text; surface the save 400 inline on the URL field; simplify the mutual-exclusion now that a URL never survives a save)
|
||||
- Modify: `web/src/screens/ChannelEditScreen.test.tsx`
|
||||
- Modify: `docs/spa-conventions.md` only if a documented screen convention changes (likely not)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: the channel `PUT` now returns a normal cached logo on success and a `400` with a specific message on a bad URL.
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — in `ChannelEditScreen.test.tsx`:
|
||||
- after a successful save of a channel whose logo was an external URL, the logo preview renders (the `&& !externalUrlLogo` suppression is gone);
|
||||
- a save that returns a `400` "Could not download logo…" shows that message inline near the URL field and does not navigate away;
|
||||
- the removed help text ("cannot be used as the on-screen bug") is absent.
|
||||
|
||||
- [ ] **Step 2: Run to verify fail.**
|
||||
Run: `cd web && npx vitest run src/screens/ChannelEditScreen.test.tsx` → FAIL
|
||||
|
||||
- [ ] **Step 3: Implement** — delete the `externalUrlLogo` branch in the "Use logo as on-screen bug" help (`:797-803`), remove the `&& !externalUrlLogo` guard on the preview (`:817`), and render the save error (from the existing `ApiError` handling) beside the External-logo-URL `Input`. Keep the URL field as an input that, on a successful save, is cleared and the cached logo shown (hydration already treats an external URL specially at `:136`/`:164` — since a saved logo is no longer external, that path naturally stops triggering).
|
||||
|
||||
- [ ] **Step 4: Run to verify pass.**
|
||||
Run: `cd web && npm run typecheck && npx vitest run src/screens/ChannelEditScreen.test.tsx` → PASS
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
```bash
|
||||
git add -A && git commit -m "feat(525): SPA previews saved logos, drops stale external-URL copy"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 10: Docs + final gate
|
||||
|
||||
**Files:**
|
||||
- Modify: `docs/decisions.md` (new entry — see below)
|
||||
- Modify: `docs/channels.md` (replace the "External logo URLs drive the bug… fetched at render time" text with the download-on-save behavior + the save-time failure)
|
||||
- Modify: `docs/api-conventions.md` (note `PUT /api/v1/channels/{id}` and `POST /api/v1/artwork/uploads` can now `400` on a bad/oversized/over-budget logo)
|
||||
- Modify: `docs/README.md` only if a doc is added/retitled (no)
|
||||
|
||||
- [ ] **Step 1: `docs/decisions.md` entry** (append at EOF + index line). Must state: external logo URLs are downloaded and cached at save time (content-hash name, identical to an upload); this **supersedes the #511 "not cached, re-fetched per element init" paragraph** and **narrows #502's "external artwork passes through"** to the client-facing consumers (M3U/XMLTV/SPA still emit whatever `Artwork.Path` resolves to — now a cache URL, not the external URL); the decode budget is shared (`RemoteImageDecodeBudget`) and now also guards direct uploads; the render path no longer fetches a logo (a leftover URL row degrades to no bug + warning); migration is a startup task, failures left intact; no refresh button by design (re-add the URL).
|
||||
|
||||
- [ ] **Step 2: `docs/channels.md`** — rewrite the external-logo paragraph to the new behavior.
|
||||
|
||||
- [ ] **Step 3: OpenAPI** — response shapes are unchanged (still `ChannelViewModel` / `ArtworkUploadResponseModel`), only error status/messages differ, so `v1.json` likely does not change. Run `./scripts/update-openapi.sh` and `git diff --exit-code docs/v1.json`; commit only if it actually changed.
|
||||
|
||||
- [ ] **Step 4: Full local gate** (BEFORE any push):
|
||||
```bash
|
||||
dotnet build ErsatzTV.sln # Build succeeded, 0 warnings
|
||||
dotnet test ErsatzTV.sln # all green
|
||||
cd web && npm run typecheck && npm run test && cd ..
|
||||
# BOM + format on the touched set:
|
||||
for f in $(git diff --name-only origin/main...HEAD -- '*.cs'); do head -c3 "$f" | xxd -p | grep -q '^efbbbf' && echo "BOM: $f"; done
|
||||
bash -c 'mapfile -t files < <(git diff --name-only --diff-filter=ACM origin/main...HEAD -- "*.cs"); dotnet format whitespace . --folder --verify-no-changes --include "${files[@]}"'
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Cold adversarial review** over the whole diff (mandatory here — this touches API write-path handlers and a data migration; see the review skip rubric). Fold fixes in, then push and open the PR (arm the CI monitor at open). Live-E2E the write path (`scripts/e2e-local.sh`): create a channel with an external-URL logo, confirm it downloads + previews + the M3U emits an `/iptv/logos/` URL; a deliberately-bad URL is rejected in the editor.
|
||||
|
||||
- [ ] **Step 6: Commit + PR**
|
||||
```bash
|
||||
git add -A && git commit -m "docs(525): record download-on-save; supersede #511 not-cached note"
|
||||
git push -u origin feat/525-external-logo-download-on-save
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Self-Review
|
||||
|
||||
**Spec coverage:** save path (Tasks 4/5) ✓; content-hash naming (reuses `SaveArtworkToCache`) ✓; downstream no-change (verified — nothing in M3U/XMLTV/mapper touched) ✓; render path stops fetching (Task 7) ✓; decode validation shared + uploads folded in (Tasks 1/2/3/6) ✓; migration startup task, failures intact (Task 8) ✓; synchronous save + 400 (Task 4 + Task 9) ✓; preview works, stale copy gone (Task 9) ✓; docs incl. superseding #511 + narrowing #502 (Task 10) ✓; `IRemoteImageFetcher` namespace unchanged (respected — not touched) ✓.
|
||||
|
||||
**Placeholder scan:** the two `<verbatim body …>` markers in Task 2 point at an exact existing method (`ImageElementBase.DecodeRemoteImage`, quoted in the spec's source recon) to move unchanged — not new logic to invent. One explicit VERIFY (does `ErsatzTV.Core` reference ImageSharp) with a stated fallback, because the answer changes the interface signature and must be checked in-repo rather than guessed.
|
||||
|
||||
**Type consistency:** `IRemoteLogoCacher.CacheFromUrl → Task<Either<BaseError,string>>` (Task 3) is what Tasks 4/5/8 consume; `IRemoteImageValidator.DecodeAndValidate → Task<Image>` (Task 2) is what Tasks 3/6 consume and what `ImageElementBase` delegates to; `RemoteImageDecodeBudget` static members (Task 1) are consumed by Tasks 2 and (transitively) 6. Names match across tasks.
|
||||
Reference in New Issue
Block a user