Reimplement the GDS ApplicationsNodeManager as a generated fluent node manager - #4434
Merged
marcschier merged 17 commits intoSep 9, 2026
Merged
Conversation
marcschier
approved these changes
Sep 7, 2026
Contributor
Author
|
This needs the NodeID Factory work, to select the right namespace without adding the custom constructor logic |
Three gaps the GDS conversion ran into, each usable on its own: - 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 it. DiNodeManager had already invented the hook privately, so its declaration becomes an override. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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: the manager and its fluent surface are emitted locally while the model types keep coming from the reference. A design nothing binds to is still skipped, so this cannot start duplicating types by accident. The project-wide ModelSourceGeneratorGenerateNodeManager switch stays incompatible with accessors-only mode: it would emit conventionally named managers into the referenced model's own C# namespace for every design. Its message now names the attribute as the supported route. The emitted manager also gains what a real manager needs: - DefaultNamespaceUris() plus a protected constructor taking a replacement set, so a manager can take collaborators the generated signature does not carry, or change which namespaces it owns and in what order. Entry 0 becomes the manager's NamespaceIndex and is baked into every NodeId it mints, so the order is part of its contract. - GenerateDefaultConstructor=false to suppress the two-argument form, so a manager with dependencies cannot be built half-initialized. - Its own logger category rather than the shared base class one. - A call to OnAddressSpaceReadyAsync between loading the model and Configure. 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The companion model, its loader and the fluent plumbing now come from a [NodeManager] attribute. The design stays owned by Opc.Ua.Gds.Common, which emits the model types; this assembly only binds a manager to it. What is written by hand is the behaviour. Node ids 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 class's 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: - New: the base allocator already mints into NamespaceIndexes[0], with a thread-safe counter. The override only existed because Create(assignNodeIds: true) asks the factory with the declaration id still set; AssignInstanceNodeId nulls it and re-asks, which is documented as being there for exactly this allocator. - AddBehaviourToPredefinedNodeAsync: its AuthorizationServiceType branch re-wired a node CreateDefaultAuthorizationService had already wired, and its KeyCredentialServiceType branch had nothing to wire — the model ships that folder empty. Host-contributed services now call ConfigureAuthorizationService / ConfigureKeyCredentialService. - GetManagerHandleAsync and ValidateNodeAsync: verbatim reimplementations of the base that also blocked the fluent base's virtual-node support. - DeleteAddressSpaceAsync: a TBD passthrough. What remains is OnAddressSpaceReadyAsync, the fluent async seam, and Dispose(bool). Everything else — all sixteen directory methods with their self-administration permissions, the certificate groups' concrete types and trust-list flags, and the authorization service — is one Configure pass. Certificate group acquisition stays in the async hook. Moving the group-to-node binding into Configure split a group's acquisition from its ownership: InitAsync opens certificate stores, so a server failing between the phases leaked them, and PushTest failed deterministically. The call site records this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 happens in the phase it belongs to, so OnAddressSpaceReadyAsync brings the authorities up and Configure binds them, and the whole of the wiring is finally 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. m_certificateGroups still indexes the groups by node id, filled during binding because that is the earliest point a group knows the id it is addressed by. An earlier attempt at this move read the resulting PushTest failures as proof that acquisition, binding and ownership cannot be separated. That was a guess, and it was wrong. Two real defects were behind it: - 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 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. That second defect is why two PushTests change here. Picking 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. 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
force-pushed
the
romanett/applications-node-manager-fluent-8c0aa7
branch
from
September 7, 2026 11:27
84b019e to
de97409
Compare
Code coverage✅ Coverage gate passed.
Uncovered changed lines
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
GetCertificates returns parallel arrays -- one certificate per certificate type the group is configured for (OPC 10000-12 7.8.4) -- and the test checked entry zero. Which entry that is depends on the order the server reports its types in, an empty result throws IndexOutOfRange instead of failing an assertion, and a second type going missing or unparseable would not be noticed at all. Every returned pair is now checked, and a failure names the certificate type rather than an index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
force-pushed
the
romanett/applications-node-manager-fluent-8c0aa7
branch
from
September 7, 2026 12:48
c05b7f9 to
f50c11b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4434 +/- ##
==========================================
+ Coverage 80.83% 80.88% +0.04%
==========================================
Files 2032 2034 +2
Lines 296559 296634 +75
Branches 51186 51176 -10
==========================================
+ Hits 239738 239920 +182
+ Misses 39048 38941 -107
Partials 17773 17773
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ns-node-manager-fluent-8c0aa7
…ns-node-manager-fluent-8c0aa7
3 of 7 tasks
Merging the .Common rename (#4438) left this project's AdditionalFiles entries naming ..\Opc.Ua.Gds.Common\Design, a directory the rename had just removed. Git could not see the conflict: those lines are additions from this branch, so there was nothing for it to mark, and the merge resolved cleanly onto a broken path. The design now resolves at ..\Opc.Ua.Gds\Design, and the three comments that still named the old project follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three changes landed on master that meet this branch. Two needed more than a textual resolution. NodeManagerGeneratorTests: #4432 rewrote the ordering assertions this branch had extended, and both sides declared idxConfigure. The chain now asserts base -> OnAddressSpaceReadyAsync -> Configure -> RegisterAuthoredNodes -> CompleteConfigure -> SealConfiguration, so master's new guarantees and this branch's async-seam guarantee both still hold. The generated manager now reaches for the model's NodeSet import factory provider (#4432). Normally NodeStateGenerator emits that beside the manager, but a manager bound to a model a referenced assembly supplies lives in a different assembly from the model -- and the model-only assembly cannot emit the provider at all, because it implements an Opc.Ua.Server contract such an assembly deliberately does not reference. The manager was left naming a type that could never exist. NodeStateGenerator gains ImportSupportOnly so the provider is emitted into the assembly that does reference Opc.Ua.Server, under the model's namespace, which keeps the import feature working for these managers rather than switching it off for them. #4432 also added a sealed node manager, and the namespace-taking constructor this branch emits then trips CS0628 -- an error under warnings as errors. The generator cannot see whether the user's partial is sealed, so the emitted file suppresses it: in a sealed manager that constructor is unreachable, not wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 of 7 tasks
marcschier
pushed a commit
that referenced
this pull request
Sep 8, 2026
…4442) # Description The fluent node-manager pipeline had two synchronous chokepoints, and two independent efforts have now hit both of them. `Configure` is a `partial void`, so it cannot `await`. A manager whose wiring depends on asynchronous setup had to invent a second hook outside the pipeline, create its own builder there, and hand-roll `CreateFluentBuilder(...).Configure(...).Seal()` inside it. `Seal()` was synchronous, so nothing a registration staged during `Configure` could be completed by awaiting. Registrations that needed to await either blocked — `EventSourceRegistry` did `.GetAwaiter().GetResult()` on the manager's root-notifier registration — or could not be expressed at all. ## What changed **`FluentNodeManagerBase.ConfigureAsync(INodeManagerBuilder, CancellationToken)`** is the awaitable wiring seam. It runs once per activation, with the manager's attached builder, immediately *before* the synchronous `Configure` partial(s) — so instances it materialises are in the address space by the time `Configure` wires callbacks against them. The default implementation is a no-op. It is a `virtual` method rather than a second `partial` declaration because a partial method can be optional (`partial void`, no return value) or awaitable (an extended partial method, which must be implemented) — not both, and the generator would otherwise have to detect which one the user declared. `partial void Configure(INodeManagerBuilder)` keeps working unchanged; a manager uses either hook or both. **`NodeManagerBuilder.Seal()` becomes `SealAsync(CancellationToken)`.** Sealing is now the single point where registrations that could not finish inside `Configure` get their turn to await. Every seal site awaits it, so a later change can move registry *setup* onto the same seam. **`DiNodeManager` now runs the standard pipeline itself** — create and attach the builder, `await ConfigureAsync`, `RegisterAuthoredNodesAsync`, `CompleteConfigureAsync`, the post-setup runner, then `await builder.SealAsync` — instead of handing subclasses a bare callback and leaving them to build and seal on their own. DI subclasses gain node registration and the reverse-reference pass, which they previously skipped. Sealing deliberately comes **after** the post-setup runner. The fluent registries are owned by the manager, not by a single builder, and sealing starts them — so sealing first locks post-setup configurators out of registering simulation loops of their own, which is exactly what `AddRobotics`' configurators do. Sealing before the runner makes `MinimalRobotServer` fail to start with *"Cannot add a simulation loop after the registry has started"*. Worth knowing for follow-up work: with several builders attached to one manager, the first seal freezes the shared registry for all of them. ## Workarounds removed - **`DiNodeManager.OnAddressSpaceReadyAsync`** — deleted. Its three overriders (`RoboticsNodeManager`, and the `PumpNodeManager` / `GeneratorNodeManager` samples) now override `ConfigureAsync` and no longer hand-roll `CreateFluentBuilder(...).Configure(Configure).Seal()`. - **`EventSourceRegistry`'s `.GetAwaiter().GetResult()`** on `AddRootNotifierAsync` — gone. `Publish(..., RegisterAsRootNotifier: true)` now stages the registration and `SealAsync` drains it, so the wait happens on an awaited path. A source whose root-notifier registration fails is rolled out of the registry before the failure surfaces, and a registry disposed before its builder sealed drops what it staged. - **`Isa95NodeManager.ConfigureStatusEvents()`** — the synchronous second builder pass is now `ConfigureStatusEventsAsync(CancellationToken)`, awaited from `CreateAddressSpaceAsync`. (`ConfigureCatalogChanges()` stays synchronous: it starts a background task and never seals a builder, so there was nothing to convert.) - **`IRoboticsBuildContext.Seal()`** — now `SealAsync(CancellationToken)`. These really were `NodeManagerBuilder.Seal()` calls behind a same-named method. The state transition still happens under the context lock; the builder's asynchronous completion runs outside it, so no `await` is taken while the lock is held. - The Pump / Generator incremental registration callbacks (`onRegistered`) changed from `Action<T>` to `Func<T, CancellationToken, ValueTask>` so their per-instance builder pass can await its own seal. No sync-over-async was introduced anywhere. `Dispose(bool)` is unchanged and stays non-blocking; teardown remains in `DeleteAddressSpaceAsync`. `NodeManagerBuilder.Seal()` and `IRoboticsBuildContext.Seal()` are removed rather than kept as synchronous overloads. A synchronous seal that silently skipped the asynchronous completion would reintroduce exactly the failure mode this change exists to remove. ## Generator `NodeManagerTemplates` emits, in order: predefined nodes load → `await ConfigureAsync(__m_builder, cancellationToken)` → `Configure(__m_builder)` and the typed `Configure` → `RegisterAuthoredNodesAsync` → `CompleteConfigureAsync` → `await SealConfigurationAsync(__m_builder, cancellationToken)`. `NodeManagerGeneratorTests` asserts that order literally, and that neither `__m_builder.Seal()` nor the synchronous `SealConfiguration(__m_builder)` is emitted. ### Merging with #4432 #4432 landed on `master` while this was in review and reworked the same seam from the other side: it split `Seal()` into `SealGraphAuthoring()` + `StartSimulations()` so a manager can replay `NotifyNodeAdded` *between* the two halves — sealing first stops a lifecycle handler authoring nodes nothing would register, starting the simulations last stops a simulated value change preceding its own node''s `OnNodeAdded` — and added `FluentNodeManagerBase.SealConfiguration(builder)` as the one call the generator emits. Both intents are kept. That split is precisely why the asynchronous completion could not simply live inside `SealAsync`: a manager reaching the seam through the split path would silently skip it, which is the failure mode this PR exists to remove. So the asynchronous half became its own step: - `NodeManagerBuilder.CompleteSealAsync(ct)` drains the staged root-notifier registrations, then starts the simulations. `SealAsync(ct)` is `SealGraphAuthoring()` followed by it. - #4432''s synchronous `StartSimulations()` is gone, folded into `CompleteSealAsync`. It had exactly one production caller after the merge, and leaving it standing would have kept a second, synchronous way to activate a builder that skips the staged registrations — the very hazard this PR removes. Activation is now one step with one entry point; the *split* itself stays, because the replay has to sit between the halves. - `SealConfiguration` becomes `SealConfigurationAsync(builder, ct)`: seal the graph, replay `NotifyNodeAdded`, then `CompleteSealAsync`. Activation still happens last, so #4432''s ordering guarantee holds unchanged. ## Consumers unblocked - **#4434 ("Reimplement the GDS ApplicationsNodeManager as a generated fluent node manager")** adds `OnAddressSpaceReadyAsync` to `FluentNodeManagerBase` purely because, in its own words, *"`Configure` is a `partial void` and cannot await"*. That hook is unnecessary once `ConfigureAsync` exists — the GDS manager overrides `ConfigureAsync` instead, and the ordering it wants (predefined nodes → async setup → `Configure`) is what the generator now emits. #4434 has not merged yet, so it can drop its addition rather than have it simplified afterwards. - **Branch `romanett/fluent-node-behaviors`** moved `SimulationRegistry`, `EventSourceRegistry`, `MonitoredSourceRegistry` and the alarm wiring onto `AttachToType`/`Attach` for *release* only, because activation ran from `CompleteConfigureAsync` and most managers only called the synchronous `Seal()`. Every seal site now reaches `SealAsync`, so registry and alarm *setup* can move onto behavior activation too — the remaining half of that work. ## Tests - New `DiNodeManagerConfigureAsyncTests` drives the real `DiNodeManager.CreateAddressSpaceAsync` and asserts the hook runs once with an attached builder, that nodes it stages are registered, that their references to externally owned nodes are published, and that the builder is sealed before `CreateAddressSpaceAsync` returns. - `PublishTests` now pins the root-notifier deferral contract: registration is staged during `Configure`, lands only on the seal, and draining twice does not double-register. - Every `Seal()` call site in the test suites became `await ...SealAsync()`, including the three #4432 added in `NodeSetImportBuilderTests` and `SimulationBuilderExtensionsTests`. ## Related Issues No tracking issue was opened for this; it is the shared prerequisite extracted from #4434 and branch `romanett/fluent-node-behaviors`, both of which had independently worked around the same synchrony. Happy to open one if maintainers want the ADR trail. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### What was actually run locally `UA.slnx` builds clean. Post-merge on **net10.0**: `Opc.Ua.Server.Tests` 5020/0 and `Opc.Ua.SourceGeneration.Core.Tests` 3813/0. Pre-merge the same run plus `Opc.Ua.Di.Tests` 425/0, `Opc.Ua.Robotics.Tests` 627/0, `Opc.Ua.ISA95.Tests` 136/0, `Opc.Ua.Positioning.Tests` and `Opc.Ua.Robotics.Intent.Tests` were all green; `MinimalRobotServer` was also started directly to confirm the hosted robotics path comes up. Three caveats, so the unchecked box above is not a mystery: the full suite was **not** run against .NET Framework locally; `Opc.Ua.OpenUsd.Tests` has one pre-existing failure on this machine (`StructuredCartesianCoordinatesAreAccepted` expects `"1.5"` and gets `"1,5"` — a German-locale decimal separator, unrelated to this change); and one pre-existing `CA1861` warning comes from `NodeSetImportIntegrationTests.cs`, which is byte-identical to `master` on this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#4442 adds ConfigureAsync(INodeManagerBuilder, CancellationToken) to FluentNodeManagerBase -- the awaitable wiring seam this branch had introduced as OnAddressSpaceReadyAsync, but with the builder handed over as well, and reasoned from the same constraint: a partial method can be optional or awaitable, not both. DiNodeManager, which had invented the hook privately and which this branch had turned into an override, has already moved onto it upstream. Shipping both would leave two overlapping async seams on one base class, so OnAddressSpaceReadyAsync goes: from the base class, the generated CreateAddressSpace sequence, the generator's ordering test, the GDS manager (now an override of ConfigureAsync, which ignores the builder because its work is I/O rather than address space), and docs, where master's account is fuller than the section this branch added. #4442 also replaces Seal() with SealAsync(cancellationToken). ConfigureKeyCredentialService is host-facing and had to follow, so it becomes ConfigureKeyCredentialServiceAsync; its caller and cref move with it, along with one test and a doc example. The net effect is a smaller branch: it no longer adds an API the stack now provides. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ConfigureAsync can await and carries the builder, so the two-pass shape this manager had is no longer forced by anything. The partial void Configure -> OnConfigure indirection existed only because a partial method cannot be virtual and cannot await; both hooks collapse into the ConfigureAsync override, which was already protected virtual. Subclasses override that instead, the same seam the rest of the stack uses. Acquiring a certificate group and binding it to its nodes were split across the two passes, so the groups were walked twice with m_ownedCertificateGroups carrying the hand-off. They are now acquired and bound in one step per group, which is what gives m_certificateGroups its key before anything looks a group up, and leaves ConfigureCertificateGroups doing only what its name says. The 60-line certificate type dictionary moves out of the startup path into CreateCertificateTypeMap. An earlier attempt at joining those phases failed and was read as proof that they could not be joined. That was wrong twice over: the failure was the namespace-0 declaration id, and the phases were only ever separated by the sync/async boundary that has now gone. Also fixes the fluent example in StateMachines.md, which had `protected override void OnConfigure(INodeManagerBuilder)` on a FluentNodeManagerBase subclass. No base class has ever declared OnConfigure, so that example did not compile; it now shows the [NodeManager] partial with `partial void Configure`, as NodeManagers.md and HistoricalAccess.md do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#4433 routes runtime NodeId minting through DefaultNodeIdFactory and drops SystemContext.NodeIdFactory = this from generated managers, so a manager now selects a mode instead of overriding New. It also edited the ApplicationsNodeManager this branch has rewritten, so git produced four conflict regions. Three are this branch's rewrite against nothing, or against the very overrides it removed -- DeleteAddressSpaceAsync and GetManagerHandleAsync, kept out rather than resurrected by a reflexive "keep both sides". The fourth carries real intent and is taken: the GDS mints Counter identifiers. Applications, certificate groups and trust lists are registered and unregistered under repeating names, and Counter is the only mode that stays unique when a browse path repeats; every other mode derives the identifier from the path and would collide across a remove and re-add. Master's companion NamespaceUris assignment is not taken. That is how the hand-written manager declared its namespaces; the generated one carries the same order through its constructor chain already. The namespace half of the NodeId contract still holds: NodeIdFactory rebases an assigned factory onto the manager's own namespace, so ids still land in the application record namespace, which the invariant test pins against a live server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The manager reordered its own namespaces so that NamespaceIndexes[0] was the application-record one, because that is where the old CustomNodeManager2.New() minted. #4433 moved the minting namespace onto the factory, so the reordering and the two accessors that worked around it can go: - the constructor takes the generated namespace order (the companion model first) and points the factory at the application-record namespace instead, as DiNodeManager already does for its instance namespace; - GdsNamespaceIndex is gone, since the model namespace is the manager's own again; ApplicationsNamespaceIndex names the other one and is resolved through the namespace table rather than a fixed position; - the custom certificate group root goes in with a null NodeId again. It only carried a hand-built one because PrepareAuthoredNodeIdsForRegistration skipped the root, which #4433 fixed with HasStandardDeclarationNodeId. The two GDS node manager factories now advertise ApplicationsNodeManager.DefaultNamespaceUris() so their namespace list cannot drift from the manager's. The two namespaces swap index in the server's namespace table. Nothing in the stack keys off the numbers - the assertions resolve URIs and browse names - but a client that cached indexes across an upgrade sees it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The audit that prompted this found nothing dead - every fluent and generator addition on this branch still has a live consumer - but three places still told readers to reorder a manager's namespaces so that NamespaceIndexes[0] is where its ids are minted. #4433 moved that decision onto the NodeId factory, and this branch's own manager stopped doing it, so the guidance now teaches a workaround that its only example no longer follows. DefaultNamespaceUris() is emitted into every generated manager, so its summary is the widest-read of the three: it now says the order picks the namespace an unqualified browse path resolves in, and says the factory owns the minting namespace. The attribute's remarks and the NodeManagers.md section carry the same correction, the latter with a worked NodeIdFactory.WithDefaultNamespaceIndex call in place of the reordered array and a pointer to NodeIdAssignment.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
docs/NodeManagers.md shows builder.Seal() even though the API is SealAsync, which is misleading and should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR advances the move to source-generated fluent node managers by extending the generator/binding surface (including referenced-model bindings and constructor control) and then applying it to reimplement the GDS ApplicationsNodeManager with behavior remaining hand-written but wiring shifting to a generated fluent pipeline.
Changes:
- Extend node-manager source generation to support referenced-model bindings, accessors-only composition, and optional emission of the public default constructor.
- Add fluent wiring entry points for role-permissions read hooks and for wrapping already-materialized
NodeStateinstances. - Rework GDS tests and fixtures to align with the new GDS node-manager wiring and more robust certificate-group/certificate assertions.
File summaries
| File | Description |
|---|---|
| tools/Opc.Ua.SourceGeneration/NodeManagerAttributeDiscovery.cs | Adds attribute binding support for GenerateDefaultConstructor and refactors bool-flag parsing. |
| tools/Opc.Ua.SourceGeneration/ModelCompilation.cs | Improves diagnostics guidance around accessors-only + node-manager generation. |
| tools/Opc.Ua.SourceGeneration.Core/Templating/Tokens.cs | Adds token for conditional default-constructor emission. |
| tools/Opc.Ua.SourceGeneration.Core/NodeManagerAttributeBinding.cs | Introduces GenerateDefaultConstructor binding option. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs | Adds “import-support-only” emission mode for referenced-model scenarios. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/NodeManagerTemplates.cs | Updates generated manager template: namespace-set API, protected constructor, logger category, conditional default constructor. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/NodeManagerGenerator.cs | Plumbs EmitDefaultConstructor into node-manager generation. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/FluentBuilderTemplates.cs | Adjusts nullable directive in generated fluent builders to avoid re-enabling warnings. |
| tools/Opc.Ua.SourceGeneration.Core/Generators.cs | Enables binding a manager to a referenced model by switching to accessors-only emission for that design. |
| tools/Opc.Ua.SourceGeneration.Core/DesignFile.cs | Adds per-design option to suppress the generated public default constructor. |
| tests/Opc.Ua.SourceGeneration.Tests/ModelGeneratorTests.cs | Updates assertions for new constructor/namespace plumbing and adds coverage for suppressing default constructor. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeManagerGeneratorTests.cs | Adds tests for new namespace-set API, logging category, and default-constructor suppression. |
| tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeManagerBindingToReferencedModelTests.cs | New tests covering node-manager binding to models provided by referenced assemblies. |
| tests/Opc.Ua.Server.Tests/Fluent/RolePermissionBuilderExtensionsTests.cs | New tests for fluent role-permission read-hook wiring. |
| tests/Opc.Ua.Server.Tests/Fluent/ResolvedNodeBuilderExtensionsTests.cs | New tests for fluent wiring over already-resolved nodes (Node(TState node)). |
| tests/Opc.Ua.Gds.Tests/PushTest.cs | Fixes custom-group selection by browse name, removes invalid ApplyChanges calls for non-transactional groups, improves certificate assertions. |
| tests/Opc.Ua.Gds.Tests/CustomCertificateGroupIntegrationTest.cs | Pins custom certificate group node namespace invariant. |
| tests/Opc.Ua.Gds.Tests/AuthorizationService/RefreshTokenTests.cs | Uses manager-provided namespace URI list to avoid drift. |
| tests/Opc.Ua.Gds.Tests/ApplicationsNodeManagerKeyCredentialTests.cs | Updates namespace-index usage and key-credential service wiring flow to match new manager behavior. |
| tests/Opc.Ua.Gds.Tests/ApplicationsNodeManagerCoverageTests.cs | Adds a test pinning the application vs model namespace mapping. |
| src/Opc.Ua.Server/Fluent/RolePermissionBuilderExtensions.cs | Adds fluent extensions for OnReadRolePermissions / OnReadUserRolePermissions. |
| src/Opc.Ua.Server/Fluent/ResolvedNodeBuilderExtensions.cs | Adds fluent Node<TState>(TState node) overload for runtime-materialized nodes. |
| src/Opc.Ua.Server/Fluent/NodeManagerAttribute.cs | Adds GenerateDefaultConstructor to the [NodeManager] attribute surface. |
| src/Opc.Ua.Gds.Server/Opc.Ua.Gds.Server.csproj | Enables model source-generation inputs for binding to the design from a referenced assembly. |
| src/Opc.Ua.Gds.Server/ApplicationsNodeManager.cs | Converts GDS ApplicationsNodeManager to a generated [NodeManager] + fluent wiring approach; behavior remains hand-written. |
| samples/Reference/ConsoleReferenceServer/GdsNodeManagerFactory.cs | Uses manager namespace list to avoid drift between factory and manager. |
| docs/StateMachines.md | Updates example to match generated node-manager pattern ([NodeManager] + partial void Configure). |
| docs/NodeManagers.md | Documents constructor/namespace override patterns, referenced-model binding, and new addressing/wiring hooks. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
marcschier
pushed a commit
that referenced
this pull request
Sep 9, 2026
…4441) # Description The fluent builder resolves exactly **one node per call**. `NodeFromTypeId` is exact-type-only and throws `BadBrowseNameDuplicated` the moment two instances match, and handlers land in single-occupant `NodeState` slots — a second handler for an occupied slot is an error by design. So the fluent surface cannot express four things: - **fan-out** over the instances of a type, - **subtype matching**, so a registration for `AnalogItemType` also covers a user's derived type, - **composition** of two behaviors on one node, - **teardown** for a behavior that *owns* something — a semaphore, a file handle, a store, a timer. Three hand-rolled registries already work around exactly this — `EventSourceRegistry` (730 lines), `MonitoredSourceRegistry` (998), `SimulationRegistry` (328) — each with its own register → activate → reconcile → deactivate → dispose loop and its own failure policy, written three times because the slot model had no lifecycle to hang them on. This PR adds that lifecycle and moves the registries onto it. ## The new surface Ten public members. A registration returns an `IAsyncDisposable` that the node manager owns: ```csharp builder.AttachToType<AlarmConditionState>( ObjectTypeIds.AlarmConditionType, async (alarm, ctx, ct) => { MySubscription sub = await Source.SubscribeAsync(alarm, ctx.Lifetime).ConfigureAwait(false); return sub; // released on teardown, in reverse order }); ``` - `AttachToType<TState>(NodeId, attach, NodeAttachOptions?)` — one behavior per matching instance, subtypes included by default. - `Attach<TState>(attach)` on a node builder — one behavior on an already-resolved node. - `Attach(attach)` on the manager builder — manager-scoped, owns no node. - `AcquireWhileMonitored<TValue>(acquire)` — acquire on the first subscriber, release on the last **and at shutdown**. - `INodeAttachContext`, `NodeAttachOptions`, and `protected ActivateNodeBehaviorsAsync` on `FluentNodeManagerBase`. **Guarantees.** Behaviors activate deepest node first, and on one node from the base-registered behavior to the derived-registered one; manager-scoped behaviors run last, so they can drive what the node behaviors prepared. Any failure — including cancellation — unwinds every lease recorded so far in **exact reverse order**, and aggregates cleanup failures *with* the original rather than replacing it. **The engine stays internal.** Its vocabulary is not worth freezing into public API, and `INodeAttachContext` + `IAsyncDisposable` is the whole contract a caller needs. Two deliberate decisions worth a reviewer's attention: - **Method nodes are excluded from type matching.** `MethodState` aliases `MethodDeclarationId` onto `TypeDefinitionId`, so without an explicit filter every method node would match a registration for its declaration — a surprise nobody asked for. Method behavior stays on the call handler. Pinned by a test. - **A CLR type mismatch throws `BadTypeMismatch`**, naming node, expected and actual type, rather than silently skipping the instance. A whole set of alarms quietly getting no behavior is worse than a startup failure. `Dispose(bool)` is **signal-only**. Real release belongs in `DeleteAddressSpaceAsync`, which `MasterNodeManager.ShutdownAsync` invokes before it disposes managers on every production path — including the synchronous one, where `StandardServer.Dispose` blocks on `DisposeAsyncCore` → `StopAsync` → `ShutdownAsync` first. Blocking in `Dispose(bool)` would sit under a sync-over-async dispose and `MasterNodeManager`'s semaphore `Wait`. ## The registry migrations **`SimulationRegistry` — release moved, plus two latent bugs.** `Dispose` cancelled the loops then blocked up to five seconds draining them, from a synchronous dispose. Release now runs on the async teardown path and `Dispose` only trips the token. Loops are driven from the server `TimeProvider` instead of `PeriodicTimer`/`Stopwatch`, so they are testable on a fake clock rather than with sleeps. Fixed alongside: the running loop enumerated the handler list **by reference** while a late `OnTick` could still mutate it (handlers are now snapshotted, and a late `OnTick` is rejected rather than silently dropped); and a handler throwing `OperationCanceledException` for its own reasons killed the whole loop in silence. **`EventSourceRegistry` — ownership only; the reconcile loop stays.** It cannot collapse: `OnSubscribeToEventsAsync` runs while the monitored-item semaphore is held and `AddRootNotifierAsync` re-enters `SubscribeToEventsAsync`, so awaiting anything lease-shaped from that hook would self-deadlock. What moves is ownership of the two side effects nothing reversed — the `EventNotifier` bit ORed into the notifier, and the root-notifier registration. A node that *already* carried the flag is not ours to clear, so only what registration actually changed is recorded and undone. **`MonitoredSourceRegistry` — ownership only, and the real leak.** Rerouting teardown to `DisposeAsync` turned out to be necessary but **not sufficient**: that method only clears the item map, cancels the worker and disposes the lock, while the last-subscriber handler runs *solely* from the reconcile path's `Deactivate` arm — which server shutdown never reaches, because subscriptions dispose their items locally without deleting them. Anything acquired on the first subscriber therefore outlived the server. `ReleaseAsync` now runs those handlers for still-active sources, and `AcquireWhileMonitored` is the demand verb over the pair, with a test that proves release at shutdown with a live subscription. **Alarms.** Attaching an alarm enabled the condition, promoted the whole notifier chain and registered a root notifier, with no inverse anywhere. `RegisterAlarmEventSource` now reports what it changed and an `AlarmRelease` behavior undoes it. While wiring that up: the parent's `EventNotifier` was being promoted **twice** — once directly and once inside `RegisterAlarmEventSource` — which left the reversible path with nothing recorded. The redundant promotion is gone. Alarms still work on managers that never opted into the fluent surface; those simply get no automatic release, exactly as before, rather than a hard failure. ## What this deliberately does not do **Registry and alarm *setup* stays where it is.** Only release moved. `NodeManagerBuilder.Seal()` is synchronous and is called from sites that never call `CompleteConfigureAsync` — `Isa95NodeManager.cs:1046` seals from inside `private void ConfigureStatusEvents()` — so making behavior activation the sole setup path would silently disable simulations, event sources and alarms in ISA-95, Pump, Generator and RuntimeNodeSet. That same synchrony is what forced the `OnAddressSpaceReadyAsync` hook in #4434 (*"`Configure` is a `partial void` and cannot await"*), which `Opc.Ua.Di.Server/DiNodeManager.cs` had already invented privately. **A separate PR making the fluent configure path async is planned to land first**; after it, setup can move onto activation here and both copies of that workaround can go. Also out of scope: incremental attach on runtime node add and release on node removal (the ledger needs real synchronisation first), and the Quickstart `AlarmNodeManager` port that would give the alarm work end-to-end coverage. ## Related Issues No tracking issue yet — this came out of a design review of an earlier behavior-activation prototype, which was rejected for being unreachable from any application and hooked into a loading path rather than the node manager. Happy to open one and link it if maintainers want the ADR on file before review. ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. *(XML doc comments on every new public member, including why method nodes are excluded and why `Dispose` is signal-only. No `docs/` page — say the word if one is wanted.)* - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. *(net10.0 only — see Validation. Not run against .NET Framework locally.)* - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ## Validation net10.0: - `UA.slnx` builds clean, 0 errors. - `Opc.Ua.Server.Tests`: **4,984 passed, 0 failed**, 5 skipped. - 23 new tests: - **17 on the behavior surface** — child-first ordering, base-before-derived on one node, exact-reverse unwind, fan-out over three instances, subtype vs exact-type matching, zero-match diagnostics, declined instances, mid-activation rollback, lifetime-token ordering, second-pass unwind order, drained registrations, method-node exclusion, alarm release, `Publish` release, `Dispose` tripping an activated behavior's lifetime, and the two aggregation contracts (several release failures all surface and do not stop the remaining releases; a rollback that itself fails does not hide the activation failure that triggered it). - **4 on the simulation lifecycle** — fake-clock ticking, teardown awaiting an in-flight tick, non-blocking `Dispose`, late-handler rejection. - **2 on `AcquireWhileMonitored`** — release on the last subscriber, and release at shutdown with a live subscription. - No new analyzer warnings. Not run locally: .NET Framework targets, and suites outside `Opc.Ua.Server.Tests`. **On the advisory coverage gate.** The first report put patch coverage at 47.65%, listing whole files as uncovered that these tests exercise directly — `NodeBehaviorRegistry.cs` end to end, for instance. That report is assembled from whichever legs published Cobertura fragments, and the `Server` leg had not. It did point at three paths that genuinely had no test, all now covered: `EventSourceRegistry.ReleaseAsync`, `SignalShutdown` (reachable only via `Dispose` on a manager whose behaviors actually activated), and the two failure-aggregation paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
romanett
deleted the
romanett/applications-node-manager-fluent-8c0aa7
branch
September 9, 2026 08:55
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
Builds on #4428, which is now merged; this branch is rebased onto master.
The GDS
ApplicationsNodeManagerbecomes 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/OnReadUserRolePermissionson the node builder. Every otherNodeStateOn*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 afterConfigureresolved its graph.OnAddressSpaceReadyAsynconFluentNodeManagerBase— the one seam between the address space existing and the wiring being applied, for managers whose wiring depends on asynchronous setup.Configureis apartial voidand cannot await.DiNodeManagerhad already invented the hook privately, so its declaration becomes anoverride.2. Let a
[NodeManager]bind to a model a referenced assembly suppliesA 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=falseto suppress the two-argument form; its own logger category; and the call toOnAddressSpaceReadyAsync.Generated fluent accessors also stop re-enabling nullable warnings — a bare
#nullable enableundid the shared header'sdisable warnings, so any model with a structure-typed method argument reported CS8600 on every generatedout Tin a project that builds warnings as errors.3. Reimplement the GDS
ApplicationsNodeManageras a generated node managerThe 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
OnAddressSpaceReadyAsyncandDispose(bool)— no address-space overrides at all:NewNamespaceIndexes[0], with a thread-safe counter. The override existed only becauseCreate(assignNodeIds: true)asks the factory with the declaration id still set;AssignInstanceNodeIdnulls it and re-asks, documented as being there for exactly this allocator.AddBehaviourToPredefinedNodeAsyncAuthorizationServiceTypebranch re-wired a node that was already wired, and itsKeyCredentialServiceTypebranch had nothing to wire — the model ships that folder empty.GetManagerHandleAsync,ValidateNodeAsyncDeleteAddressSpaceAsync// TBDpassthrough.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
Addsurface and wired in the same place) — is oneConfigurepass.4. Bind the certificate groups from the
ConfigurepassAcquiring a certificate group and binding it to its nodes are different kinds of work: the first is I/O (
InitAsyncopens 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_ownedCertificateGroupsthe moment it is constructed, beforeInitAsynccan 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.Create(..., NodeId.Null, ..., assignNodeIds: false)does not null the NodeId.NodeState.Createoverrides it only when handed a non-null one, so a custom group node kept the namespace-0 id of its type declaration.AssignInstanceNodeIdhid 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 theDefaultauthorization service already was.GetCertificateGroupsandReadNode, 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
PushTestmethods change — please review these closelyThe second defect had been masking what those tests actually did. Selecting the group positionally,
groups[3]resolved to the server's ownServerConfigurationgroup while the custom one stayed invisible, soUpdateTrustListOfCustomGroupAsyncandAddRemoveCertOfCustomGroupAsyncwere exercising a transactional trust list under a custom-group name.Browse results, before and after:
ns=3)ns=3)DefaultApplicationGroupi=14156— the server's ownMyCustomGroupns=2DefaultApplicationGroupi=14156They 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:CloseAndUpdatewrites the stores at once and reportsapplyChangesRequired == false, so theApplyChangescalls go away (they would returnBad_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
PushTestmethods, which target it deliberately.5. Select
GetCertificatesresults by type instead of by positionGetCertificatesAsyncchecked 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 throwsIndexOutOfRangerather 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
.Commonsuffix from the three GDS projectsEvery other companion specification ships as
<Family>/<Family>.Client/<Family>.Server— Di, ISA95, Positioning, Robotics, Vision, WotCon, XRegistry. GDS was the exception, with a.Commonsuffix that named nothing.src/Opc.Ua.Gds.Commonsrc/Opc.Ua.Gdssrc/Opc.Ua.Gds.Client.Commonsrc/Opc.Ua.Gds.Clientsrc/Opc.Ua.Gds.Server.Commonsrc/Opc.Ua.Gds.ServerNo source changes were needed.
RootNamespacewas already the final name in all three projects — the namespaces have always beenOpc.Ua.Gds,Opc.Ua.Gds.ClientandOpc.Ua.Gds.Server, and only the assembly and package ids disagreed. Nothing compiling against these assemblies changes ausing.The package ids do change, which is consumer-visible, so
docs/migrate/2.0.x/packages.mdgains 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.slnxis derived fromUA.slnxand needs no edit. The migration analyzer's shim tree mirrors the source layout by its own documented convention, soGds.Client.Common/becomesGds.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 inXRegistry,Types.TestsandWotCon.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.Opc.Ua.Gds.dll/.Client.dll/.Server.dll, with no*.Common.dllproduced anywhere.Opc.Ua.Gds.Server.Commonalso builds fornet48andnetstandard2.1(checked before the rebase onto Add node creation to the fluent node manager builder #4428).Not yet run: the full
Opc.Ua.SourceGeneration.Core.Testssuite (~3,800), and the older target frameworks since the rebase.Checklist
🤖 Generated with Claude Code