Merge pull request 'fix(alarms): populate EventType/SourceNode/SourceName on native + scripted conditions (#473)' (#474) from fix/alarm-condition-source-fields into master

docs/AlarmTracking.md

@@ -57,8 +57,7 @@ Every condition event — native and scripted — carries the mandatory `BaseEve
 fields, assigned at materialize time in `OtOpcUaNodeManager.MaterialiseAlarmCondition`. The SDK
 does **not** synthesize them on this path (`Create` builds the children from the type definition
 but leaves them unset; `ReportEvent` / `InstanceStateSnapshot` copy children verbatim), so they
-are set explicitly. Leaving them unset shipped them as **null** on every event — see issues #473
+are set explicitly. Leaving them unset shipped them as **null** on every event — see issue #473.
-(the `BaseEventType` trio) and #475 (the `ConditionType` classification pair).
 
 | Field | Value | Notes |
 |---|---|---|
@@ -66,20 +65,6 @@ are set explicitly. Leaving them unset shipped them as **null** on every event
 | `SourceNode` | the condition's **own NodeId** — equal to `ConditionId` | The condition **is** the source: an alarm-bearing raw tag materializes only the condition, with no sibling value variable, so there is no other node to point at |
 | `SourceName` | the same identifying id string: **RawPath** (native) / **ScriptedAlarmId** (scripted) | Deliberately the *unique* id, **not** the leaf name |
 | `ConditionName` | the leaf / display name (e.g. `HR200`) | Where the short human-readable name lives |
-| `ConditionClassId` | always **`BaseConditionClassType`** | Part 9's "no condition class modelled" value. Unset shipped `NodeId.Null` (#475) |
-| `ConditionClassName` | always **`"BaseConditionClass"`** | Matches `ConditionClassId`. Unset shipped empty text (#475) |
-
-**Why `BaseConditionClassType` and not `ProcessConditionClassType`.** We hold no per-alarm
-classification at the materialize seam, and `ConditionClassId` is a wire contract clients bucket on.
-`BaseConditionClassType` is the honest, spec-conformant report of *"this server does not model
-condition classes"* — it fixes the real defect (a null that breaks conformant clients) without
-asserting a classification we cannot back. `ProcessConditionClassType` — the SDK sample's pick —
-was rejected deliberately: it would be *actively wrong* for a Galaxy alarm whose upstream category is
-Safety or Diagnostics, trading a detectable null for an undetectable lie. Real per-alarm
-classification is a separate future feature: it needs the driver's alarm category, which today lives
-only on the runtime `AlarmEventArgs` transition, carried to the deploy-time authored composition that
-`MaterialiseAlarmCondition` sees. Until then the `IAlarmSource` doc comment claiming the category
-"maps to `ConditionClassName` downstream" describes an intent, not the implementation.
 
 **Why `SourceName` is the id, not the leaf name.** The leaf is ambiguous across devices (`HR200` on
 two PLCs collides) and is already carried by `ConditionName`, so the leaf-name option would add no
@@ -94,12 +79,10 @@ identifier and is unique, so it is safe to key on *by itself* — but do **not**
 `ConditionName` (`$"{SourceName}.{ConditionName}"`), because `SourceName` already ends in the
 condition's leaf name and the result stutters (`pymodbus/plc/HR200.HR200`).
 
-Wire-level guard: `NativeAlarmEventIdentityFieldDeliveryTests` asserts the three `BaseEventType`
+Wire-level guard: `NativeAlarmEventIdentityFieldDeliveryTests` asserts the three fields arrive
-fields arrive populated on a real subscription using the standard `[EventType, SourceNode,
+populated on a real subscription using the standard `[EventType, SourceNode, SourceName, Time,
-SourceName, Time, Message, Severity]` select clause, and — in a second test with its own clause —
+Message, Severity]` select clause; `NodeManagerAlarmSourceFieldsTests` guards the node itself
-that `ConditionClassId` / `ConditionClassName` do too. The two class fields are declared on
+across both realms.
-`ConditionType`, **not** `BaseEventType`, so a client must select them against that type.
-`NodeManagerAlarmSourceFieldsTests` guards the node itself across both realms.
 
 > **Do not correlate live events to HistoryRead on `SourceName` — the two paths disagree.**
 > The HistoryRead *events* projection (`OtOpcUaNodeManager.ProjectEventField`) returns

src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs

@@ -815,18 +815,6 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
             alarm.SourceNode.Value = alarm.NodeId; // Create() assigned this above; do not rebuild it
             alarm.SourceName.Value = alarmNodeId;
 
-            // #475 — the mandatory ConditionType classification fields, unset by Create() for the same reason as
-            // the fields above (mandatory, no default, nothing downstream synthesises them) ⇒ NodeId.Null + empty
-            // text on the wire, which buckets every alarm as unclassified in a Part 9 HMI.
-            // BaseConditionClassType is Part 9's "no class modelled" value and is the honest report: we hold no
-            // classification at this seam. Deliberately NOT ProcessConditionClassType (the SDK sample's pick) — it
-            // would assert a classification we cannot back, and would be actively wrong for a Galaxy alarm whose
-            // upstream category is Safety/Diagnostics. Real per-alarm classification needs the driver's
-            // AlarmCategory, which today exists only on the runtime AlarmEventArgs transition and not on the
-            // authored composition this deploy-time seam sees — a separate feature, not a default picked here.
-            if (alarm.ConditionClassId is not null) alarm.ConditionClassId.Value = ObjectTypeIds.BaseConditionClassType;
-            if (alarm.ConditionClassName is not null) alarm.ConditionClassName.Value = new LocalizedText("BaseConditionClass");
-
             // Initial state via the SDK setters (T14: basic state only, NO event firing).
             alarm.SetEnableState(SystemContext, true);
             alarm.SetActiveState(SystemContext, false);

tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/NativeAlarmEventIdentityFieldDeliveryTests.cs

@@ -30,7 +30,6 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests;
 public sealed class NativeAlarmEventIdentityFieldDeliveryTests
 {
     private const string ServerUri = "urn:OtOpcUa.AlarmEventIdentityFields";
-    private const string ConditionClassServerUri = "urn:OtOpcUa.AlarmEventConditionClassFields";
 
     private const string RawDeviceFolder = "pymodbus/plc";
     private const string RawAlarmPath = "pymodbus/plc/HR200";
@@ -46,13 +45,6 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests
     private const int MessageIndex = 4;
     private const int SeverityIndex = 5;
 
-    // Field indices in BuildConditionClassEventFilter's clause (#475). A SEPARATE clause on purpose: the
-    // ScadaBridge one above is mirrored field-for-field from the real consumer and its indices are load-bearing,
-    // so appending to it would silently shift them. Message is re-selected here only as the collector's filter key.
-    private const int ConditionClassIdIndex = 0;
-    private const int ConditionClassNameIndex = 1;
-    private const int ConditionClassMessageIndex = 2;
-
     /// <summary>A live native condition event delivers a populated EventType / SourceNode / SourceName to a
     /// Server-object subscriber using the standard BaseEventType select clause.</summary>
     [Fact]
@@ -75,7 +67,7 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests
             var rawNs = (ushort)session.NamespaceUris.GetIndex(V3NodeIds.RawNamespaceUri);
             rawNs.ShouldBeGreaterThan((ushort)0);
 
-            var collector = new EventCollector(MessageIndex);
+            var collector = new EventCollector();
             var subscription = new Subscription(session.DefaultSubscription) { PublishingInterval = 100 };
             subscription.FastEventCallback = collector.OnEvents;
             session.AddSubscription(subscription);
@@ -83,7 +75,7 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests
 
             // The collector filters by our unique Message, so the item's ClientHandle is not needed here
             // (unlike the sibling multi-notifier test, which tallies delivery per monitored item).
-            AddEventItem(subscription, ObjectIds.Server, BuildEventFilter());
+            AddEventItem(subscription, ObjectIds.Server);
             await subscription.ApplyChangesAsync(ct);
 
             sink.WriteAlarmCondition(RawAlarmPath, ActiveSnapshot(), DateTime.UtcNow, AddressSpaceRealm.Raw);
@@ -121,57 +113,6 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests
         }
     }
 
-    /// <summary>Issue #475 — a live condition event delivers a populated ConditionClassId / ConditionClassName.
-    /// Both previously arrived unset (NodeId.Null / empty text) via the same mechanism as the #473 fields, so an
-    /// HMI bucketing alarms by condition class dropped every OtOpcUa alarm into an unclassified bin.</summary>
-    [Fact]
-    public async Task Condition_event_carries_populated_ConditionClass_fields_on_the_wire()
-    {
-        var pkiRoot = Path.Combine(Path.GetTempPath(), $"otopcua-alarm-condclass-{Guid.NewGuid():N}");
-        var port = AllocateFreePort();
-        var ct = TestContext.Current.CancellationToken;
-        try
-        {
-            var (server, host) = await BootServerAsync(port, pkiRoot + "-srv", ConditionClassServerUri, ct);
-            await using var _ = host;
-
-            var sink = new SdkAddressSpaceSink(server.NodeManager!);
-            WireCondition(sink);
-
-            using var session = await OpenSessionAsync($"opc.tcp://127.0.0.1:{port}/OtOpcUa", ct);
-
-            var collector = new EventCollector(ConditionClassMessageIndex);
-            var subscription = new Subscription(session.DefaultSubscription) { PublishingInterval = 100 };
-            subscription.FastEventCallback = collector.OnEvents;
-            session.AddSubscription(subscription);
-            await subscription.CreateAsync(ct);
-
-            AddEventItem(subscription, ObjectIds.Server, BuildConditionClassEventFilter());
-            await subscription.ApplyChangesAsync(ct);
-
-            sink.WriteAlarmCondition(RawAlarmPath, ActiveSnapshot(), DateTime.UtcNow, AddressSpaceRealm.Raw);
-
-            await WaitUntilAsync(() => collector.Fields.Count >= 1, TimeSpan.FromSeconds(5));
-            var fields = collector.Fields.ShouldHaveSingleItem();
-
-            // ConditionClassId — a resolvable class NodeId, NOT NodeId.Null. We model no condition classes, so
-            // Part 9's "no class modelled" value (BaseConditionClassType) is the conformant report.
-            fields[ConditionClassIdIndex].Value.ShouldBe(ObjectTypeIds.BaseConditionClassType,
-                "ConditionClassId must carry a resolvable condition class, not null");
-
-            // ConditionClassName — the matching human-readable name, NOT empty text.
-            var className = fields[ConditionClassNameIndex].Value.ShouldBeOfType<LocalizedText>();
-            className.Text.ShouldBe("BaseConditionClass",
-                "ConditionClassName must name the reported condition class, not be empty");
-
-            await subscription.DeleteAsync(true, ct);
-        }
-        finally
-        {
-            SafeDelete(pkiRoot + "-srv");
-        }
-    }
-
     /// <summary>Materialise the raw device folder + the native condition at its RawPath, plus one referencing
     /// equipment folder wired as a notifier (the production topology).</summary>
     private static void WireCondition(SdkAddressSpaceSink sink)
@@ -186,13 +127,13 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests
         new(Active: true, Acknowledged: false, Confirmed: true, Enabled: true,
             Shelving: AlarmShelvingKind.Unshelved, Severity: 700, Message: AlarmMessage);
 
-    private static MonitoredItem AddEventItem(Subscription subscription, NodeId source, EventFilter filter)
+    private static MonitoredItem AddEventItem(Subscription subscription, NodeId source)
     {
         var item = new MonitoredItem(subscription.DefaultItem)
         {
             StartNodeId = source,
             AttributeId = Attributes.EventNotifier,
-            Filter = filter,
+            Filter = BuildEventFilter(),
             QueueSize = 100,
             SamplingInterval = 0,
         };
@@ -213,23 +154,9 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests
         return filter;
     }
 
-    /// <summary>#475's clause: [ConditionClassId, ConditionClassName, Message]. The two class fields are declared on
-    /// <c>ConditionType</c>, not BaseEventType, so they must be selected against that type or the server returns no
-    /// value for them regardless of the fix. Message rides along solely as the collector's filter key.</summary>
-    private static EventFilter BuildConditionClassEventFilter()
-    {
-        var filter = new EventFilter();
-        filter.AddSelectClause(ObjectTypes.ConditionType, BrowseNames.ConditionClassId);
-        filter.AddSelectClause(ObjectTypes.ConditionType, BrowseNames.ConditionClassName);
-        filter.AddSelectClause(ObjectTypes.BaseEventType, BrowseNames.Message);
-        return filter;
-    }
-
     /// <summary>Captures the delivered event field lists, filtered to our unique alarm Message so unrelated
     /// server events / refresh brackets never count.</summary>
-    /// <param name="messageIndex">Position of the Message field in the select clause this collector is paired
+    private sealed class EventCollector
-    /// with — the two clauses here place it differently, so it cannot be a shared constant.</param>
-    private sealed class EventCollector(int messageIndex)
     {
         private readonly object _gate = new();
         private readonly List<VariantCollection> _fields = new();
@@ -245,8 +172,8 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests
             {
                 foreach (var e in notification.Events)
                 {
-                    if (e.EventFields.Count > messageIndex &&
+                    if (e.EventFields.Count > MessageIndex &&
-                        e.EventFields[messageIndex].Value is LocalizedText lt &&
+                        e.EventFields[MessageIndex].Value is LocalizedText lt &&
                         lt.Text == AlarmMessage)
                     {
                         _fields.Add(e.EventFields);

tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmSourceFieldsTests.cs

@@ -23,12 +23,6 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests;
 ///         scripted), matching <c>ConditionId</c>. The leaf/display name stays on <c>ConditionName</c>, so
 ///         no information is lost by SourceName carrying the unique id rather than the ambiguous leaf.
 ///     </para>
-///     <para>
-///         Issue #475 — the same mechanism leaves the mandatory <c>ConditionType</c> classification fields
-///         <c>ConditionClassId</c> / <c>ConditionClassName</c> unset. The contract locked in here: a server that
-///         does not model condition classes must still report <c>BaseConditionClassType</c> (Part 9's
-///         "no class modelled" value) rather than null. See <c>docs/AlarmTracking.md</c>.
-///     </para>
 /// </summary>
 public sealed class NodeManagerAlarmSourceFieldsTests : IDisposable
 {
@@ -150,52 +144,6 @@ public sealed class NodeManagerAlarmSourceFieldsTests : IDisposable
 
     }
 
-    /// <summary>#475 — a NATIVE condition (Raw realm) carries the mandatory ConditionType classification fields.
-    /// We do not model condition classes, so Part 9's "no class modelled" value — BaseConditionClassType — is the
-    /// conformant answer; the point is that it is a resolvable class NodeId a client can bucket on rather than a
-    /// null that drops the alarm into an unclassified bin.</summary>
-    [Trait("Category", "Unit")]
-    [Fact]
-    public async Task Native_condition_carries_ConditionClassId_and_ConditionClassName()
-    {
-        await using var host = await BootAsync();
-        var nm = host.Nm;
-
-        nm.EnsureFolder(RawDeviceFolder, parentNodeId: null, displayName: "dev1", realm: AddressSpaceRealm.Raw);
-        nm.MaterialiseAlarmCondition(RawAlarmPath, RawDeviceFolder, "HR200", "OffNormalAlarm", 700,
-            realm: AddressSpaceRealm.Raw, isNative: true);
-
-        var condition = nm.TryGetAlarmCondition(RawAlarmPath, AddressSpaceRealm.Raw);
-        condition.ShouldNotBeNull();
-
-        condition.ConditionClassId.ShouldNotBeNull();
-        condition.ConditionClassId.Value.ShouldBe(ObjectTypeIds.BaseConditionClassType);
-
-        condition.ConditionClassName.ShouldNotBeNull();
-        condition.ConditionClassName.Value.ShouldNotBeNull();
-        condition.ConditionClassName.Value.Text.ShouldBe("BaseConditionClass");
-    }
-
-    /// <summary>#475 — a SCRIPTED condition (UNS realm) carries the SAME classification fields: native and scripted
-    /// share the materialise path, so neither may regress independently.</summary>
-    [Trait("Category", "Unit")]
-    [Fact]
-    public async Task Scripted_condition_carries_ConditionClassId_and_ConditionClassName()
-    {
-        await using var host = await BootAsync();
-        var nm = host.Nm;
-
-        nm.EnsureFolder("eq-4", parentNodeId: null, displayName: "Station 4", realm: AddressSpaceRealm.Uns);
-        nm.MaterialiseAlarmCondition("tank-dry", "eq-4", "Tank Dry", "OffNormalAlarm", 700,
-            realm: AddressSpaceRealm.Uns, isNative: false);
-
-        var condition = nm.TryGetAlarmCondition("tank-dry", AddressSpaceRealm.Uns);
-        condition.ShouldNotBeNull();
-
-        condition.ConditionClassId!.Value.ShouldBe(ObjectTypeIds.BaseConditionClassType);
-        condition.ConditionClassName!.Value.Text.ShouldBe("BaseConditionClass");
-    }
-
     /// <summary>A booted server + its node manager, disposed via <c>await using</c> so an assertion failure
     /// cannot leak a live server (and its bound port) into the rest of the test run.</summary>
     private sealed class BootedServer(OpcUaApplicationHost host, OtOpcUaNodeManager nm) : IAsyncDisposable