timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

fix(568): bound the client-supplied id lists, name their field, and make Kind part of the built-in identity

Browse Source 
Three of the four findings standing on the 2026-09-05 16:24 review verdict, which the
branch had not answered.

The count cap is the blocking one. The three id-list validators took whatever the
request carried, so the only bound on `graphicsElementIds`/`watermarkIds` was the
Kestrel body cap -- a transport limit, not a collection limit. The earlier disposition
deferred it to #917 on the grounds that `ApplyUpdateRequest` reconciles the same list
uncapped anyway; that is true and does not answer the ask, because the reconcile is
downstream of a validator that can refuse the request outright. One shared
`Validators.IdsMustExist` now carries the cap for all three, counted on the RAW list
before `Distinct` (a million copies of one id costs the same to parse and materialize
whatever the distinct count is) and before any database work.

The same helper is where the field name and the diagnostic cap now live. The 422 said
"Graphics element(s) do not exist: 999" without naming which request field carried the
999, and echoed every rejected id -- an oversized request answered with an oversized
response. Both fixed once, in the shared place, so the three sites cannot drift.

`Kind` moves into `GraphicsElementDefaults.IsOnNowNext`. The seeder required
`Kind == Text` and the API's `builtIn` did not, so an Image row at the exact seeded path
was `builtIn:true` on the wire while `GetBuiltInElementId` refused to treat it as the
built-in element -- two sites disagreeing about one row, which is the shape #568 exists
to close. Identity is now one predicate applied whole at both sites; the seeder's SQL
`Kind` filter is gone rather than kept as a duplicate, since a duplicate guard would mask
the predicate's own clause.

Also the fourth finding, the check-then-write race: `RefreshGraphicsElements` can delete a
validated element between `Validate` and `SaveChangesAsync`, handing the join insert the
FK violation the validator exists to prevent. A transaction does not close it -- neither
provider locks rows the validator merely read -- so both handlers catch `DbUpdateException`,
re-ask the existence question on a fresh context, and return the validator's own 422 when
an id has since gone; anything else keeps its own exception. Foreign keys are off in
`InMemoryTvContext`, so the trigger is simulated by an armed save-failure interceptor while
the recovery itself runs against real post-delete state.

Refs #568

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
This commit is contained in:
timothy
2026-09-05 21:35:15 +02:00
co-authored by Claude Fable 5.1
parent 722f0057e8
commit e7f794057d
11 changed files with 546 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs
+62 -25
View File
@@ -47,8 +47,13 @@ public class UpdateChannelHandler(
{
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)),
Right: logoPath =>
ApplyUpdateRequestTranslatingLostRace(
dbContext,
c,
request,
logoPath,
cancellationToken),
Left: e => Task.FromResult(Left<BaseError, ChannelViewModel>(e)));
},
Fail: errors => Task.FromResult(Left<BaseError, ChannelViewModel>(errors.Join())));
@@ -76,6 +81,47 @@ public class UpdateChannelHandler(
return cached;
}
// Validation and the write are two statements, not one atomic step: RefreshGraphicsElements
// deletes elements whose template file is gone, and a delete landing between the two turns the
// join insert back into the FK violation the validator exists to prevent -- the unhandled 500
// again (#568). A transaction does not close that window either: neither provider locks the rows
// the validator merely READ, so the concurrent delete still commits. Ask the existence question
// again on the failure path instead, and return the same 422 the validator would have returned;
// a DbUpdateException from any other cause keeps its own exception rather than being reported as
// a client error.
private async Task<Either<BaseError, ChannelViewModel>> ApplyUpdateRequestTranslatingLostRace(
TvContext dbContext,
Channel channel,
UpdateChannel request,
string logoPath,
CancellationToken cancellationToken)
{
try
{
return Right<BaseError, ChannelViewModel>(
await ApplyUpdateRequest(dbContext, channel, request, logoPath, cancellationToken));
}
catch (DbUpdateException)
{
// a fresh context: the failed save left the original one tracking the changes that
// could not be written, so the same query there could be answered from those.
await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
Validation<BaseError, Unit> recheck =
await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken);
Option<BaseError> maybeError = recheck.Match(
Succ: _ => Option<BaseError>.None,
Fail: errors => Some(errors.Join()));
foreach (BaseError error in maybeError)
{
return Left<BaseError, ChannelViewModel>(error);
}
throw;
}
}
private async Task<ChannelViewModel> ApplyUpdateRequest(
TvContext dbContext,
Channel c,
@@ -300,31 +346,22 @@ public class UpdateChannelHandler(
// id; an id with no matching GraphicsElement row would otherwise hit
// FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId at SaveChangesAsync and surface as
// an unhandled 500 (there is no global exception filter). Reject it here instead, for parity
// with every other FK field on this full-replace DTO (#568).
private static async Task<Validation<BaseError, Unit>> GraphicsElementIdsMustExist(
// with every other FK field on this full-replace DTO (#568). The count cap, the request field
// named in the message and the cap on echoed ids all live in Validators.IdsMustExist, shared
// with the two UpdateDecoHandler twins so the three cannot drift apart.
private static Task<Validation<BaseError, Unit>> GraphicsElementIdsMustExist(
TvContext dbContext,
UpdateChannel request,
CancellationToken cancellationToken)
{
List<int> requested = request.GraphicsElementIds?.Distinct().ToList() ?? [];
if (requested.Count == 0)
{
return Unit.Default;
}
List<int> existingIds = await dbContext.GraphicsElements
.Where(e => requested.Contains(e.Id))
.Select(e => e.Id)
.ToListAsync(cancellationToken);
List<int> missingIds = requested.Except(existingIds).OrderBy(id => id).ToList();
if (missingIds.Count == 0)
{
return Unit.Default;
}
return BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}");
}
CancellationToken cancellationToken) =>
Validators.IdsMustExist(
request,
r => r.GraphicsElementIds,
"Graphics element",
(ids, token) => dbContext.GraphicsElements
.Where(e => ids.Contains(e.Id))
.Select(e => e.Id)
.ToListAsync(token),
cancellationToken);
private static async Task<Validation<BaseError, Unit>> MirrorSourceMustBeValid(
TvContext dbContext,
ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs
+1 -1
View File
@@ -22,7 +22,7 @@ public class GetAllGraphicsElementsForApiHandler(IDbContextFactory<TvContext> db
.Select(e => new
{
Vm = ProjectToViewModel(e),
BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path)
BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path, e.Kind)
})
.OrderBy(x => x.Vm.Name == x.Vm.FileName)
.ThenBy(x => x.Vm.Name)
ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs
+72 -50
View File
@@ -19,7 +19,49 @@ public class UpdateDecoHandler(
{
await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
Validation<BaseError, Deco> validation = await Validate(dbContext, request, cancellationToken);
return await validation.Apply(ps => ApplyUpdateRequest(dbContext, ps, request, cancellationToken));
return await validation.Match(
Succ: deco => ApplyUpdateRequestTranslatingLostRace(dbContext, deco, request, cancellationToken),
Fail: errors => Task.FromResult(Left<BaseError, Unit>(errors.Join())));
}
// Mirrors UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRace (#568): validation and the
// write are two statements, so a concurrent delete of a validated watermark or graphics element
// -- RefreshGraphicsElements deletes elements whose template file is gone -- lands the join
// insert on the FK violation the validators exist to prevent, as an unhandled 500. A transaction
// does not close that window either (neither provider locks the rows the validator merely READ),
// so ask the existence questions again on the failure path and return the same 422; a
// DbUpdateException from any other cause keeps its own exception.
private async Task<Either<BaseError, Unit>> ApplyUpdateRequestTranslatingLostRace(
TvContext dbContext,
Deco existing,
UpdateDeco request,
CancellationToken cancellationToken)
{
try
{
return await ApplyUpdateRequest(dbContext, existing, request, cancellationToken);
}
catch (DbUpdateException)
{
// a fresh context: the failed save left the original one tracking the changes that
// could not be written, so the same query there could be answered out of those.
await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
Validation<BaseError, Unit> recheck = (
await WatermarkIdsMustExist(recheckContext, request, cancellationToken),
await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken))
.Apply((_, _) => Unit.Default);
Option<BaseError> maybeError = recheck.Match(
Succ: _ => Option<BaseError>.None,
Fail: errors => Some(errors.Join()));
foreach (BaseError error in maybeError)
{
return Left<BaseError, Unit>(error);
}
throw;
}
}
private async Task<Unit> ApplyUpdateRequest(
@@ -241,60 +283,40 @@ public class UpdateDecoHandler(
// an id with no matching row hits the FK constraint at SaveChangesAsync and surfaces as an
// unhandled 500 (there is no global exception filter). These are top-level fields on
// ReplaceDecoRequest, the same position as graphicsElementIds on UpdateChannelRequest -- not the
// "deep FK ids nested inside item-list request bodies" carve-out in api-conventions.md.
private static async Task<Validation<BaseError, Unit>> WatermarkIdsMustExist(
// "deep FK ids nested inside item-list request bodies" carve-out in api-conventions.md. Both go
// through Validators.IdsMustExist, the one place the count cap, the request field named in the
// message and the cap on echoed ids are written.
private static Task<Validation<BaseError, Unit>> WatermarkIdsMustExist(
TvContext dbContext,
UpdateDeco request,
CancellationToken cancellationToken)
{
if (!ConsumesWatermarkIds(request))
{
return Unit.Default;
}
CancellationToken cancellationToken) =>
!ConsumesWatermarkIds(request)
? Task.FromResult<Validation<BaseError, Unit>>(Unit.Default)
: Validators.IdsMustExist(
request,
r => r.WatermarkIds,
"Watermark",
(ids, token) => dbContext.ChannelWatermarks
.Where(w => ids.Contains(w.Id))
.Select(w => w.Id)
.ToListAsync(token),
cancellationToken);
List<int> requested = request.WatermarkIds?.Distinct().ToList() ?? [];
if (requested.Count == 0)
{
return Unit.Default;
}
List<int> existingIds = await dbContext.ChannelWatermarks
.Where(w => requested.Contains(w.Id))
.Select(w => w.Id)
.ToListAsync(cancellationToken);
List<int> missingIds = requested.Except(existingIds).OrderBy(id => id).ToList();
return missingIds.Count == 0
? Unit.Default
: BaseError.New($"Watermark(s) do not exist: {string.Join(", ", missingIds)}");
}
private static async Task<Validation<BaseError, Unit>> GraphicsElementIdsMustExist(
private static Task<Validation<BaseError, Unit>> GraphicsElementIdsMustExist(
TvContext dbContext,
UpdateDeco request,
CancellationToken cancellationToken)
{
if (!ConsumesGraphicsElementIds(request))
{
return Unit.Default;
}
List<int> requested = request.GraphicsElementIds?.Distinct().ToList() ?? [];
if (requested.Count == 0)
{
return Unit.Default;
}
List<int> existingIds = await dbContext.GraphicsElements
.Where(e => requested.Contains(e.Id))
.Select(e => e.Id)
.ToListAsync(cancellationToken);
List<int> missingIds = requested.Except(existingIds).OrderBy(id => id).ToList();
return missingIds.Count == 0
? Unit.Default
: BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}");
}
CancellationToken cancellationToken) =>
!ConsumesGraphicsElementIds(request)
? Task.FromResult<Validation<BaseError, Unit>>(Unit.Default)
: Validators.IdsMustExist(
request,
r => r.GraphicsElementIds,
"Graphics element",
(ids, token) => dbContext.GraphicsElements
.Where(e => ids.Contains(e.Id))
.Select(e => e.Id)
.ToListAsync(token),
cancellationToken);
private static Task<Validation<BaseError, Deco>> DecoMustExist(
TvContext dbContext,
ErsatzTV.Application/Validators/IdListValidation.cs
+73
View File
@@ -0,0 +1,73 @@
using System.Linq.Expressions;
using ErsatzTV.Core;
namespace ErsatzTV.Application;
public static partial class Validators
{
/// <summary>
/// The largest id list a full-replace write path accepts in one of its top-level id fields.
/// Deliberately far above any real payload -- the lists it bounds select from tables an
/// operator curates by hand (graphics elements, watermarks), where a few dozen rows is a
/// large install -- so the bound is a ceiling on abuse, not a product limit anyone can reach
/// by using the editor (#568).
/// </summary>
public const int MaximumIdListCount = 512;
// A 422 that echoes every rejected id turns an oversized request into an oversized response.
// Enough ids to fix the payload by hand, then a count.
private const int MaximumReportedMissingIds = 10;
/// <summary>
/// The shared existence check for a top-level list of FK ids on a full-replace request:
/// bound the list, resolve which of its ids exist through <paramref name="findExisting" />,
/// and reject the rest with a 422 that names the request field it came from.
/// </summary>
/// <remarks>
/// The count is taken from the RAW list, before <c>Distinct</c> and before any database
/// work: deduplication is not what the request costs. A million-entry list of one repeated
/// id parses, allocates and materializes in full whatever the distinct count turns out to
/// be, so a cap applied after <c>Distinct</c> would bound the query and leave the request
/// itself unbounded.
/// </remarks>
public static async Task<Validation<BaseError, Unit>> IdsMustExist<T>(
T input,
Expression<Func<T, List<int>>> expression,
string noun,
Func<List<int>, CancellationToken, Task<List<int>>> findExisting,
CancellationToken cancellationToken)
{
string field = GetMemberName(expression);
List<int> submitted = expression.Compile()(input) ?? [];
if (submitted.Count > MaximumIdListCount)
{
return BaseError.New(
$"[{field}] contains {submitted.Count} ids; at most {MaximumIdListCount} are accepted. " +
"The whole list is materialized into one existence query and then reconciled against every " +
"row already attached, so a longer list turns a single request into unbounded work.");
}
List<int> requested = submitted.Distinct().ToList();
if (requested.Count == 0)
{
return Unit.Default;
}
List<int> existingIds = await findExisting(requested, cancellationToken);
List<int> missingIds = requested.Except(existingIds).OrderBy(id => id).ToList();
if (missingIds.Count == 0)
{
return Unit.Default;
}
return BaseError.New($"[{field}] {noun}(s) do not exist: {DescribeIds(missingIds)}");
}
private static string DescribeIds(IReadOnlyList<int> ids) =>
ids.Count <= MaximumReportedMissingIds
? string.Join(", ", ids)
: $"{string.Join(", ", ids.Take(MaximumReportedMissingIds))} (and " +
$"{ids.Count - MaximumReportedMissingIds} more)";
}
ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs
+25 -13
View File
@@ -1,4 +1,5 @@
using System.IO;
using ErsatzTV.Core.Domain;
namespace ErsatzTV.Core.Graphics;
@@ -21,20 +22,31 @@ public static class GraphicsElementDefaults
Path.Combine(FileSystemLayout.GraphicsElementsTextTemplatesFolder, OnNowNextFileName);
/// <summary>
/// The one identity test for the built-in On Now / Next element. Ordinal on purpose, and so
/// case-sensitive on purpose: the built-in element is the exact file the seeder wrote, at
/// the exact path it wrote it to.
/// The one identity test for the built-in On Now / Next element: the exact file the seeder
/// wrote, at the exact path it wrote it to, of the kind it wrote it as. Ordinal on purpose,
/// and so case-sensitive on purpose.
/// </summary>
/// <remarks>
/// Callers compare in memory rather than in a <c>Where</c> clause, because in SQL the answer
/// would be the PROVIDER's to give: <c>GraphicsElement.Path</c> takes no explicit collation
/// (<c>TvContext.OnModelCreating</c> pins one only on the listed name/title columns), so
/// SQLite compares it case-sensitively while MySQL uses the server default, which is
/// normally case-INsensitive. Evaluating one discriminator site in SQL and the other in
/// memory would let the two disagree on MySQL alone. The SQLite test suite cannot tell the
/// two apart -- BINARY collation and an ordinal comparison agree on every input -- so this
/// is held by keeping the comparison out of SQL, not by a test.
/// <para>
/// <c>Kind</c> is part of the identity rather than a second test any caller may add or
/// skip: the seeder's own "does the built-in row exist yet?" check resolves through this
/// predicate, so a row of another kind at the seeded path answering yes would suppress
/// the Text row every consumer resolves. A caller applying only the path half would
/// report that wrong-kind row as the built-in element while the seeder refused to treat
/// it as one -- the two sites disagreeing about the same row, which is the defect this
/// predicate exists to make impossible (#568).
/// </para>
/// <para>
/// Callers compare in memory rather than in a <c>Where</c> clause, because in SQL the
/// answer would be the PROVIDER's to give: <c>GraphicsElement.Path</c> takes no explicit
/// collation (<c>TvContext.OnModelCreating</c> pins one only on the listed name/title
/// columns), so SQLite compares it case-sensitively while MySQL uses the server default,
/// which is normally case-INsensitive. Evaluating one discriminator site in SQL and the
/// other in memory would let the two disagree on MySQL alone. The SQLite test suite
/// cannot tell the two apart -- BINARY collation and an ordinal comparison agree on
/// every input -- so this is held by keeping the comparison out of SQL, not by a test.
/// </para>
/// </remarks>
public static bool IsOnNowNext(string path) =>
string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal);
public static bool IsOnNowNext(string path, GraphicsElementKind kind) =>
kind == GraphicsElementKind.Text && string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal);
}
ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs
+16 -13
View File
@@ -260,29 +260,32 @@ public static class GraphicsElementSeeder
/// Identity is the full seeded path, never the user-editable Name (the #67 lesson carried into
/// #74) and never the bare filename (#568: filename-only matching is folder-agnostic, so a user
/// element named exactly `on-now-next.yml` in a different template folder would also match).
/// The <c>Kind</c> filter is load-bearing rather than decorative: <c>EnsureBuiltInElementRow</c>
/// asks this method whether the row it is about to create already exists, so a row of another
/// kind at the seeded path must NOT answer yes -- it would suppress the Text row every consumer
/// resolves.
/// The <c>Kind</c> half of that identity is load-bearing rather than decorative:
/// <c>EnsureBuiltInElementRow</c> asks this method whether the row it is about to create already
/// exists, so a row of another kind at the seeded path must NOT answer yes -- it would suppress
/// the Text row every consumer resolves.
/// </summary>
/// <remarks>
/// The path comparison is <see cref="GraphicsElementDefaults.IsOnNowNext"/> in memory rather
/// than a <c>Where</c> clause: in SQL the match would be the provider's collation to decide, and
/// this site would then disagree with the API's `builtIn` (which compares in memory) on MySQL.
/// The <c>Kind</c> filter stays in SQL because it is an enum, not a string.
/// Both halves are <see cref="GraphicsElementDefaults.IsOnNowNext(string,GraphicsElementKind)"/>
/// in memory rather than a <c>Where</c> clause. The path half must be, or the match would be the
/// provider's collation to decide and this site would disagree with the API's `builtIn` (which
/// compares in memory) on MySQL. The <c>Kind</c> half could be a SQL filter -- it is an enum,
/// not a string -- but then this site would hold half the identity and the predicate the other
/// half, and the API site could apply the predicate alone and quietly answer for rows this one
/// rejects. That is exactly the disagreement #568 found, so identity is one predicate applied
/// whole, at every site.
/// </remarks>
public static async Task<Option<int>> GetBuiltInElementId(
TvContext context,
CancellationToken cancellationToken)
{
List<(int Id, string Path)> candidates = await context.GraphicsElements
.Where(e => e.Kind == GraphicsElementKind.Text)
.Select(e => new { e.Id, e.Path })
List<(int Id, string Path, GraphicsElementKind Kind)> candidates = await context.GraphicsElements
.Select(e => new { e.Id, e.Path, e.Kind })
.ToListAsync(cancellationToken)
.Map(rows => rows.Select(r => (r.Id, r.Path)).ToList());
.Map(rows => rows.Select(r => (r.Id, r.Path, r.Kind)).ToList());
List<int> matches = candidates
.Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path))
.Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path, c.Kind))
.Select(c => c.Id)
.OrderBy(id => id)
.ToList();
ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs
+151
View File
@@ -1,3 +1,5 @@
using System.Globalization;
using ErsatzTV.Application;
using ErsatzTV.Application.Channels;
using ErsatzTV.Core;
using ErsatzTV.Core.Domain;
@@ -29,6 +31,28 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase
return (elementA.Id, elementB.Id);
}
private async Task<List<int>> SeedGraphicsElements(int count)
{
await using TvContext context = Db.CreateContext();
List<GraphicsElement> elements = Enumerable.Range(0, count)
.Select(i => new GraphicsElement { Path = $"element-{i}.yml" })
.ToList();
context.GraphicsElements.AddRange(elements);
await context.SaveChangesAsync();
return elements.Select(e => e.Id).ToList();
}
// Replaces the harness the base class built with one whose SaveChangesAsync can be made to fail
// on demand. Foreign keys are off in InMemoryTvContext, so the FK violation a concurrent delete
// really produces cannot be raised by seeding alone.
private async Task<ArmedSaveFailureInterceptor> UseFailingSaveHarness()
{
await Db.DisposeAsync();
var interceptor = new ArmedSaveFailureInterceptor();
Db = await InMemoryTvContext.CreateAsync(interceptor);
return interceptor;
}
[Test]
public async Task Should_Reconcile_GraphicsElement_Join_Add_Then_Remove()
{
@@ -95,6 +119,7 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase
BaseError error = LeftOf(result);
error.ShouldNotBeOfType<NotFoundError>();
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain("999");
// no partial write: the channel keeps no graphics element association
@@ -117,6 +142,132 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase
BaseError error = LeftOf(result);
error.ShouldNotBeOfType<NotFoundError>();
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain("12345");
}
// #568: the id list is client-supplied and was bounded only by the Kestrel body cap, which is a
// transport limit and not a collection limit. The cap is Validators.MaximumIdListCount, shared
// by all three id-list validators; these three tests pin its two edges and the shape that makes
// its placement matter. Removing the cap from Validators.IdsMustExist is row 41 of the mutation
// table in docs/graphics-elements.md.
[Test]
public async Task Should_Accept_Exactly_The_Maximum_Number_Of_GraphicsElementIds()
{
await SeedFFmpegProfile();
Channel channel = await SeedChannel(1, "5");
List<int> ids = await SeedGraphicsElements(Validators.MaximumIdListCount);
Either<BaseError, ChannelViewModel> result = await MakeHandler().Handle(
MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids),
CancellationToken.None);
result.IsRight.ShouldBeTrue();
await using TvContext context = Db.CreateContext();
Channel reloaded = await context.Channels.Include(c => c.ChannelGraphicsElements)
.SingleAsync(c => c.Id == channel.Id);
reloaded.ChannelGraphicsElements.Count.ShouldBe(Validators.MaximumIdListCount);
}
[Test]
public async Task Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds()
{
await SeedFFmpegProfile();
Channel channel = await SeedChannel(1, "5");
List<int> ids = Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList();
Either<BaseError, ChannelViewModel> result = await MakeHandler().Handle(
MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids),
CancellationToken.None);
BaseError error = LeftOf(result);
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain((Validators.MaximumIdListCount + 1).ToString(CultureInfo.InvariantCulture));
error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture));
}
// The cap counts the RAW list, before Distinct: every one of these ids exists and they collapse
// to a single distinct id, so a cap applied after deduplication would accept this request and
// leave the parse and materialization it costs unbounded.
[Test]
public async Task Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count()
{
await SeedFFmpegProfile();
Channel channel = await SeedChannel(1, "5");
(int elementAId, _) = await SeedGraphicsElements();
List<int> ids = Enumerable.Repeat(elementAId, Validators.MaximumIdListCount + 1).ToList();
Either<BaseError, ChannelViewModel> result = await MakeHandler().Handle(
MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids),
CancellationToken.None);
BaseError error = LeftOf(result);
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain((Validators.MaximumIdListCount + 1).ToString(CultureInfo.InvariantCulture));
error.Value.ShouldNotContain("do not exist");
}
// A 422 that echoes every rejected id turns an oversized request into an oversized response.
// Removing the truncation from Validators.DescribeIds is row 42 of the mutation table.
[Test]
public async Task Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422()
{
await SeedFFmpegProfile();
Channel channel = await SeedChannel(1, "5");
List<int> ids = Enumerable.Range(1001, 30).ToList();
Either<BaseError, ChannelViewModel> result = await MakeHandler().Handle(
MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids),
CancellationToken.None);
BaseError error = LeftOf(result);
error.Value.ShouldContain("1001");
error.Value.ShouldContain("(and 20 more)");
error.Value.ShouldNotContain("1030");
}
// #568: validation and the write are two statements, so RefreshGraphicsElements can delete a
// validated element in between and hand the join insert the FK violation the validator exists to
// prevent -- the unhandled 500 again. Removing the DbUpdateException catch from
// ApplyUpdateRequestTranslatingLostRace is row 43 of the mutation table.
[Test]
public async Task Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422()
{
ArmedSaveFailureInterceptor interceptor = await UseFailingSaveHarness();
await SeedFFmpegProfile();
Channel channel = await SeedChannel(1, "5");
(int elementAId, _) = await SeedGraphicsElements();
interceptor.SqlBeforeFailing =
$"DELETE FROM GraphicsElement WHERE Id = {elementAId.ToString(CultureInfo.InvariantCulture)}";
interceptor.Armed = true;
Either<BaseError, ChannelViewModel> result = await MakeHandler().Handle(
MakeUpdate(channel.Id, number: "5", graphicsElementIds: [elementAId]),
CancellationToken.None);
BaseError error = LeftOf(result);
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain(elementAId.ToString(CultureInfo.InvariantCulture));
}
// The other half of that catch: a DbUpdateException whose cause is NOT a missing graphics
// element is a real fault and must keep its own exception rather than be reported to the client
// as a validation error about ids that are all still present.
[Test]
public async Task Should_Not_Report_An_Unrelated_DbUpdateException_As_A_Graphics_Element_422()
{
ArmedSaveFailureInterceptor interceptor = await UseFailingSaveHarness();
await SeedFFmpegProfile();
Channel channel = await SeedChannel(1, "5");
(int elementAId, _) = await SeedGraphicsElements();
interceptor.Armed = true;
await Should.ThrowAsync<DbUpdateException>(
async () => await MakeHandler().Handle(
MakeUpdate(channel.Id, number: "5", graphicsElementIds: [elementAId]),
CancellationToken.None));
}
}
ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs
+24
View File
@@ -120,6 +120,30 @@ public class GraphicsElementHandlerTests
result[0].BuiltIn.ShouldBeFalse();
}
// #568: Kind is part of the built-in element's identity, not a second test the seeder applies
// and the API skips. GetBuiltInElementId refuses a wrong-kind row at the seeded path -- it has
// to, since EnsureBuiltInElementRow asks it whether the Text row it is about to create already
// exists -- so an API that reported the same row as builtIn:true would have the two sites
// disagreeing about one row. Reddens if the Kind conjunct is dropped from
// GraphicsElementDefaults.IsOnNowNext (row 18 of the mutation table in docs/graphics-elements.md).
[Test]
public async Task GetAllGraphicsElementsForApi_Should_Not_Mark_A_Wrong_Kind_Row_At_The_Seeded_Path_As_BuiltIn()
{
await SeedElement(
1,
GraphicsElementDefaults.OnNowNextSeededPath,
GraphicsElementKind.Image,
string.Empty);
var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory);
List<GraphicsElementResponseModel> result =
await handler.Handle(new GetAllGraphicsElementsForApi(), CancellationToken.None);
result.Count.ShouldBe(1);
result[0].BuiltIn.ShouldBeFalse();
}
// The seeded path with only the FILENAME's case changed -- same folder, same spelling.
private static string CaseVariantOfSeededPath() =>
System.IO.Path.Combine(
ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs
+70
View File
@@ -1,3 +1,4 @@
using System.Globalization;
using System.Threading.Channels;
using ErsatzTV.Application;
using ErsatzTV.Application.Scheduling;
@@ -137,6 +138,7 @@ public class UpdateDecoGraphicsElementsTests
IsLeft(result).ShouldBeTrue();
BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left"));
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain("999");
// no partial write: the deco keeps no graphics element association
@@ -183,6 +185,7 @@ public class UpdateDecoGraphicsElementsTests
IsLeft(result).ShouldBeTrue();
BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left"));
error.Value.ShouldContain("[WatermarkIds]");
error.Value.ShouldContain("999");
}
@@ -253,4 +256,71 @@ public class UpdateDecoGraphicsElementsTests
Deco reloaded = await reload.Decos.Include(d => d.DecoGraphicsElements).SingleAsync(d => d.Id == 1);
reloaded.DecoGraphicsElements.Select(x => x.GraphicsElementId).ShouldBe(new[] { elementId });
}
// Both deco id lists go through the same Validators.IdsMustExist as the channel's, so both
// inherit the same raw-count cap; the channel fixture pins its edges, these two pin that each
// deco field is actually behind it and names itself when it rejects.
[Test]
public async Task Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds()
{
await SeedDeco();
var handler = new UpdateDecoHandler(_db.Factory, _channel);
Either<BaseError, Unit> result = await handler.Handle(
MakeUpdate(graphicsElementIds: Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList()),
CancellationToken.None);
IsLeft(result).ShouldBeTrue();
BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left"));
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture));
}
[Test]
public async Task Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds()
{
await SeedDeco();
var handler = new UpdateDecoHandler(_db.Factory, _channel);
Either<BaseError, Unit> result = await handler.Handle(
MakeUpdate(
watermarkMode: DecoMode.Override,
watermarkIds: Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList()),
CancellationToken.None);
IsLeft(result).ShouldBeTrue();
BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left"));
error.Value.ShouldContain("[WatermarkIds]");
error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture));
}
// The deco twin of the channel handler's lost-race translation: an element deleted between
// Validate and SaveChangesAsync must come back as the validator's own 422, not the FK
// exception. Removing the DbUpdateException catch from
// UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace is row 44 of the mutation table in
// docs/graphics-elements.md.
[Test]
public async Task Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422()
{
var interceptor = new ArmedSaveFailureInterceptor();
await _db.DisposeAsync();
_db = await InMemoryTvContext.CreateAsync(interceptor);
await SeedDeco();
int elementId = await SeedGraphicsElement();
interceptor.SqlBeforeFailing =
$"DELETE FROM GraphicsElement WHERE Id = {elementId.ToString(CultureInfo.InvariantCulture)}";
interceptor.Armed = true;
var handler = new UpdateDecoHandler(_db.Factory, _channel);
Either<BaseError, Unit> result = await handler.Handle(
MakeUpdate(graphicsElementIds: [elementId]),
CancellationToken.None);
IsLeft(result).ShouldBeTrue();
BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left"));
error.Value.ShouldContain("[GraphicsElementIds]");
error.Value.ShouldContain(elementId.ToString(CultureInfo.InvariantCulture));
}
}
ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs
+43
View File
@@ -0,0 +1,43 @@
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Diagnostics;
namespace ErsatzTV.Tests.Support;
/// <summary>
/// Makes the next <c>SaveChangesAsync</c> fail the way a lost race against a concurrent delete
/// does: run <see cref="SqlBeforeFailing" /> first (the delete), then throw the
/// <see cref="DbUpdateException" /> the foreign-key violation would have produced.
/// </summary>
/// <remarks>
/// What is simulated is the TRIGGER, not the decision: <see cref="InMemoryTvContext" /> runs
/// with foreign keys OFF, so a real constraint cannot fire here. The handler's recovery — re-ask
/// the existence question against the post-delete state, translate a now-missing id into the
/// validator's own 422, re-throw anything else — is real code running against real data.
/// Disarms itself on the first fire so the recovery path gets a working context.
/// </remarks>
public sealed class ArmedSaveFailureInterceptor : SaveChangesInterceptor
{
public bool Armed { get; set; }
public string SqlBeforeFailing { get; set; } = string.Empty;
public override async ValueTask<InterceptionResult<int>> SavingChangesAsync(
DbContextEventData eventData,
InterceptionResult<int> result,
CancellationToken cancellationToken = default)
{
if (!Armed)
{
return await base.SavingChangesAsync(eventData, result, cancellationToken);
}
Armed = false;
if (!string.IsNullOrEmpty(SqlBeforeFailing))
{
await eventData.Context!.Database.ExecuteSqlRawAsync(SqlBeforeFailing, cancellationToken);
}
throw new DbUpdateException("simulated: a validated principal row was deleted concurrently");
}
}
ErsatzTV.Tests/Support/InMemoryTvContext.cs
+9 -1
View File
@@ -3,6 +3,7 @@ using ErsatzTV.Infrastructure.Data;
using ErsatzTV.Infrastructure.Sqlite.Data;
using Microsoft.Data.Sqlite;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.Extensions.Logging.Abstractions;
namespace ErsatzTV.Tests.Support;
@@ -27,7 +28,13 @@ public sealed class InMemoryTvContext : IAsyncDisposable
public IDbContextFactory<TvContext> Factory => new TestDbContextFactory(_options);
public static async Task<InMemoryTvContext> CreateAsync()
/// <param name="interceptors">
/// Extra EF interceptors registered on every context this harness hands out. The only
/// current use is making <c>SaveChangesAsync</c> fail on demand: foreign keys are off here
/// (see the type summary), so a write-path failure a real FK constraint would raise cannot
/// be produced by seeding alone.
/// </param>
public static async Task<InMemoryTvContext> CreateAsync(params IInterceptor[] interceptors)
{
TvContext.IsSqlite = true;
TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation;
@@ -38,6 +45,7 @@ public sealed class InMemoryTvContext : IAsyncDisposable
DbContextOptions<TvContext> options = new DbContextOptionsBuilder<TvContext>()
.UseSqlite(connection)
.AddInterceptors(interceptors)
.Options;
await using (TvContext context = Create(options))