From e588443ced6e6fc3233b28a35278e6b2981260c7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 22:53:04 -0400 Subject: [PATCH 1/3] fix(abcip,adminui): ParseEnum device wording + libplctag probe assertion (#574); AB forms preserve unknown keys + MaxConcurrentReads (#576); probe off for new AbCip driver (#577); docs T4:0.DN (#584) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/v2/driver-specs.md | 6 +- docs/v2/test-data-sources.md | 8 +- .../AbCipDriverFactoryExtensions.cs | 5 +- .../Shared/Drivers/DriverConfigJsonOverlay.cs | 65 +++++++++++++++++ .../Drivers/Forms/AbCipDriverForm.razor | 34 ++++++++- .../Drivers/Forms/AbLegacyDriverForm.razor | 18 ++++- .../AbCipTransportSwitchTests.cs | 10 +++ .../AbCipDriverPageFormSerializationTests.cs | 73 +++++++++++++++++++ ...bLegacyDriverPageFormSerializationTests.cs | 47 ++++++++++++ 9 files changed, 251 insertions(+), 15 deletions(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs diff --git a/docs/v2/driver-specs.md b/docs/v2/driver-specs.md index 4365114e..d7d679a1 100644 --- a/docs/v2/driver-specs.md +++ b/docs/v2/driver-specs.md @@ -303,9 +303,9 @@ Same library as AB CIP — libplctag handles both protocols via the `plc` attrib | I (Input) | 1 | 2 bytes | INT16/bit | `I:0`, `I:0/3` | | S (Status) | 2 | 2 bytes | INT16/bit | `S:0`, `S:1/0` | | B (Binary) | 3 | 2 bytes | INT16/bit | `B3:0`, `B3:0/5` | -| T (Timer) | 4 | 6 bytes | Struct | `T4:0.ACC`, `T4:0.PRE`, `T4:0/DN` | -| C (Counter) | 5 | 6 bytes | Struct | `C5:0.ACC`, `C5:0/DN`, `C5:0/CU` | -| R (Control) | 6 | 6 bytes | Struct | `R6:0.LEN`, `R6:0.POS`, `R6:0/DN` | +| T (Timer) | 4 | 6 bytes | Struct | `T4:0.ACC`, `T4:0.PRE`, `T4:0.DN` | +| C (Counter) | 5 | 6 bytes | Struct | `C5:0.ACC`, `C5:0.DN`, `C5:0.CU` | +| R (Control) | 6 | 6 bytes | Struct | `R6:0.LEN`, `R6:0.POS`, `R6:0.DN` | | N (Integer) | 7 | 2 bytes | INT16 | `N7:0`, `N7:0/5`, `N10:50` | | F (Float) | 8 | 4 bytes | FLOAT32 | `F8:0`, `F20:10` | | ST (String) | 9 | 84 bytes | STRING | `ST9:0` (82 char max) | diff --git a/docs/v2/test-data-sources.md b/docs/v2/test-data-sources.md index c8d6a18a..3acd4fb3 100644 --- a/docs/v2/test-data-sources.md +++ b/docs/v2/test-data-sources.md @@ -245,8 +245,8 @@ PCCC types are file-based. Int32/Float64/DateTime are not native to SLC/MicroLog | Int32 | — (decompose in driver from two N words) | partial | | Float32 | `F8:n` | ✔ | | String | `ST9:n` | ✔ | -| Timer struct | `T4:n.ACC` / `.PRE` / `/DN` | ✔ | -| Counter struct | `C5:n.ACC` / `.PRE` / `/DN` | ✔ | +| Timer struct | `T4:n.ACC` / `.PRE` / `.DN` | ✔ | +| Counter struct | `C5:n.ACC` / `.PRE` / `.DN` | ✔ | ### Standard Scenario Mapping @@ -260,8 +260,8 @@ PCCC types are file-based. Int32/Float64/DateTime are not native to SLC/MicroLog | Ramp (Float32) | `F8:1` sine wave | | Write-read-back (Bool / Int16 / Float32 / String) | `B3:1/0` / `N7:100` / `F8:100` / `ST9:100` | | Array element write (Int16[10]) | `N7:200..209` | -| Timer fidelity | `T4:0.ACC`, `T4:0.PRE`, `T4:0/DN` | -| Counter fidelity | `C5:0.ACC`, `C5:0.PRE`, `C5:0/DN` | +| Timer fidelity | `T4:0.ACC`, `T4:0.PRE`, `T4:0.DN` | +| Counter fidelity | `C5:0.ACC`, `C5:0.PRE`, `C5:0.DN` | | Connection-limit refusal | Driver harness toggle to simulate 4-conn limit | | Bad on demand | Connection-refused toggle | diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs index 66236c62..a5612b5f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs @@ -84,7 +84,7 @@ public static class AbCipDriverFactoryExtensions ? [.. dto.Devices.Select(d => new AbCipDeviceOptions( HostAddress: d.HostAddress ?? throw new InvalidOperationException( $"AB CIP config for '{driverInstanceId}' has a device missing HostAddress"), - PlcFamily: ParseEnum(d.PlcFamily, "device", driverInstanceId, "PlcFamily", + PlcFamily: ParseEnum(d.PlcFamily, tagName: null, driverInstanceId, "PlcFamily", fallback: AbCipPlcFamily.ControlLogix), DeviceName: d.DeviceName, AllowPacking: d.AllowPacking, @@ -130,7 +130,8 @@ public static class AbCipDriverFactoryExtensions private static TimeSpan PositiveTimeoutOrDefault(int? ms, int defaultMs) => TimeSpan.FromMilliseconds(ms is int v and > 0 ? v : defaultMs); - // tagName null = a driver-level field: the error text names the driver config, not a tag. + // tagName null = a driver-level (or device-level) config field: the error text names the driver config, + // not a tag. Never pass a placeholder like "device" as tagName — it renders as "tag 'device'" (#574). private static T ParseEnum(string? raw, string? tagName, string driverInstanceId, string field, T? fallback = null) where T : struct, Enum { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs new file mode 100644 index 00000000..09901ccd --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs @@ -0,0 +1,65 @@ +using System.Text.Json; +using System.Text.Json.Nodes; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Components.Shared.Drivers; + +/// +/// Preserve-unknown save for the typed driver forms (#576). A form deserialises the stored DriverConfig +/// into a typed options object and re-serialises it on save, which erases every key the type does not +/// model (a newer option, a hand-edited key, rawTags, a device's extra keys…). This overlays only +/// the paths the form OWNS onto the ORIGINAL JSON object: an owned path is replaced by the form's value, +/// or removed when the form emits nothing/null ("Default"); every other key in the original survives. +/// +internal static class DriverConfigJsonOverlay +{ + /// Overlays the of onto . + /// The stored config as loaded; blank or non-object input yields unchanged. + /// The form's own serialised options. + /// Dot-separated, case-insensitive property paths the form edits (e.g. transport, probe.enabled). + public static string Apply(string? originalJson, string typedJson, IEnumerable ownedPaths) + { + if (string.IsNullOrWhiteSpace(originalJson)) return typedJson; + JsonObject? original; + try { original = JsonNode.Parse(originalJson) as JsonObject; } + catch (JsonException) { return typedJson; } + if (original is null || JsonNode.Parse(typedJson) is not JsonObject typed) return typedJson; + + foreach (var path in ownedPaths) + { + var segments = path.Split('.'); + JsonObject? from = typed; + JsonObject? to = original; + for (var i = 0; i < segments.Length - 1 && to is not null; i++) + { + from = from is null ? null : Find(from, segments[i]) as JsonObject; + var next = Find(to, segments[i]) as JsonObject; + if (next is null && from is not null) + { + next = []; + to[segments[i]] = next; + } + to = next; + } + if (to is null) continue; + + var leaf = segments[^1]; + var value = from is null ? null : Find(from, leaf); + Remove(to, leaf); + if (value is not null) to[leaf] = value.DeepClone(); + } + return original.ToJsonString(); + } + + private static JsonNode? Find(JsonObject o, string key) + { + foreach (var kv in o) + if (string.Equals(kv.Key, key, StringComparison.OrdinalIgnoreCase)) return kv.Value; + return null; + } + + private static void Remove(JsonObject o, string key) + { + var hit = o.Select(kv => kv.Key).FirstOrDefault(k => string.Equals(k, key, StringComparison.OrdinalIgnoreCase)); + if (hit is not null) o.Remove(hit); + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor index 86bb6c7b..e3e765c7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor @@ -18,6 +18,11 @@
Default libplctag call timeout. Default 2 s.
+
+ + +
Read work items in flight at once. Default 8; 1 = sequential.
+
@@ -124,6 +129,20 @@ private FormModel _form = new(); private string? _lastParsed; + // The config exactly as loaded; the save overlays the form-owned keys onto it so keys this form does not + // model (unknown/newer options, rawTags, per-device extras) survive an unrelated edit (#576). + private string? _originalJson; + + private static bool IsBlankConfig(string? json) + => string.IsNullOrWhiteSpace(json) || json.AsSpan().Trim().SequenceEqual("{}"); + + /// Top-level/nested JSON paths this form edits; everything else in the stored config is preserved. + internal static readonly string[] OwnedPaths = + [ + "timeout", "transport", "enableControllerBrowse", "enableDeclarationOnlyUdtGrouping", + "enableAlarmProjection", "alarmPollInterval", "maxConcurrentReads", "probeTimeoutSeconds", + "probe.enabled", "probe.interval", "probe.timeout", "probe.probeTagPath", + ]; // Preserve the driver's existing Devices across a load→save (the device form authors the real endpoints; // the merge reconstructs the Devices array from the device rows at deploy/probe time). @@ -136,6 +155,11 @@ { var opts = TryDeserialize(DriverConfigJson) ?? new AbCipDriverOptions(); _form = FormModel.FromOptions(opts); + // #577: a brand-new driver (nothing authored yet) must not default the probe ON with no tag path — + // that logs an operator warning per driver and runs no probe. An existing config keeps whatever + // it stores (an absent "probe" key still means "enabled" to the driver, so don't flip it on edit). + if (IsBlankConfig(DriverConfigJson)) _form.ProbeEnabled = false; + _originalJson = DriverConfigJson; _preservedDevices = opts.Devices; _lastParsed = DriverConfigJson; } @@ -145,16 +169,17 @@ /// Devices array forward (the device form authors the real endpoints; the merge reconstructs the array /// from the device rows at deploy/probe time). public string GetConfigJson() - => Serialize(_form.ToOptions(_preservedDevices)); + => Serialize(_form.ToOptions(_preservedDevices), _originalJson); /// Serializes options with the form's JSON settings (camelCase, enums as strings). - internal static string Serialize(AbCipDriverOptions options) - => JsonSerializer.Serialize(options, _jsonOpts); + internal static string Serialize(AbCipDriverOptions options, string? originalJson = null) + => DriverConfigJsonOverlay.Apply(originalJson, JsonSerializer.Serialize(options, _jsonOpts), OwnedPaths); private async Task EmitAsync() { var json = GetConfigJson(); _lastParsed = json; + _originalJson = json; await DriverConfigJsonChanged.InvokeAsync(json); } @@ -180,6 +205,7 @@ // PLC-5 / LogixPccc exclusion still applies). Never re-emit a concrete default here — that would pin // every edited driver to it. public AbCipTransport? Transport { get; set; } + public int MaxConcurrentReads { get; set; } = 8; public bool EnableControllerBrowse { get; set; } = false; public bool EnableDeclarationOnlyUdtGrouping { get; set; } = false; @@ -200,6 +226,7 @@ { TimeoutSeconds = (int)o.Timeout.TotalSeconds, Transport = o.Transport, + MaxConcurrentReads = o.MaxConcurrentReads, EnableControllerBrowse = o.EnableControllerBrowse, EnableDeclarationOnlyUdtGrouping = o.EnableDeclarationOnlyUdtGrouping, EnableAlarmProjection = o.EnableAlarmProjection, @@ -224,6 +251,7 @@ }, Timeout = TimeSpan.FromSeconds(TimeoutSeconds), Transport = Transport, + MaxConcurrentReads = MaxConcurrentReads, EnableControllerBrowse = EnableControllerBrowse, EnableAlarmProjection = EnableAlarmProjection, AlarmPollInterval = TimeSpan.FromSeconds(AlarmPollIntervalSeconds), diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor index 1a91c666..972648de 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor @@ -90,6 +90,16 @@ private FormModel _form = new(); private string? _lastParsed; + // The config exactly as loaded; the save overlays the form-owned keys onto it so keys this form does not + // model (unknown/newer options, rawTags, per-device extras) survive an unrelated edit (#576). + private string? _originalJson; + + /// JSON paths this form edits; everything else in the stored config is preserved. + internal static readonly string[] OwnedPaths = + [ + "timeout", "transport", "probeTimeoutSeconds", + "probe.enabled", "probe.interval", "probe.timeout", "probe.probeAddress", + ]; // Devices are separate v3 entities — this form doesn't edit them, but preserves the driver's existing // Devices array verbatim across a load→save (the merge rebuilds it at deploy/probe time). @@ -102,6 +112,7 @@ { var opts = TryDeserialize(DriverConfigJson) ?? new AbLegacyDriverOptions(); _form = FormModel.FromOptions(opts); + _originalJson = DriverConfigJson; _preservedDevices = opts.Devices; _lastParsed = DriverConfigJson; } @@ -110,16 +121,17 @@ /// Serializes the current channel/protocol config to camelCase JSON, preserving the driver's /// existing Devices array (authored as separate v3 entities). public string GetConfigJson() - => Serialize(_form.ToOptions(_preservedDevices)); + => Serialize(_form.ToOptions(_preservedDevices), _originalJson); /// Serializes options with the form's JSON settings (camelCase, enums as strings). - internal static string Serialize(AbLegacyDriverOptions options) - => JsonSerializer.Serialize(options, _jsonOpts); + internal static string Serialize(AbLegacyDriverOptions options, string? originalJson = null) + => DriverConfigJsonOverlay.Apply(originalJson, JsonSerializer.Serialize(options, _jsonOpts), OwnedPaths); private async Task EmitAsync() { var json = GetConfigJson(); _lastParsed = json; + _originalJson = json; await DriverConfigJsonChanged.InvokeAsync(json); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs index b8bfb43c..a624f19b 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs @@ -64,6 +64,9 @@ public sealed class AbCipTransportSwitchTests "ab1", """{"devices":[{"hostAddress":"ab://10.0.0.1/1,0","plcFamily":"Bogus"}]}""")); existing.GetType().ShouldBe(bad.GetType()); existing.Message.ShouldContain("unknown PlcFamily 'Bogus'"); + // #574: a device-level field must not be reported as a tag named 'device'. + existing.Message.ShouldNotContain("tag '"); + existing.Message.ShouldStartWith("AB CIP config in 'ab1' has unknown PlcFamily 'Bogus'"); } /// @@ -145,6 +148,9 @@ public sealed class AbCipTransportSwitchTests native.Message.ShouldNotBeNull(); native.Message!.ShouldContain("CIP handshake failed: Connection lost"); lib.Message!.ShouldNotContain("Connection lost"); + // Positive (#574): the libplctag path ran and reported its own outcome (the bounded probe timeout), + // not merely "something other than the native text". + lib.Message!.ShouldContain("timed out"); } /// @@ -180,6 +186,9 @@ public sealed class AbCipTransportSwitchTests absent.Ok.ShouldBeFalse(); absent.Message!.ShouldContain("CIP handshake failed: Connection lost"); lib.Message!.ShouldNotContain("Connection lost"); + // Positive (#574): the libplctag path ran and reported its own outcome (the bounded probe timeout), + // not merely "something other than the native text". + lib.Message!.ShouldContain("timed out"); } /// The probe deserialises with its own string-enum converter; a bad transport is a config error. @@ -259,5 +268,6 @@ public sealed class AbCipTransportSwitchTests unpinned.Message!.ShouldContain("CIP handshake failed: Connection lost"); pinned.Ok.ShouldBeFalse(); pinned.Message!.ShouldNotContain("Connection lost"); + pinned.Message!.ShouldContain("timed out"); // positive: libplctag's own outcome (#574) } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs index b52af00f..426d0bb4 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs @@ -172,4 +172,77 @@ public sealed class AbCipDriverPageFormSerializationTests form.Transport = null; // the "Default (...)" dropdown option AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices)).ShouldNotContain("transport", Case.Insensitive); } + + // ---- #576: preserve-unknown + MaxConcurrentReads ---- + + private const string FullConfig = """ + {"transport":"Native","timeout":"00:00:05","maxConcurrentReads":3,"enableControllerBrowse":true, + "enableDeclarationOnlyUdtGrouping":true,"enableAlarmProjection":true,"alarmPollInterval":"00:00:02", + "probeTimeoutSeconds":12, + "probe":{"enabled":true,"interval":"00:00:10","timeout":"00:00:03","probeTagPath":"Program:Main.Hb","futureProbeKnob":7}, + "devices":[{"hostAddress":"ab://10.0.0.1/1,0","plcFamily":"ControlLogix","deviceName":"PLC-1","futureDeviceKey":"keep"}], + "rawTags":[{"rawPath":"a/b"}], + "futureOption":{"nested":[1,2,3]},"anotherUnknown":"x"} + """; + + [Fact] + public void Editing_one_field_preserves_every_other_option_and_unknown_keys() + { + var loaded = AbCipDriverForm.TryDeserialize(FullConfig)!; + var form = AbCipDriverForm.FormModel.FromOptions(loaded); + form.TimeoutSeconds = 9; // the one edit + + var root = System.Text.Json.Nodes.JsonNode.Parse( + AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), FullConfig))!.AsObject(); + + root["timeout"]!.GetValue().ShouldBe("00:00:09"); + root["maxConcurrentReads"]!.GetValue().ShouldBe(3); // was reset to 8 before #576 + root["transport"]!.GetValue().ShouldBe("Native"); + root["enableControllerBrowse"]!.GetValue().ShouldBeTrue(); + root["enableDeclarationOnlyUdtGrouping"]!.GetValue().ShouldBeTrue(); + root["enableAlarmProjection"]!.GetValue().ShouldBeTrue(); + root["alarmPollInterval"]!.GetValue().ShouldBe("00:00:02"); + root["probeTimeoutSeconds"]!.GetValue().ShouldBe(12); + root["probe"]!["enabled"]!.GetValue().ShouldBeTrue(); + root["probe"]!["interval"]!.GetValue().ShouldBe("00:00:10"); + root["probe"]!["timeout"]!.GetValue().ShouldBe("00:00:03"); + root["probe"]!["probeTagPath"]!.GetValue().ShouldBe("Program:Main.Hb"); + // unknown keys at every level survive + root["probe"]!["futureProbeKnob"]!.GetValue().ShouldBe(7); + root["devices"]![0]!["futureDeviceKey"]!.GetValue().ShouldBe("keep"); + root["rawTags"]![0]!["rawPath"]!.GetValue().ShouldBe("a/b"); + root["futureOption"]!["nested"]!.AsArray().Count.ShouldBe(3); + root["anotherUnknown"]!.GetValue().ShouldBe("x"); + } + + [Fact] + public void Choosing_Default_transport_and_clearing_the_probe_path_removes_those_keys_from_the_overlay() + { + var loaded = AbCipDriverForm.TryDeserialize(FullConfig)!; + var form = AbCipDriverForm.FormModel.FromOptions(loaded); + form.Transport = null; + form.ProbeTagPath = ""; + + var root = System.Text.Json.Nodes.JsonNode.Parse( + AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), FullConfig))!.AsObject(); + + root.ContainsKey("transport").ShouldBeFalse(); + root["probe"]!.AsObject().ContainsKey("probeTagPath").ShouldBeFalse(); + root["futureOption"].ShouldNotBeNull(); + } + + [Fact] + public void A_stored_PascalCase_key_is_replaced_not_duplicated() + { + var original = """{"Transport":"Native","TimeoutSeconds":1,"futureOption":true}"""; + var loaded = AbCipDriverForm.TryDeserialize(original)!; + var form = AbCipDriverForm.FormModel.FromOptions(loaded); + form.Transport = AbCipTransport.Libplctag; + + var json = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), original); + System.Text.RegularExpressions.Regex.Matches(json, "transport", System.Text.RegularExpressions.RegexOptions.IgnoreCase) + .Count.ShouldBe(1); + json.ShouldContain("\"transport\":\"Libplctag\""); + json.ShouldContain("futureOption"); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs index ec8b74d3..2df3c36b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs @@ -187,4 +187,51 @@ public sealed class AbLegacyDriverPageFormSerializationTests razor.ShouldContain(""); razor.ShouldNotContain("Default (Native"); } + + // ---- #576: preserve-unknown ---- + + private const string FullConfig = """ + {"transport":"Native","timeout":"00:00:05","probeTimeoutSeconds":12, + "probe":{"enabled":true,"interval":"00:00:10","timeout":"00:00:03","probeAddress":"S:1","futureProbeKnob":7}, + "devices":[{"hostAddress":"ab://10.0.0.1/1,0","plcFamily":"Slc500","futureDeviceKey":"keep"}], + "rawTags":[{"rawPath":"a/b"}], + "futureOption":{"nested":[1,2,3]}} + """; + + [Fact] + public void Editing_one_field_preserves_every_other_option_and_unknown_keys() + { + var loaded = AbLegacyDriverForm.TryDeserialize(FullConfig)!; + var form = AbLegacyDriverForm.FormModel.FromOptions(loaded); + form.TimeoutSeconds = 9; + + var root = System.Text.Json.Nodes.JsonNode.Parse( + AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices), FullConfig))!.AsObject(); + + root["timeout"]!.GetValue().ShouldBe("00:00:09"); + root["transport"]!.GetValue().ShouldBe("Native"); + root["probeTimeoutSeconds"]!.GetValue().ShouldBe(12); + root["probe"]!["enabled"]!.GetValue().ShouldBeTrue(); + root["probe"]!["interval"]!.GetValue().ShouldBe("00:00:10"); + root["probe"]!["timeout"]!.GetValue().ShouldBe("00:00:03"); + root["probe"]!["probeAddress"]!.GetValue().ShouldBe("S:1"); + root["probe"]!["futureProbeKnob"]!.GetValue().ShouldBe(7); + root["devices"]![0]!["futureDeviceKey"]!.GetValue().ShouldBe("keep"); + root["rawTags"]![0]!["rawPath"]!.GetValue().ShouldBe("a/b"); + root["futureOption"]!["nested"]!.AsArray().Count.ShouldBe(3); + } + + [Fact] + public void Choosing_Default_transport_removes_the_key_but_keeps_unknown_ones() + { + var loaded = AbLegacyDriverForm.TryDeserialize(FullConfig)!; + var form = AbLegacyDriverForm.FormModel.FromOptions(loaded); + form.Transport = null; + + var root = System.Text.Json.Nodes.JsonNode.Parse( + AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices), FullConfig))!.AsObject(); + + root.ContainsKey("transport").ShouldBeFalse(); + root["futureOption"].ShouldNotBeNull(); + } } From 3d081ad29c8c17d8ee6ab9fb1d4abf5ee792afc3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 22:56:47 -0400 Subject: [PATCH 2/3] fix(adminui,cli): /raw device-name placeholder + no-prerender tree, CLI read prints Bad status (#577) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/Client.CLI.md | 2 + docs/drivers/AbCip.md | 3 +- .../Commands/ReadCommand.cs | 18 +++++++- .../Components/Pages/Raw/GlobalRaw.razor | 6 ++- .../Shared/Drivers/DeviceModal.razor | 4 +- .../ReadCommandTests.cs | 43 ++++++++++++++++++- 6 files changed, 70 insertions(+), 6 deletions(-) diff --git a/docs/Client.CLI.md b/docs/Client.CLI.md index 1761c6b0..870f55ac 100644 --- a/docs/Client.CLI.md +++ b/docs/Client.CLI.md @@ -116,6 +116,8 @@ Source Time: 2026-03-30T19:58:38.0961252Z Server Time: 2026-03-30T19:58:38.0971257Z ``` +A Bad node status (unknown node, access denied, ...) is reported, not thrown: the command prints `Node:`, an empty `Value:` and `Status: 0x80340000 (BadNodeIdUnknown)`. Session-level failures (closed connection / channel) still fail the command. + ### write Writes a value to a node. The shared service reads the current value first to determine the target data type, then converts the supplied string value using `ValueConverter.ConvertValue()`. diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 6d7b7193..ff97cbb4 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -75,7 +75,8 @@ the driver's `DriverConfig` JSON. Key fields: - **`Devices`** — one `AbCipDeviceOptions` per PLC (`HostAddress`, `PlcFamily`, optional `DeviceName`, per-device `AllowPacking` / `ConnectionSize` overrides). - **`Tags`** — pre-declared `AbCipTagDefinition` list; `Members` for UDT fan-out, `SafetyTag` for GuardLogix safety-partition tags. -- **`Probe`** — connectivity-probe `Enabled` / `Interval` / `Timeout` / `ProbeTagPath`. +- **`Probe`** — connectivity-probe `Enabled` / `Interval` / `Timeout` / `ProbeTagPath`. A driver created from the AdminUI starts with the probe **off** (an enabled probe with no tag path only logs a warning); an absent `probe` key in a stored config still means enabled to the driver. +- AdminUI driver forms (AbCip/AbLegacy) overlay only the keys they edit onto the stored JSON, so unmodelled keys survive a save (#576). - **`Transport`** — absent (default ⇒ `Native`), `Native`, or `Libplctag` (fallback); see [Transport](#transport-transport-option). - **Discovery** — `EnableControllerBrowse` (`@tags` walk) and `EnableDeclarationOnlyUdtGrouping` (whole-UDT read fast path). - **Alarms** — `EnableAlarmProjection` + `AlarmPollInterval` for the ALMD projection. diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs index a8a0ff93..4f42413b 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs @@ -45,7 +45,23 @@ public class ReadCommand : CommandBase var ct = console.RegisterCancellationHandler(); (service, _) = await CreateServiceAndConnectAsync(ct); - var value = await service.ReadValueAsync(nodeId, ct); + DataValue value; + try + { + value = await service.ReadValueAsync(nodeId, ct); + } + catch (ServiceResultException ex) when (ex.StatusCode != StatusCodes.BadSessionIdInvalid + && ex.StatusCode != StatusCodes.BadConnectionClosed + && ex.StatusCode != StatusCodes.BadSecureChannelClosed) + { + // The SDK's Session.ReadValue surfaces a Bad node status (BadNodeIdUnknown, + // BadUserAccessDenied, ...) as an exception. A read command's job is to REPORT that status, + // not crash with a stack trace (#577) — session-level failures still propagate. + await console.Output.WriteLineAsync($"Node: {NodeId}"); + await console.Output.WriteLineAsync($"Value: "); + await console.Output.WriteLineAsync($"Status: {(StatusCode)ex.StatusCode} ({StatusCodes.GetBrowseName(ex.StatusCode)})"); + return; + } await console.Output.WriteLineAsync($"Node: {NodeId}"); await console.Output.WriteLineAsync($"Value: {value.Value}"); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor index 845b9e06..4398507e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor @@ -1,6 +1,10 @@ @page "/raw" @attribute [Authorize(Policy = AdminUiPolicies.ConfigEditor)] -@rendermode RenderMode.InteractiveServer +@* prerender: false (#577 "first tree click ignored"): with prerender on, the server-rendered tree is shown + before the circuit attaches, so a click on its (handler-less) twists is silently dropped, and the + component then re-runs OnInitializedAsync and swaps in a second, freshly loaded node set. The tree + is the only content here and needs the circuit anyway, so render it once, interactively. *@ +@rendermode @(new InteractiveServerRenderMode(prerender: false)) @using ZB.MOM.WW.OtOpcUa.AdminUI.Uns @using ZB.MOM.WW.OtOpcUa.AdminUI.Components.Shared.Raw @inject IRawTreeService Svc diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor index 5363f91e..9ca4b51e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DeviceModal.razor @@ -28,7 +28,7 @@
- +
A RawPath segment — letters, digits, dash, underscore.
@@ -145,7 +145,7 @@ { _resolvedDriverInstanceId = DriverInstanceId; _driverType = DriverType ?? ""; - _name = "Device1"; + _name = ""; // placeholder only: a pre-filled value made typing append to it (#577); the name is validated on save _enabled = true; _deviceConfigJson = "{}"; _rowVersion = []; diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/ReadCommandTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/ReadCommandTests.cs index 1b894c0a..1dddb1bc 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/ReadCommandTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/ReadCommandTests.cs @@ -99,4 +99,45 @@ public class ReadCommandTests fakeService.DisconnectCalled.ShouldBeTrue(); fakeService.DisposeCalled.ShouldBeTrue(); } -} \ No newline at end of file + + /// A Bad node status is printed, not thrown (#577). + [Fact] + public async Task Execute_PrintsStatusCodeWhenTheReadFailsWithABadStatus() + { + var fakeService = new FakeOpcUaClientService + { + ReadException = new ServiceResultException(StatusCodes.BadNodeIdUnknown) + }; + var command = new ReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=Missing" + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); // must not throw + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("Node: ns=2;s=Missing"); + output.ShouldContain("BadNodeIdUnknown"); + fakeService.DisconnectCalled.ShouldBeTrue(); // positive control: the finally still ran + } + + /// A session-level failure is still an error, not a printed status. + [Fact] + public async Task Execute_StillThrowsOnASessionLevelFailure() + { + var fakeService = new FakeOpcUaClientService + { + ReadException = new ServiceResultException(StatusCodes.BadConnectionClosed) + }; + var command = new ReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=Any" + }; + + using var console = TestConsoleHelper.CreateConsole(); + await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + } +} From bafbcbee7e27313561ebb485db40abb6532e64ba Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 23:03:38 -0400 Subject: [PATCH 3/3] fix(adminui): AB forms read/write the factory-bound *Ms duration keys, migrate legacy TimeSpan strings (#576) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/drivers/AbCip.md | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.AbCip.csproj | 1 + .../ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.csproj | 1 + .../Shared/Drivers/DriverConfigJsonOverlay.cs | 65 +++++++++++++++-- .../Shared/Drivers/DriverConfigMs.cs | 39 ++++++++++ .../Drivers/Forms/AbCipDriverForm.razor | 48 +++++++++--- .../Drivers/Forms/AbLegacyDriverForm.razor | 39 ++++++++-- .../AbCipDriverPageFormSerializationTests.cs | 73 +++++++++++++++++-- ...bLegacyDriverPageFormSerializationTests.cs | 52 +++++++++++-- .../ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj | 3 + 10 files changed, 288 insertions(+), 35 deletions(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigMs.cs diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index ff97cbb4..d3c6e69b 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -76,7 +76,7 @@ the driver's `DriverConfig` JSON. Key fields: - **`Devices`** — one `AbCipDeviceOptions` per PLC (`HostAddress`, `PlcFamily`, optional `DeviceName`, per-device `AllowPacking` / `ConnectionSize` overrides). - **`Tags`** — pre-declared `AbCipTagDefinition` list; `Members` for UDT fan-out, `SafetyTag` for GuardLogix safety-partition tags. - **`Probe`** — connectivity-probe `Enabled` / `Interval` / `Timeout` / `ProbeTagPath`. A driver created from the AdminUI starts with the probe **off** (an enabled probe with no tag path only logs a warning); an absent `probe` key in a stored config still means enabled to the driver. -- AdminUI driver forms (AbCip/AbLegacy) overlay only the keys they edit onto the stored JSON, so unmodelled keys survive a save (#576). +- AdminUI driver forms (AbCip/AbLegacy) overlay only the keys they edit onto the stored JSON, so unmodelled keys survive a save (#576). They also write durations as the integer-millisecond keys the factory binds (`timeoutMs`, `alarmPollIntervalMs`, `probe.intervalMs`, `probe.timeoutMs`); a legacy TimeSpan-string key (`timeout`: "00:00:05") is loaded and migrated on save. - **`Transport`** — absent (default ⇒ `Native`), `Native`, or `Libplctag` (fallback); see [Transport](#transport-transport-option). - **Discovery** — `EnableControllerBrowse` (`@tags` walk) and `EnableDeclarationOnlyUdtGrouping` (whole-UDT read fast path). - **Alarms** — `EnableAlarmProjection` + `AlarmPollInterval` for the ALMD projection. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/ZB.MOM.WW.OtOpcUa.Driver.AbCip.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/ZB.MOM.WW.OtOpcUa.Driver.AbCip.csproj index 59436b5e..8a42ca70 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/ZB.MOM.WW.OtOpcUa.Driver.AbCip.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/ZB.MOM.WW.OtOpcUa.Driver.AbCip.csproj @@ -28,6 +28,7 @@ + diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.csproj index b4fe82f3..fa98205f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.csproj @@ -28,6 +28,7 @@ + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs index 09901ccd..ddc10468 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigJsonOverlay.cs @@ -16,15 +16,37 @@ internal static class DriverConfigJsonOverlay /// The stored config as loaded; blank or non-object input yields unchanged. /// The form's own serialised options. /// Dot-separated, case-insensitive property paths the form edits (e.g. transport, probe.enabled). - public static string Apply(string? originalJson, string typedJson, IEnumerable ownedPaths) + /// + /// (TimeSpan-string path, integer-millisecond path) pairs. The typed options serialise durations as + /// "00:00:05" but the driver factories bind *Ms integers (#576); each pair is rewritten + /// in the typed output (and the legacy TimeSpan path becomes owned, so a stale one is removed). + /// + public static string Apply(string? originalJson, string typedJson, IEnumerable ownedPaths, + IReadOnlyList<(string TimeSpanPath, string MsPath)>? timeSpanToMs = null) { - if (string.IsNullOrWhiteSpace(originalJson)) return typedJson; - JsonObject? original; - try { original = JsonNode.Parse(originalJson) as JsonObject; } - catch (JsonException) { return typedJson; } - if (original is null || JsonNode.Parse(typedJson) is not JsonObject typed) return typedJson; + if (JsonNode.Parse(typedJson) is not JsonObject typed) return typedJson; + var owned = ownedPaths.ToList(); + foreach (var (tsPath, msPath) in timeSpanToMs ?? []) + { + owned.Add(tsPath); + owned.Add(msPath); + if (Get(typed, tsPath) is JsonValue v && v.TryGetValue(out var text) + && TimeSpan.TryParse(text, System.Globalization.CultureInfo.InvariantCulture, out var ts)) + { + RemovePath(typed, tsPath); + Set(typed, msPath, (int)ts.TotalMilliseconds); + } + } - foreach (var path in ownedPaths) + JsonObject? original = null; + if (!string.IsNullOrWhiteSpace(originalJson)) + { + try { original = JsonNode.Parse(originalJson) as JsonObject; } + catch (JsonException) { /* unusable original: fall through to the typed output */ } + } + if (original is null) return typed.ToJsonString(); + + foreach (var path in owned) { var segments = path.Split('.'); JsonObject? from = typed; @@ -50,6 +72,35 @@ internal static class DriverConfigJsonOverlay return original.ToJsonString(); } + private static JsonNode? Get(JsonObject root, string path) + { + JsonNode? cur = root; + foreach (var seg in path.Split('.')) + cur = cur is JsonObject o ? Find(o, seg) : null; + return cur; + } + + private static void RemovePath(JsonObject root, string path) + { + var segs = path.Split('.'); + JsonObject? cur = root; + for (var i = 0; i < segs.Length - 1 && cur is not null; i++) + cur = Find(cur, segs[i]) as JsonObject; + if (cur is not null) Remove(cur, segs[^1]); + } + + private static void Set(JsonObject root, string path, JsonNode value) + { + var segs = path.Split('.'); + var cur = root; + for (var i = 0; i < segs.Length - 1; i++) + { + if (Find(cur, segs[i]) is not JsonObject next) { next = []; cur[segs[i]] = next; } + cur = next; + } + cur[segs[^1]] = value; + } + private static JsonNode? Find(JsonObject o, string key) { foreach (var kv in o) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigMs.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigMs.cs new file mode 100644 index 00000000..34250323 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverConfigMs.cs @@ -0,0 +1,39 @@ +using System.Text.Json; +using System.Text.Json.Nodes; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Components.Shared.Drivers; + +/// +/// Shared millisecond helpers for the AB forms (#576): the driver factories bind *Ms integers +/// while the forms edit whole seconds, so a sub-second stored value must survive an edit of an +/// unrelated field. +/// +internal static class DriverConfigMs +{ + /// Reads an integer-millisecond key at from , or null. + public static int? Read(string? json, string path) + { + if (string.IsNullOrWhiteSpace(json)) return null; + try + { + JsonNode? cur = JsonNode.Parse(json); + foreach (var seg in path.Split('.')) + { + if (cur is not JsonObject o) return null; + cur = o.FirstOrDefault(kv => string.Equals(kv.Key, seg, StringComparison.OrdinalIgnoreCase)).Value; + } + return cur is JsonValue v && v.TryGetValue(out var ms) ? ms : null; + } + catch (JsonException) { return null; } + } + + /// Whole seconds for display (truncating, like the pre-#576 forms). + public static int ToSeconds(TimeSpan t) => (int)t.TotalSeconds; + + /// + /// The duration to save: the loaded value when the operator left the seconds field unchanged + /// (keeps 500 ms from collapsing to 0 s), else the edited whole seconds. + /// + public static TimeSpan Save(int seconds, TimeSpan? loaded) + => loaded is { } l && ToSeconds(l) == seconds ? l : TimeSpan.FromSeconds(seconds); +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor index e3e765c7..093822d8 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor @@ -139,9 +139,16 @@ /// Top-level/nested JSON paths this form edits; everything else in the stored config is preserved. internal static readonly string[] OwnedPaths = [ - "timeout", "transport", "enableControllerBrowse", "enableDeclarationOnlyUdtGrouping", - "enableAlarmProjection", "alarmPollInterval", "maxConcurrentReads", "probeTimeoutSeconds", - "probe.enabled", "probe.interval", "probe.timeout", "probe.probeTagPath", + "transport", "enableControllerBrowse", "enableDeclarationOnlyUdtGrouping", + "enableAlarmProjection", "maxConcurrentReads", "probeTimeoutSeconds", + "probe.enabled", "probe.probeTagPath", + ]; + + /// The factory binds integer-millisecond keys, not the TimeSpan strings the typed options serialise (#576). + internal static readonly (string TimeSpanPath, string MsPath)[] TimeSpanToMs = + [ + ("timeout", "timeoutMs"), ("alarmPollInterval", "alarmPollIntervalMs"), + ("probe.interval", "probe.intervalMs"), ("probe.timeout", "probe.timeoutMs"), ]; // Preserve the driver's existing Devices across a load→save (the device form authors the real endpoints; @@ -154,7 +161,7 @@ if (!string.Equals(_lastParsed, DriverConfigJson, StringComparison.Ordinal)) { var opts = TryDeserialize(DriverConfigJson) ?? new AbCipDriverOptions(); - _form = FormModel.FromOptions(opts); + _form = FormModel.FromJson(DriverConfigJson, opts); // #577: a brand-new driver (nothing authored yet) must not default the probe ON with no tag path — // that logs an operator warning per driver and runs no probe. An existing config keeps whatever // it stores (an absent "probe" key still means "enabled" to the driver, so don't flip it on edit). @@ -173,7 +180,7 @@ /// Serializes options with the form's JSON settings (camelCase, enums as strings). internal static string Serialize(AbCipDriverOptions options, string? originalJson = null) - => DriverConfigJsonOverlay.Apply(originalJson, JsonSerializer.Serialize(options, _jsonOpts), OwnedPaths); + => DriverConfigJsonOverlay.Apply(originalJson, JsonSerializer.Serialize(options, _jsonOpts), OwnedPaths, TimeSpanToMs); private async Task EmitAsync() { @@ -222,8 +229,31 @@ // Admin UI probe timeout public int AdminProbeTimeoutSeconds { get; set; } = 5; + // The loaded durations, so an unchanged seconds field saves the exact stored value (sub-second safe). + private TimeSpan? _loadedTimeout, _loadedAlarmPoll, _loadedProbeInterval, _loadedProbeTimeout; + + /// Loads from the stored JSON: the factory's *Ms keys win; a legacy TimeSpan-string key is the fallback. + public static FormModel FromJson(string? json, AbCipDriverOptions o) + { + static TimeSpan? Ms(string? j, string path) => DriverConfigMs.Read(j, path) is int ms ? TimeSpan.FromMilliseconds(ms) : null; + var f = FromOptions(o); + f._loadedTimeout = Ms(json, "timeoutMs") ?? f._loadedTimeout; + f._loadedAlarmPoll = Ms(json, "alarmPollIntervalMs") ?? f._loadedAlarmPoll; + f._loadedProbeInterval = Ms(json, "probe.intervalMs") ?? f._loadedProbeInterval; + f._loadedProbeTimeout = Ms(json, "probe.timeoutMs") ?? f._loadedProbeTimeout; + f.TimeoutSeconds = DriverConfigMs.ToSeconds(f._loadedTimeout ?? o.Timeout); + f.AlarmPollIntervalSeconds = DriverConfigMs.ToSeconds(f._loadedAlarmPoll ?? o.AlarmPollInterval); + f.ProbeIntervalSeconds = DriverConfigMs.ToSeconds(f._loadedProbeInterval ?? o.Probe.Interval); + f.ProbeTimeoutSeconds = DriverConfigMs.ToSeconds(f._loadedProbeTimeout ?? o.Probe.Timeout); + return f; + } + public static FormModel FromOptions(AbCipDriverOptions o) => new() { + _loadedTimeout = o.Timeout, + _loadedAlarmPoll = o.AlarmPollInterval, + _loadedProbeInterval = o.Probe.Interval, + _loadedProbeTimeout = o.Probe.Timeout, TimeoutSeconds = (int)o.Timeout.TotalSeconds, Transport = o.Transport, MaxConcurrentReads = o.MaxConcurrentReads, @@ -245,16 +275,16 @@ Probe = new AbCipProbeOptions { Enabled = ProbeEnabled, - Interval = TimeSpan.FromSeconds(ProbeIntervalSeconds), - Timeout = TimeSpan.FromSeconds(ProbeTimeoutSeconds), + Interval = DriverConfigMs.Save(ProbeIntervalSeconds, _loadedProbeInterval), + Timeout = DriverConfigMs.Save(ProbeTimeoutSeconds, _loadedProbeTimeout), ProbeTagPath = string.IsNullOrWhiteSpace(ProbeTagPath) ? null : ProbeTagPath, }, - Timeout = TimeSpan.FromSeconds(TimeoutSeconds), + Timeout = DriverConfigMs.Save(TimeoutSeconds, _loadedTimeout), Transport = Transport, MaxConcurrentReads = MaxConcurrentReads, EnableControllerBrowse = EnableControllerBrowse, EnableAlarmProjection = EnableAlarmProjection, - AlarmPollInterval = TimeSpan.FromSeconds(AlarmPollIntervalSeconds), + AlarmPollInterval = DriverConfigMs.Save(AlarmPollIntervalSeconds, _loadedAlarmPoll), EnableDeclarationOnlyUdtGrouping = EnableDeclarationOnlyUdtGrouping, ProbeTimeoutSeconds = AdminProbeTimeoutSeconds, }; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor index 972648de..9db2085a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor @@ -97,8 +97,14 @@ /// JSON paths this form edits; everything else in the stored config is preserved. internal static readonly string[] OwnedPaths = [ - "timeout", "transport", "probeTimeoutSeconds", - "probe.enabled", "probe.interval", "probe.timeout", "probe.probeAddress", + "transport", "probeTimeoutSeconds", + "probe.enabled", "probe.probeAddress", + ]; + + /// The factory binds integer-millisecond keys, not the TimeSpan strings the typed options serialise (#576). + internal static readonly (string TimeSpanPath, string MsPath)[] TimeSpanToMs = + [ + ("timeout", "timeoutMs"), ("probe.interval", "probe.intervalMs"), ("probe.timeout", "probe.timeoutMs"), ]; // Devices are separate v3 entities — this form doesn't edit them, but preserves the driver's existing @@ -111,7 +117,7 @@ if (!string.Equals(_lastParsed, DriverConfigJson, StringComparison.Ordinal)) { var opts = TryDeserialize(DriverConfigJson) ?? new AbLegacyDriverOptions(); - _form = FormModel.FromOptions(opts); + _form = FormModel.FromJson(DriverConfigJson, opts); _originalJson = DriverConfigJson; _preservedDevices = opts.Devices; _lastParsed = DriverConfigJson; @@ -125,7 +131,7 @@ /// Serializes options with the form's JSON settings (camelCase, enums as strings). internal static string Serialize(AbLegacyDriverOptions options, string? originalJson = null) - => DriverConfigJsonOverlay.Apply(originalJson, JsonSerializer.Serialize(options, _jsonOpts), OwnedPaths); + => DriverConfigJsonOverlay.Apply(originalJson, JsonSerializer.Serialize(options, _jsonOpts), OwnedPaths, TimeSpanToMs); private async Task EmitAsync() { @@ -171,8 +177,27 @@ public string? ResilienceConfig { get; set; } public byte[] RowVersion { get; set; } = []; + private TimeSpan? _loadedTimeout, _loadedProbeInterval, _loadedProbeTimeout; + + /// Loads from the stored JSON: the factory's *Ms keys win; a legacy TimeSpan-string key is the fallback. + public static FormModel FromJson(string? json, AbLegacyDriverOptions o) + { + static TimeSpan? Ms(string? j, string path) => DriverConfigMs.Read(j, path) is int ms ? TimeSpan.FromMilliseconds(ms) : null; + var f = FromOptions(o); + f._loadedTimeout = Ms(json, "timeoutMs") ?? f._loadedTimeout; + f._loadedProbeInterval = Ms(json, "probe.intervalMs") ?? f._loadedProbeInterval; + f._loadedProbeTimeout = Ms(json, "probe.timeoutMs") ?? f._loadedProbeTimeout; + f.TimeoutSeconds = DriverConfigMs.ToSeconds(f._loadedTimeout ?? o.Timeout); + f.ProbeIntervalSeconds = DriverConfigMs.ToSeconds(f._loadedProbeInterval ?? o.Probe.Interval); + f.ProbeTimeoutSeconds = DriverConfigMs.ToSeconds(f._loadedProbeTimeout ?? o.Probe.Timeout); + return f; + } + public static FormModel FromOptions(AbLegacyDriverOptions o) => new() { + _loadedTimeout = o.Timeout, + _loadedProbeInterval = o.Probe.Interval, + _loadedProbeTimeout = o.Probe.Timeout, TimeoutSeconds = (int)o.Timeout.TotalSeconds, Transport = o.Transport, ProbeEnabled = o.Probe.Enabled, @@ -189,11 +214,11 @@ Probe = new AbLegacyProbeOptions { Enabled = ProbeEnabled, - Interval = TimeSpan.FromSeconds(ProbeIntervalSeconds), - Timeout = TimeSpan.FromSeconds(ProbeTimeoutSeconds), + Interval = DriverConfigMs.Save(ProbeIntervalSeconds, _loadedProbeInterval), + Timeout = DriverConfigMs.Save(ProbeTimeoutSeconds, _loadedProbeTimeout), ProbeAddress = string.IsNullOrWhiteSpace(ProbeAddress) ? null : ProbeAddress, }, - Timeout = TimeSpan.FromSeconds(TimeoutSeconds), + Timeout = DriverConfigMs.Save(TimeoutSeconds, _loadedTimeout), Transport = Transport, ProbeTimeoutSeconds = AdminProbeTimeoutSeconds, }; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs index 426d0bb4..9ac7ee98 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbCipDriverPageFormSerializationTests.cs @@ -141,7 +141,7 @@ public sealed class AbCipDriverPageFormSerializationTests var json = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices)); json.ShouldContain($"\"transport\":\"{savedValue}\""); - json.ShouldContain("\"timeout\":\"00:00:07\""); + json.ShouldContain("\"timeoutMs\":7000"); } [Fact] @@ -158,7 +158,7 @@ public sealed class AbCipDriverPageFormSerializationTests form.TimeoutSeconds = 7; var json = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices)); json.ShouldNotContain("transport", Case.Insensitive); - json.ShouldContain("\"timeout\":\"00:00:07\""); + json.ShouldContain("\"timeoutMs\":7000"); } [Fact] @@ -195,17 +195,18 @@ public sealed class AbCipDriverPageFormSerializationTests var root = System.Text.Json.Nodes.JsonNode.Parse( AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), FullConfig))!.AsObject(); - root["timeout"]!.GetValue().ShouldBe("00:00:09"); + root["timeoutMs"]!.GetValue().ShouldBe(9000); + root.ContainsKey("timeout").ShouldBeFalse(); root["maxConcurrentReads"]!.GetValue().ShouldBe(3); // was reset to 8 before #576 root["transport"]!.GetValue().ShouldBe("Native"); root["enableControllerBrowse"]!.GetValue().ShouldBeTrue(); root["enableDeclarationOnlyUdtGrouping"]!.GetValue().ShouldBeTrue(); root["enableAlarmProjection"]!.GetValue().ShouldBeTrue(); - root["alarmPollInterval"]!.GetValue().ShouldBe("00:00:02"); + root["alarmPollIntervalMs"]!.GetValue().ShouldBe(2000); root["probeTimeoutSeconds"]!.GetValue().ShouldBe(12); root["probe"]!["enabled"]!.GetValue().ShouldBeTrue(); - root["probe"]!["interval"]!.GetValue().ShouldBe("00:00:10"); - root["probe"]!["timeout"]!.GetValue().ShouldBe("00:00:03"); + root["probe"]!["intervalMs"]!.GetValue().ShouldBe(10000); + root["probe"]!["timeoutMs"]!.GetValue().ShouldBe(3000); root["probe"]!["probeTagPath"]!.GetValue().ShouldBe("Program:Main.Hb"); // unknown keys at every level survive root["probe"]!["futureProbeKnob"]!.GetValue().ShouldBe(7); @@ -245,4 +246,64 @@ public sealed class AbCipDriverPageFormSerializationTests json.ShouldContain("\"transport\":\"Libplctag\""); json.ShouldContain("futureOption"); } + + // ---- #576: the saved JSON must be what the driver FACTORY binds (integer-ms keys) ---- + + [Fact] + public void An_edited_timeout_reaches_the_factory_bound_options() + { + var original = """{"timeoutMs":5000,"alarmPollIntervalMs":500,"probe":{"enabled":true,"intervalMs":7000,"timeoutMs":1500,"probeTagPath":"Program:Main.Hb"},"devices":[{"hostAddress":"ab://10.0.0.1/1,0"}]}"""; + var loaded = AbCipDriverForm.TryDeserialize(original)!; + var form = AbCipDriverForm.FormModel.FromJson(original, loaded); + form.TimeoutSeconds.ShouldBe(5); // the form SEES the factory-keyed value + form.TimeoutSeconds = 11; + + var saved = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), original); + var bound = ZB.MOM.WW.OtOpcUa.Driver.AbCip.AbCipDriverFactoryExtensions.ParseOptions("t", saved); + + bound.Timeout.ShouldBe(TimeSpan.FromSeconds(11)); + // untouched durations keep their exact stored value, including sub-second ones + bound.AlarmPollInterval.ShouldBe(TimeSpan.FromMilliseconds(500)); + bound.Probe.Interval.ShouldBe(TimeSpan.FromSeconds(7)); + bound.Probe.Timeout.ShouldBe(TimeSpan.FromMilliseconds(1500)); + bound.Probe.ProbeTagPath.ShouldBe("Program:Main.Hb"); + } + + [Fact] + public void A_legacy_TimeSpan_string_key_is_loaded_then_migrated_to_ms_and_removed() + { + var original = """{"timeout":"00:00:05","alarmPollInterval":"00:00:02","probe":{"interval":"00:00:10","timeout":"00:00:03"}}"""; + var loaded = AbCipDriverForm.TryDeserialize(original)!; + var form = AbCipDriverForm.FormModel.FromJson(original, loaded); + form.TimeoutSeconds.ShouldBe(5); + form.TimeoutSeconds = 6; + + var saved = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), original); + var root = System.Text.Json.Nodes.JsonNode.Parse(saved)!.AsObject(); + root.ContainsKey("timeout").ShouldBeFalse(); + root.ContainsKey("alarmPollInterval").ShouldBeFalse(); + root["probe"]!.AsObject().ContainsKey("interval").ShouldBeFalse(); + root["probe"]!.AsObject().ContainsKey("timeout").ShouldBeFalse(); + + var bound = ZB.MOM.WW.OtOpcUa.Driver.AbCip.AbCipDriverFactoryExtensions.ParseOptions("t", saved); + bound.Timeout.ShouldBe(TimeSpan.FromSeconds(6)); + bound.AlarmPollInterval.ShouldBe(TimeSpan.FromSeconds(2)); + bound.Probe.Interval.ShouldBe(TimeSpan.FromSeconds(10)); + bound.Probe.Timeout.ShouldBe(TimeSpan.FromSeconds(3)); + } + + [Fact] + public void A_form_saved_config_is_still_accepted_by_the_Test_Connect_probe_parse() + { + // The probe deserialises the typed options and skips unmapped keys, so the ms keys must not break it. + var original = """{"timeoutMs":5000,"devices":[{"hostAddress":"ab://10.0.0.1/1,0"}]}"""; + var loaded = AbCipDriverForm.TryDeserialize(original)!; + var form = AbCipDriverForm.FormModel.FromJson(original, loaded); + form.TimeoutSeconds = 8; + var saved = AbCipDriverForm.Serialize(form.ToOptions(loaded.Devices), original); + + var opts = System.Text.Json.JsonSerializer.Deserialize(saved, TestJsonOpts); + opts.ShouldNotBeNull(); + opts.Devices[0].HostAddress.ShouldBe("ab://10.0.0.1/1,0"); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs index 2df3c36b..f4297127 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs @@ -137,7 +137,7 @@ public sealed class AbLegacyDriverPageFormSerializationTests var json = AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices)); json.ShouldContain($"\"transport\":\"{savedValue}\""); - json.ShouldContain("\"timeout\":\"00:00:07\""); + json.ShouldContain("\"timeoutMs\":7000"); } [Fact] @@ -153,7 +153,7 @@ public sealed class AbLegacyDriverPageFormSerializationTests form.TimeoutSeconds = 7; var json = AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices)); json.ShouldNotContain("transport", Case.Insensitive); - json.ShouldContain("\"timeout\":\"00:00:07\""); + json.ShouldContain("\"timeoutMs\":7000"); } [Fact] @@ -208,12 +208,13 @@ public sealed class AbLegacyDriverPageFormSerializationTests var root = System.Text.Json.Nodes.JsonNode.Parse( AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices), FullConfig))!.AsObject(); - root["timeout"]!.GetValue().ShouldBe("00:00:09"); + root["timeoutMs"]!.GetValue().ShouldBe(9000); + root.ContainsKey("timeout").ShouldBeFalse(); root["transport"]!.GetValue().ShouldBe("Native"); root["probeTimeoutSeconds"]!.GetValue().ShouldBe(12); root["probe"]!["enabled"]!.GetValue().ShouldBeTrue(); - root["probe"]!["interval"]!.GetValue().ShouldBe("00:00:10"); - root["probe"]!["timeout"]!.GetValue().ShouldBe("00:00:03"); + root["probe"]!["intervalMs"]!.GetValue().ShouldBe(10000); + root["probe"]!["timeoutMs"]!.GetValue().ShouldBe(3000); root["probe"]!["probeAddress"]!.GetValue().ShouldBe("S:1"); root["probe"]!["futureProbeKnob"]!.GetValue().ShouldBe(7); root["devices"]![0]!["futureDeviceKey"]!.GetValue().ShouldBe("keep"); @@ -234,4 +235,45 @@ public sealed class AbLegacyDriverPageFormSerializationTests root.ContainsKey("transport").ShouldBeFalse(); root["futureOption"].ShouldNotBeNull(); } + + // ---- #576: the saved JSON must be what the driver FACTORY binds (integer-ms keys) ---- + + [Fact] + public void An_edited_timeout_reaches_the_factory_bound_options() + { + var original = """{"timeoutMs":5000,"probe":{"enabled":true,"intervalMs":7000,"timeoutMs":1500,"probeAddress":"S:1"},"devices":[{"hostAddress":"ab://10.0.0.1/1,0","plcFamily":"Slc500"}]}"""; + var loaded = AbLegacyDriverForm.TryDeserialize(original)!; + var form = AbLegacyDriverForm.FormModel.FromJson(original, loaded); + form.TimeoutSeconds.ShouldBe(5); + form.TimeoutSeconds = 11; + + var saved = AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices), original); + var bound = ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.AbLegacyDriverFactoryExtensions.ParseOptions("t", saved); + + bound.Timeout.ShouldBe(TimeSpan.FromSeconds(11)); + bound.Probe.Interval.ShouldBe(TimeSpan.FromSeconds(7)); + bound.Probe.Timeout.ShouldBe(TimeSpan.FromMilliseconds(1500)); + bound.Probe.ProbeAddress.ShouldBe("S:1"); + } + + [Fact] + public void A_legacy_TimeSpan_string_key_is_loaded_then_migrated_to_ms_and_removed() + { + var original = """{"timeout":"00:00:05","probe":{"interval":"00:00:10","timeout":"00:00:03"}}"""; + var loaded = AbLegacyDriverForm.TryDeserialize(original)!; + var form = AbLegacyDriverForm.FormModel.FromJson(original, loaded); + form.TimeoutSeconds.ShouldBe(5); + form.TimeoutSeconds = 6; + + var saved = AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices), original); + var root = System.Text.Json.Nodes.JsonNode.Parse(saved)!.AsObject(); + root.ContainsKey("timeout").ShouldBeFalse(); + root["probe"]!.AsObject().ContainsKey("interval").ShouldBeFalse(); + root["probe"]!.AsObject().ContainsKey("timeout").ShouldBeFalse(); + + var bound = ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.AbLegacyDriverFactoryExtensions.ParseOptions("t", saved); + bound.Timeout.ShouldBe(TimeSpan.FromSeconds(6)); + bound.Probe.Interval.ShouldBe(TimeSpan.FromSeconds(10)); + bound.Probe.Timeout.ShouldBe(TimeSpan.FromSeconds(3)); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj index ba5aba32..742ab993 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj @@ -37,6 +37,9 @@ project referencing a runtime driver is fine — tests are not in the AdminUI's deploy graph, which is the whole thing #526 was about. --> + + +