Assign instance NodeIds in generated CreateOrReplace<Child> helpers

docs/DeviceIntegration.md

@@ -189,11 +189,12 @@ ITopologyElementBuilder<TElement> TopologyElementByBrowseName<TElement>(
    `HardwareRevision`, `SoftwareRevision`, `DeviceRevision`,
    `DeviceManual`, `SerialNumber`, `RevisionCounter`) with correct
    DI-namespace BrowseNames — plus the type's `HasInterface`
-   references. It then sets BrowseName/SymbolicName/DisplayName, stamps
-   the `TypeDefinitionId`, assigns the NodeId via the active
-   `Context.NodeIdFactory`, and walks the whole subtree assigning
-   per-instance NodeIds so multiple instances of the same type never
-   collide on the TYPE NodeIds emitted by the generator.
+   references. Because a browse name is supplied, the factory also
+   rebases the whole subtree onto per-instance NodeIds from the active
+   `Context.NodeIdFactory`, so multiple instances of the same type
+   never collide on the TYPE NodeIds emitted by the generator. It then
+   sets BrowseName/SymbolicName/DisplayName and stamps the
+   `TypeDefinitionId`.
 5. Calls the real `AsyncCustomNodeManager.AddPredefinedNodeAsync` so
    subscription wiring, type-tree registration, and root-notifier
    propagation all happen exactly as for nodes loaded from a NodeSet2.

docs/SourceGeneratedNodeManagers.md

@@ -958,6 +958,48 @@ input is supplied to the others as a resolution dependency (both
 > NodeSet2's generated types — set the per-file MSBuild metadata on the
 > NodeSet2 entry to control it.
 
+## Materialising instances at runtime — NodeId assignment
+
+Every model gets three families of instance helpers. They differ only in
+**who owns the NodeIds** of the nodes they produce:
+
+| Helper | Produces | NodeIds |
+| --- | --- | --- |
+| `context.CreateInstanceOf<Type>(parent, browseName)` | A full typed subtree | Rebased onto per-instance NodeIds minted by `ISystemContext.NodeIdFactory` whenever a `browseName` is supplied |
+| `owner.Add<Child>(context, nodeId = default)` | One optional child (+ its declared subtree) | Per-instance NodeIds, or the explicit `nodeId` you pass |
+| `owner.CreateOrReplace<Child>(context, replacement)` | One child slot — also the plumbing behind `NodeState.CreateChild` / `ReplaceChild` | Per-instance NodeIds for a child that carries no NodeId yet or still carries the type-level one |
+
+```csharp
+// Two instances of the same type never collide: the factory rebases the
+// mandatory children, and every subsequent child materialisation mints
+// its own NodeId from the parent chain.
+PumpState pump = SystemContext.CreateInstanceOfPumpType(deviceSet, pumpBrowseName);
+deviceSet.AddChild(pump);
+
+pump.AddOperational(SystemContext);                     // optional child
+pump.CreateChild(SystemContext, someBrowseName);        // CreateOrReplace<Child>
+
+await AddPredefinedNodeAsync(SystemContext, pump, cancellationToken);
+```
+
+Notes:
+
+* An explicit `browseName` is what marks a *dynamically materialised
+  instance*. `CreateInstanceOf<Type>()` without one (as used by the
+  generated `NodeStateActivator`s and when replacing a well-known
+  singleton) keeps the declaration NodeIds.
+* A NodeId **you** assigned is never overwritten — pass a fully
+  configured child as the `replacement`, or use `Add<Child>(context,
+  nodeId)`, to keep control.
+* The generated type factories opt out through
+  `assignInstanceNodeIds: false`: they build declaration subtrees whose
+  NodeIds `CreateInstanceOf<Type>` rebases in a single pass afterwards.
+  The same parameter is available to you if you need that behaviour.
+* Assignment only happens when the context carries an
+  `ISystemContext.NodeIdFactory`. `AsyncCustomNodeManager` supplies one
+  that allocates from the manager's namespace; override `New` to derive
+  ids from the parent chain instead.
+
 ## Current limitations
 
 - **Browse-path wildcards** (`*`, `**`) are not supported. Wire each

samples/ConsoleReferenceClient/ClientSamples.cs

@@ -404,6 +404,10 @@ public async Task<bool> SubscribeToDataChangesAsync(
                 }
 
                 // Define Subscription parameters
+                // Ownership of the subscription transfers to the session in AddSubscription
+                // below, which disposes all of its subscriptions when it is disposed.
+                // TODO: remove this suppression if CA2000 learns this ownership transfer pattern.
+#pragma warning disable CA2000
                 var subscription = new Subscription(session.DefaultSubscription)
                 {
                     DisplayName = "Console ReferenceClient Subscription",
@@ -413,6 +417,7 @@ public async Task<bool> SubscribeToDataChangesAsync(
                     MinLifetimeInterval = lifetime,
                     KeepAliveCount = 5
                 };
+#pragma warning restore CA2000
 
                 session.AddSubscription(subscription);
 
@@ -1284,6 +1289,10 @@ public async Task SubscribeAllValuesAsync(
                 session.MinPublishRequestCount = 3;
 
                 // Define Subscription parameters
+                // Ownership of the subscription transfers to the session in AddSubscription
+                // below, which disposes all of its subscriptions when it is disposed.
+                // TODO: remove this suppression if CA2000 learns this ownership transfer pattern.
+#pragma warning disable CA2000
                 var subscription = new Subscription(session.DefaultSubscription)
                 {
                     DisplayName = "Console ReferenceClient Subscription",
@@ -1299,6 +1308,8 @@ public async Task SubscribeAllValuesAsync(
                     FastDataChangeCallback = FastDataChangeNotification,
                     FastKeepAliveCallback = FastKeepAliveNotification
                 };
+#pragma warning restore CA2000
+
                 session.AddSubscription(subscription);
 
                 // Create the subscription on Server side

samples/PumpDeviceIntegrationServer/PumpNodeManager.cs

@@ -28,7 +28,6 @@
  * ======================================================================*/
 
 using System;
-using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
@@ -197,15 +196,17 @@ await MaterialisePumpInstanceAsync(pumpBrowseName, cancellationToken)
         /// from the DI <c>DeviceSet</c> with <c>Organizes</c>. The instance carries
         /// <c>PumpType</c> as its TypeDefinitionId so clients see the
         /// full OPC 40223 pump surface; the source-generated factory
-        /// materialises mandatory children (Identification) automatically.
+        /// materialises mandatory children (Identification) automatically
+        /// and - because a browse name is supplied - rebases the whole
+        /// subtree onto per-instance NodeIds minted by <see cref="New"/>.
         /// Optional children that the fluent simulation wires
         /// (Operational/Measurements/{analog states}, Events with the
         /// SupervisionProcessFluid + SupervisionPumpOperation subtrees,
         /// Maintenance) are materialised here via the generator-emitted
-        /// <c>AddXxx(context)</c> helpers; each new node gets a
-        /// per-instance NodeId via <see cref="AssignChildNodeIds"/>
-        /// before <c>AddPredefinedNodeAsync</c> recursively registers
-        /// the entire subtree.
+        /// <c>AddXxx(context)</c> helpers, which assign per-instance
+        /// NodeIds to every node they add before
+        /// <c>AddPredefinedNodeAsync</c> recursively registers the
+        /// entire subtree.
         /// </summary>
         private async ValueTask<PumpState> MaterialisePumpInstanceAsync(
             QualifiedName pumpBrowseName,
@@ -235,22 +236,17 @@ private async ValueTask<PumpState> MaterialisePumpInstanceAsync(
                     pumpBrowseName);
             }
 
+            // The DeviceSet is passed as the parent so New() can derive the
+            // per-instance NodeIds the factory stamps on the pump and its
+            // mandatory children from the parent chain.
             PumpState pump = SystemContext
-                .CreateInstanceOfPumpType(parent: null!, pumpBrowseName);
+                .CreateInstanceOfPumpType(deviceSet, pumpBrowseName);
 
             pump.ReferenceTypeId = Opc.Ua.Types.ReferenceTypeIds.Organizes;
             deviceSet.AddChild(pump);
-            pump.NodeId = SystemContext.NodeIdFactory.New(SystemContext, pump);
 
             MaterialisePumpOptionalChildren(pump);
 
-            // Walk the whole pump subtree assigning per-instance NodeIds
-            // BEFORE AddPredefinedNodeAsync uses them as the PredefinedNodes
-            // dictionary key. The generator's AddXxx helpers stamp the
-            // TYPE NodeId on every new child; without this walk every
-            // instance of PumpType would collide on those NodeIds.
-            AssignChildNodeIds(pump);
-
             await AddPredefinedNodeAsync(SystemContext, pump, cancellationToken)
                 .ConfigureAwait(false);
 
@@ -262,7 +258,8 @@ await AddPredefinedNodeAsync(SystemContext, pump, cancellationToken)
         /// Materialises the optional PumpType children that the fluent
         /// simulation in <see cref="Configure"/> wires. Each call to a
         /// generator-emitted <c>AddXxx(context)</c> helper creates the
-        /// child and assigns it to the parent's typed property; the
+        /// child, assigns it to the parent's typed property and stamps a
+        /// per-instance NodeId on the new node and its descendants; the
         /// parent.AddChild bookkeeping happens inside the helpers
         /// transparently.
         /// </summary>
@@ -303,25 +300,6 @@ private void MaterialisePumpOptionalChildren(
             pump.AddMaintenance(SystemContext);
         }
 
-        /// <summary>
-        /// Recursively walks the children of <paramref name="parent"/>
-        /// and assigns per-instance NodeIds via the active
-        /// <see cref="ISystemContext.NodeIdFactory"/>. Required after
-        /// calling generator-emitted <c>AddXxx(context)</c> helpers
-        /// which stamp the TYPE NodeId on every new child.
-        /// </summary>
-        private void AssignChildNodeIds(NodeState parent)
-        {
-            var children = new List<BaseInstanceState>();
-            parent.GetChildren(SystemContext, children);
-            foreach (BaseInstanceState child in children)
-            {
-                child.NodeId = SystemContext.NodeIdFactory.New(
-                    SystemContext, child);
-                AssignChildNodeIds(child);
-            }
-        }
-
         /// <summary>
         /// Partial wired by the Configure.cs sibling.
         /// </summary>

src/Opc.Ua.Types/State/BaseDataVariableState.cs

@@ -193,22 +193,39 @@ public override void GetChildren(ISystemContext context, IList<BaseInstanceState
         /// <summary>
         /// Create or replace enum strings
         /// </summary>
+        /// <param name="context">The system context.</param>
+        /// <param name="replacement">
+        /// The child to adopt or to copy onto the existing child. When
+        /// <c>null</c> a new child is created.
+        /// </param>
+        /// <param name="assignInstanceNodeIds">
+        /// When <c>true</c> (the default) and the context supplies a
+        /// <see cref="ISystemContext.NodeIdFactory"/>, a child that does not
+        /// yet carry a NodeId - and its descendants - are assigned per-instance
+        /// NodeIds so multiple instances of the same variable never collide. A
+        /// NodeId the caller already assigned is never overwritten.
+        /// </param>
         public PropertyState<ArrayOf<LocalizedText>> CreateOrReplaceEnumStrings(
             ISystemContext context,
-            BaseInstanceState? replacement)
+            BaseInstanceState? replacement,
+            bool assignInstanceNodeIds = true)
         {
             if (EnumStrings == null)
             {
                 if (replacement is not PropertyState<ArrayOf<LocalizedText>> child)
                 {
                     child = PropertyState<ArrayOf<LocalizedText>>.With<VariantBuilder>(this);
+                    child.SymbolicName = BrowseNames.EnumStrings;
+                    child.BrowseName = QualifiedName.From(BrowseNames.EnumStrings);
+                    child.DisplayName = LocalizedText.From(BrowseNames.EnumStrings);
                     if (replacement != null)
                     {
                         child.Create(context, replacement);
                     }
                 }
                 EnumStrings = child;
             }
+            context.AssignNewChildInstanceNodeIds(this, EnumStrings, assignInstanceNodeIds);
             return EnumStrings;
         }
 

src/Opc.Ua.Types/State/MethodState.cs

@@ -546,44 +546,78 @@ public override void GetChildren(ISystemContext context, IList<BaseInstanceState
         /// <summary>
         /// Create or replace output arguments
         /// </summary>
+        /// <param name="context">The system context.</param>
+        /// <param name="replacement">
+        /// The child to adopt or to copy onto the existing child. When
+        /// <c>null</c> a new child is created.
+        /// </param>
+        /// <param name="assignInstanceNodeIds">
+        /// When <c>true</c> (the default) and the context supplies a
+        /// <see cref="ISystemContext.NodeIdFactory"/>, a child that does not
+        /// yet carry a NodeId - and its descendants - are assigned per-instance
+        /// NodeIds so multiple instances of the same method never collide. A
+        /// NodeId the caller already assigned is never overwritten.
+        /// </param>
         public PropertyState<ArrayOf<Argument>> CreateOrReplaceOutputArguments(
             ISystemContext context,
-            BaseInstanceState? replacement)
+            BaseInstanceState? replacement,
+            bool assignInstanceNodeIds = true)
         {
             if (OutputArguments == null)
             {
                 if (replacement is not PropertyState<ArrayOf<Argument>> child)
                 {
                     child = PropertyState<ArrayOf<Argument>>.With<StructureBuilder<Argument>>(this);
+                    child.SymbolicName = BrowseNames.OutputArguments;
+                    child.BrowseName = QualifiedName.From(BrowseNames.OutputArguments);
+                    child.DisplayName = LocalizedText.From(BrowseNames.OutputArguments);
                     if (replacement != null)
                     {
                         child.Create(context, replacement);
                     }
                 }
                 OutputArguments = child;
             }
+            context.AssignNewChildInstanceNodeIds(this, OutputArguments, assignInstanceNodeIds);
             return OutputArguments;
         }
 
         /// <summary>
         /// Create or replace input arguments
         /// </summary>
+        /// <param name="context">The system context.</param>
+        /// <param name="replacement">
+        /// The child to adopt or to copy onto the existing child. When
+        /// <c>null</c> a new child is created.
+        /// </param>
+        /// <param name="assignInstanceNodeIds">
+        /// When <c>true</c> (the default) and the context supplies a
+        /// <see cref="ISystemContext.NodeIdFactory"/>, a child that does not
+        /// yet carry a NodeId - and its descendants - are assigned per-instance
+        /// NodeIds so multiple instances of the same method never collide. A
+        /// NodeId the caller already assigned is never overwritten.
+        /// </param>
         public PropertyState<ArrayOf<Argument>> CreateOrReplaceInputArguments(
             ISystemContext context,
-            BaseInstanceState? replacement)
+            BaseInstanceState? replacement,
+            bool assignInstanceNodeIds = true)
         {
             if (InputArguments == null)
             {
                 if (replacement is not PropertyState<ArrayOf<Argument>> child)
                 {
                     child = PropertyState<ArrayOf<Argument>>.With<StructureBuilder<Argument>>(this);
+                    child.SymbolicName = BrowseNames.InputArguments;
+                    child.BrowseName = QualifiedName.From(BrowseNames.InputArguments);
+                    child.DisplayName = LocalizedText.From(BrowseNames.InputArguments);
                     if (replacement != null)
                     {
                         child.Create(context, replacement);
                     }
                 }
                 InputArguments = child;
             }
+            context.AssignNewChildInstanceNodeIds(this, InputArguments, assignInstanceNodeIds);
             return InputArguments;
         }
 

src/Opc.Ua.Types/State/NodeInstanceExtensions.cs

@@ -211,5 +211,41 @@ private static void AssignChildNodeIds(
                 AssignChildNodeIds(context, child, mappingTable);
             }
         }
+
+        /// <summary>
+        /// Assigns a per-instance NodeId to a child that a
+        /// <c>CreateOrReplace&lt;Child&gt;</c> helper just materialised, and
+        /// rebases its descendants. A child whose NodeId the caller already
+        /// assigned is left untouched.
+        /// </summary>
+        /// <param name="context">
+        /// The system context supplying the NodeIdFactory.
+        /// </param>
+        /// <param name="owner">
+        /// The node owning the child. Its subtree carries the references that
+        /// must be remapped.
+        /// </param>
+        /// <param name="child">The child to rebase.</param>
+        /// <param name="assignInstanceNodeIds">
+        /// When <c>false</c> the call is a no-op. Lets callers that build
+        /// declaration subtrees keep their type-level NodeIds.
+        /// </param>
+        internal static void AssignNewChildInstanceNodeIds(
+            this ISystemContext context,
+            NodeState owner,
+            NodeState child,
+            bool assignInstanceNodeIds)
+        {
+            if (!assignInstanceNodeIds ||
+                context?.NodeIdFactory == null ||
+                child == null ||
+                !child.NodeId.IsNull)
+            {
+                return;
+            }
+
+            NodeId previousNodeId = context.AssignInstanceNodeId(child);
+            context.AssignInstanceChildNodeIds(child, previousNodeId, owner);
+        }
     }
 }

tests/Opc.Ua.Di.Tests/Opc.Ua.Di.Tests.csproj

@@ -46,6 +46,7 @@
   <ItemGroup Condition="'$(TargetFramework)' == 'net472'">
     <Compile Remove="PumpHostingIntegrationTests.cs" />
     <Compile Remove="PumpHostedReferenceTests.cs" />
+    <Compile Remove="PumpInstanceNodeIdRegressionTests.cs" />
     <Compile Remove="PumpNodeManagerFactoryTests.cs" />
     <Compile Remove="PumpTypeInstanceTests.cs" />
     <Compile Remove="PumpsNodeSetLoadingTests.cs" />