fix(opcua): equipment-tag planner diff + folder-scoped NodeIds (review findings)

Two bundle-review fixes + idempotency coverage: - CRITICAL: the planner ignored EquipmentTags, so an incremental deploy changing only equipment tags produced an empty plan and HandleRebuild short-circuited before materialising them. Add TagId to EquipmentTagPlan + Added/Removed/ChangedEquipmentTags to Phase7Plan (diffed by TagId, in IsEmpty, driving Apply's needsRebuild) — mirroring the GalaxyTags treatment. - IMPORTANT: equipment variable NodeId was the raw driver FullName, which collides across identical machines (e.g. two PLCs both exposing register 40001) — the second variable was silently dropped. NodeId is now folder-scoped (parent/Name); FullName stays on EquipmentTagPlan for the later values-routing milestone. - Task 4: SDK-backed idempotency test (double-apply -> single variable); restart-safety confirmed (RestoreApplied reuses the same RebuildAddressSpace -> HandleRebuild path). - Minor: align composer equipment-tag sort with the artifact decoder (coalesce FolderPath).
2026-06-06 15:02:50 -04:00
parent 08cddfe128
commit aaf869145a
9 changed files with 192 additions and 28 deletions
@@ -110,6 +110,54 @@ public sealed class Phase7ApplierHierarchyTests : IDisposable
        sdkServer.NodeManager!.FolderCount.ShouldBe(5);
    }

+    /// <summary>Verifies MaterialiseEquipmentTags is idempotent against a real SDK node manager:
+    /// applying the same composition twice yields a single Variable node (no duplicates). This is the
+    /// restart-safety guarantee — HandleRebuild runs on both the apply path and the
+    /// DriverHostActor.RestoreApplied bootstrap path (same RebuildAddressSpace message), so a node
+    /// restart re-runs this pass and must not double-materialise.</summary>
+    [Fact]
+    public async Task MaterialiseEquipmentTags_against_real_SDK_node_manager_is_idempotent()
+    {
+        await using var host = new OpcUaApplicationHost(
+            new OpcUaApplicationHostOptions
+            {
+                ApplicationName = "OtOpcUa.EquipmentTags",
+                ApplicationUri = $"urn:OtOpcUa.EquipmentTags:{Guid.NewGuid():N}",
+                OpcUaPort = AllocateFreePort(),
+                PublicHostname = "localhost",
+                PkiStoreRoot = _pkiRoot,
+            },
+            NullLogger<OpcUaApplicationHost>.Instance);
+
+        var sdkServer = new OtOpcUaSdkServer();
+        await host.StartAsync(sdkServer, Ct);
+        sdkServer.NodeManager.ShouldNotBeNull();
+
+        var sink = new SdkAddressSpaceSink(sdkServer.NodeManager!);
+        var applier = new Phase7Applier(sink, NullLogger<Phase7Applier>.Instance);
+
+        var composition = new Phase7CompositionResult(
+            UnsAreas: Array.Empty<UnsAreaProjection>(),
+            UnsLines: Array.Empty<UnsLineProjection>(),
+            EquipmentNodes: new[] { new EquipmentNode("eq-1", "filling-eq", UnsLineId: "") },
+            DriverInstancePlans: Array.Empty<DriverInstancePlan>(),
+            ScriptedAlarmPlans: Array.Empty<ScriptedAlarmPlan>(),
+            GalaxyTags: Array.Empty<GalaxyTagPlan>())
+        {
+            EquipmentTags = new[]
+            {
+                new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001"),
+            },
+        };
+
+        // Equipment folder first (the variable's parent), then the tag pass — applied twice.
+        applier.MaterialiseHierarchy(composition);
+        applier.MaterialiseEquipmentTags(composition);
+        applier.MaterialiseEquipmentTags(composition);
+
+        sdkServer.NodeManager!.VariableCount.ShouldBe(1); // single variable despite the double-apply
+    }
+
    private static int AllocateFreePort()
    {
        using var listener = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0);
@@ -178,8 +178,9 @@ public sealed class Phase7ApplierTests
    }

    /// <summary>Verifies MaterialiseEquipmentTags creates one Variable per equipment tag directly
-    /// under its existing equipment folder (NodeId == FullName, parent == EquipmentId,
-    /// displayName == Name) and does NOT re-create the equipment folder (decision #4).</summary>
+    /// under its existing equipment folder, with a folder-scoped NodeId (parent/Name — NOT the raw
+    /// FullName), parent == EquipmentId, displayName == Name, and does NOT re-create the equipment
+    /// folder (decision #4).</summary>
    [Fact]
    public void MaterialiseEquipmentTags_creates_variable_under_equipment_folder()
    {
@@ -196,18 +197,19 @@ public sealed class Phase7ApplierTests
        {
            EquipmentTags = new[]
            {
-                new EquipmentTagPlan("eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001"),
+                new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001"),
            },
        };

        applier.MaterialiseEquipmentTags(composition);

        sink.FolderCalls.ShouldBeEmpty(); // equipment folder already exists; no sub-folder needed
-        sink.VariableCalls.ShouldHaveSingleItem().ShouldBe(("40001", "eq-1", "Speed", "Float"));
+        sink.VariableCalls.ShouldHaveSingleItem().ShouldBe(("eq-1/Speed", "eq-1", "Speed", "Float"));
    }

    /// <summary>Verifies a FolderPath on an equipment tag becomes a sub-folder UNDER the equipment
-    /// folder (not the namespace root), with the variable parented to that sub-folder.</summary>
+    /// folder (not the namespace root), with the variable parented to that sub-folder and a
+    /// folder-scoped NodeId.</summary>
    [Fact]
    public void MaterialiseEquipmentTags_nests_FolderPath_subfolder_under_equipment()
    {
@@ -219,14 +221,64 @@ public sealed class Phase7ApplierTests
        {
            EquipmentTags = new[]
            {
-                new EquipmentTagPlan("eq-1", "drv", FolderPath: "Diagnostics", Name: "Temp", DataType: "Float", FullName: "40002"),
+                new EquipmentTagPlan("tag-2", "eq-1", "drv", FolderPath: "Diagnostics", Name: "Temp", DataType: "Float", FullName: "40002"),
            },
        };

        applier.MaterialiseEquipmentTags(composition);

        sink.FolderCalls.ShouldHaveSingleItem().ShouldBe(("eq-1/Diagnostics", "eq-1", "Diagnostics"));
-        sink.VariableCalls.ShouldHaveSingleItem().ShouldBe(("40002", "eq-1/Diagnostics", "Temp", "Float"));
+        sink.VariableCalls.ShouldHaveSingleItem().ShouldBe(("eq-1/Diagnostics/Temp", "eq-1/Diagnostics", "Temp", "Float"));
+    }
+
+    /// <summary>Regression for the FullName-as-NodeId collision: two identical machines exposing the
+    /// SAME driver FullName (e.g. Modbus register 40001) must produce TWO distinct variables — one
+    /// under each equipment folder — because the NodeId is folder-scoped, not the raw FullName.</summary>
+    [Fact]
+    public void MaterialiseEquipmentTags_identical_FullName_across_two_equipments_does_not_collide()
+    {
+        var sink = new RecordingSink();
+        var applier = new Phase7Applier(sink, NullLogger<Phase7Applier>.Instance);
+
+        var composition = new Phase7CompositionResult(
+            Array.Empty<EquipmentNode>(), Array.Empty<DriverInstancePlan>(), Array.Empty<ScriptedAlarmPlan>())
+        {
+            EquipmentTags = new[]
+            {
+                new EquipmentTagPlan("tag-a", "eq-1", "drv-1", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001"),
+                new EquipmentTagPlan("tag-b", "eq-2", "drv-2", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001"),
+            },
+        };
+
+        applier.MaterialiseEquipmentTags(composition);
+
+        sink.VariableCalls.Count.ShouldBe(2);
+        sink.VariableCalls.ShouldContain(("eq-1/Speed", "eq-1", "Speed", "Float"));
+        sink.VariableCalls.ShouldContain(("eq-2/Speed", "eq-2", "Speed", "Float"));
+    }
+
+    /// <summary>Verifies that added equipment tags in an otherwise-empty plan trigger an
+    /// address-space rebuild (parity with the Galaxy-tag path — the planner now diffs equipment
+    /// tags, so a tags-only deploy is no longer a silent no-op).</summary>
+    [Fact]
+    public void Added_equipment_tags_trigger_rebuild()
+    {
+        var sink = new RecordingSink();
+        var applier = new Phase7Applier(sink, NullLogger<Phase7Applier>.Instance);
+
+        var plan = EmptyPlan with
+        {
+            AddedEquipmentTags = new[]
+            {
+                new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001"),
+            },
+        };
+
+        var outcome = applier.Apply(plan);
+
+        outcome.RebuildCalled.ShouldBeTrue();
+        outcome.AddedNodes.ShouldBe(1);
+        sink.RebuildCalls.ShouldBe(1);
    }

    /// <summary>Verifies that added Galaxy tags in an otherwise-empty plan trigger an address-space rebuild.</summary>
@@ -30,6 +30,31 @@ public sealed class Phase7PlannerTests
        plan.IsEmpty.ShouldBeTrue();
    }

+    /// <summary>Verifies an equipment-tag-only delta (no equipment/driver/alarm/galaxy change)
+    /// yields a NON-empty plan, so OpcUaPublishActor.HandleRebuild does not short-circuit at the
+    /// IsEmpty gate before materialising the new equipment variables.</summary>
+    [Fact]
+    public void Equipment_tag_only_change_yields_non_empty_plan_with_added_tag()
+    {
+        var prev = new Phase7CompositionResult(
+            Array.Empty<EquipmentNode>(), Array.Empty<DriverInstancePlan>(), Array.Empty<ScriptedAlarmPlan>());
+        var next = new Phase7CompositionResult(
+            Array.Empty<EquipmentNode>(), Array.Empty<DriverInstancePlan>(), Array.Empty<ScriptedAlarmPlan>())
+        {
+            EquipmentTags = new[]
+            {
+                new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001"),
+            },
+        };
+
+        var plan = Phase7Planner.Compute(prev, next);
+
+        plan.IsEmpty.ShouldBeFalse();
+        plan.AddedEquipmentTags.Single().TagId.ShouldBe("tag-1");
+        plan.RemovedEquipmentTags.ShouldBeEmpty();
+        plan.ChangedEquipmentTags.ShouldBeEmpty();
+    }
+
    /// <summary>Verifies that new equipment goes to the AddedEquipment list.</summary>
    [Fact]
    public void New_equipment_goes_to_AddedEquipment()
@@ -167,6 +167,7 @@ public sealed class DeploymentArtifactTests
        var c = DeploymentArtifact.ParseComposition(blob);

        var tag = c.EquipmentTags.ShouldHaveSingleItem();
+        tag.TagId.ShouldBe("tag-eq");
        tag.EquipmentId.ShouldBe("eq-1");
        tag.DriverInstanceId.ShouldBe("drv-modbus");
        tag.Name.ShouldBe("Speed");