fix(client-shared): resolve Low code-review findings (Client.Shared-003,004,009,010,011)

- Client.Shared-003: DefaultSessionAdapter.WriteValueAsync / CallMethodAsync guard against null/empty Results and throw ServiceResultException with the response's ServiceResult code instead of indexing into a missing list. - Client.Shared-004: DefaultSessionAdapter.CloseAsync / HistoryReadRawAsync / HistoryReadAggregateAsync use the Session.*Async overloads and honour the caller's CancellationToken. - Client.Shared-009: AcknowledgeAlarmAsync returns the underlying ServiceResultException.StatusCode on failure instead of always Good; IOpcUaClientService doc updated to describe the new contract. - Client.Shared-010: ConnectionSettings.CertificateStorePath defaults to empty; DefaultApplicationConfigurationFactory resolves the canonical PKI path lazily, so per-failover ConnectionSettings copies don't hit the filesystem. - Client.Shared-011: added the alarm-fallback regression test, extracted EndpointSelector as a pure static, and added EndpointSelectorTests covering security-mode match, Basic256Sha256 preference, fallback, diagnostics, hostname rewrite, and null/empty guards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-23 11:13:21 -04:00
parent 7fe9f16cf8
commit 2a6ac07111
11 changed files with 444 additions and 33 deletions
@@ -14,7 +14,13 @@ internal sealed class DefaultApplicationConfigurationFactory : IApplicationConfi

    public async Task<ApplicationConfiguration> CreateAsync(ConnectionSettings settings, CancellationToken ct)
    {
-        var storePath = settings.CertificateStorePath;
+        // Resolve the canonical PKI path lazily on first use so constructing a
+        // ConnectionSettings instance — including the throwaway copies the client
+        // service builds per failover attempt — does not touch the filesystem.
+        // Callers that supply an explicit path override the default.
+        var storePath = string.IsNullOrWhiteSpace(settings.CertificateStorePath)
+            ? ClientStoragePaths.GetPkiPath()
+            : settings.CertificateStorePath;

        var config = new ApplicationConfiguration
        {
@@ -24,9 +24,47 @@ internal sealed class DefaultEndpointDiscovery : IEndpointDiscovery
        using var client = DiscoveryClient.Create(new Uri(endpointUrl));
        var allEndpoints = client.GetEndpoints(null);

+        return EndpointSelector.SelectBest(allEndpoints, endpointUrl, requestedMode);
+    }
+}
+
+/// <summary>
+///     Pure best-endpoint selection logic, extracted from <see cref="DefaultEndpointDiscovery"/>
+///     so it can be unit tested without standing up a real <see cref="DiscoveryClient"/>.
+/// </summary>
+internal static class EndpointSelector
+{
+    private static readonly ILogger Logger = Log.ForContext(typeof(EndpointSelector));
+
+    /// <summary>
+    ///     Picks the best endpoint from the discovery response that matches the requested
+    ///     security mode, preferring <c>Basic256Sha256</c>, and rewrites the endpoint URL
+    ///     host to match the user-supplied URL when the discovery response advertises a
+    ///     different hostname.
+    /// </summary>
+    /// <param name="allEndpoints">Endpoints returned by the discovery query, in any order.</param>
+    /// <param name="endpointUrl">The endpoint URL the operator supplied; supplies the hostname rewrite target.</param>
+    /// <param name="requestedMode">The requested OPC UA message security mode.</param>
+    /// <exception cref="InvalidOperationException">
+    ///     Thrown when no endpoint matches <paramref name="requestedMode"/>; the message lists the
+    ///     security mode + policy combinations the server returned so operators can diagnose mismatches.
+    /// </exception>
+    public static EndpointDescription SelectBest(
+        IEnumerable<EndpointDescription> allEndpoints,
+        string endpointUrl,
+        MessageSecurityMode requestedMode)
+    {
+        ArgumentNullException.ThrowIfNull(allEndpoints);
+        if (string.IsNullOrWhiteSpace(endpointUrl))
+            throw new ArgumentException("Endpoint URL must not be null or empty.", nameof(endpointUrl));
+
+        // Materialise once so we can both iterate and produce a diagnostic message
+        // without re-running the underlying discovery enumeration.
+        var endpoints = allEndpoints.ToList();
+
        EndpointDescription? best = null;

-        foreach (var ep in allEndpoints)
+        foreach (var ep in endpoints)
        {
            if (ep.SecurityMode != requestedMode)
                continue;
@@ -37,18 +75,21 @@ internal sealed class DefaultEndpointDiscovery : IEndpointDiscovery
                continue;
            }

+            // Prefer Basic256Sha256 when multiple endpoints match the requested mode.
            if (ep.SecurityPolicyUri == SecurityPolicies.Basic256Sha256)
                best = ep;
        }

        if (best == null)
        {
-            var available = string.Join(", ", allEndpoints.Select(e => $"{e.SecurityMode}/{e.SecurityPolicyUri}"));
+            var available = string.Join(", ", endpoints.Select(e => $"{e.SecurityMode}/{e.SecurityPolicyUri}"));
            throw new InvalidOperationException(
                $"No endpoint found with security mode '{requestedMode}'. Available endpoints: {available}");
        }

-        // Rewrite endpoint URL hostname to match user-supplied hostname
+        // Rewrite endpoint URL hostname to match user-supplied hostname. Necessary
+        // when the OPC UA server returns a discovery URL using a different hostname
+        // (e.g. internal DNS name) than the one the operator routed to.
        var serverUri = new Uri(best.EndpointUrl);
        var requestedUri = new Uri(endpointUrl);
        if (serverUri.Host != requestedUri.Host)
@@ -73,6 +73,17 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter

        var writeCollection = new WriteValueCollection { writeValue };
        var response = await _session.WriteAsync(null, writeCollection, ct);
+        // A malformed or service-level-faulted response can come back with an empty
+        // Results collection alongside a service fault. Surface the service result
+        // (or BadUnexpectedError) rather than letting Results[0] throw
+        // IndexOutOfRangeException upstream.
+        if (response.Results == null || response.Results.Count == 0)
+        {
+            var serviceResult = response.ResponseHeader?.ServiceResult.Code ?? StatusCodes.BadUnexpectedError;
+            throw new ServiceResultException(serviceResult,
+                $"Write response contained no results for node {nodeId}.");
+        }
+
        return response.Results[0];
    }

@@ -143,15 +154,18 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
            if (continuationPoint != null)
                nodesToRead[0].ContinuationPoint = continuationPoint;

-            _session.HistoryRead(
+            // Use the async overload so this method is genuinely asynchronous,
+            // honors the cancellation token, and does not block the caller's thread
+            // (which would block the UI dispatcher for client.ui consumers).
+            var response = await _session.HistoryReadAsync(
                null,
                new ExtensionObject(details),
                TimestampsToReturn.Source,
                continuationPoint != null,
                nodesToRead,
-                out var results,
-                out _);
+                ct).ConfigureAwait(false);

+            var results = response.Results;
            if (results == null || results.Count == 0)
                break;

@@ -186,15 +200,17 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
            new HistoryReadValueId { NodeId = nodeId }
        };

-        _session.HistoryRead(
+        // Use the async overload so the method honors the cancellation token and
+        // does not block on a synchronous service round-trip.
+        var response = await _session.HistoryReadAsync(
            null,
            new ExtensionObject(details),
            TimestampsToReturn.Source,
            false,
            nodesToRead,
-            out var results,
-            out _);
+            ct).ConfigureAwait(false);

+        var results = response.Results;
        var allValues = new List<DataValue>();

        if (results != null && results.Count > 0)
@@ -229,7 +245,9 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
    {
        try
        {
-            if (_session.Connected) _session.Close();
+            // Use the async overload so the caller does not block on the close
+            // service round-trip and the cancellation token is honored.
+            if (_session.Connected) await _session.CloseAsync(ct).ConfigureAwait(false);
        }
        catch (Exception ex)
        {
@@ -270,6 +288,15 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
            },
            ct);

+        // An empty Results collection paired with a service fault must surface as
+        // a ServiceResultException, not an IndexOutOfRangeException from Results[0].
+        if (result.Results == null || result.Results.Count == 0)
+        {
+            var serviceResult = result.ResponseHeader?.ServiceResult.Code ?? StatusCodes.BadUnexpectedError;
+            throw new ServiceResultException(serviceResult,
+                $"Call response contained no results for method {methodId} on {objectId}.");
+        }
+
        var callResult = result.Results[0];
        if (StatusCode.IsBad(callResult.StatusCode))
            throw new ServiceResultException(callResult.StatusCode);