From a9af93ff0c50f73657f5565096ef874b97abfbae Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 16:35:06 +0700 Subject: [PATCH] Report FwHeadless failures with one exception and one status mapping FwHeadlessClient now throws FwHeadlessException(statusCode, detail) on any non-success FwHeadless response, reading the detail from a problem+json body, a JSON string body or plain text. Lexbox maps it once in the exception handler's StatusCodeSelector: FwHeadless 4xx pass through, 401/403 and 5xx become 502. The ProblemDetails response carries the detail, and the dev exception page applies the same mapping so CI matches production. SyncController actions no longer map tuples, nulls and exceptions by hand. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AM78WDo2i51h7253MJHFy9 --- .../LexBoxApi/Controllers/SyncController.cs | 81 +++-------------- .../AddExceptionFeatureDevExceptionFilter.cs | 4 + .../ErrorHandling/ExceptionStatusCodes.cs | 31 +++++++ backend/LexBoxApi/Program.cs | 10 +-- .../LexBoxApi/Services/FwHeadlessClient.cs | 88 ++++++++++--------- .../LexCore/Exceptions/FwHeadlessException.cs | 13 +++ .../ExceptionStatusCodesTests.cs | 29 ++++++ .../Services/FwHeadlessClientTests.cs | 74 ++++++++++++++-- 8 files changed, 205 insertions(+), 125 deletions(-) create mode 100644 backend/LexBoxApi/ErrorHandling/ExceptionStatusCodes.cs create mode 100644 backend/LexCore/Exceptions/FwHeadlessException.cs create mode 100644 backend/Testing/LexBoxApi/ErrorHandling/ExceptionStatusCodesTests.cs diff --git a/backend/LexBoxApi/Controllers/SyncController.cs b/backend/LexBoxApi/Controllers/SyncController.cs index 02dda0fc23..3f4093933a 100644 --- a/backend/LexBoxApi/Controllers/SyncController.cs +++ b/backend/LexBoxApi/Controllers/SyncController.cs @@ -1,7 +1,6 @@ using LexBoxApi.Auth.Attributes; using LexBoxApi.Services; using LexCore.Auth; -using LexCore.Exceptions; using LexCore.ServiceInterfaces; using LexCore.Sync; using Microsoft.AspNetCore.Mvc; @@ -25,9 +24,7 @@ public class SyncController( public async Task> GetSyncStatus(Guid projectId) { if (!await permissionService.CanViewProject(projectId)) return Forbid(); - var result = await fwHeadlessClient.CrdtSyncStatus(projectId); - if (result is null) return NotFound(); - return Ok(result); + return Ok(await fwHeadlessClient.CrdtSyncStatus(projectId)); } [HttpPost("trigger/{projectId}")] @@ -35,16 +32,7 @@ public async Task> GetSyncStatus(Guid projectId) public async Task TriggerSync(Guid projectId) { if (!await permissionService.CanSyncProject(projectId)) return Forbid(); - var (started, statusCode, error) = await fwHeadlessClient.SyncMercurialAndHarmony(projectId); - if (!started) - { - return statusCode switch - { - System.Net.HttpStatusCode.Locked => Problem(error ?? "Project is blocked from syncing", statusCode: StatusCodes.Status423Locked), - System.Net.HttpStatusCode.NotFound => Problem(error ?? "Project not found", statusCode: StatusCodes.Status404NotFound), - _ => Problem(error ?? "Failed to trigger sync", statusCode: (int)statusCode) - }; - } + await fwHeadlessClient.SyncMercurialAndHarmony(projectId); return Ok(); } @@ -77,19 +65,8 @@ public async Task> AwaitSyncFinished(Guid projectId) public async Task SyncHarmony(Guid projectId) { await permissionService.AssertCanSyncProject(projectId); - try - { - var result = await fwHeadlessClient.SyncHarmony(projectId); - if (result is not null) - { - return Problem(result); - } - return Ok(); - } - catch (Exception e) - { - return Problem(e.Message); - } + await fwHeadlessClient.SyncHarmony(projectId); + return Ok(); } /// @@ -110,19 +87,8 @@ public async Task SyncHarmony(Guid projectId) public async Task RegenerateProjectSnapshot(Guid projectId, [FromQuery] Guid? commitId = null, [FromQuery] bool preserveAllFieldWorksCommits = false) { await permissionService.AssertCanSyncProject(projectId); - try - { - var (success, statusCode, error) = await fwHeadlessClient.RegenerateProjectSnapshot(projectId, commitId, preserveAllFieldWorksCommits); - if (!success) - { - return Problem(error, statusCode: (int)statusCode); - } - return Ok(); - } - catch (Exception e) - { - return Problem(e.Message); - } + await fwHeadlessClient.RegenerateProjectSnapshot(projectId, commitId, preserveAllFieldWorksCommits); + return Ok(); } /// @@ -161,19 +127,8 @@ public async Task BlockProject(Guid? projectId = null, string? pro await permissionService.AssertCanSyncProject(id.Value); - try - { - await fwHeadlessClient.BlockProject(id.Value, reason); - return Ok(); - } - catch (NotFoundException) - { - return NotFound(); - } - catch (InvalidOperationException ex) - { - return BadRequest(ex.Message); - } + await fwHeadlessClient.BlockProject(id.Value, reason); + return Ok(); } [HttpPost("unblock")] @@ -192,19 +147,8 @@ public async Task UnblockProject(Guid? projectId = null, string? p await permissionService.AssertCanSyncProject(id.Value); - try - { - await fwHeadlessClient.UnblockProject(id.Value); - return Ok(); - } - catch (NotFoundException) - { - return NotFound(); - } - catch (InvalidOperationException ex) - { - return BadRequest(ex.Message); - } + await fwHeadlessClient.UnblockProject(id.Value); + return Ok(); } [HttpGet("block-status")] @@ -224,10 +168,7 @@ public async Task> GetBlockStatus(Guid? projectId // Check permissions with the resolved projectId await permissionService.AssertCanViewProject(id.Value); - var status = await fwHeadlessClient.GetBlockStatus(id.Value); - if (status is null) - return NotFound(); - return status; + return await fwHeadlessClient.GetBlockStatus(id.Value); } private static bool IsProjectIdOrCodeProvided(Guid? projectId, string? projectCode) diff --git a/backend/LexBoxApi/ErrorHandling/AddExceptionFeatureDevExceptionFilter.cs b/backend/LexBoxApi/ErrorHandling/AddExceptionFeatureDevExceptionFilter.cs index 9e10b16ee5..2dd9e042fe 100644 --- a/backend/LexBoxApi/ErrorHandling/AddExceptionFeatureDevExceptionFilter.cs +++ b/backend/LexBoxApi/ErrorHandling/AddExceptionFeatureDevExceptionFilter.cs @@ -1,3 +1,4 @@ +using LexCore.Exceptions; using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Http.Features; @@ -19,6 +20,9 @@ public async Task HandleExceptionAsync(ErrorContext errorContext, Func()?.RouteValues }); } + // the developer page always answers 500, so apply the FwHeadless status mapping here too, as in production + if (errorContext.Exception is FwHeadlessException) + httpContext.Response.StatusCode = ExceptionStatusCodes.Select(errorContext.Exception); await next(errorContext); } } diff --git a/backend/LexBoxApi/ErrorHandling/ExceptionStatusCodes.cs b/backend/LexBoxApi/ErrorHandling/ExceptionStatusCodes.cs new file mode 100644 index 0000000000..4dbbd960d2 --- /dev/null +++ b/backend/LexBoxApi/ErrorHandling/ExceptionStatusCodes.cs @@ -0,0 +1,31 @@ +using LexCore.Exceptions; + +namespace LexBoxApi.ErrorHandling; + +public static class ExceptionStatusCodes +{ + /// + /// The response status for an unhandled exception, used as the exception handler's StatusCodeSelector. + /// + public static int Select(Exception exception) + { + return exception switch + { + UnauthorizedAccessException => StatusCodes.Status401Unauthorized, + FwHeadlessException fwHeadless => FromFwHeadless((int)fwHeadless.StatusCode), + _ => StatusCodes.Status500InternalServerError + }; + } + + // FwHeadless's client errors describe the caller's request, so they pass through. + // Its auth failures are between Lexbox and FwHeadless (they must not tell the caller they aren't logged in), + // and its server errors are ours, not the caller's, so both become 502. + private static int FromFwHeadless(int statusCode) + { + return statusCode is >= 400 and < 500 + and not StatusCodes.Status401Unauthorized + and not StatusCodes.Status403Forbidden + ? statusCode + : StatusCodes.Status502BadGateway; + } +} diff --git a/backend/LexBoxApi/Program.cs b/backend/LexBoxApi/Program.cs index 0e1798af4b..ceb4b69efb 100644 --- a/backend/LexBoxApi/Program.cs +++ b/backend/LexBoxApi/Program.cs @@ -71,18 +71,16 @@ builder.Services.AddSingleton(); builder.Services.AddExceptionHandler((options) => { - options.StatusCodeSelector = exception => - { - if (exception is UnauthorizedAccessException) - return StatusCodes.Status401Unauthorized; - return StatusCodes.Status500InternalServerError; - }; + options.StatusCodeSelector = ExceptionStatusCodes.Select; }); builder.Services.AddProblemDetails(o => { o.CustomizeProblemDetails = context => { var exceptionHandlerFeature = context.HttpContext.Features.Get(); + // the exception handler leaves detail empty; FwHeadless's message is meant for the caller + if (exceptionHandlerFeature?.Error is FwHeadlessException fwHeadlessException) + context.ProblemDetails.Detail = fwHeadlessException.Message; if (exceptionHandlerFeature?.Error is not IExceptionWithCode exceptionWithCode) return; context.ProblemDetails.Extensions["app-error-code"] = exceptionWithCode.Code; }; diff --git a/backend/LexBoxApi/Services/FwHeadlessClient.cs b/backend/LexBoxApi/Services/FwHeadlessClient.cs index 604d1588d0..6ad6aaf722 100644 --- a/backend/LexBoxApi/Services/FwHeadlessClient.cs +++ b/backend/LexBoxApi/Services/FwHeadlessClient.cs @@ -2,34 +2,22 @@ using System.Text.Json; using LexCore.Exceptions; using LexCore.Sync; -using Microsoft.AspNetCore.Mvc; namespace LexBoxApi.Services; public class FwHeadlessClient(HttpClient httpClient, ILogger logger) { - public async Task<(bool success, HttpStatusCode statusCode, string? error)> SyncMercurialAndHarmony(Guid projectId) + public async Task SyncMercurialAndHarmony(Guid projectId) { var response = await httpClient.PostAsync($"/api/merge/execute?projectId={projectId}", null); - string? error = null; if (!response.IsSuccessStatusCode) { logger.LogError("Failed to sync Mercurial and Harmony: {StatusCode} {StatusDescription}, projectId: {ProjectId}", response.StatusCode, response.ReasonPhrase, projectId); - try - { - var problemDetails = JsonSerializer.Deserialize(await response.Content.ReadAsStringAsync()); - error = problemDetails?.Detail ?? problemDetails?.Title; - } - catch - { - error = response.ReasonPhrase; - } + throw await FailedResponse(response); } - - return (response.IsSuccessStatusCode, response.StatusCode, error); } /// @@ -38,7 +26,7 @@ public class FwHeadlessClient(HttpClient httpClient, ILogger l public async Task ReadyLexiconProjects(Guid[] projectIds) { var response = await httpClient.PostAsJsonAsync("/api/lexicon/projects/ready", projectIds); - response.EnsureSuccessStatusCode(); + if (!response.IsSuccessStatusCode) throw await FailedResponse(response); return await response.Content.ReadFromJsonAsync() ?? []; } @@ -65,17 +53,18 @@ public async Task AwaitStatus(Guid projectId, CancellationToken c return jobResult; } - public async Task CrdtSyncStatus(Guid projectId) + public async Task CrdtSyncStatus(Guid projectId) { var response = await httpClient.GetAsync($"/api/merge/status?projectId={projectId}"); if (response.IsSuccessStatusCode) - return await response.Content.ReadFromJsonAsync(); + return await response.Content.ReadFromJsonAsync() + ?? throw new FwHeadlessException(HttpStatusCode.BadGateway, "FwHeadless returned no sync status"); logger.LogError("Failed to get CRDT sync status: {StatusCode} {StatusDescription}, projectId: {ProjectId}, response: {Response}", response.StatusCode, response.ReasonPhrase, projectId, await response.Content.ReadAsStringAsync()); - return null; + throw await FailedResponse(response); } public async Task DeleteRepo(Guid projectId, CancellationToken cancellationToken = default) @@ -90,7 +79,7 @@ public async Task DeleteRepo(Guid projectId, CancellationToken cancellationToken response.ReasonPhrase, projectId, await response.Content.ReadAsStringAsync(cancellationToken)); - throw new InvalidOperationException($"Failed to delete repo {projectId}: {response.StatusCode} {response.ReasonPhrase}"); + throw await FailedResponse(response, cancellationToken); } public async Task DeleteProject(Guid projectId, CancellationToken cancellationToken = default) @@ -105,10 +94,10 @@ public async Task DeleteProject(Guid projectId, CancellationToken cancellationTo response.ReasonPhrase, projectId, await response.Content.ReadAsStringAsync(cancellationToken)); - throw new InvalidOperationException($"Failed to delete project {projectId}: {response.StatusCode} {response.ReasonPhrase}"); + throw await FailedResponse(response, cancellationToken); } - public async Task<(bool success, HttpStatusCode statusCode, string? error)> RegenerateProjectSnapshot(Guid projectId, Guid? commitId = null, bool preserveAllFieldWorksCommits = false) + public async Task RegenerateProjectSnapshot(Guid projectId, Guid? commitId = null, bool preserveAllFieldWorksCommits = false) { var url = $"/api/merge/regenerate-snapshot?projectId={projectId}"; if (commitId.HasValue) @@ -126,13 +115,11 @@ public async Task DeleteProject(Guid projectId, CancellationToken cancellationTo response.ReasonPhrase, projectId, responseBody); - return (false, response.StatusCode, responseBody); + throw await FailedResponse(response); } - - return (true, response.StatusCode, null); } - internal async Task SyncHarmony(Guid projectId) + internal async Task SyncHarmony(Guid projectId) { var response = await httpClient.PostAsync($"/api/merge/sync-harmony?projectId={projectId}", null); if (!response.IsSuccessStatusCode) @@ -143,10 +130,8 @@ public async Task DeleteProject(Guid projectId, CancellationToken cancellationTo response.ReasonPhrase, projectId, responseBody); - return responseBody; + throw await FailedResponse(response); } - - return null; } public async Task BlockProject(Guid projectId, string? reason = null) @@ -164,11 +149,7 @@ public async Task BlockProject(Guid projectId, string? reason = null) response.ReasonPhrase, projectId, responseBody); - - if (response.StatusCode == HttpStatusCode.NotFound) - throw new NotFoundException($"Project not found in FwHeadless: {projectId}", "Project"); - - throw new InvalidOperationException($"Failed to block project: {response.StatusCode} {response.ReasonPhrase}"); + throw await FailedResponse(response); } } @@ -184,27 +165,52 @@ public async Task UnblockProject(Guid projectId) response.ReasonPhrase, projectId, responseBody); - - if (response.StatusCode == HttpStatusCode.NotFound) - throw new NotFoundException($"Project not found in FwHeadless: {projectId}", "Project"); - - throw new InvalidOperationException($"Failed to unblock project: {response.StatusCode} {response.ReasonPhrase}"); + throw await FailedResponse(response); } } - public async Task GetBlockStatus(Guid projectId) + public async Task GetBlockStatus(Guid projectId) { var url = $"/api/merge/block-status?projectId={projectId}"; var response = await httpClient.GetAsync(url); if (response.IsSuccessStatusCode) - return await response.Content.ReadFromJsonAsync(); + return await response.Content.ReadFromJsonAsync() + ?? throw new FwHeadlessException(HttpStatusCode.BadGateway, "FwHeadless returned no block status"); logger.LogError("Failed to get block status: {StatusCode} {StatusDescription}, projectId: {ProjectId}, response: {Response}", response.StatusCode, response.ReasonPhrase, projectId, await response.Content.ReadAsStringAsync()); - return null; + throw await FailedResponse(response); + } + + private static async Task FailedResponse(HttpResponseMessage response, CancellationToken cancellationToken = default) + { + return new FwHeadlessException(response.StatusCode, ReadDetail(await response.Content.ReadAsStringAsync(cancellationToken))); } + /// + /// The message in a FwHeadless error body: a problem+json detail (or title), a JSON string such as a NotFound<string>, or plain text. + /// + private static string? ReadDetail(string body) + { + if (string.IsNullOrWhiteSpace(body)) return null; + try + { + using var json = JsonDocument.Parse(body); + var root = json.RootElement; + if (root.ValueKind == JsonValueKind.String) return root.GetString(); + if (root.ValueKind == JsonValueKind.Object) + { + if (root.TryGetProperty("detail", out var detail) && detail.ValueKind == JsonValueKind.String) return detail.GetString(); + if (root.TryGetProperty("title", out var title) && title.ValueKind == JsonValueKind.String) return title.GetString(); + } + } + catch (JsonException) + { + // plain text + } + return body; + } } diff --git a/backend/LexCore/Exceptions/FwHeadlessException.cs b/backend/LexCore/Exceptions/FwHeadlessException.cs new file mode 100644 index 0000000000..3b58ce2f07 --- /dev/null +++ b/backend/LexCore/Exceptions/FwHeadlessException.cs @@ -0,0 +1,13 @@ +using System.Net; + +namespace LexCore.Exceptions; + +/// +/// FwHeadless answered with a non-success status. Lexbox maps to its own response status in one place. +/// +public class FwHeadlessException(HttpStatusCode statusCode, string? detail) + : Exception(detail ?? $"FwHeadless responded with {(int)statusCode} {statusCode}") +{ + public HttpStatusCode StatusCode { get; } = statusCode; + public string? Detail { get; } = detail; +} diff --git a/backend/Testing/LexBoxApi/ErrorHandling/ExceptionStatusCodesTests.cs b/backend/Testing/LexBoxApi/ErrorHandling/ExceptionStatusCodesTests.cs new file mode 100644 index 0000000000..3d5eb8e062 --- /dev/null +++ b/backend/Testing/LexBoxApi/ErrorHandling/ExceptionStatusCodesTests.cs @@ -0,0 +1,29 @@ +using System.Net; +using LexBoxApi.ErrorHandling; +using LexCore.Exceptions; + +namespace Testing.LexBoxApi.ErrorHandling; + +public class ExceptionStatusCodesTests +{ + [Theory] + [InlineData(HttpStatusCode.BadRequest, 400)] + [InlineData(HttpStatusCode.NotFound, 404)] + [InlineData(HttpStatusCode.Conflict, 409)] + [InlineData(HttpStatusCode.Locked, 423)] + [InlineData(HttpStatusCode.Unauthorized, 502)] + [InlineData(HttpStatusCode.Forbidden, 502)] + [InlineData(HttpStatusCode.InternalServerError, 502)] + [InlineData(HttpStatusCode.ServiceUnavailable, 502)] + public void FwHeadlessStatus_MapsToLexboxStatus(HttpStatusCode fwHeadlessStatus, int expected) + { + ExceptionStatusCodes.Select(new FwHeadlessException(fwHeadlessStatus, null)).Should().Be(expected); + } + + [Fact] + public void OtherExceptions_KeepTheirStatus() + { + ExceptionStatusCodes.Select(new UnauthorizedAccessException()).Should().Be(401); + ExceptionStatusCodes.Select(new InvalidOperationException()).Should().Be(500); + } +} diff --git a/backend/Testing/LexBoxApi/Services/FwHeadlessClientTests.cs b/backend/Testing/LexBoxApi/Services/FwHeadlessClientTests.cs index d3133e7524..36fb1f75c5 100644 --- a/backend/Testing/LexBoxApi/Services/FwHeadlessClientTests.cs +++ b/backend/Testing/LexBoxApi/Services/FwHeadlessClientTests.cs @@ -1,5 +1,6 @@ using System.Net; using LexBoxApi.Services; +using LexCore.Exceptions; using Microsoft.Extensions.Logging.Abstractions; using Moq; using Moq.Contrib.HttpClient; @@ -8,19 +9,76 @@ namespace Testing.LexBoxApi.Services; public class FwHeadlessClientTests { - [Fact] - public async Task RegenerateProjectSnapshot_PassesThroughFwHeadlessStatusCode() + private static FwHeadlessClient ClientReturning(HttpStatusCode statusCode, string body, string mediaType) { var handler = new Mock(MockBehavior.Strict); - handler.SetupAnyRequest().ReturnsResponse(HttpStatusCode.NotFound, "Commit not found"); + handler.SetupAnyRequest().ReturnsResponse(statusCode, body, mediaType); var httpClient = handler.CreateClient(); httpClient.BaseAddress = new Uri("http://fw-headless"); - var client = new FwHeadlessClient(httpClient, NullLogger.Instance); + return new FwHeadlessClient(httpClient, NullLogger.Instance); + } + + [Fact] + public async Task RegenerateProjectSnapshot_ThrowsWithUnquotedDetail_ForJsonStringBody() + { + // FwHeadless's NotFound serializes the message as a JSON string + var client = ClientReturning(HttpStatusCode.NotFound, "\"Commit not found\"", "application/json"); + + var act = () => client.RegenerateProjectSnapshot(Guid.NewGuid(), Guid.NewGuid()); + + var exception = (await act.Should().ThrowAsync()).Which; + exception.StatusCode.Should().Be(HttpStatusCode.NotFound); + exception.Detail.Should().Be("Commit not found"); + exception.Message.Should().Be("Commit not found"); + } + + [Fact] + public async Task SyncMercurialAndHarmony_ThrowsWithProblemDetail_ForProblemJsonBody() + { + var client = ClientReturning(HttpStatusCode.Locked, + """{"type":"https://tools.ietf.org/html/rfc4918#section-11.3","title":"Locked","status":423,"detail":"Project is blocked from syncing. Reason: testing"}""", + "application/problem+json"); + + var act = () => client.SyncMercurialAndHarmony(Guid.NewGuid()); + + var exception = (await act.Should().ThrowAsync()).Which; + exception.StatusCode.Should().Be(HttpStatusCode.Locked); + exception.Detail.Should().Be("Project is blocked from syncing. Reason: testing"); + } + + [Fact] + public async Task BlockProject_ThrowsWithProblemTitle_WhenThereIsNoDetail() + { + var client = ClientReturning(HttpStatusCode.NotFound, + """{"type":"https://tools.ietf.org/html/rfc9110#section-15.5.5","title":"Not Found","status":404}""", + "application/problem+json"); + + var act = () => client.BlockProject(Guid.NewGuid()); + + var exception = (await act.Should().ThrowAsync()).Which; + exception.StatusCode.Should().Be(HttpStatusCode.NotFound); + exception.Detail.Should().Be("Not Found"); + } + + [Fact] + public async Task UnblockProject_ThrowsWithPlainTextDetail() + { + var client = ClientReturning(HttpStatusCode.InternalServerError, "Something broke", "text/plain"); + + var act = () => client.UnblockProject(Guid.NewGuid()); + + var exception = (await act.Should().ThrowAsync()).Which; + exception.StatusCode.Should().Be(HttpStatusCode.InternalServerError); + exception.Detail.Should().Be("Something broke"); + } + + [Fact] + public async Task DeleteRepo_ThrowsProjectSyncInProgress_OnConflict() + { + var client = ClientReturning(HttpStatusCode.Conflict, "", "text/plain"); - var (success, statusCode, error) = await client.RegenerateProjectSnapshot(Guid.NewGuid(), Guid.NewGuid()); + var act = () => client.DeleteRepo(Guid.NewGuid()); - success.Should().BeFalse(); - statusCode.Should().Be(HttpStatusCode.NotFound); - error.Should().Be("Commit not found"); + await act.Should().ThrowAsync(); } }