diff --git a/docs/MigrationGuide.md b/docs/MigrationGuide.md index 32c8bb45e2..73dc35f321 100644 --- a/docs/MigrationGuide.md +++ b/docs/MigrationGuide.md @@ -43,6 +43,30 @@ Looking for the broader narrative (non-prescriptive overview of what changed in a release)? See [What's New in 2.0](WhatsNewIn2.0.md). +## Migrating node types that override FindChild or CreateChild + +`NodeState.FindChild` and `NodeState.CreateChild` take +`assignInstanceNodeIds` as their last parameter, and the four argument +`FindChild` / two argument `CreateChild` virtuals are gone. The parameter +defaults to `true`, so **call sites are unaffected**; an override fails to +compile (`CS0115`) until the parameter is added and passed on. + +Behaviour note: a node copy — `NodeState.Create(context, source)` and the +`Initialize(ISystemContext, NodeState)` path behind it — now passes +`assignInstanceNodeIds: false`. It no longer asks +`ISystemContext.NodeIdFactory` for identifiers that the copy overwrites +from the source on the very next statement. If your `INodeIdFactory` +counts, reserves or audits every allocation, expect **fewer** calls than in +1.5.378 for the same address space; the resulting NodeIds are unchanged. +Any `NodeState` subclass you own must thread the argument into its +`CreateOrReplace` calls to get that benefit. + +See +[Node states § FindChild and CreateChild](migrate/2.0.x/node-states.md#nodestate-findchild-and-createchild-state-nodeid-assignment) +for the before/after and +[Custom node types and assignment control](NodeManagers.md#custom-node-types-and-assignment-control) +for the runtime rules. + ## Migrating servers that relied on unserved history advertisement Server startup now reconciles variables that advertise history with the diff --git a/docs/NodeManagers.md b/docs/NodeManagers.md index e09cc4c7d8..0318025a25 100644 --- a/docs/NodeManagers.md +++ b/docs/NodeManagers.md @@ -1592,6 +1592,51 @@ Notes: `ISystemContext.NodeIdFactory`. `AsyncCustomNodeManager` supplies one that allocates from the manager's namespace; override `New` to derive ids from the parent chain instead. +* **A node copy never assigns.** `NodeState.Create(context, source)` + initialises each child from its source right after creating it, which + overwrites any NodeId minted along the way — so minting one would only + consume identifiers, and leak them for factories that track outstanding + allocations. The copy therefore calls + `CreateChild(context, browseName, assignInstanceNodeIds: false)`. + +#### Custom node types and assignment control + +`NodeState.FindChild` and `NodeState.CreateChild` carry +`assignInstanceNodeIds` as their last parameter. It defaults to `true`, so +callers that state no intent keep materialising children with per-instance +NodeIds. A type that declares children overrides `FindChild`, resolves the +ones it declares, and passes the request on — both to its +`CreateOrReplace` helpers and to the base: + +```csharp +protected override BaseInstanceState? FindChild( + ISystemContext context, + QualifiedName browseName, + bool createOrReplace, + BaseInstanceState? replacement, + bool assignInstanceNodeIds = true) +{ + if (browseName.Name == BrowseNames.EnumStrings) + { + return !createOrReplace + ? EnumStrings + : CreateOrReplaceEnumStrings(context, replacement, assignInstanceNodeIds); + } + + return base.FindChild( + context, browseName, createOrReplace, replacement, assignInstanceNodeIds); +} +``` + +Source generated types emit exactly this shape. Because the request is an +argument, every type — generated or hand-written — sees the real +`ISystemContext` during a copy; nothing wraps the context to hide the +`NodeIdFactory`. + +> **Breaking change in 2.0.** The four argument `FindChild` and the two +> argument `CreateChild` are gone. An override written against 1.5.378 fails +> to compile until the parameter is added; see the +> [migration guide](migrate/2.0.x/node-states.md#nodestate-findchild-and-createchild-state-nodeid-assignment). ### Current limitations diff --git a/docs/migrate/2.0.x/README.md b/docs/migrate/2.0.x/README.md index 777d1932af..23798ce64c 100644 --- a/docs/migrate/2.0.x/README.md +++ b/docs/migrate/2.0.x/README.md @@ -34,7 +34,7 @@ table; loading a single sub-doc keeps the context window small. | `OPCFoundation.NetStandard.Opc.Ua.*` package upgrade, TFM changes, Newtonsoft removal | [`packages.md`](packages.md) | | Source-generated `*Collection` shims, NodeManager generator, default of boolean properties, project structure | [`source-generation.md`](source-generation.md) | | `IEncodeableFactoryBuilder`, `IType`, JSON / XML / binary encoders, `EncodeableFactory.GlobalFactory`, `IJsonEncodeable`, `ComplexTypes` namespace move | [`encoders.md`](encoders.md) | -| `CustomNodeManager`, `NodeState` clone / read / write helpers, `OnAfterCreate(CancellationToken)`, `INodeManager3`, `INodeCache.InvalidateNode`, generics on `BaseVariableState` / `BaseVariableTypeState`, predefined-node processing | [`node-states.md`](node-states.md) | +| `CustomNodeManager`, `NodeState` clone / read / write helpers, `OnAfterCreate(CancellationToken)`, `FindChild` / `CreateChild` NodeId assignment, `INodeManager3`, `INodeCache.InvalidateNode`, generics on `BaseVariableState` / `BaseVariableTypeState`, predefined-node processing | [`node-states.md`](node-states.md) | | `IUserIdentityTokenHandler`, `IClientIdentityProvider`, `IUserTokenAuthenticator`, `IAccessTokenProvider`, `ITokenIssuer`, `IIdentityClaims`, caller-supplied secrets, secret store | [`identity.md`](identity.md) | | `CertificateValidator`, ref-counted `Certificate` wrapper, `CertificateManager`, `ICertificateProvider`, obsoleted `X509Certificate2` direct-exposure APIs, PushManagement transactions (`ApplyChanges`-gated TrustList updates) | [`certificates.md`](certificates.md) | | `ApplicationConfiguration` changes, Data-Contract serializer removal, `ParseExtension` / `UpdateExtension` signature, session / browser state persistence | [`configuration.md`](configuration.md) | diff --git a/docs/migrate/2.0.x/node-states.md b/docs/migrate/2.0.x/node-states.md index 80526f5f07..787f90ca65 100644 --- a/docs/migrate/2.0.x/node-states.md +++ b/docs/migrate/2.0.x/node-states.md @@ -113,6 +113,65 @@ protected override void OnAfterCreate(ISystemContext context, NodeState node, Ca } ``` +#### NodeState FindChild and CreateChild state NodeId assignment + +`NodeState.FindChild` and `NodeState.CreateChild` now take +`assignInstanceNodeIds` as their last parameter, and the old four argument +`FindChild` / two argument `CreateChild` virtuals are gone. The parameter +defaults to `true`, so **call sites keep compiling and keep the 1.5.378 +behaviour** — only overrides have to change: + +```csharp +// Before +protected override BaseInstanceState FindChild( + ISystemContext context, + QualifiedName browseName, + bool createOrReplace, + BaseInstanceState replacement) +{ + if (browseName.Name == BrowseNames.MyChild) + { + return createOrReplace + ? CreateOrReplaceMyChild(context, replacement) + : MyChild; + } + return base.FindChild(context, browseName, createOrReplace, replacement); +} + +// After — add the parameter and pass it on +protected override BaseInstanceState FindChild( + ISystemContext context, + QualifiedName browseName, + bool createOrReplace, + BaseInstanceState replacement, + bool assignInstanceNodeIds = true) +{ + if (browseName.Name == BrowseNames.MyChild) + { + return createOrReplace + ? CreateOrReplaceMyChild(context, replacement, assignInstanceNodeIds) + : MyChild; + } + return base.FindChild( + context, browseName, createOrReplace, replacement, assignInstanceNodeIds); +} +``` + +A missing override raises `CS0115` (`no suitable method found to +override`), so the compiler points at every site that needs the parameter. +Repeat the `= true` default in the override so callers bound to your derived +type keep the same behaviour. + +Why: a node copy creates each child and then initialises it from its source, +which overwrites any NodeId minted along the way. Passing +`assignInstanceNodeIds: false` — what `NodeState.Create(context, source)` +now does — stops the `ISystemContext.NodeIdFactory` from being asked for +identifiers that are immediately discarded, and leaked by factories that +track outstanding allocations. Thread the argument into every +`CreateOrReplace` call your override makes; source generated types +already do. See +[Custom node types and assignment control](../../NodeManagers.md#custom-node-types-and-assignment-control). + ### INodeManager3 - new role-permission and method-resolution hooks 2.0 introduces `INodeManager3`, an extension of `INodeManager2` that surfaces explicit hooks for per-role permission evaluation and for resolving the target of a `Call` request. `CustomNodeManager2` implements the new members with safe defaults that mirror the previous behavior, so node managers that already derive from `CustomNodeManager2` need no changes. diff --git a/src/Opc.Ua.Types/State/BaseDataVariableState.cs b/src/Opc.Ua.Types/State/BaseDataVariableState.cs index 1e7275dabe..492feb2aa7 100644 --- a/src/Opc.Ua.Types/State/BaseDataVariableState.cs +++ b/src/Opc.Ua.Types/State/BaseDataVariableState.cs @@ -165,29 +165,23 @@ public override void GetChildren(ISystemContext context, IList - /// Finds the child with the specified browse name. - /// + /// protected override BaseInstanceState? FindChild( ISystemContext context, QualifiedName browseName, bool createOrReplace, - BaseInstanceState? replacement) + BaseInstanceState? replacement, + bool assignInstanceNodeIds = true) { - if (browseName.IsNull) + if (browseName.Name == BrowseNames.EnumStrings) { - return null; + return !createOrReplace + ? EnumStrings + : CreateOrReplaceEnumStrings(context, replacement, assignInstanceNodeIds); } - BaseInstanceState? instance = null; - switch (browseName.Name) - { - case BrowseNames.EnumStrings: - instance = !createOrReplace ? - EnumStrings : CreateOrReplaceEnumStrings(context, replacement); - break; - } - return instance ?? base.FindChild(context, browseName, createOrReplace, replacement); + return base.FindChild( + context, browseName, createOrReplace, replacement, assignInstanceNodeIds); } /// diff --git a/src/Opc.Ua.Types/State/ISystemContext.cs b/src/Opc.Ua.Types/State/ISystemContext.cs index 10be515f23..02c6e006cf 100644 --- a/src/Opc.Ua.Types/State/ISystemContext.cs +++ b/src/Opc.Ua.Types/State/ISystemContext.cs @@ -90,12 +90,9 @@ public interface ISystemContext /// A factory that can be used to create node ids. /// /// - /// The node identifiers factory, or null when the context - /// suppresses NodeId assignment. Callers that assign NodeIds must - /// check for null; see - /// , which a node copy - /// uses so materialising its children does not consume identifiers - /// the copy immediately overwrites. + /// The node identifiers factory, or null when the context does + /// not assign NodeIds. Callers that assign NodeIds must check for + /// null. /// INodeIdFactory? NodeIdFactory { get; } diff --git a/src/Opc.Ua.Types/State/MethodState.cs b/src/Opc.Ua.Types/State/MethodState.cs index 1bf09e72d7..795ff6d3fb 100644 --- a/src/Opc.Ua.Types/State/MethodState.cs +++ b/src/Opc.Ua.Types/State/MethodState.cs @@ -522,25 +522,26 @@ public override void GetChildren(ISystemContext context, IList diff --git a/src/Opc.Ua.Types/State/NodeIdFactorySuppressedContext.cs b/src/Opc.Ua.Types/State/NodeIdFactorySuppressedContext.cs deleted file mode 100644 index 41bcea20da..0000000000 --- a/src/Opc.Ua.Types/State/NodeIdFactorySuppressedContext.cs +++ /dev/null @@ -1,97 +0,0 @@ -/* ======================================================================== - * Copyright (c) 2005-2026 The OPC Foundation, Inc. All rights reserved. - * - * OPC Foundation MIT License 1.00 - * - * Permission is hereby granted, free of charge, to any person - * obtaining a copy of this software and associated documentation - * files (the "Software"), to deal in the Software without - * restriction, including without limitation the rights to use, - * copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to the following - * conditions: - * - * The above copyright notice and this permission notice shall be - * included in all copies or substantial portions of the Software. - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, - * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES - * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND - * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT - * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, - * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR - * OTHER DEALINGS IN THE SOFTWARE. - * - * The complete license agreement can be found here: - * http://opcfoundation.org/License/MIT/1.00/ - * ======================================================================*/ - -using Opc.Ua.Types; - -namespace Opc.Ua -{ - /// - /// Forwards every member of a system context except its - /// , which is reported as absent. - /// - /// - /// A node copy materialises its children through - /// , and the - /// CreateOrReplace<Child> plumbing behind it assigns a - /// per-instance NodeId whenever the context carries a factory. In a copy - /// every child is initialised from its source immediately afterwards, which - /// overwrites that NodeId, so the assignment only consumes identifiers - - /// and permanently leaks them for factories that track outstanding - /// allocations. Hiding the factory for the duration of the copy leaves - /// those identifiers unused; the assignInstanceNodeIds flag cannot - /// serve here because it is not part of the virtual FindChild - /// contract the copy goes through. - /// - internal sealed class NodeIdFactorySuppressedContext : ISystemContext - { - /// - /// Wraps the supplied context. - /// - /// The context to forward to. - public NodeIdFactorySuppressedContext(ISystemContext context) - { - m_context = context; - } - - /// - public object? SystemHandle => m_context.SystemHandle; - - /// - public string? UserId => m_context.UserId; - - /// - public ArrayOf PreferredLocales => m_context.PreferredLocales; - - /// - public string? AuditEntryId => m_context.AuditEntryId; - - /// - public NamespaceTable NamespaceUris => m_context.NamespaceUris; - - /// - public StringTable ServerUris => m_context.ServerUris; - - /// - public ITypeTable TypeTable => m_context.TypeTable; - - /// - public IEncodeableFactory EncodeableFactory => m_context.EncodeableFactory; - - /// - public INodeIdFactory? NodeIdFactory => null; - - /// - public NodeStateFactory NodeStateFactory => m_context.NodeStateFactory; - - /// - public ITelemetryContext Telemetry => m_context.Telemetry; - - private readonly ISystemContext m_context; - } -} diff --git a/src/Opc.Ua.Types/State/NodeState.cs b/src/Opc.Ua.Types/State/NodeState.cs index 18d3a6bdfb..3bbd6e9c88 100644 --- a/src/Opc.Ua.Types/State/NodeState.cs +++ b/src/Opc.Ua.Types/State/NodeState.cs @@ -337,15 +337,14 @@ protected virtual void Initialize(ISystemContext context, NodeState source) // Every child created below is initialized from its source right // afterwards, which overwrites the NodeId a factory would hand out - // here, so the copy must not reach the factory at all. - ISystemContext childContext = children.Count > 0 && context.NodeIdFactory != null - ? new NodeIdFactorySuppressedContext(context) - : context; - + // here, so the copy must not consume identifiers for them. for (int ii = 0; ii < children.Count; ii++) { BaseInstanceState sourceChild = children[ii]; - BaseInstanceState? child = CreateChild(childContext, sourceChild.BrowseName); + BaseInstanceState? child = CreateChild( + context, + sourceChild.BrowseName, + assignInstanceNodeIds: false); if (child == null) { @@ -4687,19 +4686,32 @@ protected virtual ServiceResult WriteValueAttribute( /// /// Finds or creates the child with the specified browse name. /// + /// + /// A caller that overwrites the child's NodeId immediately afterwards - + /// a node copy is the canonical case - passes false for + /// so the + /// is never asked for an + /// identifier that is about to be discarded. + /// /// The context to use. /// The browse name. + /// + /// Whether a newly created child may be given a per-instance NodeId. + /// Defaults to true, which is what materialising a child onto a + /// live tree wants. + /// /// The child if available. Null otherwise. public virtual BaseInstanceState? CreateChild( ISystemContext context, - QualifiedName browseName) + QualifiedName browseName, + bool assignInstanceNodeIds = true) { if (browseName.IsNull) { return null; } - return FindChild(context, browseName, true, null); + return FindChild(context, browseName, true, null, assignInstanceNodeIds); } /// @@ -5355,6 +5367,16 @@ public virtual void GetReferences( /// /// Finds the child with the specified browse name. /// + /// + /// A type that declares children overrides this method, resolves the + /// ones it declares, and forwards everything else to the base. It must + /// pass on to every + /// CreateOrReplace<Child> helper it calls: a caller that + /// overwrites the child's NodeId immediately afterwards - a node copy is + /// the canonical case - declines assignment so the + /// is never asked for an + /// identifier that is about to be discarded. + /// /// The context for the system being accessed. /// The browse name of the children to add. /// if set to true and the child does @@ -5364,12 +5386,18 @@ public virtual void GetReferences( /// true. If not of same type, the node state is used to initialize a new /// instance of the required type (for narrowing conversation to the type /// definition + /// + /// Whether a newly created child may be given a per-instance NodeId. + /// Defaults to true, which is what materialising a child onto a + /// live tree wants. + /// /// The child. protected virtual BaseInstanceState? FindChild( ISystemContext context, QualifiedName browseName, bool createOrReplace, - BaseInstanceState? replacement) + BaseInstanceState? replacement, + bool assignInstanceNodeIds = true) { if (browseName.IsNull) { diff --git a/tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeManagerGeneratorTests.cs b/tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeManagerGeneratorTests.cs index 4566dca936..e0aa7cac37 100644 --- a/tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeManagerGeneratorTests.cs +++ b/tests/Opc.Ua.SourceGeneration.Core.Tests/Generators/NodeManagerGeneratorTests.cs @@ -267,6 +267,48 @@ public void CreateOrReplaceChildAssignsInstanceNodeIdsByDefault() }); } + /// + /// Every FindChild override a generated type emits carries the + /// assignment argument with its true default: the caller states + /// its intent, so no second overload, capability property or context + /// wrapper is needed to reach the override. + /// + [Test] + public void GeneratedTypesEmitOneFindChildOverrideCarryingTheAssignmentFlag() + { + Dictionary files = GenerateForTestModel(generateNodeManager: false); + string source = files.Values.Single(value => + value.Contains(" CreateOrReplaceRed(", StringComparison.Ordinal)); + + const string signature = + "protected override global::Opc.Ua.BaseInstanceState? FindChild("; + var parameterLists = new List(); + for (int index = source.IndexOf(signature, StringComparison.Ordinal); + index >= 0; + index = source.IndexOf(signature, index + signature.Length, StringComparison.Ordinal)) + { + int start = index + signature.Length; + int end = source.IndexOf(')', start); + parameterLists.Add(source.Substring(start, end - start)); + } + + Assert.Multiple(() => + { + Assert.That(parameterLists, Is.Not.Empty, + "A type declaring children must resolve them in a FindChild override."); + Assert.That(parameterLists, Has.All.Contains("bool assignInstanceNodeIds = true"), + "Every override must carry the argument, and repeat the default so " + + "callers keep the 1.5.378 behaviour."); + Assert.That(source, Does.Not.Contain("SupportsInstanceNodeIdAssignmentControl"), + "Assignment control is stated per call, not advertised per type."); + Assert.That(source, Does.Contain("return base.FindChild("), + "An unmatched browse name must pass the request on to the base."); + Assert.That(source, Does.Contain( + "context, browseName, createOrReplace, replacement, assignInstanceNodeIds);"), + "The request must be forwarded verbatim to the base."); + }); + } + /// /// The generated factories build declaration subtrees whose NodeIds /// must stay at their type-level values - the enclosing diff --git a/tests/Opc.Ua.Types.Tests/State/NodeInstanceExtensionsTests.cs b/tests/Opc.Ua.Types.Tests/State/NodeInstanceExtensionsTests.cs index 37381e1ebf..3132710356 100644 --- a/tests/Opc.Ua.Types.Tests/State/NodeInstanceExtensionsTests.cs +++ b/tests/Opc.Ua.Types.Tests/State/NodeInstanceExtensionsTests.cs @@ -83,6 +83,171 @@ public NodeId New(ISystemContext context, NodeState node) } } + /// + /// Records every identifier it hands out. A copy must not consume any, + /// because each child is initialized from its source right afterwards + /// and the assigned NodeId is discarded. + /// + private sealed class CountingNodeIdFactory : INodeIdFactory + { + public int Handouts { get; private set; } + + public NodeId New(ISystemContext context, NodeState node) + { + Handouts++; + return new NodeId(++m_nextId, 3); + } + + private uint m_nextId; + } + + /// + /// A hand written type that declares a child, standing in for a custom + /// node manager. It threads the assignment request into the child it + /// materialises, which is what every node type is now expected to do. + /// + private sealed class CustomOwnerState : BaseObjectState + { + public CustomOwnerState(NodeState parent) + : base(parent) + { + } + + public PropertyState Detail { get; private set; } + + /// + /// Whether a NodeIdFactory was visible the last time a child was + /// created. Every type is asked not to assign rather than shown a + /// context that misreports the factory, so this stays true. + /// + public bool SawNodeIdFactory { get; private set; } + + public override void GetChildren( + ISystemContext context, + IList children) + { + if (Detail != null) + { + children.Add(Detail); + } + base.GetChildren(context, children); + } + + protected override BaseInstanceState FindChild( + ISystemContext context, + QualifiedName browseName, + bool createOrReplace, + BaseInstanceState replacement, + bool assignInstanceNodeIds = true) + { + if (browseName.Name != "Detail") + { + return base.FindChild( + context, browseName, createOrReplace, replacement, + assignInstanceNodeIds); + } + if (!createOrReplace) + { + return Detail; + } + SawNodeIdFactory = context.NodeIdFactory != null; + Detail ??= new PropertyState(this) + { + SymbolicName = "Detail", + BrowseName = new QualifiedName("Detail", 3), + ReferenceTypeId = ReferenceTypeIds.HasProperty + }; + if (assignInstanceNodeIds && + context.NodeIdFactory != null && + Detail.NodeId.IsNull) + { + Detail.NodeId = context.NodeIdFactory.New(context, Detail); + } + return Detail; + } + } + + /// + /// A hand written type deriving from a type that declares children of + /// its own. Its override must be reached by a copy, and must decline + /// assignment for the child it adds on top. + /// + private sealed class DerivedMethodState : MethodState + { + public DerivedMethodState(NodeState parent) + : base(parent) + { + } + + public PropertyState Extra { get; private set; } + + public override void GetChildren( + ISystemContext context, + IList children) + { + if (Extra != null) + { + children.Add(Extra); + } + base.GetChildren(context, children); + } + + protected override BaseInstanceState FindChild( + ISystemContext context, + QualifiedName browseName, + bool createOrReplace, + BaseInstanceState replacement, + bool assignInstanceNodeIds = true) + { + if (browseName.Name != "Extra") + { + return base.FindChild( + context, browseName, createOrReplace, replacement, + assignInstanceNodeIds); + } + if (!createOrReplace) + { + return Extra; + } + Extra ??= new PropertyState(this) + { + SymbolicName = "Extra", + BrowseName = new QualifiedName("Extra", 3), + ReferenceTypeId = ReferenceTypeIds.HasProperty + }; + if (assignInstanceNodeIds && + context.NodeIdFactory != null && + Extra.NodeId.IsNull) + { + Extra.NodeId = context.NodeIdFactory.New(context, Extra); + } + return Extra; + } + } + + /// + /// A hand written type that intercepts child creation by overriding the + /// public CreateChild rather than FindChild. + /// + private sealed class CreateChildOverrideState : BaseObjectState + { + public CreateChildOverrideState(NodeState parent) + : base(parent) + { + } + + public int CreateChildCalls { get; private set; } + + public override BaseInstanceState CreateChild( + ISystemContext context, + QualifiedName browseName, + bool assignInstanceNodeIds = true) + { + CreateChildCalls++; + return base.CreateChild(context, browseName, assignInstanceNodeIds); + } + } + private static SystemContext CreateContext(INodeIdFactory factory) { ITelemetryContext telemetry = NUnitTelemetryContext.Create(); @@ -349,6 +514,246 @@ public void CreateOrReplaceArgumentsHonoursTheAssignmentOptOut() "Callers building declaration subtrees must keep control of the NodeIds."); } + /// + /// A copy initializes every child from its source right after creating + /// it, which overwrites any NodeId handed out along the way. Consuming + /// identifiers for them therefore only burns - and for factories that + /// track outstanding allocations, leaks - them. + /// + [Test] + public void CopyOfDeclaringTypeConsumesNoNodeIds() + { + var factory = new CountingNodeIdFactory(); + SystemContext context = CreateContext(factory); + + MethodState source = CreateMethod("Start", 1); + source.CreateOrReplaceInputArguments(context, null); + source.CreateOrReplaceOutputArguments(context, null); + int handoutsAfterSource = factory.Handouts; + + var copy = new MethodState(null); + copy.Create(context, source); + + Assert.That(factory.Handouts, Is.EqualTo(handoutsAfterSource), + "Copying must not consume identifiers for children whose NodeId " + + "is overwritten from the source immediately afterwards."); + } + + /// + /// The copy must still reproduce the source subtree while consuming + /// nothing, otherwise the optimisation would have changed behaviour. + /// + [Test] + public void CopyOfDeclaringTypeReproducesTheSource() + { + var factory = new CountingNodeIdFactory(); + SystemContext context = CreateContext(factory); + + MethodState source = CreateMethod("Start", 1); + PropertyState> sourceArguments = + source.CreateOrReplaceInputArguments(context, null); + + var copy = new MethodState(null); + copy.Create(context, source); + + Assert.That(copy.NodeId, Is.EqualTo(source.NodeId)); + Assert.That(copy.InputArguments, Is.Not.Null); + Assert.That(copy.InputArguments.NodeId, Is.EqualTo(sourceArguments.NodeId), + "A copied child must carry the source NodeId, not a freshly minted one."); + } + + /// + /// A hand written type is asked not to assign during a copy, and the + /// factory it is shown is the real one - nothing misreports the context. + /// + [Test] + public void CopyOfCustomTypeConsumesNoNodeIds() + { + var factory = new CountingNodeIdFactory(); + SystemContext context = CreateContext(factory); + + var source = new CustomOwnerState(null) + { + NodeId = new NodeId("Owner", 3), + SymbolicName = "Owner", + BrowseName = new QualifiedName("Owner", 3) + }; + source.CreateChild(context, new QualifiedName("Detail", 3)); + Assert.That(source.Detail, Is.Not.Null); + int handoutsAfterSource = factory.Handouts; + + var copy = new CustomOwnerState(null); + copy.Create(context, source); + + Assert.That(copy.Detail, Is.Not.Null, + "A hand written override must still be reached by a copy."); + Assert.That(factory.Handouts, Is.EqualTo(handoutsAfterSource), + "The type was asked not to assign, so no identifier may be consumed."); + } + + /// + /// Declining assignment is stated as an argument, so every type keeps + /// seeing the real system context - no wrapper reports the factory as + /// absent. + /// + [Test] + public void CopySeesTheRealContext() + { + var factory = new CountingNodeIdFactory(); + SystemContext context = CreateContext(factory); + + var source = new CustomOwnerState(null) + { + NodeId = new NodeId("Owner", 3), + SymbolicName = "Owner", + BrowseName = new QualifiedName("Owner", 3) + }; + source.CreateChild(context, new QualifiedName("Detail", 3)); + int handoutsAfterSource = factory.Handouts; + + var copy = new CustomOwnerState(null); + copy.Create(context, source); + + Assert.That(copy.Detail, Is.Not.Null); + Assert.That(copy.SawNodeIdFactory, Is.True, + "The context must not misreport the factory to a type that " + + "understands the assignment request."); + Assert.That(factory.Handouts, Is.EqualTo(handoutsAfterSource), + "It was asked not to assign, so no identifier may be consumed."); + } + + /// + /// Callers that state no intent keep the 1.5.378 behaviour: the default + /// of the assignment argument is true. + /// + [Test] + public void CreateChildAssignsInstanceNodeIdsByDefault() + { + var factory = new CountingNodeIdFactory(); + SystemContext context = CreateContext(factory); + + var owner = new CustomOwnerState(null) + { + NodeId = new NodeId("Owner", 3), + SymbolicName = "Owner", + BrowseName = new QualifiedName("Owner", 3) + }; + + BaseInstanceState detail = owner.CreateChild( + context, new QualifiedName("Detail", 3)); + + Assert.That(detail, Is.Not.Null); + Assert.That(detail.NodeId.IsNull, Is.False, + "A caller that states no intent must still get a per-instance NodeId."); + Assert.That(factory.Handouts, Is.EqualTo(1)); + } + + /// + /// A type deriving from one that declares children of its own must be + /// reached by a copy, and must not consume identifiers either. + /// + [Test] + public void CopyOfDerivedCustomOverrideConsumesNoNodeIds() + { + var factory = new CountingNodeIdFactory(); + SystemContext context = CreateContext(factory); + + var source = new DerivedMethodState(null) + { + NodeId = new NodeId("Start", 3), + SymbolicName = "Start", + BrowseName = new QualifiedName("Start", 3) + }; + source.CreateChild(context, new QualifiedName("Extra", 3)); + source.CreateOrReplaceInputArguments(context, null); + Assert.That(source.Extra, Is.Not.Null); + int handoutsAfterSource = factory.Handouts; + + var copy = new DerivedMethodState(null); + copy.Create(context, source); + + Assert.That(copy.Extra, Is.Not.Null, + "An override on a derived type must still be reached."); + Assert.That(copy.InputArguments, Is.Not.Null, + "The children the base type declares must be copied as well."); + Assert.That(factory.Handouts, Is.EqualTo(handoutsAfterSource), + "A derived override must decline assignment just like its base."); + } + + /// + /// A type that intercepts by overriding the public CreateChild rather + /// than FindChild must still be dispatched through by a copy. + /// + [Test] + public void CopyStillDispatchesThroughCreateChildOverride() + { + SystemContext context = CreateContext(new CountingNodeIdFactory()); + + var source = new CreateChildOverrideState(null) + { + NodeId = new NodeId("Owner", 3), + SymbolicName = "Owner", + BrowseName = new QualifiedName("Owner", 3) + }; + var detail = new PropertyState(source) + { + NodeId = new NodeId("Owner_Detail", 3), + SymbolicName = "Detail", + BrowseName = new QualifiedName("Detail", 3), + ReferenceTypeId = ReferenceTypeIds.HasProperty + }; + source.AddChild(detail); + + var copy = new CreateChildOverrideState(null); + copy.Create(context, source); + + Assert.That(copy.CreateChildCalls, Is.GreaterThan(0), + "Overriding CreateChild must keep working across a node copy."); + } + + /// + /// Resolving a browse name no type in the hierarchy declares walks the + /// whole override chain. It must terminate. + /// + [Test] + public void FindingAnUndeclaredChildDoesNotRecurse() + { + SystemContext context = CreateContext(new CountingNodeIdFactory()); + var owner = new CustomOwnerState(null) + { + NodeId = new NodeId("Owner", 3), + SymbolicName = "Owner", + BrowseName = new QualifiedName("Owner", 3) + }; + + BaseInstanceState found = null; + Assert.DoesNotThrow( + () => found = owner.CreateChild( + context, new QualifiedName("NoSuchChild", 3), false)); + Assert.That(found, Is.Null); + } + + /// + /// Reading InputArguments must not return OutputArguments. + /// + [Test] + public void FindChildReturnsTheRequestedArgumentsProperty() + { + SystemContext context = CreateContext(new ChildIdFactory()); + MethodState method = CreateMethod("Start", 1); + PropertyState> inputs = + method.CreateOrReplaceInputArguments(context, null); + PropertyState> outputs = + method.CreateOrReplaceOutputArguments(context, null); + Assert.That(inputs.NodeId, Is.Not.EqualTo(outputs.NodeId)); + + BaseInstanceState found = method.FindChild( + context, new QualifiedName(BrowseNames.InputArguments, 3)); + + Assert.That(found, Is.SameAs(inputs), + "InputArguments must resolve to the input arguments property."); + } + [Test] public void CreateOrReplaceEnumStringsAssignsInstanceNodeIds() { diff --git a/tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateTemplates.cs b/tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateTemplates.cs index a59e03372d..18067c8305 100644 --- a/tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateTemplates.cs +++ b/tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateTemplates.cs @@ -1161,12 +1161,9 @@ public override void GetChildren( global::Opc.Ua.ISystemContext context, global::Opc.Ua.QualifiedName browseName, bool createOrReplace, - global::Opc.Ua.BaseInstanceState? replacement) + global::Opc.Ua.BaseInstanceState? replacement, + bool assignInstanceNodeIds = true) { - if (browseName.IsNull) - { - return null; - } global::Opc.Ua.BaseInstanceState? instance = null; switch (browseName.Name) @@ -1175,13 +1172,11 @@ public override void GetChildren( } if (instance != null) - { - return instance; - } - return base.FindChild(context, browseName, createOrReplace, replacement); + return base.FindChild( + context, browseName, createOrReplace, replacement, assignInstanceNodeIds); } /// @@ -1210,7 +1205,8 @@ protected override void RemoveExplicitlyDefinedChild(global::Opc.Ua.BaseInstance case "{{Tokens.ChildBrowseNameLiteral}}": { instance = !createOrReplace ? - {{Tokens.ChildName}} : CreateOrReplace{{Tokens.ChildName}}(context, replacement); + {{Tokens.ChildName}} : CreateOrReplace{{Tokens.ChildName}}( + context, replacement, assignInstanceNodeIds); break; }