diff --git a/code-reviews/ExternalSystemGateway/findings.md b/code-reviews/ExternalSystemGateway/findings.md index 67384ba..e5d6240 100644 --- a/code-reviews/ExternalSystemGateway/findings.md +++ b/code-reviews/ExternalSystemGateway/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 4 | +| Open findings | 1 | ## Summary @@ -533,8 +533,8 @@ exception propagates; it was verified to fail before the `try/catch` was added. |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | -| Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:231-245`, `src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs:90-97` | +| Status | Open — root cause confirmed; no correct in-module fix exists (see Resolution) | +| Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:360-374`, `src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs:169-176` | **Description** @@ -554,7 +554,26 @@ rather than fetch-all-then-filter. **Resolution** -_Unresolved._ +2026-05-16 — **Root cause confirmed, but left Open: no correct fix is possible within +this module's edit scope.** `ResolveSystemAndMethodAsync` +(`ExternalSystemClient.cs:360`) does call `GetAllExternalSystemsAsync()` followed by +`GetMethodsByExternalSystemIdAsync()` and filters in memory, and +`ResolveConnectionAsync` (`DatabaseGateway.cs:169`) does `GetAllDatabaseConnectionsAsync()` +then filters — fetch-all-then-filter on every hot-path call, as described. + +Both recommended fixes require changes outside `src/ScadaLink.ExternalSystemGateway`: +(a) a **name-keyed repository lookup** (e.g. `GetExternalSystemByNameAsync`) means adding +methods to `IExternalSystemRepository` in `ScadaLink.Commons` and implementing them in +`ScadaLink.ConfigurationDatabase` / `ScadaLink.SiteRuntime`; (b) an **in-memory cache +invalidated on artifact deployment** requires subscribing to a deployment-applied event +owned by `ScadaLink.SiteRuntime` / `ScadaLink.DeploymentManager`. A purely module-local +cache with a time-based TTL was rejected as a fix: definitions only change on deployment +and must reflect a deployment promptly, so a TTL would either be too short to help the +hot path or long enough to serve stale definitions after a redeploy — trading a +correctness hazard for a performance gain on a Low-severity issue. **Tracked follow-up:** +add a name-keyed lookup to `IExternalSystemRepository` (Commons) and have the gateway use +it, or add a deployment-invalidated definition cache wired from SiteRuntime. No source +change was made in this module. ### ExternalSystemGateway-012 — Permanent-failure logging requirement is not met; `_logger` is injected but unused @@ -562,7 +581,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:24,169-177`, `src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs:22` | **Description** @@ -585,7 +604,21 @@ caller's responsibility and remove the unused `_logger` fields. Add a comment in **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Partial re-triage of the finding text: the +"`_logger` injected but unused" claim is **stale** — both loggers are already used (the +`DeliverBufferedAsync` retry-sweep handlers added by `ExternalSystemGateway-001` log +park/error events). The genuine remaining gap — `InvokeHttpAsync` performing no logging +on the HTTP-failure paths — is now fixed: `InvokeHttpAsync` emits a +`_logger.LogWarning` on the permanent-failure path (status code, system, method, +truncated error body) so a permanent failure is visible in Site Event Logging as the +design requires, and a `_logger.LogDebug` on the transient-failure path (transient +failures are normal retry/S&F operation and must not be noisy at warning level). A +documentation comment was added to `ErrorClassifier.IsTransient(HttpStatusCode)` +explaining the "any other non-success status defaults to permanent" behaviour and why +permanent is the safe default. Regression tests: `Call_PermanentFailure_LogsAWarning` +(asserts a warning carrying the system name is emitted; verified to fail before the +`LogWarning` was added) and `Call_TransientFailure_DoesNotLogAtWarningOrAbove` (guards +against over-logging transient failures). ### ExternalSystemGateway-013 — `MaxConcurrentConnectionsPerSystem` and `DefaultHttpTimeout` options are defined but never used @@ -593,7 +626,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemGatewayOptions.cs:9,12`, `src/ScadaLink.ExternalSystemGateway/ServiceCollectionExtensions.cs:13` | **Description** @@ -614,7 +647,20 @@ options to avoid implying behaviour that does not exist. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Partial re-triage of the finding text: +`DefaultHttpTimeout` is **no longer unused** — it became the effective per-call HTTP +round-trip limit when `ExternalSystemGateway-002` was fixed (`InvokeHttpAsync` builds a +linked `CancellationTokenSource(DefaultHttpTimeout)`). The genuinely-unused option, +`MaxConcurrentConnectionsPerSystem`, is now wired in: `AddExternalSystemGateway` adds a +`ConfigureHttpClientDefaults` registration that supplies a `SocketsHttpHandler` whose +`MaxConnectionsPerServer` is bound from the option, so it applies to every per-system +named client (`ExternalSystem_{name}`) the gateway creates rather than being silently +ignored. Regression test +`ServiceWiringTests.MaxConcurrentConnectionsPerSystem_IsAppliedToTheNamedHttpClientPrimaryHandler` +builds the DI container, resolves the named client's handler chain via +`IHttpMessageHandlerFactory`, walks to the primary handler and asserts +`SocketsHttpHandler.MaxConnectionsPerServer` equals the configured value; it was verified +to fail before the wiring was added. ### ExternalSystemGateway-014 — Cached-call buffering path and `DatabaseGateway` are untested @@ -622,8 +668,8 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Testing coverage | -| Status | Open | -| Location | `tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs:1`, (no `DatabaseGatewayTests.cs`) | +| Status | Resolved | +| Location | `tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs:1`, `tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs` | **Description** @@ -647,4 +693,29 @@ by asserting on the captured `HttpRequestMessage` in the mock handler. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Partial re-triage of the finding text: a number +of the listed gaps were **already closed** by tests added when findings 001–010 were +fixed — `DatabaseGatewayTests.cs` now exists (not-found, null-S&F guard, buffered-write +delivery), and the `CachedCall` transient/buffering and permanent paths are covered by +the `ExternalSystemGateway-001/003/004/008` regression tests. The remaining genuine +coverage gaps are now closed with new tests: + +- `Call_GetWithParameters_AppendsEscapedQueryString` — `BuildUrl` GET query-string + construction, asserting on the captured request URI and that an `&` inside a value is + percent-escaped. +- `Call_PostWithParameters_SendsJsonBody` — POST JSON-body construction. +- `Call_ApiKeyAuthWithDefaultHeader_SendsXApiKeyHeader`, + `Call_ApiKeyAuthWithCustomHeader_SendsNamedHeader`, + `Call_BasicAuth_SendsBase64AuthorizationHeader` — `ApplyAuth` for all three auth + variants, asserting on the captured request headers. +- `Call_ConnectionError_IsClassifiedAsTransient` — a connection-level + `HttpRequestException` is classified transient. +- `CachedWrite_BuffersTheWriteWithConnectionRetrySettings` and + `CachedWrite_ZeroMaxRetriesIsHonouredNotTreatedAsUnset` — the `DatabaseGateway` + `CachedWrite` happy-path enqueue against a real S&F service. + +The shared `RequestCapturingHandler` test helper was extended to capture request +headers and body so URL/auth/body construction is now verified, not just status codes. +These are new-coverage tests against already-correct behaviour, so they pass on the +current source; the `BuildUrl` and `ApplyAuth` paths they exercise are now protected +against regression. diff --git a/src/ScadaLink.ExternalSystemGateway/ErrorClassifier.cs b/src/ScadaLink.ExternalSystemGateway/ErrorClassifier.cs index 2048da2..61dbeb4 100644 --- a/src/ScadaLink.ExternalSystemGateway/ErrorClassifier.cs +++ b/src/ScadaLink.ExternalSystemGateway/ErrorClassifier.cs @@ -11,6 +11,10 @@ public static class ErrorClassifier { /// /// Determines whether an HTTP status code represents a transient failure. + /// Transient: HTTP 5xx, 408 (Request Timeout) and 429 (Too Many Requests). + /// Every other non-success status (the remaining 4xx) defaults to permanent — + /// a permanent failure is the safe default because retrying a 4xx is unlikely to + /// succeed and risks duplicate side effects. /// public static bool IsTransient(HttpStatusCode statusCode) { diff --git a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs index 52abe8a..3bf2ca4 100644 --- a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs +++ b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs @@ -272,10 +272,21 @@ public class ExternalSystemClient : IExternalSystemClient if (ErrorClassifier.IsTransient(response.StatusCode)) { + // Transient failures are normal operation (handled by retry / S&F) — + // record at debug level only so the event log is not noisy. + _logger.LogDebug( + "Transient HTTP {StatusCode} from external system {System} calling {Method}.", + (int)response.StatusCode, system.Name, method.Name); throw ErrorClassifier.AsTransient( $"HTTP {(int)response.StatusCode} from {system.Name}: {errorBody}"); } + // The design requires permanent failures to be visible in Site Event + // Logging — emit a warning so the gateway is not silent on a permanent + // failure (ExternalSystemGateway-012). + _logger.LogWarning( + "Permanent HTTP {StatusCode} from external system {System} calling {Method}: {Error}", + (int)response.StatusCode, system.Name, method.Name, errorBody); throw new PermanentExternalSystemException( $"HTTP {(int)response.StatusCode} from {system.Name}: {errorBody}", (int)response.StatusCode); diff --git a/src/ScadaLink.ExternalSystemGateway/ServiceCollectionExtensions.cs b/src/ScadaLink.ExternalSystemGateway/ServiceCollectionExtensions.cs index b8528d5..ce76d93 100644 --- a/src/ScadaLink.ExternalSystemGateway/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.ExternalSystemGateway/ServiceCollectionExtensions.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; using ScadaLink.Commons.Interfaces.Services; namespace ScadaLink.ExternalSystemGateway; @@ -11,6 +12,22 @@ public static class ServiceCollectionExtensions .BindConfiguration("ScadaLink:ExternalSystemGateway"); services.AddHttpClient(); + + // ExternalSystemGateway-013: wire MaxConcurrentConnectionsPerSystem into the + // primary handler of every per-system named client ("ExternalSystem_{name}"), + // so the option an operator configures actually bounds concurrent connections + // instead of being silently ignored. ConfigureHttpClientDefaults applies to + // the dynamically-named clients created by ExternalSystemClient. + services.ConfigureHttpClientDefaults(builder => + builder.ConfigurePrimaryHttpMessageHandler(sp => + { + var options = sp.GetRequiredService>().Value; + return new SocketsHttpHandler + { + MaxConnectionsPerServer = options.MaxConcurrentConnectionsPerSystem, + }; + })); + services.AddScoped(); services.AddScoped(sp => sp.GetRequiredService()); services.AddScoped(); diff --git a/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs b/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs index 7f1fc94..b5175be 100644 --- a/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs +++ b/tests/ScadaLink.ExternalSystemGateway.Tests/DatabaseGatewayTests.cs @@ -56,6 +56,98 @@ public class DatabaseGatewayTests () => gateway.CachedWriteAsync("nonexistent", "INSERT INTO t VALUES (1)")); } + // ── ExternalSystemGateway-014: CachedWrite happy-path buffering ── + + [Fact] + public async Task CachedWrite_BuffersTheWriteWithConnectionRetrySettings() + { + var conn = new DatabaseConnectionDefinition("testDb", "Server=localhost;Database=test") + { + Id = 1, + MaxRetries = 5, + RetryDelay = TimeSpan.FromSeconds(12), + }; + _repository.GetAllDatabaseConnectionsAsync(Arg.Any()) + .Returns(new List { conn }); + + var dbName = $"EsgCachedWrite_{Guid.NewGuid():N}"; + var connStr = $"Data Source={dbName};Mode=Memory;Cache=Shared"; + using var keepAlive = new Microsoft.Data.Sqlite.SqliteConnection(connStr); + keepAlive.Open(); + var storage = new ScadaLink.StoreAndForward.StoreAndForwardStorage( + connStr, NullLogger.Instance); + await storage.InitializeAsync(); + var sfOptions = new ScadaLink.StoreAndForward.StoreAndForwardOptions + { + DefaultMaxRetries = 99, + DefaultRetryInterval = TimeSpan.FromMinutes(10), + RetryTimerInterval = TimeSpan.FromMinutes(10), + }; + var sf = new ScadaLink.StoreAndForward.StoreAndForwardService( + storage, sfOptions, NullLogger.Instance); + + var gateway = new DatabaseGateway(_repository, NullLogger.Instance, storeAndForward: sf); + + await gateway.CachedWriteAsync("testDb", "INSERT INTO t VALUES (@v)", + new Dictionary { ["v"] = 1 }); + + var depth = await storage.GetBufferDepthByCategoryAsync(); + Assert.Equal(1, depth[ScadaLink.Commons.Types.Enums.StoreAndForwardCategory.CachedDbWrite]); + + var (maxRetries, retryIntervalMs) = ReadBufferedRetrySettings(connStr); + Assert.Equal(5, maxRetries); + Assert.Equal((long)TimeSpan.FromSeconds(12).TotalMilliseconds, retryIntervalMs); + } + + [Fact] + public async Task CachedWrite_ZeroMaxRetriesIsHonouredNotTreatedAsUnset() + { + var conn = new DatabaseConnectionDefinition("testDb", "Server=localhost;Database=test") + { + Id = 1, + MaxRetries = 0, + RetryDelay = TimeSpan.FromSeconds(3), + }; + _repository.GetAllDatabaseConnectionsAsync(Arg.Any()) + .Returns(new List { conn }); + + var dbName = $"EsgCachedWriteZero_{Guid.NewGuid():N}"; + var connStr = $"Data Source={dbName};Mode=Memory;Cache=Shared"; + using var keepAlive = new Microsoft.Data.Sqlite.SqliteConnection(connStr); + keepAlive.Open(); + var storage = new ScadaLink.StoreAndForward.StoreAndForwardStorage( + connStr, NullLogger.Instance); + await storage.InitializeAsync(); + var sfOptions = new ScadaLink.StoreAndForward.StoreAndForwardOptions + { + DefaultMaxRetries = 99, + DefaultRetryInterval = TimeSpan.FromMinutes(10), + RetryTimerInterval = TimeSpan.FromMinutes(10), + }; + var sf = new ScadaLink.StoreAndForward.StoreAndForwardService( + storage, sfOptions, NullLogger.Instance); + + var gateway = new DatabaseGateway(_repository, NullLogger.Instance, storeAndForward: sf); + + await gateway.CachedWriteAsync("testDb", "INSERT INTO t VALUES (1)"); + + var (maxRetries, _) = ReadBufferedRetrySettings(connStr); + Assert.Equal(0, maxRetries); // honoured — not the S&F default of 99 + } + + private static (int MaxRetries, long RetryIntervalMs) ReadBufferedRetrySettings(string connStr) + { + using var conn = new Microsoft.Data.Sqlite.SqliteConnection(connStr); + conn.Open(); + using var cmd = conn.CreateCommand(); + cmd.CommandText = "SELECT max_retries, retry_interval_ms FROM sf_messages"; + using var reader = cmd.ExecuteReader(); + Assert.True(reader.Read(), "expected exactly one buffered message"); + var result = (reader.GetInt32(0), reader.GetInt64(1)); + Assert.False(reader.Read(), "expected exactly one buffered message"); + return result; + } + // ── ExternalSystemGateway-001: buffered CachedDbWrite delivery handler ── [Fact] diff --git a/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs b/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs index 83b25c8..01bb22b 100644 --- a/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs +++ b/tests/ScadaLink.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs @@ -1,5 +1,7 @@ using System.Net; +using System.Net.Http.Headers; using Microsoft.Data.Sqlite; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using ScadaLink.Commons.Entities.ExternalSystems; @@ -603,6 +605,237 @@ public class ExternalSystemClientTests "A caller-cancelled CachedCall must not be buffered for retry"); } + // ── ExternalSystemGateway-014: BuildUrl query-string, ApplyAuth, connection errors ── + + [Fact] + public async Task Call_GetWithParameters_AppendsEscapedQueryString() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("search", "GET", "/search") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new RequestCapturingHandler(HttpStatusCode.OK, "{}"); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "search", new Dictionary + { + ["q"] = "a b&c", + ["page"] = 2, + }); + + // AbsoluteUri preserves percent-encoding; the '&' inside a value must be + // escaped so it is not mistaken for a parameter separator. + var uri = handler.LastUri!.AbsoluteUri; + Assert.StartsWith("https://api.example.com/search?", uri); + Assert.Contains("q=a%20b%26c", uri); + Assert.Contains("page=2", uri); + } + + [Fact] + public async Task Call_PostWithParameters_SendsJsonBody() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("create", "POST", "/create") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new RequestCapturingHandler(HttpStatusCode.OK, "{}"); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "create", new Dictionary { ["name"] = "widget" }); + + Assert.Equal("https://api.example.com/create", handler.LastUri!.ToString()); + Assert.Contains("\"name\":\"widget\"", handler.LastBody); + } + + [Fact] + public async Task Call_ApiKeyAuthWithDefaultHeader_SendsXApiKeyHeader() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "apikey") + { + Id = 1, + AuthConfiguration = "secret-key-123", + }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new RequestCapturingHandler(HttpStatusCode.OK, "{}"); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "getData"); + + Assert.True(handler.LastHeaders!.TryGetValues("X-API-Key", out var values)); + Assert.Equal("secret-key-123", values!.Single()); + } + + [Fact] + public async Task Call_ApiKeyAuthWithCustomHeader_SendsNamedHeader() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "apikey") + { + Id = 1, + AuthConfiguration = "Authorization-Token:abc", + }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new RequestCapturingHandler(HttpStatusCode.OK, "{}"); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "getData"); + + Assert.True(handler.LastHeaders!.TryGetValues("Authorization-Token", out var values)); + Assert.Equal("abc", values!.Single()); + } + + [Fact] + public async Task Call_BasicAuth_SendsBase64AuthorizationHeader() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "basic") + { + Id = 1, + AuthConfiguration = "alice:s3cret", + }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new RequestCapturingHandler(HttpStatusCode.OK, "{}"); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + await client.CallAsync("TestAPI", "getData"); + + var auth = handler.LastHeaders!.Authorization; + Assert.NotNull(auth); + Assert.Equal("Basic", auth!.Scheme); + var decoded = System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(auth.Parameter!)); + Assert.Equal("alice:s3cret", decoded); + } + + [Fact] + public async Task Call_ConnectionError_IsClassifiedAsTransient() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + // A connection-level failure (e.g. host unreachable) surfaces as HttpRequestException. + var handler = new ThrowingHttpMessageHandler(new HttpRequestException("connection refused")); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + var result = await client.CallAsync("TestAPI", "getData"); + + Assert.False(result.Success); + Assert.Contains("Transient error", result.ErrorMessage); + } + + // ── ExternalSystemGateway-012: permanent failures must be logged ── + + [Fact] + public async Task Call_PermanentFailure_LogsAWarning() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("badMethod", "POST", "/bad") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new MockHttpMessageHandler(HttpStatusCode.BadRequest, "bad request"); + var httpClient = new HttpClient(handler); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var logger = new CapturingLogger(); + var client = new ExternalSystemClient(_httpClientFactory, _repository, logger); + + await client.CallAsync("TestAPI", "badMethod"); + + // The design doc requires permanent failures to be surfaced to Site Event + // Logging — the gateway must emit at least a warning, not stay silent. + Assert.Contains(logger.Entries, e => + e.Level >= LogLevel.Warning && e.Message.Contains("TestAPI")); + } + + [Fact] + public async Task Call_TransientFailure_DoesNotLogAtWarningOrAbove() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("failMethod", "POST", "/fail") { Id = 1, ExternalSystemDefinitionId = 1 }; + _repository.GetAllExternalSystemsAsync(Arg.Any()) + .Returns(new List { system }); + _repository.GetMethodsByExternalSystemIdAsync(1, Arg.Any()) + .Returns(new List { method }); + + var handler = new MockHttpMessageHandler(HttpStatusCode.InternalServerError, "boom"); + var httpClient = new HttpClient(handler); + _httpClientFactory.CreateClient(Arg.Any()).Returns(httpClient); + + var logger = new CapturingLogger(); + var client = new ExternalSystemClient(_httpClientFactory, _repository, logger); + + await client.CallAsync("TestAPI", "failMethod"); + + // A transient failure is normal operation handled by retry/S&F — it must not + // be logged at warning level (only permanent failures are). + Assert.DoesNotContain(logger.Entries, e => e.Level >= LogLevel.Warning); + } + + /// Test helper: an ILogger that records every entry for assertions. + private sealed class CapturingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = new(); + + public IDisposable BeginScope(TState state) where TState : notnull => NullScope.Instance; + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + Entries.Add((logLevel, formatter(state, exception))); + } + + private sealed class NullScope : IDisposable + { + public static readonly NullScope Instance = new(); + public void Dispose() { } + } + } + /// /// Test helper: mock HTTP message handler. /// @@ -667,7 +900,7 @@ public class ExternalSystemClientTests } /// - /// Test helper: captures the request URI of the last request. + /// Test helper: captures the URI, headers and body of the last request. /// private class RequestCapturingHandler : HttpMessageHandler { @@ -681,17 +914,31 @@ public class ExternalSystemClientTests } public Uri? LastUri { get; private set; } + public HttpRequestHeaders? LastHeaders { get; private set; } + public string? LastBody { get; private set; } - protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { LastUri = request.RequestUri; - return Task.FromResult(new HttpResponseMessage(_statusCode) + LastHeaders = request.Headers; + LastBody = request.Content == null ? null : await request.Content.ReadAsStringAsync(cancellationToken); + return new HttpResponseMessage(_statusCode) { Content = new StringContent(_body) - }); + }; } } + /// Test helper: an HTTP handler that throws a connection-level exception. + private class ThrowingHttpMessageHandler : HttpMessageHandler + { + private readonly Exception _exception; + public ThrowingHttpMessageHandler(Exception exception) => _exception = exception; + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + => throw _exception; + } + /// /// Test helper: an HTTP handler that hangs until cancelled (simulates a slow/hung system). /// diff --git a/tests/ScadaLink.ExternalSystemGateway.Tests/ServiceWiringTests.cs b/tests/ScadaLink.ExternalSystemGateway.Tests/ServiceWiringTests.cs new file mode 100644 index 0000000..44ab1b8 --- /dev/null +++ b/tests/ScadaLink.ExternalSystemGateway.Tests/ServiceWiringTests.cs @@ -0,0 +1,52 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Http; +using NSubstitute; +using ScadaLink.Commons.Interfaces.Repositories; + +namespace ScadaLink.ExternalSystemGateway.Tests; + +/// +/// ExternalSystemGateway-013: configuration options must actually influence the +/// registered HTTP client — an operator setting them must not be silently ignored. +/// +public class ServiceWiringTests +{ + [Fact] + public void MaxConcurrentConnectionsPerSystem_IsAppliedToTheNamedHttpClientPrimaryHandler() + { + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["ScadaLink:ExternalSystemGateway:MaxConcurrentConnectionsPerSystem"] = "4", + }) + .Build(); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSingleton(config); + services.AddSingleton(Substitute.For()); + services.AddExternalSystemGateway(); + + using var provider = services.BuildServiceProvider(); + + // Resolve the per-system named client's message-handler chain and walk to the + // primary handler — the option must be reflected in MaxConnectionsPerServer. + var handlerFactory = provider.GetRequiredService(); + var handler = handlerFactory.CreateHandler("ExternalSystem_AnySystem"); + + var primary = FindPrimaryHandler(handler); + var sockets = Assert.IsType(primary); + Assert.Equal(4, sockets.MaxConnectionsPerServer); + } + + private static HttpMessageHandler FindPrimaryHandler(HttpMessageHandler handler) + { + var current = handler; + while (current is DelegatingHandler delegating && delegating.InnerHandler != null) + { + current = delegating.InnerHandler; + } + return current; + } +}