From 89a5f951874929a6bf26ce6a51ad3d25afc4bdb9 Mon Sep 17 00:00:00 2001 From: Aitor Reviriego Amor Date: Tue, 2 Jun 2026 21:52:23 +0200 Subject: [PATCH] chore(backend): mejoras del code review (auth SAP, saneo errores, deuda, seed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SAP: 401/403 del OData ahora se mapean a Error.Unauthorized (antes 502); usa GetAsync + chequeo de status (como el adaptador Shopify) y maneja timeout. Guard ct.ThrowIfCancellationRequested al entrar. - ErrorHttpResults: ProblemDetails.Detail genérico para Unavailable/Unexpected (no filtra mensajes internos de SAP/Shopify/SQLite al cliente). El controlador loguea el detalle real (ILogger en SalesController + helper Fail). - DEUDA-TECNICA.md: nueva sección de paginación SAP; se quitan los TODO truncados de los adaptadores Shopify (ya cubiertos por #5/#6). - Program.cs: backoff del seed más corto (Cloud Run arranca rápido), comentario actualizado. - Tests (72 -> 76): SAP 401/403 -> Unauthorized; endpoint Unauthorized -> 401; ErrorHttpResults no filtra detalle en errores de servidor. Co-Authored-By: Claude Opus 4.8 --- DEUDA-TECNICA.md | 14 +++++++++-- .../Inbound/Http/ErrorHttpResults.cs | 12 +++++++++- .../Inbound/Http/SalesController.cs | 24 +++++++++++++------ .../Outbound/Sap/SapODataSalesRepository.cs | 20 +++++++++++++++- .../Shopify/ShopifyOrdersRepository.cs | 2 -- .../Outbound/Shopify/ShopifyTokenProvider.cs | 1 - backend/Program.cs | 15 +++++------- .../Api/ErrorMappingEndpointsTests.cs | 1 + .../Inbound/Http/ErrorHttpResultsTests.cs | 12 ++++++++++ .../Outbound/SapODataSalesRepositoryTests.cs | 12 ++++++++++ 10 files changed, 90 insertions(+), 23 deletions(-) diff --git a/DEUDA-TECNICA.md b/DEUDA-TECNICA.md index 8ff0134..8883d95 100644 --- a/DEUDA-TECNICA.md +++ b/DEUDA-TECNICA.md @@ -67,7 +67,7 @@ solo lee la primera página de `orders.json` (`limit=250`). - **Cuándo abordarla:** antes de apuntar a una tienda con más de 250 pedidos relevantes. La paginación de la Admin REST se hace por la cabecera `Link: <…>; rel="next"`; hay que iterar siguiendo ese cursor y respetar el rate-limit (40 calls/app/store en buckets de - leaky-bucket). Misma iteración que el TODO de `Retry-After` en 429. + leaky-bucket), respetando `Retry-After` cuando devuelva 429. ### 6. Refresco proactivo del token de Shopify @@ -109,6 +109,16 @@ No hay middleware de manejo de errores en [`Program.cs`](backend/Program.cs). - **Cuándo abordarla:** al desplegar, añadir manejo de errores consistente y asegurar que `ASPNETCORE_ENVIRONMENT` nunca es `Development` en entornos accesibles. +### 9. Paginación del adaptador SAP + +[`SapODataSalesRepository`](backend/Infrastructure/Outbound/Sap/SapODataSalesRepository.cs) pide +una sola página del OData (`$top=200`), sin seguir `__next`. + +- **Por qué se pospone:** la sandbox del Business Accelerator Hub devuelve un dataset acotado; + 200 ítems bastan para validar el flujo end-to-end del MVP. +- **Cuándo abordarla:** al apuntar a un S/4HANA real con más volumen. OData v2 pagina por el + enlace `d.__next`; hay que iterar siguiéndolo (análogo a la paginación de Shopify, #5). + --- -_Última revisión: 2026-05-24._ +_Última revisión: 2026-06-02._ diff --git a/backend/Infrastructure/Inbound/Http/ErrorHttpResults.cs b/backend/Infrastructure/Inbound/Http/ErrorHttpResults.cs index 9d03441..54c653a 100644 --- a/backend/Infrastructure/Inbound/Http/ErrorHttpResults.cs +++ b/backend/Infrastructure/Inbound/Http/ErrorHttpResults.cs @@ -21,8 +21,18 @@ public static IActionResult ToActionResult(Error error) { Status = status, Title = error.Type.ToString(), - Detail = error.Message, + Detail = ClientDetail(error), }; return new ObjectResult(problem) { StatusCode = status }; } + + // Server-side failures carry upstream/infra exception text (SAP/Shopify/SQLite messages); keep + // it out of the client response (the controller logs the real detail). Client-facing errors + // (NotFound/Validation/Unauthorized) carry intentional, safe domain messages. + private static string ClientDetail(Error error) => error.Type switch + { + ErrorType.Unavailable => "The data source is currently unavailable.", + ErrorType.Unexpected => "An unexpected error occurred.", + _ => error.Message, + }; } diff --git a/backend/Infrastructure/Inbound/Http/SalesController.cs b/backend/Infrastructure/Inbound/Http/SalesController.cs index 478011e..d0a45bc 100644 --- a/backend/Infrastructure/Inbound/Http/SalesController.cs +++ b/backend/Infrastructure/Inbound/Http/SalesController.cs @@ -1,39 +1,49 @@ using Microsoft.AspNetCore.Mvc; using ConnectAnalyzer.Application; +using ConnectAnalyzer.Domain; namespace ConnectAnalyzer.Infrastructure.Inbound.Http; [ApiController] [Route("api/sales")] -public sealed class SalesController(SalesAnalytics analytics, IngestSales ingest) : ControllerBase +public sealed class SalesController( + SalesAnalytics analytics, + IngestSales ingest, + ILogger logger) : ControllerBase { [HttpPost("refresh")] public async Task Refresh(CancellationToken ct) { var result = await ingest.ExecuteAsync(ct); - return result.Match( - ingested => Ok(new { ingested }), - ErrorHttpResults.ToActionResult); + return result.Match(ingested => Ok(new { ingested }), Fail); } [HttpGet] public async Task GetAll(CancellationToken ct) { var result = await analytics.GetAllAsync(ct); - return result.Match(Ok, ErrorHttpResults.ToActionResult); + return result.Match(Ok, Fail); } [HttpGet("by-product")] public async Task ByProduct(CancellationToken ct) { var result = await analytics.TotalsByProductAsync(ct); - return result.Match(Ok, ErrorHttpResults.ToActionResult); + return result.Match(Ok, Fail); } [HttpGet("by-customer")] public async Task ByCustomer(CancellationToken ct) { var result = await analytics.TotalsByCustomerAsync(ct); - return result.Match(Ok, ErrorHttpResults.ToActionResult); + return result.Match(Ok, Fail); + } + + // Logs the real error detail server-side (it's stripped from the client response by + // ErrorHttpResults for server-side failures), then maps to the HTTP status. + private IActionResult Fail(Error error) + { + logger.LogWarning("Sales request failed: {Type} {Message}", error.Type, error.Message); + return ErrorHttpResults.ToActionResult(error); } } diff --git a/backend/Infrastructure/Outbound/Sap/SapODataSalesRepository.cs b/backend/Infrastructure/Outbound/Sap/SapODataSalesRepository.cs index 1ff754f..bd20168 100644 --- a/backend/Infrastructure/Outbound/Sap/SapODataSalesRepository.cs +++ b/backend/Infrastructure/Outbound/Sap/SapODataSalesRepository.cs @@ -1,4 +1,5 @@ using System.Globalization; +using System.Net; using System.Text.Json; using System.Text.Json.Serialization; using ConnectAnalyzer.Application.Ports; @@ -18,9 +19,21 @@ public sealed class SapODataSalesRepository(HttpClient http) : ISalesRepository public async Task>> SearchAsync(CancellationToken ct = default) { + ct.ThrowIfCancellationRequested(); try { - var json = await http.GetStringAsync(SalesItemsResource, ct); + using var response = await http.GetAsync(SalesItemsResource, ct); + + // A bad/missing API key is an auth problem (401), not the source being down (502). + if (response.StatusCode is HttpStatusCode.Unauthorized or HttpStatusCode.Forbidden) + return Result>.Failure( + Error.Unauthorized("SAP rejected the API key (check Sap:ApiKey).")); + + if (!response.IsSuccessStatusCode) + return Result>.Failure( + Error.Unavailable($"The SAP data source returned {(int)response.StatusCode}.")); + + var json = await response.Content.ReadAsStringAsync(ct); var payload = JsonSerializer.Deserialize(json, JsonOptions); IReadOnlyList sales = (payload?.D?.Results ?? []) @@ -40,6 +53,11 @@ public async Task>> SearchAsync(CancellationToken ct return Result>.Failure( Error.Unexpected($"Malformed OData payload from the SAP data source: {ex.Message}")); } + catch (OperationCanceledException) when (!ct.IsCancellationRequested) + { + return Result>.Failure( + Error.Unavailable("SAP request timed out.")); + } } private static Sale? TryParseSale(SalesOrderItemDto item) diff --git a/backend/Infrastructure/Outbound/Shopify/ShopifyOrdersRepository.cs b/backend/Infrastructure/Outbound/Shopify/ShopifyOrdersRepository.cs index 99e00b7..a92dd09 100644 --- a/backend/Infrastructure/Outbound/Shopify/ShopifyOrdersRepository.cs +++ b/backend/Infrastructure/Outbound/Shopify/ShopifyOrdersRepository.cs @@ -8,7 +8,6 @@ namespace ConnectAnalyzer.Infrastructure.Outbound.Shopify; -// TODO: paginate via the Link header's `rel="next"` cursor. The MVP fetches a single page of public sealed class ShopifyOrdersRepository( HttpClient http, ShopifyTokenProvider tokens, @@ -39,7 +38,6 @@ private async Task>> FetchOrdersAsync(string token, C return Result>.Failure(Error.Unauthorized( "Shopify rejected the Admin API token (it may have been revoked or lack the required scopes).")); - // TODO: respect Retry-After header on 429 instead of failing immediately. if ((int)response.StatusCode == 429) return Result>.Failure(Error.Unavailable( "Shopify rate limit hit.")); diff --git a/backend/Infrastructure/Outbound/Shopify/ShopifyTokenProvider.cs b/backend/Infrastructure/Outbound/Shopify/ShopifyTokenProvider.cs index e87d6d0..a70a82c 100644 --- a/backend/Infrastructure/Outbound/Shopify/ShopifyTokenProvider.cs +++ b/backend/Infrastructure/Outbound/Shopify/ShopifyTokenProvider.cs @@ -6,7 +6,6 @@ namespace ConnectAnalyzer.Infrastructure.Outbound.Shopify; -// TODO: react to a future 401 from the data endpoint by invalidating the cached token and public sealed class ShopifyTokenProvider(HttpClient http, string clientId, string clientSecret) { private readonly SemaphoreSlim _gate = new(1, 1); diff --git a/backend/Program.cs b/backend/Program.cs index c2f8c85..f1de7c5 100644 --- a/backend/Program.cs +++ b/backend/Program.cs @@ -111,11 +111,10 @@ app.MapControllers(); // Seed the local store from the configured source on startup so the dashboard isn't empty on -// first load. Runs in the background with retries: on free-tier hosting the upstream source can -// take 30-60 s to wake up from a cold start, longer than the HttpClient default, so the first -// attempt frequently lands while the source is still returning 502/timeouts. Retries with backoff -// let the store self-heal without blocking app.Run(). Skipped under "Testing" (integration tests -// wire their own store). POST /api/sales/refresh remains available as a manual retry. +// first load. Runs in the background with a short backoff to ride out a transient startup race +// (e.g. the mock waking a second after the backend on Cloud Run), without blocking app.Run(). +// Skipped under "Testing" (integration tests wire their own store). POST /api/sales/refresh +// remains available as a manual retry. if (!app.Environment.IsEnvironment("Testing")) { _ = Task.Run(async () => @@ -123,10 +122,8 @@ TimeSpan[] backoffs = [ TimeSpan.Zero, - TimeSpan.FromSeconds(5), - TimeSpan.FromSeconds(15), - TimeSpan.FromSeconds(30), - TimeSpan.FromSeconds(60), + TimeSpan.FromSeconds(3), + TimeSpan.FromSeconds(10), ]; var logger = app.Services.GetRequiredService>(); diff --git a/backend/tests/ConnectAnalyzer.Tests/Api/ErrorMappingEndpointsTests.cs b/backend/tests/ConnectAnalyzer.Tests/Api/ErrorMappingEndpointsTests.cs index 6a3b4c3..0b3007a 100644 --- a/backend/tests/ConnectAnalyzer.Tests/Api/ErrorMappingEndpointsTests.cs +++ b/backend/tests/ConnectAnalyzer.Tests/Api/ErrorMappingEndpointsTests.cs @@ -15,6 +15,7 @@ public class ErrorMappingEndpointsTests [Theory] [InlineData(ErrorType.NotFound, HttpStatusCode.NotFound)] [InlineData(ErrorType.Validation, HttpStatusCode.BadRequest)] + [InlineData(ErrorType.Unauthorized, HttpStatusCode.Unauthorized)] [InlineData(ErrorType.Unavailable, HttpStatusCode.BadGateway)] [InlineData(ErrorType.Unexpected, HttpStatusCode.InternalServerError)] public async Task Failure_IsTranslatedToItsHttpStatus(ErrorType type, HttpStatusCode expected) diff --git a/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Inbound/Http/ErrorHttpResultsTests.cs b/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Inbound/Http/ErrorHttpResultsTests.cs index 3719f8d..74a5325 100644 --- a/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Inbound/Http/ErrorHttpResultsTests.cs +++ b/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Inbound/Http/ErrorHttpResultsTests.cs @@ -29,4 +29,16 @@ public void ToActionResult_CarriesStatusAndMessageInProblemDetails() Assert.Equal(StatusCodes.Status404NotFound, problem.Status); Assert.Equal("customer C999 not found", problem.Detail); } + + [Fact] + public void ToActionResult_DoesNotLeakInternalDetailForServerErrors() + { + var result = ErrorHttpResults.ToActionResult( + Error.Unavailable("Could not reach the SAP data source: raw 401 text")); + + var objectResult = Assert.IsType(result); + var problem = Assert.IsType(objectResult.Value); + Assert.Equal(StatusCodes.Status502BadGateway, problem.Status); + Assert.DoesNotContain("raw 401 text", problem.Detail!); + } } diff --git a/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Outbound/SapODataSalesRepositoryTests.cs b/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Outbound/SapODataSalesRepositoryTests.cs index 8c3b912..44cbcec 100644 --- a/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Outbound/SapODataSalesRepositoryTests.cs +++ b/backend/tests/ConnectAnalyzer.Tests/Infrastructure/Outbound/SapODataSalesRepositoryTests.cs @@ -119,6 +119,18 @@ public async Task ReturnsUnavailableFailureWhenSourceReturnsHttpError(HttpStatus Assert.Equal(ErrorType.Unavailable, FailureError(result).Type); } + [Theory] + [InlineData(HttpStatusCode.Unauthorized)] + [InlineData(HttpStatusCode.Forbidden)] + public async Task ReturnsUnauthorizedWhenSapRejectsTheApiKey(HttpStatusCode status) + { + var sut = CreateSut("ignored", status); + + var result = await sut.SearchAsync(); + + Assert.Equal(ErrorType.Unauthorized, FailureError(result).Type); + } + [Fact] public async Task ReturnsUnavailableFailureWhenSourceIsUnreachable() {