Stop the GL account-code check from being able to abort startup
The catch filter only matched DomainException/OperationCanceledException, so anything else escaped a hosted service and took the host down. One such path was mine: `"items": null` deserializes over the property initialiser, so `data.Items.Select(...)` would NullReferenceException straight past the filter. Fixed at both ends - the reader tolerates a null or absent items array, and the catch is now a deliberate catch-all that only lets a genuine host shutdown through. This is a diagnostic; a diagnostic that can core-dump the thing it diagnoses is worse than no diagnostic at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -91,6 +91,30 @@ public class GlAccountCodeValidatorTests
|
|||||||
await validator.StartAsync(CancellationToken.None);
|
await validator.StartAsync(CancellationToken.None);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The earlier catch filter only matched DomainException/OperationCanceledException, so anything
|
||||||
|
/// else — a parse failure, or a null `items` NRE in the reader — escaped and aborted the host.
|
||||||
|
/// A diagnostic must not be able to crash the thing it is diagnosing.
|
||||||
|
/// </summary>
|
||||||
|
[Theory]
|
||||||
|
[MemberData(nameof(UnexpectedFailures))]
|
||||||
|
public async Task StartAsync_DoesNotThrow_OnAnyUnexpectedFailure(Exception failure)
|
||||||
|
{
|
||||||
|
var validator = CreateValidator(
|
||||||
|
new() { ["Grn:GlInventoryAccountCode"] = "1100" },
|
||||||
|
new StubGl(failure));
|
||||||
|
|
||||||
|
await validator.StartAsync(CancellationToken.None);
|
||||||
|
}
|
||||||
|
|
||||||
|
public static TheoryData<Exception> UnexpectedFailures() =>
|
||||||
|
[
|
||||||
|
new NullReferenceException("items was null"),
|
||||||
|
new global::System.Text.Json.JsonException("unexpected shape"),
|
||||||
|
new InvalidOperationException("something else entirely"),
|
||||||
|
new HttpRequestException("connection reset"),
|
||||||
|
];
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task StartAsync_DoesNotThrow_WhenTheChartOfAccountsReadIsCancelled()
|
public async Task StartAsync_DoesNotThrow_WhenTheChartOfAccountsReadIsCancelled()
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -1,5 +1,4 @@
|
|||||||
using ERPCore.Services.Interfaces;
|
using ERPCore.Services.Interfaces;
|
||||||
using ERPCore.System.Errors;
|
|
||||||
|
|
||||||
namespace ERPCore.Infra.Gl;
|
namespace ERPCore.Infra.Gl;
|
||||||
|
|
||||||
@@ -61,10 +60,12 @@ public sealed class GlAccountCodeValidator(
|
|||||||
timeout.CancelAfter(FetchTimeout);
|
timeout.CancelAfter(FetchTimeout);
|
||||||
present = await gl.ListAccountCodesAsync(timeout.Token);
|
present = await gl.ListAccountCodesAsync(timeout.Token);
|
||||||
}
|
}
|
||||||
catch (Exception e) when (e is DomainException or OperationCanceledException && !ct.IsCancellationRequested)
|
// Catch-all on purpose. This is a diagnostic, and a diagnostic that can abort the host is
|
||||||
|
// worse than no diagnostic — whatever GL answers with (unreachable, a timeout, a redirect,
|
||||||
|
// an unparseable body, a shape this reader mishandles) must degrade to a log line, never a
|
||||||
|
// core-dump. The only exception that still propagates is a genuine host shutdown.
|
||||||
|
catch (Exception e) when (!ct.IsCancellationRequested)
|
||||||
{
|
{
|
||||||
// Unreachable, timed out, or rejected the read — can't verify, so don't block startup.
|
|
||||||
// The per-flow 404 remains the backstop it has always been.
|
|
||||||
logger.LogWarning(e,
|
logger.LogWarning(e,
|
||||||
"Could not read GL's chart of accounts at startup; {Count} configured account code(s) left unverified.",
|
"Could not read GL's chart of accounts at startup; {Count} configured account code(s) left unverified.",
|
||||||
expected.Length);
|
expected.Length);
|
||||||
|
|||||||
@@ -56,9 +56,10 @@ public sealed class GeneralLedgerService : IGeneralLedgerService
|
|||||||
// `parentAccountId` is a field, not a nested `children` array).
|
// `parentAccountId` is a field, not a nested `children` array).
|
||||||
var response = await _client.SendAsync(HttpMethod.Get, "accounts", null, null, null, ct);
|
var response = await _client.SendAsync(HttpMethod.Get, "accounts", null, null, null, ct);
|
||||||
var data = ParseSuccess<GlAccountListData>(response);
|
var data = ParseSuccess<GlAccountListData>(response);
|
||||||
return data.Items
|
// `"items": null` deserializes over the property initialiser, so the null check is load-bearing.
|
||||||
|
return (data.Items ?? [])
|
||||||
|
.Where(a => a is not null && !string.IsNullOrWhiteSpace(a.AccountCode))
|
||||||
.Select(a => a.AccountCode)
|
.Select(a => a.AccountCode)
|
||||||
.Where(c => !string.IsNullOrWhiteSpace(c))
|
|
||||||
.ToHashSet(StringComparer.Ordinal);
|
.ToHashSet(StringComparer.Ordinal);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -111,7 +112,7 @@ public sealed class GeneralLedgerService : IGeneralLedgerService
|
|||||||
|
|
||||||
private sealed class GlAccountListData
|
private sealed class GlAccountListData
|
||||||
{
|
{
|
||||||
public List<GlAccountData> Items { get; set; } = [];
|
public List<GlAccountData>? Items { get; set; }
|
||||||
}
|
}
|
||||||
|
|
||||||
private sealed class GlAccountData
|
private sealed class GlAccountData
|
||||||
|
|||||||
Reference in New Issue
Block a user