From d2851745971da2aa27f9862e6ee71d40f088d4a3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 28 May 2026 12:09:43 -0400 Subject: [PATCH] feat(dcl+ui): rename BrowseOpcUaNode -> ConnectionName-keyed; implement site handler + dialog failure mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - BrowseOpcUaNodeCommand: int DataConnectionId -> string ConnectionName (site DataConnectionManagerActor indexes children by name; CentralUI already has the connection name in scope via the dropdown — no extra plumbing across the trust boundary). - IOpcUaBrowseService / OpcUaBrowseService: parameter renamed accordingly. - OpcUaBrowserDialog: collapse the duplicate ConnectionName parameters (display label and routing key are the same string). - Task 10: DataConnectionManagerActor forwards BrowseOpcUaNodeCommand to its child by name (owns ConnectionNotFound); DataConnectionActor adds the receive across all three lifecycle states (Connecting / Connected / Reconnecting) and maps adapter outcomes to BrowseFailureKind (NotBrowsable / ConnectionNotConnected / Timeout / ServerError). - Task 17: SetFailure in OpcUaBrowserDialog implements the full BrowseFailureKind switch with friendly UI messages. - Tests: DataConnectionManagerBrowseHandlerTests covers ConnectionNotFound, NotBrowsable, success, and ConnectionNotConnectedException paths. --- .../Dialogs/OpcUaBrowserDialog.razor | 28 ++- .../Services/IOpcUaBrowseService.cs | 6 +- .../Services/OpcUaBrowseService.cs | 4 +- .../Messages/Management/BrowseCommands.cs | 11 +- .../Actors/DataConnectionActor.cs | 83 +++++++++ .../Actors/DataConnectionManagerActor.cs | 29 +++ ...DataConnectionManagerBrowseHandlerTests.cs | 165 ++++++++++++++++++ 7 files changed, 313 insertions(+), 13 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionManagerBrowseHandlerTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Dialogs/OpcUaBrowserDialog.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Dialogs/OpcUaBrowserDialog.razor index 04f0042d..b1895772 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Dialogs/OpcUaBrowserDialog.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Dialogs/OpcUaBrowserDialog.razor @@ -57,7 +57,12 @@ @code { [Parameter] public string SiteId { get; set; } = ""; - [Parameter] public int DataConnectionId { get; set; } + /// + /// Name of the site-local data connection. Serves both as the modal-header + /// display label AND as the routing key for the browse round-trip — the + /// site's DataConnectionManagerActor indexes its children by + /// connection name (no id-keyed lookup at the site). + /// [Parameter] public string ConnectionName { get; set; } = ""; [Parameter] public string? InitialNodeId { get; set; } [Parameter] public EventCallback OnSelected { get; set; } @@ -105,7 +110,7 @@ _rootNodes = new(); StateHasChanged(); - var result = await BrowseService.BrowseChildrenAsync(SiteId, DataConnectionId, parentNodeId: null); + var result = await BrowseService.BrowseChildrenAsync(SiteId, ConnectionName, parentNodeId: null); if (result.Failure is not null) { SetFailure(result.Failure); @@ -130,7 +135,7 @@ { node.Loading = true; StateHasChanged(); - var result = await BrowseService.BrowseChildrenAsync(SiteId, DataConnectionId, node.NodeId); + var result = await BrowseService.BrowseChildrenAsync(SiteId, ConnectionName, node.NodeId); node.Loading = false; if (result.Failure is not null) @@ -155,12 +160,23 @@ _manualNodeId = node.NodeId; } - // NOTE: Task 17 will replace this body with the full BrowseFailureKind switch - // that maps each failure kind to a friendly UI message. + // Task 17: map each BrowseFailureKind to a friendly UI message. The raw + // failure.Message is surfaced verbatim only for ServerError (which carries + // the OPC UA SDK's own Bad_* text) and as the default fallback for any + // future failure kind added without a UI mapping. private void SetFailure(BrowseFailure failure) { _failure = failure; - _failureMessage = failure.Message; + _failureMessage = failure.Kind switch + { + BrowseFailureKind.ConnectionNotFound => "Connection no longer exists at the site.", + BrowseFailureKind.ConnectionNotConnected => "OPC UA session not connected — retry shortly or use manual entry.", + BrowseFailureKind.NotBrowsable => "This connection does not support browsing.", + BrowseFailureKind.Timeout => "Browse timed out — the server may be slow. Try again or enter the node id manually.", + BrowseFailureKind.ServerError => $"OPC UA server error: {failure.Message}", + _ => failure.Message + }; + StateHasChanged(); } private Task RetryRootLoad() => LoadRootAsync(); diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/IOpcUaBrowseService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/IOpcUaBrowseService.cs index 37fedeee..ea9aaf0e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/IOpcUaBrowseService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/IOpcUaBrowseService.cs @@ -21,17 +21,17 @@ public interface IOpcUaBrowseService { /// /// Enumerates the immediate children of an OPC UA node on the live server - /// backing at . + /// backing at . /// Pass null for to browse from the /// server root (ObjectsFolder). /// /// The target site identifier. - /// Id of the site-local data connection to browse against. + /// Name of the site-local data connection to browse against — the site's DataConnectionManagerActor indexes its children by name. /// Node to browse, or null to browse from the server root. /// Cancellation token. Task BrowseChildrenAsync( string siteId, - int dataConnectionId, + string connectionName, string? parentNodeId, CancellationToken cancellationToken = default); } diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/OpcUaBrowseService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/OpcUaBrowseService.cs index 2b50d9b3..6adec7f3 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/OpcUaBrowseService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/OpcUaBrowseService.cs @@ -37,7 +37,7 @@ public sealed class OpcUaBrowseService : IOpcUaBrowseService /// public async Task BrowseChildrenAsync( string siteId, - int dataConnectionId, + string connectionName, string? parentNodeId, CancellationToken cancellationToken = default) { @@ -56,7 +56,7 @@ public sealed class OpcUaBrowseService : IOpcUaBrowseService { return await _communication.BrowseOpcUaNodeAsync( siteId, - new BrowseOpcUaNodeCommand(dataConnectionId, parentNodeId), + new BrowseOpcUaNodeCommand(connectionName, parentNodeId), cancellationToken); } catch (TimeoutException ex) diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/BrowseCommands.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/BrowseCommands.cs index fbc58205..73fffa58 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/BrowseCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/BrowseCommands.cs @@ -6,10 +6,17 @@ namespace ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; /// Sent from CentralUI to a specific site to enumerate the immediate children /// of an OPC UA node on the live server backing the given data connection. /// -/// Id of the site-local data connection to browse against. +/// +/// Keyed by (not id) because the site-side +/// DataConnectionManagerActor indexes its children by connection name — +/// the central UI already has the connection name in scope (dropdown), so a +/// string carries no extra plumbing across the trust boundary. The central +/// DataConnections table's id is intentionally not exposed at the site. +/// +/// Name of the site-local data connection to browse against. /// Node to browse, or null to browse from the server root (ObjectsFolder). public record BrowseOpcUaNodeCommand( - int DataConnectionId, + string ConnectionName, string? ParentNodeId); public record BrowseOpcUaNodeResult( diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs index 2c6b5963..dc5ab774 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs @@ -2,6 +2,7 @@ using Akka.Actor; using Akka.Event; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Protocol; using ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; using ZB.MOM.WW.ScadaBridge.HealthMonitoring; using ZB.MOM.WW.ScadaBridge.SiteEventLogging; @@ -233,6 +234,13 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers // apply it so its state survives into the next ReSubscribeAll. HandleSubscribeCompleted(sc); break; + case BrowseOpcUaNodeCommand browse: + // Browse is an interactive design-time query; never stash. The + // adapter has no session yet in this state, so reply with a + // typed ConnectionNotConnected failure so the dialog can render + // an inline banner. + HandleBrowse(browse); + break; case GetHealthReport: ReplyWithHealthReport(); break; @@ -293,6 +301,9 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers case RetryTagResolution: HandleRetryTagResolution(); break; + case BrowseOpcUaNodeCommand browse: + HandleBrowse(browse); + break; case GetHealthReport: ReplyWithHealthReport(); break; @@ -412,6 +423,12 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers // apply it so its state survives into the next ReSubscribeAll. HandleSubscribeCompleted(sc); break; + case BrowseOpcUaNodeCommand browse: + // Browse is design-time and never stashed. While reconnecting + // the adapter has no live session, so the adapter call will + // throw ConnectionNotConnectedException — mapped by HandleBrowse. + HandleBrowse(browse); + break; case GetHealthReport: ReplyWithHealthReport(); break; @@ -947,6 +964,72 @@ public class DataConnectionActor : UntypedActor, IWithStash, IWithTimers }).PipeTo(sender); } + // ── OPC UA Tag Browser (interactive design-time query) ── + + /// + /// Handles a forwarded by the + /// . The capability check (does + /// this adapter support browsing?) and all browse-failure mapping live + /// here because the adapter is held by this actor, not the manager. + /// + /// Failure mapping: + /// + /// — adapter is not . + /// — adapter threw . + /// — adapter threw . + /// — any other exception, message carried verbatim. + /// + /// + /// The reply is sent via PipeTo(sender) — the same pattern used by + /// — so the captured is + /// safe to use from the continuation (which runs off the actor thread). + /// + private void HandleBrowse(BrowseOpcUaNodeCommand command) + { + var sender = Sender; + + if (_adapter is not IBrowsableDataConnection browsable) + { + _log.Debug("[{0}] Browse requested but adapter does not implement IBrowsableDataConnection", _connectionName); + sender.Tell(new BrowseOpcUaNodeResult( + Array.Empty(), + Truncated: false, + new BrowseFailure( + BrowseFailureKind.NotBrowsable, + $"Connection '{_connectionName}' does not support browsing."))); + return; + } + + _log.Debug("[{0}] Browsing OPC UA children of {1}", _connectionName, command.ParentNodeId ?? "(root)"); + + browsable.BrowseChildrenAsync(command.ParentNodeId).ContinueWith(t => + { + if (t.IsCompletedSuccessfully) + { + return new BrowseOpcUaNodeResult(t.Result.Children, t.Result.Truncated, Failure: null); + } + + var baseEx = t.Exception?.GetBaseException(); + return baseEx switch + { + ConnectionNotConnectedException notConnected => new BrowseOpcUaNodeResult( + Array.Empty(), + Truncated: false, + new BrowseFailure(BrowseFailureKind.ConnectionNotConnected, notConnected.Message)), + OperationCanceledException => new BrowseOpcUaNodeResult( + Array.Empty(), + Truncated: false, + new BrowseFailure(BrowseFailureKind.Timeout, "Browse cancelled.")), + _ => new BrowseOpcUaNodeResult( + Array.Empty(), + Truncated: false, + new BrowseFailure( + BrowseFailureKind.ServerError, + baseEx?.Message ?? "Unknown browse error.")), + }; + }).PipeTo(sender); + } + // ── Tag Resolution Retry (WP-12) ── private void HandleRetryTagResolution() diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs index 5ff66fd1..18e0998b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs @@ -2,6 +2,7 @@ using Akka.Actor; using Akka.Event; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Protocol; using ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; using ZB.MOM.WW.ScadaBridge.HealthMonitoring; using ZB.MOM.WW.ScadaBridge.SiteEventLogging; @@ -45,6 +46,7 @@ public class DataConnectionManagerActor : ReceiveActor Receive(HandleRouteWrite); Receive(HandleRemoveConnection); Receive(HandleGetAllHealthReports); + Receive(HandleBrowse); } private void HandleCreateConnection(CreateConnectionCommand command) @@ -111,6 +113,33 @@ public class DataConnectionManagerActor : ReceiveActor } } + /// + /// Routes a from the central UI's OPC UA + /// Tag Browser to the child that owns the + /// named connection. The manager is the only actor that knows whether a + /// connection exists at this site — so it owns the + /// failure. Everything + /// else (capability check, session state, server errors) lives inside the + /// child where the adapter is held. + /// + private void HandleBrowse(BrowseOpcUaNodeCommand command) + { + if (_connectionActors.TryGetValue(command.ConnectionName, out var actor)) + { + actor.Forward(command); + } + else + { + _log.Warning("No connection actor for {0} during browse", command.ConnectionName); + Sender.Tell(new BrowseOpcUaNodeResult( + Array.Empty(), + Truncated: false, + new BrowseFailure( + BrowseFailureKind.ConnectionNotFound, + $"No data connection named '{command.ConnectionName}' at this site."))); + } + } + private void HandleRemoveConnection(RemoveConnectionCommand command) { if (_connectionActors.TryGetValue(command.ConnectionName, out var actor)) diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionManagerBrowseHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionManagerBrowseHandlerTests.cs new file mode 100644 index 00000000..110fdce3 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionManagerBrowseHandlerTests.cs @@ -0,0 +1,165 @@ +using Akka.Actor; +using Akka.TestKit.Xunit2; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Protocol; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Actors; +using ZB.MOM.WW.ScadaBridge.HealthMonitoring; + +namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests.Actors; + +/// +/// Task 10 (opcua-tag-browser): the site-side +/// + child +/// together resolve +/// against the live adapter and surface +/// every browse outcome as a typed . The split is: +/// the manager owns (only it +/// knows the per-site connection set); everything else lives in the child where +/// the adapter is held — from the +/// capability check, / +/// / +/// from the adapter call. These tests guard that split. +/// +public class DataConnectionManagerBrowseHandlerTests : TestKit +{ + private readonly IDataConnectionFactory _factory; + private readonly ISiteHealthCollector _healthCollector; + private readonly DataConnectionOptions _options; + + public DataConnectionManagerBrowseHandlerTests() + : base(@"akka.loglevel = WARNING") + { + _factory = Substitute.For(); + _healthCollector = Substitute.For(); + _options = new DataConnectionOptions + { + ReconnectInterval = TimeSpan.FromSeconds(30), + TagResolutionRetryInterval = TimeSpan.FromSeconds(30), + }; + } + + [Fact] + public void Unknown_connection_name_returns_ConnectionNotFound() + { + var manager = Sys.ActorOf(Props.Create(() => + new DataConnectionManagerActor(_factory, _options, _healthCollector, null))); + + // No CreateConnectionCommand sent — the manager has zero children, so a + // browse against any name must be rejected with ConnectionNotFound + // (the manager is the only actor with site-level visibility). + manager.Tell(new BrowseOpcUaNodeCommand("unknown-connection", ParentNodeId: null)); + + var reply = ExpectMsg(); + Assert.NotNull(reply.Failure); + Assert.Equal(BrowseFailureKind.ConnectionNotFound, reply.Failure!.Kind); + Assert.Empty(reply.Children); + } + + [Fact] + public void Non_browsable_adapter_returns_NotBrowsable() + { + // Bare IDataConnection — no IBrowsableDataConnection. The child actor's + // capability check must surface this as NotBrowsable. + var adapter = Substitute.For(); + adapter.ConnectAsync(Arg.Any>(), Arg.Any()) + .Returns(Task.CompletedTask); + adapter.Status.Returns(ConnectionHealth.Connected); + _factory.Create("OpcUa", Arg.Any>()).Returns(adapter); + + var manager = Sys.ActorOf(Props.Create(() => + new DataConnectionManagerActor(_factory, _options, _healthCollector, null))); + manager.Tell(new CreateConnectionCommand( + "conn-bare", "OpcUa", new Dictionary(), null, 3)); + + // Give the manager a moment to spawn the child actor. We do not need to + // wait for Connected — the browse handler runs in all states. + AwaitCondition( + () => _factory.ReceivedCalls().Any(c => c.GetMethodInfo().Name == "Create"), + TimeSpan.FromSeconds(2)); + + manager.Tell(new BrowseOpcUaNodeCommand("conn-bare", ParentNodeId: null)); + + var reply = ExpectMsg(TimeSpan.FromSeconds(3)); + Assert.NotNull(reply.Failure); + Assert.Equal(BrowseFailureKind.NotBrowsable, reply.Failure!.Kind); + Assert.Empty(reply.Children); + } + + [Fact] + public void Success_path_returns_mapped_children() + { + // Adapter implementing both IDataConnection (so DataConnectionActor can + // run its lifecycle) AND IBrowsableDataConnection (so the browse handler + // takes the success path). + var adapter = Substitute.For(); + ((IDataConnection)adapter).ConnectAsync(Arg.Any>(), Arg.Any()) + .Returns(Task.CompletedTask); + ((IDataConnection)adapter).Status.Returns(ConnectionHealth.Connected); + + var children = new[] + { + new BrowseNode("ns=2;s=A", "A", BrowseNodeClass.Variable, HasChildren: false), + new BrowseNode("ns=2;s=B", "B", BrowseNodeClass.Object, HasChildren: true), + }; + ((IBrowsableDataConnection)adapter) + .BrowseChildrenAsync(null, Arg.Any()) + .Returns(new BrowseChildrenResult(children, Truncated: false)); + + _factory.Create("OpcUa", Arg.Any>()) + .Returns((IDataConnection)adapter); + + var manager = Sys.ActorOf(Props.Create(() => + new DataConnectionManagerActor(_factory, _options, _healthCollector, null))); + manager.Tell(new CreateConnectionCommand( + "conn-ok", "OpcUa", new Dictionary(), null, 3)); + + AwaitCondition( + () => _factory.ReceivedCalls().Any(c => c.GetMethodInfo().Name == "Create"), + TimeSpan.FromSeconds(2)); + + manager.Tell(new BrowseOpcUaNodeCommand("conn-ok", ParentNodeId: null)); + + var reply = ExpectMsg(TimeSpan.FromSeconds(3)); + Assert.Null(reply.Failure); + Assert.Equal(2, reply.Children.Count); + Assert.Equal("ns=2;s=A", reply.Children[0].NodeId); + Assert.Equal("ns=2;s=B", reply.Children[1].NodeId); + Assert.False(reply.Truncated); + } + + [Fact] + public void ConnectionNotConnectedException_maps_to_ConnectionNotConnected() + { + var adapter = Substitute.For(); + ((IDataConnection)adapter).ConnectAsync(Arg.Any>(), Arg.Any()) + .Returns(Task.CompletedTask); + ((IDataConnection)adapter).Status.Returns(ConnectionHealth.Connected); + + ((IBrowsableDataConnection)adapter) + .BrowseChildrenAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromException( + new ConnectionNotConnectedException("OPC UA session is not connected."))); + + _factory.Create("OpcUa", Arg.Any>()) + .Returns((IDataConnection)adapter); + + var manager = Sys.ActorOf(Props.Create(() => + new DataConnectionManagerActor(_factory, _options, _healthCollector, null))); + manager.Tell(new CreateConnectionCommand( + "conn-down", "OpcUa", new Dictionary(), null, 3)); + + AwaitCondition( + () => _factory.ReceivedCalls().Any(c => c.GetMethodInfo().Name == "Create"), + TimeSpan.FromSeconds(2)); + + manager.Tell(new BrowseOpcUaNodeCommand("conn-down", ParentNodeId: null)); + + var reply = ExpectMsg(TimeSpan.FromSeconds(3)); + Assert.NotNull(reply.Failure); + Assert.Equal(BrowseFailureKind.ConnectionNotConnected, reply.Failure!.Kind); + Assert.Empty(reply.Children); + } +}