fix(616): document paging as 0-based, expose channelId on playout detail
#616 filed three MCP/API paging traps. Two were real; one was not, and one was already half-fixed on main. Verified each against the code before changing it. REAL — pageNum documented as 1-based. `ToolCatalog.Page()` described pageNum as "1-based page number" while every paged controller defaults it to 0, floors it with `Math.Max(0, pageNum)`, and skips `PageNum * PageSize`. A caller that trusted the description started at page 1 and silently lost the first page: no error, just a short set that reads as data loss rather than an off-by-one (it cost #487 a verification pass). Fixed in the description rather than by making the MCP layer 1-based: /api/v1 is additive-only post-freeze, 0-based is load-bearing in a dozen controllers and the SPA, and a 1-based wrapper over a 0-based API would make the same parameter name mean two different things on two surfaces a reader reads together. NOT REAL — "pageSize caps the page but the offset honors the requested value". Not reproducible on any endpoint. Every controller clamps before passing, every handler skips by the clamped size, and GetCollectionItemsHandler re-clamps defensively. The reported observation (pageSize=500&pageNum=2 on a 204-item collection returning 4 items) is exactly correct 0-based behaviour at the clamped width of 100 — page 2 is items 201-204. The issue's own trap-1 table states this. Pinned by test rather than "fixed". ALREADY FIXED — playout LIST rows gained channelId in #297 (2026-07-22), three days before #616 was filed; the report was measured against prod, which runs an older :prod image. The DETAIL response (PlayoutResponseModel) genuinely still lacked it, so channelId is added there (additive) and the reset_channel_playout argument now names the trap: the id spaces overlap numerically, so passing a playout id silently resets a different channel and returns a plausible 202. Tests, both mutation-verified (each fails when its fix is reverted): - ToolCatalogTests pins "0-based" on EVERY paged tool's pageNum description, with a non-empty guard so it can't pass vacuously over an empty tool set. - GetCollectionItemsHandlerTests pins 0-based page boundaries and proves the offset derives from the clamped pageSize (page 1 at pageSize=500 returns items 101-150; the mutation that honors 500 returns an empty page). Docs: new decision record api.paging-zero-based (catalog regenerated), the api-conventions paging bullet, and a Paging section in docs/mcp.md. OpenAPI v1.json + web/src/api/generated/v1.d.ts regenerated for the added field. fixes #616 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -8,6 +8,7 @@ public record PlayoutResponseModel(
|
||||
PlayoutScheduleKind ScheduleKind,
|
||||
string ChannelName,
|
||||
string ChannelNumber,
|
||||
int ChannelId,
|
||||
ChannelPlayoutMode PlayoutMode,
|
||||
string ScheduleName,
|
||||
string? ScheduleFile,
|
||||
@@ -23,6 +24,7 @@ public record PlayoutResponseModel(
|
||||
PlayoutScheduleKind scheduleKind,
|
||||
string channelName,
|
||||
string channelNumber,
|
||||
int channelId,
|
||||
ChannelPlayoutMode playoutMode,
|
||||
string scheduleName,
|
||||
string? scheduleFile,
|
||||
@@ -37,6 +39,7 @@ public record PlayoutResponseModel(
|
||||
scheduleKind,
|
||||
channelName,
|
||||
channelNumber,
|
||||
channelId,
|
||||
playoutMode,
|
||||
scheduleName,
|
||||
scheduleFile,
|
||||
|
||||
@@ -202,6 +202,35 @@ public class ToolCatalogTests
|
||||
(tool.QueryParameters ?? new HashSet<string>()).ShouldNotContain("ifMatch");
|
||||
}
|
||||
|
||||
// #616: the catalog described pageNum as "1-based" while every paged REST controller defaults it
|
||||
// to 0 and skips pageNum * pageSize. A caller that trusted the description started at page 1 and
|
||||
// silently lost the first page — no error, just a short set that reads like missing data. The
|
||||
// description is the whole contract an MCP client has, so it is pinned here for EVERY paged tool.
|
||||
[Test]
|
||||
public void Paged_Tools_Should_Document_PageNum_As_Zero_Based()
|
||||
{
|
||||
ToolDefinition[] paged = ToolCatalog.All
|
||||
.Where(t => t.InputSchema.RootElement.GetProperty("properties").TryGetProperty("pageNum", out _))
|
||||
.ToArray();
|
||||
|
||||
// Guard the guard: if the catalog stops exposing paged tools this test must fail loudly
|
||||
// rather than pass vacuously over an empty set.
|
||||
paged.ShouldNotBeEmpty();
|
||||
|
||||
foreach (ToolDefinition tool in paged)
|
||||
{
|
||||
string description = tool.InputSchema.RootElement
|
||||
.GetProperty("properties")
|
||||
.GetProperty("pageNum")
|
||||
.GetProperty("description")
|
||||
.GetString()
|
||||
.ShouldNotBeNull();
|
||||
|
||||
description.ShouldContain("0-based");
|
||||
description.ShouldNotContain("1-based");
|
||||
}
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Scan_Library_Tool_Should_Register_Deep_As_A_Query_Parameter()
|
||||
{
|
||||
|
||||
@@ -161,7 +161,12 @@ public static class ToolCatalog
|
||||
"ersatztv_reset_channel_playout",
|
||||
"Queue a rebuild of a channel's playout (202 Accepted; 409 if a build is already running).",
|
||||
"/api/v1/channels/{id}/playout/reset",
|
||||
[IdPath("Channel id.")],
|
||||
[
|
||||
IdPath(
|
||||
"CHANNEL id — not the playout id. The two id spaces overlap numerically, so "
|
||||
+ "passing a playout id here silently resets a different channel; take this "
|
||||
+ "value from a playout row's channelId field (issue #616).")
|
||||
],
|
||||
[Str("mode", "Optional playout build mode; omit for the default. GET a playout to see valid values.", arg: In.Query)]),
|
||||
Delete("ersatztv_delete_channel", "Delete a channel.", "/api/v1/channels/{id}", IdPath("Channel id.")),
|
||||
Put(
|
||||
@@ -230,10 +235,20 @@ public static class ToolCatalog
|
||||
private static Arg ObjArray(string name, string description, bool required = false) =>
|
||||
new(name, "array", description, required, In.Body, ItemType: "object");
|
||||
|
||||
// Paging mirrors the REST API it wraps, which is 0-BASED everywhere (issue #616): every paged
|
||||
// controller defaults pageNum to 0 and skips `pageNum * pageSize`. The description said "1-based",
|
||||
// so a caller that trusted it silently skipped the first page and read the result as data loss.
|
||||
// Documented rather than translated: a 1-based MCP over a 0-based API would make the same
|
||||
// parameter name mean two different things depending on which surface you were reading.
|
||||
// pageSize is clamped server-side (100 on most endpoints), so an over-large value yields
|
||||
// clamped-width pages — it does not widen the offset.
|
||||
private static Arg[] Page() =>
|
||||
[
|
||||
Int("pageNum", "1-based page number (optional).", arg: In.Query),
|
||||
Int("pageSize", "Page size (optional).", arg: In.Query)
|
||||
Int("pageNum", "0-based page number; the first page is 0 (optional, default 0).", arg: In.Query),
|
||||
Int(
|
||||
"pageSize",
|
||||
"Page size (optional). Clamped server-side, so pages may be narrower than requested.",
|
||||
arg: In.Query)
|
||||
];
|
||||
|
||||
// The channel create/update body (CreateChannelRequest / UpdateChannelRequest — the id comes from
|
||||
|
||||
@@ -61,6 +61,93 @@ public class GetCollectionItemsHandlerTests
|
||||
page.Page.Select(i => i.Title).ShouldBe(["Zeta", "Alpha", "Beta"]);
|
||||
}
|
||||
|
||||
// Paging semantics, pinned because #616 reported them as two bugs that measurement did not
|
||||
// support. pageNum is 0-BASED (the trap: the MCP catalog documented it as 1-based, so a caller
|
||||
// starting at 1 silently skipped the first page and read a short set as data loss).
|
||||
[Test]
|
||||
public async Task Handle_Should_Treat_PageNum_As_Zero_Based()
|
||||
{
|
||||
await SeedNumberedCollection(150);
|
||||
var handler = new GetCollectionItemsHandler(_db.Factory);
|
||||
|
||||
Either<BaseError, PagedLibraryBrowseItemsResponseModel> first =
|
||||
await handler.Handle(new GetCollectionItems(10, 0, 10), CancellationToken.None);
|
||||
Either<BaseError, PagedLibraryBrowseItemsResponseModel> second =
|
||||
await handler.Handle(new GetCollectionItems(10, 1, 10), CancellationToken.None);
|
||||
|
||||
// Page 0 is the FIRST page, not a skipped one; page 1 is the second.
|
||||
first.RightToSeq().Single().Page.Select(i => i.Title).First().ShouldBe("Item 001");
|
||||
second.RightToSeq().Single().Page.Select(i => i.Title).First().ShouldBe("Item 011");
|
||||
}
|
||||
|
||||
// The second half of #616's claim was that an over-large pageSize caps the returned page but
|
||||
// leaves the OFFSET computed from the requested value, so page 1 at pageSize=500 would land past
|
||||
// item 500. It does not: the size is clamped first and the offset derives from the clamped value.
|
||||
[Test]
|
||||
public async Task Handle_Should_Derive_Offset_From_The_Clamped_PageSize()
|
||||
{
|
||||
await SeedNumberedCollection(150);
|
||||
var handler = new GetCollectionItemsHandler(_db.Factory);
|
||||
|
||||
// pageSize 500 clamps to 100, so page 1 starts at item 101 and runs to the end (50 items).
|
||||
// If the offset honored the requested 500, this page would start past the collection and be
|
||||
// empty — which is exactly what the mutation of this fix produces.
|
||||
Either<BaseError, PagedLibraryBrowseItemsResponseModel> result =
|
||||
await handler.Handle(new GetCollectionItems(10, 1, 500), CancellationToken.None);
|
||||
|
||||
PagedLibraryBrowseItemsResponseModel page = result.RightToSeq().Single();
|
||||
page.TotalCount.ShouldBe(150);
|
||||
page.Page.Count.ShouldBe(50);
|
||||
page.Page.Select(i => i.Title).First().ShouldBe("Item 101");
|
||||
page.Page.Select(i => i.Title).Last().ShouldBe("Item 150");
|
||||
}
|
||||
|
||||
private async Task SeedNumberedCollection(int count)
|
||||
{
|
||||
await using TvContext context = _db.CreateContext();
|
||||
|
||||
var library = new LocalLibrary
|
||||
{
|
||||
Id = 1,
|
||||
Name = "Library",
|
||||
MediaKind = LibraryMediaKind.Movies,
|
||||
Paths = []
|
||||
};
|
||||
var path = new LibraryPath
|
||||
{
|
||||
Id = 1,
|
||||
Path = "/media",
|
||||
Library = library,
|
||||
LibraryFolders = [],
|
||||
MediaItems = []
|
||||
};
|
||||
library.Paths.Add(path);
|
||||
|
||||
var collection = new Collection
|
||||
{
|
||||
Id = 10,
|
||||
Name = "Manual",
|
||||
UseCustomPlaybackOrder = false,
|
||||
MediaItems = [],
|
||||
CollectionItems = [],
|
||||
MultiCollections = [],
|
||||
MultiCollectionItems = []
|
||||
};
|
||||
|
||||
var movies = new List<Movie>();
|
||||
for (var i = 1; i <= count; i++)
|
||||
{
|
||||
// Zero-padded so the handler's title ordering matches numeric order.
|
||||
movies.Add(MakeMovie(1000 + i, path, $"Item {i:D3}"));
|
||||
collection.CollectionItems.Add(new CollectionItem { MediaItemId = 1000 + i });
|
||||
}
|
||||
|
||||
context.LocalLibraries.Add(library);
|
||||
context.Movies.AddRange(movies);
|
||||
context.Collections.Add(collection);
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
private async Task SeedCollectionGraph(bool useCustomPlaybackOrder)
|
||||
{
|
||||
await using TvContext context = _db.CreateContext();
|
||||
|
||||
@@ -166,6 +166,25 @@ public class PlayoutControllerTests
|
||||
.Seed.ShouldBe(4242);
|
||||
}
|
||||
|
||||
// #616: the playout DETAIL response carried channelName/channelNumber but no channelId, while
|
||||
// reset_channel_playout takes a CHANNEL id. The two id spaces overlap numerically, so a caller
|
||||
// that reached for the row's `id` reset a different channel and got a plausible 202 back. The
|
||||
// list rows gained channelId in #297; this pins the same field on the detail response, and the
|
||||
// distinct ids below prove it is the channel's, not the playout's.
|
||||
[Test]
|
||||
public async Task GetById_Should_Surface_ChannelId_Distinct_From_PlayoutId()
|
||||
{
|
||||
_mediator.Send(Arg.Any<GetPlayoutById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlayoutNameViewModel>.Some(MakePlayout(9) with { ChannelId = 400 }));
|
||||
|
||||
IActionResult result = await _controller.GetById(9, CancellationToken.None);
|
||||
|
||||
PlayoutResponseModel body = result.ShouldBeOfType<OkObjectResult>()
|
||||
.Value.ShouldBeOfType<PlayoutResponseModel>();
|
||||
body.Id.ShouldBe(9);
|
||||
body.ChannelId.ShouldBe(400);
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task GetAll_Should_Surface_Seed()
|
||||
{
|
||||
@@ -1432,6 +1451,7 @@ public class PlayoutControllerTests
|
||||
vm.ScheduleKind,
|
||||
vm.ChannelName,
|
||||
vm.ChannelNumber,
|
||||
vm.ChannelId,
|
||||
vm.PlayoutMode,
|
||||
vm.ScheduleName,
|
||||
vm.ScheduleFile,
|
||||
|
||||
@@ -818,6 +818,7 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) :
|
||||
vm.ScheduleKind,
|
||||
vm.ChannelName,
|
||||
vm.ChannelNumber,
|
||||
vm.ChannelId,
|
||||
vm.PlayoutMode,
|
||||
vm.ScheduleName,
|
||||
vm.ScheduleFile,
|
||||
|
||||
@@ -28954,6 +28954,7 @@
|
||||
"scheduleKind",
|
||||
"channelName",
|
||||
"channelNumber",
|
||||
"channelId",
|
||||
"playoutMode",
|
||||
"scheduleName",
|
||||
"scheduleFile",
|
||||
@@ -28979,6 +28980,10 @@
|
||||
"channelNumber": {
|
||||
"type": "string"
|
||||
},
|
||||
"channelId": {
|
||||
"type": "integer",
|
||||
"format": "int32"
|
||||
},
|
||||
"playoutMode": {
|
||||
"$ref": "#/components/schemas/ChannelPlayoutMode"
|
||||
},
|
||||
|
||||
@@ -41,7 +41,12 @@ Exemplars:
|
||||
- **Paged GET with clamped params**: `ErsatzTV/Controllers/Api/LogsController.cs` —
|
||||
`pageNum` clamped via `Math.Max(0, pageNum)`, `pageSize` via `Math.Clamp(pageSize, 1, MaxPageSize)`
|
||||
(`MaxPageSize = 100`). Any new paged endpoint should clamp the same way — don't trust client
|
||||
input for page math.
|
||||
input for page math. **`pageNum` is 0-based** across the whole surface (the first page is `0`) and
|
||||
the offset is always derived from the *clamped* `pageSize`, so an over-large `pageSize` yields
|
||||
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`.
|
||||
- **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
|
||||
|
||||
@@ -19,6 +19,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `api.logs-sort-params` | `GET /api/logs` takes allow-listed `sortField` (`timestamp`\|`level`) and `sortDirection` (`asc`\|`desc`) query params, normalized (not rejected) on an unrecognized value. | 2026-07-11 | [link](records/api/logs-sort-params.md) |
|
||||
| `api.mediatr-passthrough` | The REST API is thin controllers over existing MediatR handlers, with no new service/business-logic layer. | 2026-06 | [link](records/api/mediatr-passthrough.md) |
|
||||
| `api.openapi-mirrors-runtime` | The generated OpenAPI spec is made to match the runtime Newtonsoft wire contract (via `NewtonsoftSchemaNamingTransformer`), not the reverse. | 2026-07-09 | [link](records/api/openapi-mirrors-runtime.md) |
|
||||
| `api.paging-zero-based` | `pageNum` is 0-based across the entire `/api/v1` surface and every wrapper of it (MCP tool catalog, SPA hooks, docs); the page offset is always derived from the CLAMPED `pageSize`, so an over-large `pageSize` narrows the page without widening the offset. A paging parameter description that omits or contradicts "0-based" is a defect. | 2026-07-25 | [link](records/api/paging-zero-based.md) |
|
||||
| `api.parentid-drillin` | Media drill-in (season/episode/artist/music-video) is served by an optional `parentId` query param on library-browse, not dedicated per-kind child-listing endpoints. | 2026-07-07 | [link](records/api/parentid-drillin.md) |
|
||||
| `api.playout-build-lock-409` | Every id-keyed playout/channel mutation endpoint checks `IEntityLocker.IsPlayoutLocked(id)` and returns 409 Conflict while a build is in-flight, mirroring Blazor's disabled-buttons behavior; reset-all stays 202 and silently skips locked playouts. | 2026-07-10 | [link](records/api/playout-build-lock-409.md) |
|
||||
| `api.postcommit-cancellation-none` | Once a mutation commits, its entire compensating side effect (enqueues, publishes, reindexes, cache refresh, and any post-commit lookup gating one of those) runs on `CancellationToken.None` so a late client disconnect can't half-abort an already-committed change. | 2026-07-11 | [link](records/api/postcommit-cancellation-none.md) |
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
---
|
||||
key: api.paging-zero-based
|
||||
title: 2026-07-25 — Paging is 0-based everywhere, and every wrapper says so (#616)
|
||||
status: active
|
||||
since: '2026-07-25'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: '`pageNum` is 0-based across the entire `/api/v1` surface and every wrapper of it (MCP tool catalog, SPA hooks, docs); the page offset is always derived from the CLAMPED `pageSize`, so an over-large `pageSize` narrows the page without widening the offset. A paging parameter description that omits or contradicts "0-based" is a defect.'
|
||||
signals: '`pageNum`, `pageSize`, `Math.Clamp(pageSize`, `Skip(pageNum * pageSize)`, "1-based", off-by-one paging, short result set, MCP `Page()` · paths: `ErsatzTV.Mcp/ToolCatalog.cs`, `ErsatzTV/Controllers/Api/*Controller.cs` · issues: #616, #487, #58'
|
||||
mechanics: '`Math.Max(0, pageNum)` + `Math.Clamp(pageSize, 1, MaxPageSize)` in the controller, `Skip(PageNum * PageSize)` in the handler'
|
||||
---
|
||||
|
||||
Every paged controller on `/api/v1` defaults `pageNum` to `0`, floors it with `Math.Max(0, pageNum)`,
|
||||
clamps `pageSize` with `Math.Clamp(pageSize, 1, MaxPageSize)`, and passes **both clamped values** to a
|
||||
handler that skips `PageNum * PageSize`. That makes paging uniformly 0-based, and makes the offset a
|
||||
function of the clamped size rather than the requested one.
|
||||
|
||||
The convention was correct in code and unwritten everywhere else, which is how it produced a bug
|
||||
report. The MCP tool catalog described `pageNum` as "1-based page number", so a caller that started
|
||||
at `pageNum=1` skipped the first page: a 15-item collection returned 0 items and a 204-item
|
||||
collection returned 104. Nothing errored — the caller just got a short set, which reads as *data
|
||||
loss*, not as an off-by-one, and cost a verification pass being chased as one (#487).
|
||||
|
||||
The same report's second claim — that `pageSize` is capped for the returned page while the offset
|
||||
still honours the requested value — was **not** reproducible and is not true of any endpoint. The
|
||||
observation behind it (`pageSize=500&pageNum=2` on a 204-item collection returning 4 items) is
|
||||
exactly correct 0-based behaviour at the clamped width of 100: page 2 is items 201–204. Both
|
||||
behaviours are now pinned by mutation-verified tests in `GetCollectionItemsHandlerTests`, so the next
|
||||
reader does not have to re-derive which half was real.
|
||||
|
||||
**Direction of the fix.** The alternative was to make the MCP layer 1-based and translate. Rejected:
|
||||
`/api/v1` is additive-only post-freeze (`api.versioning-v1`), 0-based is already load-bearing in a
|
||||
dozen controllers and the SPA, and a 1-based wrapper over a 0-based API would make the *same
|
||||
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.
|
||||
|
||||
**Corollary — ids in paged rows.** A row that names a related entity must expose that entity's id, not
|
||||
only its display fields. `reset_channel_playout` takes a *channel* id while playout rows exposed only
|
||||
the playout `id` plus channel name/number; the id spaces overlap numerically, so passing the row's id
|
||||
silently reset a different channel and returned a plausible 202. List rows gained `channelId` in #297;
|
||||
#616 added it to `PlayoutResponseModel` (the detail response) and named the trap in the MCP argument
|
||||
description.
|
||||
+16
@@ -84,6 +84,22 @@ When a response carries an `ETag` header (versioned aggregates emit it on GET an
|
||||
replace PUT), the tool result appends a `\n[etag: "N"]` marker so an agent can round-trip it as
|
||||
`ifMatch` on a subsequent write.
|
||||
|
||||
### Paging (`api.paging-zero-based`)
|
||||
|
||||
The paged tools take `pageNum`/`pageSize` and pass them through unchanged — the MCP layer does **not**
|
||||
translate them, so they mean exactly what they mean on `/api/v1`:
|
||||
|
||||
- **`pageNum` is 0-based.** The first page is `0`. Starting at `1` silently skips a page and returns a
|
||||
short set with no error, which is easy to misread as missing data (ersatztv#616 — the catalog used
|
||||
to describe it as 1-based, and #487 lost a verification pass to it).
|
||||
- **`pageSize` is clamped server-side** (100 on most endpoints, 200 on a few). An over-large value
|
||||
gives you narrower pages, not a wider offset: `pageSize=500&pageNum=1` returns items 101–200, not
|
||||
501–1000. Page to completeness against `totalCount` rather than assuming your requested size held.
|
||||
|
||||
Rows that reference another entity carry that entity's id, not just its display fields — take the
|
||||
channel id for `ersatztv_reset_channel_playout` from a playout row's `channelId`, never from its `id`
|
||||
(that is the *playout* id, and the two id spaces overlap numerically).
|
||||
|
||||
### Optimistic concurrency (`api-conventions.md` §7a)
|
||||
|
||||
Only the **replace-all aggregate PUTs** honor `If-Match` — here that is
|
||||
|
||||
Vendored
+1
@@ -1168,6 +1168,7 @@ export interface components {
|
||||
"scheduleKind": components["schemas"]["PlayoutScheduleKind"];
|
||||
"channelName": string;
|
||||
"channelNumber": string;
|
||||
"channelId": number;
|
||||
"playoutMode": components["schemas"]["ChannelPlayoutMode"];
|
||||
"scheduleName": string;
|
||||
"scheduleFile": null | string;
|
||||
|
||||
Reference in New Issue
Block a user