diff --git a/code-reviews/Client.Shared/findings.md b/code-reviews/Client.Shared/findings.md index 7444304..447e917 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 | 9 | +| Open findings | 5 | ## Checklist coverage @@ -33,13 +33,13 @@ | Severity | Medium | | Category | Correctness & logic bugs | | Location | `OpcUaClientService.cs:552` | -| Status | Open | +| Status | Resolved | **Description:** `OnAlarmEventNotification` returns early when `eventFields.EventFields` has fewer than 6 entries. The event filter built by `CreateAlarmEventFilter` always registers 13 select clauses, so a conforming server returns 13 fields. The `< 6` threshold is arbitrary and inconsistent: SourceName is index 2 and Severity index 5, but ConditionName (6), Retain (7), Acked/Active (8/9) and ConditionNodeId (12) are all needed for a usable alarm and are each guarded individually with `fields.Count > N`. A non-conforming server that returns a truncated list (or fewer fields than requested) makes the `< 6` early return silently drop the entire notification, including the EventId/SourceName/Severity that are present. **Recommendation:** Drop the `< 6` early return (or lower it to `< 1`) and rely on the existing per-index `fields.Count > N` guards, which already default missing fields safely. If a hard floor is wanted, document why 6 and not 13. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — lowered the early-return threshold to `< 1` (null or empty guard only); per-index `fields.Count > N` guards already default missing fields safely for all higher indices. ### Client.Shared-002 @@ -48,13 +48,13 @@ | Severity | Medium | | Category | Correctness & logic bugs | | Location | `OpcUaClientService.cs:351-355`, `OpcUaClientService.cs:373` | -| Status | Open | +| Status | Resolved | **Description:** `GetRedundancyInfoAsync` performs unguarded unboxing casts on values read from the server: `(int)redundancySupportValue.Value` and `(byte)serviceLevelValue.Value`. Unlike the `ServerUriArray`/`ServerArray` reads below them, the `RedundancySupport` and `ServiceLevel` reads are not wrapped in try/catch. If the server returns the value boxed as a different numeric type than expected (e.g. `ServiceLevel` boxed as `int` instead of `byte`), or returns a null `Value` on a `Bad` DataValue, the cast throws `InvalidCastException`/`NullReferenceException` and the whole call fails instead of returning a sensible default. **Recommendation:** Wrap the `RedundancySupport` and `ServiceLevel` reads in the same defensive pattern used for the array reads, using `Convert.ToInt32`/`Convert.ToByte` on the boxed value and falling back to `None`/`0` when the read status is bad or the value is null. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — replaced direct casts with `StatusCode.IsGood` guard + `Convert.ToInt32`/`Convert.ToByte` coercion; falls back to `None`/`0` when status is bad or value is null. ### Client.Shared-003 @@ -123,13 +123,13 @@ | Severity | Medium | | Category | Concurrency & thread safety | | Location | `OpcUaClientService.cs:581-622` | -| Status | Open | +| Status | Resolved | **Description:** In the alarm fallback path, the `Task.Run` closure mutates the captured locals `activeState`, `ackedState`, `time`, and `capturedMessage`, then reads them when invoking `AlarmEvent`. Because the captured `_session` reference can be replaced by a concurrent failover (see Client.Shared-006), the supplemental `ReadValueAsync` calls may run against a session being disposed, throwing `ObjectDisposedException` — caught by the bare `catch`, after which the alarm is delivered with default (false/MinValue) states, silently misreporting it as inactive/unacknowledged. The notification callback also has no back-pressure: a burst of alarm events spawns an unbounded number of `Task.Run` continuations each doing 3-4 server round-trips. **Recommendation:** Capture the session under the same lock proposed in Client.Shared-005 and skip the supplemental read if the session has changed or is disposed. Consider batching the four sequential `ReadValueAsync` calls into one `Read` request. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added a `ReferenceEquals(session, _session)` guard at the top of the `Task.Run` body to skip reads if the session was replaced by failover; separated `ObjectDisposedException` from the general catch to drop rather than deliver the stale alarm. ### Client.Shared-008 @@ -138,13 +138,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `OpcUaClientService.cs:170-180`, `Helpers/ValueConverter.cs:15-31` | -| Status | Open | +| Status | Resolved | **Description:** `WriteValueAsync` coerces a string input to the target type by reading the node's current value and inferring the type from `currentDataValue.Value`. When the node has never been written, or the read returns a `Bad` status with a null `Value`, `ValueConverter.ConvertValue` falls through to the `_ => rawValue` default and writes a raw `string` into, for example, an `Int32` node — the server then rejects it with `BadTypeMismatch`, surfacing as a confusing failure unrelated to the operator's input. Separately, `ConvertValue` uses `bool.Parse`, which accepts only `true`/`false` — operator input of `1`/`0` throws `FormatException` that propagates raw to the caller. The read-before-write also doubles the round-trip cost of every string write. **Recommendation:** Inspect `currentDataValue.StatusCode` before trusting `Value`; when the type cannot be inferred, surface a clear error rather than writing a mistyped value. Make boolean parsing accept `1`/`0`/`yes`/`no`, and wrap parse failures in a descriptive exception naming the node and target type. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `WriteValueAsync` now checks `StatusCode.IsGood` and non-null `Value` before calling `ConvertValue`, throwing a descriptive `InvalidOperationException` on bad reads; `ValueConverter` now uses a `ParseBool` helper accepting `true/false/1/0/yes/no` (case-insensitive) and wraps all parse/overflow failures in a `FormatException` with the target type and input value in the message. ### Client.Shared-009 diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/ValueConverter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/ValueConverter.cs index 58c54ea..559188d 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/ValueConverter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/ValueConverter.cs @@ -10,23 +10,53 @@ public static class ValueConverter /// Converts a raw string value into the runtime type expected by the target node. /// /// The raw string supplied by the user. - /// The current node value used to infer the target type. May be null. + /// + /// The current node value used to infer the target type. When null the raw string + /// is returned unchanged; callers should validate this case before writing. + /// /// A typed value suitable for an OPC UA write request. + /// + /// Thrown with a descriptive message when cannot be parsed + /// into the type inferred from . + /// public static object ConvertValue(string rawValue, object? currentValue) { - return currentValue switch + try { - bool => bool.Parse(rawValue), - byte => byte.Parse(rawValue), - short => short.Parse(rawValue), - ushort => ushort.Parse(rawValue), - int => int.Parse(rawValue), - uint => uint.Parse(rawValue), - long => long.Parse(rawValue), - ulong => ulong.Parse(rawValue), - float => float.Parse(rawValue), - double => double.Parse(rawValue), - _ => rawValue + return currentValue switch + { + bool => ParseBool(rawValue), + byte => byte.Parse(rawValue), + short => short.Parse(rawValue), + ushort => ushort.Parse(rawValue), + int => int.Parse(rawValue), + uint => uint.Parse(rawValue), + long => long.Parse(rawValue), + ulong => ulong.Parse(rawValue), + float => float.Parse(rawValue), + double => double.Parse(rawValue), + _ => rawValue + }; + } + catch (Exception ex) when (ex is FormatException or OverflowException) + { + var targetType = currentValue?.GetType().Name ?? "unknown"; + throw new FormatException( + $"Cannot convert value \"{rawValue}\" to target type {targetType}: {ex.Message}", ex); + } + } + + /// + /// Parses a boolean from common string representations including numeric and word forms. + /// Accepts: true/false, 1/0, yes/no (case-insensitive). + /// + private static bool ParseBool(string rawValue) + { + return rawValue.Trim().ToLowerInvariant() switch + { + "true" or "1" or "yes" => true, + "false" or "0" or "no" => false, + _ => throw new FormatException($"String '{rawValue}' was not recognized as a valid Boolean.") }; } } \ No newline at end of file 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 ee019ad..3869d2e 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -187,6 +187,9 @@ public sealed class OpcUaClientService : IOpcUaClientService if (value is string rawString) { var currentDataValue = await _session!.ReadValueAsync(nodeId, ct); + if (!StatusCode.IsGood(currentDataValue.StatusCode) || currentDataValue.Value == null) + throw new InvalidOperationException( + $"Cannot infer target type for node {nodeId}: read returned status {currentDataValue.StatusCode} with no value. Provide a typed value instead of a string."); typedValue = ValueConverter.ConvertValue(rawString, currentDataValue.Value); } @@ -388,10 +391,17 @@ public sealed class OpcUaClientService : IOpcUaClientService var redundancySupportValue = await _session!.ReadValueAsync(VariableIds.Server_ServerRedundancy_RedundancySupport, ct); - var redundancyMode = ((RedundancySupport)(int)redundancySupportValue.Value).ToString(); + RedundancySupport redundancySupport; + if (StatusCode.IsGood(redundancySupportValue.StatusCode) && redundancySupportValue.Value != null) + redundancySupport = (RedundancySupport)Convert.ToInt32(redundancySupportValue.Value); + else + redundancySupport = RedundancySupport.None; + var redundancyMode = redundancySupport.ToString(); var serviceLevelValue = await _session.ReadValueAsync(VariableIds.Server_ServiceLevel, ct); - var serviceLevel = (byte)serviceLevelValue.Value; + var serviceLevel = StatusCode.IsGood(serviceLevelValue.StatusCode) && serviceLevelValue.Value != null + ? Convert.ToByte(serviceLevelValue.Value) + : (byte)0; string[] serverUris = []; try @@ -627,7 +637,7 @@ public sealed class OpcUaClientService : IOpcUaClientService private void OnAlarmEventNotification(EventFieldList eventFields) { var fields = eventFields.EventFields; - if (fields == null || fields.Count < 6) + if (fields == null || fields.Count < 1) return; var eventId = fields.Count > 0 ? fields[0].Value as byte[] : null; @@ -656,6 +666,8 @@ public sealed class OpcUaClientService : IOpcUaClientService // Fallback: read InAlarm/Acked from condition node Galaxy attributes // when the server doesn't populate standard event fields. // Must run on a background thread to avoid deadlocking the notification thread. + // Capture the session reference now; skip supplemental reads if the session has + // been replaced by a concurrent failover before the Task.Run body executes. if (ackedField == null && activeField == null && conditionNodeId != null && _session != null) { var session = _session; @@ -663,6 +675,11 @@ public sealed class OpcUaClientService : IOpcUaClientService var capturedMessage = message; _ = Task.Run(async () => { + // If the session was replaced by a failover before we started reading, + // skip the supplemental reads to avoid hitting a disposed session. + if (!ReferenceEquals(session, _session)) + return; + try { var inAlarmValue = await session.ReadValueAsync(NodeId.Parse($"{capturedConditionNodeId}.InAlarm")); @@ -687,9 +704,16 @@ public sealed class OpcUaClientService : IOpcUaClientService } catch { /* DescAttrName may not exist */ } } + catch (ObjectDisposedException) + { + // Session was disposed during supplemental reads (concurrent failover); + // skip the event rather than delivering stale/default states. + Logger.Debug("Supplemental alarm read skipped — session disposed during failover for {ConditionNodeId}", capturedConditionNodeId); + return; + } catch { - // Supplemental read failed; use defaults + // Other supplemental read failure; deliver event with defaults } AlarmEvent?.Invoke(this, new AlarmEventArgs( diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/ValueConverterTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/ValueConverterTests.cs index 1865e2b..462ad98 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/ValueConverterTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/ValueConverterTests.cs @@ -97,14 +97,43 @@ public class ValueConverterTests } [Fact] - public void ConvertValue_InvalidInt_Throws() + public void ConvertValue_InvalidInt_ThrowsWithDescription() { - Should.Throw(() => ValueConverter.ConvertValue("notanint", 0)); + var ex = Should.Throw(() => ValueConverter.ConvertValue("notanint", 0)); + ex.Message.ShouldContain("Int32"); + ex.Message.ShouldContain("notanint"); } [Fact] - public void ConvertValue_Overflow_Throws() + public void ConvertValue_Overflow_ThrowsFormatException() { - Should.Throw(() => ValueConverter.ConvertValue("256", (byte)0)); + // OverflowException is now wrapped in a descriptive FormatException + var ex = Should.Throw(() => ValueConverter.ConvertValue("256", (byte)0)); + ex.InnerException.ShouldBeOfType(); + } + + // --- Client.Shared-008: Boolean aliases --- + + [Theory] + [InlineData("1", true)] + [InlineData("0", false)] + [InlineData("yes", true)] + [InlineData("no", false)] + [InlineData("YES", true)] + [InlineData("NO", false)] + [InlineData("true", true)] + [InlineData("false", false)] + [InlineData("True", true)] + [InlineData("False", false)] + public void ConvertValue_Bool_AcceptsNumericAndWordAliases(string input, bool expected) + { + ValueConverter.ConvertValue(input, true).ShouldBe(expected); + } + + [Fact] + public void ConvertValue_InvalidBool_ThrowsDescriptiveFormatException() + { + var ex = Should.Throw(() => ValueConverter.ConvertValue("maybe", false)); + ex.Message.ShouldContain("Boolean"); } } \ No newline at end of file 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 9d2cf55..fcc6c39 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 @@ -293,6 +293,46 @@ public class OpcUaClientServiceTests : IDisposable _service.WriteValueAsync(new NodeId("ns=2;s=MyNode"), 42)); } + /// + /// Verifies that writing a string to a node whose current read returns a bad status + /// surfaces a clear error instead of writing a mistyped string value (Client.Shared-008). + /// + [Fact] + public async Task WriteValueAsync_StringValueWithBadReadStatus_ThrowsInvalidOperationException() + { + var session = new FakeSessionAdapter + { + ReadResponse = new DataValue(StatusCodes.BadNodeIdUnknown) // Bad status, null Value + }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + var ex = await Should.ThrowAsync(() => + _service.WriteValueAsync(new NodeId("ns=2;s=MyNode"), "42")); + + ex.Message.ShouldContain("Cannot infer target type"); + } + + /// + /// Verifies that writing a string to a node whose read returns bad status and null Value + /// surfaces a clear error for both the bad-status case (Client.Shared-008). + /// + [Fact] + public async Task WriteValueAsync_StringValueWithBadStatus_MessageMentionsNode() + { + var session = new FakeSessionAdapter + { + ReadResponse = new DataValue(StatusCodes.BadNotReadable) + }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + var ex = await Should.ThrowAsync(() => + _service.WriteValueAsync(new NodeId("ns=2;s=MyNode"), "42")); + + ex.Message.ShouldContain("ns=2;s=MyNode"); + } + // --- Browse tests --- /// @@ -842,6 +882,120 @@ public class OpcUaClientServiceTests : IDisposable _service.GetRedundancyInfoAsync()); } + /// + /// Verifies that RedundancySupport boxed as a different numeric type (e.g. short) is handled + /// without InvalidCastException — defensive Convert.ToInt32 coercion (Client.Shared-002). + /// + [Fact] + public async Task GetRedundancyInfoAsync_RedundancySupportBoxedAsShort_DoesNotThrow() + { + var session = new FakeSessionAdapter + { + ReadResponseFunc = nodeId => + { + if (nodeId == VariableIds.Server_ServerRedundancy_RedundancySupport) + // Boxed as short instead of int — simulates a non-conforming server + return new DataValue(new Variant((short)RedundancySupport.Warm), StatusCodes.Good); + if (nodeId == VariableIds.Server_ServiceLevel) + return new DataValue(new Variant((int)200), StatusCodes.Good); // int instead of byte + throw new ServiceResultException(StatusCodes.BadNodeIdUnknown); + } + }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + var info = await _service.GetRedundancyInfoAsync(); + + info.Mode.ShouldBe("Warm"); + info.ServiceLevel.ShouldBe((byte)200); + } + + /// + /// Verifies that a bad-status response for RedundancySupport/ServiceLevel falls back to defaults + /// rather than throwing (Client.Shared-002). + /// + [Fact] + public async Task GetRedundancyInfoAsync_BadStatusOnRequiredReads_ReturnsDefaults() + { + var session = new FakeSessionAdapter + { + ReadResponseFunc = nodeId => + { + if (nodeId == VariableIds.Server_ServerRedundancy_RedundancySupport) + return new DataValue(StatusCodes.BadNotReadable); // bad status, null Value + if (nodeId == VariableIds.Server_ServiceLevel) + return new DataValue(StatusCodes.BadNotReadable); + throw new ServiceResultException(StatusCodes.BadNodeIdUnknown); + } + }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + var info = await _service.GetRedundancyInfoAsync(); + + info.Mode.ShouldBe("None"); + info.ServiceLevel.ShouldBe((byte)0); + } + + // --- Alarm truncated-fields tests (Client.Shared-001) --- + + /// + /// Verifies that an alarm event with fewer than 6 fields (but at least 1) is still raised + /// with available fields — the old hard <6 early return silently dropped it (Client.Shared-001). + /// + [Fact] + public async Task OnAlarmEvent_TruncatedFields_StillRaisesEvent() + { + var fakeSub = new FakeSubscriptionAdapter(); + var session = new FakeSessionAdapter { NextSubscription = fakeSub }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + AlarmEventArgs? received = null; + _service.AlarmEvent += (_, e) => received = e; + + await _service.SubscribeAlarmsAsync(); + + var handle = fakeSub.ActiveHandles.First(); + // Only 3 fields — EventId, EventType, SourceName — fewer than the old < 6 threshold + var fields = new EventFieldList + { + EventFields = + [ + new Variant(new byte[] { 9, 8, 7 }), // 0: EventId + new Variant(ObjectTypeIds.AlarmConditionType), // 1: EventType + new Variant("PartialSource") // 2: SourceName + ] + }; + fakeSub.SimulateEvent(handle, fields); + + received.ShouldNotBeNull(); + received!.SourceName.ShouldBe("PartialSource"); + received.Severity.ShouldBe((ushort)0); // default — field 5 not present + } + + /// + /// Verifies that a null or empty event field list is silently ignored (defensive guard). + /// + [Fact] + public async Task OnAlarmEvent_EmptyFields_DoesNotRaiseEvent() + { + var fakeSub = new FakeSubscriptionAdapter(); + var session = new FakeSessionAdapter { NextSubscription = fakeSub }; + _sessionFactory.EnqueueSession(session); + await _service.ConnectAsync(ValidSettings()); + + var eventCount = 0; + _service.AlarmEvent += (_, _) => eventCount++; + + await _service.SubscribeAlarmsAsync(); + + var handle = fakeSub.ActiveHandles.First(); + fakeSub.SimulateEvent(handle, new EventFieldList { EventFields = [] }); + + eventCount.ShouldBe(0); + } + // --- Failover tests --- ///