Return network failures, timeouts and bad payloads as errors

docs/BACKLOG.md

@@ -6,7 +6,7 @@ Work through items one PR at a time. When an item is done, delete it from this f
 
 ## P0: Bugs
 
-4. **Failures that escape the result** ([ADR-0002](adr/0002-result-pattern-and-error-handling.md)): `HttpRequestException` and timeouts are thrown; mappers throw `ArgumentException` when `Rates` is null; `catch (Exception)` blocks in `GetNbpAsync`.
+None open.
 
 ## P1: NBP correctness
 

src/OpenUrzednik.Core/Errors/RequestTimeoutError.cs

@@ -0,0 +1,32 @@
+using OpenUrzednik.Core.Exceptions;
+
+namespace OpenUrzednik.Core.Errors;
+
+/// <summary>
+/// Represents an error indicating that the service did not respond within the configured timeout.
+/// Cancellation requested by the caller is not an error; it throws <see cref="OperationCanceledException"/>.
+/// </summary>
+/// <param name="message">The message associated with the error.</param>
+/// <param name="timeout">The timeout that elapsed, if known.</param>
+/// <param name="exception">The exception that signalled the timeout, if any.</param>
+public sealed class RequestTimeoutError(string message, TimeSpan? timeout = null, Exception? exception = null) : OpenUrzednikError(ErrorCode, message)
+{
+    public const string ErrorCode = "requestTimeout";
+
+    /// <summary>
+    /// Gets the timeout that elapsed, if known.
+    /// </summary>
+    public TimeSpan? Timeout { get; } = timeout;
+
+    /// <summary>
+    /// Gets the exception that signalled the timeout, if any.
+    /// </summary>
+    public Exception? Exception { get; } = exception;
+
+    /// <inheritdoc />
+    /// <remarks>The returned exception keeps <see cref="Exception"/> as its <see cref="System.Exception.InnerException"/>.</remarks>
+    public override Exception ToException()
+        => Exception is null
+            ? new RequestTimeoutException(Message)
+            : new RequestTimeoutException(Message, Exception);
+}

src/OpenUrzednik.Core/Errors/ServiceUnavailableError.cs

@@ -3,15 +3,25 @@
 namespace OpenUrzednik.Core.Errors;
 
 /// <summary>
-/// Represents an error indicating that the service is currently unavailable.
+/// Represents an error indicating that the service is currently unavailable: it returned a 5xx status,
+/// or it could not be reached because of a network failure.
 /// </summary>
 /// <param name="message">The message associated with the error.</param>
 /// <param name="statusCode">HTTP status code of the response that caused the error, if any.</param>
-public sealed class ServiceUnavailableError(string message, int? statusCode = null) : OpenUrzednikError(ErrorCode, message, statusCode)
+/// <param name="exception">The exception that caused the error (e.g. an <see cref="System.Net.Http.HttpRequestException"/>), if any.</param>
+public sealed class ServiceUnavailableError(string message, int? statusCode = null, Exception? exception = null) : OpenUrzednikError(ErrorCode, message, statusCode)
 {
     public const string ErrorCode = "serviceUnavailable";
 
+    /// <summary>
+    /// Gets the exception that caused the error, if any.
+    /// </summary>
+    public Exception? Exception { get; } = exception;
+
     /// <inheritdoc />
+    /// <remarks>The returned exception keeps <see cref="Exception"/> as its <see cref="System.Exception.InnerException"/>.</remarks>
     public override Exception ToException()
-        => new ServiceUnavailableException(Message);
+        => Exception is null
+            ? new ServiceUnavailableException(Message)
+            : new ServiceUnavailableException(Message, Exception);
 }

src/OpenUrzednik.Core/Exceptions/RequestTimeoutException.cs

@@ -0,0 +1,24 @@
+using OpenUrzednik.Core.Errors;
+
+namespace OpenUrzednik.Core.Exceptions;
+
+/// <summary>
+/// Represents an exception that is thrown when the service does not respond within the configured timeout.
+/// </summary>
+public sealed class RequestTimeoutException : OpenUrzednikException
+{
+    /// <summary>
+    /// Initializes a new instance of the <see cref="RequestTimeoutException"/> class with a specified error message.
+    /// </summary>
+    /// <param name="message">The error message.</param>
+    public RequestTimeoutException(string message)
+        : base(RequestTimeoutError.ErrorCode, message) { }
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="RequestTimeoutException"/> class with a specified error message and a reference to the inner exception that is the cause of this exception.
+    /// </summary>
+    /// <param name="message">The error message.</param>
+    /// <param name="innerException">The inner exception.</param>
+    public RequestTimeoutException(string message, Exception innerException)
+        : base(RequestTimeoutError.ErrorCode, message, innerException) { }
+}

src/OpenUrzednik.Nbp/Common/NbpPayload.cs

@@ -0,0 +1,40 @@
+using OpenUrzednik.Core;
+using OpenUrzednik.Core.Errors;
+using OpenUrzednik.Core.Extensions;
+using OpenUrzednik.Core.Telemetry;
+using OpenUrzednik.Nbp.Telemetry;
+
+namespace OpenUrzednik.Nbp.Common;
+
+/// <summary>
+/// Converts deserialized NBP payloads to public models. Mappers reject bad API data (e.g. <c>"rates": null</c>)
+/// with <see cref="InvalidPayloadException"/>, and <see cref="Map{TDto, TModel}"/> returns it as a <see cref="SerializationError"/>.
+/// </summary>
+internal static class NbpPayload
+{
+    internal static void EnsurePresent(object? value, string name)
+    {
+        if (value is null)
+            throw new InvalidPayloadException($"NBP API response is missing '{name}'.");
+    }
+
+    internal static OpenUrzednikResult<TModel> Map<TDto, TModel>(TDto dto, Func<TDto, TModel> map, NbpTelemetryProvider telemetryProvider, IOpenUrzednikSpan traceSpan)
+    {
+        try
+        {
+            return OpenUrzednikResult.Success(map(dto));
+        }
+        catch (InvalidPayloadException ex)
+        {
+            telemetryProvider.Logger.Log(OpenUrzednikLogLevel.Error, ex, "Failed to map NBP response: {reason}", "reason", ex.Message);
+            var error = new SerializationError(ex.Message);
+            traceSpan.RecordError(error);
+            return OpenUrzednikResult.Failure<TModel>(error);
+        }
+    }
+}
+
+/// <summary>
+/// Thrown by mappers when a deserialized NBP payload is missing data the model needs.
+/// </summary>
+internal sealed class InvalidPayloadException(string message) : Exception(message);

src/OpenUrzednik.Nbp/Currency/Mapper.cs

@@ -1,3 +1,4 @@
+using OpenUrzednik.Nbp.Common;
 using OpenUrzednik.Nbp.Dto;
 
 namespace OpenUrzednik.Nbp.Currency;
@@ -6,9 +7,8 @@ internal static class Mapper
 {
     internal static CurrencyExchangeRates MapToCurrencyExchangeRates(CurrencyExchangeRatesDto dto)
     {
-        ArgumentNullException.ThrowIfNull(dto);
-        if (dto.Rates is null)
-            throw new ArgumentException($"{nameof(dto.Rates)} cannot be null.", nameof(dto));
+        NbpPayload.EnsurePresent(dto, "item");
+        NbpPayload.EnsurePresent(dto.Rates, "rates");
 
         var rates = new ExchangeRate[dto.Rates.Length];
         for (int i = 0; i < rates.Length; i++)
@@ -19,16 +19,15 @@ internal static CurrencyExchangeRates MapToCurrencyExchangeRates(CurrencyExchang
 
     internal static ExchangeRate MapToCurrencyExchangeRate(CurrencyExchangeRateDto dto)
     {
-        ArgumentNullException.ThrowIfNull(dto);
+        NbpPayload.EnsurePresent(dto, "item");
 
         return new ExchangeRate(dto.TableId, dto.PublicationDate, dto.Price);
     }
 
     internal static BuySellExchangeRates MapToBuySellExchangeRates(BuySellCurrencyExchangeRatesDto dto)
     {
-        ArgumentNullException.ThrowIfNull(dto);
-        if (dto.Rates is null)
-            throw new ArgumentException($"{nameof(dto.Rates)} cannot be null.", nameof(dto));
+        NbpPayload.EnsurePresent(dto, "item");
+        NbpPayload.EnsurePresent(dto.Rates, "rates");
 
         var rates = new BuySellExchangeRate[dto.Rates.Length];
         for (var i = 0; i < rates.Length; i++)
@@ -39,7 +38,7 @@ internal static BuySellExchangeRates MapToBuySellExchangeRates(BuySellCurrencyEx
 
     internal static BuySellExchangeRate MapToBuySellExchangeRate(BuySellCurrencyExchangeRateDto dto)
     {
-        ArgumentNullException.ThrowIfNull(dto);
+        NbpPayload.EnsurePresent(dto, "item");
 
         return new BuySellExchangeRate(dto.TableId, dto.PublicationDate, dto.Buy, dto.Sell);
     }

src/OpenUrzednik.Nbp/Currency/NbpCurrencyExchangeRateClient.BuySell.cs

@@ -1,6 +1,7 @@
 using OpenUrzednik.Core;
 using OpenUrzednik.Core.Extensions;
 using OpenUrzednik.Core.Telemetry;
+using OpenUrzednik.Nbp.Common;
 using OpenUrzednik.Nbp.Extensions;
 using OpenUrzednik.Nbp.UrlBuilder;
 using OpenUrzednik.Nbp.Validation;
@@ -28,7 +29,7 @@ public async Task<OpenUrzednikResult<BuySellExchangeRates>> GetBuySellLatestAsyn
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.Latest(), JsonContext.BuySellCurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToBuySellExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToBuySellExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -56,7 +57,7 @@ public async Task<OpenUrzednikResult<BuySellExchangeRates>> GetBuySellTopCountAs
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.ForTopCount(topCount), JsonContext.BuySellCurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToBuySellExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToBuySellExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -81,7 +82,7 @@ public async Task<OpenUrzednikResult<BuySellExchangeRates>> GetBuySellTodayAsync
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.Today(), JsonContext.BuySellCurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToBuySellExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToBuySellExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -109,7 +110,7 @@ public async Task<OpenUrzednikResult<BuySellExchangeRates>> GetBuySellAsync(stri
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.ForDate(date), JsonContext.BuySellCurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToBuySellExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToBuySellExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -139,7 +140,7 @@ public async Task<OpenUrzednikResult<BuySellExchangeRates>> GetBuySellAsync(stri
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.ForDateRange(from, to), JsonContext.BuySellCurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToBuySellExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToBuySellExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);

src/OpenUrzednik.Nbp/Currency/NbpCurrencyExchangeRateClient.Currency.cs

@@ -1,6 +1,7 @@
 using OpenUrzednik.Core;
 using OpenUrzednik.Core.Extensions;
 using OpenUrzednik.Core.Telemetry;
+using OpenUrzednik.Nbp.Common;
 using OpenUrzednik.Nbp.Extensions;
 using OpenUrzednik.Nbp.Table;
 using OpenUrzednik.Nbp.Validation;
@@ -31,7 +32,7 @@ public async Task<OpenUrzednikResult<CurrencyExchangeRates>> GetLatestAsync(stri
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.Latest(), JsonContext.CurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToCurrencyExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToCurrencyExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -61,7 +62,7 @@ public async Task<OpenUrzednikResult<CurrencyExchangeRates>> GetTopCountAsync(st
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.ForTopCount(topCount), JsonContext.CurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToCurrencyExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToCurrencyExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -89,7 +90,7 @@ public async Task<OpenUrzednikResult<CurrencyExchangeRates>> GetTodayAsync(strin
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.Today(), JsonContext.CurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToCurrencyExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToCurrencyExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -119,7 +120,7 @@ public async Task<OpenUrzednikResult<CurrencyExchangeRates>> GetAsync(string cur
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.ForDate(date), JsonContext.CurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToCurrencyExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToCurrencyExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);
@@ -151,7 +152,7 @@ public async Task<OpenUrzednikResult<CurrencyExchangeRates>> GetAsync(string cur
         var requestResult = await _httpClient.GetNbpAsync(urlBuilder.ForDateRange(from, to), JsonContext.CurrencyExchangeRatesDto, _telemetryProvider, _timeProvider, cancellationToken);
 
         if (requestResult.IsSuccess)
-            return OpenUrzednikResult.Success(Mapper.MapToCurrencyExchangeRates(requestResult.Value));
+            return NbpPayload.Map(requestResult.Value, Mapper.MapToCurrencyExchangeRates, _telemetryProvider, traceSpan);
 
         traceSpan.RecordErrors(requestResult.Errors);
         return OpenUrzednikResult.Failure(requestResult.Errors);