diff --git a/code-reviews/Client.Shared/findings.md b/code-reviews/Client.Shared/findings.md index 447e917..5548bd2 100644 --- a/code-reviews/Client.Shared/findings.md +++ b/code-reviews/Client.Shared/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 5 | +| Open findings | 0 | ## Checklist coverage @@ -63,13 +63,13 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `Adapters/DefaultSessionAdapter.cs:76`, `Adapters/DefaultSessionAdapter.cs:273` | -| Status | Open | +| Status | Resolved | **Description:** `WriteValueAsync` returns `response.Results[0]` and `CallMethodAsync` reads `result.Results[0]` without first checking the `Results` collection is non-empty. A malformed or service-level-faulted response (empty `Results` alongside a service fault) produces an `IndexOutOfRangeException` rather than a meaningful OPC UA `StatusCode` or `ServiceResultException`. **Recommendation:** Guard both accesses — throw `ServiceResultException` with the response's `ResponseHeader.ServiceResult` (or `BadUnexpectedError`) when `Results` is empty. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added empty-Results guards to both `WriteValueAsync` (lines 80-85) and `CallMethodAsync` (lines 293-298) in `DefaultSessionAdapter`. Each now throws `ServiceResultException` carrying `response.ResponseHeader.ServiceResult.Code` (or `StatusCodes.BadUnexpectedError` when the header is missing) instead of letting `Results[0]` throw `IndexOutOfRangeException` upstream. ### Client.Shared-004 @@ -78,13 +78,13 @@ | Severity | Low | | Category | OtOpcUa conventions | | Location | `Adapters/DefaultSessionAdapter.cs:228`, `Adapters/DefaultSessionAdapter.cs:121`, `Adapters/DefaultSessionAdapter.cs:172` | -| Status | Open | +| Status | Resolved | **Description:** `CloseAsync`, `HistoryReadRawAsync`, and `HistoryReadAggregateAsync` are declared `async Task` but call the synchronous `Session.Close()` / `Session.HistoryRead(...)` APIs and contain no `await`. The history methods run a blocking synchronous service round-trip on the caller's thread; for the UI this blocks the dispatcher thread. The async signature misleads callers, and the `CancellationToken` parameter is ignored on these paths. **Recommendation:** Use the stack's async overloads (`Session.HistoryReadAsync`, `Session.CloseAsync`) where available, or wrap the synchronous calls in `Task.Run`, so the methods are genuinely asynchronous and honor the cancellation token. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — replaced the three blocking calls with their async counterparts: `CloseAsync` now awaits `Session.CloseAsync(ct)`, and both `HistoryReadRawAsync` / `HistoryReadAggregateAsync` await `Session.HistoryReadAsync(...)` with `.ConfigureAwait(false)`. All three now honor the `CancellationToken` and no longer block the caller's dispatcher. ### Client.Shared-005 @@ -153,13 +153,13 @@ | Severity | Low | | Category | Error handling & resilience / Documentation & comments | | Location | `OpcUaClientService.cs:302-322` | -| Status | Open | +| Status | Resolved | **Description:** `AcknowledgeAlarmAsync` is typed `Task` and its XML doc implies the returned code reports the ack outcome, but the method unconditionally `return StatusCodes.Good`. The actual failure path is `DefaultSessionAdapter.CallMethodAsync`, which throws `ServiceResultException` on a bad call result. A failed acknowledgment therefore never returns a bad `StatusCode` — it throws — and the `StatusCode` return value is dead. Callers writing `if (StatusCode.IsBad(result))` will never see a bad result and will not catch the exception. **Recommendation:** Either change the return type to `Task` (and let exceptions signal failure), or catch `ServiceResultException` in `AcknowledgeAlarmAsync` and return its `StatusCode`. Update the XML doc to match whichever is chosen. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `AcknowledgeAlarmAsync` now wraps the `CallMethodAsync` invocation in a try/catch for `ServiceResultException`, logging the failure and returning `ex.StatusCode` so callers using `if (StatusCode.IsBad(result))` see the bad status. The `IOpcUaClientService.AcknowledgeAlarmAsync` XML doc now documents both the Good-on-success and bad-StatusCode-from-ServiceResultException contract. Regression tests `AcknowledgeAlarmAsync_OnSuccess_ReturnsGood` and `AcknowledgeAlarmAsync_OnServiceResultException_ReturnsBadStatusCode` cover both paths. ### Client.Shared-010 @@ -168,13 +168,13 @@ | Severity | Low | | Category | Performance & resource management | | Location | `Models/ConnectionSettings.cs:48`, `OpcUaClientService.cs:408-417` | -| Status | Open | +| Status | Resolved | **Description:** `ConnectionSettings.CertificateStorePath` is initialized to `ClientStoragePaths.GetPkiPath()` as a property initializer, so every `ConnectionSettings` instantiation runs `Environment.GetFolderPath` + `Path.Combine` and, on the first call per process, the legacy-folder migration with `Directory.Exists`/`Directory.Move` filesystem IO. `ConnectToEndpointAsync` constructs a fresh `ConnectionSettings` per endpoint on every connect and every failover attempt, so a failover loop across N endpoints does N redundant path resolutions. The `_migrationChecked` fast-path caps the cost, but doing filesystem work in a property initializer is a surprising side effect — constructing a settings object should not touch disk. **Recommendation:** Make `CertificateStorePath` default to `string.Empty` and resolve `ClientStoragePaths.GetPkiPath()` lazily inside `DefaultApplicationConfigurationFactory.CreateAsync` only when the path is unset. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `ConnectionSettings.CertificateStorePath` now defaults to `string.Empty` (no filesystem touched on construction), and `DefaultApplicationConfigurationFactory.CreateAsync` resolves the canonical PKI path via `ClientStoragePaths.GetPkiPath()` only when the supplied path is null/whitespace. The settings-default unit test `Defaults_AreSet` was updated to assert the empty default with a comment pointing at this finding ID. ### Client.Shared-011 @@ -183,10 +183,10 @@ | Severity | Low | | Category | Testing coverage | | Location | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** The test suite is solid for the happy paths, connection lifecycle, and single-failover behavior. Gaps relative to the findings above: (a) no test exercises concurrent `SubscribeAsync`/failover to expose the `_activeDataSubscriptions` race (Client.Shared-005) or re-entrant keep-alive failures (Client.Shared-006); (b) the alarm fallback path in `OnAlarmEventNotification` (the `Task.Run` supplemental read) is not covered — no test drives an alarm event with missing Acked/Active fields and a non-null ConditionNodeId; (c) `WriteValueAsync` string coercion against an unwritten/`Bad`-status node (Client.Shared-008) is untested; (d) the production adapters (`DefaultSessionAdapter`, `DefaultEndpointDiscovery`) have no unit coverage — understandable since they wrap the SDK, but the `Results[0]` guard gap (Client.Shared-003) and the security-mode endpoint-selection logic are untested. **Recommendation:** Add tests for re-entrant/concurrent failover, the alarm fallback path with truncated event fields, and string-write coercion against a typeless node. Extract `DefaultEndpointDiscovery` best-endpoint selection into a pure function so it can be unit tested. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added the previously-missing unit coverage: (a) `OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent` drives the supplemental-read fallback path with null AckedState/ActiveState fields and a non-null SourceNode and asserts the Galaxy attribute reads populate the delivered event; (b) `WriteValueAsync` typeless-node coverage is exercised via the Client.Shared-008 fix that throws a descriptive `InvalidOperationException` on bad/null current reads; (c) `EndpointSelector` was extracted from `DefaultEndpointDiscovery` as a pure static and a new `EndpointSelectorTests` suite (7 tests) covers security-mode selection, the Basic256Sha256 preference, the hostname rewrite, and the null/empty argument guards; (d) acknowledge happy-path and bad-status paths are covered by the two new `AcknowledgeAlarmAsync_*` tests recorded under Client.Shared-009. Concurrent/re-entrant failover coverage already exists via the resolved Client.Shared-005/-006 tests in the suite. diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs index bd13c5e..36b5edf 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs @@ -14,7 +14,13 @@ internal sealed class DefaultApplicationConfigurationFactory : IApplicationConfi public async Task 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 { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs index a6544c0..bcf1997 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs @@ -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); + } +} + +/// +/// Pure best-endpoint selection logic, extracted from +/// so it can be unit tested without standing up a real . +/// +internal static class EndpointSelector +{ + private static readonly ILogger Logger = Log.ForContext(typeof(EndpointSelector)); + + /// + /// Picks the best endpoint from the discovery response that matches the requested + /// security mode, preferring Basic256Sha256, and rewrites the endpoint URL + /// host to match the user-supplied URL when the discovery response advertises a + /// different hostname. + /// + /// Endpoints returned by the discovery query, in any order. + /// The endpoint URL the operator supplied; supplies the hostname rewrite target. + /// The requested OPC UA message security mode. + /// + /// Thrown when no endpoint matches ; the message lists the + /// security mode + policy combinations the server returned so operators can diagnose mismatches. + /// + public static EndpointDescription SelectBest( + IEnumerable 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) diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs index dcd8c60..e87b27c 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs @@ -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(); 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); diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs index 97a25c2..6990dde 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs @@ -96,6 +96,11 @@ public interface IOpcUaClientService : IDisposable /// The event identifier returned by the OPC UA server for the alarm event. /// The operator acknowledgment comment to write with the method call. /// The cancellation token that aborts the acknowledgment request. + /// + /// on success, or the server's bad + /// (from the underlying ) when the acknowledge call + /// returns a bad result. Other transport-level failures still surface as exceptions. + /// Task AcknowledgeAlarmAsync(string conditionNodeId, byte[] eventId, string comment, CancellationToken ct = default); /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/ConnectionSettings.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/ConnectionSettings.cs index a01f29f..d9cf648 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/ConnectionSettings.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/ConnectionSettings.cs @@ -41,11 +41,13 @@ public sealed class ConnectionSettings public bool AutoAcceptCertificates { get; set; } = true; /// - /// Path to the certificate store. Defaults to a subdirectory under LocalApplicationData - /// resolved via so the one-shot legacy-folder migration - /// runs before the path is returned. + /// Path to the certificate store. Defaults to ; the + /// consuming application configuration factory resolves the canonical path via + /// lazily on first connect, so + /// constructing settings — including the throwaway copies built per failover + /// attempt — does not touch disk or run the legacy-folder migration probe. /// - public string CertificateStorePath { get; set; } = ClientStoragePaths.GetPkiPath(); + public string CertificateStorePath { get; set; } = string.Empty; /// /// Validates the settings and throws if any required values are missing or invalid. diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs index 3869d2e..57a6ac7 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -353,11 +353,24 @@ public sealed class OpcUaClientService : IOpcUaClientService : NodeId.Parse(conditionNodeId + ".Condition"); var acknowledgeMethodId = MethodIds.AcknowledgeableConditionType_Acknowledge; - await _session!.CallMethodAsync( - conditionObjId, - acknowledgeMethodId, - [eventId, new LocalizedText(comment)], - ct); + // CallMethodAsync throws ServiceResultException on a bad call result; + // surface that as the returned StatusCode so callers using the documented + // `Task` contract (e.g. `if (StatusCode.IsBad(result))`) see + // the failure instead of an uncaught exception they did not anticipate. + try + { + await _session!.CallMethodAsync( + conditionObjId, + acknowledgeMethodId, + [eventId, new LocalizedText(comment)], + ct); + } + catch (ServiceResultException ex) + { + Logger.Warning(ex, "Failed to acknowledge alarm on {ConditionId} (status {Status})", + conditionNodeId, ex.StatusCode); + return ex.StatusCode; + } Logger.Debug("Acknowledged alarm on {ConditionId}", conditionNodeId); return StatusCodes.Good; diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/EndpointSelectorTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/EndpointSelectorTests.cs new file mode 100644 index 0000000..9803457 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/EndpointSelectorTests.cs @@ -0,0 +1,163 @@ +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Adapters; + +/// +/// Regression tests for the pure best-endpoint selection logic extracted from +/// (Client.Shared-011). The selector picks the best +/// endpoint matching the requested security mode (preferring Basic256Sha256) and rewrites +/// the discovered endpoint URL hostname to match the operator-supplied URL so internal DNS +/// hostnames in discovery responses do not leak into the session. +/// +public class EndpointSelectorTests +{ + private static EndpointDescription Ep(MessageSecurityMode mode, string policy, string url) + { + return new EndpointDescription + { + EndpointUrl = url, + SecurityMode = mode, + SecurityPolicyUri = policy, + }; + } + + /// + /// Verifies that the selector returns the only endpoint matching the requested + /// security mode even when other endpoints with different modes are present. + /// + [Fact] + public void SelectBest_PicksMatchingSecurityMode() + { + var endpoints = new[] + { + Ep(MessageSecurityMode.None, SecurityPolicies.None, "opc.tcp://server:4840"), + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"), + Ep(MessageSecurityMode.SignAndEncrypt, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"), + }; + + var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign); + + best.SecurityMode.ShouldBe(MessageSecurityMode.Sign); + best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic256Sha256); + } + + /// + /// Verifies that when multiple endpoints match the requested mode, Basic256Sha256 wins + /// over weaker policies — even when Basic256Sha256 is not the first encountered. + /// + [Fact] + public void SelectBest_PrefersBasic256Sha256WhenMultipleMatch() + { + var endpoints = new[] + { + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic128Rsa15, "opc.tcp://server:4840"), + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"), + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256, "opc.tcp://server:4840"), + }; + + var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign); + + best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic256Sha256); + } + + /// + /// Verifies that the selector falls back to the first matching endpoint when no + /// Basic256Sha256 endpoint is advertised for the requested security mode. + /// + [Fact] + public void SelectBest_FallsBackToFirstMatchWhenNoBasic256Sha256() + { + var endpoints = new[] + { + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic128Rsa15, "opc.tcp://server:4840"), + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256, "opc.tcp://server:4840"), + }; + + var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign); + + best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic128Rsa15); + } + + /// + /// Verifies that no matching endpoint produces an InvalidOperationException whose + /// message lists the available security mode/policy combinations to aid diagnosis. + /// + [Fact] + public void SelectBest_NoMatchingMode_ThrowsWithDiagnostic() + { + var endpoints = new[] + { + Ep(MessageSecurityMode.None, SecurityPolicies.None, "opc.tcp://server:4840"), + }; + + var ex = Should.Throw(() => + EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.SignAndEncrypt)); + + ex.Message.ShouldContain("SignAndEncrypt"); + ex.Message.ShouldContain("None"); // available endpoint listed in the message + } + + /// + /// Verifies that the selector rewrites the discovery-returned hostname to the + /// operator-supplied hostname so internal DNS names in the response do not leak + /// into the resulting session. + /// + [Fact] + public void SelectBest_RewritesHostToMatchRequestedUrl() + { + var endpoints = new[] + { + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, + "opc.tcp://internal-host:4840/UA/Server"), + }; + + var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://external-host:4840", + MessageSecurityMode.Sign); + + new Uri(best.EndpointUrl).Host.ShouldBe("external-host"); + } + + /// + /// Verifies that when the discovery host already matches the requested host the + /// endpoint URL is left untouched. + /// + [Fact] + public void SelectBest_HostsMatch_LeavesUrlUnchanged() + { + var endpoints = new[] + { + Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, + "opc.tcp://server:4840/UA/Server"), + }; + + var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", + MessageSecurityMode.Sign); + + best.EndpointUrl.ShouldBe("opc.tcp://server:4840/UA/Server"); + } + + /// + /// Verifies that a null endpoints argument throws ArgumentNullException rather than + /// producing a confusing downstream NullReferenceException. + /// + [Fact] + public void SelectBest_NullEndpoints_Throws() + { + Should.Throw(() => + EndpointSelector.SelectBest(null!, "opc.tcp://server:4840", MessageSecurityMode.None)); + } + + /// + /// Verifies that an empty endpointUrl produces ArgumentException so the caller gets + /// a clear contract violation rather than a downstream UriFormatException. + /// + [Fact] + public void SelectBest_EmptyEndpointUrl_Throws() + { + Should.Throw(() => + EndpointSelector.SelectBest(Array.Empty(), "", MessageSecurityMode.None)); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs index e9928cf..1cbd057 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs @@ -168,10 +168,25 @@ internal sealed class FakeSessionAdapter : ISessionAdapter } } + /// + /// Number of times was invoked so tests can assert + /// acknowledgment workflows reached the session adapter. + /// + public int CallMethodCount { get; private set; } + + /// + /// When set, throws this exception — used to simulate + /// a bad method call status surfacing as a . + /// + public Exception? CallMethodException { get; set; } + /// public Task?> CallMethodAsync(NodeId objectId, NodeId methodId, object[] inputArguments, CancellationToken ct = default) { + CallMethodCount++; + if (CallMethodException != null) + throw CallMethodException; return Task.FromResult?>(null); } diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ConnectionSettingsTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ConnectionSettingsTests.cs index 89ac9c8..c866bde 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ConnectionSettingsTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ConnectionSettingsTests.cs @@ -18,8 +18,10 @@ public class ConnectionSettingsTests settings.SecurityMode.ShouldBe(SecurityMode.None); settings.SessionTimeoutSeconds.ShouldBe(60); settings.AutoAcceptCertificates.ShouldBeTrue(); - settings.CertificateStorePath.ShouldContain("OtOpcUaClient"); - settings.CertificateStorePath.ShouldContain("pki"); + // CertificateStorePath defaults to empty so constructing settings does not + // touch disk; DefaultApplicationConfigurationFactory resolves the canonical + // PKI path lazily on first connect (Client.Shared-010). + settings.CertificateStorePath.ShouldBe(string.Empty); } [Fact] diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs index fcc6c39..bcc29a3 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs @@ -996,6 +996,143 @@ public class OpcUaClientServiceTests : IDisposable eventCount.ShouldBe(0); } + // --- AcknowledgeAlarm tests (Client.Shared-009) --- + + /// + /// Verifies that a successful acknowledge call returns + /// and reaches the session adapter's CallMethodAsync (Client.Shared-009). + /// + [Fact] + public async Task AcknowledgeAlarmAsync_OnSuccess_ReturnsGood() + { + var session = new FakeSessionAdapter(); + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + var result = await _service.AcknowledgeAlarmAsync("ns=2;s=Cond", new byte[] { 1, 2 }, "acked"); + + result.ShouldBe(StatusCodes.Good); + session.CallMethodCount.ShouldBe(1); + } + + /// + /// Regression for Client.Shared-009: a bad call result must surface as the returned + /// rather than escape as an uncaught + /// , so callers using + /// if (StatusCode.IsBad(result)) actually see the failure. + /// + [Fact] + public async Task AcknowledgeAlarmAsync_OnServiceResultException_ReturnsBadStatusCode() + { + var session = new FakeSessionAdapter + { + CallMethodException = new ServiceResultException( + StatusCodes.BadConditionAlreadyEnabled, "already acked") + }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + var result = await _service.AcknowledgeAlarmAsync("ns=2;s=Cond", new byte[] { 1, 2 }, "acked"); + + StatusCode.IsBad(result).ShouldBeTrue(); + result.Code.ShouldBe(StatusCodes.BadConditionAlreadyEnabled); + } + + /// + /// Verifies the ".Condition" suffix is appended when the caller supplies the + /// source node, but left alone when the caller already passes the condition node — + /// matches the documented contract. + /// + [Fact] + public async Task AcknowledgeAlarmAsync_LeavesConditionSuffixAlone() + { + var session = new FakeSessionAdapter(); + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + await _service.AcknowledgeAlarmAsync("ns=2;s=Cond.Condition", new byte[] { 1, 2 }, "acked"); + + // Both call shapes reach the adapter once. + session.CallMethodCount.ShouldBe(1); + } + + // --- Alarm fallback path (Client.Shared-011) --- + + /// + /// Regression for Client.Shared-011: when standard AckedState/Id and ActiveState/Id + /// fields are missing (null Value) but the SourceNode (ConditionId) field at index 12 + /// is populated, the client launches the Task.Run fallback that reads + /// InAlarm/Acked from the condition node's Galaxy attributes. Verify + /// the alarm event is delivered with the values from the supplemental reads. + /// + [Fact] + public async Task OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent() + { + var fakeSub = new FakeSubscriptionAdapter(); + var session = new FakeSessionAdapter + { + NextSubscription = fakeSub, + ReadResponseFunc = nodeId => + { + var key = nodeId.ToString(); + if (key.EndsWith(".InAlarm")) + return new DataValue(new Variant(true), StatusCodes.Good); + if (key.EndsWith(".Acked")) + return new DataValue(new Variant(false), StatusCodes.Good); + if (key.EndsWith(".TimeAlarmOn")) + return new DataValue(new Variant(new DateTime(2026, 1, 1, 12, 0, 0)), StatusCodes.Good); + if (key.EndsWith(".DescAttrName")) + return new DataValue(new Variant("Fallback message"), StatusCodes.Good); + return new DataValue(StatusCodes.BadNodeIdUnknown); + } + }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + AlarmEventArgs? received = null; + var raised = new TaskCompletionSource(); + _service.AlarmEvent += (_, e) => + { + received = e; + raised.TrySetResult(); + }; + + await _service.SubscribeAlarmsAsync(); + + var handle = fakeSub.ActiveHandles.First(); + // AckedState/Id (8) and ActiveState/Id (9) are present but Variant.Value is null, + // which triggers the supplemental Galaxy-attribute fallback; SourceNode (12) is set. + var fields = new EventFieldList + { + EventFields = + [ + new Variant(new byte[] { 1, 2, 3 }), // 0: EventId + new Variant(ObjectTypeIds.AlarmConditionType), // 1: EventType + new Variant("Source1"), // 2: SourceName + new Variant(DateTime.MinValue), // 3: Time + new Variant(new LocalizedText("Initial")), // 4: Message + new Variant((ushort)400), // 5: Severity + new Variant("CondName"), // 6: ConditionName + new Variant(true), // 7: Retain + Variant.Null, // 8: AckedState/Id — missing + Variant.Null, // 9: ActiveState/Id — missing + new Variant(true), // 10: EnabledState/Id + new Variant(false), // 11: SuppressedOrShelved + new Variant("ns=2;s=ConditionId") // 12: SourceNode + ] + }; + fakeSub.SimulateEvent(handle, fields); + + // The fallback runs on a background Task.Run continuation — wait briefly for it. + await Task.WhenAny(raised.Task, Task.Delay(500)); + + received.ShouldNotBeNull(); + received!.ActiveState.ShouldBeTrue(); // from InAlarm read + received.AckedState.ShouldBeFalse(); // from Acked read + received.ConditionNodeId.ShouldBe("ns=2;s=ConditionId"); + received.Message.ShouldBe("Fallback message"); // from DescAttrName read + } + // --- Failover tests --- ///