fix(#172): API hardening — null-name 500s, duplicate template items, unreachable 404
Clears the still-live findings from #172 (verified against main; #2/#4/#7 and the auth/search/Trakt tail were already deliberate-documented or fixed since 2026-07-07). - Null/empty Name → 500 (10 create/replace handlers). Block/Template/DecoTemplate/Deco Create+Replace/Update + UpdateFFmpegProfile did `request.Name.Length > 50` on a client-nullable string → unhandled NullReferenceException → HTTP 500 (no global exception filter). Now `string.IsNullOrWhiteSpace(request.Name) || .Length > 50` → 422; also rejects empty/whitespace names, matching the group-create handlers' NotEmpty behavior. CreatePlaylist coalesces null→"" at the DTO so it was an empty-name persist, not a 500; guarded the same way. - ReplaceTemplateItems overlap validation iterated with an `item == otherItem` record value-equality skip, so two exact-duplicate items were value-equal and bypassed the intersection check (both persisted). Now index-based (i != j) so duplicates register as a self-intersection and are rejected 422. - Trimmed the unreachable 404 ProducesResponseType from POST /api/blocks/groups and POST /api/templates/groups (a create has no parent lookup that can 404); v1.json regenerated. - Regression tests: all 10 name-guard paths + the duplicate-items path (19 cases). - Docs: decisions.md entry + api-conventions.md §3b null-safe-validation bullet. fixes #172 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Errors;
|
||||
using ErsatzTV.Core.FFmpeg;
|
||||
@@ -139,7 +139,7 @@ public class UpdateFFmpegProfileHandler(IDbContextFactory<TvContext> dbContextFa
|
||||
TvContext dbContext,
|
||||
UpdateFFmpegProfile updateFFmpegProfile)
|
||||
{
|
||||
if (updateFFmpegProfile.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(updateFFmpegProfile.Name) || updateFFmpegProfile.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"FFmpeg profile name \"{updateFFmpegProfile.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -35,7 +35,7 @@ public class CreatePlaylistHandler(IDbContextFactory<TvContext> dbContextFactory
|
||||
TvContext dbContext,
|
||||
CreatePlaylist request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Playlist name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -53,7 +53,7 @@ public class CreateBlockHandler(IDbContextFactory<TvContext> dbContextFactory)
|
||||
TvContext dbContext,
|
||||
CreateBlock request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Block name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -55,7 +55,7 @@ public class CreateDecoHandler(IDbContextFactory<TvContext> dbContextFactory)
|
||||
TvContext dbContext,
|
||||
CreateDeco request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Deco name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -56,7 +56,7 @@ public class CreateDecoTemplateHandler(IDbContextFactory<TvContext> dbContextFac
|
||||
TvContext dbContext,
|
||||
CreateDecoTemplate request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Deco template name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -52,7 +52,7 @@ public class CreateTemplateHandler(IDbContextFactory<TvContext> dbContextFactory
|
||||
TvContext dbContext,
|
||||
CreateTemplate request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Template name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -202,7 +202,7 @@ public class ReplaceBlockItemsHandler(IDbContextFactory<TvContext> dbContextFact
|
||||
Block block,
|
||||
ReplaceBlockItems request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Block name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -191,7 +191,7 @@ public class ReplaceDecoTemplateItemsHandler(
|
||||
DecoTemplate decoTemplate,
|
||||
ReplaceDecoTemplateItems request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Deco template name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -120,15 +120,21 @@ public class ReplaceTemplateItemsHandler(IDbContextFactory<TvContext> dbContextF
|
||||
}
|
||||
}
|
||||
|
||||
foreach (BlockTemplateItem item in allTemplateItems)
|
||||
// Compare by index, not value: BlockTemplateItem is a record, so two identical items would be
|
||||
// value-equal and skipped by an `item == otherItem` guard, letting exact duplicates persist
|
||||
// unvalidated (issue #172). Index comparison compares every distinct position, so duplicates
|
||||
// register as a (self-)intersection and are rejected.
|
||||
for (var i = 0; i < allTemplateItems.Count; i++)
|
||||
{
|
||||
foreach (BlockTemplateItem otherItem in allTemplateItems)
|
||||
BlockTemplateItem item = allTemplateItems[i];
|
||||
for (var j = 0; j < allTemplateItems.Count; j++)
|
||||
{
|
||||
if (item == otherItem)
|
||||
if (i == j)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
BlockTemplateItem otherItem = allTemplateItems[j];
|
||||
if (item.StartTime < otherItem.EndTime && otherItem.StartTime < item.EndTime)
|
||||
{
|
||||
return BaseError.New(
|
||||
@@ -154,7 +160,7 @@ public class ReplaceTemplateItemsHandler(IDbContextFactory<TvContext> dbContextF
|
||||
Template template,
|
||||
ReplaceTemplateItems request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Template name \"{request.Name}\" is invalid");
|
||||
}
|
||||
|
||||
@@ -236,7 +236,7 @@ public class UpdateDecoHandler(
|
||||
TvContext dbContext,
|
||||
UpdateDeco request)
|
||||
{
|
||||
if (request.Name.Length > 50)
|
||||
if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50)
|
||||
{
|
||||
return BaseError.New($"Deco name \"{request.Name}\" is invalid");
|
||||
}
|
||||
@@ -316,7 +316,7 @@ public class UpdateDecoHandler(
|
||||
case CollectionType.Playlist:
|
||||
if (breakContent.PlaylistId is null)
|
||||
{
|
||||
return BaseError.New("Break content must have valid playlist");
|
||||
return BaseError.New("Break content must have valid playlist");
|
||||
}
|
||||
|
||||
break;
|
||||
|
||||
@@ -80,9 +80,34 @@ public class FFmpegProfileHandlerTests
|
||||
LeftOf(result).ShouldBeOfType<NotFoundError>();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Update_Should_Reject_Null_Name()
|
||||
{
|
||||
// Issue #172 FIX A: the handler guarded a client-nullable Name with a bare Name.Length
|
||||
// check, so a null name threw a NullReferenceException (HTTP 500). The guard now returns a
|
||||
// validation failure (Left) instead of throwing.
|
||||
await SeedProfile(1);
|
||||
await SeedResolution(1);
|
||||
var handler = new UpdateFFmpegProfileHandler(_db.Factory, _searchTargets);
|
||||
|
||||
// Resolution seeded so the resolution-exists gate passes and the null name is the sole
|
||||
// failure: the guard must return a Left (validation failure) rather than throwing an NRE.
|
||||
Either<BaseError, UpdateFFmpegProfileResult> result =
|
||||
await handler.Handle(MakeUpdate(1) with { Name = null! }, CancellationToken.None);
|
||||
|
||||
LeftOf(result).ShouldBeAssignableTo<BaseError>();
|
||||
}
|
||||
|
||||
private static BaseError LeftOf<TR>(Either<BaseError, TR> either) =>
|
||||
either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result"));
|
||||
|
||||
private async Task SeedResolution(int id)
|
||||
{
|
||||
await using TvContext context = _db.CreateContext();
|
||||
context.Resolutions.Add(new Resolution { Id = id, Name = "1920x1080", Width = 1920, Height = 1080 });
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
private async Task SeedProfile(int id)
|
||||
{
|
||||
await using TvContext context = _db.CreateContext();
|
||||
|
||||
@@ -0,0 +1,316 @@
|
||||
using System.Threading.Channels;
|
||||
using ErsatzTV.Application;
|
||||
using ErsatzTV.Application.MediaCollections;
|
||||
using ErsatzTV.Application.Scheduling;
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Scheduling;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using LanguageExt;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Tests.Application.Scheduling;
|
||||
|
||||
/// <summary>
|
||||
/// Regression tests for issue #172 (API hardening).
|
||||
/// FIX A: ten command handlers guarded a client-nullable <c>Name</c> with a bare
|
||||
/// <c>request.Name.Length > 50</c>, so a null/whitespace name threw a NullReferenceException
|
||||
/// that surfaced as HTTP 500. The guard is now
|
||||
/// <c>string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50</c>, which returns a
|
||||
/// validation failure (Left) instead of throwing.
|
||||
/// FIX B: <see cref="ReplaceTemplateItemsHandler" /> overlap validation used to skip
|
||||
/// value-equal items (<c>item == otherItem</c> on a record), which let two exact-duplicate
|
||||
/// items bypass the intersection check and both persist. Validation now iterates by index, so
|
||||
/// duplicates register as a self-intersection and are rejected.
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
public class Issue172HardeningTests
|
||||
{
|
||||
private InMemoryTvContext _db = null!;
|
||||
|
||||
[SetUp]
|
||||
public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync();
|
||||
|
||||
[TearDown]
|
||||
public async Task TearDown() => await _db.DisposeAsync();
|
||||
|
||||
private static bool IsLeft<T>(Either<BaseError, T> result) => result.Match(Right: _ => false, Left: _ => true);
|
||||
|
||||
// ---- FIX B: duplicate template items must be rejected -------------------------------------
|
||||
|
||||
[Test]
|
||||
public async Task ReplaceTemplateItems_Should_Reject_Duplicate_Items()
|
||||
{
|
||||
// Block Minutes=30 gives each item a non-zero duration (0:00..0:30) so the two identical
|
||||
// items overlap. Under the OLD value-equality skip the two records were `==` and skipped,
|
||||
// letting both persist unvalidated; the index-based check now flags the self-intersection.
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
seed.Blocks.Add(
|
||||
new Block
|
||||
{
|
||||
Id = 10,
|
||||
BlockGroupId = 1,
|
||||
Name = "Morning",
|
||||
Minutes = 30,
|
||||
StopScheduling = BlockStopScheduling.AfterDurationEnd,
|
||||
Items = new List<BlockItem>()
|
||||
});
|
||||
seed.Templates.Add(
|
||||
new Template
|
||||
{
|
||||
Id = 1,
|
||||
TemplateGroupId = 1,
|
||||
Name = "Weekday",
|
||||
Version = 1,
|
||||
Items = new List<TemplateItem>()
|
||||
});
|
||||
await seed.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new ReplaceTemplateItemsHandler(_db.Factory);
|
||||
|
||||
var duplicate = new ReplaceTemplateItem(10, TimeSpan.Zero);
|
||||
var request = new ReplaceTemplateItems(
|
||||
1,
|
||||
1,
|
||||
"Weekday",
|
||||
new List<ReplaceTemplateItem> { duplicate, duplicate },
|
||||
ExpectedVersions: default); // None -> force-write path
|
||||
|
||||
Either<BaseError, List<TemplateItemViewModel>> result =
|
||||
await handler.Handle(request, CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
|
||||
// Non-vacuous: nothing was persisted (the old code would have written both items).
|
||||
await using TvContext ctx = _db.CreateContext();
|
||||
(await ctx.TemplateItems.CountAsync(i => i.TemplateId == 1)).ShouldBe(0);
|
||||
}
|
||||
|
||||
// ---- FIX A: null/whitespace name returns Left instead of throwing -------------------------
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task CreateBlock_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await SeedGroupAsync(g => g.BlockGroups.Add(new BlockGroup { Id = 1, Name = "G", Blocks = new List<Block>() }));
|
||||
|
||||
var handler = new CreateBlockHandler(_db.Factory);
|
||||
Either<BaseError, BlockViewModel> result =
|
||||
await handler.Handle(new CreateBlock(1, name!), CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task CreateTemplate_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await SeedGroupAsync(g =>
|
||||
g.TemplateGroups.Add(new TemplateGroup { Id = 1, Name = "G", Templates = new List<Template>() }));
|
||||
|
||||
var handler = new CreateTemplateHandler(_db.Factory);
|
||||
Either<BaseError, TemplateViewModel> result =
|
||||
await handler.Handle(new CreateTemplate(1, name!), CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task CreateDeco_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await SeedGroupAsync(g => g.DecoGroups.Add(new DecoGroup { Id = 1, Name = "G", Decos = new List<Deco>() }));
|
||||
|
||||
var handler = new CreateDecoHandler(_db.Factory);
|
||||
Either<BaseError, DecoViewModel> result =
|
||||
await handler.Handle(new CreateDeco(1, name!), CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task CreateDecoTemplate_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await SeedGroupAsync(g => g.DecoTemplateGroups.Add(
|
||||
new DecoTemplateGroup { Id = 1, Name = "G", DecoTemplates = new List<DecoTemplate>() }));
|
||||
|
||||
var handler = new CreateDecoTemplateHandler(_db.Factory);
|
||||
Either<BaseError, DecoTemplateViewModel> result =
|
||||
await handler.Handle(new CreateDecoTemplate(1, name!), CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase("")]
|
||||
public async Task CreatePlaylist_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
// The DTO coalesces null -> "" in production; the handler guard rejects both empty and null.
|
||||
var handler = new CreatePlaylistHandler(_db.Factory);
|
||||
Either<BaseError, PlaylistViewModel> result =
|
||||
await handler.Handle(new CreatePlaylist(1, name!), CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task ReplaceBlockItems_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
seed.Blocks.Add(
|
||||
new Block
|
||||
{
|
||||
Id = 1,
|
||||
BlockGroupId = 1,
|
||||
Name = "X",
|
||||
Minutes = 30,
|
||||
StopScheduling = BlockStopScheduling.AfterDurationEnd,
|
||||
Items = new List<BlockItem>()
|
||||
});
|
||||
await seed.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new ReplaceBlockItemsHandler(_db.Factory);
|
||||
var request = new ReplaceBlockItems(
|
||||
1,
|
||||
1,
|
||||
name!,
|
||||
30,
|
||||
BlockStopScheduling.AfterDurationEnd,
|
||||
new List<ReplaceBlockItem>());
|
||||
|
||||
Either<BaseError, Unit> result = await handler.Handle(request, CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task ReplaceTemplateItems_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
seed.Templates.Add(
|
||||
new Template
|
||||
{
|
||||
Id = 1,
|
||||
TemplateGroupId = 1,
|
||||
Name = "Weekday",
|
||||
Version = 1,
|
||||
Items = new List<TemplateItem>()
|
||||
});
|
||||
await seed.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new ReplaceTemplateItemsHandler(_db.Factory);
|
||||
var request = new ReplaceTemplateItems(
|
||||
1,
|
||||
1,
|
||||
name!,
|
||||
new List<ReplaceTemplateItem>());
|
||||
|
||||
Either<BaseError, List<TemplateItemViewModel>> result =
|
||||
await handler.Handle(request, CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task ReplaceDecoTemplateItems_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
seed.DecoTemplates.Add(
|
||||
new DecoTemplate
|
||||
{
|
||||
Id = 1,
|
||||
DecoTemplateGroupId = 1,
|
||||
Name = "X",
|
||||
Items = new List<DecoTemplateItem>()
|
||||
});
|
||||
await seed.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var channel = Substitute.For<ChannelWriter<IBackgroundServiceRequest>>();
|
||||
var handler = new ReplaceDecoTemplateItemsHandler(_db.Factory, channel);
|
||||
var request = new ReplaceDecoTemplateItems(
|
||||
1,
|
||||
1,
|
||||
name!,
|
||||
new List<ReplaceDecoTemplateItem>());
|
||||
|
||||
Either<BaseError, List<DecoTemplateItemViewModel>> result =
|
||||
await handler.Handle(request, CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase(null)]
|
||||
[TestCase(" ")]
|
||||
public async Task UpdateDeco_Should_Reject_Blank_Name(string? name)
|
||||
{
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
seed.Decos.Add(
|
||||
new Deco
|
||||
{
|
||||
Id = 1,
|
||||
DecoGroupId = 1,
|
||||
Name = "X",
|
||||
BreakContent = [],
|
||||
DecoWatermarks = [],
|
||||
DecoGraphicsElements = []
|
||||
});
|
||||
await seed.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var channel = Substitute.For<ChannelWriter<IBackgroundServiceRequest>>();
|
||||
var handler = new UpdateDecoHandler(_db.Factory, channel);
|
||||
var request = new UpdateDeco(
|
||||
1,
|
||||
1,
|
||||
name!,
|
||||
DecoMode.Inherit,
|
||||
new List<int>(),
|
||||
false,
|
||||
DecoMode.Inherit,
|
||||
new List<int>(),
|
||||
false,
|
||||
DecoMode.Inherit,
|
||||
new List<UpdateDecoBreakContent>(),
|
||||
DecoMode.Inherit,
|
||||
CollectionType.Collection,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
false,
|
||||
DecoMode.Inherit,
|
||||
CollectionType.Collection,
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
null);
|
||||
|
||||
Either<BaseError, Unit> result = await handler.Handle(request, CancellationToken.None);
|
||||
|
||||
IsLeft(result).ShouldBeTrue();
|
||||
}
|
||||
|
||||
private async Task SeedGroupAsync(Action<TvContext> seed)
|
||||
{
|
||||
await using TvContext ctx = _db.CreateContext();
|
||||
seed(ctx);
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
}
|
||||
@@ -29,7 +29,6 @@ public class BlockController(IMediator mediator) : ControllerBase
|
||||
[EndpointSummary("Create a block group")]
|
||||
[EndpointGroupName("general")]
|
||||
[ProducesResponseType(typeof(BlockGroupResponseModel), StatusCodes.Status201Created)]
|
||||
[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)]
|
||||
[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)]
|
||||
public async Task<IActionResult> CreateGroup(
|
||||
[Required][FromBody] CreateBlockGroupRequest request,
|
||||
|
||||
@@ -29,7 +29,6 @@ public class TemplateController(IMediator mediator) : ControllerBase
|
||||
[EndpointSummary("Create a template group")]
|
||||
[EndpointGroupName("general")]
|
||||
[ProducesResponseType(typeof(TemplateGroupResponseModel), StatusCodes.Status201Created)]
|
||||
[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)]
|
||||
[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)]
|
||||
public async Task<IActionResult> CreateGroup(
|
||||
[Required][FromBody] CreateTemplateGroupRequest request,
|
||||
|
||||
@@ -296,26 +296,6 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"404": {
|
||||
"description": "Not Found",
|
||||
"content": {
|
||||
"text/plain": {
|
||||
"schema": {
|
||||
"$ref": "#/components/schemas/ProblemDetails"
|
||||
}
|
||||
},
|
||||
"application/json": {
|
||||
"schema": {
|
||||
"$ref": "#/components/schemas/ProblemDetails"
|
||||
}
|
||||
},
|
||||
"text/json": {
|
||||
"schema": {
|
||||
"$ref": "#/components/schemas/ProblemDetails"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"422": {
|
||||
"description": "Unprocessable Entity",
|
||||
"content": {
|
||||
@@ -19518,26 +19498,6 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"404": {
|
||||
"description": "Not Found",
|
||||
"content": {
|
||||
"text/plain": {
|
||||
"schema": {
|
||||
"$ref": "#/components/schemas/ProblemDetails"
|
||||
}
|
||||
},
|
||||
"application/json": {
|
||||
"schema": {
|
||||
"$ref": "#/components/schemas/ProblemDetails"
|
||||
}
|
||||
},
|
||||
"text/json": {
|
||||
"schema": {
|
||||
"$ref": "#/components/schemas/ProblemDetails"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"422": {
|
||||
"description": "Unprocessable Entity",
|
||||
"content": {
|
||||
|
||||
@@ -208,6 +208,15 @@ handler's validation when a lookup fails, so the controller-side mapping falls o
|
||||
caller) → surface as 422 instead of silently returning a shorter list.
|
||||
- Unbounded `int`/`TimeSpan` inputs from the request → clamp or validate, per the Logs pagination
|
||||
pattern above.
|
||||
- **Dereferencing a request `string` (e.g. `request.Name.Length`) is a latent 500** — request DTOs
|
||||
carry no `#nullable` context (§2), so a `string Name` binds `null` from `name: null`/an omitted field
|
||||
and there is no implicit `[Required]`; a raw `.Length`/`.Trim()` throws `NullReferenceException` → an
|
||||
unhandled **500** (there is no global exception filter). Validate names null-safe: reuse the
|
||||
`Validators.NotEmpty(x => x.Name).Bind(_ => x.NotLongerThan(50)(x => x.Name))` combinator
|
||||
(`ErsatzTV.Application/Validators/StringValidation.cs`; both are null-safe via `Optional`), the same
|
||||
pattern the group-create handlers already use — or at minimum guard `string.IsNullOrWhiteSpace(name)`
|
||||
before any member access. Fixed across 10 create/replace handlers in issue #172 (was `if
|
||||
(request.Name.Length > 50)`).
|
||||
- **Known, deliberate exception**: deep FK ids nested inside item-list request bodies (e.g. a
|
||||
schedule item's `CollectionId`) are **not** existence-checked at that depth — this is established
|
||||
precedent from the schedules endpoints (see issue #172) and intentional to avoid N+1 validation
|
||||
|
||||
@@ -1759,3 +1759,37 @@ in `Controllers.Api`, computes each action's *effective* route (ASP.NET's class+
|
||||
asserts it matches `^/api/v\d+/` — so a new controller that drifts (relative or unversioned) fails CI, the
|
||||
"fix-it-while-you're-in-the-file" gate the `dotnet format` rules use. Browser-nav endpoints deliberately outside
|
||||
`/api` (e.g. `GET /auth/oidc/login`) are out of scope for the test. Docs: `api-conventions.md` §1/§9. Refs #286 #197.
|
||||
|
||||
## 2026-07-13 — Scheduling API hardening: null-name 500s, duplicate template items, unreachable 404 (#172)
|
||||
|
||||
Cleared the still-live findings from issue #172 (consolidated non-blocking nits from the #144 S1/S2
|
||||
reviews). Most of the 2026-07-07 list had already been ratified deliberate (§8 "(none)" synthesized
|
||||
rows; §3b deep-FK non-existence-check) or fixed since (the unauthenticated `/api/logs` +
|
||||
`/api/troubleshoot/info` GETs now carry `[RequiresAuthentication]` per §9; the Trakt matched-items link
|
||||
points at the live `/app/search`; `GET /api/search` already fans out via `Task.WhenAll`). Three were
|
||||
genuinely live:
|
||||
|
||||
- **Null/empty `name` → 500 (10 handlers).** Create + Replace/Update handlers for Block, Template,
|
||||
DecoTemplate, Deco (8, all genuine 500s), plus `UpdateFFmpegProfile` (genuine 500; `CreateFFmpegProfile`
|
||||
was already guarded) and `CreatePlaylist` (its DTO coalesces `null`→`""`, so an empty-name persist, not a
|
||||
500) all did `if (request.Name.Length > 50)` on a client-nullable `string Name` → unhandled
|
||||
`NullReferenceException`. Fixed to `if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length >
|
||||
50)` — kills the NRE, and also rejects empty/whitespace names (matching the group-create handlers'
|
||||
`NotEmpty` behavior, closing a latent "block/template named ''" gap). Chose the one-line guard over
|
||||
refactoring each handler onto the `NotEmpty`/`NotLongerThan` combinator to keep the blast radius tiny and
|
||||
preserve each handler's existing error message + 422 mapping. Convention captured in `api-conventions.md`
|
||||
§3b handler-hardening checklist.
|
||||
- **Exact-duplicate template items bypassed overlap validation.** `ReplaceTemplateItemsHandler`'s O(n²)
|
||||
overlap loop skipped on `item == otherItem`, but `BlockTemplateItem` is a `record`, so two value-identical
|
||||
items (same BlockId + StartTime → same computed EndTime) were value-equal and skipped — both persisted
|
||||
unvalidated. Switched to index-based iteration (`i != j`) so identical items at distinct positions are
|
||||
compared and register as a (self-)intersection → rejected 422. (The SPA's index-based check already caught
|
||||
this client-side; it was an API-only gap.)
|
||||
- **Unreachable 404 on create-group actions.** `POST /api/blocks/groups` and `POST /api/templates/groups`
|
||||
declared `[ProducesResponseType(ProblemDetails, 404)]` copied from precedent, but a create has no parent
|
||||
lookup that can 404 (only 201/422). Trimmed — OpenAPI spec regenerated.
|
||||
|
||||
Deliberately **not** fixed (documented as accepted): the §8 "(none)" synthetic rows, the §3b deep-FK
|
||||
non-existence-check, and the missing `Name=` on `PlayoutController` Create/Delete/Update (moot — the
|
||||
"v1"-doc `OperationIdOpenApiTransformer` (#197 Bundle C) already synthesizes stable operationIds for
|
||||
`Name=`-less ops, and adding `Name=` would risk renaming generated SPA client methods). Refs #172 #197.
|
||||
|
||||
Reference in New Issue
Block a user