From c40e78d8409d76de0dcdf2cd1018aead9a8e2909 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 12:08:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(api):=20#197=20Bundle=20C=20review=20nits?= =?UTF-8?q?=20=E2=80=94=20order-independent=20operationIds=20+=20nullable?= =?UTF-8?q?=20MediaSources=20fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs #287 #288 #197 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PathReplacementResponseModel.cs | 2 +- .../RemoteLibraryResponseModel.cs | 2 +- .../RemoteMediaSourceItemResponseModel.cs | 2 +- .../OpenApiContractHonestyTests.cs | 36 ++++++++ .../ApiSecuritySchemeDocumentTransformer.cs | 29 +++++++ .../OperationIdOpenApiTransformer.cs | 86 ++++++++++++++++--- ErsatzTV/wwwroot/openapi/v1.json | 37 +++++--- docs/api-conventions.md | 6 +- docs/endpoint-index.md | 12 +-- web/src/api/generated/v1.d.ts | 10 +-- 10 files changed, 182 insertions(+), 40 deletions(-) diff --git a/ErsatzTV.Core/Api/MediaSources/PathReplacementResponseModel.cs b/ErsatzTV.Core/Api/MediaSources/PathReplacementResponseModel.cs index 301e2ba6e..b0ff54d26 100644 --- a/ErsatzTV.Core/Api/MediaSources/PathReplacementResponseModel.cs +++ b/ErsatzTV.Core/Api/MediaSources/PathReplacementResponseModel.cs @@ -1,4 +1,4 @@ #nullable enable namespace ErsatzTV.Core.Api.MediaSources; -public record PathReplacementResponseModel(int Id, string RemotePath, string LocalPath); +public record PathReplacementResponseModel(int Id, string? RemotePath, string? LocalPath); diff --git a/ErsatzTV.Core/Api/MediaSources/RemoteLibraryResponseModel.cs b/ErsatzTV.Core/Api/MediaSources/RemoteLibraryResponseModel.cs index 5cc37b5de..76e5ce918 100644 --- a/ErsatzTV.Core/Api/MediaSources/RemoteLibraryResponseModel.cs +++ b/ErsatzTV.Core/Api/MediaSources/RemoteLibraryResponseModel.cs @@ -3,4 +3,4 @@ using ErsatzTV.Core.Domain; namespace ErsatzTV.Core.Api.MediaSources; -public record RemoteLibraryResponseModel(int Id, string Name, LibraryMediaKind MediaKind, bool ShouldSyncItems); +public record RemoteLibraryResponseModel(int Id, string? Name, LibraryMediaKind MediaKind, bool ShouldSyncItems); diff --git a/ErsatzTV.Core/Api/MediaSources/RemoteMediaSourceItemResponseModel.cs b/ErsatzTV.Core/Api/MediaSources/RemoteMediaSourceItemResponseModel.cs index ad211e7ba..ef032337d 100644 --- a/ErsatzTV.Core/Api/MediaSources/RemoteMediaSourceItemResponseModel.cs +++ b/ErsatzTV.Core/Api/MediaSources/RemoteMediaSourceItemResponseModel.cs @@ -1,4 +1,4 @@ #nullable enable namespace ErsatzTV.Core.Api.MediaSources; -public record RemoteMediaSourceItemResponseModel(int Id, string Name, string Address); +public record RemoteMediaSourceItemResponseModel(int Id, string? Name, string? Address); diff --git a/ErsatzTV.Tests/Controllers/OpenApiContractHonestyTests.cs b/ErsatzTV.Tests/Controllers/OpenApiContractHonestyTests.cs index fe6aa2b7d..da94713c7 100644 --- a/ErsatzTV.Tests/Controllers/OpenApiContractHonestyTests.cs +++ b/ErsatzTV.Tests/Controllers/OpenApiContractHonestyTests.cs @@ -71,6 +71,42 @@ public class OpenApiContractHonestyTests } } + [Test] + public void Colliding_Troubleshoot_HeadGet_Pairs_Should_Carry_VerbDerived_OperationIds() + { + // The 3 Troubleshoot HEAD/GET pairs share a controller+action and differ only by verb. Their + // synthesized operationIds must be verb-suffixed (structural, order-independent) — NOT one bare + // base + one arbitrary suffix that depends on ApiExplorer visitation order. See #197 Bundle C. + var ids = EnumerateOperations().Select(op => op.Operation.OperationId!).ToHashSet(StringComparer.Ordinal); + + foreach (string expected in new[] + { + "TroubleshootTroubleshootPlaybackGet", + "TroubleshootTroubleshootPlaybackHead", + "TroubleshootTroubleshootPlaybackArchiveGet", + "TroubleshootTroubleshootPlaybackArchiveHead", + "TroubleshootTroubleshootPlaybackSampleGet", + "TroubleshootTroubleshootPlaybackSampleHead" + }) + { + ids.ShouldContain(expected); + } + + // The old order-dependent forms (bare base + "...GET" suffix) must be gone. + foreach (string stale in new[] + { + "TroubleshootTroubleshootPlayback", + "TroubleshootTroubleshootPlaybackArchive", + "TroubleshootTroubleshootPlaybackSample", + "TroubleshootTroubleshootPlaybackGET", + "TroubleshootTroubleshootPlaybackArchiveGET", + "TroubleshootTroubleshootPlaybackSampleGET" + }) + { + ids.ShouldNotContain(stale); + } + } + [Test] public void ValidationProblemDetails_Schema_And_400_Should_Be_Documented_For_Binding_Operations() { diff --git a/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs b/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs index 21b3623fb..6b00164f1 100644 --- a/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs +++ b/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs @@ -15,6 +15,7 @@ namespace ErsatzTV.Serialization; public static class ApiSecuritySchemeDocumentTransformer { public const string ValidationProblemDetailsSchemaId = "ValidationProblemDetails"; + public const string ProblemDetailsSchemaId = "ProblemDetails"; public static Task TransformAsync( OpenApiDocument document, @@ -40,9 +41,37 @@ public static class ApiSecuritySchemeDocumentTransformer document.Components.Schemas[ValidationProblemDetailsSchemaId] = BuildValidationProblemDetailsSchema(); } + // The injected 401 (ApiSecurityOperationTransformer) references the ProblemDetails schema. It + // normally already exists because other actions declare ProducesResponseType(typeof(ProblemDetails)), + // but self-provision it if absent so the 401 reference never dangles (belt-and-suspenders). + if (!document.Components.Schemas.ContainsKey(ProblemDetailsSchemaId)) + { + document.Components.Schemas[ProblemDetailsSchemaId] = BuildProblemDetailsSchema(); + } + return Task.CompletedTask; } + // The RFC 7807 shape ASP.NET Core returns for a plain ProblemDetails response (the ValidationProblemDetails + // members minus the "errors" map). + private static OpenApiSchema BuildProblemDetailsSchema() => + new() + { + Type = JsonSchemaType.Object, + Properties = new Dictionary + { + ["type"] = new OpenApiSchema { Type = JsonSchemaType.String | JsonSchemaType.Null }, + ["title"] = new OpenApiSchema { Type = JsonSchemaType.String | JsonSchemaType.Null }, + ["status"] = new OpenApiSchema + { + Type = JsonSchemaType.Integer | JsonSchemaType.Null, + Format = "int32" + }, + ["detail"] = new OpenApiSchema { Type = JsonSchemaType.String | JsonSchemaType.Null }, + ["instance"] = new OpenApiSchema { Type = JsonSchemaType.String | JsonSchemaType.Null } + } + }; + // The RFC 7807 shape ASP.NET Core's [ApiController] + FluentValidation return on a model-binding / // validation failure: the standard ProblemDetails members plus an "errors" map of field -> messages. private static OpenApiSchema BuildValidationProblemDetailsSchema() => diff --git a/ErsatzTV/Serialization/OperationIdOpenApiTransformer.cs b/ErsatzTV/Serialization/OperationIdOpenApiTransformer.cs index e8cbf74b0..1d22ccd38 100644 --- a/ErsatzTV/Serialization/OperationIdOpenApiTransformer.cs +++ b/ErsatzTV/Serialization/OperationIdOpenApiTransformer.cs @@ -1,7 +1,9 @@ #nullable enable +using Microsoft.AspNetCore.Mvc.ApiExplorer; using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.OpenApi; using Microsoft.OpenApi; +using HashSet = System.Collections.Generic.HashSet; namespace ErsatzTV.Serialization; @@ -13,15 +15,26 @@ namespace ErsatzTV.Serialization; /// (ControllerName + ActionName) whenever the /// id is null/empty, and never rename an id the developer set explicitly. /// -/// A single instance is created per document generation and reused across every operation, so the -/// set gives whole-document uniqueness: a synthesized id that collides is -/// disambiguated deterministically (append the HTTP method, then a counter). Explicit ids are also -/// recorded so a later synthesized id can't shadow one. +/// Disambiguation is structural, not visitation-order-dependent: the colliding cases are the +/// HEAD/GET pairs that share a controller+action but differ only by verb. We precompute (from the +/// full , filtered to the same +/// general group the "v1" document includes) the set of synthesized base ids that 2+ operations +/// share, and suffix each such operation by its HTTP verb (<base>Get, +/// <base>Head). A base id that is unique across the document stays unsuffixed. The result +/// is a pure function of (controller, action, verb): an SDK/framework change to discovery order can +/// no longer rename a generated client method. Explicit ids are recorded so a synthesized id can't +/// shadow one; a defensive counter remains a last-resort backstop for an impossible residual clash. /// /// public sealed class OperationIdOpenApiTransformer : IOpenApiOperationTransformer { - private readonly System.Collections.Generic.HashSet _used = new(StringComparer.Ordinal); + private const string GeneralGroupName = "general"; + + private readonly HashSet _used = new(StringComparer.Ordinal); + private readonly HashSet _collidingBaseIds; + + public OperationIdOpenApiTransformer(IApiDescriptionGroupCollectionProvider descriptionProvider) + => _collidingBaseIds = ComputeCollidingBaseIds(descriptionProvider); public Task TransformAsync( OpenApiOperation operation, @@ -41,22 +54,67 @@ public sealed class OperationIdOpenApiTransformer : IOpenApiOperationTransformer } string baseId = controllerAction.ControllerName + controllerAction.ActionName; - string candidate = baseId; - if (_used.Contains(candidate)) - { - // Overloaded action (same controller+action name, different verb/route): qualify by verb. - candidate = baseId + (context.Description.HttpMethod ?? string.Empty); - } + // Structural, order-independent: if 2+ synthesized operations share this base id, suffix EACH + // by its verb; otherwise keep the bare base. Pure function of (controller, action, verb). + string candidate = _collidingBaseIds.Contains(baseId) + ? baseId + PascalCaseVerb(context.Description.HttpMethod) + : baseId; + // Defensive backstop only — with verb suffixing this cannot fire for the known document, but + // guarantees uniqueness if some future shape produces an unexpected clash. int suffix = 2; - while (!_used.Add(candidate)) + string finalId = candidate; + while (!_used.Add(finalId)) { - candidate = baseId + (context.Description.HttpMethod ?? string.Empty) + suffix; + finalId = candidate + suffix; suffix++; } - operation.OperationId = candidate; + operation.OperationId = finalId; return Task.CompletedTask; } + + private static HashSet ComputeCollidingBaseIds(IApiDescriptionGroupCollectionProvider provider) + { + var counts = new Dictionary(StringComparer.Ordinal); + foreach (ApiDescriptionGroup group in provider.ApiDescriptionGroups.Items) + { + foreach (ApiDescription description in group.Items) + { + // Mirror the "v1" document's ShouldInclude predicate. + if (description.GroupName != GeneralGroupName) + { + continue; + } + + if (description.ActionDescriptor is not ControllerActionDescriptor controllerAction) + { + continue; + } + + // Only operations that will actually be *synthesized* participate — an explicit + // Name= sets the route name (the framework's operationId), so it is excluded. + if (!string.IsNullOrEmpty(controllerAction.AttributeRouteInfo?.Name)) + { + continue; + } + + string baseId = controllerAction.ControllerName + controllerAction.ActionName; + counts[baseId] = counts.TryGetValue(baseId, out int c) ? c + 1 : 1; + } + } + + return counts.Where(kvp => kvp.Value >= 2).Select(kvp => kvp.Key).ToHashSet(StringComparer.Ordinal); + } + + private static string PascalCaseVerb(string? httpMethod) + { + if (string.IsNullOrEmpty(httpMethod)) + { + return string.Empty; + } + + return char.ToUpperInvariant(httpMethod[0]) + httpMethod[1..].ToLowerInvariant(); + } } diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 7d7765ad2..3838d6143 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -21065,7 +21065,7 @@ "Troubleshooting" ], "summary": "Start a troubleshooting playback session", - "operationId": "TroubleshootTroubleshootPlayback", + "operationId": "TroubleshootTroubleshootPlaybackHead", "parameters": [ { "name": "mediaItem", @@ -21243,7 +21243,7 @@ "Troubleshooting" ], "summary": "Start a troubleshooting playback session", - "operationId": "TroubleshootTroubleshootPlaybackGET", + "operationId": "TroubleshootTroubleshootPlaybackGet", "parameters": [ { "name": "mediaItem", @@ -21423,7 +21423,7 @@ "Troubleshooting" ], "summary": "Download the last troubleshooting playback session archive", - "operationId": "TroubleshootTroubleshootPlaybackArchive", + "operationId": "TroubleshootTroubleshootPlaybackArchiveHead", "responses": { "200": { "description": "OK" @@ -21448,7 +21448,7 @@ "Troubleshooting" ], "summary": "Download the last troubleshooting playback session archive", - "operationId": "TroubleshootTroubleshootPlaybackArchiveGET", + "operationId": "TroubleshootTroubleshootPlaybackArchiveGet", "responses": { "200": { "description": "OK" @@ -21475,7 +21475,7 @@ "Troubleshooting" ], "summary": "Download a media sample archive for troubleshooting", - "operationId": "TroubleshootTroubleshootPlaybackSample", + "operationId": "TroubleshootTroubleshootPlaybackSampleHead", "parameters": [ { "name": "mediaItemId", @@ -21521,7 +21521,7 @@ "Troubleshooting" ], "summary": "Download a media sample archive for troubleshooting", - "operationId": "TroubleshootTroubleshootPlaybackSampleGET", + "operationId": "TroubleshootTroubleshootPlaybackSampleGet", "parameters": [ { "name": "mediaItemId", @@ -27446,10 +27446,16 @@ "format": "int32" }, "remotePath": { - "type": "string" + "type": [ + "null", + "string" + ] }, "localPath": { - "type": "string" + "type": [ + "null", + "string" + ] } } }, @@ -28540,7 +28546,10 @@ "format": "int32" }, "name": { - "type": "string" + "type": [ + "null", + "string" + ] }, "mediaKind": { "$ref": "#/components/schemas/LibraryMediaKind" @@ -28563,10 +28572,16 @@ "format": "int32" }, "name": { - "type": "string" + "type": [ + "null", + "string" + ] }, "address": { - "type": "string" + "type": [ + "null", + "string" + ] } } }, diff --git a/docs/api-conventions.md b/docs/api-conventions.md index ecffc78cb..7ee695f02 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -536,7 +536,11 @@ the runtime filter enforces, so the spec can never drift from enforcement. The d against the effective default (`Api:RequireKeyForReads=true`), under which every documented operation requires the key. Two companion transformers run on the "v1" document only: `OperationIdOpenApiTransformer` synthesizes a stable `operationId` (from controller+action) for the ~90 -operations that lacked a `Name=`, and `ValidationProblemOperationTransformer` documents the +operations that lacked a `Name=` — and disambiguates the HEAD/GET pairs that share a controller+action +**structurally, independent of ApiExplorer visitation order** (#197 Bundle C): when 2+ synthesized ops +share a base id it suffixes **each** by its verb (`…Get`/`…Head`), so discovery-order churn can't rename +a generated client method (a base id unique across the document stays unsuffixed; explicit `Name=` ids are +never touched). `ValidationProblemOperationTransformer` documents the `400 ValidationProblemDetails` a model-binding/FluentValidation failure actually returns for any body/param-binding operation. All four are registered in `Startup.cs` on "v1" only, mirroring `NewtonsoftSchemaNamingTransformer`. `/api/v1` route versioning remains #286. diff --git a/docs/endpoint-index.md b/docs/endpoint-index.md index 385e6d786..a21d04d56 100644 --- a/docs/endpoint-index.md +++ b/docs/endpoint-index.md @@ -415,12 +415,12 @@ | Method | Path | Operation | Summary | |---|---|---|---| | GET | `/api/troubleshoot/info` | GetTroubleshootingInfo | Get troubleshooting diagnostic info | -| GET | `/api/troubleshoot/playback.m3u8` | TroubleshootTroubleshootPlaybackGET | Start a troubleshooting playback session | -| HEAD | `/api/troubleshoot/playback.m3u8` | TroubleshootTroubleshootPlayback | Start a troubleshooting playback session | -| GET | `/api/troubleshoot/playback/archive` | TroubleshootTroubleshootPlaybackArchiveGET | Download the last troubleshooting playback session archive | -| HEAD | `/api/troubleshoot/playback/archive` | TroubleshootTroubleshootPlaybackArchive | Download the last troubleshooting playback session archive | -| GET | `/api/troubleshoot/playback/sample/{mediaItemId}` | TroubleshootTroubleshootPlaybackSampleGET | Download a media sample archive for troubleshooting | -| HEAD | `/api/troubleshoot/playback/sample/{mediaItemId}` | TroubleshootTroubleshootPlaybackSample | Download a media sample archive for troubleshooting | +| GET | `/api/troubleshoot/playback.m3u8` | TroubleshootTroubleshootPlaybackGet | Start a troubleshooting playback session | +| HEAD | `/api/troubleshoot/playback.m3u8` | TroubleshootTroubleshootPlaybackHead | Start a troubleshooting playback session | +| GET | `/api/troubleshoot/playback/archive` | TroubleshootTroubleshootPlaybackArchiveGet | Download the last troubleshooting playback session archive | +| HEAD | `/api/troubleshoot/playback/archive` | TroubleshootTroubleshootPlaybackArchiveHead | Download the last troubleshooting playback session archive | +| GET | `/api/troubleshoot/playback/sample/{mediaItemId}` | TroubleshootTroubleshootPlaybackSampleGet | Download a media sample archive for troubleshooting | +| HEAD | `/api/troubleshoot/playback/sample/{mediaItemId}` | TroubleshootTroubleshootPlaybackSampleHead | Download a media sample archive for troubleshooting | | GET | `/api/troubleshoot/playback/status` | GetTroubleshootingPlaybackStatus | Get the status of the current or last troubleshooting playback session | | GET | `/api/troubleshoot/playback/stream-selectors` | GetTroubleshootingStreamSelectors | List available channel stream selectors | | GET | `/api/troubleshoot/playback/subtitles/{mediaItemId}` | GetTroubleshootingSubtitles | List selectable subtitle streams for a media item | diff --git a/web/src/api/generated/v1.d.ts b/web/src/api/generated/v1.d.ts index b55bf47c6..e43b190d1 100644 --- a/web/src/api/generated/v1.d.ts +++ b/web/src/api/generated/v1.d.ts @@ -956,8 +956,8 @@ export interface components { }; "PathReplacementResponseModel": { "id": number; - "remotePath": string; - "localPath": string; + "remotePath": null | string; + "localPath": null | string; }; "PlaybackOrder": "None" | "Chronological" | "Random" | "Shuffle" | "ShuffleInOrder" | "MultiEpisodeShuffle" | "SeasonEpisode" | "RandomRotation" | "Marathon"; "PlaylistGroupResponseModel": { @@ -1164,14 +1164,14 @@ export interface components { }; "RemoteLibraryResponseModel": { "id": number; - "name": string; + "name": null | string; "mediaKind": components["schemas"]["LibraryMediaKind"]; "shouldSyncItems": boolean; }; "RemoteMediaSourceItemResponseModel": { "id": number; - "name": string; - "address": string; + "name": null | string; + "address": null | string; }; "RemoteMediaSourceStateResponseModel": { "isAuthorized": boolean;