Skip to content

Adopt the fluent builder's node-creation surface in the remaining hand-rolled node managers - #4460

Merged
marcschier merged 2 commits into
masterfrom
romanett/opcua-dotnet-4429-6784ff
Sep 10, 2026
Merged

Adopt the fluent builder's node-creation surface in the remaining hand-rolled node managers#4460
marcschier merged 2 commits into
masterfrom
romanett/opcua-dotnet-4429-6784ff

Conversation

@romanett

@romanett romanett commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Closes #4429.

PR #4428 added the node-creation surface to INodeManagerBuilder. This picks up the remaining candidates that needed a base-class change first, and reports on the two that turn out not to be candidates at all.

Converted

samples/WotCon/FlatTagServer/FlatTagNodeManager.cs — now derives from FluentNodeManagerBase. The private CreateObject / CreateVariable / CreateMethod helpers are gone, along with every pumpNodeId + ".Operational.Measurements.MassFlow" concatenation; nodes are staged with AddObject / AddVariable<T> / AddMethod and the hand-maintained externalReferences[ObjectsFolder] entry is replaced by CompleteConfigureAsync.

The EventNotifier bits and AddRootNotifierAsync(pump) are kept verbatim — the pump is the notifier an aggregating server subscribes to, and OPC 10000-3 only delivers the supervision conditions to a client that can reach one.

SupervisionSignal loses its namespaceIndex and tagPath parameters and derives both from the supervision Object it hangs off, so the signal follows wherever that Object was staged.

samples/Redundancy/RedundantServer/HaSampleNodeManager.cs — same shape. The Counter still reaches EnableDistributedValueParticipation through INodeBuilder.Node. The variables keep their Organizes reference type, which the fluent default would otherwise have made HasComponent.

samples/DI/PumpDeviceIntegrationServer/OpenUsdComposition.cs — both AddPredefinedNodeAsync sites stage on the builder now, threaded through ConfigureAsync; DiNodeManager.CreateAddressSpaceAsync already calls RegisterAuthoredNodesAsync between ConfigureAsync and the reverse-reference pass, so no new plumbing was needed. Both methods became synchronous. The NodeIds are unchanged because the hand SystemContext.NodeIdFactory.New(...) calls and AssignInstanceChildNodeIds were already doing exactly what staging does.

CreateRepresentedComponent is left alone: it sits on CreatePumpAsync, a public runtime API, and so is out of scope for the same reason as the DI device builders.

NodeIds are part of two samples' contracts

The issue expected the staged path to mint through the manager's INodeIdFactory, and it does — but for two of these samples the identifiers are a published contract, not an implementation detail:

  • the aggregating client's checked-in Thing Descriptions under samples/WotCon/AggregationClient/Documents/ spell out nsu=…SourceA;s=Pump1.Identification.Manufacturer;
  • samples/Redundancy/RedundantClient/Program.cs addresses new NodeId("Counter", ns) directly.

Both managers therefore override New for those nodes instead of taking the default factory's derived identifier, which satisfies the acceptance criterion that each manager keeps its current address-space shape. FlatTag mints the full dotted browse path; the HA sample mints the bare browse name, but only for the sample folder and its direct children.

Two things are worth knowing for anyone writing such an override.

It has to keep an identifier the caller already chose. DefaultNodeIdFactory.New returns node.NodeId unchanged when it is non-null and in the manager's own namespace. Without that same guard, NodeState.Create(context, nodeId, …, assignNodeIds: true) sets the explicit id and then AssignNodeIds overwrites it from the factory — which silently moved …SupervisionProcessFluid.Cavitation.Alarm to …SupervisionProcessFluid.CavitationAlarm and broke five alarm tests until the guard was added.

It has to be narrow. A browse name is unique only among its siblings. The HA override originally named every node the manager minted, and the historian hangs an HA Configuration object off each node it historizes — so the Counter's and the HistoryEvents' one both landed on ns=2;s=HA Configuration and the strong active/passive replicas refused to start. It is now limited to the nodes the redundant client actually spells out.

Merge with master

Merged origin/master (0e600c0f0). The one conflict was in HaSampleNodeManager.CreateAddressSpaceAsync, where #4445 added AddFactoryAssignedNodes next to the HistoryEvents object this branch had converted. Both are kept, and the factory-assigned subtree stays hand-built and unstaged on purpose: it exists to show what an identity minted straight through NodeIdFactory looks like beside the sample's named ones. It is attached once the folder has been staged, so those identifiers survive untouched and the subtree is still registered as part of the folder's. Its parent's identifier is s=HighAvailability on both sides, so the ids it mints are unchanged from master.

Not candidates

src/Opc.Ua.PubSub.Server/PubSubNodeManager.cs is out for the issue's own DI-device-builder reasoning. All three of its build paths are reachable from runtime Method calls — OnAddDataSetFolder and OnRemoveDataSetFolderRebuildConfigurationAddressSpaceAsync, OnAddPushTarget / OnRemovePushTargetRebuildKeyPushTargetAddressSpaceAsync, OnGetSecurityGroup / OnAddSecurityGroup / OnRemoveSecurityGroupRebuildSecurityGroupAddressSpaceAsync — plus ConfigurationChanged. Each removes its previous roots and rebuilds them, and Add* throws BadInvalidState once RegisterAuthoredNodesAsync has run. Independently of that, its parents (PublishSubscribe, PublishedDataSets, SecurityGroups, KeyPushTargets) are owned by the diagnostics node manager, so AttachToParent would reject them as BadNodeIdUnknown.

src/Opc.Ua.OpenUsd.Server/Scene/UsdSceneMaterializer.Properties.cs — the issue asked whether a builder is reachable at those call sites. It is not. MaterializeUsdStage is a public, documented ISystemContext extension (docs/OpenUsd.md, the package README) whose only input is the context; it returns a detached subtree for the caller to register. It already mints through context.RequireNodeIdFactory(), which is the same factory the staged path uses, so there is nothing to gain and a public signature to break.

Verification

Run against the merged tree:

  • Opc.Ua.Di.Tests — 425/425 pass (covers PumpOpenUsdE2eTests, PumpHostedReferenceTests).
  • Opc.Ua.Redundancy.Samples.Tests — 14/14 pass, including both variants of master's new FactoryAssignedNodeIdsSurviveActiveReplicaFailureAsync and StrongHistorianContinuationsSurviveActiveReplicaFailureAsync.
  • Opc.Ua.WotCon.Samples.Tests — 25 pass, 4 fail. The same 4 fail on a clean baseline worktree (CompanionTypeDefinitionsMatchNativePumpServerAsync, WotPumpInstanceMatchesNativePumpSubsetAsync, RealSamplesAggregateSubscribeAndReplaceGenerationAsync, RealSamplesRouteManagementAndConditionActionsToEachSourceAsync — all BadConfigurationError out of the aggregation client). Pre-existing, unrelated to this change.
  • All three sample projects build with 0 warnings and 0 errors.

🤖 Generated with Claude Code

FlatTagNodeManager, HaSampleNodeManager and the pump sample's OpenUSD
composition each hand-built their instances, minted NodeIds by string
concatenation and kept their own externalReferences[ObjectsFolder] entry.
All three now derive from (or already derived from) FluentNodeManagerBase
and stage their nodes through the builder's Add* surface, so registration
and the reverse-reference pass do that bookkeeping instead.

The two samples whose identifiers are part of their contract keep them:
the aggregating client's Thing Descriptions spell out
s=Pump1.Identification.Manufacturer, and the redundant client addresses
ns=N;s=Counter directly. Both managers therefore override New to mint the
browse path rather than take the default factory's hashed identifier.
Each override reproduces that factory's keep-an-existing-id rule, without
which NodeState.Create(assignNodeIds: true) silently moves a node the
caller named explicitly - the supervision conditions are created that way.

FlatTagNodeManager keeps its EventNotifier bits and its root-notifier
registration: the pump is the notifier an aggregating server subscribes
to, and OPC 10000-3 only delivers the supervision conditions to a client
that can reach one. SupervisionSignal now derives its namespace and
identifier prefix from the supervision Object it hangs off instead of
taking them as parameters.

Two candidates named in the issue are deliberately untouched.
PubSubNodeManager rebuilds its dynamic subtree from runtime Method calls
(AddDataSetFolder, AddPushTarget, AddSecurityGroup) and from
ConfigurationChanged, so it needs a mechanism that works after startup -
the same reason the DI device builders are out of scope - and its parents
belong to the diagnostics node manager, which the staging surface refuses
as unknown. UsdSceneMaterializer is a public ISystemContext extension
that hands back a detached subtree for the caller to register; no builder
is reachable at its call sites, and it already mints through the same
NodeId factory the staged path uses.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Code coverage

Coverage gate passed.

Check Result Threshold
✅ Project line rate 86.91% (261413/300775 lines) >= 70.00%
✅ Project branch rate 76.99% >= 60.00%
ℹ️ Patch coverage no changed C# files -
ℹ️ Baseline delta (advisory) +13.31 pp 73.60% recorded

Coverage is above the recorded baseline - consider ratcheting coverage-thresholds.json.

Thresholds live in coverage-thresholds.json. Whole report before exclusions: line 86.05%, branch 76.17%.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.98%. Comparing base (0e600c0) to head (b326555).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4460   +/-   ##
=======================================
  Coverage   80.98%   80.98%           
=======================================
  Files        2050     2050           
  Lines      300775   300775           
  Branches    52050    52050           
=======================================
+ Hits       243582   243589    +7     
+ Misses      39209    39206    -3     
+ Partials    17984    17980    -4     
Flag Coverage Δ
actions 80.98% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fluent-staging conversions follow the expected RegisterAuthoredNodesAsyncCompleteConfigureAsyncSealConfigurationAsync lifecycle and preserve the samples’ published NodeId/address-space contracts.

Pull request overview

This PR completes adoption of the new fluent INodeManagerBuilder node-creation surface across the remaining sample node managers that were still hand-rolling NodeState creation, while preserving each sample’s published NodeId/address-space contract.

Changes:

  • Converted FlatTagNodeManager and HaSampleNodeManager to derive from FluentNodeManagerBase, stage nodes via NodeManagerBuilder, and register via RegisterAuthoredNodesAsync + CompleteConfigureAsync + SealConfigurationAsync.
  • Added New(ISystemContext, NodeState) overrides to keep sample-contract NodeIds stable (browse-name / dotted-path identifiers instead of default hashed ids).
  • Updated DI Pump sample OpenUSD composition to stage authored subtrees on the fluent builder during ConfigureAsync rather than registering them immediately.
File summaries
File Description
samples/WotCon/FlatTagServer/SupervisionSignal.cs Derives supervision tag/alarm NodeId namespace + prefix from the parent supervision object to match staged creation.
samples/WotCon/FlatTagServer/FlatTagNodeManager.cs Migrates to FluentNodeManagerBase and fluent staging; overrides New to preserve dotted-path NodeIds and replaces manual externalReferences maintenance with CompleteConfigureAsync.
samples/Redundancy/RedundantServer/HaSampleNodeManager.cs Migrates to FluentNodeManagerBase and fluent staging; overrides New to preserve browse-name NodeIds used by the redundant client.
samples/DI/PumpDeviceIntegrationServer/PumpNodeManager.cs Threads the fluent builder into the async configure phase so OpenUSD composition can stage authored nodes instead of registering them immediately.
samples/DI/PumpDeviceIntegrationServer/OpenUsdComposition.cs Converts two materialization paths to stage nodes on the builder (sync) rather than calling AddPredefinedNodeAsync directly.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@romanett
romanett marked this pull request as ready for review September 10, 2026 07:22
Conflict in HaSampleNodeManager.CreateAddressSpaceAsync, where #4445 added
AddFactoryAssignedNodes next to the HistoryEvents object this branch had
converted to the staged builder. Both are kept: the factory-assigned
subtree stays hand-built and unstaged on purpose, because it exists to
show what an identity minted straight through NodeIdFactory looks like
beside the sample's named ones, and it is attached after the folder has
been staged so those identifiers survive untouched.

The merge also exposed a defect in this branch's New override. Naming
every node after its browse name is too broad: a browse name is unique
only among its siblings, and the historian hangs an "HA Configuration"
object off each node it historizes, so the Counter's and the
HistoryEvents' one both landed on ns=2;s=HA Configuration and the strong
active/passive replicas refused to start. The override is now limited to
the sample folder and its direct children - the nodes the redundant
client spells out - and everything else keeps the default factory's
derived identifier.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@marcschier
marcschier enabled auto-merge (squash) September 10, 2026 10:52
@marcschier marcschier added the ready Ready to merge once CI Passes label Sep 10, 2026
@marcschier
marcschier merged commit a05a360 into master Sep 10, 2026
276 of 278 checks passed
@marcschier
marcschier deleted the romanett/opcua-dotnet-4429-6784ff branch September 11, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready Ready to merge once CI Passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt the fluent builder's node-creation surface in the remaining hand-rolled node managers

3 participants