feat(525): external channel-logo URLs download + cache at save time (not fetched at render) #528
@@ -9,8 +9,13 @@ namespace ErsatzTV.Application.Artworks;
|
||||
public class UploadArtworkHandler : IRequestHandler<UploadArtwork, Either<BaseError, ArtworkUploadResponseModel>>
|
||||
{
|
||||
private readonly IImageCache _imageCache;
|
||||
private readonly IRemoteImageValidator _validator;
|
||||
|
||||
public UploadArtworkHandler(IImageCache imageCache) => _imageCache = imageCache;
|
||||
public UploadArtworkHandler(IImageCache imageCache, IRemoteImageValidator validator)
|
||||
{
|
||||
_imageCache = imageCache;
|
||||
_validator = validator;
|
||||
}
|
||||
|
||||
public async Task<Either<BaseError, ArtworkUploadResponseModel>> Handle(
|
||||
UploadArtwork request,
|
||||
@@ -38,6 +43,22 @@ public class UploadArtworkHandler : IRequestHandler<UploadArtwork, Either<BaseEr
|
||||
|
||||
string contentType = maybeContentType.IfNone(string.Empty);
|
||||
|
||||
// One rule: anything entering the logo cache is decode-budget-checked. A supported format is
|
||||
// not enough — a small header can declare a multi-gigabyte canvas (a decompression bomb), so
|
||||
// reject it here before it lands in the cache. The synthetic upload:// Uri is only for the
|
||||
// exception message text. (ersatztv#525)
|
||||
using (var probe = new MemoryStream(bytes, writable: false))
|
||||
{
|
||||
try
|
||||
{
|
||||
await _validator.Validate(probe, new Uri("upload://artwork"), cancellationToken);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
return BaseError.New($"Image cannot be used: {ex.Message}");
|
||||
}
|
||||
}
|
||||
|
||||
using var toCache = new MemoryStream(bytes, writable: false);
|
||||
Either<BaseError, string> maybeFileName = await _imageCache.SaveArtworkToCache(
|
||||
toCache,
|
||||
|
||||
@@ -8,6 +8,7 @@ using ErsatzTV.Core.Api.LibraryBrowse;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Filler;
|
||||
using ErsatzTV.Core.Errors;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Search;
|
||||
using ErsatzTV.Core.Scheduling;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
@@ -21,6 +22,7 @@ public class CreateChannelFromLineupHandler(
|
||||
ChannelWriter<IBackgroundServiceRequest> workerChannel,
|
||||
IDbContextFactory<TvContext> dbContextFactory,
|
||||
ISearchTargets searchTargets,
|
||||
IRemoteLogoCacher remoteLogoCacher,
|
||||
ILogger<CreateChannelFromLineupHandler> logger)
|
||||
: IRequestHandler<CreateChannelFromLineup, Either<BaseError, CreateChannelFromLineupResponseModel>>
|
||||
{
|
||||
@@ -37,7 +39,42 @@ public class CreateChannelFromLineupHandler(
|
||||
Either<BaseError, PreparedCreate> validation = await Validate(dbContext, request, cancellationToken);
|
||||
return await validation.Match(
|
||||
Left: error => Task.FromResult<Either<BaseError, CreateChannelFromLineupResponseModel>>(error),
|
||||
Right: prepared => PersistAndDispatch(dbContext, prepared, cancellationToken));
|
||||
Right: async prepared =>
|
||||
{
|
||||
Either<BaseError, PreparedCreate> resolved =
|
||||
await ResolveExternalLogo(request, prepared, cancellationToken);
|
||||
return await resolved.Match(
|
||||
Left: error => Task.FromResult<Either<BaseError, CreateChannelFromLineupResponseModel>>(error),
|
||||
Right: p => PersistAndDispatch(dbContext, p, cancellationToken));
|
||||
});
|
||||
}
|
||||
|
||||
// The lineup logo artwork is built (in BuildChannel) with the raw request path. When that path is
|
||||
// an external http(s) URL, download + cache it and swap the cache name onto the logo artwork before
|
||||
// persisting (a cacher Left fails the whole create); a blank or already-local/cached path is left
|
||||
// unchanged. (ersatztv#525)
|
||||
private async Task<Either<BaseError, PreparedCreate>> ResolveExternalLogo(
|
||||
CreateChannelFromLineup request,
|
||||
PreparedCreate prepared,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
string path = request.Logo?.Path ?? string.Empty;
|
||||
|
||||
if (!Artwork.IsExternalUrl(path))
|
||||
{
|
||||
return prepared;
|
||||
}
|
||||
|
||||
Either<BaseError, string> cached = await remoteLogoCacher.CacheFromUrl(new Uri(path), cancellationToken);
|
||||
return cached.Map(name =>
|
||||
{
|
||||
foreach (Artwork logo in prepared.Channel.Artwork.Where(a => a.ArtworkKind == ArtworkKind.Logo))
|
||||
{
|
||||
logo.Path = name;
|
||||
}
|
||||
|
||||
return prepared;
|
||||
});
|
||||
}
|
||||
|
||||
private async Task<Either<BaseError, CreateChannelFromLineupResponseModel>> PersistAndDispatch(
|
||||
|
||||
@@ -1,9 +1,10 @@
|
||||
using System.Globalization;
|
||||
using System.Globalization;
|
||||
using System.Text.RegularExpressions;
|
||||
using System.Threading.Channels;
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Filler;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Search;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Infrastructure.Extensions;
|
||||
@@ -16,7 +17,8 @@ namespace ErsatzTV.Application.Channels;
|
||||
public class CreateChannelHandler(
|
||||
ChannelWriter<IBackgroundServiceRequest> workerChannel,
|
||||
IDbContextFactory<TvContext> dbContextFactory,
|
||||
ISearchTargets searchTargets)
|
||||
ISearchTargets searchTargets,
|
||||
IRemoteLogoCacher remoteLogoCacher)
|
||||
: IRequestHandler<CreateChannel, Either<BaseError, CreateChannelResult>>
|
||||
{
|
||||
public async Task<Either<BaseError, CreateChannelResult>> Handle(
|
||||
@@ -25,7 +27,52 @@ public class CreateChannelHandler(
|
||||
{
|
||||
await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
|
||||
Validation<BaseError, Channel> validation = await Validate(dbContext, request, cancellationToken);
|
||||
return await validation.Apply(c => PersistChannel(dbContext, c));
|
||||
return await validation.Match(
|
||||
Succ: async channel =>
|
||||
{
|
||||
Either<BaseError, string> resolvedLogo = await ResolveLogoPath(request, cancellationToken);
|
||||
return await resolvedLogo.Match(
|
||||
Right: async logoPath =>
|
||||
{
|
||||
ApplyResolvedLogo(request, channel, logoPath);
|
||||
return Right<BaseError, CreateChannelResult>(await PersistChannel(dbContext, channel));
|
||||
},
|
||||
Left: e => Task.FromResult(Left<BaseError, CreateChannelResult>(e)));
|
||||
},
|
||||
Fail: errors => Task.FromResult(Left<BaseError, CreateChannelResult>(errors.Join())));
|
||||
}
|
||||
|
||||
// Resolve the incoming logo path into a value safe to persist. An external http(s) URL is
|
||||
// downloaded and cached (a cacher Left fails the whole save); an empty path or an
|
||||
// already-local/cached path passes through unchanged. (ersatztv#525)
|
||||
private async Task<Either<BaseError, string>> ResolveLogoPath(
|
||||
CreateChannel request,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
string path = request.Logo?.Path ?? string.Empty;
|
||||
|
||||
if (!Artwork.IsExternalUrl(path))
|
||||
{
|
||||
return path;
|
||||
}
|
||||
|
||||
Either<BaseError, string> cached = await remoteLogoCacher.CacheFromUrl(new Uri(path), cancellationToken);
|
||||
return cached;
|
||||
}
|
||||
|
||||
// When the incoming logo was an external URL, swap the downloaded cache name onto the logo
|
||||
// artwork built during validation so no URL is ever persisted in Artwork.Path. (ersatztv#525)
|
||||
private static void ApplyResolvedLogo(CreateChannel request, Channel channel, string resolvedLogoPath)
|
||||
{
|
||||
if (!Artwork.IsExternalUrl(request.Logo?.Path ?? string.Empty))
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
foreach (Artwork logo in channel.Artwork.Where(a => a.ArtworkKind == ArtworkKind.Logo))
|
||||
{
|
||||
logo.Path = resolvedLogoPath;
|
||||
}
|
||||
}
|
||||
|
||||
private async Task<CreateChannelResult> PersistChannel(TvContext dbContext, Channel channel)
|
||||
|
||||
@@ -6,6 +6,7 @@ using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Filler;
|
||||
using ErsatzTV.Core.Errors;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Search;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Infrastructure.Extensions;
|
||||
@@ -19,7 +20,8 @@ namespace ErsatzTV.Application.Channels;
|
||||
public class UpdateChannelHandler(
|
||||
ChannelWriter<IBackgroundServiceRequest> workerChannel,
|
||||
IDbContextFactory<TvContext> dbContextFactory,
|
||||
ISearchTargets searchTargets)
|
||||
ISearchTargets searchTargets,
|
||||
IRemoteLogoCacher remoteLogoCacher)
|
||||
: IRequestHandler<UpdateChannel, Either<BaseError, ChannelViewModel>>
|
||||
{
|
||||
public async Task<Either<BaseError, ChannelViewModel>> Handle(
|
||||
@@ -39,17 +41,45 @@ public class UpdateChannelHandler(
|
||||
{
|
||||
Validation<BaseError, Channel> validation =
|
||||
await Validate(dbContext, request, channel, cancellationToken);
|
||||
return await validation.Apply(c => ApplyUpdateRequest(dbContext, c, request, 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.Join())));
|
||||
},
|
||||
None: () => Task.FromResult(
|
||||
Left<BaseError, ChannelViewModel>(
|
||||
new NotFoundError($"Channel {request.ChannelId} does not exist."))));
|
||||
}
|
||||
|
||||
// Resolve the incoming logo path into a value safe to persist. An external http(s) URL is
|
||||
// downloaded and cached (a cacher Left fails the whole save); an empty path (logo removal) or an
|
||||
// already-local/cached path passes through unchanged. (ersatztv#525)
|
||||
private async Task<Either<BaseError, string>> ResolveLogoPath(
|
||||
UpdateChannel request,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
string path = request.Logo?.Path ?? string.Empty;
|
||||
|
||||
if (!Artwork.IsExternalUrl(path))
|
||||
{
|
||||
return path;
|
||||
}
|
||||
|
||||
Either<BaseError, string> cached = await remoteLogoCacher.CacheFromUrl(new Uri(path), cancellationToken);
|
||||
return cached;
|
||||
}
|
||||
|
||||
private async Task<ChannelViewModel> ApplyUpdateRequest(
|
||||
TvContext dbContext,
|
||||
Channel c,
|
||||
UpdateChannel update,
|
||||
string resolvedLogoPath,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
bool hasEpgChange = c.PlayoutSource != update.PlayoutSource || c.ShowInEpg != update.ShowInEpg;
|
||||
@@ -76,9 +106,9 @@ public class UpdateChannelHandler(
|
||||
c.ShowInEpg = update.IsEnabled && update.ShowInEpg;
|
||||
c.Artwork ??= [];
|
||||
|
||||
if (!string.IsNullOrWhiteSpace(update.Logo?.Path))
|
||||
if (!string.IsNullOrWhiteSpace(resolvedLogoPath))
|
||||
{
|
||||
string logo = update.Logo.Path;
|
||||
string logo = resolvedLogoPath;
|
||||
if (logo.StartsWith("iptv/logos/", StringComparison.Ordinal))
|
||||
{
|
||||
logo = logo.Replace("iptv/logos/", string.Empty);
|
||||
|
||||
@@ -72,10 +72,15 @@ public class WatermarkSelectorChannelLogoTests
|
||||
return channel;
|
||||
}
|
||||
|
||||
// ---- external URL logo: the #502 defect ----------------------------------------------------
|
||||
// ---- external URL logo: render path must degrade to no bug, never fetch (#525) --------------
|
||||
//
|
||||
// As of #525 an external-URL logo is downloaded and cached at save time, so a URL path can only be a
|
||||
// row that failed migration. The render/watermark path must NOT fetch at compositing time: it degrades
|
||||
// to None (no on-screen bug) with a warning, rather than handing the URL downstream as a renderable
|
||||
// ImagePath (the #502 behavior these tests previously pinned).
|
||||
|
||||
[Test]
|
||||
public void PlayoutItemWatermark_Should_Use_External_Url_Channel_Logo()
|
||||
public void PlayoutItemWatermark_Should_Ignore_External_Url_Channel_Logo()
|
||||
{
|
||||
ChannelWatermark watermark = ChannelLogoWatermark(1, "PlayoutItem");
|
||||
Channel channel = ChannelWithLogo(ExternalLogoUrl);
|
||||
@@ -85,14 +90,11 @@ public class WatermarkSelectorChannelLogoTests
|
||||
watermark,
|
||||
Option<ChannelWatermark>.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
WatermarkOptions options = result.IfNone(() => throw new InvalidOperationException());
|
||||
options.ImagePath.ShouldBe(ExternalLogoUrl);
|
||||
options.Watermark.ShouldBe(watermark);
|
||||
result.IsNone.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void ChannelWatermark_Should_Use_External_Url_Channel_Logo()
|
||||
public void ChannelWatermark_Should_Ignore_External_Url_Channel_Logo()
|
||||
{
|
||||
ChannelWatermark watermark = ChannelLogoWatermark(2, "Channel");
|
||||
Channel channel = ChannelWithLogo(ExternalLogoUrl, watermark);
|
||||
@@ -102,14 +104,13 @@ public class WatermarkSelectorChannelLogoTests
|
||||
Option<ChannelWatermark>.None,
|
||||
Option<ChannelWatermark>.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
WatermarkOptions options = result.IfNone(() => throw new InvalidOperationException());
|
||||
options.ImagePath.ShouldBe(ExternalLogoUrl);
|
||||
options.Watermark.ShouldBe(watermark);
|
||||
result.IsNone.ShouldBeTrue();
|
||||
// never hand the URL downstream as a renderable path
|
||||
result.IfSome(o => o.ImagePath.ShouldNotBe(ExternalLogoUrl));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void GlobalWatermark_Should_Use_External_Url_Channel_Logo()
|
||||
public void GlobalWatermark_Should_Ignore_External_Url_Channel_Logo()
|
||||
{
|
||||
ChannelWatermark watermark = ChannelLogoWatermark(3, "Global");
|
||||
Channel channel = ChannelWithLogo(ExternalLogoUrl);
|
||||
@@ -119,10 +120,7 @@ public class WatermarkSelectorChannelLogoTests
|
||||
Option<ChannelWatermark>.None,
|
||||
watermark);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
WatermarkOptions options = result.IfNone(() => throw new InvalidOperationException());
|
||||
options.ImagePath.ShouldBe(ExternalLogoUrl);
|
||||
options.Watermark.ShouldBe(watermark);
|
||||
result.IsNone.ShouldBeTrue();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -131,7 +129,7 @@ public class WatermarkSelectorChannelLogoTests
|
||||
/// existence-gated branch and re-introduce the defect for an oddly-cased URL.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void ChannelWatermark_Should_Use_External_Url_Channel_Logo_Regardless_Of_Scheme_Case()
|
||||
public void ChannelWatermark_Should_Ignore_External_Url_Channel_Logo_Regardless_Of_Scheme_Case()
|
||||
{
|
||||
const string UpperCaseUrl = "HTTPS://cdn.example.com/logos/channel.png";
|
||||
ChannelWatermark watermark = ChannelLogoWatermark(2, "Channel");
|
||||
@@ -142,8 +140,7 @@ public class WatermarkSelectorChannelLogoTests
|
||||
Option<ChannelWatermark>.None,
|
||||
Option<ChannelWatermark>.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfNone(() => throw new InvalidOperationException()).ImagePath.ShouldBe(UpperCaseUrl);
|
||||
result.IsNone.ShouldBeTrue();
|
||||
}
|
||||
|
||||
// ---- regressions: local-file behavior must not change ---------------------------------------
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
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);
|
||||
}
|
||||
}
|
||||
@@ -292,11 +292,11 @@ public class WatermarkSelector(
|
||||
/// shared by the playout-item, channel and global precedence levels so all three agree.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// An external-URL logo has no local file, so it must not be existence-checked: the branding tab
|
||||
/// advertises the URL as winning, but <c>File.Exists("https://…")</c> is always false, which silently
|
||||
/// dropped the bug for every such channel (#502). External URLs are handed downstream as-is, matching
|
||||
/// how every other consumer treats external artwork (M3U, XMLTV, SPA JSON all emit the raw URL);
|
||||
/// <c>ImageElementBase.LoadImage</c> fetches it over http when compositing.
|
||||
/// As of #525 an external-URL logo is downloaded and cached at save time, so a URL path here can only
|
||||
/// be a row that failed migration. The render path must never fetch at compositing time, so such a row
|
||||
/// degrades to no watermark (no on-screen bug) with a warning rather than being handed downstream as a
|
||||
/// renderable URL (the #502 behavior). Other consumers (M3U, XMLTV, SPA JSON) still emit the raw URL for
|
||||
/// a not-yet-migrated row; only this render/watermark path changed.
|
||||
/// </remarks>
|
||||
private Option<WatermarkOptions> ChannelLogoWatermarkOptions(Channel channel, ChannelWatermark watermark)
|
||||
{
|
||||
@@ -304,7 +304,14 @@ public class WatermarkSelector(
|
||||
{
|
||||
if (Artwork.IsExternalUrl(logoArtwork.Path))
|
||||
{
|
||||
return new WatermarkOptions(watermark, logoArtwork.Path, None);
|
||||
// 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;
|
||||
}
|
||||
|
||||
string cachedPath = imageCache.GetPathForImage(logoArtwork.Path, ArtworkKind.Logo, Option<int>.None);
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
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");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,13 @@
|
||||
namespace ErsatzTV.Core.Interfaces.Images;
|
||||
|
||||
/// <summary>
|
||||
/// Validates that a stream is a decodable image within the decode budget, throwing if not.
|
||||
/// Used by the logo save path and the artwork upload path (neither needs the decoded pixels,
|
||||
/// only "is this safe to cache"). The graphics engine uses the static
|
||||
/// RemoteImageValidator.DecodeAndValidate instead, which returns the Image it composites.
|
||||
/// (ersatztv#525)
|
||||
/// </summary>
|
||||
public interface IRemoteImageValidator
|
||||
{
|
||||
Task Validate(Stream stream, Uri uri, CancellationToken cancellationToken);
|
||||
}
|
||||
@@ -0,0 +1,14 @@
|
||||
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);
|
||||
}
|
||||
@@ -0,0 +1,151 @@
|
||||
using System.Buffers.Binary;
|
||||
using ErsatzTV.Core.Images;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Infrastructure.Images;
|
||||
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 RemoteImageValidatorTests
|
||||
{
|
||||
private static readonly Uri Uri = new("https://example.com/logo.png");
|
||||
|
||||
// decode cases exercise the static method (used by the render path)
|
||||
[Test]
|
||||
public async Task Should_Decode_A_Normal_Image()
|
||||
{
|
||||
await using MemoryStream stream = await RealPng(64, 32);
|
||||
using Image image = await RemoteImageValidator.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>(
|
||||
() => RemoteImageValidator.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, RemoteImageDecodeBudget.MaxRemoteFrames + 100);
|
||||
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
|
||||
() => RemoteImageValidator.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 RemoteImageValidator.DecodeAndValidate(stream, Uri, CancellationToken.None);
|
||||
image.Frames.Count.ShouldBe(60);
|
||||
}
|
||||
|
||||
// the Core interface Validate() is the save/upload contract: throws on invalid, returns on valid,
|
||||
// never surfaces an ImageSharp type
|
||||
[Test]
|
||||
public async Task Validate_Returns_On_A_Good_Image()
|
||||
{
|
||||
IRemoteImageValidator validator = new RemoteImageValidator();
|
||||
await using MemoryStream stream = await RealPng(64, 32);
|
||||
await Should.NotThrowAsync(() => validator.Validate(stream, Uri, CancellationToken.None));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Validate_Throws_On_A_Bomb()
|
||||
{
|
||||
IRemoteImageValidator validator = new RemoteImageValidator();
|
||||
await using MemoryStream stream = PngHeaderDeclaring(30000, 30000);
|
||||
await Should.ThrowAsync<InvalidOperationException>(
|
||||
() => validator.Validate(stream, Uri, CancellationToken.None));
|
||||
}
|
||||
|
||||
/// <summary>A real multi-frame APNG. Small on the wire, many frames — the shape that matters.</summary>
|
||||
private static MemoryStream Apng(int width, int height, int frames)
|
||||
{
|
||||
using var image = new Image<Rgba32>(width, height);
|
||||
for (var i = 1; i < frames; i++)
|
||||
{
|
||||
image.Frames.CreateFrame();
|
||||
}
|
||||
|
||||
var stream = new MemoryStream();
|
||||
image.Save(stream, new PngEncoder { ColorType = PngColorType.RgbWithAlpha });
|
||||
stream.Position = 0;
|
||||
return stream;
|
||||
}
|
||||
|
||||
// PNG chunk CRC-32 (IEEE, reflected). hand-rolled because the repo does not reference
|
||||
// System.IO.Hashing, and ImageSharp validates the CRC of critical chunks like IHDR.
|
||||
private static uint Crc32(ReadOnlySpan<byte> data)
|
||||
{
|
||||
uint crc = 0xFFFFFFFF;
|
||||
foreach (byte b in data)
|
||||
{
|
||||
crc ^= b;
|
||||
for (var i = 0; i < 8; i++)
|
||||
{
|
||||
crc = (crc & 1) != 0 ? (crc >> 1) ^ 0xEDB88320 : crc >> 1;
|
||||
}
|
||||
}
|
||||
|
||||
return crc ^ 0xFFFFFFFF;
|
||||
}
|
||||
|
||||
/// <summary>A real, decodable PNG.</summary>
|
||||
private static async Task<MemoryStream> RealPng(int width, int height)
|
||||
{
|
||||
using var image = new Image<Rgba32>(width, height);
|
||||
var stream = new MemoryStream();
|
||||
await image.SaveAsync(stream, new PngEncoder());
|
||||
stream.Position = 0;
|
||||
return stream;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A PNG signature plus a single valid IHDR chunk declaring <paramref name="width" /> x
|
||||
/// <paramref name="height" /> and nothing else — enough for Identify, far too little to
|
||||
/// decode. This is what a decompression bomb looks like at the point we have to reject it.
|
||||
/// </summary>
|
||||
private static MemoryStream PngHeaderDeclaring(int width, int height)
|
||||
{
|
||||
var stream = new MemoryStream();
|
||||
stream.Write([0x89, (byte)'P', (byte)'N', (byte)'G', 0x0D, 0x0A, 0x1A, 0x0A]);
|
||||
|
||||
var ihdr = new byte[17];
|
||||
"IHDR"u8.CopyTo(ihdr);
|
||||
BinaryPrimitives.WriteInt32BigEndian(ihdr.AsSpan(4), width);
|
||||
BinaryPrimitives.WriteInt32BigEndian(ihdr.AsSpan(8), height);
|
||||
ihdr[12] = 8; // bit depth
|
||||
ihdr[13] = 6; // color type: truecolor + alpha
|
||||
ihdr[14] = 0; // compression
|
||||
ihdr[15] = 0; // filter
|
||||
ihdr[16] = 0; // interlace
|
||||
|
||||
var length = new byte[4];
|
||||
BinaryPrimitives.WriteInt32BigEndian(length, 13);
|
||||
stream.Write(length);
|
||||
stream.Write(ihdr);
|
||||
|
||||
var crc = new byte[4];
|
||||
BinaryPrimitives.WriteUInt32BigEndian(crc, Crc32(ihdr));
|
||||
stream.Write(crc);
|
||||
|
||||
stream.Position = 0;
|
||||
return stream;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,79 @@
|
||||
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 static LanguageExt.Prelude;
|
||||
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.Validate(png, Uri, Arg.Any<CancellationToken>()).Returns(Task.CompletedTask);
|
||||
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.Validate(png, Uri, Arg.Any<CancellationToken>())
|
||||
.Returns<Task>(_ => 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;
|
||||
}
|
||||
}
|
||||
@@ -1,4 +1,5 @@
|
||||
using System.Buffers.Binary;
|
||||
using ErsatzTV.Core.Images;
|
||||
using ErsatzTV.Infrastructure.Streaming.Graphics;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
@@ -51,7 +52,7 @@ public class RemoteImageDecodeLimitTests
|
||||
// 10000 x 5000 = 50,000,000 -- exactly the budget, so it must NOT be rejected. the decode
|
||||
// then fails on the truncated body, which proves the check let it through. NOTE this test
|
||||
// would also pass with the guard deleted entirely; deletion is covered by the bomb test
|
||||
// above, and the boundary arithmetic by EnsureDecodeAffordable's own tests.
|
||||
// above, and the boundary arithmetic by RemoteImageDecodeBudgetTests (ErsatzTV.Core.Tests).
|
||||
await using MemoryStream stream = PngHeaderDeclaring(10000, 5000);
|
||||
|
||||
Exception ex = await Should.ThrowAsync<Exception>(
|
||||
@@ -60,77 +61,12 @@ public class RemoteImageDecodeLimitTests
|
||||
ex.Message.ShouldNotContain("pixel limit");
|
||||
}
|
||||
|
||||
// --- the budget policy itself, tested as arithmetic so no multi-GB image is ever allocated ---
|
||||
|
||||
// THE bomb the first fix missed: 2500x2500 x600 frames is ~60 KiB on the wire, passes a
|
||||
// dimensions-only check (6.25 MP) AND a frames-only check (exactly 600), and costs ~14 GiB to
|
||||
// decode. Only the PRODUCT catches it. (Found by adversarial re-review; ersatztv#511.)
|
||||
[Test]
|
||||
public void Should_Reject_Dimensions_And_Frames_That_Are_Affordable_Alone_But_Not_Together()
|
||||
{
|
||||
const int Width = 2500;
|
||||
const int Height = 2500;
|
||||
const int Frames = 600;
|
||||
|
||||
// each guard, in isolation, says yes
|
||||
((long)Width * Height).ShouldBeLessThanOrEqualTo(ImageElementBase.MaxRemoteDecodedPixels);
|
||||
Frames.ShouldBeLessThanOrEqualTo(ImageElementBase.MaxRemoteFrames);
|
||||
|
||||
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
|
||||
() => ImageElementBase.EnsureDecodeAffordable(Width, Height, Frames, ImageUri));
|
||||
|
||||
ex.Message.ShouldContain("pixel limit");
|
||||
}
|
||||
|
||||
// M1: the frame guard had no coverage at all in the first fix
|
||||
[Test]
|
||||
public void Should_Reject_Too_Many_Frames_Even_When_Each_Is_Tiny()
|
||||
{
|
||||
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
|
||||
() => ImageElementBase.EnsureDecodeAffordable(8, 8, ImageElementBase.MaxRemoteFrames + 1, ImageUri));
|
||||
|
||||
ex.Message.ShouldContain("frame limit");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Should_Allow_A_Single_Large_Still_Within_Budget()
|
||||
{
|
||||
// 8K is ~33 MP -- must keep working
|
||||
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(7680, 4320, 1, ImageUri));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Should_Allow_A_Typical_Animated_Logo()
|
||||
{
|
||||
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(288, 288, 600, ImageUri));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Should_Afford_Fewer_Frames_As_Frames_Get_Larger()
|
||||
{
|
||||
// tiny frames are capped by the frame guard, not the pixel budget
|
||||
ImageElementBase.AffordableFrames(8, 8).ShouldBe(ImageElementBase.MaxRemoteFrames);
|
||||
|
||||
// 1000x1000 -> 50M / 1M = 50 frames
|
||||
ImageElementBase.AffordableFrames(1000, 1000).ShouldBe(50);
|
||||
|
||||
// a frame so large only one fits
|
||||
ImageElementBase.AffordableFrames(7000, 7000).ShouldBe(1);
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Should_Allow_A_Product_Exactly_At_The_Budget()
|
||||
{
|
||||
// 10000 x 5000 x 1 == MaxRemoteDecodedPixels exactly
|
||||
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(10000, 5000, 1, ImageUri));
|
||||
}
|
||||
|
||||
// the retained-frame budget is INDEPENDENT of the decode budget: this source is trivial to
|
||||
// decode (6 MP total) but retains ~5 GB of SKBitmap once every frame is scaled to 1080p
|
||||
[Test]
|
||||
public void Should_Reject_Cheap_Frames_That_Are_Expensive_Once_Scaled()
|
||||
{
|
||||
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(100, 100, 600, ImageUri));
|
||||
Should.NotThrow(() => RemoteImageDecodeBudget.EnsureDecodeAffordable(100, 100, 600, ImageUri));
|
||||
|
||||
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
|
||||
() => ImageElementBase.EnsureScaledFramesAffordable(600, 1920, 1080, ImageUri));
|
||||
@@ -156,13 +92,13 @@ public class RemoteImageDecodeLimitTests
|
||||
[Test]
|
||||
public async Task Should_Reject_An_Animation_Whose_Header_Under_Reports_Its_Frames()
|
||||
{
|
||||
await using MemoryStream stream = Apng(64, 64, ImageElementBase.MaxRemoteFrames + 100);
|
||||
await using MemoryStream stream = Apng(64, 64, RemoteImageDecodeBudget.MaxRemoteFrames + 100);
|
||||
|
||||
// the premise: the header really does under-report, so a header-derived budget waves it through
|
||||
stream.Position = 0;
|
||||
ImageInfo info = await Image.IdentifyAsync(stream);
|
||||
info.FrameMetadataCollection.Count.ShouldBe(0, "the APNG header under-reports; that is the whole point");
|
||||
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(64, 64, info.FrameMetadataCollection.Count, ImageUri));
|
||||
Should.NotThrow(() => RemoteImageDecodeBudget.EnsureDecodeAffordable(64, 64, info.FrameMetadataCollection.Count, ImageUri));
|
||||
|
||||
stream.Position = 0;
|
||||
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
using ErsatzTV.Core.Images;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using SixLabors.ImageSharp;
|
||||
using SixLabors.ImageSharp.Formats;
|
||||
using Image = SixLabors.ImageSharp.Image;
|
||||
|
||||
namespace ErsatzTV.Infrastructure.Images;
|
||||
|
||||
public class RemoteImageValidator : IRemoteImageValidator
|
||||
{
|
||||
public async Task Validate(Stream stream, Uri uri, CancellationToken cancellationToken)
|
||||
{
|
||||
using Image _ = await DecodeAndValidate(stream, uri, cancellationToken);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Decodes a remote image only after the header says decoding it is affordable.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The fetcher's byte cap does NOT bound this: a decompression bomb is small on the wire and
|
||||
/// huge in memory. A 4 KB PNG can declare 30000x30000 (~3.6 GB), and a 60 KiB GIF can
|
||||
/// declare 2500x2500 across 600 frames (~14 GiB). The budget is therefore on the PRODUCT of
|
||||
/// dimensions and frames, read from the header before the decoder allocates.
|
||||
/// Local images are deliberately not checked — they are files an operator put on disk, not
|
||||
/// bytes an arbitrary host returned. (ersatztv#511)
|
||||
/// </remarks>
|
||||
public static async Task<Image> DecodeAndValidate(Stream stream, Uri uri, CancellationToken cancellationToken)
|
||||
{
|
||||
if (!stream.CanSeek)
|
||||
{
|
||||
// Identify consumes the stream, so the decode below needs to rewind it. Fail with the
|
||||
// real reason rather than letting Position throw NotSupportedException, which the
|
||||
// caller's blanket catch would report as a generic initialization failure.
|
||||
throw new InvalidOperationException(
|
||||
$"Remote image {uri} was returned on a non-seekable stream; IRemoteImageFetcher must "
|
||||
+ "return a fully buffered, seekable stream");
|
||||
}
|
||||
|
||||
// MaxFrames = 1 on the IDENTIFY is not a limit, it is a workaround: a default Identify
|
||||
// throws InvalidImageContentException on most APNGs — including files ImageSharp's own
|
||||
// PngEncoder wrote, which Image.Load then reads back perfectly (measured: 13 of 16 shapes).
|
||||
// Without this, adding the header pre-pass would silently disable every animated-PNG logo
|
||||
// that worked before this change. Only Width/Height are read below, and those stay correct.
|
||||
ImageInfo info = await Image.IdentifyAsync(
|
||||
new DecoderOptions { MaxFrames = 1 },
|
||||
stream,
|
||||
cancellationToken);
|
||||
|
||||
// DIMENSIONS from the header are trustworthy; the FRAME COUNT is not, and is deliberately
|
||||
// not used as a budget input. Measured on ImageSharp 3.1.12: an APNG reports
|
||||
// FrameMetadataCollection.Count == 0 while the decoder happily produces 600 frames, so a
|
||||
// header-derived frame budget is enforced on a number the decoder does not honor — a
|
||||
// 134 KiB file decodes to ~36 GiB. (Second adversarial re-review; ersatztv#511.)
|
||||
RemoteImageDecodeBudget.EnsureDimensionsAffordable(info.Width, info.Height, uri);
|
||||
|
||||
int affordableFrames = RemoteImageDecodeBudget.AffordableFrames(info.Width, info.Height);
|
||||
|
||||
stream.Position = 0;
|
||||
|
||||
// MaxFrames is enforced BY THE DECODER, so it holds whatever the header claimed — measured
|
||||
// as honored by every animated decoder here (APNG, GIF, WebP, TIFF). Ask for two more than
|
||||
// the budget allows so that an animation exactly AT the limit still decodes in full, while
|
||||
// anything over it is present in the decoded image for the post-decode check below to
|
||||
// reject. Slop is at most two frames: MaxFrames = N yields N frames for GIF/WebP/TIFF but
|
||||
// N-1 for APNG, so the exact count varies by format and only the upper bound matters.
|
||||
var decoderOptions = new DecoderOptions { MaxFrames = (uint)(affordableFrames + 2) };
|
||||
|
||||
Image image = await Image.LoadAsync(decoderOptions, stream, cancellationToken);
|
||||
|
||||
try
|
||||
{
|
||||
// re-verify against REALITY rather than against the header. this is the check that
|
||||
// actually holds; everything above it only avoids decoding when we can tell in advance.
|
||||
RemoteImageDecodeBudget.EnsureDecodeAffordable(image.Width, image.Height, image.Frames.Count, uri);
|
||||
return image;
|
||||
}
|
||||
catch
|
||||
{
|
||||
image.Dispose();
|
||||
throw;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,35 @@
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Streaming;
|
||||
using LanguageExt;
|
||||
|
||||
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 (throws if unsafe); we cache the raw bytes
|
||||
await validator.Validate(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}");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,7 +1,9 @@
|
||||
using System.Runtime.InteropServices;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Images;
|
||||
using ErsatzTV.Core.Interfaces.Streaming;
|
||||
using ErsatzTV.FFmpeg.State;
|
||||
using ErsatzTV.Infrastructure.Images;
|
||||
using SixLabors.ImageSharp;
|
||||
using SixLabors.ImageSharp.Formats;
|
||||
using SixLabors.ImageSharp.Formats.Gif;
|
||||
@@ -17,24 +19,10 @@ namespace ErsatzTV.Infrastructure.Streaming.Graphics;
|
||||
|
||||
public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) : GraphicsElement, IDisposable
|
||||
{
|
||||
/// <summary>
|
||||
/// Ceiling on TOTAL decoded pixels for a remote image — width x height x frames, as one
|
||||
/// product. Checking dimensions and frame count independently does not bound the decode:
|
||||
/// 2500x2500 x 600 frames is 60 KiB on the wire, passes a 50 MP dimension check and a 600
|
||||
/// frame check, and costs ~14 GiB to decode. Only the product catches that.
|
||||
/// 8K is ~33 MP, so a single large still fits comfortably.
|
||||
/// </summary>
|
||||
internal const long MaxRemoteDecodedPixels = 50_000_000;
|
||||
|
||||
/// <summary>
|
||||
/// Frame ceiling for a remote animation, kept alongside the product budget as a cheap,
|
||||
/// legible guard against absurd frame counts of tiny frames.
|
||||
/// </summary>
|
||||
internal const int MaxRemoteFrames = 600;
|
||||
|
||||
/// <summary>
|
||||
/// Ceiling on total pixels RETAINED after scaling — frames x scaled width x scaled height.
|
||||
/// Independent of the source budget above: a 100x100 source is trivial to decode but, at 600
|
||||
/// Independent of the source decode budget in <see cref="RemoteImageDecodeBudget" />: a
|
||||
/// 100x100 source is trivial to decode but, at 600
|
||||
/// frames scaled to 1920x1080, retains ~5 GB of <see cref="SKBitmap" />. At 4 bytes per
|
||||
/// pixel this bounds retention at ~800 MB, which still allows ~96 full-frame 1080p frames
|
||||
/// (~3s at 30fps) or 600 frames of a 577x577 logo.
|
||||
@@ -144,111 +132,7 @@ public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) :
|
||||
/// bytes an arbitrary host returned. (ersatztv#511)
|
||||
/// </remarks>
|
||||
internal static async Task<Image> DecodeRemoteImage(Stream stream, Uri uri, CancellationToken cancellationToken)
|
||||
{
|
||||
if (!stream.CanSeek)
|
||||
{
|
||||
// Identify consumes the stream, so the decode below needs to rewind it. Fail with the
|
||||
// real reason rather than letting Position throw NotSupportedException, which the
|
||||
// caller's blanket catch would report as a generic initialization failure.
|
||||
throw new InvalidOperationException(
|
||||
$"Remote image {uri} was returned on a non-seekable stream; IRemoteImageFetcher must "
|
||||
+ "return a fully buffered, seekable stream");
|
||||
}
|
||||
|
||||
// MaxFrames = 1 on the IDENTIFY is not a limit, it is a workaround: a default Identify
|
||||
// throws InvalidImageContentException on most APNGs — including files ImageSharp's own
|
||||
// PngEncoder wrote, which Image.Load then reads back perfectly (measured: 13 of 16 shapes).
|
||||
// Without this, adding the header pre-pass would silently disable every animated-PNG logo
|
||||
// that worked before this change. Only Width/Height are read below, and those stay correct.
|
||||
ImageInfo info = await Image.IdentifyAsync(
|
||||
new DecoderOptions { MaxFrames = 1 },
|
||||
stream,
|
||||
cancellationToken);
|
||||
|
||||
// DIMENSIONS from the header are trustworthy; the FRAME COUNT is not, and is deliberately
|
||||
// not used as a budget input. Measured on ImageSharp 3.1.12: an APNG reports
|
||||
// FrameMetadataCollection.Count == 0 while the decoder happily produces 600 frames, so a
|
||||
// header-derived frame budget is enforced on a number the decoder does not honor — a
|
||||
// 134 KiB file decodes to ~36 GiB. (Second adversarial re-review; ersatztv#511.)
|
||||
EnsureDimensionsAffordable(info.Width, info.Height, uri);
|
||||
|
||||
int affordableFrames = AffordableFrames(info.Width, info.Height);
|
||||
|
||||
stream.Position = 0;
|
||||
|
||||
// MaxFrames is enforced BY THE DECODER, so it holds whatever the header claimed — measured
|
||||
// as honored by every animated decoder here (APNG, GIF, WebP, TIFF). Ask for two more than
|
||||
// the budget allows so that an animation exactly AT the limit still decodes in full, while
|
||||
// anything over it is present in the decoded image for the post-decode check below to
|
||||
// reject. Slop is at most two frames: MaxFrames = N yields N frames for GIF/WebP/TIFF but
|
||||
// N-1 for APNG, so the exact count varies by format and only the upper bound matters.
|
||||
var decoderOptions = new DecoderOptions { MaxFrames = (uint)(affordableFrames + 2) };
|
||||
|
||||
Image image = await Image.LoadAsync(decoderOptions, stream, cancellationToken);
|
||||
|
||||
try
|
||||
{
|
||||
// re-verify against REALITY rather than against the header. this is the check that
|
||||
// actually holds; everything above it only avoids decoding when we can tell in advance.
|
||||
EnsureDecodeAffordable(image.Width, image.Height, image.Frames.Count, uri);
|
||||
return image;
|
||||
}
|
||||
catch
|
||||
{
|
||||
image.Dispose();
|
||||
throw;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Rejects a single frame that cannot fit the decode budget on its own.</summary>
|
||||
internal 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");
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// How many frames of this size the decode budget affords. Used to cap the DECODER, so the
|
||||
/// bound does not depend on the header's frame count being honest.
|
||||
/// </summary>
|
||||
internal static int AffordableFrames(int width, int height)
|
||||
{
|
||||
long perFrame = Math.Max((long)width * height, 1);
|
||||
return (int)Math.Clamp(MaxRemoteDecodedPixels / perFrame, 1, MaxRemoteFrames);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The decode-budget policy, kept free of I/O so the arithmetic can be tested at every
|
||||
/// boundary without materializing multi-gigabyte images. Call this with the number of frames
|
||||
/// the decoder ACTUALLY produced — never with a header-reported count, which can be zero for
|
||||
/// an animation the decoder then expands to hundreds of frames.
|
||||
/// </summary>
|
||||
internal 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");
|
||||
}
|
||||
|
||||
// THE PRODUCT is the real bound. Checking dimensions and frames separately lets a 60 KiB
|
||||
// 2500x2500 x600 GIF through at a ~14 GiB decode cost. (Found by adversarial re-review of
|
||||
// the first fix for this, which checked them independently.)
|
||||
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");
|
||||
}
|
||||
}
|
||||
=> await RemoteImageValidator.DecodeAndValidate(stream, uri, cancellationToken);
|
||||
|
||||
/// <summary>
|
||||
/// Bounds what is RETAINED after scaling. Separate from the source budget because the two
|
||||
|
||||
@@ -1,12 +1,17 @@
|
||||
using System.Buffers.Binary;
|
||||
using ErsatzTV.Application.Artworks;
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Api.Artwork;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
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 static LanguageExt.Prelude;
|
||||
|
||||
namespace ErsatzTV.Tests.Application.Artworks;
|
||||
@@ -14,10 +19,11 @@ namespace ErsatzTV.Tests.Application.Artworks;
|
||||
[TestFixture]
|
||||
public class UploadArtworkHandlerTests
|
||||
{
|
||||
// A minimal valid 1x1 PNG. The handler derives the content type from bytes like these, never
|
||||
// from a client-declared value (issue #283), so the tests exercise the real sniffer.
|
||||
private static readonly byte[] PngBytes = Convert.FromBase64String(
|
||||
"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+M8AAAMCAoAB/QwAAAAASUVORK5CYII=");
|
||||
// A minimal, genuinely decodable PNG. The handler derives the content type from bytes like
|
||||
// these, never from a client-declared value (issue #283), so the tests exercise the real
|
||||
// sniffer; and it now also decode-budget-validates (issue #525), so these bytes must survive a
|
||||
// full decode, not just a magic-byte sniff.
|
||||
private static readonly byte[] PngBytes = EncodePng(1, 1);
|
||||
|
||||
private IImageCache _imageCache = null!;
|
||||
private UploadArtworkHandler _handler = null!;
|
||||
@@ -26,7 +32,8 @@ public class UploadArtworkHandlerTests
|
||||
public void SetUp()
|
||||
{
|
||||
_imageCache = Substitute.For<IImageCache>();
|
||||
_handler = new UploadArtworkHandler(_imageCache);
|
||||
// Use the REAL validator so the decode-budget check is exercised end to end, not stubbed.
|
||||
_handler = new UploadArtworkHandler(_imageCache, new RemoteImageValidator());
|
||||
}
|
||||
|
||||
[Test]
|
||||
@@ -101,6 +108,94 @@ public class UploadArtworkHandlerTests
|
||||
LeftOf(result).Value.ShouldBe("disk full");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Reject_An_Upload_That_Busts_The_Decode_Budget()
|
||||
{
|
||||
// A tiny PNG header declaring a 30000x30000 canvas: a decompression bomb, small on the wire
|
||||
// and huge in memory. The content-type sniff passes (it is a real PNG), so only the
|
||||
// decode-budget check can stop it entering the cache.
|
||||
await using MemoryStream bomb = PngHeaderDeclaring(30000, 30000);
|
||||
Either<BaseError, ArtworkUploadResponseModel> result =
|
||||
await _handler.Handle(new UploadArtwork(bomb, ArtworkKind.Logo), CancellationToken.None);
|
||||
|
||||
result.IsLeft.ShouldBeTrue();
|
||||
LeftOf(result).Value.ShouldContain("pixel limit");
|
||||
await _imageCache.DidNotReceive().SaveArtworkToCache(Arg.Any<Stream>(), Arg.Any<ArtworkKind>());
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Accept_A_Normal_Upload()
|
||||
{
|
||||
_imageCache.SaveArtworkToCache(Arg.Any<Stream>(), ArtworkKind.Logo)
|
||||
.Returns(Right<BaseError, string>("ok789"));
|
||||
|
||||
await using MemoryStream png = await RealPng(64, 64);
|
||||
Either<BaseError, ArtworkUploadResponseModel> result =
|
||||
await _handler.Handle(new UploadArtwork(png, ArtworkKind.Logo), CancellationToken.None);
|
||||
|
||||
result.IsRight.ShouldBeTrue();
|
||||
}
|
||||
|
||||
/// <summary>A real, decodable PNG as a stream.</summary>
|
||||
private static async Task<MemoryStream> RealPng(int width, int height)
|
||||
{
|
||||
using var image = new Image<Rgba32>(width, height);
|
||||
var stream = new MemoryStream();
|
||||
await image.SaveAsync(stream, new PngEncoder());
|
||||
stream.Position = 0;
|
||||
return stream;
|
||||
}
|
||||
|
||||
/// <summary>A real, decodable PNG as bytes.</summary>
|
||||
private static byte[] EncodePng(int width, int height)
|
||||
{
|
||||
using var image = new Image<Rgba32>(width, height);
|
||||
using var stream = new MemoryStream();
|
||||
image.Save(stream, new PngEncoder());
|
||||
return stream.ToArray();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A structurally complete PNG whose IHDR is patched to declare <paramref name="width" /> x
|
||||
/// <paramref name="height" />: a decompression bomb. It starts from a real 1x1 PNG so the
|
||||
/// upload's content-type sniff (SkiaSharp <c>SKCodec.Create</c>, which needs a full codec,
|
||||
/// not a lone IHDR) still recognizes it as image/png; the huge declared dimensions are read
|
||||
/// from the header by the validator's Identify and rejected by the pixel budget before any
|
||||
/// pixels are decoded (the patched IDAT never has to be valid at that size).
|
||||
/// </summary>
|
||||
private static MemoryStream PngHeaderDeclaring(int width, int height)
|
||||
{
|
||||
// A real PNG: [8-byte signature][IHDR: 4 len + 4 "IHDR" + 13 data + 4 CRC] then IDAT/IEND.
|
||||
byte[] bytes = EncodePng(1, 1);
|
||||
|
||||
// IHDR data begins at offset 16 (8 signature + 4 length + 4 "IHDR"); width then height.
|
||||
BinaryPrimitives.WriteInt32BigEndian(bytes.AsSpan(16), width);
|
||||
BinaryPrimitives.WriteInt32BigEndian(bytes.AsSpan(20), height);
|
||||
|
||||
// Recompute the IHDR CRC over "IHDR" + the 13 data bytes (offset 12, length 17).
|
||||
uint crc = Crc32(bytes.AsSpan(12, 17));
|
||||
BinaryPrimitives.WriteUInt32BigEndian(bytes.AsSpan(29), crc);
|
||||
|
||||
return new MemoryStream(bytes) { Position = 0 };
|
||||
}
|
||||
|
||||
// PNG chunk CRC-32 (IEEE, reflected). hand-rolled because the repo does not reference
|
||||
// System.IO.Hashing, and ImageSharp validates the CRC of critical chunks like IHDR.
|
||||
private static uint Crc32(ReadOnlySpan<byte> data)
|
||||
{
|
||||
uint crc = 0xFFFFFFFF;
|
||||
foreach (byte b in data)
|
||||
{
|
||||
crc ^= b;
|
||||
for (var i = 0; i < 8; i++)
|
||||
{
|
||||
crc = (crc & 1) != 0 ? (crc >> 1) ^ 0xEDB88320 : crc >> 1;
|
||||
}
|
||||
}
|
||||
|
||||
return crc ^ 0xFFFFFFFF;
|
||||
}
|
||||
|
||||
private static TR RightOf<TR>(Either<BaseError, TR> either) =>
|
||||
either.Match(Right: v => v, Left: e => throw new AssertionException($"Expected Right, got Left: {e.Value}"));
|
||||
|
||||
|
||||
@@ -9,6 +9,7 @@ using ErsatzTV.Core.Api.LibraryBrowse;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Filler;
|
||||
using ErsatzTV.Core.Errors;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Search;
|
||||
using ErsatzTV.Core.Scheduling;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
@@ -19,6 +20,7 @@ using Microsoft.Extensions.Logging.Abstractions;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
using static LanguageExt.Prelude;
|
||||
using DomainChannel = ErsatzTV.Core.Domain.Channel;
|
||||
using DomainPlaylistItem = ErsatzTV.Core.Domain.PlaylistItem;
|
||||
|
||||
@@ -30,6 +32,7 @@ public class CreateChannelFromLineupHandlerTests
|
||||
private Channel<IBackgroundServiceRequest> _background = null!;
|
||||
private InMemoryTvContext _db = null!;
|
||||
private ISearchTargets _searchTargets = null!;
|
||||
private IRemoteLogoCacher _remoteLogoCacher = null!;
|
||||
|
||||
[SetUp]
|
||||
public async Task SetUp()
|
||||
@@ -37,6 +40,7 @@ public class CreateChannelFromLineupHandlerTests
|
||||
_background = System.Threading.Channels.Channel.CreateUnbounded<IBackgroundServiceRequest>();
|
||||
_db = await InMemoryTvContext.CreateAsync();
|
||||
_searchTargets = Substitute.For<ISearchTargets>();
|
||||
_remoteLogoCacher = Substitute.For<IRemoteLogoCacher>();
|
||||
}
|
||||
|
||||
[TearDown]
|
||||
@@ -610,6 +614,71 @@ public class CreateChannelFromLineupHandlerTests
|
||||
LeftOf(result).Value.ShouldContain("External logo url is invalid");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Download_External_Url_Logo_And_Store_Cache_Name()
|
||||
{
|
||||
await SeedTemplateDependencies();
|
||||
await SeedTemplate();
|
||||
await SeedMovie(42);
|
||||
_remoteLogoCacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, string>("cachedhash"));
|
||||
|
||||
var logo = new ArtworkContentTypeModel("https://example.com/logo.png", string.Empty);
|
||||
|
||||
Either<BaseError, CreateChannelFromLineupResponseModel> result =
|
||||
await MakeHandler().Handle(MakeRequest(logo: logo), CancellationToken.None);
|
||||
|
||||
result.IsRight.ShouldBeTrue();
|
||||
await using TvContext context = _db.CreateContext();
|
||||
Artwork artwork = (await context.Channels.Include(c => c.Artwork).SingleAsync())
|
||||
.Artwork.Single(a => a.ArtworkKind == ArtworkKind.Logo);
|
||||
artwork.Path.ShouldBe("cachedhash");
|
||||
artwork.IsExternalUrl().ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Fail_The_Create_When_The_Logo_Download_Fails()
|
||||
{
|
||||
await SeedTemplateDependencies();
|
||||
await SeedTemplate();
|
||||
await SeedMovie(42);
|
||||
_remoteLogoCacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Left<BaseError, string>(BaseError.New("Could not download logo: host timed out")));
|
||||
|
||||
var logo = new ArtworkContentTypeModel("https://example.com/logo.png", string.Empty);
|
||||
|
||||
Either<BaseError, CreateChannelFromLineupResponseModel> result =
|
||||
await MakeHandler().Handle(MakeRequest(logo: logo), CancellationToken.None);
|
||||
|
||||
LeftOf(result).Value.ShouldContain("Could not download logo");
|
||||
|
||||
// nothing is persisted when the download fails (resolution runs before PersistAndDispatch)
|
||||
await using TvContext context = _db.CreateContext();
|
||||
(await context.Channels.CountAsync()).ShouldBe(0);
|
||||
(await context.Playouts.CountAsync()).ShouldBe(0);
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Not_Call_The_Cacher_For_An_Uploaded_Logo_Path()
|
||||
{
|
||||
await SeedTemplateDependencies();
|
||||
await SeedTemplate();
|
||||
await SeedMovie(42);
|
||||
|
||||
var logo = new ArtworkContentTypeModel("iptv/logos/deadbeef", string.Empty);
|
||||
|
||||
Either<BaseError, CreateChannelFromLineupResponseModel> result =
|
||||
await MakeHandler().Handle(MakeRequest(logo: logo), CancellationToken.None);
|
||||
|
||||
result.IsRight.ShouldBeTrue();
|
||||
await _remoteLogoCacher.DidNotReceive().CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>());
|
||||
|
||||
await using TvContext context = _db.CreateContext();
|
||||
Artwork artwork = (await context.Channels.Include(c => c.Artwork).SingleAsync())
|
||||
.Artwork.Single(a => a.ArtworkKind == ArtworkKind.Logo);
|
||||
artwork.Path.ShouldBe("deadbeef");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Return_NotFound_When_Lineup_Target_Is_Missing()
|
||||
{
|
||||
@@ -638,6 +707,7 @@ public class CreateChannelFromLineupHandlerTests
|
||||
_background.Writer,
|
||||
_db.Factory,
|
||||
_searchTargets,
|
||||
_remoteLogoCacher,
|
||||
NullLogger<CreateChannelFromLineupHandler>.Instance);
|
||||
|
||||
private async Task SeedTemplateDependencies()
|
||||
|
||||
@@ -1,19 +1,22 @@
|
||||
using ErsatzTV.Application.Channels;
|
||||
using ErsatzTV.Core;
|
||||
using LanguageExt;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Errors;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
using static LanguageExt.Prelude;
|
||||
|
||||
namespace ErsatzTV.Tests.Application.Channels;
|
||||
|
||||
[TestFixture]
|
||||
public class CreateChannelHandlerTests : ChannelHandlerTestBase
|
||||
{
|
||||
private CreateChannelHandler MakeHandler() => new(Worker, Db.Factory, SearchTargets);
|
||||
private CreateChannelHandler MakeHandler() => new(Worker, Db.Factory, SearchTargets, RemoteLogoCacher);
|
||||
|
||||
[Test]
|
||||
public async Task Should_Create_Channel_When_Valid()
|
||||
@@ -99,6 +102,61 @@ public class CreateChannelHandlerTests : ChannelHandlerTestBase
|
||||
error.Value.ShouldContain("FFmpegProfile");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Download_External_Url_Logo_And_Store_Cache_Name()
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
RemoteLogoCacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, string>("cachedhash"));
|
||||
|
||||
Either<BaseError, CreateChannelResult> result = await MakeHandler().Handle(
|
||||
MakeCreate(number: "20", 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.Number == "20")
|
||||
.Artwork.Single(a => a.ArtworkKind == ArtworkKind.Logo);
|
||||
logo.Path.ShouldBe("cachedhash");
|
||||
logo.IsExternalUrl().ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Fail_The_Create_When_The_Logo_Download_Fails()
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
RemoteLogoCacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Left<BaseError, string>(BaseError.New("Could not download logo: host timed out")));
|
||||
|
||||
Either<BaseError, CreateChannelResult> result = await MakeHandler().Handle(
|
||||
MakeCreate(number: "21", logoPath: "https://example.com/logo.png"),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsLeft.ShouldBeTrue();
|
||||
LeftOf(result).Value.ShouldContain("Could not download logo");
|
||||
|
||||
await using TvContext db = Db.CreateContext();
|
||||
(await db.Channels.AnyAsync(c => c.Number == "21")).ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Not_Call_The_Cacher_For_An_Uploaded_Logo_Path()
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
|
||||
Either<BaseError, CreateChannelResult> result = await MakeHandler().Handle(
|
||||
MakeCreate(number: "22", logoPath: "iptv/logos/deadbeef"),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsRight.ShouldBeTrue();
|
||||
await RemoteLogoCacher.DidNotReceive().CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>());
|
||||
|
||||
await using TvContext db = Db.CreateContext();
|
||||
Artwork logo = db.Channels.Include(c => c.Artwork).Single(c => c.Number == "22")
|
||||
.Artwork.Single(a => a.ArtworkKind == ArtworkKind.Logo);
|
||||
logo.Path.ShouldBe("deadbeef");
|
||||
}
|
||||
|
||||
private static BaseError LeftOf<TR>(Either<BaseError, TR> either) =>
|
||||
either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result"));
|
||||
}
|
||||
|
||||
@@ -6,15 +6,17 @@ using ErsatzTV.Core.Errors;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
using static LanguageExt.Prelude;
|
||||
|
||||
namespace ErsatzTV.Tests.Application.Channels;
|
||||
|
||||
[TestFixture]
|
||||
public class UpdateChannelHandlerTests : ChannelHandlerTestBase
|
||||
{
|
||||
private UpdateChannelHandler MakeHandler() => new(Worker, Db.Factory, SearchTargets);
|
||||
private UpdateChannelHandler MakeHandler() => new(Worker, Db.Factory, SearchTargets, RemoteLogoCacher);
|
||||
|
||||
[Test]
|
||||
public async Task Should_Return_NotFoundError_When_Channel_Missing()
|
||||
@@ -115,6 +117,24 @@ public class UpdateChannelHandlerTests : ChannelHandlerTestBase
|
||||
error.Value.ShouldContain("FFmpegProfile");
|
||||
}
|
||||
|
||||
// the applicative validation accumulates every failure; the 400 body must carry all of them,
|
||||
// not just the first (regression guard for the #525 handler refactor — errors.Join, not .Head).
|
||||
[Test]
|
||||
public async Task Should_Report_All_Validation_Errors_Not_Just_The_First()
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
await SeedChannel(1, "5");
|
||||
|
||||
Either<BaseError, ChannelViewModel> result =
|
||||
await MakeHandler().Handle(
|
||||
MakeUpdate(1, number: "5", group: "", ffmpegProfileId: 999),
|
||||
CancellationToken.None);
|
||||
|
||||
BaseError error = LeftOf(result);
|
||||
error.Value.ShouldContain("group");
|
||||
error.Value.ShouldContain("FFmpegProfile");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Reject_Mirror_Transition_When_Channel_Has_Playout()
|
||||
{
|
||||
@@ -186,6 +206,53 @@ public class UpdateChannelHandlerTests : ChannelHandlerTestBase
|
||||
channel.PlayoutOffset.ShouldBeNull();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Download_External_Url_Logo_And_Store_Cache_Name()
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
Channel channel = await SeedChannel(1, "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()
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
Channel channel = await SeedChannel(1, "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()
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
Channel channel = await SeedChannel(1, "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>());
|
||||
}
|
||||
|
||||
private static BaseError LeftOf<TR>(Either<BaseError, TR> either) =>
|
||||
either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result"));
|
||||
}
|
||||
|
||||
@@ -21,7 +21,7 @@ public class ChannelLifecycleIntegrationTests : ChannelHandlerTestBase
|
||||
{
|
||||
await SeedFFmpegProfile();
|
||||
|
||||
var createHandler = new CreateChannelHandler(Worker, Db.Factory, SearchTargets);
|
||||
var createHandler = new CreateChannelHandler(Worker, Db.Factory, SearchTargets, RemoteLogoCacher);
|
||||
Either<BaseError, CreateChannelResult> created =
|
||||
await createHandler.Handle(MakeCreate(number: "42", name: "Integration"), CancellationToken.None);
|
||||
|
||||
|
||||
@@ -0,0 +1,114 @@
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Services.RunOnce;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using LanguageExt;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Tests.Services;
|
||||
|
||||
[TestFixture]
|
||||
public class ExternalLogoMigratorTests
|
||||
{
|
||||
private InMemoryTvContext _db = null!;
|
||||
private IRemoteLogoCacher _cacher = null!;
|
||||
|
||||
[SetUp]
|
||||
public async Task SetUp()
|
||||
{
|
||||
_db = await InMemoryTvContext.CreateAsync();
|
||||
_cacher = Substitute.For<IRemoteLogoCacher>();
|
||||
}
|
||||
|
||||
[TearDown]
|
||||
public async Task TearDown() => await _db.DisposeAsync();
|
||||
|
||||
[Test]
|
||||
public async Task Should_Convert_Url_Logo_Row_To_Cache_Name()
|
||||
{
|
||||
await Seed(new Artwork
|
||||
{
|
||||
Path = "https://example.com/logo.png",
|
||||
ArtworkKind = ArtworkKind.Logo
|
||||
});
|
||||
|
||||
_cacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Either<BaseError, string>.Right("hash"));
|
||||
|
||||
await using (TvContext db = _db.CreateContext())
|
||||
{
|
||||
await ExternalLogoMigratorService.MigrateAsync(
|
||||
db,
|
||||
_cacher,
|
||||
NullLogger.Instance,
|
||||
CancellationToken.None);
|
||||
}
|
||||
|
||||
await using TvContext verify = _db.CreateContext();
|
||||
Artwork row = await verify.Artwork.SingleAsync();
|
||||
row.Path.ShouldBe("hash");
|
||||
row.IsExternalUrl().ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Leave_Row_Unchanged_When_Cacher_Fails()
|
||||
{
|
||||
const string Url = "https://example.com/logo.png";
|
||||
await Seed(new Artwork
|
||||
{
|
||||
Path = Url,
|
||||
ArtworkKind = ArtworkKind.Logo
|
||||
});
|
||||
|
||||
_cacher.CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Either<BaseError, string>.Left(BaseError.New("boom")));
|
||||
|
||||
await using (TvContext db = _db.CreateContext())
|
||||
{
|
||||
await ExternalLogoMigratorService.MigrateAsync(
|
||||
db,
|
||||
_cacher,
|
||||
NullLogger.Instance,
|
||||
CancellationToken.None);
|
||||
}
|
||||
|
||||
await using TvContext verify = _db.CreateContext();
|
||||
Artwork row = await verify.Artwork.SingleAsync();
|
||||
row.Path.ShouldBe(Url);
|
||||
row.IsExternalUrl().ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Should_Not_Call_Cacher_For_Already_Migrated_Rows()
|
||||
{
|
||||
await Seed(new Artwork
|
||||
{
|
||||
Path = "some-bare-hash",
|
||||
ArtworkKind = ArtworkKind.Logo
|
||||
});
|
||||
|
||||
await using (TvContext db = _db.CreateContext())
|
||||
{
|
||||
await ExternalLogoMigratorService.MigrateAsync(
|
||||
db,
|
||||
_cacher,
|
||||
NullLogger.Instance,
|
||||
CancellationToken.None);
|
||||
}
|
||||
|
||||
await _cacher.DidNotReceive().CacheFromUrl(Arg.Any<Uri>(), Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
private async Task Seed(Artwork artwork)
|
||||
{
|
||||
await using TvContext db = _db.CreateContext();
|
||||
await db.Artwork.AddAsync(artwork);
|
||||
await db.SaveChangesAsync();
|
||||
}
|
||||
}
|
||||
@@ -3,6 +3,7 @@ using ErsatzTV.Application;
|
||||
using ErsatzTV.Application.Artworks;
|
||||
using ErsatzTV.Application.Channels;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Core.Interfaces.Search;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using NSubstitute;
|
||||
@@ -16,6 +17,7 @@ public abstract class ChannelHandlerTestBase
|
||||
protected InMemoryTvContext Db = null!;
|
||||
protected ChannelWriter<IBackgroundServiceRequest> Worker = null!;
|
||||
protected ISearchTargets SearchTargets = null!;
|
||||
protected IRemoteLogoCacher RemoteLogoCacher = null!;
|
||||
|
||||
[SetUp]
|
||||
public async Task BaseSetUp()
|
||||
@@ -23,6 +25,7 @@ public abstract class ChannelHandlerTestBase
|
||||
Db = await InMemoryTvContext.CreateAsync();
|
||||
Worker = System.Threading.Channels.Channel.CreateUnbounded<IBackgroundServiceRequest>().Writer;
|
||||
SearchTargets = Substitute.For<ISearchTargets>();
|
||||
RemoteLogoCacher = Substitute.For<IRemoteLogoCacher>();
|
||||
}
|
||||
|
||||
[TearDown]
|
||||
@@ -35,7 +38,7 @@ public abstract class ChannelHandlerTestBase
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
protected async Task SeedChannel(
|
||||
protected async Task<DomainChannel> SeedChannel(
|
||||
int id,
|
||||
string number,
|
||||
string name = "Test",
|
||||
@@ -43,25 +46,26 @@ public abstract class ChannelHandlerTestBase
|
||||
string group = "ErsatzTV")
|
||||
{
|
||||
await using TvContext context = Db.CreateContext();
|
||||
context.Channels.Add(
|
||||
new DomainChannel(Guid.NewGuid())
|
||||
{
|
||||
Id = id,
|
||||
Number = number,
|
||||
Name = name,
|
||||
Group = group,
|
||||
Categories = string.Empty,
|
||||
FFmpegProfileId = ffmpegProfileId,
|
||||
StreamSelector = string.Empty,
|
||||
PreferredAudioLanguageCode = string.Empty,
|
||||
PreferredAudioTitle = string.Empty,
|
||||
PreferredSubtitleLanguageCode = string.Empty,
|
||||
MusicVideoCreditsTemplate = string.Empty,
|
||||
StreamingMode = StreamingMode.TransportStreamHybrid,
|
||||
PlayoutSource = ChannelPlayoutSource.Generated,
|
||||
PlayoutMode = ChannelPlayoutMode.Continuous
|
||||
});
|
||||
var channel = new DomainChannel(Guid.NewGuid())
|
||||
{
|
||||
Id = id,
|
||||
Number = number,
|
||||
Name = name,
|
||||
Group = group,
|
||||
Categories = string.Empty,
|
||||
FFmpegProfileId = ffmpegProfileId,
|
||||
StreamSelector = string.Empty,
|
||||
PreferredAudioLanguageCode = string.Empty,
|
||||
PreferredAudioTitle = string.Empty,
|
||||
PreferredSubtitleLanguageCode = string.Empty,
|
||||
MusicVideoCreditsTemplate = string.Empty,
|
||||
StreamingMode = StreamingMode.TransportStreamHybrid,
|
||||
PlayoutSource = ChannelPlayoutSource.Generated,
|
||||
PlayoutMode = ChannelPlayoutMode.Continuous
|
||||
};
|
||||
context.Channels.Add(channel);
|
||||
await context.SaveChangesAsync();
|
||||
return channel;
|
||||
}
|
||||
|
||||
protected async Task SeedPlayout(int id, int channelId)
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Images;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using LanguageExt;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
|
||||
namespace ErsatzTV.Services.RunOnce;
|
||||
|
||||
/// <summary>
|
||||
/// One-time startup migration that downloads existing external-URL channel logos into the image
|
||||
/// cache. Before ersatztv#525 a channel logo could be stored as a raw http(s) URL in
|
||||
/// <see cref="Artwork.Path" />; the render path used to fetch it live. Now that URLs are cached on
|
||||
/// save, these legacy rows are converted here. A download failure leaves the row untouched and logs
|
||||
/// a warning naming the URL — re-saving the channel fixes it. Idempotent: a converted row's Path is
|
||||
/// a bare cache name, so a second run selects nothing.
|
||||
/// </summary>
|
||||
public class ExternalLogoMigratorService(
|
||||
IServiceScopeFactory serviceScopeFactory,
|
||||
ILogger<ExternalLogoMigratorService> logger,
|
||||
SystemStartup systemStartup)
|
||||
: BackgroundService
|
||||
{
|
||||
protected override async Task ExecuteAsync(CancellationToken stoppingToken)
|
||||
{
|
||||
await Task.Yield();
|
||||
|
||||
await systemStartup.WaitForDatabase(stoppingToken);
|
||||
if (stoppingToken.IsCancellationRequested)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
logger.LogInformation("Migrating external URL channel logos to the image cache");
|
||||
|
||||
try
|
||||
{
|
||||
using IServiceScope scope = serviceScopeFactory.CreateScope();
|
||||
await using TvContext dbContext = scope.ServiceProvider.GetRequiredService<TvContext>();
|
||||
IRemoteLogoCacher cacher = scope.ServiceProvider.GetRequiredService<IRemoteLogoCacher>();
|
||||
|
||||
await MigrateAsync(dbContext, cacher, logger, stoppingToken);
|
||||
|
||||
logger.LogInformation("Done migrating external URL channel logos to the image cache");
|
||||
}
|
||||
catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested)
|
||||
{
|
||||
// shutdown mid-migration — the single trailing SaveChangesAsync never ran, so no partial
|
||||
// persist; the next boot retries idempotently.
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// this is a run-once BackgroundService: an escaping exception trips the default
|
||||
// StopHost behavior and kills the app. A logo migration must never do that — the fetch
|
||||
// races (e.g. a channel deleted mid-run -> DbUpdateConcurrencyException) are transient
|
||||
// and self-heal on the next boot. Log and let the host keep serving.
|
||||
logger.LogError(ex, "Failed migrating external URL channel logos to the image cache; will retry next start");
|
||||
}
|
||||
}
|
||||
|
||||
internal static async Task MigrateAsync(
|
||||
TvContext db,
|
||||
IRemoteLogoCacher cacher,
|
||||
ILogger logger,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
// IsExternalUrl is a C# predicate EF cannot translate, so load logo artwork then filter in memory.
|
||||
List<Artwork> logos = await db.Artwork
|
||||
.Where(a => a.ArtworkKind == ArtworkKind.Logo)
|
||||
.ToListAsync(cancellationToken);
|
||||
|
||||
foreach (Artwork artwork in logos.Where(a => a.IsExternalUrl()))
|
||||
{
|
||||
string oldUrl = artwork.Path;
|
||||
Either<BaseError, string> result = await cacher.CacheFromUrl(new Uri(oldUrl), cancellationToken);
|
||||
result.Match(
|
||||
name =>
|
||||
{
|
||||
artwork.Path = name;
|
||||
artwork.DateUpdated = DateTime.UtcNow;
|
||||
},
|
||||
error => logger.LogWarning(
|
||||
"Could not download existing channel logo {Url}; leaving it. Re-save the channel to fix. ({Error})",
|
||||
oldUrl,
|
||||
error.Value));
|
||||
}
|
||||
|
||||
await db.SaveChangesAsync(cancellationToken);
|
||||
}
|
||||
}
|
||||
@@ -1101,6 +1101,8 @@ public class Startup
|
||||
services.AddScoped<IExternalJsonPlayoutItemProvider, ExternalJsonPlayoutItemProvider>();
|
||||
services.AddScoped<IRemoteStreamProber, HttpRemoteStreamProber>();
|
||||
services.AddScoped<IRemoteImageFetcher, HttpRemoteImageFetcher>();
|
||||
services.AddScoped<IRemoteImageValidator, RemoteImageValidator>();
|
||||
services.AddScoped<IRemoteLogoCacher, RemoteLogoCacher>();
|
||||
services.AddScoped<IPlayoutBuilder, PlayoutBuilder>();
|
||||
services.AddScoped<IBlockPlayoutBuilder, BlockPlayoutBuilder>();
|
||||
services.AddScoped<IBlockPlayoutPreviewBuilder, BlockPlayoutPreviewBuilder>();
|
||||
@@ -1175,6 +1177,10 @@ public class Startup
|
||||
// BackgroundService, so registration order alone does not guarantee the schema exists.
|
||||
services.AddHostedService<LocalAdminSeedService>();
|
||||
services.AddHostedService<DatabaseCleanerService>();
|
||||
// One-time migration of existing external-URL channel logos into the image cache (ersatztv#525).
|
||||
// It awaits SystemStartup.WaitForDatabase itself, so the schema is guaranteed; registration
|
||||
// order relative to the other hosted services is not load-bearing (it touches only logo artwork).
|
||||
services.AddHostedService<ExternalLogoMigratorService>();
|
||||
services.AddHostedService<LoadLoggingLevelService>();
|
||||
services.AddHostedService<CacheCleanerService>();
|
||||
services.AddHostedService<ResourceExtractorService>();
|
||||
|
||||
+20
-6
@@ -282,12 +282,26 @@ mappers) for new API DTOs — those still return the old Blazor-convention relat
|
||||
the domain/VM directly and root the path yourself, following the PR #181 pattern.
|
||||
|
||||
Channel **logos** live under a different route than posters/thumbnails: an uploaded logo roots to
|
||||
`/iptv/logos/{file}` (served by `IptvController`), and an external logo is an absolute URL passed
|
||||
through unchanged. Browse-surface DTOs (`ChannelResponseModel` list, `ChannelGuideChannelResponseModel`
|
||||
guide) get this rooted `Logo` URL from the single `Channels.Mapper.GetLogoUrl` helper (#464), which
|
||||
returns `null` when the channel has no logo so the SPA falls back to its generated initials icon. The
|
||||
raw un-rooted `{path, contentType}` form is still used only by the channel **editor** DTO
|
||||
(`ChannelDetailResponseModel.Logo`), which round-trips it back on save.
|
||||
`/iptv/logos/{file}` (served by `IptvController`). An **external logo URL is no longer stored as a URL** —
|
||||
since #525, `PUT`/`POST /api/v1/channels…` downloads it, decode-validates it, and caches it at save
|
||||
time, so `Artwork.Path` holds a content-hash name and the browse/guide DTOs emit an `/iptv/logos/…`
|
||||
URL exactly as for an uploaded logo. Browse-surface DTOs (`ChannelResponseModel` list,
|
||||
`ChannelGuideChannelResponseModel` guide) get this rooted `Logo` URL from the single
|
||||
`Channels.Mapper.GetLogoUrl` helper (#464), which returns `null` when the channel has no logo so the
|
||||
SPA falls back to its generated initials icon. The raw un-rooted `{path, contentType}` form is still
|
||||
used only by the channel **editor** DTO (`ChannelDetailResponseModel.Logo`), which round-trips it back
|
||||
on save.
|
||||
|
||||
**New logo-download rejections (#525).** `PUT /api/v1/channels/{id}`, the two channel-create
|
||||
endpoints, and `POST /api/v1/artwork/uploads` now reject a logo that cannot be used. The failure is a
|
||||
`BaseError`, so it surfaces as this API's standard **422 `ValidationProblemDetails`** (via
|
||||
`ToErrorResult()`), **not** a 400 — a 400 here still means model-binding/validation-attribute failure.
|
||||
Rejected cases: an external URL that is unreachable, times out (>10s), is oversized (>10 MiB), is not
|
||||
an image, or is a decode bomb (over 50 MP total pixels or 600 frames); an upload gets the same
|
||||
decode-budget check. The `detail` names the reason (e.g. *"Could not download logo from … : Connection
|
||||
refused"*, *"Remote image … returned content type 'text/html'"*, *"Image cannot be used: … pixel
|
||||
limit"*). Response **shapes are unchanged** — only the error set — so the OpenAPI models did not change.
|
||||
(Verified by local live-E2E: good URL → cached `/iptv/logos/<hash>`; unreachable/non-image → 422.)
|
||||
|
||||
`GET /api/v1/watermarks` returns picker-grade rows that carry `imageSource` alongside `id`/`name`
|
||||
(#67), so a client can find the seeded logo-driven `Channel Bug` preset without matching its
|
||||
|
||||
+13
-4
@@ -164,10 +164,19 @@ the system channel templates it creates, so the library-to-lineup builder (which
|
||||
are left alone, so builder-created and auto-tuned channels there inherit whatever the template
|
||||
already specifies.
|
||||
|
||||
**Limitation:** a logo set via **External logo URL** cannot drive the bug. `WatermarkSelector`
|
||||
resolves it to the URL and then `File.Exists`-checks it, which is never true, so the watermark is
|
||||
silently dropped — the URL wins for the guide listing but disables the on-screen bug. Tracked as
|
||||
**#502**; the editor does not offer a bug preview in that case.
|
||||
**An external logo URL drives the bug too — it is downloaded and cached at save time (#525).** When
|
||||
you save a channel whose logo is an **External logo URL**, the URL is fetched, decode-validated, and
|
||||
stored in the image cache under a content-hash name — after which it is byte-identical to an
|
||||
uploaded logo. So `Artwork.Path` never holds a URL: the on-screen bug renders, the editor previews
|
||||
it, and M3U/XMLTV emit the cached `/iptv/logos/…` URL like any uploaded logo. A URL that is dead,
|
||||
slow (>10s), oversized (>10 MiB), a non-image, or a decode bomb (over 50 MP total or 600 frames)
|
||||
**fails the save with a specific 422** in the editor — you see it immediately, rather than a silent
|
||||
render-time drop at 3am (the pre-#502/#511 behavior). To change the remote image, re-enter the URL;
|
||||
there is no refresh button by design. Existing channels that still hold a raw URL are converted by a
|
||||
one-time startup migration; one that fails to download is left alone (a warning names it) and renders
|
||||
with no bug until you re-save it. Historical context (the old `File.Exists`-on-a-URL drop, and the
|
||||
bounded render-time fetch that preceded caching) is in `docs/decisions.md` under
|
||||
`graphics.channel-logo-caching`, #502 and #511.
|
||||
|
||||
Note: `ChannelLogoGenerator.GenerateChannelLogoUrl()` hardcodes `localhost` for watermark logo
|
||||
fetching — see issue #1 for details.
|
||||
|
||||
@@ -130,6 +130,7 @@ in-file entries.
|
||||
- [2026-07-20 — Remote graphics-engine images are fetched through a bounded, pooled `IRemoteImageFetcher`; re-fetched per element init, not cached (#511)](#2026-07-20--remote-graphics-engine-images-are-fetched-through-a-bounded-pooled-iremoteimagefetcher-re-fetched-per-element-init-not-cached-511)
|
||||
- [2026-07-21 — Decision records carry a lifecycle schema, validated by a script; append-only-by-diff is retired (#521)](#2026-07-21--decision-records-carry-a-lifecycle-schema-validated-by-a-script-append-only-by-diff-is-retired-521)
|
||||
- [2026-07-21 — Parallel orientation + selection is the startup protocol; #237 retired (#520)](#2026-07-21--parallel-orientation--selection-is-the-startup-protocol-237-retired-520)
|
||||
- [2026-07-21 — External channel-logo URLs are downloaded and cached at save time; the render path never fetches a logo (#525)](#2026-07-21--external-channel-logo-urls-are-downloaded-and-cached-at-save-time-the-render-path-never-fetches-a-logo-525)
|
||||
|
||||
---
|
||||
|
||||
@@ -3152,3 +3153,57 @@ Protocol and the kickoff's "Closing record" section.
|
||||
for #237-as-live-state phrasings ("read #237", "tracker #237", "queue state lives in", …), exempting
|
||||
lines/sections that are explicitly archival. It is wired into the `decisions lifecycle` CI job so a
|
||||
future PR cannot silently reintroduce the reversed rule.
|
||||
## 2026-07-21 — External channel-logo URLs are downloaded and cached at save time; the render path never fetches a logo (#525)
|
||||
`key: graphics.channel-logo-caching` · `status: active` · `since: 2026-07-21` · `supersedes: none` · `superseded-by: none`
|
||||
**Rule:** An external `http(s)` channel-logo URL is fetched, decode-budget-validated, and stored in the image cache under a content-hash name at SAVE time — becoming byte-identical to an uploaded logo — so the render path never fetches a logo over HTTP; a bad URL fails the save with a 422 (BaseError → ValidationProblemDetails).
|
||||
**Signals:** channel logo url · watermark on-screen bug · paths: `ErsatzTV.Infrastructure/Images/RemoteLogoCacher.cs`, `ErsatzTV.Core/Images/RemoteImageDecodeBudget.cs`, `ErsatzTV.Application/Channels/Commands/*ChannelHandler*.cs`, `ErsatzTV/Services/RunOnce/ExternalLogoMigratorService.cs` · issues: #525, #511, #502
|
||||
**Mechanics:** `docs/channels.md` → Channel logo & on-screen bug; `docs/api-conventions.md` → error mapping
|
||||
|
||||
`ImageElementBase.LoadImage` used to fetch an external-URL logo over HTTP *inside stream startup*,
|
||||
once per playout item. #511 bounded that fetch (timeout, wire cap, redirect cap, decode budgets) but
|
||||
left it in the render path, where a dead/slow/oversized/non-image URL surfaces only as a render-time
|
||||
log line — invisible to the operator who typed it, and re-paid every playout-item transition.
|
||||
|
||||
**A URL is now an input method, not a storage format.** On save, `IRemoteLogoCacher` fetches the URL
|
||||
(reusing #511's hardened `IRemoteImageFetcher`), validates it against the shared
|
||||
`RemoteImageDecodeBudget` (via `IRemoteImageValidator`), and writes the bytes through
|
||||
`IImageCache.SaveArtworkToCache`, storing the returned MD5 content-hash name in `Artwork.Path`. After
|
||||
a successful save the logo is indistinguishable from an uploaded one, so **every downstream consumer
|
||||
is unchanged** — M3U, XMLTV and the SPA mapper all resolve `Artwork.Path` to an `/iptv/logos/…` URL,
|
||||
and the render path finds a local cached file. To refresh a changed remote image the operator
|
||||
re-enters the URL; there is deliberately no refresh button and no staleness/ETag tracking (the
|
||||
content hash makes a re-add of unchanged bytes a natural no-op and of changed bytes a natural new
|
||||
name).
|
||||
|
||||
**Content-hash name, not a GUID.** `SaveArtworkToCache` already returns an opaque MD5-of-bytes name
|
||||
identical to the upload path, so there is one cache convention rather than two — and dedup + change
|
||||
detection fall out for free. A GUID would deviate for no benefit.
|
||||
|
||||
**The decode budget now guards uploads too.** The byte/wire cap does not bound decoding, so the same
|
||||
`RemoteImageDecodeBudget` (product of `width × height × frames ≤ 50 MP`, `≤ 600` frames, enforced on
|
||||
the decoder via `DecoderOptions.MaxFrames` and re-verified against the decoded image — header frame
|
||||
counts lie, see #511) is applied at BOTH the URL-download path and `UploadArtworkHandler`. One rule:
|
||||
anything entering the logo cache is budget-checked, however it arrived. This closes a pre-existing
|
||||
gap that this feature would otherwise have widened (a URL logo becoming an unchecked upload).
|
||||
|
||||
**Narrows `ffmpeg.remote-image-fetcher-bounded` (#511) and `ffmpeg.external-logo-graphics-engine`
|
||||
(#502), does not reverse either.** #511's `IRemoteImageFetcher` bounded-fetch primitive and its "not
|
||||
cached, re-fetched per element init" statement REMAIN active for operator-authored YAML `image:`
|
||||
graphics elements, which still legitimately fetch a URL at render time — only channel logos moved to
|
||||
save-time caching. #502's "external artwork passes through, it is not downloaded into the image
|
||||
cache" still describes the CLIENT-facing consumers (M3U/XMLTV/SPA emit whatever `Artwork.Path`
|
||||
resolves to) — now a cache URL rather than the raw external URL, because the row no longer holds a
|
||||
URL. So neither predecessor is superseded (both stay `active`); this is a new decision layered on
|
||||
top, hence `supersedes: none` — not a keyed supersession.
|
||||
|
||||
**Existing rows migrate at startup, fail-open.** `ExternalLogoMigratorService` (a run-once
|
||||
`BackgroundService`, after the schema migrator + DB cleaner) downloads existing URL logo rows into
|
||||
the cache; a row whose download fails is left exactly as-is with a warning naming it, and
|
||||
`WatermarkSelector.ChannelLogoWatermarkOptions` degrades such a leftover URL to "no on-screen bug"
|
||||
(a warning, never a render-time fetch). The migration is idempotent by construction — a converted
|
||||
row's path is no longer a URL, so a second pass selects it out — and all-or-nothing on cancel (a
|
||||
single trailing `SaveChangesAsync`).
|
||||
|
||||
**Accepted residual:** the SPA can render a not-yet-migrated external-URL logo as an `<img>` preview
|
||||
that looks working while the server-side bug won't resolve until the row is re-saved/migrated — a
|
||||
narrow transitional-state cosmetic mismatch, since the startup migration eagerly converts old rows.
|
||||
|
||||
@@ -51,6 +51,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `ffmpeg.hls-cold-start-burst` | HLS cold-start latency is fixed with a bounded `-readrate_initial_burst` (gated on FFmpeg ≥6.1 capability detection), not by raising `work_ahead_limit`, which would remove the concurrency guarantee it exists for. | 2026-07-20 | [link](../decisions.md#2026-07-20--hls-cold-start-is-fixed-with--readrate_initial_burst-not-by-raising-the-work-ahead-limit-350) |
|
||||
| `ffmpeg.qsv-decode-encode-split` | QSV decode is decoupled from QSV encode via a single `FFmpegProfile.QsvPreferNativeDecoder` bool (default ON, Linux-only), so a QSV encode profile can decode with the more tolerant native VA-API decoder instead of the QSV decoder, mirroring Jellyfin's hybrid decode/encode toggle instead of a general decode-family enum. | 2026-07-20 | [link](../decisions.md#2026-07-20-498--qsv-decode-is-split-from-qsv-encode-via-a-single-qsvprefernativedecoder-bool) |
|
||||
| `ffmpeg.remote-image-fetcher-bounded` | remote graphics-engine images are fetched through `IRemoteImageFetcher` with a pooled `HttpClientFactory` client, a body-covering deadline, a wire-transfer size cap, and a decoder-enforced `DecoderOptions.MaxFrames` bound re-verified post-decode — never cached, re-fetched per element init. | 2026-07-20 | [link](../decisions.md#2026-07-20--remote-graphics-engine-images-are-fetched-through-a-bounded-pooled-iremoteimagefetcher-re-fetched-per-element-init-not-cached-511) |
|
||||
| `graphics.channel-logo-caching` | An external `http(s)` channel-logo URL is fetched, decode-budget-validated, and stored in the image cache under a content-hash name at SAVE time — becoming byte-identical to an uploaded logo — so the render path never fetches a logo over HTTP; a bad URL fails the save with a 422 (BaseError → ValidationProblemDetails). | 2026-07-21 | [link](../decisions.md#2026-07-21--external-channel-logo-urls-are-downloaded-and-cached-at-save-time-the-render-path-never-fetches-a-logo-525) |
|
||||
| `iptv.base-url` | An optional advertised base URL (`iptv.base_url`) is resolved centrally via a pure Core helper (`AdvertisedBaseUrl`) inside the two IPTV generation handlers (M3U + XMLTV); unset/malformed values fall back byte-identical to the request-derived host, and it's a new `iptv` settings group distinct from `ETV_BASE_URL` and out of scope for HDHomeRun. | 2026-07-16 | [link](../decisions.md#2026-07-16--optional-advertised-iptv-base-url-iptvbase_url-resolved-centrally-in-the-two-generators-340) |
|
||||
| `iptv.logo-drives-bug-preset` | One uploaded channel logo drives both the listing logo and the on-screen bug via a shared, seeded `ChannelLogo`-sourced watermark preset (`Channel Bug`), not new per-channel schema. | 2026-07-20 | [link](../decisions.md#2026-07-20--one-logo-drives-the-bug-via-a-shared-channellogo-preset-not-new-schema-67) |
|
||||
| `locking.entitylocker-atomic-flags` | `EntityLocker` uses `Interlocked.CompareExchange`-guarded atomic flags plus a documented single-owner-release discipline (no owner tokens/leases); `Unlock*` on an already-unlocked slot returns `false` and logs a Warning rather than throwing. | 2026-07-11 | [link](../decisions.md#2026-07-11--entitylocker-atomic-flags--single-owner-release-discipline-no-owner-tokens-231) |
|
||||
|
||||
@@ -0,0 +1,899 @@
|
||||
# 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`)
|
||||
|
||||
**RESOLVED (was a VERIFY item): `ErsatzTV.Core` does NOT reference SixLabors.ImageSharp** (it has SkiaSharp only). So the Core interface must NOT return an ImageSharp `Image`. Final shape:
|
||||
- Core interface `IRemoteImageValidator.Validate(Stream, Uri, CancellationToken) : Task` — throws on an invalid image (budget violation or corrupt stream), completes on valid. No ImageSharp type crosses Core. This is all the save/upload paths need.
|
||||
- The render path keeps returning the decoded `Image`, but via a **static** method on the Infra `RemoteImageValidator` (`ImageElementBase` calls it directly — no interface, no DI threading through `GraphicsEngine`). This is a simplification vs. the original draft: no new constructor param on the graphics elements.
|
||||
|
||||
**Interfaces:**
|
||||
- Produces:
|
||||
- Core: `IRemoteImageValidator.Validate(Stream stream, Uri uri, CancellationToken) : Task` (throws `InvalidOperationException` on a budget violation / ImageSharp exception on a corrupt stream; returns on success)
|
||||
- Infra static: `RemoteImageValidator.DecodeAndValidate(Stream stream, Uri uri, CancellationToken) : Task<Image>` (SixLabors `Image`; same throws; caller owns + disposes the returned `Image`) — used by `ImageElementBase` and internally by `Validate`
|
||||
- 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");
|
||||
|
||||
// decode cases exercise the static method (used by the render path)
|
||||
[Test]
|
||||
public async Task Should_Decode_A_Normal_Image()
|
||||
{
|
||||
await using MemoryStream stream = await RealPng(64, 32);
|
||||
using Image image = await RemoteImageValidator.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>(
|
||||
() => RemoteImageValidator.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, RemoteImageDecodeBudget.MaxRemoteFrames + 100);
|
||||
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
|
||||
() => RemoteImageValidator.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 RemoteImageValidator.DecodeAndValidate(stream, Uri, CancellationToken.None);
|
||||
image.Frames.Count.ShouldBe(60);
|
||||
}
|
||||
|
||||
// the Core interface Validate() is the save/upload contract: throws on invalid, returns on valid,
|
||||
// never surfaces an ImageSharp type
|
||||
[Test]
|
||||
public async Task Validate_Returns_On_A_Good_Image()
|
||||
{
|
||||
IRemoteImageValidator validator = new RemoteImageValidator();
|
||||
await using MemoryStream stream = await RealPng(64, 32);
|
||||
await Should.NotThrowAsync(() => validator.Validate(stream, Uri, CancellationToken.None));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Validate_Throws_On_A_Bomb()
|
||||
{
|
||||
IRemoteImageValidator validator = new RemoteImageValidator();
|
||||
await using MemoryStream stream = PngHeaderDeclaring(30000, 30000);
|
||||
await Should.ThrowAsync<InvalidOperationException>(
|
||||
() => validator.Validate(stream, Uri, CancellationToken.None));
|
||||
}
|
||||
|
||||
// (move RealPng / PngHeaderDeclaring / Apng / Crc32 helpers here verbatim from RemoteImageDecodeLimitTests)
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **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` (NO ImageSharp — Core does not reference it):
|
||||
|
||||
```csharp
|
||||
namespace ErsatzTV.Core.Interfaces.Images;
|
||||
|
||||
/// <summary>
|
||||
/// Validates that a stream is a decodable image within the decode budget, throwing if not.
|
||||
/// Used by the logo save path and the artwork upload path (neither needs the decoded pixels,
|
||||
/// only "is this safe to cache"). The graphics engine uses the static
|
||||
/// RemoteImageValidator.DecodeAndValidate instead, which returns the Image it composites.
|
||||
/// (ersatztv#525)
|
||||
/// </summary>
|
||||
public interface IRemoteImageValidator
|
||||
{
|
||||
Task Validate(Stream stream, Uri uri, CancellationToken cancellationToken);
|
||||
}
|
||||
```
|
||||
|
||||
`ErsatzTV.Infrastructure/Images/RemoteImageValidator.cs` — move the body of `ImageElementBase.DecodeRemoteImage` here verbatim into the **static** `DecodeAndValidate` (the `!CanSeek` guard, the `MaxFrames = 1` Identify workaround, `RemoteImageDecodeBudget.*` calls, the `MaxFrames = affordable + 2` decode, the post-decode re-verify + dispose-on-throw). `Validate` wraps it and disposes:
|
||||
|
||||
```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 Validate(Stream stream, Uri uri, CancellationToken cancellationToken)
|
||||
{
|
||||
using Image _ = await DecodeAndValidate(stream, uri, cancellationToken);
|
||||
}
|
||||
|
||||
public static 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>
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Delegate `DecodeRemoteImage` to the static method**
|
||||
|
||||
`ImageElementBase.DecodeRemoteImage` body becomes `return await RemoteImageValidator.DecodeAndValidate(stream, uri, cancellationToken);` (add `using ErsatzTV.Infrastructure.Images;`). **No** constructor change, **no** `GraphicsEngine` threading — the render path calls the static method directly (as it already calls `Image.LoadAsync` statically today). The existing `RemoteImageDecodeLimitTests` decode tests either move to `RemoteImageValidatorTests` (Step 1) or keep calling `ImageElementBase.DecodeRemoteImage` (which now delegates) — either is fine; do not duplicate. Register the interface for the save/upload paths 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.Validate(png, Uri, Arg.Any<CancellationToken>()).Returns(Task.CompletedTask);
|
||||
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.Validate(png, Uri, Arg.Any<CancellationToken>())
|
||||
.Returns<Task>(_ => 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;
|
||||
|
||||
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 (throws if unsafe); we cache the raw bytes
|
||||
await validator.Validate(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.
|
||||
|
||||
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
|
||||
{
|
||||
await validator.Validate(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 text only.)
|
||||
|
||||
- [ ] **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; the Core `IRemoteImageValidator.Validate → Task` (Task 2, throws-on-invalid, no ImageSharp type) is what Tasks 3/6 consume; the Infra static `RemoteImageValidator.DecodeAndValidate → Task<Image>` (Task 2) is what `ImageElementBase` delegates to; `RemoteImageDecodeBudget` static members (Task 1) are consumed by Task 2. Names match across tasks. **Layering note resolved:** Core does not reference ImageSharp, so the Core interface returns `Task`, not `Image`.
|
||||
@@ -0,0 +1,220 @@
|
||||
# External channel-logo URLs become download-on-save
|
||||
|
||||
**Date:** 2026-07-21
|
||||
**Status:** design, awaiting approval
|
||||
**Relates to:** #502 (external URL logos reach the graphics engine), #511 / PR #518 (bounded
|
||||
render-time fetch), #1 (generated-initials `localhost` URL), #510 (deco path)
|
||||
|
||||
## Problem
|
||||
|
||||
A channel logo set as an **external URL** is stored raw in `Artwork.Path` and passed through to every
|
||||
consumer. The render path therefore has to fetch it over HTTP *during stream startup*, once per
|
||||
playout item, while ffmpeg waits on the pipe. #511 bounded that fetch (10s deadline, 10 MiB wire cap,
|
||||
3 redirects, decode budgets) but did not remove it.
|
||||
|
||||
Bounding the fetch treats the symptom. The fetch itself is the problem:
|
||||
|
||||
- **Failure is invisible and late.** A dead, slow, oversized or non-image URL surfaces as a log line
|
||||
at render time. The operator who typed the URL is long gone.
|
||||
- **No preview.** The editor cannot show the bug for an external URL, so the operator cannot tell
|
||||
whether it will work until a stream runs.
|
||||
- **Repeated work.** The same image is re-fetched on every playout item transition.
|
||||
- **Third-party dependency inside stream startup.** A logo host having a bad day degrades tuning.
|
||||
|
||||
## Goal
|
||||
|
||||
**An external logo URL becomes an input method, not a storage format.** Entering a URL downloads the
|
||||
image once, at save time, into the existing artwork cache — after which it is indistinguishable from
|
||||
an uploaded logo. Nothing downstream knows the logo ever came from a URL.
|
||||
|
||||
To refresh a changed image, the operator re-enters the URL. There is no refresh button and no
|
||||
staleness tracking; that is a deliberate simplification, not an oversight.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- **No refresh button, no TTL, no ETag/Last-Modified tracking.** Re-add the URL.
|
||||
- **No change to `ImageGraphicsElement`** (operator-authored YAML `image:`), which may still point at
|
||||
a URL and still fetches at render time through the hardened `IRemoteImageFetcher`. Removing that is
|
||||
an unrelated feature removal.
|
||||
- **No change to the generated-initials fallback** (#1) or the deco path (#510).
|
||||
- **No new artwork storage mechanism.** Reuses `IImageCache` exactly as the upload path does.
|
||||
|
||||
## Design
|
||||
|
||||
### Save path
|
||||
|
||||
All three handlers that persist a channel logo share one code path today and will share the new one:
|
||||
|
||||
| Handler | Current logo logic |
|
||||
|---|---|
|
||||
| `UpdateChannelHandler.ApplyUpdateRequest` | `UpdateChannelHandler.cs:79-123` |
|
||||
| `CreateChannelHandler` | `CreateChannelHandler.cs:63-65` |
|
||||
| `CreateChannelFromLineupHandler` | `CreateChannelFromLineupHandler.cs:360-362` |
|
||||
|
||||
New behavior when the incoming logo path is an absolute `http(s)` URL:
|
||||
|
||||
1. Fetch it with **`IRemoteImageFetcher`** — the primitive #511 already built and hardened (bounded
|
||||
deadline covering headers and body, 10 MiB wire cap, 3 redirects, content-type check, pooled
|
||||
client).
|
||||
2. **Validate the decode budgets** against the downloaded bytes (see *Decode validation* below).
|
||||
3. `IImageCache.SaveArtworkToCache(stream, ArtworkKind.Logo)` → an opaque content-hash name.
|
||||
4. Store that name in `Artwork.Path`, stamp `DateAdded`/`DateUpdated`, exactly as the upload path does.
|
||||
|
||||
Any failure **rejects the save** with a validation error naming the cause. The channel is not
|
||||
persisted and the field stays editable.
|
||||
|
||||
### Naming: content hash, not GUID
|
||||
|
||||
The request was "a random name/guid". This design uses the **existing content hash** that
|
||||
`SaveArtworkToCache` already returns (MD5 of the bytes, stored as `{hash}` with the file at
|
||||
`{LogoCacheFolder}/{hash[..2]}/{hash}`).
|
||||
|
||||
Rationale — it satisfies the intent (opaque, generated, not the URL) while being *strictly better*
|
||||
than a GUID here:
|
||||
|
||||
- It is byte-for-byte the same mechanism as an uploaded logo, so there is one storage convention
|
||||
rather than two.
|
||||
- Re-adding an **unchanged** URL is a natural no-op (same bytes → same hash → same file).
|
||||
- Re-adding a **changed** URL naturally produces a new name, which is exactly the refresh semantic.
|
||||
|
||||
A GUID would deviate from the established convention for no benefit, which the deviation policy in
|
||||
`docs/contributing.md` §10 asks us not to do.
|
||||
|
||||
### Downstream consumers: no code change
|
||||
|
||||
Because `Artwork.Path` now holds a cache name, every consumer already does the right thing:
|
||||
|
||||
| Consumer | Result |
|
||||
|---|---|
|
||||
| M3U (`ChannelPlaylist.cs:63-70`) | `{scheme}://{host}{baseUrl}/iptv/logos/{hash}.jpg` |
|
||||
| XMLTV (`RefreshChannelListHandler.cs:85-95`, `_channel.sbntxt:29-35`) | `{RequestBase}/iptv/logos/{hash}.jpg` |
|
||||
| SPA/API mapper (`Channels/Mapper.cs:129-166`) | `iptv/logos/{hash}` |
|
||||
| Render (`WatermarkSelector.cs:301-325`) | resolves via `imageCache.GetPathForImage`, existence-checked |
|
||||
|
||||
This is the intended outcome: clients stop depending on the third-party host, and the
|
||||
`IsExternalUrl` branches in those consumers become unreachable *for channel logos*. Those branches
|
||||
are **left in place** — `Artwork` is shared with other artwork kinds and with rows that failed
|
||||
migration.
|
||||
|
||||
### Render path
|
||||
|
||||
`WatermarkSelector.ChannelLogoWatermarkOptions` stops treating a URL as renderable. For a logo path
|
||||
that is still a URL (only possible for a row that failed migration), it logs a warning naming the
|
||||
channel and returns `None` — no fetch, no bug, stream unaffected.
|
||||
|
||||
`ImageElementBase.LoadImage` keeps its remote branch for `ImageGraphicsElement`. #511's fetch and
|
||||
decode budgets stay exactly as merged.
|
||||
|
||||
### Decode validation (important)
|
||||
|
||||
Today `ImageElementBase` exempts **local** images from the decode budgets, on the reasoning that a
|
||||
local file is something an operator put on disk rather than bytes an arbitrary host returned. This
|
||||
design invalidates that reasoning for logos: a downloaded URL *becomes* a local file, so without a
|
||||
check at save time the decode bomb simply relocates from the render path to the cache.
|
||||
|
||||
Therefore the save path must validate before caching:
|
||||
|
||||
- Reuse #511's budgets — dimensions, `width × height × frames ≤ 50 MP`, `≤ 600` frames — enforced the
|
||||
same way (`DecoderOptions.MaxFrames` + post-decode re-verification against the decoded image,
|
||||
because header frame counts lie).
|
||||
- To share them, extract the budget helpers currently on `ImageElementBase`
|
||||
(`EnsureDimensionsAffordable`, `AffordableFrames`, `EnsureDecodeAffordable`) into a single reusable
|
||||
component. Proposed: `ErsatzTV.Core/Images/RemoteImageDecodeBudget.cs`, with `ImageElementBase` and
|
||||
the save path both calling it. The retention budget (`EnsureScaledFramesAffordable`) stays in
|
||||
`ImageElementBase` — it depends on render-time scale and has no meaning at save time.
|
||||
|
||||
**Uploads are budget-checked too (decided: fold in).** Direct **uploads** (`UploadArtworkHandler`)
|
||||
are not budget-checked today. Once URL logos become uploads, they inherit that gap on any subsequent
|
||||
re-upload — an inconsistency this change would *create* (same bytes, same cache, enforcement
|
||||
depending only on arrival path). The `RemoteImageDecodeBudget` component is being built regardless, so
|
||||
`UploadArtworkHandler` calls it too. One consistent rule: **anything entering the logo cache is
|
||||
budget-checked, however it arrived.** A budget failure returns a `400` from the upload endpoint the
|
||||
same way it does from the channel save. (Risk is admin-only, like #511's SSRF stance, but the failure
|
||||
mode — cache succeeds, render OOMs concurrent streams later — is exactly the fail-late pattern this
|
||||
redesign exists to kill, so it is closed here rather than deferred.)
|
||||
|
||||
### Migration of existing rows
|
||||
|
||||
A one-time migration walks `Artwork` rows whose `Path` is an absolute `http(s)` URL and whose kind is
|
||||
`Logo`:
|
||||
|
||||
- fetch through the same hardened fetcher → validate → `SaveArtworkToCache` → rewrite `Path`, bump
|
||||
`DateUpdated`;
|
||||
- **on failure, leave the row untouched** and log a warning naming the channel and the reason, so the
|
||||
operator gets an actionable list rather than silent breakage.
|
||||
|
||||
Run as a **startup task**, not an EF migration: it performs network I/O and must be resilient and
|
||||
restartable, which does not belong in a schema migration (and would have to be written twice for
|
||||
SQLite and MySql). It follows the existing precedent of `LocalFolderScanner.RefreshArtwork`
|
||||
(`LocalFolderScanner.cs:132-200`), which already does fetch → `*ArtworkToCache` → persist.
|
||||
|
||||
Idempotent by construction: after a successful pass the row's `Path` is no longer a URL, so it is not
|
||||
selected again.
|
||||
|
||||
### Editor UX
|
||||
|
||||
- Save is **synchronous**: the `PUT` performs the download and returns `400` with a specific message
|
||||
on failure (e.g. *"Could not download logo: host did not respond within 10s"*, *"Logo is 41 MB;
|
||||
the limit is 10 MB"*, *"URL returned text/html, not an image"*). Worst case latency is the fetch
|
||||
deadline.
|
||||
- On success the response carries a normal cached logo, so **the preview works with no special
|
||||
casing** — the `&& !externalUrlLogo` suppression at `ChannelEditScreen.tsx:817` is deleted, as is
|
||||
the help text claiming external URLs cannot drive the bug.
|
||||
- The external-URL field is an *input*: after a successful save it clears and the uploaded-logo
|
||||
preview shows the cached image. The existing mutual-exclusion logic
|
||||
(`ChannelEditScreen.tsx:119-123`) is simplified accordingly — the two fields can no longer disagree
|
||||
because only one storage form now exists.
|
||||
- Help text states that changing the remote image requires re-entering the URL.
|
||||
|
||||
## Error handling
|
||||
|
||||
| Case | Behavior |
|
||||
|---|---|
|
||||
| Host unreachable / times out | Save rejected, message names the timeout |
|
||||
| Non-2xx | Save rejected, message names the status |
|
||||
| Not an image content type | Save rejected |
|
||||
| Over the wire cap | Save rejected, message names actual vs limit |
|
||||
| Over a decode budget | Save rejected, message names dimensions/frames vs limit |
|
||||
| Cache write fails | Save rejected, `BaseError` surfaced |
|
||||
| Migration failure | Row untouched, warning logged, channel keeps rendering without a bug |
|
||||
|
||||
## Testing
|
||||
|
||||
- **Handler tests** (`ErsatzTV.Tests`): URL → fetch → cache → `Artwork.Path` is the hash; each failure
|
||||
mode rejects the save and persists nothing; a non-URL path is unchanged; re-adding identical bytes
|
||||
is a no-op.
|
||||
- **Decode budget tests**: move/extend the existing `RemoteImageDecodeLimitTests`, keeping the APNG
|
||||
regression coverage (a default `Identify` throws on most APNGs; header frame counts lie).
|
||||
- **Migration tests**: URL row is converted; failing row is left intact and warned about; a second run
|
||||
is a no-op.
|
||||
- **`WatermarkSelector` tests**: extend `WatermarkSelectorChannelLogoTests` — a cached path renders; a
|
||||
leftover URL path returns `None` with a warning and never fetches.
|
||||
- **SPA tests**: `ChannelEditScreen.test.tsx` — preview renders after a URL save; the removed
|
||||
suppression is not reintroduced; error surfaces inline on a rejected save.
|
||||
- **Golden nets**: `ChannelPlaylistGoldenTests` / `ChannelGuideGoldenTests` should be *unchanged* for
|
||||
uploaded logos, and a channel whose logo came from a URL should now emit an `/iptv/logos/` URL.
|
||||
|
||||
## Docs to update in the same PR
|
||||
|
||||
- `docs/decisions.md` — new entry; explicitly supersedes the "not cached, re-fetched per element
|
||||
init" paragraph of the #511 entry and narrows #502's "external artwork passes through" to the
|
||||
client-facing consumers it still describes.
|
||||
- `docs/channels.md` — replace the stale limitation text (this supersedes PR #522, which should be
|
||||
closed unmerged).
|
||||
- `docs/api-conventions.md` — `PUT /api/v1/channels/{id}` can now fail on logo download; note the new
|
||||
400 cases. Regenerate `v1.json` + `endpoint-index.md` if any response shape changes.
|
||||
|
||||
## Out of scope / follow-ups
|
||||
|
||||
- `ArtworkController.RedirectArtwork` (`ArtworkController.cs:37-57`) builds `"/iptv/logos/" + Path`
|
||||
unconditionally, producing a malformed redirect when `Path` is a URL. Pre-existing, unrelated to
|
||||
this change, and largely mooted by it for logos — **file separately**.
|
||||
- #1 (generated-initials `localhost`) and #510 (deco path) remain untouched.
|
||||
|
||||
## Resolved decisions
|
||||
|
||||
1. **`UploadArtworkHandler` decode validation is folded into this PR**, not deferred — see *Decode
|
||||
validation*. The component exists either way and the inconsistency is created by this change.
|
||||
2. **`IRemoteImageFetcher` stays in `Core/Interfaces/Streaming/`.** It is still used by the streaming
|
||||
path (YAML image elements), and a namespace move is churn against `git blame` for weak
|
||||
naming-accuracy benefit. Trivial standalone rename if ever wanted.
|
||||
@@ -552,7 +552,7 @@ describe('ChannelEditScreen', () => {
|
||||
expect(preview.style.top).toBe('');
|
||||
});
|
||||
|
||||
it('does not render the preview when an external logo URL is set', async () => {
|
||||
it('does not offer the stale "external URL cannot drive the bug" copy', async () => {
|
||||
mockApi({
|
||||
channelOverrides: {
|
||||
logo: { path: 'iptv/logos/cartoons.png', contentType: 'image/png' },
|
||||
@@ -565,15 +565,116 @@ describe('ChannelEditScreen', () => {
|
||||
await screen.findByDisplayValue('Cartoons');
|
||||
fireEvent.click(screen.getByRole('button', { name: /^Branding/ }));
|
||||
|
||||
// Preview renders from the uploaded logo first.
|
||||
await screen.findByAltText('On-screen bug preview');
|
||||
const urlInput = screen.getByPlaceholderText('https://example.com/logo.png');
|
||||
fireEvent.change(urlInput, { target: { value: 'https://example.com/new-logo.png' } });
|
||||
|
||||
// #525: a saved external URL is downloaded server-side and comes back as a normal cached
|
||||
// logo, so the bug preview works fine for it — the old blanket "cannot be used" claim is gone.
|
||||
expect(screen.queryByText(/cannot be used as the on-screen bug/i)).toBeNull();
|
||||
});
|
||||
|
||||
it('renders the bug preview for a logo that was saved via an external URL', async () => {
|
||||
const puts: unknown[] = [];
|
||||
mockApi({
|
||||
channelOverrides: {
|
||||
logo: { path: '', contentType: '' },
|
||||
watermarkId: 9
|
||||
},
|
||||
watermarks: [{ id: 9, imageSource: 'ChannelLogo', name: 'Channel Bug' }],
|
||||
onPut: (body) => puts.push(body),
|
||||
// The PUT response models what the backend now does (#525): the external URL was
|
||||
// downloaded and cached server-side, so the saved channel comes back with a normal,
|
||||
// non-external cached logo rather than the URL itself.
|
||||
putResponseOverrides: {
|
||||
logo: { path: 'iptv/logos/downloaded.png', contentType: 'image/png', isExternalUrl: false },
|
||||
watermarkId: 9
|
||||
}
|
||||
});
|
||||
render(<ChannelEditScreen />);
|
||||
|
||||
await screen.findByDisplayValue('Cartoons');
|
||||
fireEvent.click(screen.getByRole('button', { name: /^Branding/ }));
|
||||
|
||||
const urlInput = screen.getByPlaceholderText('https://example.com/logo.png');
|
||||
fireEvent.change(urlInput, { target: { value: 'https://example.com/new-logo.png' } });
|
||||
|
||||
// An external URL can never resolve to a real bug (see the comment on the WatermarkSelector
|
||||
// File.Exists behavior), so the preview must disappear once one is set.
|
||||
await waitFor(() => expect(screen.queryByAltText('On-screen bug preview')).toBeNull());
|
||||
const saveButton = await screen.findByRole('button', { name: 'Save changes' });
|
||||
fireEvent.click(saveButton);
|
||||
|
||||
await waitFor(() => expect(puts).toHaveLength(1));
|
||||
expect(await screen.findByText('Channel saved')).toBeInTheDocument();
|
||||
|
||||
const preview = await screen.findByAltText('On-screen bug preview');
|
||||
expect(preview).toHaveAttribute('src', 'iptv/logos/downloaded.png');
|
||||
|
||||
// The URL field is cleared post-save since the channel's logo is no longer external.
|
||||
expect(screen.getByPlaceholderText('https://example.com/logo.png')).toHaveValue('');
|
||||
});
|
||||
|
||||
it('shows a 400 save error inline near the URL field and does not navigate away', async () => {
|
||||
mockApi({
|
||||
channelOverrides: { logo: { path: '', contentType: '' } }
|
||||
});
|
||||
vi.spyOn(window, 'fetch').mockImplementation((input, init) => {
|
||||
const url = typeof input === 'string' ? input : input instanceof URL ? input.toString() : (input as Request).url;
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
|
||||
if (url === '/api/v1/channels/5' && method === 'PUT') {
|
||||
return Promise.resolve(
|
||||
json({ status: 400, title: 'Bad Request', detail: 'Could not download logo: host timed out' }, 400)
|
||||
);
|
||||
}
|
||||
|
||||
if (url === '/api/v1/channels/5') {
|
||||
return Promise.resolve(json({ ...channel, logo: { path: '', contentType: '' } }));
|
||||
}
|
||||
|
||||
if (url === '/api/v1/ffmpeg/profiles') {
|
||||
return Promise.resolve(json([{ id: 1, name: 'Default profile' }]));
|
||||
}
|
||||
|
||||
if (url === '/api/v1/watermarks') {
|
||||
return Promise.resolve(json([{ id: 2, name: 'Corner bug', imageSource: 'Custom' }]));
|
||||
}
|
||||
|
||||
if (url === '/api/v1/filler-presets') {
|
||||
return Promise.resolve(json([{ id: 3, name: 'Bumpers' }]));
|
||||
}
|
||||
|
||||
if (url === '/api/v1/channels') {
|
||||
return Promise.resolve(json([{ id: 5, number: '5', name: 'Cartoons', group: 'ChicoryTV' }]));
|
||||
}
|
||||
|
||||
if (url === '/api/v1/languages') {
|
||||
return Promise.resolve(json([{ code: 'eng', englishName: 'English' }]));
|
||||
}
|
||||
|
||||
if (url === '/api/v1/channels/music-video-credits-templates') {
|
||||
return Promise.resolve(json(['default']));
|
||||
}
|
||||
|
||||
if (url === '/api/v1/channels/stream-selectors') {
|
||||
return Promise.resolve(json(['selector.py']));
|
||||
}
|
||||
|
||||
return Promise.resolve(json({ status: 404, title: 'Not Found' }, 404));
|
||||
});
|
||||
render(<ChannelEditScreen />);
|
||||
|
||||
await screen.findByDisplayValue('Cartoons');
|
||||
fireEvent.click(screen.getByRole('button', { name: /^Branding/ }));
|
||||
|
||||
const urlInput = screen.getByPlaceholderText('https://example.com/logo.png');
|
||||
fireEvent.change(urlInput, { target: { value: 'https://example.com/bad-logo.png' } });
|
||||
|
||||
const saveButton = await screen.findByRole('button', { name: 'Save changes' });
|
||||
fireEvent.click(saveButton);
|
||||
|
||||
expect(await screen.findByText('Could not download logo: host timed out')).toBeInTheDocument();
|
||||
|
||||
// Still on the edit screen, with the failed URL still in the field.
|
||||
expect(screen.getByPlaceholderText('https://example.com/logo.png')).toHaveValue('https://example.com/bad-logo.png');
|
||||
expect(screen.queryByText('Channel saved')).toBeNull();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -679,9 +679,6 @@ function BrandingPane({
|
||||
const referenced = data.watermarks.find((watermark) => watermark.id === draft.watermarkId) ?? null;
|
||||
const logoBugEnabled = referenced?.imageSource === 'ChannelLogo';
|
||||
const logoBugTarget = findLogoBugWatermark(data.watermarks);
|
||||
// An external-URL logo is resolved by WatermarkSelector to the URL itself and then File.Exists-ed,
|
||||
// which is never true, so no bug renders (#502). Don't promise one in the preview.
|
||||
const externalUrlLogo = trimmedUrl.length > 0;
|
||||
|
||||
// Keyed by the watermark id it was fetched for, so a stale response (or a disabled toggle) is
|
||||
// filtered out by comparing against the CURRENT draft.watermarkId at render time — no reset
|
||||
@@ -797,9 +794,7 @@ function BrandingPane({
|
||||
help={
|
||||
logoBugTarget == null
|
||||
? 'No logo-driven watermark preset exists yet.'
|
||||
: externalUrlLogo
|
||||
? 'An external logo URL cannot be used as the on-screen bug — upload an image instead.'
|
||||
: 'Overlays this channel’s own logo on the stream, using the shared preset’s position and size.'
|
||||
: 'Overlays this channel’s own logo on the stream, using the shared preset’s position and size.'
|
||||
}
|
||||
label="Use logo as on-screen bug"
|
||||
>
|
||||
@@ -814,7 +809,7 @@ function BrandingPane({
|
||||
}
|
||||
size="sm"
|
||||
/>
|
||||
{logoBugEnabled && bugGeometry && previewSrc && !externalUrlLogo && (
|
||||
{logoBugEnabled && bugGeometry && previewSrc && (
|
||||
// bugGeometry is cached keyed by `id` (see fetchedGeometry above); pass the
|
||||
// geometry fields explicitly rather than spreading so the cache key never leaks
|
||||
// into BugPreview's props.
|
||||
|
||||
Reference in New Issue
Block a user