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(); } }