Add node creation to the fluent node manager builder - #4428
Merged
Conversation
INodeManagerBuilder gains a creation surface alongside its lookup surface: AddFolder, AddObject, AddVariable<T> and AddMethod (string and QualifiedName overloads, parent by NodeId), Add<TState> for an already-constructed state, a factory-aware Add<TState> that resolves the parent first, AddRoot<TState>, and TryGetNode. Created nodes are staged on the builder and given final NodeIds through the manager's INodeIdFactory before the per-node builder is returned, so OnRead/OnWrite registrations — which key off NodeState.NodeId — stay valid once the node is registered. Registration happens in RegisterAuthoredNodesAsync, which the generated CreateAddressSpaceAsync now calls after the Configure partials and before CompleteConfigureAsync, so the reverse-reference pass mirrors references to externally owned nodes (typically the Objects folder) into externalReferences. The hosting FluentNodeManager and RuntimeNodeSetNodeManager do the same. Staging also makes the builder's own lookups see created nodes: browse paths, NodeId, TypeDefinitionId and DataType resolution all consult the staged graph before falling back to the manager's predefined nodes. The surface is available to every fluent host — there is no separate opt-in and no second builder interface. The generated typed builder forwards the new members like the rest of INodeManagerBuilder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Existing table-of-contents entries strip inline code markers from the link text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code coverage✅ Coverage gate passed.
Uncovered changed lines
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4428 +/- ##
==========================================
+ Coverage 80.88% 80.90% +0.02%
==========================================
Files 1984 1985 +1
Lines 277054 277466 +412
Branches 48084 48161 +77
==========================================
+ Hits 224088 224479 +391
- Misses 36437 36440 +3
- Partials 16529 16547 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The coverage bots flagged the new authoring file at roughly 60% patch coverage: the happy paths were tested but almost none of the validation was. Adds 19 tests covering the QualifiedName overloads, an explicit type definition, staged nodes appearing in TypeDefinitionId and DataType lookups, grandchild registration, the idempotent AddChildIfMissing and AddRoot paths, the null-argument guards, and every rejection: empty and namespace-0 browse names, a missing browse name, a non-instance node given a parent, a parentId contradicting an existing parent, a parent with no NodeId, a parent outside the graph, an unowned namespace, a collision with a predefined node, a NodeIdFactory that assigns nothing, and a builder not backed by an AsyncCustomNodeManager. Two branches turned out to be unreachable rather than untested, so they are gone instead: - `AuthoredRoots` was never read by anything. - `PrepareNodeIds` called the NodeId factory a second time when the id was still null, but `PrepareAuthoredNodeIdsForRegistration` has just run the same factory over the same node, so the retry could only repeat the result. The guard that reports a factory which assigns nothing stays, and is now tested. Line coverage on NodeManagerBuilder.Authoring.cs is 92.2%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three managers already derived from FluentNodeManagerBase and hand-rolled what the new Add* surface does. Each now stages its root through the builder instead, which drops the manual NodeId/reference bookkeeping and routes the Objects-folder edge through CompleteConfigureAsync like any other configure-created node. - FluentNodeManagerFactory: the optional root folder is staged before the build delegate runs, so the delegate can reach it by NodeId or browse path. Its explicit string NodeId is preserved, since that is the address clients browse; the builder supplies the inverse Organizes reference that CreateRootFolder used to add itself. - Isa95NodeManager: CreateRoot no longer takes externalReferences and no longer maintains the ObjectsFolder entry by hand. The root is staged once its JobControl endpoints are attached, so the subtree registers together, and the manager now runs the reverse-reference pass it previously skipped. - SiteNodeManager (sample): the site folder, its areas and their SourceServer properties are staged in one call. Staging assigns the child NodeIds through the manager's own New(...) factory, replacing the explicit AssignInstanceChildNodeIds call, and LinkAreasToObjectsFolder is gone entirely. The DI device builders were considered and rejected: DiNodeManager hands out one long-lived, never-sealed builder and creates devices at runtime through public APIs, whereas staging is a startup-time mechanism that refuses Add* once the graph is registered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a broad new public API surface and alters node-manager address-space construction/registration ordering across multiple hosts, requiring careful human review for compatibility and behavioral edge cases.
Pull request overview
This PR extends the fluent node manager builder (INodeManagerBuilder) with a staged node-creation API (“Add* surface”), enabling Configure partials to create and wire up new nodes without requiring a NodeSet or ModelDesign. It integrates registration of staged nodes into the fluent address-space build pipeline so reverse-reference mirroring includes newly authored nodes, and adds tests + documentation for the new behavior.
Changes:
- Add staged node-creation members to
INodeManagerBuilderand implement them inNodeManagerBuilder.Authoring.cs(folders/objects/variables/methods, generic add/root, factory overload, staged lookups). - Introduce
RegisterAuthoredNodesAsync(...)into fluent build flows (generated templates, hosting factory, runtime nodeset manager) to register staged nodes before reverse-reference processing. - Add comprehensive NUnit coverage and documentation updates describing the new authoring surface and its lifecycle/ordering.
File summaries
| File | Description |
|---|---|
| tools/Opc.Ua.SourceGeneration.Core/Generators/NodeManagerTemplates.cs | Emit authored-node registration in generated CreateAddressSpaceAsync before reverse-reference pass. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/FluentBuilderGenerator.cs | Forward new INodeManagerBuilder creation members through the typed builder generator. |
| src/Opc.Ua.Server/Fluent/INodeManagerBuilder.cs | Define the new public creation and staged-lookup API surface. |
| src/Opc.Ua.Server/Fluent/NodeManagerBuilder.cs | Make builder partial and route browse-path root resolution through staged roots first; include authored nodes in type/data-type/node-id resolution. |
| src/Opc.Ua.Server/Fluent/NodeManagerBuilder.Authoring.cs | Implement staging, parent attachment, NodeId preparation, indexing, registration handoff, and Add* methods. |
| src/Opc.Ua.Server/Fluent/FluentNodeManagerBase.cs | Add protected helper to register authored nodes via the builder at the correct point in the pipeline. |
| src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs | Add subtree NodeId preparation pass for authored nodes to ensure stable NodeIds before returning builders. |
| src/Opc.Ua.Server/Hosting/FluentNodeManagerFactory.cs | Stage optional root folder via builder and register authored nodes before reverse-reference mirroring. |
| src/Opc.Ua.Server/RuntimeNodeSet/RuntimeNodeSetNodeManager.cs | Register authored nodes and rerun reverse-reference pass so external references include configure-created nodes. |
| src/Opc.Ua.ISA95.Server/Isa95NodeManager.cs | Switch root registration to staged builder flow and remove manual externalReferences bookkeeping. |
| samples/OpenUsd/SiteCompositionServer/SiteNodeManager.cs | Stage site topology subtree via builder and rely on reverse-reference pass for Objects-folder linkage. |
| tests/Opc.Ua.Server.Tests/Fluent/NodeManagerBuilderAuthoringTests.cs | Add test suite covering staging semantics, NodeId stability, parent resolution, factory behavior, rejection paths, and lookup visibility. |
| docs/NodeManagers.md | Document the new Add* authoring surface, staging/registration ordering, and usage pattern. |
Review details
- Files reviewed: 13/13 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.
PrepareAuthoredNodeIdsForRegistration only reassigned when a node's id was null, or when a descendant sat in namespace 0 under a non-zero root. A subtree materialised from a type model with NodeState.Create(..., assignNodeIds: false) matches neither: Create forwards to CreateInternal, which skips AssignNodeIds entirely, so the children keep their declaration ids — non-null, and in the model's own namespace rather than ns 0. Staging such a subtree therefore hit the declaration nodes already in PredefinedNodes and threw BadNodeIdExists from Add, rejecting a subtree that AddPredefinedNodeAsync accepts: its own PrepareInstanceNodeIdsForRegistration repairs exactly this case through HasDeclarationNodeIdCollision. Where the declarations are not indexed in the manager, the divergence was quieter but worse — the ids would have been rebased later at registration, after the builder had already handed them back for callback wiring. Adding the same collision check to the authored pass makes the two agree and keeps the ids final when the per-node builder returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 tasks
marcschier
approved these changes
Sep 7, 2026
romanett
added a commit
that referenced
this pull request
Sep 7, 2026
Master added node creation to the fluent builder (#4428), which stages authored nodes for the same lookups this branch stages imported nodes for. - NodeId lookups consult the import batch first, then the authored nodes, then the manager's predefined nodes. - The type- and DataType-keyed lookups now go through master's CollectAuthoredCandidates, extended to include the imported nodes and to drop a candidate an import is about to displace. - RuntimeNodeSetNodeManager registers the configuration's authored nodes first and then completes the import batch through CompleteConfigureAsync, matching the order the generated manager uses. Imported children can therefore attach to nodes the same Configure pass created. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
INodeManagerBuildergains a node creation surface alongside its existing lookup surface, so aConfigurepartial can fill a namespace without a NodeSet or a ModelDesign.Twelve new members, implemented as ordinary public members on
NodeManagerBuilder(new partial fileNodeManagerBuilder.Authoring.cs):AddFolder(name, parentId)FolderState(Organizes)AddObject(name, parentId, typeDefinitionId)BaseObjectStateAddVariable<TValue>(name, parentId)BaseDataVariableStatewhoseDataType/ValueRankcome fromTValueAddMethod(name, parentId)MethodStateAdd<TState>(node, parentId)NodeStatesubclassAdd<TState>(factory, parentId)AddRoot<TState>(node)TryGetNode(nodeId, out node)Each
Add*for a browse name takes astring(qualified with the manager's default namespace) or aQualifiedNamecarrying an explicit nonzero namespace index — eight overloads plus the four above.Three properties make this usable straight from
Configure:Add*runs the node — and its whole subtree — through the manager'sINodeIdFactorybefore returning, soOnRead/OnWriteregistrations, which key offNodeState.NodeId, stay valid once the node is registered.RegisterAuthoredNodesAsync. That is what lets a node name a sibling created moments earlier, and it keeps the builder usable before the manager has finished building its address space.Add<TState>returnsINodeBuilder<TState>, so a hand-writtenNodeStatesubclass keeps its type through the fluent chain.Staging also makes the builder's own lookups see created nodes: browse paths,
NodeId,TypeDefinitionIdandDataTyperesolution all consult the staged graph before falling back to the manager's predefined nodes.Registration ordering
A new
protected FluentNodeManagerBase.RegisterAuthoredNodesAsync(builder, ct)hands the staged roots toAddPredefinedNodeAsync. It runs after theConfiguredelegates and beforeCompleteConfigureAsync, so the reverse-reference pass sees the new nodes and mirrors their references to externally owned nodes (typically the ns=0Objectsfolder) intoexternalReferences. Three call sites emit it in that position:CreateAddressSpaceAsync(NodeManagerTemplates)FluentNodeManager(FluentNodeManagerFactory)RuntimeNodeSetNodeManager, which additionally re-runsAddReverseReferencesAsyncbecause its first pass happens beforeConfigureA builder that created nothing registers nothing, so the call is safe to make unconditionally.
The surface is available to every fluent host — there is no opt-in gate and no second builder interface. The generated typed builder (
FluentBuilderGenerator) forwards the new members like the rest ofINodeManagerBuilder.Supporting change
AsyncCustomNodeManager.PrepareAuthoredNodeIdsForRegistration(internal) assigns an id to every node in a subtree that still lacks one and pulls namespace-0 children into the root's namespace. It differs from the existingPrepareInstanceNodeIdsForRegistration, which only rebases a subtree that collides with a declaration. It is idempotent.Related Issues
Notes for reviewers
Three judgement calls worth a look:
INodeSource-style concept here to withhold it from, so an always-on capability is the same behaviour with one fewer state flag. Misuse is still rejected:Add*afterSeal()or after registration throwsBadInvalidState, a NodeId in a namespace the manager does not own throwsBadNodeIdInvalid, and a parent in one of the manager's own namespaces that was never created throwsBadNodeIdUnknown.AddChild/RemoveChild. The factory overload hands the factory an identity-only proxy when the parent belongs to another node manager, then has to drop the parent link the factory established.BaseInstanceState.Parenthas aninternalsetter scoped toOpc.Ua.Types, so this pair is the only public route. Commented as such at the call site. An alternative would be widening that setter or addingInternalsVisibleTo— happy to switch if preferred.PrepareNodeIdsdoes not re-walk per descendant. The root pass already covers the subtree, so per-child calls would be no-ops on a quadratic walk; descendants are only namespace-validated.One inherited behaviour the new tests pinned down:
BaseObjectState(parent)setsReferenceTypeId = HasComponentin its own constructor, so a node built via the factory overload lands under theObjectsfolder withHasComponent, while the plainAddObjectpath picksOrganizes. That asymmetry predates this PR; flagging it in case it is worth normalizing separately.Import(UANodeSet)is deliberately not part of this change — it belongs to NodeSet import rather than node creation.Test results
19 new tests in
tests/Opc.Ua.Server.Tests/Fluent/NodeManagerBuilderAuthoringTests.cscover NodeId assignment before return, parent-by-NodeId nesting, the default Objects-folder placement and its inverse reference, custom state types, both factory paths (authored parent and external-parent proxy),AddRootreference preservation,TryGetNode, id-keyed read handlers firing on the registered instance, browse-path/NodeId visibility of created nodes, and every rejection path listed above.Run against net10.0 (
-p:CustomTestTarget=net10.0):Opc.Ua.Server.TestsOpc.Ua.SourceGeneration.Core.TestsOpc.Ua.SourceGeneration.TestsUA.slnxbuilds with 0 errors and no new warnings.Checklist
Creating nodes from scratch — the Add* surfacesection indocs/NodeManagers.md, plus TOC entry.)🤖 Generated with Claude Code