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
marcschier
pushed a commit
that referenced
this pull request
Sep 9, 2026
…e manager (#4434) # Description Builds on #4428, which is now merged; this branch is rebased onto master. The GDS `ApplicationsNodeManager` becomes a source-generated `[NodeManager]`: the companion model, its loader and the fluent plumbing are emitted, and what stays hand-written is the behaviour. Getting there needed three things from the shared surface, which are the first two commits. ## Commits **1. Add the fluent hooks a hand-written manager needs at startup** - `OnReadRolePermissions` / `OnReadUserRolePermissions` on the node builder. Every other `NodeState` `On*` hook was already fluent; these were not, and a server that grants access on something the static model cannot express has to compute the permission set per request. - `Node<TState>(TState)` returns a builder over a node the caller already holds, for nodes materialised after `Configure` resolved its graph. - `OnAddressSpaceReadyAsync` on `FluentNodeManagerBase` — the one seam between the address space existing and the wiring being applied, for managers whose wiring depends on asynchronous setup. `Configure` is a `partial void` and cannot await. `DiNodeManager` had already invented the hook privately, so its declaration becomes an `override`. **2. Let a `[NodeManager]` bind to a model a referenced assembly supplies** A model belongs to the assembly that emits its types, which leaves a node manager for it homeless whenever that assembly cannot reference `Opc.Ua.Server` — a model-only package shared with clients, for instance. Binding was rejected twice over: the design was skipped before the binding was matched (`MODELGEN010`), and fluent-accessors-only mode refused to run alongside manager generation (`MODELGEN014`). The binding is now matched before the skip decision, and a matched binding puts that design into accessors-only mode. A design nothing binds to is still skipped, so this cannot start duplicating types by accident. The emitted manager also gains what a real manager needs: `DefaultNamespaceUris()` plus a protected constructor taking a replacement set; `GenerateDefaultConstructor=false` to suppress the two-argument form; its own logger category; and the call to `OnAddressSpaceReadyAsync`. Generated fluent accessors also stop re-enabling nullable warnings — a bare `#nullable enable` undid the shared header's `disable warnings`, so any model with a structure-typed method argument reported CS8600 on every generated `out T` in a project that builds warnings as errors. **3. Reimplement the GDS `ApplicationsNodeManager` as a generated node manager** The design stays owned by `Opc.Ua.Gds.Common`; the server assembly only binds a manager to it. **NodeIds do not move.** `NamespaceIndexes[0]` must stay the application record namespace — it is the one the application database, the certificate request store and the base allocator mint ids in — so the constructor passes the namespace order explicitly rather than adopting the generated model-first default. A test pins both ends against a live server. Four overrides go, leaving `OnAddressSpaceReadyAsync` and `Dispose(bool)` — no address-space overrides at all: | Removed | Why it was unnecessary | | --- | --- | | `New` | The base allocator already mints into `NamespaceIndexes[0]`, with a thread-safe counter. The override existed only because `Create(assignNodeIds: true)` asks the factory with the declaration id still set; `AssignInstanceNodeId` nulls it and re-asks, documented as being there for exactly this allocator. | | `AddBehaviourToPredefinedNodeAsync` | Its `AuthorizationServiceType` branch re-wired a node that was already wired, and its `KeyCredentialServiceType` branch had nothing to wire — the model ships that folder empty. | | `GetManagerHandleAsync`, `ValidateNodeAsync` | Verbatim reimplementations of the base that also blocked the fluent base's virtual-node support. | | `DeleteAddressSpaceAsync` | A `// TBD` passthrough. | Everything else — all sixteen directory methods with their self-administration permissions, the certificate groups' concrete types and trust-list flags, and the authorization service (created through this PR's `Add` surface and wired in the same place) — is one `Configure` pass. **4. Bind the certificate groups from the `Configure` pass** Acquiring a certificate group and binding it to its nodes are different kinds of work: the first is I/O (`InitAsync` opens certificate stores and creates CA certificates), the second only touches the address space. Each now runs in the phase it belongs to, which puts the whole of the wiring in one place. Ownership no longer rides on the binding. A group joins `m_ownedCertificateGroups` the moment it is constructed, before `InitAsync` can throw, so a group that fails half way through is still disposed — which the previous code could not manage, keying ownership off a dictionary it filled only after initialization succeeded. > Commit 3's message says this move is impossible and blames `PushTest`. **That was a wrong diagnosis, and this commit supersedes it.** Two real defects were hiding behind those failures: - **`Create(..., NodeId.Null, ..., assignNodeIds: false)` does not null the NodeId.** `NodeState.Create` overrides it only when handed a non-null one, so a custom group node kept the namespace-0 id of its type declaration. `AssignInstanceNodeId` hid this by re-asking the factory with the id nulled; the builder validates instead, and rejected it. The node is now created with an id in the manager's own namespace, as the `Default` authorization service already was. - **The custom certificate group node was not browseable.** It answered `GetCertificateGroups` and `ReadNode`, but a type-filtered recursive browse from the Objects folder never returned it — which OPC 10000-12 §7.8.2 requires of a certificate group. Staging the node through the builder gives it the reference it was missing. ### Two `PushTest` methods change — please review these closely The second defect had been masking what those tests actually did. Selecting the group positionally, `groups[3]` resolved to the server's **own `ServerConfiguration` group** while the custom one stayed invisible, so `UpdateTrustListOfCustomGroupAsync` and `AddRemoveCertOfCustomGroupAsync` were exercising a *transactional* trust list under a custom-group name. Browse results, before and after: | # | Before (baseline) | After | | --- | --- | --- | | 0-2 | GDS Directory groups (`ns=3`) | GDS Directory groups (`ns=3`) | | 3 | **`DefaultApplicationGroup` `i=14156`** — the server's own | **`MyCustomGroup` `ns=2`** | | 4 | — | `DefaultApplicationGroup` `i=14156` | They now resolve the group by browse name — which asserts it is browseable at all — and expect the immediate-apply semantics the GDS actually gives the groups of its own `Directory`: `CloseAndUpdate` writes the stores at once and reports `applyChangesRequired == false`, so the `ApplyChanges` calls go away (they would return `Bad_NothingToDo`). The read-back assertions are what prove the writes landed, and they are unchanged. This is a genuine loss of the *transactional* trust-list coverage those two methods were accidentally providing — the server's own group is still covered by the other `PushTest` methods, which target it deliberately. **5. Select `GetCertificates` results by type instead of by position** `GetCertificatesAsync` checked entry zero of a parallel-array result. Which entry that is depends on the order the server reports its certificate types in, an empty result throws `IndexOutOfRange` rather than failing an assertion, and a second type going missing or unparseable would not be noticed. Every returned pair is now checked, and a failure names the certificate type. **6. Drop the `.Common` suffix from the three GDS projects** Every other companion specification ships as `<Family>` / `<Family>.Client` / `<Family>.Server` — Di, ISA95, Positioning, Robotics, Vision, WotCon, XRegistry. GDS was the exception, with a `.Common` suffix that named nothing. | Before | After | | --- | --- | | `src/Opc.Ua.Gds.Common` | `src/Opc.Ua.Gds` | | `src/Opc.Ua.Gds.Client.Common` | `src/Opc.Ua.Gds.Client` | | `src/Opc.Ua.Gds.Server.Common` | `src/Opc.Ua.Gds.Server` | **No source changes were needed.** `RootNamespace` was already the final name in all three projects — the namespaces have always been `Opc.Ua.Gds`, `Opc.Ua.Gds.Client` and `Opc.Ua.Gds.Server`, and only the assembly and package ids disagreed. Nothing compiling against these assemblies changes a `using`. The package ids do change, which is consumer-visible, so `docs/migrate/2.0.x/packages.md` gains a renamed-packages section — it is the document that undertakes to cover NuGet renames — and the migration skill's package table gains the two rows a 1.5.378 consumer needs. Build infrastructure that names the projects or their output moves with them: both solution files, `expected-packages.txt` (re-sorted — it is maintained alphabetically), both signing manifests, the reference-server Dockerfile, the metapackage nuspecs, and the analyzer's package dependencies. `preview-pack.slnx` is derived from `UA.slnx` and needs no edit. The migration analyzer's shim tree mirrors the source layout by its own documented convention, so `Gds.Client.Common/` becomes `Gds.Client/`. Prose naming past work is left alone — plan 23 still records that its deferral happened during the "GDS Client.Common modernization", which is the name that effort had. ## Validation `net10.0`, on master + these four commits, from a clean GDS PKI: - `UA.slnx`: 0 errors, 0 warnings in changed files (the solution's 30 pre-existing warning lines are CA1861/CA1850/CA1307 in `XRegistry`, `Types.Tests` and `WotCon.Tests` — same rules and projects as the base). - `Opc.Ua.Gds.Tests`: 1,117 passed, 0 failed, 54 skipped — identical to the base branch. - `Opc.Ua.Server.Tests`: 4,961 passed, 0 failed, 5 skipped (measured before commit 4, which touches only the GDS assembly and its tests). - `Opc.Ua.SourceGeneration.Tests`: 169 passed. - `Opc.Ua.SourceGeneration.Core.Tests`, NodeManager/Generators/Fluent: 84 passed; `GenerateCode`: 21 passed, 4 skipped. - `Opc.Ua.MigrationAnalyzer.Core.Tests`: 10 passed, 3 skipped. - After the rename: assemblies build as `Opc.Ua.Gds.dll` / `.Client.dll` / `.Server.dll`, with no `*.Common.dll` produced anywhere. - Commits 1-3 build standalone. - `Opc.Ua.Gds.Server.Common` also builds for `net48` and `netstandard2.1` (checked before the rebase onto #4428). Not yet run: the full `Opc.Ua.SourceGeneration.Core.Tests` suite (~3,800), and the older target frameworks since the rebase. ## Checklist - [x] Existing tests pass - [x] New tests cover the new surface (role permissions, resolved-node builder, referenced-model binding, constructor/namespace control, GDS namespace invariant, custom-group node namespace) - [ ] Full Core.Tests sweep and net48/netstandard2.1 re-run after the rebase 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
marcschier
pushed a commit
that referenced
this pull request
Sep 10, 2026
…d-rolled node managers (#4460) 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 `OnRemoveDataSetFolder` → `RebuildConfigurationAddressSpaceAsync`, `OnAddPushTarget` / `OnRemovePushTarget` → `RebuildKeyPushTargetAddressSpaceAsync`, `OnGetSecurityGroup` / `OnAddSecurityGroup` / `OnRemoveSecurityGroup` → `RebuildSecurityGroupAddressSpaceAsync` — 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](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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