dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(address-space): announce surgically-added raw nodes; equipment-with-references removal stays surgical (#599)

Browse Source 
- AnnounceAddedNodes raises NodeAdded in the Raw realm for the existing parents of
  added raw containers/tags (ComputeRawAddAnnouncements); Uns side unchanged.
- ApplyPureRemove removes UNS reference variables BEFORE the equipment subtree, so a
  reference the subtree's id-prefix scope reaches no longer ratchets to a full rebuild
  (03/P5). A NotSubsumed filter was not used: in production a reference NodeId is
  Area/Line/Equipment/Name, outside the subtree scope, so filtering would leak it.
  The matching RemoveEquipmentSubtree severing (03/S20) is in the node-manager commit.
- UnsReferenceBackingParityTests: reflection guard discovering the classifier's surgical
  raw-tag whitelist and requiring a same-typed mirror field (03/U11).

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
2026-10-08 07:12:29 -04:00
parent 3baa332d6b
commit 0bd3ec00e4
6 changed files with 414 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs
+63 -1
View File
@@ -1459,6 +1459,62 @@ public sealed class AddressSpaceApplierTests
new[] { "eq-1", V3NodeIds.Uns("eq-1", "Diag"), "line-7" }.OrderBy(x => x));
}
/// <summary>
/// Gitea #599 (archreview 01/U-8 = 03/P6) — surgically-added RAW nodes are announced in the RAW realm.
/// A new tag under an existing device announces that device; a new TagGroup carrying a new tag under a
/// new Device announces only the Device's existing parent — the inner parents are themselves new, and a
/// client re-browsing the outer one discovers the whole new subtree. Before the fix only Uns-realm
/// parents were announced, so these adds were invisible to connected model-aware clients.
/// </summary>
[Fact]
public void AnnounceAddedNodes_announces_added_raw_nodes_in_the_raw_realm()
{
var sink = new RecordingSink();
var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
var plan = EmptyPlan with
{
AddedRawContainers = new[]
{
new RawContainerNode("Plant/Modbus/dev2", "Plant/Modbus", "dev2", RawNodeKind.Device),
new RawContainerNode("Plant/Modbus/dev2/grp", "Plant/Modbus/dev2", "grp", RawNodeKind.TagGroup),
},
AddedRawTags = new[]
{
RawTag("Plant/Modbus/dev2/grp/speed") with { ParentNodeId = "Plant/Modbus/dev2/grp" },
RawTag("Plant/Modbus/dev1/temp"), // parent Plant/Modbus/dev1 already exists
},
// A UNS add in the same plan still announces in the Uns realm.
AddedUnsReferenceVariables = new[] { UnsRef("filling/line-1/station-1/speed", "speed") },
};
applier.AnnounceAddedNodes(plan);
sink.ModelChangeRealmQueue.OrderBy(x => x.NodeId, StringComparer.Ordinal).ToArray().ShouldBe(new[]
{
("Plant/Modbus", AddressSpaceRealm.Raw),
("Plant/Modbus/dev1", AddressSpaceRealm.Raw),
("eq-1", AddressSpaceRealm.Uns),
});
}
/// <summary>A top-level raw container (no parent: it hangs under the shared root, which has no
/// realm-qualified id) announces itself; duplicates collapse; the Uns-side computation is unaffected.</summary>
[Fact]
public void ComputeRawAddAnnouncements_top_level_container_announces_itself_and_dedups()
{
var applier = new AddressSpaceApplier(new RecordingSink(), NullLogger<AddressSpaceApplier>.Instance);
var plan = EmptyPlan with
{
AddedRawContainers = new[] { new RawContainerNode("Plant2", null, "Plant2", RawNodeKind.Folder) },
AddedRawTags = new[] { RawTag("Plant/Modbus/dev1/a"), RawTag("Plant/Modbus/dev1/b") },
};
applier.ComputeRawAddAnnouncements(plan).ShouldBe(new[] { "Plant/Modbus/dev1", "Plant2" });
applier.ComputeAddAnnouncements(plan).ShouldBeEmpty();
}
// ----- Surgical v3 deltas: raw-tag attribute edits (Raw realm) + UNS-reference remove -----
// Task 4 taught AddressSpaceChangeClassifier that a non-alarm RawTagDelta and any UnsReferenceDelta are
// surgical-eligible; these pin the applier actually APPLYING them (before this, such a plan classified
@@ -1963,7 +2019,13 @@ public sealed class AddressSpaceApplierTests
public List<string> ModelChangeCalls => ModelChangeQueue.ToList();
/// <summary>Records a NodeAdded model-change announcement.</summary>
/// <param name="affectedNodeId">The node under which discovered nodes were added.</param>
public void RaiseNodesAddedModelChange(string affectedNodeId, AddressSpaceRealm realm = AddressSpaceRealm.Uns) => ModelChangeQueue.Enqueue(affectedNodeId);
public void RaiseNodesAddedModelChange(string affectedNodeId, AddressSpaceRealm realm = AddressSpaceRealm.Uns)
{
ModelChangeQueue.Enqueue(affectedNodeId);
ModelChangeRealmQueue.Enqueue((affectedNodeId, realm));
}
/// <summary>Gets the NodeAdded announcements WITH the realm each was raised in (Gitea #599).</summary>
public ConcurrentQueue<(string NodeId, AddressSpaceRealm Realm)> ModelChangeRealmQueue { get; } = new();
public void WireAlarmNotifiers(string alarmNodeId, AddressSpaceRealm alarmRealm, IReadOnlyList<string> notifierFolderNodeIds, AddressSpaceRealm notifierFolderRealm) { }
public void AddReference(string sourceNodeId, AddressSpaceRealm sourceRealm, string targetNodeId, AddressSpaceRealm targetRealm, string referenceType = "Organizes") { }
}
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs
+98
View File
@@ -182,6 +182,104 @@ public sealed class NodeManagerSurgicalRemoveTests : IDisposable
await host.DisposeAsync();
}
// ---------- Gitea #599 (archreview 03/P5 + 03/S20): equipment-with-references removal ----------
/// <summary>
/// S20 — the subtree removal severs the inverse half of every cross-tree <c>Organizes</c> edge a removed
/// variable holds. The raw node survives a UNS-scoped removal, so without this it keeps a browsable,
/// unresolvable reference to a node that no longer exists — the hazard <c>RemoveVariableNode</c> was
/// already fixed for (2026-08-15 review I3), left open on the subtree path.
/// </summary>
[Trait("Category", "Unit")]
[Fact]
public async Task RemoveEquipmentSubtree_severs_the_inverse_Organizes_edge_on_the_surviving_raw_node()
{
var (host, server) = await BootAsync();
try
{
var nm = server.NodeManager!;
nm.EnsureVariable("Plant/Modbus/dev1/temp", null, "temp", "Float", writable: false, realm: AddressSpaceRealm.Raw);
nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1", realm: AddressSpaceRealm.Uns);
nm.EnsureVariable("eq-1/Temp", "eq-1", "Temp", "Float", writable: false, realm: AddressSpaceRealm.Uns);
nm.AddReference("eq-1/Temp", AddressSpaceRealm.Uns, "Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw);
var mirror = nm.TryGetVariable("eq-1/Temp", AddressSpaceRealm.Uns)!.NodeId;
InverseOrganizesTo(nm, mirror).ShouldBeTrue(); // precondition: the edge really is there
nm.RemoveEquipmentSubtree("eq-1", AddressSpaceRealm.Uns).ShouldBeTrue();
nm.TryGetVariable("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw).ShouldNotBeNull();
InverseOrganizesTo(nm, mirror).ShouldBeFalse();
}
finally
{
await host.DisposeAsync();
}
}
/// <summary>
/// P5 — removing an equipment that holds UNS references is a scoped in-place removal, NOT a full rebuild,
/// and leaves the surviving raw node with no dangling inverse edge. Run for both id shapes: a reference
/// whose NodeId sits under the equipment id (so the subtree removal reaches it — before the fix the
/// reference loop then found it gone and ratcheted to a rebuild, severing every subscription on the
/// server), and the production shape, an <c>Area/Line/Equipment/Name</c> NodeId parented to an
/// id-named equipment folder (which the subtree's id-prefix scope does not reach).
/// </summary>
[Trait("Category", "Unit")]
[Theory]
[InlineData("eq-1/Temp")]
[InlineData("Area/Line/Eq/Temp")]
public async Task Removing_equipment_with_references_is_surgical_and_leaves_no_dangling_edge(string mirrorId)
{
var (host, server) = await BootAsync();
try
{
var nm = server.NodeManager!;
nm.EnsureVariable("Plant/Modbus/dev1/temp", null, "temp", "Float", writable: false, realm: AddressSpaceRealm.Raw);
nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1", realm: AddressSpaceRealm.Uns);
nm.EnsureVariable(mirrorId, "eq-1", "Temp", "Float", writable: false, realm: AddressSpaceRealm.Uns);
nm.AddReference(mirrorId, AddressSpaceRealm.Uns, "Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw);
// A bystander whose value must survive (a rebuild would drop it).
nm.EnsureFolder("eq-2", parentNodeId: null, displayName: "Equipment 2", realm: AddressSpaceRealm.Uns);
nm.EnsureVariable("eq-2/S", "eq-2", "S", "Float", writable: false, realm: AddressSpaceRealm.Uns);
var mirror = nm.TryGetVariable(mirrorId, AddressSpaceRealm.Uns)!.NodeId;
var plan = new AddressSpacePlan(
AddedEquipment: Array.Empty<EquipmentNode>(),
RemovedEquipment: new[] { new EquipmentNode("eq-1", "Equipment 1", "") },
ChangedEquipment: Array.Empty<AddressSpacePlan.EquipmentDelta>(),
AddedDrivers: Array.Empty<DriverInstancePlan>(),
RemovedDrivers: Array.Empty<DriverInstancePlan>(),
ChangedDrivers: Array.Empty<AddressSpacePlan.DriverDelta>(),
AddedAlarms: Array.Empty<ScriptedAlarmPlan>(),
RemovedAlarms: Array.Empty<ScriptedAlarmPlan>(),
ChangedAlarms: Array.Empty<AddressSpacePlan.AlarmDelta>())
{
RemovedUnsReferenceVariables = new[]
{
new UnsReferenceVariable("ref-1", "eq-1", mirrorId, "Temp", "Plant/Modbus/dev1/temp", "Float", Writable: false),
},
};
var outcome = new AddressSpaceApplier(new SdkAddressSpaceSink(nm),
Microsoft.Extensions.Logging.Abstractions.NullLogger<AddressSpaceApplier>.Instance).Apply(plan);
outcome.RebuildCalled.ShouldBeFalse();
nm.TryGetVariable(mirrorId, AddressSpaceRealm.Uns).ShouldBeNull();
nm.TryGetFolder("eq-1").ShouldBeNull();
nm.TryGetVariable("eq-2/S", AddressSpaceRealm.Uns).ShouldNotBeNull(); // positive control: no rebuild
nm.TryGetVariable("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw).ShouldNotBeNull();
InverseOrganizesTo(nm, mirror).ShouldBeFalse();
}
finally
{
await host.DisposeAsync();
}
}
private static bool InverseOrganizesTo(OtOpcUaNodeManager nm, NodeId mirror) =>
nm.GetNodeReferences("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw)
.Any(r => r.ReferenceTypeId == ReferenceTypeIds.Organizes && r.IsInverse && r.TargetId == mirror);
private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync()
{
var host = new OpcUaApplicationHost(
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/UnsReferenceBackingParityTests.cs
+158
View File
@@ -0,0 +1,158 @@
using System.Reflection;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests;
/// <summary>
/// Gitea #599 / archreview 03/U11 — a mechanical guard on the <c>Backing*</c>-field parity invariant.
/// </summary>
/// <remarks>
/// <para>
/// A non-alarm raw-tag delta over the classifier's surgical whitelist is applied IN PLACE on the raw
/// node, and every referencing UNS mirror is repaired only because the same edit makes its
/// <see cref="UnsReferenceVariable"/> record unequal — the planner then emits a coincident
/// <c>UnsReferenceDelta</c>. That works only while every whitelisted raw-tag field the mirror inherits
/// has a counterpart on <see cref="UnsReferenceVariable"/> (same name, or <c>Backing</c> + name).
/// Omitting one is silent: the raw node updates, the mirror stays stale, and nothing fails. Until this
/// test the invariant was enforced by comments alone.
/// </para>
/// <para>
/// The whitelist is DISCOVERED, not restated: each <see cref="RawTagPlan"/> property is perturbed in
/// isolation and asked of <see cref="AddressSpaceChangeClassifier.RawTagDeltaIsSurgicalEligible"/>. So
/// widening the classifier whitelist without mirroring the field fails here, and so does adding a
/// <c>Backing*</c> field that names no raw-tag field.
/// </para>
/// </remarks>
public sealed class UnsReferenceBackingParityTests
{
/// <summary>
/// Surgically-eligible raw-tag fields that a UNS mirror deliberately does NOT inherit, each with the
/// reason. Anything else the classifier lets through surgically must be mirrored.
/// </summary>
private static readonly IReadOnlyDictionary<string, string> NotInheritedByMirror = new Dictionary<string, string>
{
// Node-inert for a value tag: it only drives native-alarm notifier wiring, and an alarm-bearing delta
// never takes the surgical path. The mirror is the referencing side, so it has nothing to inherit.
[nameof(RawTagPlan.ReferencingEquipmentPaths)] = "notifier wiring only; the mirror is the referencing side",
};
private static RawTagPlan Baseline() => new(
TagId: "tag-1",
NodeId: "MAIN/modbus/dev/grp/temp",
ParentNodeId: "MAIN/modbus/dev/grp",
DriverInstanceId: "drv-1",
Name: "temp",
DataType: "Float",
Writable: false,
Alarm: null,
ReferencingEquipmentPaths: new[] { "Area/Line/Eq" },
IsHistorized: false,
HistorianTagname: null,
IsArray: false,
ArrayLength: null,
Realm: AddressSpaceRealm.Raw);
/// <summary>The raw-tag fields the classifier lets through surgically, found by perturbing each one.</summary>
private static IReadOnlyList<PropertyInfo> SurgicallyEligibleRawTagFields()
{
var baseline = Baseline();
var eligible = new List<PropertyInfo>();
foreach (var p in RecordProperties(typeof(RawTagPlan)))
{
var current = Perturb(baseline, p);
if (AddressSpaceChangeClassifier.RawTagDeltaIsSurgicalEligible(new AddressSpacePlan.RawTagDelta(baseline, current)))
eligible.Add(p);
}
return eligible;
}
/// <summary>
/// Every raw-tag field the classifier applies surgically is mirrored on the UNS reference record (or is
/// explicitly listed as not inherited) — with the same type, so the planner's record equality sees the
/// edit.
/// </summary>
[Trait("Category", "Unit")]
[Fact]
public void Every_surgically_eligible_raw_tag_field_is_mirrored_on_the_UNS_reference_record()
{
var eligible = SurgicallyEligibleRawTagFields();
// Positive control: the perturbation machinery really does find the whitelist. If it found nothing,
// the loop below would pass vacuously.
eligible.Select(p => p.Name).ShouldBe(
new[]
{
nameof(RawTagPlan.DataType), nameof(RawTagPlan.Writable), nameof(RawTagPlan.ReferencingEquipmentPaths),
nameof(RawTagPlan.IsHistorized), nameof(RawTagPlan.HistorianTagname), nameof(RawTagPlan.IsArray),
nameof(RawTagPlan.ArrayLength),
},
ignoreOrder: true);
var mirror = RecordProperties(typeof(UnsReferenceVariable)).ToDictionary(p => p.Name, StringComparer.Ordinal);
foreach (var p in eligible)
{
if (NotInheritedByMirror.ContainsKey(p.Name)) continue;
var counterpart = mirror.GetValueOrDefault(p.Name) ?? mirror.GetValueOrDefault("Backing" + p.Name);
counterpart.ShouldNotBeNull(
$"RawTagPlan.{p.Name} is applied surgically on the raw node, but UnsReferenceVariable has neither " +
$"'{p.Name}' nor 'Backing{p.Name}' — a raw-side edit of it would leave every referencing UNS mirror stale.");
counterpart.PropertyType.ShouldBe(p.PropertyType, $"UnsReferenceVariable.{counterpart.Name} vs RawTagPlan.{p.Name}");
}
}
/// <summary>Every <c>Backing*</c> field on the UNS reference record names a real raw-tag field of the same
/// type — a misspelt or orphaned mirror field repairs nothing.</summary>
[Trait("Category", "Unit")]
[Fact]
public void Every_Backing_field_names_a_raw_tag_field_of_the_same_type()
{
var raw = RecordProperties(typeof(RawTagPlan)).ToDictionary(p => p.Name, StringComparer.Ordinal);
var backing = RecordProperties(typeof(UnsReferenceVariable))
.Where(p => p.Name.StartsWith("Backing", StringComparison.Ordinal) && p.Name != nameof(UnsReferenceVariable.BackingRawPath))
.ToList();
backing.ShouldNotBeEmpty();
foreach (var b in backing)
{
var source = raw.GetValueOrDefault(b.Name["Backing".Length..]);
source.ShouldNotBeNull($"UnsReferenceVariable.{b.Name} names no RawTagPlan field");
source.PropertyType.ShouldBe(b.PropertyType, $"UnsReferenceVariable.{b.Name} vs RawTagPlan.{source.Name}");
}
}
/// <summary>The allow-list stays honest: every entry must still be a surgically-eligible raw-tag field.</summary>
[Trait("Category", "Unit")]
[Fact]
public void Not_inherited_allow_list_names_only_surgically_eligible_fields()
{
var eligible = SurgicallyEligibleRawTagFields().Select(p => p.Name).ToHashSet(StringComparer.Ordinal);
foreach (var name in NotInheritedByMirror.Keys) eligible.ShouldContain(name);
}
private static IEnumerable<PropertyInfo> RecordProperties(Type t) =>
t.GetProperties(BindingFlags.Public | BindingFlags.Instance)
.Where(p => p.CanWrite && p.Name != "EqualityContract");
/// <summary>A copy of <paramref name="source"/> differing ONLY in <paramref name="property"/>.</summary>
private static RawTagPlan Perturb(RawTagPlan source, PropertyInfo property)
{
var clone = (RawTagPlan)typeof(RawTagPlan).GetMethod("<Clone>$")!.Invoke(source, null)!;
var value = property.GetValue(source);
object? changed = property.PropertyType switch
{
var t when t == typeof(bool) => !(bool)value!,
var t when t == typeof(string) => (string?)value + "-changed",
var t when t == typeof(uint?) => ((uint?)value ?? 0u) + 1u,
var t when t == typeof(IReadOnlyList<string>) => ((IReadOnlyList<string>)value!).Append("Other/Eq").ToArray(),
var t when t == typeof(AddressSpaceRealm) => (AddressSpaceRealm)value! == AddressSpaceRealm.Raw ? AddressSpaceRealm.Uns : AddressSpaceRealm.Raw,
var t when t == typeof(EquipmentTagAlarmInfo) => new EquipmentTagAlarmInfo("OffNormalAlarm", 500),
var t => throw new InvalidOperationException(
$"RawTagPlan.{property.Name} has type {t.Name}, which this guard cannot perturb — add a case so the field is checked."),
};
property.SetValue(clone, changed);
return clone;
}
}