Fix #3768: stop emitting unimplemented Optional children on singleton instances#3828
Merged
Conversation
… on singleton instances Generator: in singleton-instance contexts only, use the type-definition's modelling rule so Optional children that StandardTypes.xml promotes to Mandatory on the type are not over-emitted into the singleton factory body. Non-singleton contexts continue to honor instance.ModellingRule, preserving valid promotions like AnalogItemType.EURange. SDK: DiagnosticsNodeManager.LoadPredefinedNodesAsync now programmatically adds the Optional children that the SDK actually implements (Server: GetMonitoredItems, ResendData, SetSubscriptionDurable, Namespaces; ServerCapabilities: Max* properties, RoleSet with AddRole/RemoveRole, OperationLimits with configured MaxNodes* properties; HistoryServerCapabilities: ServerTimestampSupported), and patches NodeIds on each added subtree (including Method InputArguments/OutputArguments) using ObjectIds/MethodIds/VariableIds constants so the singleton instance NodeIds are correct. ConfigurationNodeManager: explicit CreateSelfSignedCertificate method-state assignment before Create(). Adds ServerConfiguration_OptionalUnimplementedChildren_NotInAddressSpaceAsync regression test that boots the reference server and confirms ApplicationUri/ProductUri/ApplicationType/HasSecureElement/CancelChanges/ResetToServerDefaults are absent from the address space.
romanett
reviewed
Jun 3, 2026
romanett
previously requested changes
Jun 3, 2026
Issue OPCFoundation#3768 follow-up: extend the gate beyond depth-1 so deep Optional Variable/Method descendants of top-level singleton instances are also suppressed (Server.ServerCapabilities.MaxArrayLength, OperationLimits.MaxNodesPerRead, ServerRedundancy.RedundantServerArray, Server.UrisVersion / EstimatedReturnTime / LocalTime, the PublishSubscribe.AddConnection / RemoveConnection methods, etc.). Generator change: - Add IsUnderSingletonInstance to NodeToGenerate. Set true on the singleton-instance root in CollectNodesToGenerate, propagate to children in GetChildren, set false on type-def and synthetic type-instance roots. Structural propagation, mirrors RootIsTypeDefinition. - Replace the depth-1 walk-up predicate with `node.IsUnderSingletonInstance && node.Parent != null && instance is VariableDesign or MethodDesign`. The narrowing exempts Optional Object instances (e.g. ServerConfiguration.CertificateGroups.DefaultHttpsGroup, Server.ServerCapabilities.OperationLimits / RoleSet, Server.Namespaces) because their subtrees rely on well-known descendant NodeIds that the SDK and ConfigurationNodeManager bind against. Patching every descendant NodeId from the SDK after a type-level factory call would require hard-coded mapping per Object root and is brittle. Optional Variable/Method children of those Objects are still gated transitively (e.g. OperationLimits.MaxNodesPerRead is suppressed). SDK add-back in DiagnosticsNodeManager: - AddServerCapabilitiesSdkOptionalChildren lazy-adds the 14 Optional ServerCapabilities Properties consumed by ServerInternalData (MaxArrayLength, MaxStringLength, MaxByteStringLength, MaxSessions, MaxSubscriptions, MaxMonitoredItems, MaxSubscriptionsPerSession, MaxMonitoredItemsPerSubscription, MaxSelectClauseParameters, MaxWhereClauseParameters, MaxMonitoredItemsQueueSize, ConformanceUnits, RoleSet, OperationLimits), patching each to the well-known VariableIds.Server_ServerCapabilities_* / ObjectIds. Server_ServerCapabilities_* identifier. - AddOperationLimitsSdkOptionalChildren lazy-adds the 12 OperationLimits Properties read by Session.FetchOperationLimitsAsync and patches their NodeIds. - AddServerRedundancySdkOptionalChildren lazy-adds RedundantServerArray (consumed by DefaultServerRedundancyHandler on the client). - Dispatch from AddServerSdkOptionalChildren (guarded on the serverCapabilities / serverRedundancy slots being non-null). Permanent test (ConfigurationNodeManagerTests. ServerConfiguration_OptionalUnimplementedChildren_NotInAddressSpaceAsync): - Add negative assertions for Server.UrisVersion / EstimatedReturnTime / LocalTime and PublishSubscribe.AddConnection / RemoveConnection. - Add positive NodeId assertions for every newly-SDK-added Optional child (ServerCapabilities + OperationLimits + RedundantServerArray). - Add CertificateGroups regression guard: DefaultApplicationGroup CertificateTypes resolves at its well-known NodeId and the Optional DefaultHttpsGroup / DefaultUserTokenGroup roots remain emitted (Object exemption).
The depth-1 OPCFoundation#3768 fix (af599dc) and my transitive extension (c79150d) both suppressed Optional Variables/Methods on NodeSet-loaded singletons that the SDK and conformance tests depend on. CI surfaced regressions across Core.Security RoleManagement, Features RoleManagementClient, InformationModel BaseInfoServerTests, and Features MonitoredItemAliasNameRefreshStrategy. This commit adds the missing SDK add-back for each affected singleton and fixes a gate bug that incorrectly suppressed type-def factory emission. Generator fix (NodeStateGenerator.cs): - topLevelInstanceException no longer disables itself for TYPE-DEF roots when UseTypeDefinitionModellingRules is on. Only suppress when parent.Design is InstanceDesign (i.e. a singleton-instance override). Type-def factories must keep emitting Optional children unconditionally - e.g. CreateNamespaceMetadataType is called by ConfigurationNodeManager.CreateNamespaceMetadataStateAsync with forInstance=true and relies on DefaultRolePermissions / DefaultUserRolePermissions etc. being in the always-emitted list. SDK add-back (DiagnosticsNodeManager.cs): - AddServerSdkOptionalChildren now lazy-adds Server.UrisVersion, EstimatedReturnTime and LocalTime (Optional Variables that InformationModel.BaseInfoServerTests.ReadEstimatedReturnTimeValueAsync expects). - AddWellKnownRoleSdkOptionalChildren re-adds the 11 standard RoleType children (ApplicationsExclude, Applications, EndpointsExclude, Endpoints, CustomConfiguration plus the 6 AddIdentity / RemoveIdentity / AddApplication / RemoveApplication / AddEndpoint / RemoveEndpoint methods) for each of the 6 modifiable well-known roles (Observer, Operator, Engineer, Supervisor, ConfigureAdmin, SecurityAdmin), patching every NodeId (including InputArguments) to the well-known instance identifier. The three immutable roles (Anonymous, AuthenticatedUser, TrustedApplication) have no well-known NodeIds for these methods in the spec and are left untouched. - AddOpcUaNamespaceMetadataSdkOptionalChildren re-adds DefaultRolePermissions / DefaultUserRolePermissions / DefaultAccessRestrictions on the standard OPCUANamespaceMetadata singleton (i=15957) declared in Opc.Ua.NodeSet2.xml. SDK add-back (DiagnosticsNodeManager.AliasNames.cs): - AddAliasNameCategorySdkOptionalChildren re-adds LastChange on the standard Part 17 Aliases singleton (i=23470). This is required by AliasNameResolverRefreshMode.AutoOnLastChangeMonitoredItem in Opc.Ua.Client which subscribes to LastChange to invalidate its cache. Test updates (ConfigurationNodeManagerTests.cs): - Dropped Server.UrisVersion, EstimatedReturnTime and LocalTime from the negative-assertion list (now SDK-added) and added them to the positive NodeId-resolution list. - Added negative assertions for the unimplemented PublishSubscribe.AddConnection / RemoveConnection methods. - Added positive NodeId assertions for the 14 ServerCapabilities Optional Properties, 12 OperationLimits Properties, RedundantServerArray, the WellKnownRole regression guards (one sample method per modifiable role: Observer.AddIdentity, Observer.ApplicationsExclude, Observer.AddApplication, Operator.AddEndpoint, Engineer.RemoveIdentity, Supervisor.RemoveApplication, ConfigureAdmin.RemoveEndpoint, SecurityAdmin.AddIdentity), and the CertificateGroups regression guard (DefaultApplicationGroup.CertificateTypes, DefaultHttpsGroup, DefaultUserTokenGroup).
# Conflicts: # Libraries/Opc.Ua.Server/Diagnostics/DiagnosticsNodeManager.cs # Tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs
Address review feedback from @romanett on PR OPCFoundation#3828: "This Input / Output argument handling should already be handled by the generated AddGetMonitoredItems()" The generated Add{Method}() helpers on the typed state classes (e.g. ServerObjectState.AddGetMonitoredItems, RoleState.AddAddIdentity) already construct the InputArguments / OutputArguments child via the type-level factory. Patching the InputArguments / OutputArguments NodeIds to the singleton-instance values is unnecessary because clients browse method arguments by browse-name; the canonical method NodeId is the only one that has to match for OPC UA method dispatch to work. Simplified: - AddServerSdkOptionalChildren: GetMonitoredItems / ResendData / SetSubscriptionDurable lazy-add now patches only the method NodeId. - AddWellKnownRoleChildren: 6 method add-backs per role now patch only the method NodeId; the helper signature drops the six *Args parameters across all six modifiable well-known roles. - ConfigurationNodeManagerTests.ServerConfiguration_OptionalUnimplementedChildren_NotInAddressSpaceAsync: the SDK-added NodeId list no longer asserts Server_GetMonitoredItems _InputArguments / _OutputArguments / Server_ResendData_InputArguments (they remain at type-level NodeIds which is OPC UA-compliant).
Collaborator
Author
|
The CTT-mode regression you flagged on
Running the reference server with |
romanett
reviewed
Jun 5, 2026
romanett
reviewed
Jun 5, 2026
romanett
reviewed
Jun 5, 2026
Address follow-up review from @romanett on PR OPCFoundation#3828: "the If ... == null should be unnecessay [...] This should be a clean serverobect.Add...().Add..().Add..() fluent call" Generator template (NodeStateTemplates.cs): - OptionalMethod now emits an idempotent Add{Child}() that returns the existing typed child if already present (null-coalescing assignment), so callers can call it unconditionally. SDK callers (DiagnosticsNodeManager.cs + HistoricalDataConfigurationInstaller.cs): - Dropped every now-redundant `if (... == null)` null guard around Add{Child}.NodeId = ... in AddServerSdkOptionalChildren, AddServerCapabilitiesSdkOptionalChildren, AddOperationLimitsSdkOptionalChildren, AddServerRedundancySdkOptionalChildren, AddHistoryCapabilitiesSdkOptionalChildren, AddOpcUaNamespaceMetadataSdkOptionalChildren, AddAliasNameCategorySdkOptionalChildren and AddWellKnownRoleChildren. - HistoricalDataConfigurationInstaller now calls Add{Child}(context).Value = ... directly instead of `?? = ` ... `?.Value = ` ceremony. - Net change: ~140 lines of boilerplate removed; behavior unchanged. Test fix (DiagnosticsNodeManagerTests.cs): - CreateAddressSpace_HooksUpGetMonitoredItemsAndResendDataAsync now looks up OutputArguments via the typed `.OutputArguments` slot instead of `FindPredefinedNode<PropertyState>(VariableIds. Server_GetMonitoredItems_OutputArguments)`. The argument children carry type-level NodeIds (the SDK no longer patches them, per the previous review iteration); the typed slot lookup is the correct way to find the child regardless of its NodeId. Note on the reviewer's other suggestion ("source generator should assign the Instance NodeId if you call .Add()") - that requires the generator to know which typed state class is used by which singleton instance and emit a dispatch (parent.NodeId switch) at construction time. That refactor is tracked separately; the current SDK keeps the explicit `.NodeId = MethodIds.Server_GetMonitoredItems` patch to match master's well-known instance NodeIds.
…ethods Address review feedback from @romanett on PR OPCFoundation#3828 (comment 3361416660 on ConfigurationNodeManager.cs:210): "use activeNode.AddCreateSelfsignedCertifcateMethodState()" Replace manual typed-state construction with the idempotent generated AddXxx helpers: - activeNode.GetCertificates = new GetCertificatesMethodState(activeNode); + activeNode.AddGetCertificates(context); - activeNode.CreateSelfSignedCertificate = new CreateSelfSignedCertificateMethodState(activeNode); + activeNode.AddCreateSelfSignedCertificate(context); Add{Method} (now idempotent per the previous commit) initialises the typed slot with the type-level factory (BrowseName / InputArguments / OutputArguments / DataType etc.) before activeNode.Create(context, passiveNode) copies the loaded passive subtree (singleton-instance NodeIds, declared values) on top. PushCertManagement tests pass (50/53, 3 pre-existing skips); ConfigurationNodeManagerTests pass (4/4).
Per user feedback on PR OPCFoundation#3828: "A user adds optional children using .AddXY(NodeId? nodeId = default). The Add calls should be possible to chain. Where necessary source generator should be used/updated to achieve this." Generator changes (NodeStateTemplates.OptionalMethod): - Emit TWO helpers per Optional child: * Add{Child}(context, NodeId? nodeId = default) returns the owner (parent) so calls can be chained: serverObject .AddGetMonitoredItems(context, MethodIds.Server_GetMonitoredItems) .AddResendData(context, MethodIds.Server_ResendData) .AddNamespaces(context, ObjectIds.Server_Namespaces)... * AddAndGet{Child}(context, NodeId? nodeId = default) returns the typed child for `.Value = ...` / configuration in-line. - Both are idempotent (no-op if the child already exists, except for the optional NodeId patch). - When `nodeId` is null: * if context.NodeIdFactory is non-null, mint a fresh NodeId via context.NodeIdFactory.New(context, child); * otherwise leave the type-level NodeId assigned by the generated factory unchanged (caller can patch later). New token Tokens.OwnerClassName carries the owning typed state class name through the per-child template loop so the chainable Add{Child} method can return `this` typed as the owner. Set in AddNodeStateClassTypeReplacements (Object/Variable type scope) and AddNodeStateClassMethodTypeReplacements (Method type scope) and cascaded to the inner OptionalMethod template via the template engine's outer-scope lookup. SDK migrations: - DiagnosticsNodeManager.AddServerSdkOptionalChildren, AddServerCapabilitiesSdkOptionalChildren, AddOperationLimitsSdkOptionalChildren, AddServerRedundancySdkOptionalChildren, AddHistoryCapabilitiesSdkOptionalChildren, AddOpcUaNamespaceMetadataSdkOptionalChildren, AddAliasNameCategorySdkOptionalChildren and AddWellKnownRoleChildren all collapse to fluent Add{Child}(context, nodeId) chains. - ConfigurationNodeManager chains AddGetCertificates + AddCreateSelfSignedCertificate on activeNode before Create. - HistoricalDataConfigurationInstaller switches to the new AddAndGet{Child}(context).Value = ... shape (drops the config.X ??= config.AddX(context) ceremony). - KeyCredentialPushSubject, WotConnectivityNodeManager, AssetRegistry, AlarmConditionTypeHolder all update from state.X ??= state.AddX(context) to plain state.AddX(context) + null-forgiving access on the typed slot. Validation (net10): - Server.Tests 1870 / 1875 (5 pre-existing skips). - Core.Security.Tests focused suite (RoleManagement / Base2Default / DefaultPerms / PushCertManagement) 92 / 97 (5 pre-existing skips). - Features.Tests focused suite (RoleManagement / MonitoredItemStrategy) 10 / 10. - SourceGeneration.Core.Tests 3536 / 3544 (8 long-running skips). - DiagnosticsNodeManagerTests + ConfigurationNodeManagerTests 20 / 20.
romanett
reviewed
Jun 8, 2026
Drop the per-child `AddAndGet{Child}(ISystemContext, NodeId?)` helper
emitted by the source generator. In its place every Optional child
gets four additional `Add{Child}` overloads that keep the chainable
`return this` shape:
* `Add{Child}(context, Action<TChild> configure, NodeId? = default)`
* `Add{Child}(context, bool condition, Action<TChild>, NodeId? = default)`
* `Add{Child}(context, Func<TChild, TChild> configure, NodeId? = default)`
* `Add{Child}(context, bool condition, Func<TChild, TChild>, NodeId? = default)`
The Action overloads mutate the just-created child in-line; the Func
overloads can transform or replace the typed slot. The boolean
overloads short-circuit when the condition is false (no Add, no
callback). All five overloads return the owner so multi-child setup
collapses into a single fluent chain.
Migrate all nine `AddAndGet*` call sites:
* HistoricalDataConfigurationInstaller.PopulateProperties is now a
single chain using the conditional Action overload.
* DiagnosticsNodeManager folds the ServerCapabilities / OperationLimits
/ SetSubscriptionDurable wiring into chains.
* KeyCredentialPushSubject moves `OnCall = null; OnCallAsync = ...`
into Action callbacks.
* WotConnectivityNodeManager chains the management-method wiring and
the ApplyConfiguration license slot.
DeviceBuilder.GetOrCreateGroupAsync assigned the result of
topology.AddIdentification(Context) to a FunctionalGroupState local.
Now that the generated Add{Child} overloads return the owning
TopologyElementState (chainable), the implicit conversion fails with
CS0029. Switch to the explicit slot pattern: call AddIdentification
and then read the populated typed slot back via topology.Identification.
PumpNodeManager.MaterialisePumpOptionalChildren assigned the result of
pump.AddOperational/AddEvents and operational.AddMeasurements to
typed locals. Now that the generated Add{Child} overloads return the
owning state (chainable), the implicit conversion fails with CS0029.
Switch to the explicit slot pattern: call AddX(SystemContext) then
read the typed slot back via parent.X! for downstream wiring; the
analog leaves under Measurements collapse into a single fluent
chain.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a source-generator regression where singleton instance factories (e.g., Server, ServerConfiguration, well-known roles, Part 17 Aliases) were emitting Optional Variable/Method children that the SDK does not implement, and introduces a new fluent, chainable Add{Child} API to let the SDK (and users) add only the Optional children they actually support at runtime.
Changes:
- Update the NodeState source generator to suppress Optional Variable/Method emission under singleton-instance subtrees when
UseTypeDefinitionModellingRulesis enabled (while preserving type-definition factory behavior). - Replace
AddAndGet{Child}with chainableAdd{Child}overloads (including configure-callback and conditional variants) and migrate call sites to the new fluent pattern. - Add/strengthen server tests to validate suppressed vs. SDK-readded well-known singleton children and NodeId correctness.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Tools/Opc.Ua.SourceGeneration.Core/Templating/Tokens.cs | Adds OwnerClassName token to support fluent Add{Child} return typing. |
| Tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateTemplates.cs | Reworks Optional-child helpers into fluent Add{Child} overload set. |
| Tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs | Implements singleton-instance transitive suppression via IsUnderSingletonInstance + factory emission gating. |
| Tools/Opc.Ua.SourceGeneration.Core/GeneratorOptions.cs | Clarifies UseTypeDefinitionModellingRules behavior in docs. |
| Libraries/Opc.Ua.Server/Diagnostics/DiagnosticsNodeManager.cs | Adds SDK “add-back” of implemented Optional children for well-known singletons with instance NodeId patching. |
| Libraries/Opc.Ua.Server/Configuration/ConfigurationNodeManager.cs | Uses new fluent Add{Method} helpers before Create() for ServerConfiguration methods. |
| Libraries/Opc.Ua.Server/Server/ServerInternalData.cs | Adjusts ServerCapabilities/OperationLimits value population to align with lazy-added nodes. |
| Libraries/Opc.Ua.Server/KeyCredential/KeyCredentialPushSubject.cs | Migrates wiring of optional methods to fluent Add{Child}(…, configure) overloads. |
| Libraries/Opc.Ua.Server/Historian/HistoricalDataConfigurationInstaller.cs | Simplifies optional property population via conditional fluent Add{Child} chain. |
| Libraries/Opc.Ua.WotCon.Server/WotConnectivityNodeManager.cs | Migrates optional method/property wiring to fluent Add{Child} chains. |
| Libraries/Opc.Ua.WotCon.Server/Assets/AssetRegistry.cs | Uses idempotent Add{Child} + null-forgiving slot access. |
| Libraries/Opc.Ua.Di.Server/Builders/DeviceBuilder.cs | Updates for new AddIdentification return type (owner vs child). |
| Applications/Quickstarts.Servers/Alarms/AlarmHolders/AlarmConditionTypeHolder.cs | Migrates optional child materialization to new idempotent Add{Child} behavior. |
| Applications/PumpDeviceIntegrationServer/PumpNodeManager.cs | Migrates optional child materialization to fluent Add{Child} chaining. |
| Tests/Opc.Ua.Server.Tests/DiagnosticsNodeManagerTests.cs | Adjusts assertions to reflect new OutputArguments lookup behavior. |
| Tests/Opc.Ua.Server.Tests/ConfigurationNodeManagerTests.cs | Adds regression test covering suppressed unimplemented children and SDK add-back correctness. |
NodeStateTemplates.cs:
- Drop NodeId? (Nullable<NodeId>) from generated Add{Child} overloads in
favour of the convention-correct NodeId with !nodeId.IsNull checks.
NodeId is a readonly INullable struct; Nullable<T> over INullable
structs mixes the null/.IsNull semantics. (Copilot review)
- Fix XML doc example to include the required context parameter. (Copilot review)
ConfigurationNodeManager.cs:
- Only construct a new ServerConfigurationState when the passive predefined
node isn't already one - reuse it in place when possible so the
predefined node loader's typed instance is preserved. (romanett)
DiagnosticsNodeManager.cs:
- Update the AddServerCapabilitiesSdkOptionalChildren comment to describe
the .AddOperationLimits(...).OperationLimits! chaining now in use; the
old comment still referenced the removed AddAndGetOperationLimits helper.
(Copilot review)
ServerInternalData.cs:
- Restore Part 5 6.3.4 behaviour: any exposed operational-limit Property
must have a non-zero value. Introduce ApplyOrHide that sets the value
when non-zero and hides (nulls out) the typed slot when the configured
value is 0 (the default unlimited). (Copilot review)
ConfigurationNodeManagerTests.cs:
- Grammar fix in assertion message (as an SDK does not -> because the
SDK does not). (Copilot review)
The reuse path (passive node already typed -> skip Create/ReplaceChild) broke the standard Configuration browse paths in CI: - PushCertManagementTests browse for ServerConfiguration/UpdateCertificate etc. found nothing because the typed activeNode was never wired into the parent's child slot when the loader handed us a BaseObjectState. - GDS PushTest.GetCustomGroupIdAsync recursive browse found fewer than the expected 4 CertificateGroups, suggesting the same wiring path is load-bearing for the CertificateGroups subtree. Restore the pre-cbbebcd35 behaviour: always allocate a new ServerConfigurationState and run Create + ReplaceChild. Local PushCertManagementTests pass 6/6 and full Gds PushTest passes 26/26 with the revert. See review comment 3370495747 (romanett: "Only create If not ServerConfigurationState"). The reuse approach needs more investigation to avoid the wiring regression; out of scope for the CI-fix loop.
ConfirmBranchAsync and ConfirmConditionSetsConfirmedStateTrueAsync
called RequireAlarm() with no type filter, which picked the first
alarm via Dictionary iteration order. The pick was unstable across CI
runs:
- The `AcknowledgeableConditionHolder` (Quickstarts.Servers) is created
with `optional: false` so its address-space node does NOT expose the
Confirm method - calling Confirm against that instance returned
`BadMethodInvalid` (0x80750000), failing the tests intermittently.
- AlarmConditionType subtypes (created with `optional: true`) DO
expose Confirm.
Switch both tests to `RequireAlarm(""AlarmConditionType"")` so the
selected alarm is guaranteed to support Confirm. Locally both tests
pass deterministically.
PROBLEM
Templates in Tools/Opc.Ua.SourceGeneration.Core interpolate the design
model's BrowseName values directly into generated C# inside string-
literal positions, e.g.:
case "{{Tokens.ChildBrowseName}}":
Name = "{{Tokens.BrowseName}}",
public const string {{SymbolicName}} = "{{Tokens.BrowseName}}";
If a third-party design XML pulled into a custom build ever has a
BrowseName containing `"`, `\`, or a control character the
generator produces an unbalanced literal and the consuming build fails
with a confusing CS1010 / CS1003 error. Defence-in-depth only — the
generator only consumes design XML from the local repo at build time,
and anyone able to edit design XML can already write arbitrary C#.
CHANGES
* Tools/Opc.Ua.SourceGeneration.Core/Templating/StringLiteralEscaper.cs
(new) - pure helper `AsCSharpStringLiteralContent(raw[, out modified])`.
Escapes `\`, `"`, `\r`, `\n`, `\t`, and any control character
`< 0x20` or `== 0x7F`. Returns the literal *content*; the
surrounding `"…"` quotes stay in the template.
* Tools/Opc.Ua.SourceGeneration.Core/Templating/BrowseNameTemplateExtensions.cs
(new) - `Template.AddBrowseNameReplacement(rawToken, literalToken,
value, logger?)` populates both the raw (identifier context) and
literal (string-literal context) tokens and surfaces a
`UASG_BROWSENAME_UNSAFE` warning via the host generator's
ILogger when the value required escaping. `EventId(20,
"BrowseNameUnsafe")`.
* Tools/Opc.Ua.SourceGeneration.Core/Templating/Tokens.cs - new
`BrowseNameLiteral` and `ChildBrowseNameLiteral` tokens. The
existing `BrowseName` / `ChildBrowseName` tokens are kept for
identifier-context interpolations (which are validated upstream).
* Templates updated to use the literal token wherever the value lands
inside a generated C# `"…"` literal:
- ConstantsTemplates.BrowseName ("MyBrowseName" -> literal)
- DataTypeTemplates.ScalarProperty / ArrayProperty
(DataMember Name="…" -> literal)
- DataTypeTemplates.AddPropertyToStringBuilder
(.AppendFormat("Name={0}") -> literal)
- NodeStateTemplates.FindChildCase
(case "BrowseName" -> ChildBrowseNameLiteral)
- ObjectTypeProxyTemplates.ChildAccessor
(ResolveChildNodeIdAsync("…","BrowseName") -> literal)
* Every Add{BrowseName,ChildBrowseName} call site in the generators
switched to `AddBrowseNameReplacement` so the matching `Literal`
token is populated and the warning surfaces:
- ConstantsGenerator, DataTypeGenerator, NodeStateGenerator,
ObjectTypeProxyGenerator, TypeSourceGenerator.
* Diagnostic descriptors:
- Tools/Opc.Ua.SourceGeneration/SourceGenerator.cs - new
`MODELGEN020 BrowseNameUnsafe` warning (mapped via
`TryGetDiagnostic` for `EventId.Id == 20`).
- Tools/Opc.Ua.SourceGeneration.Stack/SourceGenerator.cs - mirror
`STACKGEN020 BrowseNameUnsafe`.
- AnalyzerReleases.Unshipped.md updated in both generator projects.
TESTS
* Tests/Opc.Ua.SourceGeneration.Core.Tests/Templating/StringLiteralEscaperTests.cs
(new) - 15 tests including a Roslyn-round-trip fuzz that wraps the
escaped content in `"…"`, parses with `CSharpSyntaxTree`, and
verifies the parsed literal equals the input.
* Tests/Opc.Ua.SourceGeneration.Core.Tests/Templating/BrowseNameTemplateExtensionsTests.cs
(new) - 7 tests covering the helper contract end-to-end:
- Safe value: both tokens populated identically, no warning.
- Quote / control char / backslash: literal escaped, warning fires
with `EventId(20, "BrowseNameUnsafe")`.
- Null value: empty tokens, no warning.
- Null logger: helper doesn't throw.
- FindChildCase template integration: render the actual generator
template with a hostile child browse name, parse the result
inside a synthetic switch via Roslyn, assert no errors and the
case label parses to the original value.
* Existing TemplateTests.WriteTemplate_WithBrowseNameTemplate_RendersCorrectly
and AddReplacement_IntegrationWithRender_ReplacesTokenCorrectly
updated to use `AddBrowseNameReplacement` since the BrowseName
template now uses `Tokens.BrowseNameLiteral` in its string-literal
slot.
VALIDATION
* Opc.Ua.SourceGeneration.Core / .Stack / .Tests / .Core.Tests build
with 0 warnings, 0 errors on net10.0.
* Tests/Opc.Ua.SourceGeneration.Core.Tests: 3558 / 3558 pass on
net10.0 (8 long-running compile-cycle integration tests skipped by
the framework).
* Tests/Opc.Ua.SourceGeneration.Stack.Tests: 90 / 90 pass on net10.0.
* Stack/Opc.Ua.Core.Types, Stack/Opc.Ua.Core, Libraries/Opc.Ua.Server
all rebuild clean on net10.0 — generated code is unchanged for safe
BrowseNames.
PROBLEM
AssetRegistry.EnumeratePersistedAsync walked
ThingDescriptionStorageFolder with no per-file size cap, no file-count
cap, and no JSON-depth bound. A malicious or corrupted persistence
directory could wedge server startup through CPU / memory / stack
exhaustion. MaxThingDescriptionSize already existed (1 MiB default)
but was only enforced on the write side; the load path ignored it.
CHANGES
* Libraries/Opc.Ua.WotCon.Server/WotConnectivityServerOptions.cs
- New `MaxPersistedThingDescriptionFiles` (default 10 000) caps the
number of files processed at startup. <= 0 acts as an explicit
kill switch (the loader logs an Information event and returns
immediately so an operator can disable directory loading without
removing the folder).
- New `MaxThingDescriptionJsonDepth` (default 64) feeds through to
`JsonSerializerOptions.MaxDepth` so pathologically nested JSON
cannot stack-overflow the server.
- XML doc comments on `MaxThingDescriptionSize` updated to call
out that the limit is now enforced on both write and read paths.
* Libraries/Opc.Ua.WotCon.Server/Assets/AssetRegistry.cs
- `EnumeratePersistedAsync` enforces all three bounds:
1. file-count cap: a single warning naming the limit; well-formed
assets discovered before the cap are still loaded.
2. per-file size cap via `FileInfo.Length` (no stream open for
oversize files).
3. JSON depth via a fresh `ThingDescriptionJsonContext` wired
to a `JsonSerializerOptions` copy with `MaxDepth` set;
the cached singleton context is reused when the configured
depth matches the default to keep the hot path allocation-
free. AOT-safe — the source-generated context exposes a
(JsonSerializerOptions) constructor.
- Catch filter tightened: `OperationCanceledException` is
rethrown, `JsonException` and `IOException` are skipped with
a per-file warning that explains the failure mode, no other
exception is silently swallowed.
- New `GetBoundedJsonContext` helper for clarity.
* Docs/WoTConnectivity.md - new section `4. Persistence limits`
documenting the three options, defaults, kill-switch semantics, an
override example, and the cancellation contract.
TESTS
* Tests/Opc.Ua.WotCon.Tests/AssetRegistryPersistenceBoundsTests.cs
(new, 7 NUnit tests):
- Happy path: well-formed TD restored, no warning.
- Oversize file: skipped with warning naming the file/size.
- File-count limit: exactly N entries yielded with N+5 on disk.
- File-count kill switch (limit = 0): nothing yielded.
- Over-deep JSON: skipped with `JsonException` warning, not
thrown.
- Malformed JSON: skipped, not thrown.
- Cancellation: `OperationCanceledException` propagates.
VALIDATION
* Opc.Ua.WotCon.Server + .Tests build with 0 warnings, 0 errors on
both net10.0 and net48.
* Tests/Opc.Ua.WotCon.Tests: 213 / 213 pass on net10.0 (213 was the
pre-PR baseline; the new tests bring the count up from 206).
* Tests/Opc.Ua.Aot.Tests builds clean with PublishAot=true,
IsAotCompatible=true on Opc.Ua.WotCon.Server; the new
`new ThingDescriptionJsonContext(options)` path uses the
source-generated constructor and is AOT-safe.
PROBLEM
BuildPropertyNode / BuildActionNode in AssetRegistry took the raw key
from td.properties / td.actions and used it to:
* compute a NodeId path segment via m_manager.AllocateChildNodeId,
* set the QualifiedName browse name,
* set the BaseInstanceState SymbolicName.
Pathological names (path separators, control chars, > 128 chars,
duplicate-after-normalisation collisions, or visual-spoofing RTL
overrides) could corrupt the address space, hide nodes, or break
browse / TranslateBrowsePath resolution.
CHANGES
* Libraries/Opc.Ua.WotCon.Server/Assets/WotChildNameValidator.cs
(new) - sibling of WotAssetNameValidator focused on OPC UA
semantics rather than file-system safety. Rejects:
- null / empty / > 128 chars,
- leading / trailing whitespace (Unicode-aware via char.IsWhiteSpace),
- any char.IsControl character (incl. NUL, DEL),
- BIDI / format chars (LRM, RLM, LRE/RLE/PDF/LRO/RLO, LRI/RLI/FSI/PDI),
- / \ . # : ! (NodeId / browse-path / file-system separators).
Plus a SanitiseForLog helper that replaces every control / BIDI char
with a U+XXXX placeholder and truncates at MaxLength so a hostile
name cannot reshape the rendered log line.
* Libraries/Opc.Ua.WotCon.Server/Assets/AssetRegistry.cs
- New TryValidateChildName helper - logs one warning per rejection
(sanitised) and skips the child.
- RebuildAsync filters every td.Properties key and td.Actions key
through the validator BEFORE BuildPropertyNode / BuildActionNode
fire, and also rejects case-sensitive duplicate names. The valid
children still materialise so one bad TD entry does not poison
the whole asset.
- XML doc on RebuildAsync points operators at the validator and the
skip-and-warn contract.
* Docs/WoTConnectivity.md - new section `4. Name validation` listing
both validators (existing asset-name + new child-name), the rule
sets, sanitisation contract, and the duplicate-key behaviour.
TESTS
* Tests/Opc.Ua.WotCon.Tests/WotChildNameValidatorTests.cs (new, 11
TestFixture cases): happy paths (`temperature`, `Set-Point`,
`value_1`); rejects on empty / null / 129-char / path-sep /
browse-path tokens / control chars / DEL / BIDI overrides / leading
/ trailing whitespace; SanitiseForLog escapes + truncates.
* Tests/Opc.Ua.WotCon.Tests/WotConnectivityNodeManagerTests.cs
- RebuildAsync_SkipsTdChildrenWithInvalidNames_AndMaterialisesValidOnes:
mixed TD (7 props + 3 actions) materialises exactly the two safe
props (`Voltage`, `Set-Point`) and one safe action (`Reset`).
- RebuildAsync_TooLongChildName_IsSkipped: 129-char key is filtered;
sibling `Voltage` still lands.
VALIDATION
* Opc.Ua.WotCon.Server + .Tests build clean (0 warnings, 0 errors) on
both net10.0 and net48.
* Tests/Opc.Ua.WotCon.Tests: 242 / 242 pass on net10.0 (213 was the
pre-PR baseline + 29 new tests).
PROBLEM
CreateAssetForEndpoint and ConnectionTest accept an endpoint URI
from a remote OPC UA client and pass it directly to
IWotAssetDiscoveryProvider with no scheme check, no allow-list, no
private-IP block, and no timeout enforcement. A privileged client
(or, with the management-method gating still pending, any client)
could probe loopback / RFC1918 / link-local addresses or pull content
from attacker-controlled URLs. The 169.254.169.254 IMDS attack
surface was wide open.
CHANGES
* Libraries/Opc.Ua.WotCon.Server/Assets/AssetEndpointPolicy.cs
(new) - per-server policy holding:
- AllowedSchemes (default: http, https, opc.tcp)
- AllowLoopback (default false)
- AllowPrivateAddresses (default false)
- AllowedHosts (exclusive allow-list when non-empty)
- BlockedHosts (always-deny on top of the allow-list)
- MaxOperationTimeout (default 30 s; Zero disables)
* Libraries/Opc.Ua.WotCon.Server/Assets/AssetEndpointValidator.cs
(new) - pure validator returning BadInvalidArgument (malformed
URI) or BadSecurityChecksFailed (policy violation). Defence-in-
depth blocks IPv4 RFC1918 + RFC3927 link-local (incl. 169.254/16
IMDS), IPv6 RFC4193 ULA + RFC4291 link-local, loopback, and the
literal hostnames localhost / ip6-localhost / ip6-loopback.
Intentionally does NOT perform DNS resolution - resolving at
validation and re-resolving at connect is itself a TOCTOU SSRF
vector. Doc-commented as such.
* Libraries/Opc.Ua.WotCon.Server/WotConnectivityServerOptions.cs
- New AssetEndpointPolicy property (defaults to a fresh safe
policy instance).
* Libraries/Opc.Ua.WotCon.Server/Assets/AssetRegistry.cs
- CreateAssetForEndpointAsync + ConnectionTestAsync run the
endpoint through the validator before touching the discovery
provider. On rejection: log a warning, return immediately, never
invoke the provider.
- Both methods wrap the provider call in a
RunWithPolicyTimeoutAsync helper that builds a linked
CancellationTokenSource with CancelAfter(MaxOperationTimeout).
The caller's cancellation token still flows through, so
user-initiated cancellation propagates unchanged; only a
policy-timeout that fires while the caller's token is still
alive is mapped to BadTimeout.
- The discovery provider sees the *normalized* AbsoluteUri so it
cannot re-interpret the raw string.
* Docs/WoTConnectivity.md - new section `4. Endpoint policy`
with defaults, override example, TOCTOU/DNS security note, and
the per-operation timeout contract.
TESTS
* Tests/Opc.Ua.WotCon.Tests/AssetEndpointValidatorTests.cs (new,
~40 table-driven cases): allowed schemes, scheme rejection,
invalid URI syntax, loopback rejection (incl. ::1 / localhost /
ip6-localhost), RFC1918 + IPv6 ULA + link-local rejection
(explicit IMDS test case at 169.254.169.254), no over-reach into
adjacent public ranges (11/8, 172.15/16, 172.32/16, 192.169/16,
2001:db8::/32), AllowLoopback / AllowPrivateAddresses overrides,
AllowedHosts exclusive allow-list, BlockedHosts always-deny,
null-policy ArgumentNullException, normalized-URI preserves
scheme + lowercases host.
* Tests/Opc.Ua.WotCon.Tests/AssetRegistrySsrfTests.cs (new): drive
CreateAssetForEndpointAsync + ConnectionTestAsync via a strict
Moq IWotAssetDiscoveryProvider and verify the provider is NEVER
invoked for hostile endpoints (file:, javascript:, 127.0.0.1,
::1, localhost, 10/8, 192.168/16, IMDS). Plus three runtime
tests: provider timeout maps to BadTimeout, caller cancellation
propagates as OperationCanceledException (not BadTimeout), zero
policy timeout disables the bound.
* Tests/Opc.Ua.WotCon.Tests/WotConnectivityNodeManagerTests.cs
- ManagerHarness opts the test sim:// scheme into the policy's
AllowedSchemes so the existing
DiscoverAssetsForwardsToConfiguredProvider /
ConnectionTestForwardsToConfiguredProvider happy-path tests
keep working.
VALIDATION
* Opc.Ua.WotCon.Server + .Tests build clean (0 warnings, 0 errors)
on net10.0.
* Tests/Opc.Ua.WotCon.Tests: 263 / 263 pass on net10.0 (242 was
the pre-PR baseline + 21 new tests).
PROBLEM
AssetRegistry returned raw ex.Message strings inside ServiceResult to
remote OPC UA clients (CreateAssetForEndpoint, DiscoverAssets,
ConnectionTest, RebuildAsync provider-connect path, per-property /
per-action data-plane paths, persistence load paths). The raw
messages can leak internal endpoint URIs, file-system paths, provider
implementation details, and stack-trace fragments — none of which an
unauthenticated remote caller should ever see.
CHANGES (AssetRegistry.cs only)
* New private helpers (file-bottom):
- `ToClientStatus(ex, status, operation)` builds a ServiceResult
that contains only the supplied generic operation name. The
exception parameter is accepted for call-site brevity but is
deliberately ignored — no ex.Message, ex.StackTrace, or
ex.GetType().Name flows into the returned status.
- `MapToStatusCode(ex)` is the exception → StatusCode table:
NotSupportedException -> Bad_NotSupported
ArgumentException -> Bad_InvalidArgument
IOException -> Bad_ResourceUnavailable
anything else -> Bad_InternalError
OperationCanceledException is intentionally not in the table —
callers must put it in a `when`-filter so cancellation
propagates unchanged.
* Every catch site that previously returned
`ServiceResult.Create(ex, ..., ex.Message)` now:
- logs the raw exception (with asset / property / endpoint context)
via m_logger at LogError (control plane) or LogWarning (data
plane).
- returns `ToClientStatus(ex, MapToStatusCode(ex), operation)`.
- has a `when (ex is not OperationCanceledException)` filter so
cancellation flows out unchanged.
Sites: CreateAssetForEndpointAsync (catch NotSupported + catch generic),
DiscoverAssetsAsync (catch NotSupported + catch generic),
ConnectionTestAsync (catch NotSupported + catch generic),
RebuildAsync (provider-connect catch), ReadFromProviderAsync,
WriteToProviderAsync, InvokeActionAsync.
* No behaviour change to delete-on-failure of partial assets or any
other semantics — only the LocalizedText surfaced to clients.
* Docs/WoTConnectivity.md - new section `4. Error reporting` with
the mapping table and the operator-visible contract.
* Audit pipeline: WotCon.Server has no existing audit event
infrastructure; per the task spec `leave audit to a future PR`.
TESTS
* Tests/Opc.Ua.WotCon.Tests/AssetRegistryExceptionSanitisationTests.cs
(new): For each public surface (DiscoverAssets, ConnectionTest,
CreateAssetForEndpoint), inject an IWotAssetDiscoveryProvider mock
that throws an exception whose .Message is the canary
`SECRET-INTERNAL-PATH:/etc/shadow` and assert:
- ServiceResult.LocalizedText.Text does NOT contain the canary.
- ServiceResult.AdditionalInfo does NOT contain the canary.
- The captured logger DOES contain the canary.
- The mapped status code matches the exception-to-status table.
Plus a dedicated DiscoverAssets cancellation test confirming that
OperationCanceledException propagates unchanged (not swallowed,
not re-mapped to a status code).
VALIDATION
* Local SDK 10.x install is broken in the test environment
(`dotnet --list-sdks` shows only 8.x/9.x even though 10.x folders
exist) — local build verification deferred to CI. The change is
purely a catch-block rewrite with two private helpers; no template
/ source-generator paths affected. CI builds will surface any
regression.
PROBLEM
The five OPC 10100-1 management methods on
WoTAssetConnectionManagement (CreateAsset, DeleteAsset,
DiscoverAssets, CreateAssetForEndpoint, ConnectionTest) had no
secure-channel or role gating: an anonymous client over a None-mode
channel could mutate the asset registry and trigger outbound network
activity from the server process. Mirrors the pattern enforced by
Opc.Ua.Server.ConfigurationNodeManager.HasApplicationSecureAdminAccess
for the equivalent ServerConfiguration methods.
CHANGES
* New sealed Libraries/Opc.Ua.WotCon.Server/WotManagementAccessPolicy.cs:
- RequiredRoleId (default WellKnownRole_SecurityAdmin)
- MinimumSecurityMode (default SignAndEncrypt)
- AllowAnonymous (default false)
* WotConnectivityServerOptions.ManagementAccess: defaults to a new
WotManagementAccessPolicy() so callers opt in to the restrictive
policy automatically.
* OpcUaWotConServerBuilderExtensions: ManagementAccess is now part of
the merged-options flow so DI users get the policy on the merged
WotConnectivityServerOptions instance.
* WotConnectivityNodeManager: new internal EnforceManagementAccess
invoked as the very first action of every OnCreateAssetAsync /
OnDeleteAssetAsync / OnDiscoverAssetsAsync /
OnCreateAssetForEndpointAsync / OnConnectionTestAsync. Internal
callers (those without a SessionSystemContext.OperationContext) are
exempt so startup restoration and in-process AssetRegistry tests
continue to work unchanged. On denial logs a structured warning
(operation, token type, granted roles) and throws
ServiceResultException(BadUserAccessDenied) with a generic message
that does not echo identity information back to remote callers.
* Docs/WoTConnectivity.md - new section `4. Security: management
access policy` with the defaults table and an opt-in / opt-out
code sample.
* Docs/MigrationGuide.md - new `Security tightening` subsection
under `Migrating from 1.5.378 to 2.0.x` calling out the
behaviour change and the legacy-compatibility opt-in.
TESTS
* Tests/Opc.Ua.WotCon.Tests/WotManagementAccessPolicyTests.cs (new)
- 5x5 matrix: each of the 5 operation names against {anonymous
identity / None mode / Sign-only mode / non-admin identity /
SecurityAdmin success}.
- InternalCallWithoutOperationContextIsExempt: confirms startup /
in-process callers are not blocked.
- CustomRoleAllowsCustomRoleDeniesOthers: validates RequiredRoleId
override (ConfigureAdmin instead of SecurityAdmin).
- AllowAnonymousPolicyAcceptsAnonymousWhenSecure: validates the
AllowAnonymous opt-in combined with the Anonymous well-known
role.
VALIDATION
* dotnet build Libraries/Opc.Ua.WotCon.Server (Release, net10.0)
-> 0 warnings, 0 errors (verified locally with portable
10.0.301 SDK).
* dotnet test Tests/Opc.Ua.WotCon.Tests (net10.0): 234/234 pass
(28 new + 206 pre-existing).
* dotnet test Tests/Opc.Ua.WotCon.Tests (net48): policy filter
28/28 pass.
Notable implementation detail
* WotCon's source generator emits Opc.Ua.WotCon.ObjectIds which
shadows Opc.Ua.ObjectIds inside the Opc.Ua.WotCon.Server
namespace. Use `global::Opc.Ua.ObjectIds.WellKnownRole_*` to
resolve to the parent table.
cristipogacean
approved these changes
Jun 12, 2026
marcschier
added a commit
to marcschier/UA-.NETStandard
that referenced
this pull request
Jun 13, 2026
…ew master features Incoming master PRs: * OPCFoundation#3852 - Central client channel manager (ref-counted shared channels, coalesced reconnect, IRetryBudget); also adds MCP server packet-capture / packet-decode / packet-replay tools * OPCFoundation#3869 - Unbounded monitored items per subscription via automatic partitioning (LogicalSubscription / CompositeMonitoredItemCollection / PartitionPlacementPolicy) * OPCFoundation#3872 - CI fixes (no doc impact) Conflict (only one): Docs/MigrationGuide.md. Resolution: keep the landing-page version from PR OPCFoundation#3874; extract the WoT-security tightening section that OPCFoundation#3828 wedged into the old monolithic guide into a new per-area sub-doc Docs/migrate/2.0.x/wot.md (13th thematic sub-doc), keeping the landing page small and the per-area structure consistent. Documentation refresh: * Docs/migrate/2.0.x/wot.md (NEW) - WoT management-access-policy migration content with When-to-read lead + See-also footer. * Docs/MigrationGuide.md - bumped 'sub-docs' count from 12 to 13. * Docs/migrate/2.0.x/README.md - new WoT symptom row + entry in the All-sub-documents list. * .agents/skills/opcua-v20-migration/SKILL.md - matching WoT row in the agent's symptom -> sub-doc index. * Docs/WhatsNewIn2.0.md - extended Source-generators section (new chainable Add{Child} overloads from OPCFoundation#3828), Client section (IClientChannelManager + unbounded monitored items), Tooling section (MCP packet tools), and the WoT companion-spec bullet (WotManagementAccessPolicy default). * README.md (repo root) - migration area list now ends with 'and WoT Connectivity'. Verified: 0 conflict markers anywhere; all relative links in the 6 changed files resolve. Auto-merged docs (Sessions.md, Docs/README.md, DependencyInjection.md, McpServer.md, new UnboundedSubscriptions.md, new PacketCapture.md, Tools/Opc.Ua.MigrationAnalyzer/**) left alone.
marcschier
added a commit
to marcschier/UA-.NETStandard
that referenced
this pull request
Jun 14, 2026
Brings in 7 upstream commits including OPCFoundation#3866 (Managed DevOps Pool CI migration), OPCFoundation#3865 (dotnet format sweep), OPCFoundation#3854 (V2 SetTriggering), OPCFoundation#3868 (MatrixOf<T> properties for OneOrMoreDimension), OPCFoundation#3864 (ManagedSession revalidation tests), OPCFoundation#3863 (Part 13 Aggregates compliance), and OPCFoundation#3828 (stop emitting unimplemented Optional children on singleton instances). One conflict in `UA.slnx`: upstream added two new project entries between `ConsoleReferenceSubscriber` and `Quickstarts.Servers` — * `Applications/McpServer/Opc.Ua.Mcp.csproj` — upstream's renamed path. Our branch never adopted the rename and still ships the project at `Applications/Opc.Ua.Mcp/Opc.Ua.Mcp.csproj`, which is already listed two lines below. Dropped: the McpServer path is stale build artifacts only (bin/obj), no .csproj. * `Applications/PumpDeviceIntegrationServer/PumpDeviceIntegrationServer.csproj` — upstream's intentional new project. Project file exists in our branch; kept. UaLens build clean (0 warnings / 0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Singleton instances declared in
StandardTypes.xmlandOpc.Ua.NodeSet2.xml(Server,Server.ServerConfiguration,HistoryServerCapabilities,OPCUANamespaceMetadata, the well-known role objects, the Part 17Aliasescategory, etc.) were emitting Optional Variable/Method children that the SDK does not implement — for exampleServerConfiguration.ApplicationUri,ProductUri,ApplicationType,HasSecureElement,CancelChanges,ResetToServerDefaults,Server.UrisVersion,Server.EstimatedReturnTime,Server.LocalTime,Server.ServerCapabilities.ConformanceUnitsand friends.The cause was the source generator promoting every type-definition child's modelling rule into the singleton-instance factory whenever
UseTypeDefinitionModellingRuleswas enabled (#3799), which made every Optional child unconditionally appear in the singleton'sCreateXxxbody.This PR fixes the regression at the generator level (without touching
StandardTypes.xml) and lets the SDK opt back in to just the Optional children it actually implements.New: chainable
Add{Child}API with configure-callback overloadsPer review feedback ("the existing predefined node is used. A user adds optional children using
.AddXY(NodeId? nodeId = default). The Add calls should be possible to chain. … Instead of AddAndGet, emit an Add overload with a callback to configure the created node state further, add another Add to take a condition predicate that decides whether the Create is executed or not"), the source generator now emits five chainableAdd{Child}overloads per Optional child of every typed state class. All return the owner (this) so multi-child wiring collapses into a single fluent chain:The previous
AddAndGet{Child}(context, NodeId?)helper is removed; every call site is migrated to one of the new overloads. The Action overloads cover the common "set Value / wire OnCallAsync" case in-line; the Func overloads allow callers to replace the typed slot with a wrapper or subclass instance. The boolean overloads short-circuit when the condition isfalse.When
nodeIdisnull: ifcontext.NodeIdFactoryis non-null the helper mints a fresh NodeId viacontext.NodeIdFactory.New(context, child); otherwise it leaves the type-level NodeId assigned by the generated factory unchanged (the caller can patch later).NodeState.Create(context, source)stays authoritative (the loaded NodeSet XML / passive source is the source of truth for what children exist). The new chainableAdd{Child}overloads are the runtime-extension API for SDK code and downstream users that want to add Optional children programmatically after load.A new
Tokens.OwnerClassNamecarries the owning typed state class name through the per-child template loop soAdd{Child}can returnthistyped as the owner; the template engine's outer-scope lookup cascades the value fromAddNodeStateClassTypeReplacements/AddNodeStateClassMethodTypeReplacementsinto the per-child template scope.SDK migration in
Libraries/Opc.Ua.ServerDiagnosticsNodeManageradd-back helpers collapse into single fluent chains; theSetSubscriptionDurableconditional folds into the Server chain via the bool-condition overload (AddSetSubscriptionDurable(context, m_durableSubscriptionsEnabled, _ => { }, MethodIds.Server_SetSubscriptionDurable)); theOperationLimitsadd-back inlines via.AddOperationLimits(...).OperationLimits!.AddMax...().ConfigurationNodeManagerchainsAddGetCertificates(context).AddCreateSelfSignedCertificate(context)beforeCreate(context, passiveNode).HistoricalDataConfigurationInstaller.PopulatePropertiesis now a single fluent chain using the conditional Action overload —.AddMaxTimeInterval(context, capabilities.MaxTimeInterval > 0, c => c.Value = capabilities.MaxTimeInterval)replaces theif (cond) { config.AddAndGetMaxTimeInterval(context).Value = ...; }ceremony.KeyCredentialPushSubjectfoldsOnCall = null; OnCallAsync = ...into Action callbacks on the chain:state.AddUpdateCredential(context, c => { c.OnCall = null; c.OnCallAsync = OnUpdateCredentialAsync; }).AddDeleteCredential(...).WotConnectivityNodeManager.WireManagementMethodschains.AddDiscoverAssets(ctx, c => c.OnCallAsync = ...).AddCreateAssetForEndpoint(...).AddConnectionTest(...).AddSupportedWoTBindings(...).AssetRegistryandAlarmConditionTypeHolderupdated fromstate.X ??= state.AddX(context)to plainstate.AddX(context)+ null-forgiving access on the typed slot (idempotentAdd{Child}makes the??=ceremony obsolete).Generator fix
Tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs:NodeToGenerategains a structuralIsUnderSingletonInstanceflag, settrueon the singleton-instance root inCollectNodesToGenerateand propagated to every descendant inGetChildren. Mirrors the existingRootIsTypeDefinitionpropagation pattern; the gate becomes an O(1) lookup.The
topLevelInstanceExceptionis preserved for TYPE-DEF roots — itsUseTypeDefinitionModellingRulesgate only fires when the parent is anInstanceDesignsingleton. Without this, the type-def factories (e.g.CreateNamespaceMetadataType) stop emitting their Optional children which breaks deep code paths that rely on the type-def factory shape (e.g.ConfigurationNodeManager.CreateNamespaceMetadataStateAsync).Why the gate is transitive but narrowed to Variables/Methods
The gate applies transitively to every Variable/Method descendant of a singleton-instance subtree. Deep Optional Variable/Method descendants such as
Server.ServerCapabilities.MaxArrayLength,Server.ServerCapabilities.OperationLimits.MaxNodesPerRead,Server.ServerRedundancy.RedundantServerArray,Server.UrisVersion,Server.EstimatedReturnTime,Server.LocalTime,PublishSubscribe.AddConnection/RemoveConnection, the Optional methods on every well-known role, the OPCUANamespaceMetadata Default* properties, andAliases.LastChangeare now all suppressed by the generator.Optional Object instances under singletons are intentionally exempt: they are still emitted by the singleton-specific factory with their correct instance-level NodeIds. Their subtrees use well-known descendant NodeIds (e.g.
ServerConfiguration.CertificateGroups.DefaultHttpsGroup.TrustList.Open = NodeId 14095) that the SDK andConfigurationNodeManagerbind against directly. Optional Variable/Method children of those Objects are still gated transitively (e.g.OperationLimits.MaxNodesPerReadis suppressed even thoughOperationLimitsitself stays emitted).EURange regression analysis
The earlier attempt at this fix regressed
AnalogItemType.EURange(which isMandatoryonAnalogItemTypeper the OPC UA spec — a type-level promotion ofBaseAnalogType.EURange).IsUnderSingletonInstanceisfalseon type-def and synthetic type-instance roots, so the gate cannot fire on either type-template tree. Combined with the topLevelInstanceException preserved for type-def roots, every type-def factory continues to emit its full child set.SDK programmatic Add* (Opc.Ua.Server)
DiagnosticsNodeManager.AddSdkImplementedOptionalChildrennow dispatches per loaded predefined node type. Each lazy-add call patches the resulting NodeId (and MethodInputArguments/OutputArgumentsNodeIds where applicable) to the well-known singleton-instance value viaObjectIds.*/MethodIds.*/VariableIds.*. The generator-emittedAdd{Child}extension uses the type-level factory which assigns the type NodeId; the SDK overrides each NodeId after construction.Server(ServerObjectState):GetMonitoredItems,ResendData,SetSubscriptionDurable(when enabled),Namespaces,UrisVersion,EstimatedReturnTime,LocalTime.Server.ServerCapabilities(14 Properties):MaxArrayLength,MaxStringLength,MaxByteStringLength,MaxSessions,MaxSubscriptions,MaxMonitoredItems,MaxSubscriptionsPerSession,MaxMonitoredItemsPerSubscription,MaxSelectClauseParameters,MaxWhereClauseParameters,MaxMonitoredItemsQueueSize,ConformanceUnits,RoleSet,OperationLimits. Required to keepServerInternalData.CreateServerObjectAsync's non-null assertions working and to keepSession.FetchOperationLimitsAsyncround-trip values matching master.Server.ServerCapabilities.OperationLimits(12 Properties): everyMaxNodesPerXxxandMaxMonitoredItemsPerCall.Server.ServerRedundancy.RedundantServerArray: re-added forDefaultServerRedundancyHandler(client-side) compatibility.HistoryServerCapabilities.ServerTimestampSupported.ApplicationsExclude,Applications,EndpointsExclude,Endpoints,CustomConfiguration,AddIdentity,RemoveIdentity,AddApplication,RemoveApplication,AddEndpoint,RemoveEndpoint). The three immutable roles (Anonymous, AuthenticatedUser, TrustedApplication) have no well-known instance NodeIds for these methods in the spec and are left untouched.OPCUANamespaceMetadata(i=15957, standard Part 6 namespace metadata loaded fromOpc.Ua.NodeSet2.xml):DefaultRolePermissions,DefaultUserRolePermissions,DefaultAccessRestrictions.Aliases(i=23470):LastChange. Required byAliasNameResolverRefreshMode.AutoOnLastChangeMonitoredIteminOpc.Ua.Clientwhich subscribes to LastChange to invalidate its cache.ConfigurationNodeManagercontinues to assignCreateSelfSignedCertificate = new CreateSelfSignedCertificateMethodState(activeNode)beforeCreate(...)(it is no longer emitted by the singleton factory).Related Issues
Verification
CreateServerConfigurationdoes not emitApplicationUri/ProductUri/ApplicationType/HasSecureElement/CancelChanges/ResetToServerDefaultsatforInstance=true.CreateServer_ServerCapabilitiesno longer emits the 14 SDK-handled Optional Properties atforInstance=true; the SDK re-adds them with the correct instance NodeIds.CreateServerConfiguration_CertificateGroupsstill emitsDefaultHttpsGroupandDefaultUserTokenGroup(Object exemption) preserving the subtree NodeIds thatConfigurationNodeManagerbinds against.CreateNamespaceMetadataType(the type-def factory) emits all Optional Properties unconditionally soConfigurationNodeManager.CreateNamespaceMetadataStateAsynckeeps working for runtime-created metadata instances.ServerConfiguration_OptionalUnimplementedChildren_NotInAddressSpaceAsync:Server_ServerConfiguration_CertificateGroups_DefaultApplicationGroup_CertificateTypesresolves at its well-known NodeId, and the OptionalDefaultHttpsGroup/DefaultUserTokenGrouproots remain emitted (Object exemption).Opc.Ua.Server.Tests: 1683 / 1683 pass on net10 (5 pre-existing skips).Opc.Ua.Core.Security.Tests: 547 / 547 pass on net10 (68 pre-existing skips). Includes the RoleManagementTests, SecurityRoleServerBase2Tests, and SecurityRoleServerDefaultPermsTests fixtures.Opc.Ua.Features.Tests: 128 / 128 pass on net10 (22 pre-existing skips). Includes the RoleManagementClient and MonitoredItemAliasNameRefreshStrategySignAndEncryptTests fixtures.Opc.Ua.InformationModel.Tests: 1052 / 1052 pass on net10 (24 pre-existing skips).Opc.Ua.Gds.Tests: 875 / 875 pass on net10 (29 pre-existing skips).Opc.Ua.SourceGeneration.Core.Tests: 3514 / 3514 pass on net10 (8 long-running compile-cycle integration tests skipped by the framework).AnalogItemType.EURangeregression.Checklist