From 214fad2dcd20159ee54cb23f555a1c7053ac0e88 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 11:09:35 +0200 Subject: [PATCH 1/3] fix(633): document the 0-based paging contract on the OpenAPI parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `api.paging-zero-based` says `pageNum` is 0-based across `/api/v1` and every wrapper of it. That was true of the MCP tool catalog and the docs, and not true of the generated OpenAPI document: all 24 paging parameters across the 12 paged operations were emitted with no `description` at all, so a consumer reading only `v1.json` — the intended contract, and what generated clients surface to their users — had to infer the base from `default: 0`. That is the same inference that cost #487 a verification pass on the MCP side, where the description was present but wrong. Annotates each `[FromQuery]` paging parameter with `[Description]` (`System.ComponentModel`), the mechanism `parentId` already used in ImagesController, and regenerates `v1.json`. `pageSize` states the endpoint's OWN cap, because the caps genuinely differ — 100 typical, 200 auto-tune members, 1000 search/all-items — and the record forbids documenting one global number; it also states that the offset derives from the effective (capped) size, so an over-large `pageSize` narrows the page instead of widening the offset. The generated TypeScript client covers DTOs only, not query parameters, so it is unchanged; `endpoint-index.md` carries summaries, not parameter descriptions, so it is unchanged too. Pinned by OpenApiPagingContractTests against the in-process generated document. The test NAMES the expected set of 12 paged operations rather than only filtering for parameters called `pageNum`: a filter cannot see an endpoint that should page and doesn't, which is exactly how two MCP tools escaped the equivalent check in #616. Set equality is asserted in both directions, and the caps are pinned per endpoint so a description naming the wrong cap fails — a wrong justification outlives a wrong line. Mutation-verified both ways: dropping one `[Description]` reddens the description test, and making one endpoint stop exposing `pageNum`/`pageSize` under those names reddens the set-equality test. Refs #633 Decisions-Edit: yes --- .../Controllers/OpenApiPagingContractTests.cs | 167 ++++++++++++++++++ ErsatzTV/Controllers/Api/ChannelController.cs | 9 +- .../Controllers/Api/CollectionController.cs | 9 +- .../Api/LibraryBrowseController.cs | 8 +- ErsatzTV/Controllers/Api/LogsController.cs | 9 +- .../Api/MultiCollectionController.cs | 9 +- ErsatzTV/Controllers/Api/PlayoutController.cs | 25 ++- .../Api/RerunCollectionController.cs | 9 +- ErsatzTV/Controllers/Api/SearchController.cs | 17 +- ErsatzTV/Controllers/Api/TraktController.cs | 9 +- ErsatzTV/wwwroot/openapi/v1.json | 24 +++ docs/api-conventions.md | 9 +- .../records/api/paging-zero-based.md | 23 ++- 13 files changed, 296 insertions(+), 31 deletions(-) create mode 100644 ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs diff --git a/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs b/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs new file mode 100644 index 000000000..58b4426b3 --- /dev/null +++ b/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs @@ -0,0 +1,167 @@ +using ErsatzTV.Tests.Support; +using Microsoft.OpenApi; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Controllers; + +/// +/// Pins the api.paging-zero-based contract onto the generated OpenAPI document (ersatztv#633). +/// The spec is the contract REST consumers read — and what generated clients surface to their users — +/// so a paging parameter that documents nothing forces every consumer to infer the base from +/// default: 0. That is exactly the inference that cost ersatztv#487 a verification pass on the +/// MCP side, where the description was present but wrong. The MCP wrapper is pinned the same way in +/// ErsatzTV.Mcp.Tests.ToolCatalogTests; this is the API-side half. +/// +[TestFixture] +public class OpenApiPagingContractTests +{ + /// + /// Every operation that pages. Named explicitly rather than discovered, because a test that only + /// FILTERS on "declares pageNum" cannot see the endpoint that should page and does not — the + /// defect escapes the filter and the test still passes green over a shrinking scope. That is not + /// hypothetical: ersatztv#616 found two MCP tools doing precisely that. So the expected set is + /// pinned here, and asserts the + /// discovered set equals it in BOTH directions — a new paged endpoint fails until it is added + /// (with descriptions), and an endpoint that silently drops paging fails too. + /// + private static readonly string[] PagedOperations = + [ + "GET /api/v1/channels/auto-tune/members", + "GET /api/v1/collections/{id}/items", + "GET /api/v1/library/browse", + "GET /api/v1/logs", + "GET /api/v1/multi-collections", + "GET /api/v1/playouts", + "GET /api/v1/playouts/{id}/blocks/{blockId}/history", + "GET /api/v1/playouts/{id}/items", + "GET /api/v1/rerun-collections", + "GET /api/v1/search", + "GET /api/v1/search/all-items", + "GET /api/v1/trakt/lists" + ]; + + private static OpenApiDocument _document = null!; + + [OneTimeSetUp] + public async Task BuildDocument() => _document = await GeneratedOpenApiDocument.BuildV1Async(); + + [Test] + public void Paged_Operations_Should_Be_Exactly_The_Pinned_Set() + { + List discovered = EnumerateOperations() + .Where(op => ParameterNames(op.Operation).Overlaps(new[] { "pageNum", "pageSize" })) + .Select(op => $"{op.Method} {op.Path}") + .OrderBy(s => s, StringComparer.Ordinal) + .ToList(); + + discovered.ShouldBe(PagedOperations.OrderBy(s => s, StringComparer.Ordinal).ToList()); + } + + [Test] + public void Every_Paged_Operation_Should_Declare_Both_Paging_Parameters() + { + foreach (string key in PagedOperations) + { + HashSet names = ParameterNames(Find(key)); + + names.ShouldContain("pageNum", $"{key} should declare pageNum"); + names.ShouldContain("pageSize", $"{key} should declare pageSize"); + } + } + + [Test] + public void Every_PageNum_Parameter_Should_Document_The_ZeroBased_Contract() + { + foreach (string key in PagedOperations) + { + string description = Description(key, "pageNum"); + + // The whole point of the record: a consumer must not have to infer the base from `default: 0`. + description.ShouldContain("0-based", Case.Insensitive, $"{key} pageNum should say it is 0-based"); + description.ShouldNotContain("1-based", Case.Insensitive, $"{key} pageNum must not claim 1-based"); + } + } + + [Test] + public void Every_PageSize_Parameter_Should_Document_The_Cap_And_The_Effective_Offset() + { + foreach (string key in PagedOperations) + { + string description = Description(key, "pageSize"); + + // `api.paging-zero-based` is explicit that the cap is PER-ENDPOINT and must not be documented + // as one number, and that the offset derives from the effective (capped) size — so an + // over-large pageSize narrows the page without widening the offset. + description.ShouldContain("capped at", Case.Insensitive, $"{key} pageSize should state its cap"); + description.ShouldContain("this endpoint", Case.Insensitive, $"{key} pageSize cap should be scoped to the endpoint"); + description.ShouldContain("effective", Case.Insensitive, $"{key} pageSize should explain the effective-size offset"); + } + } + + [Test] + public void PageSize_Caps_Should_Match_The_Values_The_Controllers_Actually_Clamp_To() + { + // The caps genuinely differ per endpoint, which is why the record forbids documenting one number. + // A description naming the wrong cap is worse than none — a wrong justification outlives a wrong + // line — so pin each against the value its controller clamps to. + var expectedCaps = new Dictionary(StringComparer.Ordinal) + { + ["GET /api/v1/channels/auto-tune/members"] = 200, + ["GET /api/v1/collections/{id}/items"] = 100, + ["GET /api/v1/library/browse"] = 100, + ["GET /api/v1/logs"] = 100, + ["GET /api/v1/multi-collections"] = 100, + ["GET /api/v1/playouts"] = 100, + ["GET /api/v1/playouts/{id}/blocks/{blockId}/history"] = 100, + ["GET /api/v1/playouts/{id}/items"] = 100, + ["GET /api/v1/rerun-collections"] = 100, + ["GET /api/v1/search"] = 100, + ["GET /api/v1/search/all-items"] = 1000, + ["GET /api/v1/trakt/lists"] = 100 + }; + + // Guard the guard: every pinned operation must carry an expected cap, so adding one above + // without its cap here cannot quietly skip this assertion. + expectedCaps.Keys.OrderBy(k => k, StringComparer.Ordinal) + .ShouldBe(PagedOperations.OrderBy(k => k, StringComparer.Ordinal)); + + foreach ((string key, int cap) in expectedCaps) + { + Description(key, "pageSize") + .ShouldContain($"capped at {cap}", Case.Insensitive, $"{key} pageSize should document a cap of {cap}"); + } + } + + private static string Description(string key, string parameterName) + { + IOpenApiParameter parameter = Find(key).Parameters! + .First(p => string.Equals(p.Name, parameterName, StringComparison.Ordinal)); + + string? description = parameter.Description; + description.ShouldNotBeNullOrWhiteSpace($"{key} {parameterName} should carry a description"); + + return description!; + } + + private static OpenApiOperation Find(string key) => + EnumerateOperations() + .Where(op => string.Equals($"{op.Method} {op.Path}", key, StringComparison.Ordinal)) + .Select(op => op.Operation) + .FirstOrDefault() + .ShouldNotBeNull($"{key} should exist in the generated document"); + + private static HashSet ParameterNames(OpenApiOperation operation) => + (operation.Parameters ?? []).Select(p => p.Name ?? string.Empty).ToHashSet(StringComparer.Ordinal); + + private static IEnumerable<(string Method, string Path, OpenApiOperation Operation)> EnumerateOperations() + { + foreach ((string path, IOpenApiPathItem item) in _document.Paths) + { + foreach ((HttpMethod method, OpenApiOperation operation) in item.Operations!) + { + yield return (method.Method.ToUpperInvariant(), path, operation); + } + } + } +} diff --git a/ErsatzTV/Controllers/Api/ChannelController.cs b/ErsatzTV/Controllers/Api/ChannelController.cs index 496e3c244..a688295e5 100644 --- a/ErsatzTV/Controllers/Api/ChannelController.cs +++ b/ErsatzTV/Controllers/Api/ChannelController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Threading.Channels; using ErsatzTV.Application; @@ -264,8 +265,12 @@ public class ChannelController( public async Task GetAutoTuneChannelMembers( [FromQuery] AutoTuneAxis axis, [FromQuery] string value, - [FromQuery] int pageNum, - [FromQuery] int pageSize, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum, + [FromQuery] + [Description("Rows per page; capped at 200 for this endpoint. A value of 0 or less falls back to 100 rather than being clamped to 1. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize, CancellationToken cancellationToken) { pageNum = Math.Max(0, pageNum); diff --git a/ErsatzTV/Controllers/Api/CollectionController.cs b/ErsatzTV/Controllers/Api/CollectionController.cs index 2a7e81e5a..4e64d8ece 100644 --- a/ErsatzTV/Controllers/Api/CollectionController.cs +++ b/ErsatzTV/Controllers/Api/CollectionController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.ComponentModel.DataAnnotations; using ErsatzTV.Application.MediaCollections; using ErsatzTV.Controllers.Api.Requests; @@ -46,8 +47,12 @@ public class CollectionController(IMediator mediator) : ControllerBase [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] public async Task GetItems( int id, - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, CancellationToken cancellationToken = default) { int clampedPageNum = Math.Max(0, pageNum); diff --git a/ErsatzTV/Controllers/Api/LibraryBrowseController.cs b/ErsatzTV/Controllers/Api/LibraryBrowseController.cs index 0a5997550..8d05baae2 100644 --- a/ErsatzTV/Controllers/Api/LibraryBrowseController.cs +++ b/ErsatzTV/Controllers/Api/LibraryBrowseController.cs @@ -21,8 +21,12 @@ public class LibraryBrowseController(IMediator mediator) : ControllerBase [FromQuery] string query = "", [FromQuery] int? libraryId = null, [FromQuery] LibraryBrowseMediaType? mediaType = null, - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, [FromQuery] [Description("Parent id for a drill-in listing; only used with mediaType=TelevisionSeason (that show's seasons), mediaType=Episode (that season's episodes) or mediaType=MusicVideo (that artist's music videos), ignored otherwise")] int? parentId = null, diff --git a/ErsatzTV/Controllers/Api/LogsController.cs b/ErsatzTV/Controllers/Api/LogsController.cs index a079129ca..2b8276e70 100644 --- a/ErsatzTV/Controllers/Api/LogsController.cs +++ b/ErsatzTV/Controllers/Api/LogsController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.Linq.Expressions; using ErsatzTV.Application.Logs; using ErsatzTV.Core.Api.Logs; @@ -29,8 +30,12 @@ public class LogsController(IMediator mediator) : ControllerBase [EndpointGroupName("general")] [ProducesResponseType(typeof(PagedLogEntriesResponseModel), StatusCodes.Status200OK)] public async Task GetLogs( - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, [FromQuery] string filter = "", [FromQuery] string sortField = "timestamp", [FromQuery] string sortDirection = "desc", diff --git a/ErsatzTV/Controllers/Api/MultiCollectionController.cs b/ErsatzTV/Controllers/Api/MultiCollectionController.cs index 21796e795..e8f6a2f01 100644 --- a/ErsatzTV/Controllers/Api/MultiCollectionController.cs +++ b/ErsatzTV/Controllers/Api/MultiCollectionController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.ComponentModel.DataAnnotations; using ErsatzTV.Application.MediaCollections; using ErsatzTV.Controllers.Api.Requests; @@ -22,8 +23,12 @@ public class MultiCollectionController(IMediator mediator) : ControllerBase [ProducesResponseType(typeof(PagedMultiCollectionsResponseModel), StatusCodes.Status200OK)] public async Task GetAll( [FromQuery] string query = "", - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, CancellationToken cancellationToken = default) { int clampedPageNum = Math.Max(0, pageNum); diff --git a/ErsatzTV/Controllers/Api/PlayoutController.cs b/ErsatzTV/Controllers/Api/PlayoutController.cs index 4fa94da83..1d0846b80 100644 --- a/ErsatzTV/Controllers/Api/PlayoutController.cs +++ b/ErsatzTV/Controllers/Api/PlayoutController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.ComponentModel.DataAnnotations; using ErsatzTV.Application.Playouts; using ErsatzTV.Application.ProgramSchedules; @@ -41,8 +42,12 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : [ProducesResponseType(typeof(PagedPlayoutsResponseModel), StatusCodes.Status200OK)] public async Task GetAll( [FromQuery] string query = "", - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, CancellationToken cancellationToken = default) { pageNum = Math.Max(0, pageNum); @@ -83,8 +88,12 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : public async Task GetItems( int id, [FromQuery] bool showFiller = false, - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, CancellationToken cancellationToken = default) { Option maybePlayout = await mediator.Send(new GetPlayoutById(id), cancellationToken); @@ -543,8 +552,12 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : public async Task GetBlockHistory( int id, int blockId, - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, CancellationToken cancellationToken = default) { Option maybePlayout = await mediator.Send(new GetPlayoutById(id), cancellationToken); diff --git a/ErsatzTV/Controllers/Api/RerunCollectionController.cs b/ErsatzTV/Controllers/Api/RerunCollectionController.cs index 8262afe24..4426c3ce6 100644 --- a/ErsatzTV/Controllers/Api/RerunCollectionController.cs +++ b/ErsatzTV/Controllers/Api/RerunCollectionController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.ComponentModel.DataAnnotations; using ErsatzTV.Application.MediaCollections; using ErsatzTV.Controllers.Api.Requests; @@ -22,8 +23,12 @@ public class RerunCollectionController(IMediator mediator) : ControllerBase [ProducesResponseType(typeof(PagedRerunCollectionsResponseModel), StatusCodes.Status200OK)] public async Task GetAll( [FromQuery] string query = "", - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, CancellationToken cancellationToken = default) { int clampedPageNum = Math.Max(0, pageNum); diff --git a/ErsatzTV/Controllers/Api/SearchController.cs b/ErsatzTV/Controllers/Api/SearchController.cs index b2153e374..958452fab 100644 --- a/ErsatzTV/Controllers/Api/SearchController.cs +++ b/ErsatzTV/Controllers/Api/SearchController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using ErsatzTV.Application.MediaCollections; using ErsatzTV.Application.MediaItems; using ErsatzTV.Application.Search; @@ -35,8 +36,12 @@ public class SearchController(IMediator mediator) : ControllerBase [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task Search( [FromQuery] string query = "", - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 50, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 50); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 50, CancellationToken cancellationToken = default) { if (string.IsNullOrWhiteSpace(query)) @@ -65,8 +70,12 @@ public class SearchController(IMediator mediator) : ControllerBase [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task SearchAllItems( [FromQuery] string query = "", - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = DefaultAllItemsPageSize, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 500); capped at 1000 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = DefaultAllItemsPageSize, CancellationToken cancellationToken = default) { if (string.IsNullOrWhiteSpace(query)) diff --git a/ErsatzTV/Controllers/Api/TraktController.cs b/ErsatzTV/Controllers/Api/TraktController.cs index 838004695..c597ab263 100644 --- a/ErsatzTV/Controllers/Api/TraktController.cs +++ b/ErsatzTV/Controllers/Api/TraktController.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Text.RegularExpressions; using System.Threading.Channels; @@ -28,8 +29,12 @@ public partial class TraktController( [EndpointGroupName("general")] [ProducesResponseType(typeof(PagedTraktListsResponseModel), StatusCodes.Status200OK)] public async Task GetAll( - [FromQuery] int pageNum = 0, - [FromQuery] int pageSize = 100, + [FromQuery] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + int pageNum = 0, + [FromQuery] + [Description("Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] + int pageSize = 100, CancellationToken cancellationToken = default) { int clampedPageNum = Math.Max(0, pageNum); diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 5d384d488..59d3ad438 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -2578,6 +2578,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32" @@ -2586,6 +2587,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page; capped at 200 for this endpoint. A value of 0 or less falls back to 100 rather than being clamped to 1. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32" @@ -3863,6 +3865,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -3872,6 +3875,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -9385,6 +9389,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -9394,6 +9399,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -10154,6 +10160,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -10163,6 +10170,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -10760,6 +10768,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -10769,6 +10778,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -12479,6 +12489,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -12488,6 +12499,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -13075,6 +13087,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -13084,6 +13097,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -14040,6 +14054,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -14049,6 +14064,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -15533,6 +15549,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -15542,6 +15559,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -17351,6 +17369,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -17360,6 +17379,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 50); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -17454,6 +17474,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -17463,6 +17484,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 500); capped at 1000 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", @@ -21042,6 +21064,7 @@ { "name": "pageNum", "in": "query", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", "schema": { "type": "integer", "format": "int32", @@ -21051,6 +21074,7 @@ { "name": "pageSize", "in": "query", + "description": "Rows per page (default 100); capped at 100 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.", "schema": { "type": "integer", "format": "int32", diff --git a/docs/api-conventions.md b/docs/api-conventions.md index db3546bd5..260686024 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -46,7 +46,14 @@ Exemplars: narrower pages — it never widens the offset. Say "0-based" in the description of any paging parameter you expose, including on wrapper surfaces like the MCP tool catalog: describing it as 1-based makes a caller skip the first page silently, which reads as data loss rather than as an - off-by-one (ersatztv#616). See `api.paging-zero-based`. + off-by-one (ersatztv#616). Put that description on the parameter itself with + `[Description("...")]` (`System.ComponentModel`, on the `[FromQuery]` parameter) so it reaches the + generated OpenAPI document — an attribute-free paging parameter is emitted with no description at + all, leaving a REST consumer to infer the base from `default: 0` (ersatztv#633). State the + endpoint's **own** cap, never one global number: the caps differ (100 typical, 200 auto-tune + members, 1000 `search/all-items`). `OpenApiPagingContractTests` pins this and names the expected + set of paged operations, so a new paged endpoint fails until it is added there **with** + descriptions. See `api.paging-zero-based`. - **Sortable GET with allow-listed sort params**: same file — `sortField`/`sortDirection` are normalized against a fixed allow-list (`AllowedSortFields`) rather than trusted or rejected with a 422: an unrecognized `sortField` silently falls back to the default field, an unrecognized diff --git a/docs/decisions/records/api/paging-zero-based.md b/docs/decisions/records/api/paging-zero-based.md index 4b5d8f930..9c1ec058c 100644 --- a/docs/decisions/records/api/paging-zero-based.md +++ b/docs/decisions/records/api/paging-zero-based.md @@ -1,6 +1,6 @@ --- key: api.paging-zero-based -title: 2026-07-25 — Paging is 0-based everywhere; every wrapper must say so (OpenAPI still doesn't) (#616) +title: 2026-07-25 — Paging is 0-based everywhere; every wrapper must say so (#616, #633) status: active since: '2026-07-25' supersedes: none @@ -42,11 +42,22 @@ dozen controllers and the SPA, and a 1-based wrapper over a 0-based API would ma parameter name* mean different things on two surfaces a reader routinely reads together — trading a documented off-by-one for an undocumented one. Accuracy in the description is the cheaper contract. -**Where "0-based" is stated, and where it still isn't.** The MCP tool catalog and these docs say it -explicitly. The generated OpenAPI `pageNum` parameters carry **no description at all** (12 of them), -so a REST consumer reading only `v1.json` still has to infer the base from the default — a real -remaining gap, tracked separately rather than fixed here. Treat "every wrapper says 0-based" as the -target this record sets, not a property already true of the OpenAPI surface. +**Where "0-based" is stated.** The MCP tool catalog, these docs, and — since #633 — the generated +OpenAPI document all say it explicitly. All 24 paging parameters across the 12 paged operations carry +a `[Description]` (`System.ComponentModel`, on the `[FromQuery]` parameter, the same mechanism +`parentId` already used), so a REST consumer reading only `v1.json` no longer has to infer the base +from `default: 0` — which is the inference that cost #487 a verification pass on the MCP side, where +the description was present but wrong. `pageSize` descriptions state the endpoint's own cap and that +the offset derives from the effective size, never a single global number. + +That sweep is pinned by `OpenApiPagingContractTests` against the in-process generated document. The +test names the expected set of 12 operations rather than only filtering for parameters called +`pageNum`: a filter cannot see an endpoint that *should* page and doesn't, so set-equality is +asserted in both directions — a new paged endpoint fails until it is added with descriptions, and an +endpoint that quietly drops paging fails too. Both directions are mutation-verified. The residual +gap this cannot close is a brand-new endpoint that returns a page while declaring no paging +parameters at all under any name; nothing in the document distinguishes that from an unpaged +endpoint, so it stays a review concern. **Corollary — ids in paged rows.** A row that names a related entity should expose that entity's id, not only its display fields, wherever a caller is expected to act on that entity. This is a rule about -- 2.47.3 From 23791c1bbbac973f3a9fabe83336f4273f0c0db9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 11:23:51 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(633):=20review=20fixes=20=E2=80=94=20th?= =?UTF-8?q?e=20all-items=20upper=20clamp,=20and=20a=20cap=20test=20that=20?= =?UTF-8?q?couldn't=20fail?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review (Codex) found three, two of them real defects rather than polish. 1. `search/all-items` is the one paged endpoint that clamps `pageNum` ABOVE as well as below — `Math.Clamp(pageNum, 0, MaxAllItemsPageNum)`, 2,000,000, so pageNum*pageSize cannot overflow int into a 500. The description documented only the lower clamp, so the published contract looked unbounded: a client sending pageNum=int.MaxValue is silently served page 2,000,000. Now stated, and called out as the exception it is. 2. The cap assertion could not fail in the direction that matters. `ShouldContain( "capped at 100")` is satisfied by the string "capped at 1000", so a cap-100 endpoint whose description claimed 1000 passed — precisely the wrong-cap defect the test was added to catch, and a test that cannot fail on its own subject is worse than none. Matched as a whole token instead, and mutation-verified: making /logs claim 1000 now reddens it, where before it stayed green. 3. `Description` used `First`, so a missing parameter threw "Sequence contains no matching element" — naming neither endpoint nor parameter, and reading as a broken test rather than the contract violation it is. Fails informatively now. The review confirmed independently that 12 is the complete paged set, that every other cap matches its controller, that the attributes are runtime-inert, and that the unchanged TypeScript client and endpoint index are correct rather than a missed regen. Refs #633 Decisions-Edit: yes --- .../Controllers/OpenApiPagingContractTests.cs | 16 ++++++++++++---- ErsatzTV/Controllers/Api/SearchController.cs | 2 +- ErsatzTV/wwwroot/openapi/v1.json | 2 +- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs b/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs index 58b4426b3..3f1ccedf9 100644 --- a/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs +++ b/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs @@ -1,3 +1,4 @@ +using System.Text.RegularExpressions; using ErsatzTV.Tests.Support; using Microsoft.OpenApi; using NUnit.Framework; @@ -128,15 +129,22 @@ public class OpenApiPagingContractTests foreach ((string key, int cap) in expectedCaps) { - Description(key, "pageSize") - .ShouldContain($"capped at {cap}", Case.Insensitive, $"{key} pageSize should document a cap of {cap}"); + // Match the number as a WHOLE token, not a substring: "capped at 1000" contains + // "capped at 100", so a plain ShouldContain would pass a cap-100 endpoint whose + // description claims 1000 — the exact wrong-cap defect this test exists to catch. + Regex.IsMatch(Description(key, "pageSize"), $@"capped at {cap}(?!\d)", RegexOptions.IgnoreCase) + .ShouldBeTrue($"{key} pageSize should document a cap of exactly {cap}"); } } private static string Description(string key, string parameterName) { - IOpenApiParameter parameter = Find(key).Parameters! - .First(p => string.Equals(p.Name, parameterName, StringComparison.Ordinal)); + // Not `First(...)`: a missing parameter would throw "Sequence contains no matching element", + // which names neither the endpoint nor the parameter and reads as a broken test rather than + // the contract violation it is. + IOpenApiParameter parameter = (Find(key).Parameters ?? []) + .FirstOrDefault(p => string.Equals(p.Name, parameterName, StringComparison.Ordinal)) + .ShouldNotBeNull($"{key} should declare a {parameterName} parameter"); string? description = parameter.Description; description.ShouldNotBeNullOrWhiteSpace($"{key} {parameterName} should carry a description"); diff --git a/ErsatzTV/Controllers/Api/SearchController.cs b/ErsatzTV/Controllers/Api/SearchController.cs index 958452fab..1f9c39d86 100644 --- a/ErsatzTV/Controllers/Api/SearchController.cs +++ b/ErsatzTV/Controllers/Api/SearchController.cs @@ -71,7 +71,7 @@ public class SearchController(IMediator mediator) : ControllerBase public async Task SearchAllItems( [FromQuery] string query = "", [FromQuery] - [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0.")] + [Description("0-based page index: the first page is 0, not 1. A negative value is clamped to 0. Unlike the other paged endpoints this one is also bounded ABOVE, at 2000000, so that pageNum * pageSize cannot overflow; a larger value is clamped down to that maximum rather than rejected.")] int pageNum = 0, [FromQuery] [Description("Rows per page (default 500); capped at 1000 for this endpoint. The page offset is derived from the effective (capped) size, so a larger value narrows the page instead of widening the offset.")] diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 59d3ad438..383fd5bb5 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -17474,7 +17474,7 @@ { "name": "pageNum", "in": "query", - "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0.", + "description": "0-based page index: the first page is 0, not 1. A negative value is clamped to 0. Unlike the other paged endpoints this one is also bounded ABOVE, at 2000000, so that pageNum * pageSize cannot overflow; a larger value is clamped down to that maximum rather than rejected.", "schema": { "type": "integer", "format": "int32", -- 2.47.3 From 33e9abdd2048a1f8de020261a88a4bcb5c61fa23 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 11:31:11 +0200 Subject: [PATCH 3/3] fix(633): assert the cap set, not the presence of one true cap claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review finding, and a correction to what the previous commit claimed. That commit said the regex matched the cap "as a whole token" and called the result "exact". The whole-token part was true and did fix the 100-within-1000 substring hole. "Exact" was not: `capped at 100(?!\d)` asks only whether a correct claim is PRESENT, which is not the same as asking whether an incorrect one is ABSENT. A description reading "not capped at 1000 for this endpoint; capped at 100 …" satisfied it while publishing a wrong number to every consumer. Enumerate every `capped at ` in the description instead and require the set to be exactly one number, the right one. Mutation-verified on the constructed case: /logs naming both 1000 and 100 now reddens the test, where it passed under the previous form. This is the third round on this one assertion, and each round found the previous fix's blind spot rather than a fresh mistake — the failure mode was consistently "the new check tests presence of the right thing, not absence of the wrong thing." Note on verification: the reviewer could not run the suite (its sandbox could not create a temp dir, and a direct VSTest invocation could not bind its IPC socket), so it explicitly flagged the 1905/0 result as unverified rather than trusting it. That figure comes from my own run in this worktree, re-run after this change, and CI is the independent confirmation. Refs #633 Decisions-Edit: yes --- .../Controllers/OpenApiPagingContractTests.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs b/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs index 3f1ccedf9..bb7e9ebf2 100644 --- a/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs +++ b/ErsatzTV.Tests/Controllers/OpenApiPagingContractTests.cs @@ -129,11 +129,19 @@ public class OpenApiPagingContractTests foreach ((string key, int cap) in expectedCaps) { - // Match the number as a WHOLE token, not a substring: "capped at 1000" contains - // "capped at 100", so a plain ShouldContain would pass a cap-100 endpoint whose - // description claims 1000 — the exact wrong-cap defect this test exists to catch. - Regex.IsMatch(Description(key, "pageSize"), $@"capped at {cap}(?!\d)", RegexOptions.IgnoreCase) - .ShouldBeTrue($"{key} pageSize should document a cap of exactly {cap}"); + // Enumerate EVERY cap claim in the description and require the set to be exactly one + // number, the right one. Two weaker forms were rejected on the way here: + // - ShouldContain("capped at 100") is satisfied by the string "capped at 1000", so a + // cap-100 endpoint claiming 1000 passed — the very defect this test exists to catch. + // - Matching one occurrence as a whole token ("capped at 100(?!\d)") fixes that, but + // still passes a description that names a wrong cap somewhere ELSE in the sentence + // and the right one later. Presence of a true claim is not absence of a false one. + List claimedCaps = Regex + .Matches(Description(key, "pageSize"), @"capped at (\d+)", RegexOptions.IgnoreCase) + .Select(match => int.Parse(match.Groups[1].Value)) + .ToList(); + + claimedCaps.ShouldBe([cap], $"{key} pageSize should make exactly one cap claim, of {cap}"); } } -- 2.47.3