mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-19 07:02:16 +08:00
fix/encoder-stack-depth-json-flake
42 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 romanett
|
9b034f8291 |
Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700)
# Description Nullable reference types are now enabled once, in `common.props`, and every project in the repository builds warning-clean with them. **Configuration** - `<Nullable>enable</Nullable>` lives in `common.props`. All per-project `<Nullable>` settings are removed: 178 project files (enable, annotations, and the `disable` in the three Aot test projects). - 419 per-file `#nullable enable`/`restore` directives and the 4 `#nullable disable` directives are removed. - Kept on purpose: - The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`, `PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the project setting in generated code, so the directive is required there. - `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against the 1.5.378 packages and does not import the repository props. - `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now describe the central setting. **Migration** (the 43 unset and 13 annotations-only projects, roughly 10k diagnostics) - **Production code: annotated at the declarations.** Covers `tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the LDS sample. - Members, parameters and returns are nullable where null actually flows. - Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`, `IsMethodTypeDesign`). - `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take `params object?[]`. `Template.AddReplacement` takes `string?` (rendering already skips null replacements). - `IRegisteredServerStore.RegisterAsync` returns `Task<RegistrationEntry?>`: it returns null when a registration is removed. - Dereferences backed by an invariant (for example, the validated model) use `!`, so runtime behaviour is unchanged. - **Behavioural changes in production code** (small, deliberate): - `NodeSetToModelDesign` throws `InvalidOperationException` when the file is not a NodeSet. Before, it threw a NullReferenceException on the next line. - `ModelDesignValidator` starts its lookup tables empty instead of null. A redundant `if (m_nodes == null) return;` guard is gone. - **Test code: minimal annotations.** - `?` on values that can be null, `!` where the test asserts non-null. - `= null!` for fixture members assigned in SetUp. - `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the `IEncodeable` constraint. - A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`) folded into the adjacent `IsNull`/`Count` checks. **Notes for reviewers** - The diff is large (~1,500 files) but almost entirely mechanical: `?`, `!` and directive removal. The non-annotation changes are listed above. - Known follow-up, not done here: `IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay non-nullable even though the validator returns null before it loads a dictionary. Making them nullable would ripple through every generator call site. ## Related Issues - Fixes #4439 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. The "no new warnings" box covers net10.0 and net48: full non-incremental builds of `UA.slnx`, `tools/SourceGeneration.slnx`, `tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0 warnings and 0 errors under the repository's warnings-as-errors settings. net9.0 and net8.0 also had no nullable diagnostics, but they were not rebuilt after the final fixes. Tests have not been run locally yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
romanett
|
cce2004fdd |
CTT: keep GeneratesEvent off Object instances and Views off non-hierarchical references; classify Deadband Err-006 as CTT defect (#4667)
# Description Three findings from the 2026-10-03 CTT run with the patched 1.05.513 scripts. Two are server/model defects and are fixed here. The third is a CTT script defect, recorded as C56 in `plans/ctt-issues.md`. ## 1. Monitored Items Deadband Filter `Err-006.js`: CTT defect (C56, no server change) The script modifies a MonitoredItem on the first static numeric scalar (a plain BaseDataVariable with no EURange) to use a PercentDeadband. It accepts only `Good` or `Bad_MonitoredItemFilterUnsupported`. Since #4629 the server returns `Bad_DeadbandFilterInvalid`, and that is the code the spec defines for this case: - OPC 10000-8 §7.2: PercentDeadband "applies only to AnalogItems with an EURange Property". - OPC 10000-8 §7.3.2 Table 61: `Bad_DeadbandFilterInvalid` is returned when "a PercentDeadband is not supported, since an EURange is not configured". CreateMonitoredItems and ModifyMonitoredItems both go through `ValidateMonitoringFilterAsync` and return the same code. Monitor Basic `Err-022.js` already accepts `Bad_FilterNotAllowed` for non-analog items, so the scripts disagree with each other here. Draft Mantis note (not filed yet): > **Monitored Items Deadband Filter Err-006.js rejects Bad_DeadbandFilterInvalid for a PercentDeadband on a node without EURange** > Err-006.js picks the first node of `/Server Test/NodeIds/Static/All Profiles/Scalar/NumericSettings`, which is normally a BaseDataVariable without an EURange. It then calls ModifyMonitoredItems with a PercentDeadband (10 %) and accepts only Good or Bad_MonitoredItemFilterUnsupported. OPC 10000-8 §7.2 limits PercentDeadband to AnalogItems with an EURange. §7.3.2 Table 61 defines Bad_DeadbandFilterInvalid for "a PercentDeadband is not supported, since an EURange is not configured", so a compliant server fails this test. Monitor Basic Err-022.js already accepts Bad_FilterNotAllowed for the non-analog fallback. > Proposed fix: add StatusCode.BadDeadbandFilterInvalid (and, consistent with Err-022.js, StatusCode.BadFilterNotAllowed) to the ExpectedAndAcceptedResults of the ModifyMonitoredItems call. ## 2. GeneratesEvent on Objects (Base Info Core Structure 2 `001.js`): source generator defect The CTT reported `ns=9;i=234` (the `<ServiceName>` placeholder of AuthorizationServicesFolderType) and `ns=9;s=AuthorizationServices/Default` as Objects that are the source of GeneratesEvent references. OPC 10000-3 §7.15 says: "The SourceNode of References of this type shall be an ObjectType, a VariableType or a Method". `OpcUaGdsModel.xml` declares both references (AccessTokenRequested/Issued audit events) correctly, on `AuthorizationServiceType`. The source generator copied the type's own non-hierarchical references onto every Object or Variable built from the type: onto instance declarations (the placeholder) and into `CreateInstanceOf<Type>`, which `AuthorizationServiceState` initialises from, and that is how the `Default` service gets them. UA-ModelCompiler does the same thing, so the official `Opc.Ua.Gds.NodeSet2.xml` in UA-Nodeset has the same two references on `ns=1;i=234`. Fix: - `TranslateReferences` now marks references declared on a type (`HierarchyReference.DefinedOnType`, which already existed but was never set). - `NodeStateGenerator.GetReferences` drops forward GeneratesEvent references (including subtypes such as AlwaysGeneratesEvent) for Object and Variable nodes when the reference came from the type. - ObjectTypes, VariableTypes and Methods keep these references. For example, the RegisterApplication methods and the Acknowledge/AddComment methods of the alarm types are unchanged. - References that a model declares explicitly on an instance are emitted as before. ## 3. View targeted by HasCause/HasEffect (Base Info Core Structure 2 `001.js`): sample model defect #4485 added `Views_Operations HasCause/HasEffect Views_Engineering` so that View Minimum Continuation Point 01 `012.js` and View Basic 2 `018.js` have at least two references to View nodes on the configured View (`/Server Test/NodeIds/NodeClasses/View` = `Views_Operations`). OPC 10000-3 §5.4 says Views "can be the SourceNode of any hierarchical Reference. They shall not be the SourceNode of any NonHierarchical Reference." `Views_Operations` now Organizes `Views_Engineering` and a new View, `Views_Maintenance`, which organizes `DataAccess`. This keeps two View-class references and adds no loops (ContainsNoLoops stays true). ## Tests - `NodeStateGeneratorTests.TypeGeneratesEventReferencesAreNotCopiedToObjectInstances`: covers the type, the placeholder, the instance, `CreateInstanceOf`, Methods and an explicit instance reference. It fails without the fix. - `ReferenceServerTest.SampleViewsHaveNoNonHierarchicalReferencesAsync`. The existing `NodeClassSampleNodesReferenceTheirOwnNodeClassAsync` still passes. - Results with net10.0: - SourceGeneration.Core.Tests: 4267 passed, 0 failed - InformationModel.Tests: 1078 passed, 0 failed - Di.Tests, including RobotOpenUsdE2e: 438 passed, 0 failed - OpenUsd.Tests: 921 passed, 0 failed - Server.Tests `ReferenceServerTest`: 56 passed, 0 failed - `UA.slnx` builds with net10.0. The changed test projects build with net48. ## CTT (patched 1.05.513 scripts, compared with the 2026-10-03 patched baseline) | Part | Change | | --- | --- | | p02-base-info | Base Info Core Structure 2 `001.js`: Error → Skipped. The GeneratesEvent and View errors are gone; the remaining skips are the CTT's own "type not defined within the test-script" notes. No other change. | | p03-base-view-method | No change. View Basic 2 `018.js` and View Minimum Continuation Point 01 `012.js` still pass. | | p13-monitored-items | Deadband `Err-006.js` still Error (C56). With the script fix it passes. Monitor Basic `007.js` gave a response-timestamp delay warning (>200 ms) caused by machine load; it also appears in the 2026-09-29 base run. | 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Klaus Landsdorf
|
ac8dc531ee |
Source generator, ISA-95 Job Control and DI changes for companion SDKs built on referenced models (#4577)
## Summary First of four stacked PRs that contribute complete implementations of the Machinery (OPC 40001-1 with Job Management, Result Transfer, Process Values and Energy), Pumps (OPC 40223) and Scales (OPC 40200, with PackML OPC 30050) companion specifications — tracked in #4576. It follows the split used for Robotics (#4123–#4127): this PR carries only the changes to **existing shared code** those SDKs need, and adds no project. Each change stands on its own and is covered by tests in this PR; the companion models are what surfaced them. Rebased onto master after #4569. The one conflict was in `NodeSetToModelDesign.cs`: #4569's shared `AssignSymbolicIds` already makes `Import` and `GetImportedSymbols` derive the same ids, and `NormalizeParentNodeId` moves into its pre-pass. The Copilot review is addressed in the last six commits (see "Review follow-up"). ## Changes ### Source generator (`tools/Opc.Ua.SourceGeneration.Core`) - **VariableType data type across assemblies.** A model that consumes a dependency through a referenced assembly's `[ModelDependency]` payload lost a VariableType's `DataType`/`ValueRank`, so `GetNodeStateClassName` dereferenced null and reported `MODELGEN003`. First hit: DI's `LifetimeVariableType` as used by Machinery. - **Method declaration ↔ method state identity.** A consumer re-declaring an inherited method referenced an `OwnerType_MethodMethodState` class the producer never generated. First hit: OPC 34100 ECM re-declaring the DI `LockingServices` methods. - **Browse names and reference types of inherited children.** An inherited child was emitted with `ReferenceTypeId = NodeId.Null` (invisible to a filtered Browse over the wire) and, where it lives in another namespace, with the declaring model's namespace. When the carried namespace is one the consuming model does not declare, the generated code names it by URI instead of emitting an empty `GetIndexOrAppend()`. - **Payload compatibility.** The three additions above travel in a length-prefixed, versioned declaration trailer after the existing trailers, so the node and child records keep their layout: an older reader ignores the capability bit and stops after the trailers it knows, and a payload without declarations is byte-identical to before. - **Symbolic ids of children listed before their parent.** Every `ParentNodeId` is normalized before any symbolic id is derived. No NodeSet in the repository has such an ordering today, so no generated symbol changes; OPC 30050 PackML (later in the stack) declares seven method `InputArguments` before their methods and failed with `MODELGEN025`. - **Methods overridden with another method-state class.** The subtype declares its own field and property for such a method, but `GetChildren` and `RemoveExplicitlyDefinedChild` skipped every overridden child and the supertype's field stays empty, so the method was missing from every instance (not browsed, no per-instance NodeId, not callable). The existing `MethodTypeOverride` test model has the gap; since #4555 generates a method with inline arguments as `MethodState`, OPC 40001-101 `ResultTransferType.GenerateFileForRead` hits it. It also affects models already in the repository: DI `SoftwareFolderType.Delete`, ISA-95 V2 `JobOrderReceiver.Update`, WoTCon `WoTDocument.Validate` and XRegistry `Group.Delete` / `Resource.Delete` now appear on their instances; the DI, ISA-95, WoTCon and XRegistry tests pass. ### ISA-95 Job Control (`src/Opc.Ua.ISA95.Server`) - **`Isa95JobControlV2Binder`** — the V2 receiver/response-provider binding moves out of `Isa95NodeManager` so OPC 40001-3 JobManagement can bind the same eleven verbs on its own instances instead of copying them. `Isa95NodeManager` behaviour is unchanged. - **`Isa95AddressSpaceOwnership`** — a second registration that would load the ISA-95 models fails immediately and names the current owner, as `DiAddressSpaceOwnership` already does for DI. - **`IIsa95JobResponseCatalog`** — a source for `JobOrderResponseList`, which the two state-filtered request methods cannot express. - **`IIsa95JobResponseChangeSource`** — a change stream of received responses, and of responses the provider drops after `ResponseRetention`; a response changes neither the order's state nor the order catalog, so the existing streams cannot report it. Both interfaces are implemented by `InMemoryIsa95JobControlProvider`, registered by `AddInMemoryIsa95JobControlProvider`, and count in `AddIsa95Server`'s provider detection and mixed-provider rejection. ### Device Integration (`src/Opc.Ua.Di.Server`, `src/Opc.Ua.Di.Client`) - **`IDiPostSetupContext.GetService<T>()`** — an optional lookup next to `GetRequiredService<T>()`, so a companion SDK can probe for a seam the server may not have filled without catching `InvalidOperationException`. The context still exposes no raw `IServiceProvider`. - **`DiNodeManager.ConformanceUnits` / `ServerProfiles` are virtual**, so a companion subclass appends the units its own model satisfies. - **DI client browse enumerators follow `BrowseNext`.** `DiTopologyClient.BrowseChildrenAsync` and `DiDeviceClient.BrowseFunctionalGroupsAsync` discarded the continuation point and returned an incomplete list from a paginating server. A bad status for the browsed node still means "no children" and a failed service call propagates; an outstanding continuation point is released whenever the enumeration stops early. ## Review follow-up | Copilot finding | Commit | |---|---| | Payload additions break older readers (both High) | `fix(sourcegen): move the dependency declarations into a trailer` | | Browse failures reported as empty, broad exception filter (Medium ×2); missing pagination tests (Low ×2) | `fix(di-client): let a failed browse call propagate` | | New provider facets missing from detection (Medium) | `fix(isa95): count the response facets as Job Control providers` | | Second review: continuation point kept after a failed BrowseNext | `fix(di-client): release the continuation point after a failed BrowseNext` | | Second review: purged responses not reported | `fix(isa95): report dropped job responses on the response change stream` | | Second review: missing V2 namespace cast to 65535 | `fix(isa95): reject a missing Job Control V2 namespace in the binder` | ## Points for the reviewer - `GetService<T>()` adds a member to a public 2.0 interface. The only implementation is the internal `DiPostSetupContext`; an external implementer would have to add it. - `DiDeviceClient.BrowseFunctionalGroupsAsync` used to throw for a bad status of the device node; like `DiTopologyClient` it now yields nothing. - The commits are self-contained and ordered; reviewing commit by commit is the easiest route. ## Testing - New and extended tests: `ModelDependencyV1Tests`, `NodeStateGeneratorTests`, `NodesetIdentifierSidecarTests`, `InMemoryIsa95JobControlProviderV2Tests`, `Isa95ServerBuilderTests`, `Isa95JobControlV2BinderTests`, `DiDeviceClientTests`, `DiTopologyClientTests`, `OpcUaServerDiBuilderExtensionsTests`. - `UA.slnx` builds for net472, net48, netstandard2.0/2.1 and net8/9/10 with SDK 10.0.401 and no warnings. - Tests on net10: SourceGeneration.Core 4266 (8 skipped), SourceGeneration 263, ISA95 144, DI 433; the first three also on net8. The changed projects build for net48 and netstandard2.0 without warnings. - Changed-line coverage (`check-coverage.ps1`): 91.1 % (594 of 652 lines). - The companion stack on top (#4578, #4579, #4580) is rebased onto this head. - Fork CI, full PR matrix incl. macOS, for this head (running): https://github.com/biancode/UA-.NETStandard/actions/runs/36558506011. CodeQL: https://github.com/biancode/UA-.NETStandard/actions/runs/36558509242. The previous head (before the second review round) passed every Windows and Linux job: https://github.com/biancode/UA-.NETStandard/actions/runs/36542929585. |
||
|
romanett
|
442889cba5 |
Fix source generator audit findings; conform structure-field matrices and type metadata to OPC UA 1.05 (#4569)
# Description
This PR fixes a bug audit of the source generation projects
(`tools/Opc.Ua.SourceGeneration*`: model loading, NodeSet import, all
emitters, the Roslyn hosts and packaging). It also fixes a max-effort
code review of those fixes, and a check of every spec-relevant change
against the OPC UA 1.05 text.
- **Scope:** about 85 audit findings, 48 review findings and 32
spec-grounding items were addressed. Each fix has a regression test that
fails without it.
- **Wire change:** it includes one deliberate, stack-wide wire change:
**structure-field matrices now follow OPC 10000-6 5.2.5 Table 28**.
Most changes are internal correctness fixes. The sections below list
every change a user can observe: wire formats, generated code and API,
generated address-space content, diagnostics and runtime behaviour.
## Behaviour / user-facing changes
### Wire format (encoders)
- **Binary inline matrices** (OPC 10000-6 5.2.5 Table 28).
- **Layout:** a multi-dimensional *structure field* is encoded as
`Int32[] Dimensions`, then the values **without a length prefix**, with
at least 2 dimensions.
- **Empty and null:** any dimension <= 0 means no values follow, and a
null matrix is `-1`. An empty matrix is written as `[0,0]`.
- **Compatibility:** this is **not compatible with earlier builds of
this SDK** for matrix fields. It applies to generated types,
`[DataType]` types, `Structure`, client ComplexTypes and
`WriteEncodeableMatrix`.
- **Unchanged:** Variant matrices (5.2.2.16) keep their old encoding.
- **Declared rank decides matrix vs array.** A 1-D `MatrixOf<T>` in an
array field or a Variable value is written as an array again.
Matrix-declared fields go through the new public
`EncoderExtensions.WriteInlineMatrixValue`, which generated code also
uses. A non-empty 1-D value in a matrix field throws `BadEncodingError`.
- **Decoder hardening:**
- Before allocating, the decoder bounds all matrix dimensions against
overflow, `MaxArrayLength` and the remaining message length.
- The decoder rejects:
- a matrix with fewer than 2 dimensions;
- a populated matrix whose rank differs from the field's declared rank;
- a union SwitchField greater than the number of fields;
- an EncodingMask with unassigned bits set.
- **Encoder `MaxArrayLength`:** the encoder enforces `MaxArrayLength`
for inline matrices, as it already did for arrays.
- **XML structure fields** (5.3.1, 5.3.4, 5.3.5).
- **Fields:** `Structure` and client ComplexTypes write fields like
typed fields (`<A>1</A>`, array elements directly) instead of a Variant
body.
- **Matrix fields:** `Dimensions` and `Elements` go directly inside the
field element, with no `<Matrix>` wrapper.
- **Empty matrices:** written as nil, because XML dimensions must be
greater than 0.
- **Compatibility:** decoders still accept the old shapes.
- **Enum fixes:** enum array elements with value 0 are no longer
dropped, and enum array element names are fixed.
- **Other encoding fixes:**
- A null `DiagnosticInfo` structure field writes `0x00` instead of
nothing.
- A null scalar field in a runtime `Structure` is written as its default
instead of zero bytes, which used to shift every following field.
- A Variant holding an empty matrix encodes as an empty array instead of
throwing.
- `Variant.Copy` keeps the matrix identity of null and empty matrices.
- **NodeSet export and the DataContract surrogate** write Variable
values with the Variant rules again. A null or empty `MatrixOf` variable
no longer produces invalid XML or unreadable NodeSets.
- **`JsonEncoder.CanOmitFields`** is `true` only for Compact encoding.
Verbose and RawData JSON include default-valued fields.
### `[DataType]` source-generated types
- **Default-value handling** (OPC 10000-6 5.4.1, Table 1).
- **Encode:** a field is omitted only when its value *and* its declared
default are the type's default (0, false, null). Fields with a
non-default initializer or constructor default, and empty strings, are
always written, so configuration files get slightly larger.
- **JSON decode:** a field missing from JSON decodes to the type
default.
- **XML decode:** a field missing from XML keeps its declared default,
which older configuration files rely on.
- **Why:** this fixes silent round-trip loss, for example
`SamplingInterval = 0` read back as `-1`.
- **Nested `[DataType]` types:** they are now generated inside their
containing types. Without an explicit id, their default TypeId and
XmlName are qualified (e.g. `s=Outer.Foo`); **wire identity change** for
nested types. Colliding activator names get a `_2` suffix.
- **Activators:** they are `internal` when the type, or any containing
type, is not public (was CS0060).
- **Matrix properties:**
- `MatrixOf<T>` properties use the inline matrix encoding.
- Matrices of non-sealed structures, or with
`StructureHandling.ExtensionObject`, are written as ExtensionObjects.
- Their published ValueRank is 2, because the rank can't be known from
`MatrixOf<T>`.
- **Published StructureDefinitions:**
- They include base-class fields.
- Nullable properties are no longer marked optional.
- Inline structure fields name their concrete type.
- **`[Flags]` enums:** they are published as plain enumerations with
their member values, which matches how they are encoded.
- **Other generated code:**
- `Clone` deep-copies arrays and matrices of structures, Variants,
ExtensionObjects and DataValues.
- `IsEqual` and `GetHashCode` are consistent: EncodingMask, active union
member and NaN handling agree.
- Pooled `Reuse()` restores the freshly constructed state through the
new `protected virtual ResetForReuse()`, and returns an instance to its
runtime type's pool.
- **Enum capture:** enum member values are captured culture-invariantly.
Negative values were lost on cultures that use U+2212 as the minus sign.
### Model-driven generator: generated code, API and file names
- **Removed API:** generated `*_Placeholder` proxy accessors (e.g.
`PowerTrainTypeClient.GetMotorIdentifier_PlaceholderAsync`) and fluent
`X_Placeholder()` accessors are removed; they could never resolve
anything.
- Proxy accessors now look children up by BrowseName with the right
namespace.
- `RoboticsClient` finds motors and gears by type and drives via
`IsDrivenBy`. Its topology snapshots used to be empty.
- **Renamed files:** generated files for a bound `[NodeManager]` are now
named `{Namespace}.{Class}.NodeManager.g.cs` (also `.Factory` and
`.FluentBuilders`).
- **Generated NodeState code:**
- Attributes are set through `baseState`/`nodeState` locals, so children
named like base attributes (e.g. `DataType`, `EventNotifier`) compile.
- Children that hide base members are declared `public new`, using names
reflected from the runtime.
- `global::` qualification is used throughout.
- **Proxies and fluent builders:** a proxy whose supertype is excluded
(e.g. Draft) derives from the nearest emitted ancestor. Fluent wrappers
and NodeState factories fall back the same way; they used to fail with
CS0246.
- **Derived structures:**
- The optional-field `EncodingMask` continues across inheritance.
- A derived structure over a base type from another model no longer
relies on `base.ResetForReuse`.
- **Default values:**
- NodeId, QualifiedName and ExpandedNodeId defaults resolve by namespace
URI (`context.NamespaceUris.GetIndexOrAppend`) and server URI.
- StatusCode, negative enum, `double`/`float` (shortest round-trip
literal) and escaped string defaults now compile.
- **Event records:**
- Vendor event records have `Decoder.GetStandardFields(NamespaceTable)`,
and their browse paths use the model's namespace.
- A field whose namespace is unknown to the session no longer matches a
namespace-0 field.
- Companion `StandardFields` is `[Obsolete]`.
- Vendor simple subtypes such as `Percent : Double` map to their
built-in type.
- **Other fixes:**
- `MethodState.CallAsync` passes the real `objectId` to synchronous
`OnCallMethod2` handlers; it used to pass `NodeId.Null`.
- Enumeration fields with an explicit `Identifier` keep it, instead of
taking a trailing `_<digits>`.
- Explicit NumericIds are no longer reused by automatic numbering.
- Dependencies are loaded in topological order.
### Generated address-space content
- **Arguments:**
- Methods without arguments no longer get empty
`InputArguments`/`OutputArguments` properties (OPC 10000-3 5.7).
- About 2,660 automatically assigned namespace-0 NodeIds of 15000 and
above shift. Every NodeId in `StandardTypes.csv` is unchanged.
- **Permissions:** type nodes and method declarations on types now carry
the model's `RolePermissions`/`AccessRestrictions`.
- **Restored nodes:** NodeSet import keeps nodes that were silently
dropped before:
- `NamespaceMetadata` objects and the NodeSet's own TypeDictionary
variables;
- ISA95 placeholders linked only non-hierarchically;
- instances whose parent is in another namespace.
- **Import details:**
- ValueRank > 1 without ArrayDimensions keeps its rank.
- `svr=` indexes map correctly.
- Invalid MinimumSamplingInterval values import as -1, not 0.
- Localized texts prefer the invariant or English translation.
- **StructureDefinitions:**
- They include inherited fields, including for subtypes without their
own fields (e.g. `AnonymousIdentityToken` now publishes PolicyId).
- The StructureType kind covers inherited AllowSubTypes fields.
- Unions ignore IsOptional.
- StructureFields never publish ValueRank 0; a OneOrMoreDimensions field
without dimensions becomes rank 2.
- Fields with ValueRank -2/-3 are published as scalar BaseDataType,
which is what is on the wire.
- **OptionSets:**
- Implicit fields get the next free bit.
- `OptionSetValues` uses null for undefined bits instead of "Reserved",
and structure OptionSets publish all their bits (no 32/96-bit cap).
- Numeric OptionSets are limited to their base type's width, and UInt64
bit 63 works.
- **Schemas:**
- Structures with inline-matrix fields are left out of the `.bsd`
(5.2.5).
- In the BSD, Variant-encoded and non-structure subtyped fields are
described as `ua:Variant`.
- In the `.xsd`, matrix fields are typed `ua:Matrix` with
`Dimensions`/`Elements`, and `EncodingMask` is required.
### NodeSet / model input handling
- **Model selection:**
- The newest NodeSet of a model is chosen by `ModelVersion` (SemVer 2.0
precedence), then PublicationDate. The `Version` attribute, which Annex
F says is not for comparison, is only a tie-break.
- The order is total and does not depend on input order; referenced
assemblies use the same rule.
- **Default names:**
- Invalid default Name/Prefix values are repaired (`201301ISA95` becomes
`_201301ISA95`, `class` becomes `class_`).
- Valid names are unchanged.
- Sanitising no longer depends on the OS.
- **`ModelUri` metadata** that doesn't match the NodeSet raises a
warning instead of silently losing the model.
- **Input detection:**
- Only XML files whose root is `ModelDesign` or `UANodeSet` are treated
as models.
- A malformed NodeSet stays a NodeSet input, so it no longer breaks the
ModelDesign pass.
- **`IdentifierFile` paths:** they are normalized (`./`, `../`), and the
suffix match requires a folder boundary.
- **Dependency payloads** (`ModelDependencyV1`) carry field IsOptional,
AllowSubTypes, ArrayDimensions and the producer's namespace tables in
skippable trailers, so old and new readers stay compatible.
### Roslyn host, diagnostics, packaging
- **New diagnostics:**
- MODELGEN036: `[NodeManager]` target not supported (nested, generic,
file-local or global namespace).
- MODELGEN037: `[DataType]` target not supported (struct, generic,
abstract, file-local or global namespace).
- MODELGEN038: the definition of an encodeable base type can't be found.
- **New validator errors:**
- a structure with both optional and subtyped fields (OPC 10000-6 F.13);
- multi-bit or duplicate OptionSet masks.
- **Diagnostic locations:** `[DataType]`/`[NodeManager]` diagnostics
point at real source locations again, so `#pragma` and `.editorconfig`
apply. MODELGEN010 is the exception.
- **Incremental caching:** pipeline values compare by value, so
unrelated edits no longer regenerate the whole model or all
`[DataType]`s.
- **`ModelSourceGeneratorIgnore`** on a ModelDesign input is honoured;
the design still resolves as a dependency.
- **Cancellation:** a cancelled generation run propagates
`OperationCanceledException` instead of reporting a MODELGEN003 error.
- **Stack package:** the `Stack` NuGet package now ships
`build/OPCFoundation.NetStandard.Opc.Ua.SourceGeneration.Stack.props`,
so `StackSourceGeneratorMode` and `StackSourceGeneratorExclude` reach
the compiler.
## Related Issues
- No tracking issue. The audit, review and spec-grounding reports are
available on request. Contains the matrix wire-format change discussed
for D-5 (OPC 10000-6 5.2.5 Table 28).
## Checklist
- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
(`docs/SourceGeneratedDataTypes.md`, `docs/NodeManagers.md`, generator
readme)
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings. (`UA.slnx` 0 warnings / 0 errors on net48 and
net10.0)
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed. The
affected suites were run on net10.0: SourceGeneration Core/Roslyn/Stack,
Types, Core.Encoders, Encoders.Fuzz, Client, Client.ComplexTypes,
PubSub, Core, Server, GDS, Configuration, Di, Robotics, ISA95,
Positioning, OpenUsd, XRegistry. net48 was built; its test legs, and the
post-rebase verification, run in CI.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
||
|
Marc Schier
|
c4e0721a29 |
Add a documentation learning path and fix issues found while verifying it (#4555)
## Summary
### Documentation
- Stage the documentation index as a learning path: a first application,
independent client, server, and PubSub paths, then data and services,
companion models, production, advanced integration, and draft or
experimental capabilities.
- Add Getting started (repository samples or NuGet packages), OPC UA
concepts, Your first information model, and a production readiness
checklist.
- Add Native AOT application publishing, a first PubSub publisher and
subscriber exchange, and the UDP transport options.
- Add or expand contents navigation throughout `docs/`, and keep
contributor documentation separate.
- Correct outdated SDK examples and capability guidance, consolidate
repeated explanations into canonical guides, and repair local links and
heading structure.
- Rewrite What's New in 2.0 as a concise feature overview with short
linked details; keep migration instructions in the Migration Guide.
- Expand the feature catalogue with specification IDs, GDS, LDS, the OPC
10000-21 onboarding registrar, ISA-95, xRegistry, OpenUSD, and
sample-local models; distinguish draft status, explain crypto
pluggability, and bring performance guidance forward.
- Address review feedback on wording, active monitored items during
shadow reload, and the `Samples.md` filename and references.
- Regenerate the `opcua-v20-migration` plugin's bundled copy of the
migration guides so it matches the edited `docs/migrate/2.0.x` guides.
- Merge the latest `master`, preserving upstream resource-isolation
guidance within the task-based documentation index.
- Retain deferred Lingity findings where appropriate; this PR does not
claim that every document meets the Lingity score thresholds.
### Fixes found while verifying the guides
Each fix has regression tests that fail without it.
- **Client:** V2 subscriptions compared their lifetime with the
operation timeout instead of the session timeout, and logged a spurious
warning. `ISubscriptionEngineContext`, which is new in 2.0, gains a
`SessionTimeout` property; implementations must add it.
- **PubSub:** a subscriber now registers the `DataSetMetaData`
configured on its readers (OPC 10000-14 6.2.9.4), and the UADP decoder
decodes DataSetMessages that carry no ConfigurationVersion. Before, a
secured subscriber of the reference sample received no DataSets.
- **Source generator:** a ModelDesign method that declares its arguments
inline, without a method type, produced code that did not compile; it
now uses `MethodState`. The generated code of existing models is
unchanged.
- **MinimalClient sample:** waits for its first data-change notification
before it disconnects.
### Known issues, not addressed here
- UDP multicast: the publisher does not send on the interface that the
subscriber joins, and `MulticastLoopback` defaults to false. The PubSub
guide uses unicast for the exchange on one computer.
- The UDP metadata announcement is sent unsecured on secured writer
groups; a secured subscriber drops it with a warning.
- A server does not start in globalization-invariant mode, because
`ResourceManager` creates `CultureInfo("en-US")`. The Native AOT guide
cautions against the mode.
## Validation
- `git diff --check` and
`./.azurepipelines/validate-migration-plugin.ps1` (15 bundled migration
docs validated, plugin manifests synchronized).
- Local Markdown links, anchors, file-link casing, and contents lists
checked across all 87 documents under `docs/`. The index links every
top-level guide and every target of the previous index.
- Both Getting started routes, Your first information model, the PubSub
exchange, and the Native AOT publish of the Getting started client and
server were run as documented. The NuGet route and the tutorial use the
public `2.0.0-preview.5` packages.
- The changed libraries build for all target frameworks in Release
without warnings.
- Tests on .NET 10: Opc.Ua.SourceGeneration.Tests 199 passed;
Opc.Ua.SourceGeneration.Core.Tests 3,977 passed, 8 skipped;
Opc.Ua.Server.Tests `Fluent` category 621 passed; Opc.Ua.Client.Tests
subscription-engine tests 70 passed; Opc.Ua.PubSub.Tests 1,374 passed, 2
skipped; PubSub fuzz replay 145 passed. The new and affected tests also
pass on .NET Framework 4.8.
- Earlier merge validation: 174 resource-isolation tests passed on .NET
10.
- The full solution suite was not run locally.
Review and validation artifacts are maintained outside the repository.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
||
|
Cristi Pogacean
|
9b8a3b1188 |
Preserve node initialization and copied child ownership (#4544)
# Description Preserve populated derived node children through optional initialization and give copied children independent ownership by their copied parent. - Track initialization from a source in `NodeState` and guard generated optional-default recreation without removing the optional initialization hook. - Reuse `CloneChild` for dynamic children, generated typed children, and method argument properties. Preserve the clone root's existing external parent. - Add cross-model initialization/copy regressions, method-argument and nested-child ownership tests, and inherited alarm-subtree checks. This PR contains only the six files for this fix group. Certificate-store, runtime-type-loader, publish-queue and transport-exception fixes are separate work. ## Related Issues Fixes #4525. Fixes #4526. ## Validation Fresh focused Release/net10.0 checks on the final base: | Fixture | Passed | Failed | Skipped | | --- | ---: | ---: | ---: | | CrossModelInstanceTypeDefinitionGeneratorTests | 9 | 0 | 0 | | Types NodeStateTests | 227 | 0 | 0 | | Core StateTypesTests | 13 | 0 | 0 | All three test commands exited 0. `git diff --check` and the changed-file diagnostic check are clean. Regression proof was also executed in the isolated worktree: with the three production changes reversed, six generator cases failed with the missing derived Temperature child while three controls passed; two copy cases failed parent-identity assertions while the external-parent/absent-arguments control passed. The production fix was restored and the focused checks are green. The retained red runs preceded the later upstream synchronization; the affected production paths were unchanged by that synchronization. Windows win-x64 NativeAOT publish and native execution also passed: main 178/178, Historian 2/2, MCP 3/3, no skips, every invocation exit 0. These native runs preceded the final CTT-configuration/notes-only fast-forward; this PR's six files were unchanged. Local validation is intentionally change-scoped for this work, with the full matrix and repository-wide checks left to PR CI. Repository guidance and test assertions/timeouts were not modified. Local commands used a NuGet v2 source override and disabled audit only on the command line; CI feed/audit policy is unchanged. Earlier broad attempts are not claimed as a completed local gate: an unmodified-base durable restart timeout was reproduced before synchronization (the later durable run passed), and the net48 runner stalled after recording passing client tests. A direct client retry subsequently exited 0, but the full net48 matrix was not completed. Those results are not substituted for the focused evidence above. ## Checklist - [x] I have signed the CLA and read the CONTRIBUTING doc. - [x] I have added regression tests and demonstrated failure without the production fix. - [x] The focused tests pass; no new compiler diagnostics were reported by those builds. - [ ] Coverage and the full net48/net10.0 matrix are verified by PR CI. - [ ] All CI, CodeQL, and review feedback is addressed. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
romanett
|
96c5ef1623 |
Run the fluent configure pipeline in FluentNodeManagerBase (#4537) (#4538)
## Proposed changes Closes #4537. The configure pipeline now lives in `FluentNodeManagerBase`, so a hand-written fluent node manager only overrides `ConfigureAsync` and no longer writes `CreateAddressSpaceAsync` itself. `FluentNodeManagerBase.CreateAddressSpaceAsync` now: 1. loads the predefined nodes (as `AsyncCustomNodeManager` did before), 2. creates a builder for `NamespaceIndex` with `CreateFluentBuilder`, 3. awaits `ConfigureAsync(builder, ct)`, 4. runs `RegisterAuthoredNodesAsync` and then `CompleteConfigureAsync`, 5. finishes with `SealConfigurationAsync`, which seals the builder, replays `NotifyNodeAdded`, then activates behaviors and simulations. ### Breaking change (2.0) Some managers need a different order, for example a post-setup runner before sealing, a builder for another namespace, or work after sealing. They must stop calling `base.CreateAddressSpaceAsync`, because that would now run a second, empty pipeline and seal the manager-owned registries too early. These managers now call `LoadPredefinedNodesAsync(SystemContext, externalReferences, ct)` and keep driving their own pipeline: - the source-generated `NodeManager` template (the ordering test in `NodeManagerGeneratorTests` is updated) - `DiNodeManager`, `Isa95NodeManager`, `PositioningNodeManager`, `RobotIntentNodeManager`, `VisionNodeManager` - `samples/OpenUsd/SiteCompositionServer/SiteNodeManager` These now use the base pipeline: - `samples/FluentApi/BasicFluentApiServer/BasicFluentNodeManager` becomes a single `ConfigureAsync` override. Before this change it never sealed its builder. - the hosting `FluentNodeManager` (from `AddNodeManager(namespaceUri, build)`) becomes a `ConfigureAsync` override. It now seals through `SealConfigurationAsync`, so `OnNodeAdded` handlers wired in the build delegate fire. Before, it called `builder.SealAsync()` directly and they did not. Docs (`NodeManagers.md`, `DeviceIntegration.md`) are updated. `NodeManagers.md` gets a short hand-written `ConfigureAsync` example. ## Types of changes - [x] Breaking change (fix or feature that would cause existing functionality to change) - [x] Enhancement (non-breaking change which adds functionality) ## Checklist - [x] I have read the CONTRIBUTING doc. - [x] I have signed the CLA. - [x] I ran tests locally with my changes, all passed. - [x] I fixed all failing tests in the CI pipelines. - [x] I fixed all introduced issues with CodeQL and LGTM. - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added necessary documentation (if appropriate). ## Tests New `FluentNodeManagerBasePipelineTests`: nodes authored in `ConfigureAsync` get registered, the Objects-folder reference is mirrored into `externalReferences`, `OnNodeAdded` fires once, and the builder is sealed afterwards. Local run on net10.0, all passing: - Opc.Ua.Server.Tests (Fluent/Hosting/StateMachine/RuntimeNodeSet filter): 1286 - Opc.Ua.SourceGeneration.Core.Tests (NodeManager filter): 40 - Opc.Ua.Di.Tests 426, ISA95 136, Positioning 69, Robotics 636, Robotics.Intent 22, Vision 484 (+1 skipped), Vision.Intent 14, OpenUsd 921 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Marc Schier
|
138086863b |
Fix audit findings and review regressions across the OPC UA stack (#4503)
## Scope Audit fixes and review follow-ups across the OPC UA Core, Client, Server, transport, and redundancy components. This includes implemented Tier 1, Tier 2, and Tier 3 work; it does not claim that every item in the 244-finding audit backlog is complete. The review batch fixes mandatory full-key record protection and owned plaintext buffers; transactional pending-certificate claims; shared-channel nonce and certificate ownership; discovery and token-policy validation; reconnect/callback ordering without changing unbounded buffering defaults; server permissions, filters, monitored-item lifetime, and alarm topology; historian paging, retention, and annotations; atomic user/password metadata; and canonical file identities with bounded alias and role refresh workers. ## Review-batch verification at `ab686ccae` The following results come from fresh test builds using `dotnet test`, `--no-restore`, `-m:1`, and an explicit `CustomTestTarget`. They replace the earlier description's library-build/`--no-build` totals, which did not establish the state of the merged test sources. | Scope | Target | Passed | Skipped | |---|---|---:|---:| | Core review regressions | net10.0 | 827 | 0 | | Client and lifecycle regressions | net10.0 | 630 | 0 | | Server review regressions | net10.0 | 1970 | 0 | | Redundancy Server / Client / PubSub | net10.0 | 276 / 10 / 11 | 0 | | Sessions / file-and-alias / streaming / HTTPS-WSS integration | net10.0 | 8 / 21 / 30 / 20 | 0 | | Core review regressions | net48 | 786 | 39 | | Client recovery and policy regressions | net48 | 54 | 0 | | Server ownership, aliases, queues, and user-state regressions | net48 | 137 | 0 | Every listed run exited successfully. The 39 net48 skips are existing platform/capability cases. New key-agreement tests assert unavailable-policy behavior on unsupported targets rather than adding skips or pretending those targets performed an unsupported handshake. Additional exact node/permission/filter/alarm checks passed 13 Core and 46 Server cases. The PR-scoped formatter verification covers 26 projects and 389 non-generated C# files. A Roslyn syntax audit covers 53 changed interfaces and 263 members, with no missing parameter/type-parameter/return contracts and a 120-column limit for their declarations and documentation. No full-solution, all-target-framework, or NativeAOT matrix is claimed from these local runs; the new CI run must establish its own result. ## CI follow-up at `e1bc3ac3b` The follow-up fixes the failures reported by Azure build 18400. TLS certificates retain configured DNS names, including mixed-case URI schemes, without relaxing hostname validation. Reverse-WSS fixtures use the DNS identity in the client certificate. Reconnect completion is published only after cycle cleanup, so a subsequent reconnect cannot join an old completed cycle and retain old certificates. Certificate-alarm roots now support event subscriptions. The pump sample has one continuous notifier path, with no duplicate shortcut. Historian and aggregate fixtures share the provider's retention clock. Alarm collection lets the server expire idle Publish requests rather than canceling only the client wait. The corrected server, WoT, and managed-session fixtures exercise valid requests and initialized lifecycle contracts. Downlevel crypto tests preserve real credential and key-ownership assertions without enabling unsupported raw-ECDH policies or adding skips. Fresh local evidence after integration includes: | Scope | Target | Passed | Skipped | |---|---|---:|---:| | Client lifecycle / channel ownership / builder fixtures | net10.0 and net48, each | 96 / 103 / 123 | 0 | | Combined server lifecycle, events, aggregates, crypto, and contracts | net48 | 358 | 0 | | Full lifecycle plus certificate-alarm and aggregate fixtures | net10.0 | 116 | 0 | | Final server-contract and history-routing delta | net10.0 and net48, each | 84 | 0 | | Subscription / XRegistry / WoT event regressions | net10.0 | 7 / 2 / 3 | 0 | | Full Sessions project | net10.0 | 780 | 379 | | Full ComplexTypes project / downlevel resolver fixtures | net10.0 / net48 | 2854 / 15 | 0 | | Full WoT project / downlevel structured-write fixtures | net10.0 / net48 | 1496 / 27 | 0 | | TLS domain-name regressions | net10.0 and net48, each | 21 | 0 | All listed runs exited 0. The Sessions skips come from existing endpoint and policy capability guards; they are not counted as passes. No skip was added. Worker validation also covered full DI (426 passes), full History (540 passes, 21 existing skips), 38 exact downlevel event/history failures, and 60 repeated certificate-rotation race cases. The final crypto fixtures passed 73 cases on each target with the unchanged leak detector; a temporary premature-disposal mutation made all three borrower tests fail and was then removed. All 29 C# files in this follow-up pass included-file formatting at info severity across 12 projects. The complete PR interface audit now covers 54 interfaces and 271 members with zero missing contracts or line-width findings. No full-solution or NativeAOT matrix is claimed. New-head CI has started and remains authoritative for the remote result. ## Test-result gate follow-up at `4b6e1203a` Azure build 18401 reported the net48 Client job as successful despite a test-host crash after only 264 passing tests. Its old evaluator counted per-test failures but ignored the failed run verdict. This was a partial run, not a benign exit after the suite had completed. The shared result evaluator now rejects aborted, errored, incomplete, malformed, or inconsistent TRX reports. Each file must describe a completed run, so a passing file cannot mask another file's failure. It retains support for completed runs followed by a benign nonzero host exit, existing skipped-result conventions, and no-TRX MTP jobs. All 78 regression cases pass after integration using Windows PowerShell 5.1. Real completed Sessions and Client reports also pass. The process-test fixture follows the existing NET10_0-only tools-test pattern; its net48 build is not counted as a test run. Formatting passes for the added test file. The Client host crash itself has not reproduced locally. The isolated fixture passed 59 tests, and two fresh full fast-CI selections each completed with 2,392 passes and three existing skips, including a run with crash collection enabled. The CI merge tree is identical to the tested source tree. No speculative runtime change was made, and the original partial CI run is not claimed as a full-suite pass. Azure build 18402 has now completed the full net48 Client run: 2,392 passed, three existing console skips, and a successful test-host exit. The corrected evaluator independently validated outcome=Completed. Across the matrix, 119 test-project jobs succeeded; Sessions net10.0 was canceled when the remote provider deprovisioned its runner, so the overall Azure pipeline was canceled. That cancellation is not claimed as a code failure or a successful run. CodeQL and three Docker image jobs were still running at the latest check. The migration discovery failure remains separate. ## Documentation follow-up at `57ed02bea` The environment blocker has been removed by the maintainer. The three bundled migration references now exactly match the canonical documents after deterministic link rewriting; all 15 bundled documents pass `validate-migration-plugin.ps1`. The canonical migration prose was not rewritten. `FileSystemClient.md` now documents that extended Windows roots preserve literal trailing-dot names, completing the filesystem review follow-up. The filesystem thread has been replied to and resolved after publication. The review pass now has 108 of 110 threads resolved. The two explanation-only threads remain open as requested; no unfinished change request is being represented as resolved. ## Cross-platform follow-up at `0d9d936a8` With migration discovery restored, the Linux and macOS test matrix exposed two stale durable-subscription expectations. The expected value 83,442 encoded the old 32-bit overflow when converting 3,600 hours to milliseconds. The existing overflow-safe server correctly returns 14,400,000 lifetime intervals at a 900 ms publishing interval. Only those two expected constants changed; production behavior, negotiated-hour checks, and all other assertions remain unchanged. The exact local lifetime selection reproduced both failures before the correction. The full durable suite now passes 36 cases with 11 existing skips on each of net10.0 and net48. All four lifetime cases executed and passed on both targets, and included-file info-severity formatting passes. ## Session shutdown follow-up at `8daf7f091` A late net48 Sessions failure exposed a shutdown race. Canceling an in-flight keepalive read can produce a transport `ServiceResultException`. The retiring worker treated its own cancellation as proof that the peer was unavailable, so `CloseAsync` or `DisposeAsync` could skip the server-side `CloseSession` request. The worker now ignores that service error only when its own token is canceled; genuine peer errors still update liveness. The full integration fixture reproduced the original error-code mismatch. Four deterministic mock-transport cases then produced three failures and one passing genuine-error control before the fix. They verify successful close, returned server error, asynchronous disposal, and unchanged behavior for a real keepalive failure. Fresh related runs pass 75 client and 101 integration cases on each of net48 and net10.0, with no skips. Ten repetitions of the formerly flaky full net48 fixture pass all 1,010 cases. All three changed files pass info-severity formatting, and temporary diagnostic instrumentation was removed. ## Atomic listener snapshot follow-up at `82dc22b5d` Ubuntu Core CI exposed an unsafe formatter rewrite in TCP listener disposal: `ConcurrentDictionary.ToArray()` had become a collection spread. A concurrent channel removal between the spread's count and copy operations could leave a default entry, causing a null dereference during disposal. The listener now uses the dictionary's atomic `Values` snapshot and disposes those channels outside the listener lock. Deterministic regressions close zero, one, or all channels at the copy boundary. Before the fix, the one/all-removal cases reproduced the exact null dereference; the unchanged-count control passed. All 23 admission/listener cases now pass on net10.0 and net48, and the full fast Core net10.0 suite passes 5,267 cases with 88 existing platform skips. Both changed files pass info-severity formatting. A bounded audit of the formatter's other snapshot rewrites found no further verified unsafe concurrent-registry copies. ## Shutdown-test observation follow-up at `58403ee8b` The net48 shutdown-order test raced its own observation: it read `m_publishWorkerTask` after starting shutdown, which can legitimately drain the worker and clear that field immediately. The test now captures the worker before shutdown starts and confirms it is initially running. It still requires the worker to finish while shutdown remains blocked by the held manager semaphore. Production behavior is unchanged. A temporary scheduling perturbation reproduced the exact null-field failure. The corrected capture passed 100 perturbed iterations; both the temporary repeat and yield were then removed. The final 126 subscription/lifecycle cases pass on each of net48 and net10.0, and formatting passes. A complete local Sessions net10.0 run also passed 780 cases with 379 existing capability skips in 7m20s after the earlier keepalive correction. ## Retirement and alias-startup follow-up at `1a048ff39` The shadow-generation test now observes its documented asynchronous cleanup through the existing bounded helper. Removing the replacement does not join a background drain that already claimed the retired generation. The test still checks live-registration removal immediately and requires the old node to disappear; no timeout or production lifecycle behavior changed. All 81 lifecycle cases pass on each of net10.0 and net48. Initial alias-node materialization now retries one timed-out, read-only snapshot query before creating nodes for that root. The retry is logged through source-generated logging. Each query retains the original shared 100 ms deadline; repeated timeouts, other errors, and cancellation still abort startup. Client searches and live refresh do not gain this immediate retry. Fault-injection tests cover success after one timeout, a persistent timeout, a different store failure, and cancellation. All 187 related alias/hosting cases and the original certificate-rotation integration case pass on each target; the positive test records exactly one retry warning. The alias documentation now accurately describes the shared request budget. All changed C# files pass formatter checks, and the complete 54-interface/271-member documentation audit passes. Fresh CI is running on `1a048ff39`; the previous completed matrix exposed only the two failures corrected in this batch and their dependent summaries. ## Sample endpoint allocation follow-up at `d08a94925` The strong-cluster test harness allocated OPC UA and Raft endpoints in separate released-port batches. Uniqueness was enforced only inside each batch, so the same OS-assigned port could appear in both protocol groups. The harness now requests one distinct batch for both groups and splits it before launching any replicas. Deterministic one- and three-replica allocation cases reproduced overlapping groups with the old plan and pass with the corrected plan. The complete 17-case short-haul sample suite passes without skips; all failover, historian-continuation, and service assertions remain unchanged. Both changed files pass info-severity formatting. This prevents intra-cluster self-collisions but does not claim to reserve released ports against unrelated external processes. ## Code-review follow-up at `085fd9636` A review of the complete branch diff found five defects this PR introduced. All are now fixed, each in its own commit. `RelatedTo`'s recursive-to-iterative rewrite defaulted the optional `includeTypeDefinitionSubtypes` and `includeReferenceTypeSubtypes` operands to `false`. Part 4 §7.7.4 specifies both default to `true`, so a filter that omitted them silently stopped matching subtypes. Dynamically created key-credential nodes derived their namespace from the containing folder, which is the standard `i=18155` node in namespace 0 — the namespace reserved for the OPC UA standard address space; they now resolve the server-owned namespace and fail with `BadInternalError` rather than guessing index 1. The retained audience-less `CreateTokenData` overload produced tokens that verification always rejects, so it is now `[Obsolete]` and throws instead. A failed reference to a remote parent left the new node registered with a one-sided inverse reference, and is now rolled back. An existing `.pfx` is restricted before it is rewritten, rather than after its new key bytes are on disk. One further reported finding was rejected after investigation. `JwksIssuerKeyResolver` deliberately does not dispose retired key snapshots: `RetiredKeysAreCollectibleWhileResolverRemainsAliveAsync` proves the resolver retains no retired snapshot, and `ReturnedKeysSurviveRepeatedRefreshAndResolverDisposalAsync` requires already-returned keys to keep verifying across refreshes and resolver disposal. Adding disposal would have broken that tested contract, so the exploratory change was reverted. Validated on net10.0 and net48: 256 Core and 633 Server cases pass, with one pre-existing net48 skip. The build reports zero compiler and analyzer diagnostics, all eight included-file info-format checks pass, the contract audit covers 58 interfaces and 310 members with no findings, and the 15 bundled migration documents validate. All 122 review threads are resolved; the key-credential namespace thread was reopened to retract an earlier incorrect answer and then resolved against the real fix. ## Review-body follow-up at `da0d59cfb` The re-review at `8daf7f091 + 82dc22b5d` listed four items in its summary that had no inline thread of their own, so they had not been picked up. All four are now fixed. Both event-dispatch permission gates had been tightened from not-Bad to `IsGood`, which silently dropped events whose permission verdict is Uncertain; the data-change path in the same file still treated Uncertain as deliverable, so the two had diverged. `EventSourceRegistry` only cleared `ConsecutiveFailures` when a source was deactivated, so a source that had backed off to the 30-second cap kept that delay for the rest of the process even after starting successfully. `UserNameIdentityTokenHandler` built a temporary issuer chain whose `Add` calls take independent certificate references, and never released them. The fourth was the receiver-side sequence rollover, which the reviewer had correctly re-flagged as still open despite its thread being resolved. `m_sequenceRollover` was set permanently, so the second legal wrap on a long-lived channel was rejected and faulted the channel along with the transport-reconnect OPN. OPC 10000-6 does not cap a SecureChannel at one wrap. The flag is now cleared once an accepted number has traversed the normal range, which keeps rejecting a replayed low number that never left the rollover window. Validated on net10.0 and net48: 636 / 593 Core cases and 135 / 134 Server cases pass, including the new `UaSCBinaryChannelSequenceRolloverTests` and the Uncertain-permission delivery cases. The build reports zero compiler and analyzer diagnostics and the changed files pass included-file info-format checks on both targets. The previous head `085fd9636` was fully green: 279 checks passed and Azure build 18414 succeeded with 137 of 137 jobs, after the platform automatically retried a job whose NuGet tool download had hit a transient TLS error. ## Reported issues #4508 and #4509 at `6ad1ecf16` **#4508 (V2 subscription recreate deadlocks against a faulted transport) was not yet fixed and is now closed by `bac260a06`.** Both drain sites the report identifies were still unbounded. A publish worker that started its request before the quiesce parks inside the service call on the channel ready gate while holding an active publish request; the recreate that would make the channel ready is the very operation awaiting the drain, so the wait never ends. The drain now cancels in-flight attempts through a per-attempt token linked to the worker token. An aborted worker rolls its acknowledgements back, releases the request and returns to the paused park instead of terminating, so it resumes when the caller leaves the quiesce. Publish ingress stays quiesced for the caller's operation, so `DropPendingForSubscription` keeps the guarantee the report asks to preserve and no blind timeout is added. The new regression parks a worker on a cancellable gate that never completes on its own: it reproduces the hang for the full 30-second budget before the change and passes after it. **#4509 (identity provider fails the initial connect) was already fixed for its primary symptom.** `HandleConnectAsync` materializes the provider's identity through `CreateIdentitySelectionContext` before the first `CreateAsync`, so Anonymous is no longer substituted against a UserName-only endpoint. `6ad1ecf16` adds the two remaining details from the report: the enabled-policy set now includes the negotiated channel policy, so a client cannot reject the policy it already accepted as `NotEnabledByClient`; and when discovery has not populated the endpoint's token policies the connect keeps the configured identity so the attempt fails at the transport layer for the reconnect policy to retry, instead of failing at the identity layer. Validated on net10.0 and net48: 477 / 473 client session and subscription cases pass, the build is free of compiler and analyzer diagnostics, and the changed files pass included-file info-format checks on both targets. The full `Opc.Ua.Sessions.Tests` suite also passes locally on net10.0 with 782 passed and 379 pre-existing capability skips. GitHub checks are green on this head: Build and Test .NET 10.0, Docker Sample Images CI, and NuGet Publish all succeeded. Azure build 18417 completed 134 of its 135 jobs successfully; its net10.0 Sessions job was cancelled by the hosted-agent provider ("Received request to deprovision"), the same infrastructure flake that hit build 18409 earlier on a head predating these changes. That job is not claimed as passing, and the local full-suite run above covers the same project. The previous head `da0d59cfb` completed Azure build 18415 with every job succeeded. ## Review follow-up at `a08f0c12b` Eleven new review threads arrived, nearly all documentation. Before acting on each "this is new in 2.0" remark I checked the API against `master378` rather than assuming, because a genuine 1.5 API still needs its migration entry. `ManagedSession`'s server-redundancy refresh had two hard-coded two-second bounds. The new `ServerRedundancyOptions` exposes `RefreshTimeout` for the metadata read and `PeerDiscoveryTimeout` for each peer lookup, defaults both to the previous two seconds, validates that a bound is positive or `Timeout.InfiniteTimeSpan`, and flows through `ManagedSessionOptions`, `ManagedSession.CreateAsync`, the fluent builder, and `DefaultServerRedundancyHandler`. `AsyncCustomNodeManager` and `KeyCredentialBridgeAuthenticator` are both absent from `master378`, so their obsolete members had no compatibility constraint and are removed rather than carried: `AsyncCustomNodeManager.ReadInitialValue`, the audience-less `CreateTokenData` that only threw, and the audience-less `CreateProof` that produced a proof no server accepts. `CustomNodeManager2.ReadInitialValue` and the `MonitoredNode2` migration section are genuine 1.5 APIs and are untouched. The documentation now states master's behaviour instead of the delta from 1.5.378, with each moved item already covered under `docs/migrate/2.0.x/`. `IHistorianAnnotationProvider` is described by its annotation-time key rather than as "legacy" — it is new in 2.0, so the label implied a compatibility constraint that does not exist. The KeyCredential bridge payload's `"version"` field is explained as a vendor-extension format version with no OPC UA counterpart, and its version-1 upgrade narrative is gone. `CertificateManager.md`'s migration walkthrough is replaced by the resolver API it targets. `NodeManagers.md`, `Sessions.md`, `AsyncServerSupport.md`, `Certificates.md`, and `Profiles.md` no longer announce features relative to 1.5. `Benchmarks.md`, `WhatsNewIn2.0.md`, `DeveloperGuide.md`, and `docs/migrate/` keep their 1.5.378 references, which are their subject. CI then exposed a defect this batch introduced: five fixtures resolve `ManagedSession`'s private constructor by its exact positional parameter types, so the added parameter compiled cleanly but made `GetConstructor` return `null` at run time. `7df1d5c40` updates every site; `Redundancy.Client` passes 126 of 126 and the affected client fixtures 112 of 112. The BouncyCastle question is answered and tracked in #4510: `Opc.Ua.Core` already project-references `Opc.Ua.Security.Certificates`, which already carries the conditional BouncyCastle reference, so the raw-ECDH polyfill belongs there rather than as a new Core dependency. The `IUserDatabase` metadata question is addressed to another maintainer and is deliberately left open. ## Reported issue #4509 follow-up at `5d4d13d63` The reporter verified `6ad1ecf16` and found one case still failing, correctly. The empty-token-policies branch kept the configured identity so a server that is down surfaces a transport failure rather than an identity error. A provider-only configuration has no configured identity, so it kept `null`: `OpenCoreAsync` turned that into Anonymous and `OpenValidateIdentity` rejected it with `BadIdentityTokenInvalid`, silently ignoring the configured provider. The branch now requires a configured identity to defer to. Without one the provider is consulted, so its own `BadIdentityTokenRejected` names the offered and rejected policy set instead. Two regression tests cover both configurations; the first fails against the previous behaviour by reaching the session factory with a `null` identity, which is the exact step that produced the reported error. That branch had shipped without a test, which is why the case was missed. ## Review and CI status at `5d4d13d63` **All actionable review threads are resolved: 132 of 133.** The single open thread is a question the reviewer addressed to another maintainer about moving user metadata into `IUserDatabase`; it is left open for them rather than resolved silently. Every resolved thread carries a reply naming its fix commit. **Latest-head CI is green on `5d4d13d63`: 279 checks passed, zero failed, zero pending**, with one conditional signed-package publishing job skipped. Local validation for this batch: the full `Opc.Ua.Client.Tests` suite passes 2,430 of 2,430 on net10.0, `Opc.Ua.Redundancy.Client.Tests` 126 of 126, and `Opc.Ua.Server.Tests` 590 of 590 for the affected selections. Both net10.0 and net48 build clean for `Opc.Ua.Client`, `Opc.Ua.Server`, and the changed test projects, and all changed files pass included-file info-severity formatting on both targets. The PR remains open; no merge was performed. As stated at the top, this does not claim that every item in the 244-finding audit backlog is complete. ## Latest CI follow-up at `b6f2dfa43` Azure build 18442 exposed a net48 compile regression in the Kafka adapter: `RuntimeHelpers.GetUninitializedObject` is unavailable on .NET Framework. Commit `2cf1e0eed` now uses normal `ProducerMessage` construction on `NETFRAMEWORK` while retaining reflection-based construction for modern targets that can load a different Dekaf runtime asset. The exact net48 CI selection passes 92 of 92 tests, and the netstandard2.1 adapter guard selection passes 11 of 11. After merging the latest `master`, GitHub Actions run 35643592139 is green. The initial Ubuntu Core.Schema job built successfully and passed 160 of 160 tests, then hit an external 403 while finalizing its test-result artifact; its failed-job rerun completed successfully. Azure build 18451 was canceled when its remote provider deprovisioned the net10.0 Sessions runner (`Received request to deprovision: The request was cancelled by the remote provider`). Azure attached no test task log or source error, and the canceled parent left that child check stale as in-progress. No source workaround, test skip, or check relaxation was made for either infrastructure failure. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208 |
||
|
Marc Schier
|
331a71ac2c |
Fix AI model version and inference argument handling (#4512)
## Summary
- Derive the AI server''s advertised specification version from
generated NodeSet metadata (new source-generated `ModelVersions.Target`
constant), so `AINodeManager.SpecificationVersion` can no longer drift
from the shipped model.
- Preserve inference parameters through inline, asynchronous, fallback,
and oversized-transfer execution instead of silently discarding them.
- Reject unsupported `PayloadUri` input explicitly with
`BadNotSupported` before any backend call, job creation, or transfer
allocation.
- Apply the supported `temperature` / `max_tokens` / `top_p` parameters
in both built-in backends via a shared parser.
Fixes #4505
Fixes #4506
## Review follow-ups (
|
||
|
Copilot
|
02fb72e2d3 |
Support guid/opaque nodeids in NodeSet import (#4507)
# Description
`NodeSetToModelDesign.ImportNode` only stored Numeric and String NodeId
identifiers. Guid and Opaque identifiers (both valid per OPC 10000-3
5.2.2) fell through to an `Information` log line, so the node was
registered in `NodesByQName`/`NodesById` with no identifier at all and
code generation continued against a node whose identity had been
discarded.
The `ModelDesign` schema has no representation for Guid/Opaque
identifiers (`NodeDesign` exposes only `NumericId`/`StringId`), so the
import now fails loudly instead of emitting output that looks correct.
- **Importer**: the fall-through branches in `ImportNode` are replaced
with an `InvalidDataException` naming the NodeId, browse name and
identifier type. This matches the existing unsupported-input handling in
the file and surfaces through `ModelCompilation` as a generator
diagnostic.
- **Logger removal**: `m_logger` had no remaining use in
`NodeSetToModelDesign` and was removed along with its
`Microsoft.Extensions.Logging` import.
- **Tests**: `ImportUnsupportedNodeIdTypeThrowsInvalidDataException`
covers Guid, empty Guid and Opaque identifiers via an in-memory NodeSet.
- **Docs**: the source generator readme states the NodeSet2 identifier
restriction.
```csharp
else
{
// The model design can only carry numeric and string identifiers.
// Guid and Opaque identifiers (OPC 10000-3 5.2.2) would be dropped
// and code would be generated for a node without any identity, so
// the import fails instead of silently producing wrong output.
throw new InvalidDataException(
$"NodeId ({input.NodeId}) of node '{input.BrowseName}' uses the " +
$"{nodeId.IdType} identifier type which is not supported by the model " +
"design. Only Numeric and String identifiers can be imported.");
}
```
Note on behaviour: a null/unspecified NodeId parses as a Numeric
identifier (`i=0`) and is still handled by the numeric branch, so the
removed `IsNull` branch was only ever reachable for an empty Guid or
Opaque identifier, which is now reported as unsupported.
## Related Issues
## Checklist
_Put an `x` in the boxes that apply. You can complete these step by step
after opening the PR._
- [x] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.
<!-- START COPILOT CODING AGENT SUFFIX -->
- Fixes #4504
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
||
|
Marc Schier
|
9f14ddf5de |
Preserve non-child hierarchical references during NodeSet import (#4491)
# Description Preserve forward and inverse same-namespace hierarchical references that are not already represented by the NodeSet importer's parent/child relationship. A reference is implicit only when both its endpoints and its exact reference type match the selected ownership relationship; additional links do not reparent their targets. Also fix the generated-reference duplication that blocked the previous attempt in #4473. Inherited and declared `NodeDesign` objects can differ structurally while resolving to the same runtime reference. Deduplicating by the emitted reference-type ID, direction, and target ID prevents identical `AddReference` calls from throwing during Robotics/OpenUSD startup. - Add 21 importer/generator regression cases, including a minimized recursive-placeholder reproduction of the startup failure. - Strengthen the reference-server test to check the original missing forward links, inverse counterparts, subtype filtering, and unchanged ownership. - Document ownership versus graph references in `docs/NodeManagers.md`. ## Validation Confirmed the missing-reference cases fail before the importer fix, and the minimized duplicate-reference case fails before the generator fix. The original Robotics/OpenUSD fixture passed before the changes, failed with the importer-only candidate, and passes with both fixes. | Suite | net10.0 | net48 | | --- | --- | --- | | SourceGeneration.Core.Tests | 3,961 passed; 8 existing skips | 3,961 passed; 8 existing skips | | SourceGeneration.Stack.Tests | 94 passed | 91 passed | | SourceGeneration.Tests | 185 passed | 185 passed | | Di.Tests, including all 17 RobotOpenUsdE2eTests | 425 passed | 425 passed | | ReferenceServerTests | 45 passed | 45 passed | Executed using `CustomTestTarget` with `dotnet test tools/SourceGeneration.slnx`, the Di test project, and the `ReferenceServerTests` filter in the Server test project. Scoped whitespace/style checks and `git diff --check` also passed for the changed code. Pre-existing analyzer diagnostics outside the changes were left untouched. The full `UA.slnx` test suite has not been run locally. CI and CodeQL results remain pending. ## Related Issues - Fixes #4484. - Addresses the hierarchical-reference limitation deliberately deferred in #4473. ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. |
||
|
romanett
|
e639932b95 |
Fix source generator defects from the Client/Types/SourceGeneration audit (#4473)
# Description
Fixes the SourceGeneration findings of a read-only bug audit of
`a05a36055` — sections 15 (host generators, Roslyn side), 16
(SourceGeneration.Core schema / ModelDesign loader) and 17
(SourceGeneration.Core generators). 36 of the 39 findings are fixed;
each has a regression test.
### Generated-code correctness
- **State machine NodeId constants** were emitted as `const uint` even
when the aliased `Objects` constant is a `string`, so every model using
string NodeIds with a `FiniteStateMachineType` subtype failed with
CS0029. The constant type now follows the aliased node, looked up from
the same source `NodeIdGenerator` emits from — top-level nodes **plus
each node's instance hierarchy**, since a state or transition only ever
exists as a hierarchy entry.
- `NodeIdGenerator` emitted string identifiers into C# string literals
without escaping them.
- Structure field names are authored data, so they can be C# keywords or
collide with the members every generated data type carries. Properties
and `{Type}Fields` members now go through a sanitizing,
collision-breaking mapping (including two siblings that sanitize to the
same identifier). **Wire names are untouched** — OPC 10000-3 §8.51
requires the StructureField name to match the BrowseName, and the
`DataMember`/encode/decode names still come from `Parameter.Name`.
- `NodeStateGenerator`'s copy-on-write cast now uses the fully qualified
data type name, which a cross-model structure needs.
- `GetScalarValueAsCode` emitted `(double)NaN` and the infinities, which
do not compile.
- `TemplateWriter` formatted with the current culture, so a machine with
a comma decimal separator emitted invalid generated source.
### Served schema documents
- The binary schema described a **different wire layout than the
generated `Encode`/`Decode`**: unions carried no
`SwitchField`/`SwitchValue`, and structures with optional fields carried
no encoding-mask presence bits. Both are now emitted per **OPC 10000-6
§5.2.7**, matching the layout the runtime `BsdSchemaGenerator` produces.
A structure with more than 32 optional fields now fails generation
instead of silently wrapping `1 << 32` back onto the first field's bit.
- Documentation text and attribute values are XML-escaped.
### ModelDesign loading and validation
- Identifiers pinned in a CSV were dropped whenever any other loaded
namespace declared the same symbolic name. Removal is now scoped to the
file's target namespace, and explicit `NumericId`s are keyed by
`(namespace, id)` so the same number in two namespaces is legal.
- Declaration order inside a design file no longer matters — base types
are imported and validated ahead of their subtypes.
- OptionSet `OptionSetValues` were truncated at bit 31 (signed shift)
and bits 32–63 of a 64-bit option set were never emitted.
- An authored `ValueRank` is no longer overwritten by the shape of the
default value (OPC 10000-3 §6.2.8: a ValueRank may only be further
restricted), and a VariableType data type may be narrowed any number of
levels deep rather than exactly one.
- NodeSet import: `AccessRestrictions="0"` means "no restrictions", not
`EncryptionRequired` (OPC 10000-3 §8.56) — an unrecognised mask stays
**fail-closed**; a NodeSet without `<Models>` reports what is wrong; an
optional `<References>` no longer throws; the parent/child reference
scan looks at the parent's references; and `IsTypeOf` terminates on the
self-referential root reference types NodeSets routinely declare.
- Version comparison is numeric per component (ordinally `"1.05.9"`
sorts above `"1.05.10"`) and now lives on `SemVer`, so the NodeSet
collection and the dependency tie-break cannot pick different winners; a
publication-date version no longer outranks a real one.
### Host generator
- `[DataType]` on an `enum` is matched again (an enum is not a
`TypeDeclarationSyntax`), MODELGEN003 renders its arguments instead of
the raw template, and the Types hint name keeps the namespace's dots so
`A.BC` and `AB.C` cannot claim the same one.
- Compilation-derived pipeline stages carry value comparers, so editing
a file no longer re-runs the entire model generation on every keystroke.
- A NodeSet that fails to load skips that file rather than every
remaining one; a file listed twice as an `AdditionalFile` is deduped;
superseded NodeSet versions are still recognised as NodeSet inputs and
not generated a second time by the ModelDesign pass.
- `LogLevel.Critical` maps to a diagnostic instead of being silently
dropped.
### Client
- `EventRecordFieldReaders` gained the readers the event record
generator needs (including `Variant`, enumerations and the array forms).
Fields whose type had no reader were declared on the record but never
populated, so they read as `null`/`default` for every event.
## Not fixed (deliberately)
- **Forward same-namespace hierarchical references in
`NodeSetToModelDesign.ImportReferences`** — the audit flags the
inverse-only handling as asymmetric. Making it symmetric compiles and
passes every generator unit test, but regresses the address space: all
17 `RobotOpenUsdE2eTests` fail in `OneTimeSetUpAsync` because the hosted
server never finishes starting. Reverted, with a comment at the site
recording why.
- **§17.10 and §15.10** (both low + PLAUSIBLE) — each needs a wider
refactor than this PR: a consistent NodeId-constant rename across ~6
cross-reference sites, and replacing `Location` with a serializable
equivalent for incremental cache equality.
## Spec cross-check
The encoding/schema changes were checked against the specification: OPC
10000-6 §5.2.7 (32-bit EncodingMask, one bit per optional field in
order, padded), the union `SwitchField` encoding, OPC 10000-3 §8.56
(AccessRestrictionType bit mask), §6.2.8 and §5.6.2/§5.6.5 (ValueRank),
§8.51 (StructureField name == BrowseName).
`GenerateStackTests.OptionalFieldBitsAgreeBetweenBsdAndGeneratedEncoder`
pins the BSD presence-bit order against the generated encoder's
`LogRecordFields` so the two cannot drift apart again.
## Related Issues
_No tracking issue — this PR implements the SourceGeneration sections of
a bug audit. Happy to open one if maintainers prefer._
## Checklist
- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.
> Note on the test checkbox: verified on **net10.0 only** — `UA.slnx`
builds clean (0 warnings, 0 errors) and
`Opc.Ua.SourceGeneration.Core.Tests` (3915),
`Opc.Ua.SourceGeneration.Tests` (184), `Opc.Ua.Di.Tests` (425, including
the `RobotOpenUsdE2eTests` end-to-end fixture that catches address-space
content changes) and the `EventRecordFieldReaders` tests (16) all pass.
.NET Framework has **not** been run locally; CI needs to confirm it.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
|
||
|
Marc Schier
|
f5ad124891 |
Additional fuzzing plus fixes for every defect the campaigns found (#4459)
# Description
Brings the stack's fuzz testing to parity with the fork
(`aio-opcua-sdk`), then fixes every defect the resulting campaigns
exposed.
The fork's fuzzing was used as the behavioural baseline: all **28
fork-local callbacks** and **13 OneFuzz registrations** are now
accounted for by an explicit mapping, not just by a larger target count.
The work extends the existing SharpFuzz/NUnit infrastructure rather than
introducing another framework.
### Coverage
- **84 continuous libFuzzer targets** and **81 AFL adapters** across
Encoders, Certificates, Network and PubSub.
- **PubSub area integrated** (Corpus/Tools/Tests siblings, solution
entries) — it previously had host source only and was absent from the
solution and replay infrastructure.
- Segmented binary adapters, binary-to-JSON and XML-to-JSON conversions,
standalone certificate loading, both chain-parser variants, CRL encoding
and canonical TBS round-trips.
- The 12 original fork message inputs are retained byte-identically
alongside richer deterministic generators; the three historical
crash/slow/timeout reproducers are unchanged (verified identical by
SHA-256).
### Production fixes found by fuzzing
These are real robustness and data-integrity bugs, each with a retained
regression input:
- `UadpBinaryReader.Advance` accepted attacker-controlled lengths on the
receive path. Declared payload sizes are now authoritative bounded
sections, with negative-count rejection and overflow-safe reassembly
arithmetic.
- PubSub JSON decoding leaked `InvalidOperationException` for invalid
UTF-8.
- `JsonEncoder.WriteExtensionObject` leaked
`System.Text.Json.JsonReaderException`.
- `JsonEncoder` silently emitted a lossy `{}` for an `ExtensionObject`
whose TypeId could not be represented in JSON.
- `JsonDecoder` wrapped a bodyless `ExtensionObject` envelope as a raw
JSON body instead of preserving `ExtensionObjectEncoding.None`.
- `Node.Encode` lost the null/empty collection distinction.
- Binary/XML Variant matrix dimensions are prevalidated rather than
relying on a caught `ArgumentException`.
- `X509CRL.ReadTime` pre-validates strict RFC 5280 whole-second Zulu
time forms.
- `Utils.ParseCertificateChainBlob` disposes correctly on partial-chain
failure.
- **`nodeid.dict` was unloadable by libFuzzer** (empty token, unescaped
quote). libFuzzer rejects the whole file on one bad line, so four parser
targets silently executed **zero** inputs. Fixed and now guarded by
dictionary validation.
### Infrastructure
- `fuzzing/fuzz-targets.json` manifest recording adapters, corpora,
dictionaries, oracles and fork-behaviour mappings, with
`validate-fuzz-manifest.ps1` and **27 negative controls**.
- Strict host/tools discovery and replay: `--list`, `--replay`, non-zero
exits on unknown targets, missing or empty corpora, and failed callbacks
(playback previously logged failures and returned success).
- A real process watchdog for the timeout/slow regressions — NUnit
`CancelAfter` cannot interrupt a synchronous fuzz callback.
- Runner scripts made non-destructive (unique work directories, no
generic `bin`/findings deletion) and corrected to the native driver's
single-`--target_arg` apphost contract.
- Opt-in `OneFuzz=true` class-library publication producing a
relocatable, **uninstrumented** drop, validated by exact callback
discovery, dependency closure, corpora and dictionaries.
- CI: fuzz replay matrix plus a public drop gate, triggered by corpus-,
dictionary- and script-only changes.
### Verification
| Gate | Result |
|---|---|
| Bounded campaigns, all 84 targets (1,000 inputs, seed 1) | 84/84,
84,000 executions, zero findings |
| OneFuzz drop | 84/84 published callbacks replayed, zero skips |
| Fuzz replay net10.0 / net8.0 / net9.0 | 9,470 passed each, 0 failed |
| Fuzz replay net48 | 8,996 passed, 0 failed (Network/PubSub are
modern-only) |
| `Opc.Ua.Types.Tests` / `Opc.Ua.Core.Tests` | 9,800 and 4,442 passed, 0
failed |
| Retained regression assets | 132 `crash-*` inputs replayed by every
target each run |
### Known gaps (deliberately not closed)
- **Fidelity is not enforced against the pipeline crash corpus.** The
differential binary-to-JSON targets assert both robustness (no unhandled
exception, expected of arbitrary bytes) and fidelity (a decoded value
re-encodes equivalently, a property of well formed values). The pipeline
supplies 22,293 blobs collected for the targets that existed before this
PR, none of which asserted fidelity; 522 of them violate it, and that
count did not move across two rounds of genuine fixes because each fix
only lets the mutator reach the next representability gap. Rather than
weaken the assertion, the two contracts are separated: fidelity failures
raise `EncodingFidelityException`; curated assets under `Assets/Repo`,
the curated `Testcases` corpus and every continuous libFuzzer campaign
stay strictly enforced, while externally supplied assets are gated on
robustness only and their fidelity findings are reported with counts and
bounded base64 reproducers.
- **Internal MSFT OneFuzz canary has not been run.** It needs an
owner-supplied ownership profile, pipeline authorisation and corpus
snapshot; none were available. The drop builds and validates locally and
records `serviceCanaryPerformed: false`. A build or upload is not a
successful worker run.
- **Legacy JSON wire forms are a partial mapping.** The fork accepts its
four original `TypeId`/`Body` messages; this stack rejects that retired
envelope (measured JSON-only control: fork `JsonEncoder` 630/1356 lines
vs 0/1007 here). Recorded as an explicit unresolved mapping rather than
an exemption. `LegacyNonReversible` is a RawData-derived profile and is
not counted as independent coverage.
## Related Issues
_No tracking issue was supplied for this work; happy to open one and
link it if maintainers prefer._
- Fixes #github-issue-number, ...
## Checklist
- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [x] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.
> Note on the checklist: the fuzz replay matrix, `Opc.Ua.Types.Tests`
and `Opc.Ua.Core.Tests` were run locally on **net48** and **net10.0**
(plus net8.0/net9.0) and all passed. CI and CodeQL results are not yet
available for this branch, so those boxes are left unchecked. The CLA
box is for the author to confirm.
|
||
|
romanett
|
ececea15c8 |
Reimplement the GDS ApplicationsNodeManager as a generated fluent node manager (#4434)
# Description Builds on #4428, which is now merged; this branch is rebased onto master. The GDS `ApplicationsNodeManager` becomes a source-generated `[NodeManager]`: the companion model, its loader and the fluent plumbing are emitted, and what stays hand-written is the behaviour. Getting there needed three things from the shared surface, which are the first two commits. ## Commits **1. Add the fluent hooks a hand-written manager needs at startup** - `OnReadRolePermissions` / `OnReadUserRolePermissions` on the node builder. Every other `NodeState` `On*` hook was already fluent; these were not, and a server that grants access on something the static model cannot express has to compute the permission set per request. - `Node<TState>(TState)` returns a builder over a node the caller already holds, for nodes materialised after `Configure` resolved its graph. - `OnAddressSpaceReadyAsync` on `FluentNodeManagerBase` — the one seam between the address space existing and the wiring being applied, for managers whose wiring depends on asynchronous setup. `Configure` is a `partial void` and cannot await. `DiNodeManager` had already invented the hook privately, so its declaration becomes an `override`. **2. Let a `[NodeManager]` bind to a model a referenced assembly supplies** A model belongs to the assembly that emits its types, which leaves a node manager for it homeless whenever that assembly cannot reference `Opc.Ua.Server` — a model-only package shared with clients, for instance. Binding was rejected twice over: the design was skipped before the binding was matched (`MODELGEN010`), and fluent-accessors-only mode refused to run alongside manager generation (`MODELGEN014`). The binding is now matched before the skip decision, and a matched binding puts that design into accessors-only mode. A design nothing binds to is still skipped, so this cannot start duplicating types by accident. The emitted manager also gains what a real manager needs: `DefaultNamespaceUris()` plus a protected constructor taking a replacement set; `GenerateDefaultConstructor=false` to suppress the two-argument form; its own logger category; and the call to `OnAddressSpaceReadyAsync`. Generated fluent accessors also stop re-enabling nullable warnings — a bare `#nullable enable` undid the shared header's `disable warnings`, so any model with a structure-typed method argument reported CS8600 on every generated `out T` in a project that builds warnings as errors. **3. Reimplement the GDS `ApplicationsNodeManager` as a generated node manager** The design stays owned by `Opc.Ua.Gds.Common`; the server assembly only binds a manager to it. **NodeIds do not move.** `NamespaceIndexes[0]` must stay the application record namespace — it is the one the application database, the certificate request store and the base allocator mint ids in — so the constructor passes the namespace order explicitly rather than adopting the generated model-first default. A test pins both ends against a live server. Four overrides go, leaving `OnAddressSpaceReadyAsync` and `Dispose(bool)` — no address-space overrides at all: | Removed | Why it was unnecessary | | --- | --- | | `New` | The base allocator already mints into `NamespaceIndexes[0]`, with a thread-safe counter. The override existed only because `Create(assignNodeIds: true)` asks the factory with the declaration id still set; `AssignInstanceNodeId` nulls it and re-asks, documented as being there for exactly this allocator. | | `AddBehaviourToPredefinedNodeAsync` | Its `AuthorizationServiceType` branch re-wired a node that was already wired, and its `KeyCredentialServiceType` branch had nothing to wire — the model ships that folder empty. | | `GetManagerHandleAsync`, `ValidateNodeAsync` | Verbatim reimplementations of the base that also blocked the fluent base's virtual-node support. | | `DeleteAddressSpaceAsync` | A `// TBD` passthrough. | Everything else — all sixteen directory methods with their self-administration permissions, the certificate groups' concrete types and trust-list flags, and the authorization service (created through this PR's `Add` surface and wired in the same place) — is one `Configure` pass. **4. Bind the certificate groups from the `Configure` pass** Acquiring a certificate group and binding it to its nodes are different kinds of work: the first is I/O (`InitAsync` opens certificate stores and creates CA certificates), the second only touches the address space. Each now runs in the phase it belongs to, which puts the whole of the wiring in one place. Ownership no longer rides on the binding. A group joins `m_ownedCertificateGroups` the moment it is constructed, before `InitAsync` can throw, so a group that fails half way through is still disposed — which the previous code could not manage, keying ownership off a dictionary it filled only after initialization succeeded. > Commit 3's message says this move is impossible and blames `PushTest`. **That was a wrong diagnosis, and this commit supersedes it.** Two real defects were hiding behind those failures: - **`Create(..., NodeId.Null, ..., assignNodeIds: false)` does not null the NodeId.** `NodeState.Create` overrides it only when handed a non-null one, so a custom group node kept the namespace-0 id of its type declaration. `AssignInstanceNodeId` hid this by re-asking the factory with the id nulled; the builder validates instead, and rejected it. The node is now created with an id in the manager's own namespace, as the `Default` authorization service already was. - **The custom certificate group node was not browseable.** It answered `GetCertificateGroups` and `ReadNode`, but a type-filtered recursive browse from the Objects folder never returned it — which OPC 10000-12 §7.8.2 requires of a certificate group. Staging the node through the builder gives it the reference it was missing. ### Two `PushTest` methods change — please review these closely The second defect had been masking what those tests actually did. Selecting the group positionally, `groups[3]` resolved to the server's **own `ServerConfiguration` group** while the custom one stayed invisible, so `UpdateTrustListOfCustomGroupAsync` and `AddRemoveCertOfCustomGroupAsync` were exercising a *transactional* trust list under a custom-group name. Browse results, before and after: | # | Before (baseline) | After | | --- | --- | --- | | 0-2 | GDS Directory groups (`ns=3`) | GDS Directory groups (`ns=3`) | | 3 | **`DefaultApplicationGroup` `i=14156`** — the server's own | **`MyCustomGroup` `ns=2`** | | 4 | — | `DefaultApplicationGroup` `i=14156` | They now resolve the group by browse name — which asserts it is browseable at all — and expect the immediate-apply semantics the GDS actually gives the groups of its own `Directory`: `CloseAndUpdate` writes the stores at once and reports `applyChangesRequired == false`, so the `ApplyChanges` calls go away (they would return `Bad_NothingToDo`). The read-back assertions are what prove the writes landed, and they are unchanged. This is a genuine loss of the *transactional* trust-list coverage those two methods were accidentally providing — the server's own group is still covered by the other `PushTest` methods, which target it deliberately. **5. Select `GetCertificates` results by type instead of by position** `GetCertificatesAsync` checked entry zero of a parallel-array result. Which entry that is depends on the order the server reports its certificate types in, an empty result throws `IndexOutOfRange` rather than failing an assertion, and a second type going missing or unparseable would not be noticed. Every returned pair is now checked, and a failure names the certificate type. **6. Drop the `.Common` suffix from the three GDS projects** Every other companion specification ships as `<Family>` / `<Family>.Client` / `<Family>.Server` — Di, ISA95, Positioning, Robotics, Vision, WotCon, XRegistry. GDS was the exception, with a `.Common` suffix that named nothing. | Before | After | | --- | --- | | `src/Opc.Ua.Gds.Common` | `src/Opc.Ua.Gds` | | `src/Opc.Ua.Gds.Client.Common` | `src/Opc.Ua.Gds.Client` | | `src/Opc.Ua.Gds.Server.Common` | `src/Opc.Ua.Gds.Server` | **No source changes were needed.** `RootNamespace` was already the final name in all three projects — the namespaces have always been `Opc.Ua.Gds`, `Opc.Ua.Gds.Client` and `Opc.Ua.Gds.Server`, and only the assembly and package ids disagreed. Nothing compiling against these assemblies changes a `using`. The package ids do change, which is consumer-visible, so `docs/migrate/2.0.x/packages.md` gains a renamed-packages section — it is the document that undertakes to cover NuGet renames — and the migration skill's package table gains the two rows a 1.5.378 consumer needs. Build infrastructure that names the projects or their output moves with them: both solution files, `expected-packages.txt` (re-sorted — it is maintained alphabetically), both signing manifests, the reference-server Dockerfile, the metapackage nuspecs, and the analyzer's package dependencies. `preview-pack.slnx` is derived from `UA.slnx` and needs no edit. The migration analyzer's shim tree mirrors the source layout by its own documented convention, so `Gds.Client.Common/` becomes `Gds.Client/`. Prose naming past work is left alone — plan 23 still records that its deferral happened during the "GDS Client.Common modernization", which is the name that effort had. ## Validation `net10.0`, on master + these four commits, from a clean GDS PKI: - `UA.slnx`: 0 errors, 0 warnings in changed files (the solution's 30 pre-existing warning lines are CA1861/CA1850/CA1307 in `XRegistry`, `Types.Tests` and `WotCon.Tests` — same rules and projects as the base). - `Opc.Ua.Gds.Tests`: 1,117 passed, 0 failed, 54 skipped — identical to the base branch. - `Opc.Ua.Server.Tests`: 4,961 passed, 0 failed, 5 skipped (measured before commit 4, which touches only the GDS assembly and its tests). - `Opc.Ua.SourceGeneration.Tests`: 169 passed. - `Opc.Ua.SourceGeneration.Core.Tests`, NodeManager/Generators/Fluent: 84 passed; `GenerateCode`: 21 passed, 4 skipped. - `Opc.Ua.MigrationAnalyzer.Core.Tests`: 10 passed, 3 skipped. - After the rename: assemblies build as `Opc.Ua.Gds.dll` / `.Client.dll` / `.Server.dll`, with no `*.Common.dll` produced anywhere. - Commits 1-3 build standalone. - `Opc.Ua.Gds.Server.Common` also builds for `net48` and `netstandard2.1` (checked before the rebase onto #4428). Not yet run: the full `Opc.Ua.SourceGeneration.Core.Tests` suite (~3,800), and the older target frameworks since the rebase. ## Checklist - [x] Existing tests pass - [x] New tests cover the new surface (role permissions, resolved-node builder, referenced-model binding, constructor/namespace control, GDS namespace invariant, custom-group node namespace) - [ ] Full Core.Tests sweep and net48/netstandard2.1 re-run after the rebase 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
romanett
|
105508bcbd |
Mint runtime NodeIds through a single DefaultNodeIdFactory (#4433)
# Description
Nodes created at runtime now get their NodeIds from a single
`DefaultNodeIdFactory` rather than from a `New` override per
NodeManager.
The factory derives a deterministic identifier from a canonical,
length-prefixed browse path, which is injective where the classic
`{parent}_{child}` concatenation was not — `A_B` + `C` and `A` + `B_C`
both produced `A_B_C` — and it also records the parent's identifier type
and qualifies cross-namespace segments by URI, so identifiers no longer
shift with namespace-table ordering.
A NodeManager selects its style declaratively instead of writing
assignment code:
```csharp
NodeIdFactory = NodeIdFactory.WithMode(NodeIdAssignmentMode.Counter);
```
`NodeIdAssignmentMode` offers `String` (the canonical path verbatim, the
default), `Numeric` / `Guid` / `Opaque` (that path through SHA-256),
`Counter` (sequential, for nodes whose browse paths repeat — per-session
diagnostics, inference jobs, rediscovered assets) and `None`. The
counter
starts above `0x40000000`, out of reach of any authored model, so a
long-running server cannot walk a runtime instance onto a type node.
`New` overrides are gone from every NodeManager the stack ships except
two whose identifiers are a domain scheme rather than an assignment
policy: `FileSystemNodeManager`, whose NodeIds encode the file path they
resolve back to, and `RoboticsNodeManager`, whose build coordinator
reserves identifiers across managers with ownership tracking.
**Namespace rule.** Every identifier is minted into the NodeManager's
own
namespace. Neither the parent's namespace nor the browse name's is
consulted: a parent can belong to a companion-specification model whose
NodeIds are fixed by its NodeSet (`DeviceSet` in the DI namespace), and
a
browse name only names the type that declared the child. NodeManagers
whose instance namespace is not their first one rebase with
`WithDefaultNamespaceIndex`.
**Types vs instances.** An authored NodeId on a node that stands on its
own is kept. A node hanging off a parent is re-minted, because it
reached
the factory through `AssignNodeIds` walking a subtree copied from a type
declaration. Keeping those would alias every instance onto the type's
own
nodes, and the predefined-node index takes the last writer, so the type
would quietly become an instance rather than the clash being reported.
New `ISystemContext.CreateInstance(...)` builds an instance of a
generated
type and rebases its subtree through `AssignInstanceNodeId` — the path
the
generated `CreateInstanceOf<Type>` helpers already took. The AI
NodeManager was the one place calling `NodeState.Create` directly, so
its
roots kept the NodeId their state object is born with (the type's own);
it now uses the shared path.
## Breaking changes
- `IAsyncNodeManager` extends `INodeIdFactory` and gains `AddNode` and
`AddRootNotifier`. This lets the fluent surface mint and register
through the interface instead of type-testing for
`AsyncCustomNodeManager` and silently falling back to a concatenated
identifier when the test failed — that fallback was itself a source of
divergent NodeIds.
- `AsyncNodeManagerAdapter` delegates `New` to the wrapped NodeManager,
so
**`CustomNodeManager2` behaves exactly as before**: it mints nothing and
a node keeps whatever NodeId it already has. Servers built on it are
unaffected.
## Notes for reviewers
- `docs/NodeIdAssignment.md` is new and describes the whole mechanism:
the
factory contract, identifier formats, type declarations vs instances,
the generated helpers and their parameters, per-NodeManager behaviour,
and an inventory of every member involved.
- The three commits are independently revertible.
- Two pieces that look redundant are deliberately kept, with the
reasoning
recorded in the doc: `assignInstanceNodeIds` (it *suppresses* minting,
where the rebase rule *forces* it) and the generator's
`NodeId.Equals(TypeNodeIdConstant)` guard (it distinguishes a
caller-assigned NodeId from a declaration's).
- Fluent builder tests previously passed only because of the removed
fallback; they now run against a real `DefaultNodeIdFactory` through a
shared `FluentTestNodeManager` helper.
## Related Issues
_No tracking issue yet._
## Checklist
- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.
> Test runs were on **net10.0 only** — .NET Framework was not exercised
> locally. Suites run green on an idle machine: Server 4950/4955, GDS
> 1116/1170, WotCon 1225/1225, Vision 484/485, DI 424/424, AI 174/174,
> ISA95 136/136, Positioning 69/69, RobotIntent 22/22. One unrelated
> pre-existing failure remains in `Opc.Ua.Types.Tests`
> (`EveryEmbeddedExampleMatchesItsRecordedHash`) — a WoT fixture checked
> out with CRLF, fixed by
> `git add --renormalize tests/Opc.Ua.Types.Tests/Wot/Assets`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
romanett
|
668310f21f |
Make the fluent node manager's configure/seal path genuinely async (#4442)
# Description The fluent node-manager pipeline had two synchronous chokepoints, and two independent efforts have now hit both of them. `Configure` is a `partial void`, so it cannot `await`. A manager whose wiring depends on asynchronous setup had to invent a second hook outside the pipeline, create its own builder there, and hand-roll `CreateFluentBuilder(...).Configure(...).Seal()` inside it. `Seal()` was synchronous, so nothing a registration staged during `Configure` could be completed by awaiting. Registrations that needed to await either blocked — `EventSourceRegistry` did `.GetAwaiter().GetResult()` on the manager's root-notifier registration — or could not be expressed at all. ## What changed **`FluentNodeManagerBase.ConfigureAsync(INodeManagerBuilder, CancellationToken)`** is the awaitable wiring seam. It runs once per activation, with the manager's attached builder, immediately *before* the synchronous `Configure` partial(s) — so instances it materialises are in the address space by the time `Configure` wires callbacks against them. The default implementation is a no-op. It is a `virtual` method rather than a second `partial` declaration because a partial method can be optional (`partial void`, no return value) or awaitable (an extended partial method, which must be implemented) — not both, and the generator would otherwise have to detect which one the user declared. `partial void Configure(INodeManagerBuilder)` keeps working unchanged; a manager uses either hook or both. **`NodeManagerBuilder.Seal()` becomes `SealAsync(CancellationToken)`.** Sealing is now the single point where registrations that could not finish inside `Configure` get their turn to await. Every seal site awaits it, so a later change can move registry *setup* onto the same seam. **`DiNodeManager` now runs the standard pipeline itself** — create and attach the builder, `await ConfigureAsync`, `RegisterAuthoredNodesAsync`, `CompleteConfigureAsync`, the post-setup runner, then `await builder.SealAsync` — instead of handing subclasses a bare callback and leaving them to build and seal on their own. DI subclasses gain node registration and the reverse-reference pass, which they previously skipped. Sealing deliberately comes **after** the post-setup runner. The fluent registries are owned by the manager, not by a single builder, and sealing starts them — so sealing first locks post-setup configurators out of registering simulation loops of their own, which is exactly what `AddRobotics`' configurators do. Sealing before the runner makes `MinimalRobotServer` fail to start with *"Cannot add a simulation loop after the registry has started"*. Worth knowing for follow-up work: with several builders attached to one manager, the first seal freezes the shared registry for all of them. ## Workarounds removed - **`DiNodeManager.OnAddressSpaceReadyAsync`** — deleted. Its three overriders (`RoboticsNodeManager`, and the `PumpNodeManager` / `GeneratorNodeManager` samples) now override `ConfigureAsync` and no longer hand-roll `CreateFluentBuilder(...).Configure(Configure).Seal()`. - **`EventSourceRegistry`'s `.GetAwaiter().GetResult()`** on `AddRootNotifierAsync` — gone. `Publish(..., RegisterAsRootNotifier: true)` now stages the registration and `SealAsync` drains it, so the wait happens on an awaited path. A source whose root-notifier registration fails is rolled out of the registry before the failure surfaces, and a registry disposed before its builder sealed drops what it staged. - **`Isa95NodeManager.ConfigureStatusEvents()`** — the synchronous second builder pass is now `ConfigureStatusEventsAsync(CancellationToken)`, awaited from `CreateAddressSpaceAsync`. (`ConfigureCatalogChanges()` stays synchronous: it starts a background task and never seals a builder, so there was nothing to convert.) - **`IRoboticsBuildContext.Seal()`** — now `SealAsync(CancellationToken)`. These really were `NodeManagerBuilder.Seal()` calls behind a same-named method. The state transition still happens under the context lock; the builder's asynchronous completion runs outside it, so no `await` is taken while the lock is held. - The Pump / Generator incremental registration callbacks (`onRegistered`) changed from `Action<T>` to `Func<T, CancellationToken, ValueTask>` so their per-instance builder pass can await its own seal. No sync-over-async was introduced anywhere. `Dispose(bool)` is unchanged and stays non-blocking; teardown remains in `DeleteAddressSpaceAsync`. `NodeManagerBuilder.Seal()` and `IRoboticsBuildContext.Seal()` are removed rather than kept as synchronous overloads. A synchronous seal that silently skipped the asynchronous completion would reintroduce exactly the failure mode this change exists to remove. ## Generator `NodeManagerTemplates` emits, in order: predefined nodes load → `await ConfigureAsync(__m_builder, cancellationToken)` → `Configure(__m_builder)` and the typed `Configure` → `RegisterAuthoredNodesAsync` → `CompleteConfigureAsync` → `await SealConfigurationAsync(__m_builder, cancellationToken)`. `NodeManagerGeneratorTests` asserts that order literally, and that neither `__m_builder.Seal()` nor the synchronous `SealConfiguration(__m_builder)` is emitted. ### Merging with #4432 #4432 landed on `master` while this was in review and reworked the same seam from the other side: it split `Seal()` into `SealGraphAuthoring()` + `StartSimulations()` so a manager can replay `NotifyNodeAdded` *between* the two halves — sealing first stops a lifecycle handler authoring nodes nothing would register, starting the simulations last stops a simulated value change preceding its own node''s `OnNodeAdded` — and added `FluentNodeManagerBase.SealConfiguration(builder)` as the one call the generator emits. Both intents are kept. That split is precisely why the asynchronous completion could not simply live inside `SealAsync`: a manager reaching the seam through the split path would silently skip it, which is the failure mode this PR exists to remove. So the asynchronous half became its own step: - `NodeManagerBuilder.CompleteSealAsync(ct)` drains the staged root-notifier registrations, then starts the simulations. `SealAsync(ct)` is `SealGraphAuthoring()` followed by it. - #4432''s synchronous `StartSimulations()` is gone, folded into `CompleteSealAsync`. It had exactly one production caller after the merge, and leaving it standing would have kept a second, synchronous way to activate a builder that skips the staged registrations — the very hazard this PR removes. Activation is now one step with one entry point; the *split* itself stays, because the replay has to sit between the halves. - `SealConfiguration` becomes `SealConfigurationAsync(builder, ct)`: seal the graph, replay `NotifyNodeAdded`, then `CompleteSealAsync`. Activation still happens last, so #4432''s ordering guarantee holds unchanged. ## Consumers unblocked - **#4434 ("Reimplement the GDS ApplicationsNodeManager as a generated fluent node manager")** adds `OnAddressSpaceReadyAsync` to `FluentNodeManagerBase` purely because, in its own words, *"`Configure` is a `partial void` and cannot await"*. That hook is unnecessary once `ConfigureAsync` exists — the GDS manager overrides `ConfigureAsync` instead, and the ordering it wants (predefined nodes → async setup → `Configure`) is what the generator now emits. #4434 has not merged yet, so it can drop its addition rather than have it simplified afterwards. - **Branch `romanett/fluent-node-behaviors`** moved `SimulationRegistry`, `EventSourceRegistry`, `MonitoredSourceRegistry` and the alarm wiring onto `AttachToType`/`Attach` for *release* only, because activation ran from `CompleteConfigureAsync` and most managers only called the synchronous `Seal()`. Every seal site now reaches `SealAsync`, so registry and alarm *setup* can move onto behavior activation too — the remaining half of that work. ## Tests - New `DiNodeManagerConfigureAsyncTests` drives the real `DiNodeManager.CreateAddressSpaceAsync` and asserts the hook runs once with an attached builder, that nodes it stages are registered, that their references to externally owned nodes are published, and that the builder is sealed before `CreateAddressSpaceAsync` returns. - `PublishTests` now pins the root-notifier deferral contract: registration is staged during `Configure`, lands only on the seal, and draining twice does not double-register. - Every `Seal()` call site in the test suites became `await ...SealAsync()`, including the three #4432 added in `NodeSetImportBuilderTests` and `SimulationBuilderExtensionsTests`. ## Related Issues No tracking issue was opened for this; it is the shared prerequisite extracted from #4434 and branch `romanett/fluent-node-behaviors`, both of which had independently worked around the same synchrony. Happy to open one if maintainers want the ADR trail. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### What was actually run locally `UA.slnx` builds clean. Post-merge on **net10.0**: `Opc.Ua.Server.Tests` 5020/0 and `Opc.Ua.SourceGeneration.Core.Tests` 3813/0. Pre-merge the same run plus `Opc.Ua.Di.Tests` 425/0, `Opc.Ua.Robotics.Tests` 627/0, `Opc.Ua.ISA95.Tests` 136/0, `Opc.Ua.Positioning.Tests` and `Opc.Ua.Robotics.Intent.Tests` were all green; `MinimalRobotServer` was also started directly to confirm the hosted robotics path comes up. Three caveats, so the unchecked box above is not a mystery: the full suite was **not** run against .NET Framework locally; `Opc.Ua.OpenUsd.Tests` has one pre-existing failure on this machine (`StructuredCartesianCoordinatesAreAccepted` expects `"1.5"` and gets `"1,5"` — a German-locale decimal separator, unrelated to this change); and one pre-existing `CA1861` warning comes from `NodeSetImportIntegrationTests.cs`, which is byte-identical to `master` on this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Marc Schier
|
da7412c429 |
Add native xRegistry events and distinct Resource versions (#4396)
## Summary - support the xRegistry 0.6.0 distinct Resource/Version hierarchy: `Registry -> Group -> Resource -> Versions -> Version` - instantiate stable logical Resource nodes separately from exact Version nodes while preserving existing Version NodeIds - delegate logical Resource FileType operations and Version-view metadata to the selected default Version with handle pinning and shared writer reservations - make Resource and Version delete behavior role-stable and use `MetaEpoch` / `Epoch` respectively - use stable Resource SourceNodes, exact Version SourceNodes and the `Server -> Registry -> Group -> Resource -> Version` notifier chain - keep a model-metadata-gated client fallback for 0.5 flat Servers; 0.6 Servers expose no legacy aliases - target `HasWoTProjection` at the real stable logical Resource node - use `AsyncCustomNodeManager` for the registration, fast-path, and federation managers, with awaited lifecycle, node mutations, event delivery, and cleanup - handle startup rollback, reentrant deletion callbacks, undisclosed file handles, and retryable cleanup without blocking unrelated file operations ## Authoritative dependencies - companion specification: OPCF-Members/spec-drafts#32 at `d3399fca025025a73e5a6b599f850a81d985c060` - OPC UA binding: xregistry/spec#568 - xRegistry event semantics: xregistry/spec#563, merged as `66ee31e47fbadff6ab982ae1516f7d9065e46b64` - companion NodeSet SHA-256: `369f5f3f1814b333e4eeb52bfb4b6a730cdff372505a5f91a54c94f0016ac4cf` ## Review feedback - keep the WoT Connectivity information-model designation at `1.1`; draft revisions update the publication date without changing the model version - eliminate all 198 unique CA1861 diagnostics across 54 files and remove seven obsolete project suppressions - fix the analyzer-clean touched-path CA1307 and the hierarchy follow-up CA1846/CA1851 diagnostics without dropping legacy-TFM compatibility - recover the reconcile queue after callback faults/cancellation and continue accepted pending changes - remove migration guidance for the xRegistry managers, which are new in 2.0 ## Compatibility - public WoT Connectivity model designation remains `1.1` - generated event-record ABI, constructors, options, NodeIds, and stored documents remain compatible - the generic xRegistry managers are new in 2.0 and implement `IAsyncNodeManager`; use `IAsyncNodeManagerFactory` and the async lifecycle hooks (see `docs\XRegistry.md`) - persistence schema and document blobs remain compatible unless noted below ## Validation - merge validation at `980422f76` with upstream `e8bec27d5`: 454 targeted WoT/materialization/model/registry cases and all 327 XRegistry cases passed on both net10.0 and net48; all 29 net10 aggregation sample cases passed - feedback regressions at `308e8ba74`: 55 targeted queue and live registry event/projection/lifecycle cases passed on each of net10.0 and net48 - prior CI: all 266 checks passed at `6489a0908`; the unchanged Ubuntu Core certificate-counter test and dependent jobs passed on the one-time retry ([run 34117304035, attempt 2](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/34117304035/attempts/2)) - `Opc.Ua.XRegistry.Tests` — 327 passed on net10.0 and net48 for migration commit `32c914aaf` - `Opc.Ua.WotCon.Tests` net10.0 — 1348 passed - `Opc.Ua.WotCon.Bindings.Tests` net10.0 — 683 passed - `Opc.Ua.SourceGeneration.Core.Tests` net10.0 — 3807 passed, 8 skipped - `Opc.Ua.SourceGeneration.Tests` net10.0 — 145 passed - `Opc.Ua.SourceGeneration.Stack.Tests` net10.0 — 94 passed - affected XRegistry and WoT client/server projects build successfully - xRegistry NodeSet is byte-identical to the companion artifact - repository-wide CA1861 inventory — 198 unique diagnostics across 54 files reduced to zero - targeted changed-fixture validation — 1,098 passed, 3 skipped, 0 failed - constrained full `UA.slnx` net10.0 Release build — CA1861 = 0 - `Opc.Ua.XRegistry.Client` net472 and netstandard2.1 builds — 0 warnings, 0 errors - independent shared-array safety review — no significant issues - `git diff --check` The final review regressions cover concurrent reconcile/open, synthetic logical handle collisions across default switches, cross-session Close isolation, shared writer reservations, logical FileType property/change propagation, logical Labels delegation, role-fixed deletion, event sources, model-version gating, and stable `HasWoTProjection` targets. The companion specification and protocol binding are reviewed in the linked PRs above. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c79a1ed-6dab-4522-a9fc-6548caa78ad5 Copilot-Session: b605797f-a97a-4cd0-8f12-016d33c2fd88 Copilot-Session: 7ef369a5-2307-4f71-8a11-15af0e72f240 Copilot-Session: e716d455-8397-4d22-9c5a-398847ab7317 |
||
|
romanett
|
30ed2390a8 |
Import NodeSet2 documents into a fluent node manager (#4432)
## What
`INodeManagerBuilder.Import(UANodeSet, INodeSetImportFactoryProvider?)`
lets a source-generated (or hand-written fluent) node manager overlay a
NodeSet2 document that is only known at runtime onto the model it was
generated from.
Every document imported during one `Configure` pass forms a single batch
that is linked **exactly once** after the pass. A node may therefore
declare a `ParentNodeId` that lives in another document of the batch, or
a node the manager already owns — which is what makes an overlay
possible. Imported nodes resolve through
`Node(...)`/`NodeFromTypeId(...)` immediately, so they can be wired in
the same pass; they are registered with the manager when
`CompleteConfigureAsync` runs.
### Public surface
- **+1 method**: `INodeManagerBuilder.Import(UANodeSet nodeSet,
INodeSetImportFactoryProvider? factoryProvider = null)`.
- **+3 contracts** in `Opc.Ua.Server.Nodes`:
`INodeSetImportFactoryProvider`, `INodeSetImportFactory`,
`NodeSetImportDiscriminator` (TypeDefinition / MethodDeclaration /
NodeId).
- In `Opc.Ua.Export`: a public `UANodeSet.Import` overload taking a
`NodeSetImportStateFactory`, and a public batch
`LinkParentChildRelationships(context, nodes,
NodeSetImportLinkOptions)`. These replace what would otherwise have been
an `InternalsVisibleTo("Opc.Ua.Server")` on `Opc.Ua.Types`.
- `NodeState.IsExplicitlyDefinedChild(context, child)` and `protected
DetachExplicitlyDefinedChild(child)`, used to tell a generated slot
apart from an ordinary child.
### Typed states, no reflection
The generator emits `{Model}NodeSetImportFactoryProvider` — one factory
per model type/declaration, each calling a concrete constructor for an
*empty* state — and makes the generated node manager implement
`INodeSetImportFactoryProvider` by delegating to it. So the builder
resolves the provider from the manager (`NodeManager as
INodeSetImportFactoryProvider`); `Import` also accepts one directly, and
the generated manager exposes `partial void
AddNodeSetImportFactories(List<INodeSetImportFactory>)` for dependency
models. An imported node of a generated type materializes as its
generated `*State` subclass with no type lookup, which keeps the path
NativeAOT-safe.
Because the factories create empty states, an imported instance carries
exactly the children its document declares.
### Reconciliation
When an imported child lands in a slot the generated parent declares
(same BrowseName), it replaces the generated placeholder: the displaced
node and every descendant the document does not carry over are removed
from the manager, and references to them are retargeted at the
replacement. Replacing a node the same `Configure` pass already wired is
rejected with `BadInvalidState` rather than silently dropping the
wiring, and sealing a builder with an unregistered import batch is
rejected too.
### Also
- `RuntimeNodeSetNodeManager` now uses the same importer: its documents
are one batch, so a node may declare a parent in another document of the
group regardless of parse order, and duplicate NodeIds across documents
are detected while parsing.
- Linking resolves an imported child's reference type from its own
inverse reference (previously always `HasComponent`/`HasProperty`).
- Method import keeps the document's `UserExecutable`; it previously
copied `Executable` into it. View import keeps `EventNotifier`.
## Tests
- `NodeSetImporterTests` (15) — discriminator selection, generic
fallbacks, field/value/reference fidelity against a generic import,
cross-document linking, typed replacement of inherited and argument
slots, deferred linking preserving an application `Handle`, duplicate
NodeIds, factory misuse.
- `NodeSetImportBuilderTests` (15) — staging and lookup before
registration, batch linking, attaching to an existing node, provider
resolution, placeholder replacement with reference retargeting,
configured-placeholder rejection, seal/completion guards.
- `NodeSetImportIntegrationTests` (5) — a generated `[NodeManager]`
overlaying its own model on a running server: typed states through the
generated provider, cross-document linking, placeholder replacement, and
browse through a session.
- `NodeManagerGeneratorTests` (+4) — the emitted provider and the
manager's delegation, including that no factory uses
`Activator.CreateInstance`/reflection.
- `NodeSetAotTests` (+1) — the public state-factory import under
NativeAOT.
Full `Opc.Ua.Server.Tests` (4946), `Opc.Ua.SourceGeneration.Core.Tests`
(3821), `Opc.Ua.SourceGeneration.Tests` (165) and `Opc.Ua.Types.Tests`
pass locally on net10.0.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
romanett
|
1aed93bd03 |
Make the predefined node create lifecycle cancellable (#4437)
# Description
Registration of predefined nodes could not be cancelled:
`CreateAsPredefinedNode` ran the whole create lifecycle without a token,
and a cancellation that landed inside `AddPredefinedNodeAsync` could
still leave the manager half-indexed.
**Cancellable create lifecycle.** Adds a
`NodeState.CreateAsPredefinedNode(ISystemContext, CancellationToken)`
overload. The token is threaded through the internal
`CallOnBeforeCreate` / `CreateInternal` / `CallOnAfterCreate` walk, is
checked between nodes and on each convergence pass, and is passed to
every `OnAfterCreate` override. Cancellation leaves the nodes already
completed in the created state, so a later call resumes only the
remaining ones. The existing single-argument overload is unchanged and
delegates with `default`.
**Shared registration helper.** `CustomNodeManager` and
`AsyncCustomNodeManager` each carried an identical private
`CompleteCreateLifecycleForRegistration`. Both are replaced by one
internal `NodeStateLifecycle.CompleteForRegistration`.
`AddPredefinedNodeAsync` now checks cancellation on entry, propagates it
into lifecycle completion, and checks again before
`IndexPredefinedNode`, so a cancelled registration cannot leave a node
indexed, half-created, or a behaviour replacement partially wired.
_(An earlier revision also added `RemoveExplicitlyDefinedChild`
overrides on `MethodState` and `BaseDataVariableState`. Those were
dropped in
|
||
|
romanett
|
39602bccb1 |
Add node creation to the fluent node manager builder (#4428)
# Description
`INodeManagerBuilder` gains a node **creation** surface alongside its
existing lookup surface, so a `Configure` partial can fill a namespace
without a NodeSet or a ModelDesign.
Twelve new members, implemented as ordinary public members on
`NodeManagerBuilder` (new partial file
`NodeManagerBuilder.Authoring.cs`):
| Member | Creates |
| --- | --- |
| `AddFolder(name, parentId)` | a `FolderState` (`Organizes`) |
| `AddObject(name, parentId, typeDefinitionId)` | a `BaseObjectState` |
| `AddVariable<TValue>(name, parentId)` | a `BaseDataVariableState`
whose `DataType`/`ValueRank` come from `TValue` |
| `AddMethod(name, parentId)` | an executable `MethodState` |
| `Add<TState>(node, parentId)` | an already-constructed state of any
`NodeState` subclass |
| `Add<TState>(factory, parentId)` | a state built by a factory that
receives the resolved parent |
| `AddRoot<TState>(node)` | a root, with its existing references left
alone |
| `TryGetNode(nodeId, out node)` | lookup across
created-but-not-yet-registered nodes and predefined nodes |
Each `Add*` for a browse name takes a `string` (qualified with the
manager's default namespace) or a `QualifiedName` carrying an explicit
nonzero namespace index — eight overloads plus the four above.
```csharp
partial void Configure(INodeManagerBuilder builder)
{
INodeBuilder<FolderState> machines = builder.AddFolder("Machines");
builder.AddVariable<double>("Pressure", machines.Node.NodeId)
.OnRead(() => m_sensor.Pressure);
builder.AddMethod("Reset", machines.Node.NodeId)
.OnCall(ResetAsync);
}
```
Three properties make this usable straight from `Configure`:
- **NodeIds are final before the builder comes back.** Every `Add*` runs
the node — and its whole subtree — through the manager's
`INodeIdFactory` before returning, so `OnRead`/`OnWrite` registrations,
which key off `NodeState.NodeId`, stay valid once the node is
registered.
- **Creation is staged, not immediate.** Created nodes are held until
the manager calls `RegisterAuthoredNodesAsync`. That is what lets a node
name a sibling created moments earlier, and it keeps the builder usable
before the manager has finished building its address space.
- **Custom state types stay typed.** `Add<TState>` returns
`INodeBuilder<TState>`, so a hand-written `NodeState` subclass keeps its
type through the fluent chain.
Staging also makes the builder's own lookups see created nodes: browse
paths, `NodeId`, `TypeDefinitionId` and `DataType` resolution all
consult the staged graph before falling back to the manager's predefined
nodes.
### Registration ordering
A new `protected
FluentNodeManagerBase.RegisterAuthoredNodesAsync(builder, ct)` hands the
staged roots to `AddPredefinedNodeAsync`. It runs **after** the
`Configure` delegates and **before** `CompleteConfigureAsync`, so the
reverse-reference pass sees the new nodes and mirrors their references
to externally owned nodes (typically the ns=0 `Objects` folder) into
`externalReferences`. Three call sites emit it in that position:
- the source-generated `CreateAddressSpaceAsync`
(`NodeManagerTemplates`)
- the hosting `FluentNodeManager` (`FluentNodeManagerFactory`)
- `RuntimeNodeSetNodeManager`, which additionally re-runs
`AddReverseReferencesAsync` because its first pass happens before
`Configure`
A builder that created nothing registers nothing, so the call is safe to
make unconditionally.
The surface is available to **every** fluent host — there is no opt-in
gate and no second builder interface. The generated typed builder
(`FluentBuilderGenerator`) forwards the new members like the rest of
`INodeManagerBuilder`.
### Supporting change
`AsyncCustomNodeManager.PrepareAuthoredNodeIdsForRegistration`
(internal) assigns an id to every node in a subtree that still lacks one
and pulls namespace-0 children into the root's namespace. It differs
from the existing `PrepareInstanceNodeIdsForRegistration`, which only
rebases a subtree that collides with a declaration. It is idempotent.
## Related Issues
- Follow-up tracked in #4429 (adopting this surface in the remaining
hand-rolled node managers, which each need a base-class change first).
- No tracking issue for the API itself yet; happy to open one if
maintainers would like the design recorded as an ADR before review.
## Notes for reviewers
Three judgement calls worth a look:
1. **Always-on rather than gated.** The feature is available to every
fluent host with no enable call. There is no `INodeSource`-style concept
here to withhold it from, so an always-on capability is the same
behaviour with one fewer state flag. Misuse is still rejected: `Add*`
after `Seal()` or after registration throws `BadInvalidState`, a NodeId
in a namespace the manager does not own throws `BadNodeIdInvalid`, and a
parent in one of the manager's *own* namespaces that was never created
throws `BadNodeIdUnknown`.
2. **Detaching from the external-parent proxy goes through
`AddChild`/`RemoveChild`.** The factory overload hands the factory an
identity-only proxy when the parent belongs to another node manager,
then has to drop the parent link the factory established.
`BaseInstanceState.Parent` has an `internal` setter scoped to
`Opc.Ua.Types`, so this pair is the only public route. Commented as such
at the call site. An alternative would be widening that setter or adding
`InternalsVisibleTo` — happy to switch if preferred.
3. **`PrepareNodeIds` does not re-walk per descendant.** The root pass
already covers the subtree, so per-child calls would be no-ops on a
quadratic walk; descendants are only namespace-validated.
One inherited behaviour the new tests pinned down:
`BaseObjectState(parent)` sets `ReferenceTypeId = HasComponent` in its
own constructor, so a node built via the factory overload lands under
the `Objects` folder with `HasComponent`, while the plain `AddObject`
path picks `Organizes`. That asymmetry predates this PR; flagging it in
case it is worth normalizing separately.
`Import(UANodeSet)` is deliberately **not** part of this change — it
belongs to NodeSet import rather than node creation.
## Test results
19 new tests in
`tests/Opc.Ua.Server.Tests/Fluent/NodeManagerBuilderAuthoringTests.cs`
cover NodeId assignment before return, parent-by-NodeId nesting, the
default Objects-folder placement and its inverse reference, custom state
types, both factory paths (authored parent and external-parent proxy),
`AddRoot` reference preservation, `TryGetNode`, id-keyed read handlers
firing on the registered instance, browse-path/NodeId visibility of
created nodes, and every rejection path listed above.
Run against **net10.0** (`-p:CustomTestTarget=net10.0`):
| Suite | Result |
| --- | --- |
| `Opc.Ua.Server.Tests` | 4926 passed, 0 failed, 5 skipped |
| `Opc.Ua.SourceGeneration.Core.Tests` | 3809 passed, 0 failed, 8
skipped |
| `Opc.Ua.SourceGeneration.Tests` | 165 passed, 0 failed |
`UA.slnx` builds with 0 errors and no new warnings.
## Checklist
- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation. (New `Creating nodes
from scratch — the Add* surface` section in `docs/NodeManagers.md`, plus
TOC entry.)
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed. — **net10.0
only so far** (results above); .NET Framework has not been run locally
yet.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings. — pending first CI run.
- [ ] I have addressed **all** PR feedback received.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Marc Schier
|
02ea934a31 |
Complete fluent node manager extensibility (#4418)
# Description Complete the fluent node-manager extensibility surface needed by dynamic and externally backed address spaces. This change: - adds virtual node families with cheap NodeId ownership checks, async materialization, per-operation caching, custom browsing, and family-level callbacks; - adds monitored-item pre-creation decisions, complete lifecycle hooks, async batch hooks, stack-managed custom monitored items, and subscription-gated polling; - exposes the startup `ApplicationConfiguration` to generated manager partials through `FluentNodeManagerBase`; - fixes typed builder proxy support, concrete object wrapper types, and fully-qualified generated model composition; - reports unresolved `[NodeManager]` namespace expressions as `MODELGEN035` instead of silently dropping them; - documents the new APIs and compatibility behavior. ## Related Issues - Fixes #4397 - Fixes #4398 - Fixes #4399 - Fixes #4403 - Fixes #4404 - Fixes #4405 ## Validation - `Opc.Ua.Server.Tests` on `net10.0`: 4,886 passed, 5 skipped. - `Opc.Ua.SourceGeneration.Core.Tests` on `net10.0`: 3,807 passed, 8 skipped. - `Opc.Ua.SourceGeneration.Tests` on `net10.0`: 164 passed. - Runtime and generator regression selections passed on `net48`. - Review-feedback validation: 376 fluent tests passed on `net10.0`; 22 focused tests passed on `net48`. - The CI timing regression was repeated five times on `net48` and once on `net10.0`. - All 266 applicable GitHub Actions and Azure Pipelines checks, including CodeQL and coverage, pass on `83c571423`. - AppVeyor reports an immediate zero-duration no-run failure because `appveyor.yml` whitelists only the `appveyor` branch; it did not execute this PR. - `UA.slnx` builds successfully for `net10.0`; affected project builds are warning-free. - The repository-wide `dotnet test UA.slnx` invocation is currently blocked by the solution's mixed VSTest/Microsoft.Testing.Platform configuration under the .NET 10 SDK, so the affected test projects were run individually. ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [x] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. --------- Copilot-Session: afaa67c9-fc97-4185-a785-edd54a937d95 |
||
|
Marc Schier
|
e7542b7f5f |
Fix generated structured defaults and dependency metadata (#4414)
# Description Fix two source-generator paths that lost variable values or metadata: - XML `ExtensionObject` defaults without a `TypeId` now resolve their body through the encodeable factory's registered XML qualified name, decode to the generated structure, and retain the XML encoding identifier. - The existing preview `ModelDependencyV1` payload now carries effective child access and user-access values, minimum sampling interval, historizing state, and serialized default-value XML across referenced assemblies. The dependency validator rehydrates this metadata before inherited instance factories are generated. Explicit non-null extension-object TypeIds remain authoritative, unknown XML bodies remain raw XML, and explicit design files remain authoritative over referenced payloads. ## Related Issues - Fixes #4401 - Fixes #4402 ## Validation - Focused decoder, payload, validator, and Roslyn producer/consumer regressions on `net10.0` and `net48` - `tools/SourceGeneration.slnx` Debug builds on `net10.0` and `net48` - `tools/SourceGeneration.slnx` Release build on `net10.0` - Scoped `UA.slnx` Debug build on `net10.0` ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. --------- Copilot-Session: 66e6d342-d4a0-4d53-822b-524982c62f15 |
||
|
Marc Schier
|
df44b460be |
Complete CreateInstanceOf node lifecycle during registration (#4360)
# Description Generated `CreateInstanceOf<Type>` factories build typed node graphs and assign per-instance NodeIds, but intentionally leave the node create lifecycle open. Hand-written callers commonly registered those graphs without calling `CreateAsPredefinedNode`, so `OnBeforeCreate`, `OnAfterCreate`, and change-mask clearing never ran. This change: - adds `NodeState.IsCreated` and makes lifecycle completion idempotent per node; - completes newly materialized children, including children created by `OnAfterCreate`; - bounds late-child lifecycle convergence and fails explicitly if callbacks keep adding uncreated nodes; - completes uncreated nodes automatically before predefined-node registration and behavior wiring in both async and legacy custom node managers; - assigns/rebases async-node-manager NodeIds before lifecycle callbacks; - emits a null-safe Debug diagnostic when registration performs completion; - documents the generated-factory lifecycle contract and migration behavior; - adds regressions for condition/alarm method binding, finite-state-machine namespaces, Fluent state-node rebasing, behavior replacement, cloning, deletion, late children, non-converging callbacks, and synchronous/async registration. The tracked `src/` and `samples/` call-site audit covered 2,953 C# files. No additional ordering-sensitive call sites require explicit completion beyond the three existing DI/Robotics sites. ## Validation - `dotnet build UA.slnx -c Release -p:CustomTestTarget=net10.0`: passed. - `Opc.Ua.Server.Tests` net10.0: 4,811 passed, 5 skipped. - `NodeStateTests` net10.0 and net48: 213 passed on each. - Node-manager lifecycle and null-logger tests net48: 459 passed. - Source-generation core tests net10.0 and net48: 3,802 passed, 8 skipped on each. - DI, Robotics, ISA95, Positioning, Vision, Vision.Intent, and XRegistry suites passed on net10.0 and net48. - Generated alarm lifecycle regression passed on net10.0 and net48. The solution build reports six pre-existing CA warnings in untouched files. The OpenUSD suite has one pre-existing locale-sensitive failure in `StructuredCartesianCoordinatesAreAccepted` (`1.5` expected versus `1,5` under `en-DE`); all other OpenUSD tests passed on both tested frameworks. ## Related Issues - Fixes #4320 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ffdc400-0710-4a2e-a9d1-c0e1a9b3b81e |
||
|
romanett
|
24ba4855e6 |
Fix [DataType] generator Clone() dropping every inherited field of a derived structure (#4354)
# Description
The `[DataType]` source generator emitted a `Clone()` that created a
fresh instance and copied only the fields declared on the annotated type
itself, so every field inherited from an `IEncodeable` base was silently
dropped. Since the server clones node values while loading an address
space, a derived structure served by a node manager came back out of a
Read with its inherited fields empty, even though encode/decode was
correct.
This PR makes the `[DataType]` path emit the same
`MemberwiseClone`-based shape as the model-based generator:
```csharp
/// <inheritdoc/>
public override object Clone()
{
return (DerivedStruct)this.MemberwiseClone();
}
/// <inheritdoc/>
public new object MemberwiseClone()
{
DerivedStruct clone = (DerivedStruct)base.MemberwiseClone();
// deep copy of the own reference-typed fields
return clone;
}
```
`Clone()` delegates to a generated `public new object MemberwiseClone()`
that starts from `base.MemberwiseClone()` — a shallow copy of the whole
object, inherited fields included — and then deep copies the type's own
reference-typed fields (encodeables, `DataValue`, `Variant`,
`ExtensionObject`). Derived classes hide `MemberwiseClone` and chain
into the base implementation, so deep-copy semantics compose across the
hierarchy, including across assemblies into model-generated base types
(the `BicycleType` scenario that surfaced this). The two generator paths
now agree.
Because the clone now starts from a full shallow copy
(`base.MemberwiseClone()` for classes, `with { }` for records), the
per-field clone emitter only writes the deep-copy assignments; the
redundant per-field plain copies are gone. This also fixes a latent side
bug in the class path: init-only simple fields were silently dropped by
the fresh-instance clone as well.
Records were already correct (`with { }` carries every field) and are
unchanged in shape.
Tests:
- `GenerateClassCloneUsesMemberwiseClone` /
`GenerateDerivedClassCloneCarriesInheritedState` assert the emitted
shape for root and derived classes.
- `DerivedClassCloneKeepsInheritedFields` runs the exact repro from the
issue through the Roslyn driver, emits the assembly, and executes
`Clone()` at runtime to prove the inherited field survives (verified on
net10.0 and net48).
Verified locally: `Opc.Ua.SourceGeneration.Core.Tests` (3798 passed),
`Opc.Ua.SourceGeneration.Tests` (140 passed),
`Opc.Ua.SourceGeneration.Stack.Tests` (94 passed), and
`Opc.Ua.Client.Tests` (2161 passed, exercises the regenerated in-tree
`[DataType]` record types). The in-tree `[DataType]` consumers
(Opc.Ua.Client, both GDS common projects) regenerate and build without
warnings.
## Related Issues
- Fixes #4352
## Checklist
_Put an `x` in the boxes that apply. You can complete these step by step
after opening the PR._
- [x] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
romanett
|
e61ad4b306 |
Fully support sub-state machines in the fluent state-machine builder (#4322)
# Description Part 16 §B.3 places `HasSubStateMachine` on the parent **state** node, but the fluent builder attached it to the state-machine **root** because `FluentFiniteStateMachineState` never materialized state nodes at all. Investigating #3821 showed the gap was wider than the reference placement: 1. **No state nodes existed.** The NodeId written into `CurrentState/Id` — `new NodeId(stateId, ElementNamespaceIndex)` — was **dangling**: no node with that id was in the address space. 2. **`AvailableStates` was never populated**, so `GetAvailableStatesAsync` returned empty against a fluent-built server. 3. **The whole client discovery chain was therefore dead.** `ObserveEffectiveStateAsync` enumerates `AvailableStates` and browses `HasSubStateMachine` from each state node — it found nothing. The client side was already spec-correct; only the server deviated. 4. **`HasSubStateMachine` is non-hierarchical** (a subtype of `NonHierarchicalReferences`, `i=32`). Using it as the sub-SM's `BaseInstanceState.ReferenceTypeId` left the sub-SM unreachable from a hierarchical browse of the machine. ## What changed **Element nodes are materialized.** Definition-mode machines now create one `StateType` node per declared state and one `TransitionType` node per declared transition — each a `HasComponent` child carrying `StateNumber` / `TransitionNumber`, with the §B.4 `FromState` / `ToState` / `HasEffect` references and `HasCause` added by `WithCause`. `AvailableStates`, `AvailableTransitions` and `LastTransition` are populated, so `CurrentState/Id` and `LastTransition/Id` resolve to real, browsable nodes. NodeIds are per-instance, so several machines built from one definition coexist. **`HasSubStateMachine` moved to the state node**, with the inverse `SubStateMachineOf`, and the sub-SM is attached as a `HasComponent` child (fixing 4). **Core gains element-id mapping hooks.** `GetCurrentStateId` / `UpdateStateVariable` / `UpdateTransitionVariable` hard-coded the numeric form and were non-virtual, so a subclass could not redirect them. They now route through `public virtual` `GetStateNodeId` / `GetStateId` / `GetTransitionNodeId` / `GetTransitionId`, whose defaults reproduce the previous `(elementId, ElementNamespaceIndex)` convention **exactly** — every stack-shipped and generator-emitted machine is unaffected, and there is a regression test asserting that. **Generated machines report their model namespace.** The generator now emits an `ElementNamespaceUri` override on `FiniteStateMachineType` subtypes, taken from the nearest type that actually *declares* state children — a subtype that only adds behaviour inherits its elements, and their namespace, from its base. **Ordering and validation fixes.** `WithSubStateMachine` and `WithInitialState` are now order-independent (previously, attaching a sub-SM before setting the initial state left the child suspended until the parent left and re-entered the state — silently). `WithSubStateMachine` validates `parentStateId`, and the builder rejects duplicate element browse names, since element NodeIds are now derived from them. ## A defect this surfaced Code that assembles nodes with the generated `CreateInstanceOf<Type>` factories never calls `NodeState.CreateAsPredefinedNode`, so `OnBeforeCreate` / `OnAfterCreate` are skipped for the entire subtree — and `OnAfterCreate` is the only place a state machine resolves its element namespace. It is also where `ConditionState` binds its `Enable` / `Disable` / `AddComment` handlers and `AlarmConditionState` binds the shelving methods, so the impact is broader than state machines. This PR fixes only the two call sites it depends on — DI's `SoftwareUpdateFacetWiring` and Robotics' `RoboticsOperationBuilder`. **The repository-wide audit is deliberately left out of scope and tracked in #4320** (24 files call these factories; none completed the lifecycle). The Robotics regression had no test coverage at all, so an assertion on `CurrentState/Id` was added and verified to fail without the fix. ## Breaking changes - Sub-state machines are **no longer browsable from the state-machine root**. Callers passing a machine's `ObjectId` to `GetSubStateMachineAsync` must pass a state NodeId instead — from `GetAvailableStatesAsync` or a snapshot's `CurrentStateId`. - `CurrentState/Id` for fluent machines changes from the numeric form to the materialized node's id. Code reading a numeric id out of it should call `FiniteStateMachineState.GetStateId` rather than parsing the identifier. - Generated machines qualify `CurrentState/Id` / `LastTransition/Id` with their companion-spec namespace instead of the OPC UA one. Documented in `docs/WhatsNewIn2.0.md` and `docs/StateMachines.md`. ## Notes for reviewers - Adding `virtual` to the `FiniteStateMachineState` members is a binary (not source) breaking change — assumed acceptable on the 2.0 line, but worth confirming against the compatibility policy. - Address-space cost is one node plus one property per declared state and transition, per machine instance; called out in the docs for servers instantiating many machines. - `AvailableTransitions` is modelled as a `HasComponent` `BaseDataVariable` (matching the standard NodeSet and what the client's browse path expects), not a Property. ## Related Issues - Fixes #3821 - Surfaced #4320 (node create lifecycle audit — not addressed here) ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Test runs (net10.0) | Suite | Result | |---|---| | `Opc.Ua.Server.Tests` (full) | 4734 passed, 5 skipped | | `Opc.Ua.SourceGeneration.Core.Tests` | 3780 passed | | `Opc.Ua.SourceGeneration.Tests` | 132 passed | | `Opc.Ua.SourceGeneration.Stack.Tests` | 94 passed | | `Opc.Ua.Types.Tests` | 8763 passed | | `Opc.Ua.Robotics.Tests` | 540 passed | | `Opc.Ua.Robotics.Intent.Tests` | 22 passed | | `Opc.Ua.Di.Tests` | 424 passed | | `Opc.Ua.ISA95.Tests` | 136 passed | | `Opc.Ua.Aas.Tests` | 670 passed | | `Opc.Ua.Positioning.Tests` | 69 passed | | `Opc.Ua.Client.Tests` (StateMachines) | 36 passed | Only net10.0 was run locally; the .NET Framework leg is left to CI, hence the unchecked box above. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> |
||
|
romanett
|
41db5aca60 |
Fluent: publish Configure-created nodes' references on foreign-manager nodes (Objects folder placement) (#4331)
# Description A node created inside the fluent `Configure` hook (source-generated `[NodeManager]` partial, or the hosting `AddNodeManager(uri, build)` route) had no way to appear below the Objects folder — or below any node owned by another node manager. `AsyncCustomNodeManager.AddReverseReferencesAsync` mirrors inverse references into the `externalReferences` dictionary **before** the `Configure` partials run, and the fluent surface never sees the dictionary, so a configure-created node with an inverse `Organizes` reference to `ObjectsFolder` existed and simulated but never showed up under Objects. This implements the three coordinated pieces proposed in #4329, plus the companion namespace gap: **1. Idempotent mirroring pass** (independently worthwhile hardening) - `AddExternalReference` in `AsyncCustomNodeManager` and `CustomNodeManager2` now has if-missing semantics (linear scan of the per-source list). - `TypeTable.AddEncoding` no longer appends a duplicate encoding id; re-registering the same encoding is a succeeding no-op. (`AddRootNotifierAsync` and the node-side `AddReferenceIfMissing` branch were already idempotent.) **2. Re-run the pass after `Configure`** - New protected `FluentNodeManagerBase.CompleteConfigureAsync(externalReferences, ct)` wraps `AddReverseReferencesAsync` so hand-written managers using `CreateFluentBuilder` benefit too. - The generator-emitted `CreateAddressSpaceAsync` and the hosting `FluentNodeManager` invoke it once between the `Configure` callbacks and `Seal()`. Timing is safe: the master distributes `externalReferences` only after every manager's `CreateAddressSpaceAsync` returns. Side benefit: inverse `HasNotifier` references on configure-created event sources get root-notifier registration for free. - The hosting `FluentNodeManager`'s hand-rolled `externalReferences` workaround for its root folder is deleted — the root's inverse `Organizes` reference now flows through the shared pass (mirrored exactly once, covered by a test). **3. Discoverable fluent sugar** - `OrganizedBy(parentId)` / `UnderObjectsFolder()` on `INodeBuilder` write the inverse `Organizes` reference; with piece 2, placement is just "write the inverse reference". - A parentless `CreateInstance<TState>` on `INodeManagerBuilder` takes a constructor-style factory (`p => new BoilerState(p)`, keeping the surface reflection-free and AOT-safe), materializes the subtree from the type model via `NodeState.Create`, rebases all NodeIds through the manager's `INodeIdFactory` (`AssignInstanceNodeId`/`AssignInstanceChildNodeIds` — the same pair the generated factories use, so declaration-id children cannot collide with the type model), and registers via `AddPredefinedNodeSynchronously`. The Boiler #2 shape becomes fully fluent: ```csharp builder.CreateInstance( new QualifiedName("Boiler #2", NamespaceIndexes[1]), p => new BoilerState(p)) .Configure(n => n.UnderObjectsFolder()); ``` **Companion gap, same theme:** `[NodeManager]` grows `AdditionalNamespaceUris`, flowing through attribute discovery → binding → generator so the generated constructor reports a second (instance) namespace at construction and the generated factory advertises it in `NamespacesUris` — today `SetNamespaces` after construction is not enough because `MasterNodeManager` builds its namespace routing from what the manager reported when it was built. **Scope boundary** (as discussed in the issue): startup-time configuration only; for nodes created after startup the correct primitive remains `IMasterNodeManager.AddReferencesAsync`. **Reviewer notes** - The generated-code change was validated against the real pipeline: a forced fresh compile of `Opc.Ua.Server.Tests` (whose CoverageNodeSet managers are `[NodeManager]`-generated) emits `await CompleteConfigureAsync(externalReferences, cancellationToken)` between `Configure` and `Seal()` and compiles against the real base class. - The distribution consumer (`AddReferencesAsync`) already used `AddReferenceIfMissing`, so the whole chain is defense-in-depth idempotent. - 15 new tests: double-run idempotence, second-pass pickup of late-registered nodes, the new reference sugar, root-instance creation incl. child-id rebasing and reference remapping, hosting end-to-end Objects-folder placement, emitted-sequence and `AdditionalNamespaceUris` generator tests, and a full Roslyn-pipeline attribute test. Docs updated in `docs/NodeManagers.md` (new "Creating nodes under other managers' nodes" section, attribute docs, fluent-surface sections). - Verified locally: all touched projects build with 0 warnings; full `Opc.Ua.Server.Tests` (4,725 passed / 0 failed / 5 skipped, net9.0), `TypeTableTests` (113), `Opc.Ua.SourceGeneration.Core.Tests` (41), `Opc.Ua.SourceGeneration.Tests` (132), and the Quickstarts.Servers sample all pass. ## Related Issues - Fixes #4329 - Motivating case: OPCFoundation/UA-.NETStandard-Samples#756 / OPCFoundation/UA-.NETStandard-Samples#795 (Workshop/Boiler migration) ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Marc Schier
|
397db603db |
Add OPC UA for Asset Administration Shell: the published OPC 30270 and v3 draft (#4244)
# Description Implements the **Asset Administration Shell** companion specifications on top of `marcschier/wot-type-binding`, in two halves that a Server chooses between: * **AAS V3** — the [draft](https://github.com/marcschier/opcua-drafts/tree/main/companion-specs/AAS) (`http://opcfoundation.org/UA/I4AAS/v3/`), all sixteen conformance units: the metamodel, the registry built on the abstract xRegistry base, generational materialization, federation, packages with digest-bound integrity, the Digital Product Passport vocabulary, and the Annex F bridge to WoT Thing Descriptions. * **AAS V2** — the published [OPC 30270](https://reference.opcfoundation.org/specs/OPC-30270/full) (`http://opcfoundation.org/UA/I4AAS/`), which maps the AAS V2.0.1 metamodel: object model, ingestion of XML, JSON and AASX, materialization, server and client. The two are different metamodel generations rather than two revisions of one model — V2 keeps `Asset` as a first-class Identifiable, `identification.idType`, Views and ordered collections, none of which survive into V3 — so they are **alternatives**. A Server hosts one or the other; this is documented rather than enforced at startup. Each is named for the generation it maps (`Opc.Ua.Aas.V2`, `Opc.Ua.Aas.V3`, `AddAasV2Server`, `AddAasV3Server`), and what genuinely serves both carries no marker and sits at the root of `Opc.Ua.Aas`: the deterministic NodeId encoding and idShortPath conventions, `AasOptional<T>`, the digest rule, the materialization diagnostics, the AASX container walk, and the value and operation providers, which are keyed on `NodeId` and `Variant` and so carry no version at all. ## What is in here **New libraries**: `Opc.Ua.Aas`, `Opc.Ua.Aas.Server`, `Opc.Ua.Aas.Client`, `Opc.Ua.Aas.WoT`, plus `samples/Aas` and `docs/Aas.md`. **Enabling changes to existing code**, each needed by the above: * The OPC UA `Decimal` DataType added to `Opc.Ua.Types`, with its Part 6 §5.1.10 wire encoding. `BinaryDecoder` gained one internal member, `TryReadRemainingBodyBytes`, because a trailing field with no self-describing length cannot otherwise be read; `Decimal.Decode` is its only caller. Part 6 Table 3 types the unscaled `Value` as an **OctetString whose byte count is `Length` − 2**, not a length-prefixed `ByteString`, so it carries no length of its own — `DecimalEncodingTests` pins the body byte-for-byte against that mistake, and against the `Length ≤ 2` values the same table calls invalid. * The xRegistry projection engine extracted into `Opc.Ua.XRegistry.Server` and `Opc.Ua.WotCon.Server` migrated onto it (−848/+387 lines), with the WoT Connectivity tests passing **unedited**. * Two source-generator fixes, both of which made a model impossible to build (see below). ## Defects found and fixed along the way Running the stack end to end, rather than trusting per-area test suites, surfaced defects that no unit test caught. The ones worth a maintainer's attention: * **The generator emitted non-terminating constructors.** A structure field with no declared default was initialized with `new T()`, so a self-referential DataType (`AASReferenceDataType.referredSemanticId`) recursed until the stack overflowed and `AddOpcUaAas` could not load the model at all. The test host *died* rather than failing. Only the field that closes a cycle is now left null. * **The generator omitted `new` on a redeclared placeholder.** `AASOrderedSubmodelElementCollectionType` redeclares the `SubmodelElement` slot of its base type, and the generated `Add` method hid the base one without saying so, so the V2 model would not compile. No existing model could have hit either defect — both are build failures, so anything that builds today is unaffected. * **The metamodel half had never worked outside its unit tests**: the emitted NodeSet declared no reference-type aliases and was rejected by the loader; the Mandatory `Invoke` Method was never emitted, because a NodeSet is imported as written rather than instantiated from its type; the runtime bound at the authored namespace index instead of the one the Server assigns; and `MethodDeclarationId` was addressed in the wrong namespace. Every materializer test inspected the in-memory object graph, which cannot see whether the graph is loadable — two tests now import it exactly as `RuntimeNodeSetNodeManager` does. * **The client could not see any Property member.** It browsed `HasComponent` with subtypes, but `HasProperty` is a sibling under `Aggregates`, so reading a value failed and invoking failed argument validation. * **Every repeated registry write forked a duplicate group and resource** instead of adding a version, because the identifier was re-derived against a sibling set that already contained the entity itself. This also made the persistence bounds unreachable. * `.gitignore`'s `**/packages/*` silently swallowed the whole package-integrity implementation and its tests, which would have vanished on a fresh clone. ## Specification findings Nine findings against the V3 draft are fixed in [marcschier/opcua-drafts#75](https://github.com/marcschier/opcua-drafts/pull/75). The substantive one: clause 6.1.5's absent-versus-empty rule was **unsatisfiable** for five container fields that materialize as parent components with no node of their own, which made `AAS-LosslessRoundTrip` unachievable. OPC 30270 contradicts itself twice. Both are recorded in `AasV2ConformanceUnits.cs` and the documentation rather than smoothed over: Table 83 spells the unit `I4AAS MultiLangaugeProperty` where Table 85 spells it correctly, and Table 85 lists an `I4AAS Security` unit that Table 83 never defines. Table 84, which would assign profile URIs, is empty, so `ServerProfileArray` gains no I4AAS entry. ## Known limitation Calling an `Operation` **with arguments** answers `BadTooManyArguments`. Method resolution works from both the instance NodeId and the `MethodDeclarationId`, and the argument definitions are published, but `NodeState.AddChild` does not route through `MethodState.FindChild`, so a Method imported from a NodeSet leaves its typed `InputArguments` null. Closing this means changing how the shared NodeSet import claims typed children, which affects every consumer of runtime NodeSets and is out of scope here. It is documented in `docs/Aas.md` §11, the conformance matrix marks `AAS-OperationInvoke` partial rather than claiming it, and `samples/Aas` exercises the path and reports the status rather than working around it. V2 is unaffected — its `Operation` is an OptionalPlaceholder with no declared arguments. ## Related Issues - No tracking issue yet. Opening as a **draft** for design discussion first, per the ADR guidance below; happy to open one and link it once the shape is agreed. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Verification | Check | Result | |---|---| | AAS tests | **670 passed, 0 failed** on net472, net48, net8.0, net9.0, net10.0 | | Coverage | Client 80.8 %, Server 81.2 %, `Opc.Ua.Aas` 82.0 %, WoT 82.4 % — all above the 80 % gate | | Libraries | `Opc.Ua.Aas`, `.Server`, `.Client`, `.WoT` build clean on every target framework, zero warnings | | Regression | `Opc.Ua.Types` 8677, `Opc.Ua.WotCon` 1094, `Opc.Ua.XRegistry` 198, `Opc.Ua.Server` 4050 — all green | | Sample | `samples/Aas` builds warning-free and runs end to end | | Merge validation | Solution builds clean on net10.0 and net48; WoT Types 504, WotCon 1111, AAS 670, XRegistry 198 tests passed | | CI | All 99 checks green on `ff9298e44`; only the CLA signature status is outstanding | ### Review pass A full review against both specifications, and for API consistency and correctness, produced twelve fixes. Each one has a test that fails without it. | Defect | Where | Why it mattered | |---|---|---| | V2 projection aborted on **every** `File` read from a document | `AasV2EnvironmentRuntime` | The runtime bound `File/Value` unconditionally, the materializer emitted it only with content, and the builder throws `BadNotFound` for an unknown NodeId — aborting the whole registration. No reader ever populates `AasFile.File`, so no AAS V2 document could be hosted at all. | | `aas-suppl` anchored on the wrong part | `AasxPackageReaderCore`, `AasxPackageWriter` | IDTA and the Package Explorer anchor it on the environment part; we used the origin part. Every real `.aasx` returned zero supplementary files, silently. Affects both halves. | | Federation failed closed on pooled connections | `AasFederation` | `ConnectCallback` fires only for new connections, so the second fetch to an origin — and all of net472/net48/netstandard2.1 — failed with a security-flavoured error. The peer is now validated *inside* the callback, which makes reuse safe by construction. | | Malformed `Decimal` escaped as `ArgumentOutOfRangeException` | `BinaryDecoder` | A declared body shorter than `Scale` produced a negative read length. Two of the three test cases had been passing for the wrong reason. | | V2 and V3 cross-wired their value providers | `OpcUaAasServerBuilderExtensions` | Both registered `IAasValueProvider`; whichever was added first served both AddressSpaces. V2 now has its own contract, so co-hosting is harmless rather than silently wrong. `EnvironmentFolder` was also bound and ignored on the V2 path. | | Export filter skipped three nesting kinds | `AasEnvironmentExporter` | Content under `Entity`, `AnnotatedRelationshipElement` and `Operation` was exported unevaluated *and* stamped with a Digest presenting it as complete (clause 6.5.10). List authorization paths also shifted after a denial. | | Concealment held at the Method but not in the projection | `AasRegistryModel` | `GetSubmodel` answered `BadNotFound` while an anonymous Browse revealed the node, its identifier, semanticId and digest (clause 6.5.7). | | File handles were unsynchronized and unowned | `AasElementFileManager` | The async node manager holds no lock, so concurrent Calls raced a bare `Dictionary`; sequential handles let any Session read, seek or commit another's. Writes were also unbounded. | | Projection host was unimplementable outside the stack | `AasEnvironmentProjection` | The handle demanded a type with an internal constructor — our own tests used `FormatterServices.GetUninitializedObject` to get round it. Now mirrors WotCon's interface-typed registration. | | Embedded `FileType` omitted 7 of 10 Mandatory members | `AasEnvironmentMaterializer` (V2) | OPC 10000-3 6.4.4 requires them all; without `Size` and `OpenCount` a Client cannot size a read or detect an abandoned handle. | | Round-trip oracle ignored most of the model | `AasRoundTripComparer` | `Blob`, `File`, `ReferenceElement` and `BasicEventElement` had no case arm, so their fields compared equal by falling off the switch; `valueType`, `semanticId`, `entityType` and `Key.type` were never read. The clause 6.4 losslessness claim rested on it. | Known, unchanged: calling an `Operation` with arguments answers `BadTooManyArguments`, because `NodeState.AddChild` does not route through `MethodState.FindChild`. Documented in `docs/Aas.md` §11. Not addressed in this pass, and worth a maintainer decision: OPC 30270 §9.2 says all NodeIds of Nodes not defined in the companion specification **shall not** use the standard namespaces, and Table 87 marks a vendor-specific instance namespace Mandatory. Both halves currently mint instances in the companion namespace. Separating them touches NodeId minting, the runtime and the client together, so it is better done deliberately than folded into a review pass. This is a large change and touches shared code (`Opc.Ua.Types`, the source generator, the xRegistry projection). It is deliberately a draft: the enabling changes are separable if maintainers would prefer them split into their own PRs. The latest `master` is merged into this branch in `0dd0ff0e1`. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8 Copilot-Session: c3d88950-4a65-4ff6-87d2-1f545a875c9b Copilot-Session: b8ab3763-b9bd-46ae-b0ef-045db26e5782 |
||
|
romanett
|
333de8305b |
Add kitchen-sink coverage NodeSet2 with three load pipelines (#4251) (#4272)
# Description Regression fence for the NodeSet2 import + code-generation surface. This adds a single, purpose-authored "kitchen-sink" NodeSet2 (`Opc.Ua.CoverageTest.NodeSet2.xml`) plus a small dependent secondary model that deliberately flip a broad set of the knobs defined by `UANodeSet.xsd` (every node class, every `Definition` shape, custom reference types, all five modelling rules, scalar/array/matrix values, a deep branching instance tree, and a wide set of reference relations). The same bytes are then loaded into **real** servers (`ServerFixture<T>` + `ReferenceServer`) through three independent pipelines and validated node-by-node, attribute-by-attribute, reference-by-reference: 1. **Source-generation, hand-written manager** - the XML is consumed at compile time via `<AdditionalFiles ModelSourceGeneratorPrefix="Opc.Ua.CoverageTest">`; the generated `AddOpcUaCoverageTest(context)` composes the predefined nodes inside a `CustomNodeManager2`, so every node materialises as its generated `NodeState` subclass. 2. **Runtime import** - the identical XML is imported at startup through `RuntimeNodeSetNodeManagerFactory` / `AddRuntimeNodeSet`. 3. **Source-generation, generated fluent manager** - a `[NodeManager]`-attributed partial makes the generator emit the whole `FluentNodeManagerBase` manager, factory and typed fluent-builder surface. A shared, data-driven `[TestCaseSource]` battery runs the identical assertions against all three servers, and a cross-pipeline equivalence fixture asserts the three address spaces agree. A dependent **second namespace** (`.../Secondary/`) exercises cross-namespace `HasTypeDefinition`, `HasSubtype` and `HasComponent` references in every pipeline. Authoring the model surfaced two latent source-generator bugs, both fixed here: - `NodeSetToModelDesign` crashed on a top-level `<ServerUris>` table (`StringTable.Append(null)` for the reserved local-server slot). Added a null guard plus a focused regression test (`NodesetServerUrisTests`). - `FluentBuilderGenerator` emitted colliding typed-builder accessors for any model with more than one structure encoding, because `DataTypeEncoding` objects hang off their `DataType` and surfaced as parent-less "root" instances sharing the reserved `DefaultBinary`/`DefaultXml`/`DefaultJson` symbolic names. They are encoding metadata, never fluent-wired, so they are now excluded from the typed builder. A few edges are intentionally not asserted for cross-pipeline equivalence because the pipelines legitimately differ (documented in `CoverageNodeSet/README.md`): type-dictionary / `DataTypeDescription` nodes (the generator regenerates schema rather than emitting them), a handful of ambiguous duplicate-hierarchical / View-`Organizes` edges, and runtime structure stand-ins (structure definitions are validated via the `DataTypeDefinition` round-trip instead). ## Related Issues - Fixes #4251 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <7413710+romanett@users.noreply.github.com> |
||
|
romanett
|
d547747e22 |
Generate ReferenceNodeManager address space from a NodeSet2 model (#4220)
# Description The CTT/compliance `ReferenceNodeManager` built its ~6,000 node address space entirely in imperative C#. This PR migrates it to a source-generated `[NodeManager]` partial driven by an exported NodeSet2 model (`ReferenceServer.NodeSet2.xml`), mirroring the `MinimalBoilerServer` sample and completing the goal of issue #4204. The address space is now materialized from the model by the source generator. The large orchestration body was kept but converted from create-based to **find-instead-of-create**: each `Create*` primitive now looks up the model-materialized node via `FindPredefinedNode<T>` and re-applies runtime values, DataAccess metadata, and write handlers, rather than constructing nodes from scratch. This keeps the deterministic value seeding and behavior wiring intact while the structure comes from the model. ## Notable details for reviewers - **Idempotent reference wiring.** The mesh references in the `References` folder are already present in the model, so the historic manual `AddReference` calls would throw on duplicates. Added a small `AddReferenceIfMissing` helper so those calls are safe. - **Non-string NodeIds.** The `NodeIds` folder assigns numeric / GUID / opaque NodeIds. The model exports those nodes with their final ids, so find-by-string-path no longer works; added `CreateVariableById(NodeId)` to locate them by their real id. - **Generator bug fix (worth a careful look).** `ModelDesignExtensions.GetValueRankAsCode` reconstructed `ValueRank` as `dimensions.Length + 1`, so a node with `ArrayDimensions="3,3"` was emitted as `ValueRank = 3`. This affected **any** NodeSet2 with a fixed multi-dimensional array (rank-3 matrices, `BadTypeMismatch` on whole-matrix writes, wrong decoded dimensions). Corrected to `dimensions.Length` and updated the unit tests that had codified the off-by-one. ## Validation - `ReferenceServerTest`: 44/44 on net10.0 and net48 - `Opc.Ua.SourceGeneration.Core.Tests`: pass on net8.0/net9.0/net10.0/net48 - Integration on net10.0: ComplexTypes, Client, History, Sessions, Subscriptions all pass - Clean build, no new analyzer warnings ## Related Issues - Fixes #4204 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [ ] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
romanett
|
34fe1a1567 |
Emit WriteMask from the model in the source generator (#4253)
## Summary Isolates a self-contained OPC UA source-generator bug fix, extracted from PR #4220 (which targets a different base branch) so it can merge independently against `master`. ## Fix: Emit node WriteMask from the model **Bug:** The `NodeState` source generator hardcoded `global::Opc.Ua.AttributeWriteMask.None` for both the `WriteMask` and `UserWriteMask` attribute replacements. As a result, attributes declared writable in a NodeSet2 model (for example `WriteMask=96`) were emitted as `None`, so writes to those attributes failed with `BadNotWritable`. **Change:** - `tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs` - the common node replacements now emit `root.GetWriteMaskAsCode()` for the `WriteMask` and `UserWriteMask` tokens instead of the hardcoded `AttributeWriteMask.None`. - `tools/Opc.Ua.SourceGeneration.Core/Schema/ModelDesignExtensions.cs` - adds a `GetWriteMaskAsCode(this NodeDesign node)` helper that returns `global::Opc.Ua.AttributeWriteMask.None` when `WriteAccess == 0`, otherwise `(global::Opc.Ua.AttributeWriteMask){writeMask}`. ## Tests - `tests/Opc.Ua.SourceGeneration.Core.Tests/Schema/ModelDesignExtensionsTests.cs` - adds `[TestCase]`-driven coverage for `GetWriteMaskAsCode` across representative write-mask values plus a null-node test. ## Validation - `dotnet build tools\Opc.Ua.SourceGeneration.Core\Opc.Ua.SourceGeneration.Core.csproj -c Release` - 0 warnings, 0 errors. - `dotnet test tests\Opc.Ua.SourceGeneration.Core.Tests\Opc.Ua.SourceGeneration.Core.Tests.csproj -f net8.0 -c Release` - 3775 passed, 0 failed, 8 skipped. ## Note on the Views fix (excluded) A second generator fix (preserve forward same-namespace `Organizes` references to `ViewDesign` targets in `NodeSetToModelDesign`) was intended to ship here as well, but it was excluded. That fix compensates for behavior introduced by #4220 where imported Views are promoted to top-level `ViewDesign` nodes and their `ParentNodeId` is cleared. On `master` that promotion does not happen (Views remain children of their folder), so the fix and its test do not apply and cannot be cleanly isolated. It will land with #4220. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
romanett
|
cc4791e2fd |
Import NodeSet2 Views as standalone top-level nodes in the source generator (#4259)
## Problem The NodeState source generator mishandled `UAView` nodes imported from a NodeSet2 model. When an exporter set a View's `ParentNodeId` to an organizing folder, the importer/generator absorbed the View into that folder's `Children` and tried to emit an invalid `AddChild(...)` call. Because `ViewState` derives from `NodeState` (not `BaseInstanceState`), a View can never be an `AddChild` component, so the generated code was invalid. ## Fix Make Views standalone top-level predefined nodes linked purely via `Organizes` references — mirroring how `DataType`/`ReferenceType` type nodes and `DataTypeEncoding` are handled: - **`tools/Opc.Ua.SourceGeneration.Core/Schema/NodeSetToModelDesign.cs`** — clear the `UAView` `ParentNodeId` so the View stays a standalone top-level node, keep the organizing folder's forward `Organizes` references to those Views, and carry the View's `ContainsNoLoops` attribute through to the `ViewDesign`. - **`tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs`** — return `null` for `ViewDesign` in the `AddChild` and `ReplaceChild` template paths so Views are never emitted as components. - **`tests/Opc.Ua.SourceGeneration.Core.Tests/Schema/NodeSetToModelDesignTests.cs`** — new `ViewImport` NodeSet constant and `ImportPreservesFolderOrganizesReferencesToViews` test verifying Views are imported as top-level nodes while the organizing folder retains its forward `Organizes` references to them. ## Validation - `dotnet build tools\Opc.Ua.SourceGeneration.Core` (Release): 0 warnings / 0 errors. - `dotnet test tests\Opc.Ua.SourceGeneration.Core.Tests` (net8.0, filtered to `NodeSetToModelDesign`): all 17 tests pass, including the new `ImportPreservesFolderOrganizesReferencesToViews`. Extracted from the larger PR #4220 work — this PR contains only the source-generator View fix (no WriteMask, UserAccessLevel, NodeSet2.xml, or ReferenceNodeManager changes). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
Marc Schier
|
e05a820c91 |
Removal of the exposed diagnostics and node locks (#4188)
## What this is Design analysis for `Opc.Ua.Server`, the two rounds of design exploration behind it, and the first two implementation chunks it recommends. Draft: the analyses are ready to read, but the *conclusions* deserve discussion before the larger phases proceed. Later chunks stack onto this PR as they land. | File | Kind | Lines | |---|---|---:| | `plans/NodeManagerAnalysis.md` | Evidence — the node-manager plugin surface | 488 | | `plans/ServerRuntimeAnalysis.md` | Evidence — the server runtime cluster | 576 | | `plans/ServerDesignExploration.md` | Design round 2 — delivery strategy | 1,394 | | `plans/ServerDesignExplorationAlt.md` | Design round 1 — four interface designs | 2,119 | | `src/Opc.Ua.Types/State/NodeState.cs` | **Code** — node self-guards attributes | | | `src/Opc.Ua.Types/State/BaseVariableState.cs` | **Code** — value accessors take the lock | | | `src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs` | **Code** — 6 external locks removed | | | `tests/…/NodeStateAttributeConcurrencyTests.cs` | **Code** — concurrency safety net | 449 | `docs/` is untouched — it holds shipped feature documentation, and none of this is shipped API. ## The analyses Counted mechanically over `src`, `samples` and `tests`; **every figure is reproducible** from commands printed in each document. **Node manager surface.** `AsyncCustomNodeManager` declares **93 `virtual`/`abstract` members** (80 distinct names). Across the repository: **112 override sites, 25 distinct names ever overridden, 55 of 80 never**. Six carry nearly all real use. For scale, `ReferenceNodeManager` is 6,373 lines and overrides **8**. **Server runtime.** Seven findings, **two of which recommend no change**. Findings 1–5 are filed as #4183–#4187. The headline is an `object`-typed sync root published on three public interfaces and taken in **88 `lock` statements across 7 files**, including a shipped sample. **What is deliberately not flagged.** `IStandardServer` — 9 members over 5,575 lines, 16 of 37 virtuals used — is identified as the reference shape to preserve. `IApplicationInstance` and the already-`[Obsolete]` `Stop()` are examined and left alone. ## The design exploration **Round 1** explored four alternative interfaces in parallel under the current back-compat constraint. Its value is the convergence: four designers who never saw each other's work independently arrived at a 6–9 member `IServerContext` replacing the 57-member locator, removal of the `object` node-manager handle, elimination of the `Processed` protocol, and `IHistorianProvider` as the pattern to copy. **Round 2** re-runs it with back-compat lifted and asks how to deliver: full rewrite versus side-by-side fork. **Both option designers independently declined to recommend their own assignment.** Three structural facts were verified: 1. **The fork line already sits at `IAsyncNodeManager`** — old and new node managers can coexist with zero runtime changes, so a fork duplicates ~10% of the assembly, not all of it. 2. **`[OpcUaShim]` cannot shim an inheritance surface** — honest auto-fix ceiling for the authoring surface is **~10%**, against ~70% for the shipped analyzers. 3. **A second lock leak exists** — `NodeState` takes `lock (this)` under a `CA2002` suppression whose own comment documents the contract. Recommendation: **neither pure option**, but a staged path reaching the rewrite end state incrementally, gated on migrating the five framework-internal node managers that today make it impossible for the old base to ever freeze. ## Implementation: NodeState now guards its own attributes This removes the prose-only contract in which `NodeState` took `lock(this)` and every external caller was required to take `lock(source)` on the node instance. The code already carried a TODO describing exactly this fix and noting it required updating every external site. - The node self-guards rather than relying on callers. `BaseVariableState` gains a **fully private** `System.Threading.Lock` covering its value fields; `NodeState`'s other collections keep their existing private locks. - All **6** `lock(this)` sites and both `CA2002`/`RCS1059` suppressions are gone. - `Value`, `Timestamp` and `StatusCode` on `BaseVariableState` take the lock too — without that, a direct property write raced an attribute read once the external lock was gone. - **6 of 9** external sites removed. Five were already redundant (`GetReferences`, `GetChildren`, `ReferenceExists` and `SetAreEventsMonitored` guard themselves; `FindMethod` reaches its state via `GetChildren`); the sixth was an attribute read now covered by the node's own lock. - Attribute read/write deliberately take **no** node-wide lock: both dispatch to caller-supplied handlers (`OnReadValue` and siblings) that may take locks of their own, and holding a lock on the node across one gives the node no fixed place in the lock order. - **3 deliberately retained and annotated in code**: two `CreateBrowser` calls, which are not internally synchronized (tracked in #4216), and one in `CustomNodeManager`, whose class is slated for deletion so refactoring its lock would be wasted work. ### No lock is exposed anywhere, to anyone An earlier revision put the attribute lock on `NodeState` as `private protected`, which still published a lock-sharing contract on the inheritance surface. Review pushed back and it is gone: `BaseVariableState` was the only user, so the lock moved there as a plain `private readonly Lock`. There is no member for a derived type to call, in this assembly or any other. `BaseVariableValue` was the last public leak — it handed out the lock guarding its value through a `public Lock` property, so any caller could hold it across a call back into the stack. That property is removed: - Derived value classes (what the source generator emits for every structure variable) synchronize through a `protected EnterLock()` / `ExitLock()` pair. Callback helpers were the first choice, but the generated bodies pass `ref Variant` / `ref StatusCode` / `ref DateTimeUtc`, and `ref` arguments cannot be captured by a lambda. The lock *object* stays private either way. - `NodeStateTemplates` emits the new form. - `ServerInternalData`, the one cross-assembly caller, was reading the lock back off a value it had itself constructed with that lock. It now takes `m_diagnosticsLock` directly. `NodeBrowser.DataLock` remains `protected` and is tracked in #4215: a subclass extending `Next()` legitimately needs its enumeration state atomic with the base browser's, so the fix needs a base-owned acquisition rather than a deletion. ### Migration coverage for the removed lock members None of the removed lock members get an `[Obsolete]` shim, and the migration guide now says why: a shim would have to be `object`-typed to keep old call sites compiling, and on .NET 9+ `lock (o)` on a statically-`object` reference is `Monitor.Enter`, which does **not** interlock with `Lock.Enter()`. It would compile, run, and silently synchronize nothing — strictly worse than a compile error. Re-adding a member to a downstream-implemented interface is also a break in its own right on `net48` / `netstandard2.0`, where there are no default interface methods. Instead, every removed member has a named replacement on the migration path: | Rule | Covers | Replacement it names | |---|---|---| | `UA0024` | `IServerInternal` / `ISession` / `ISubscription` `DiagnosticsLock`, `DiagnosticsWriteLock` | `UpdateServerDiagnostics(...)` / `UpdateDiagnostics(...)`, `ReadServerDiagnostics` / `ReadDiagnostics` | | `UA0025` | `ILocalNode.DataLock`, `Node.DataLock` | a lock the caller owns; the node guards its own state | | `UA0026` | `BaseVariableValue.Lock` | a lock passed to the `BaseVariableValue` constructor | Each rule fires on sources still written against 1.5.378, where the compiler only says a member is missing. `docs/MigrationGuide.md` gains a section per removal with before/after. ### The tests earned their keep The four concurrency tests were landed **first**, against the unmodified code, because removing a synchronization contract cannot be verified by the compiler and the existing `NodeStateHandlerConcurrencyTests` covers a different concern (racing a handler assignment against a write, not reader/writer exclusion). They now run with **no external locking**, which is what demonstrates the node guards itself. They caught two real problems, and one of them was mine: - The `Value`/`StatusCode`/`Timestamp` setters bypassed the new lock — a genuine race. - One test asserted an invariant the design does not offer. Two property setters are two lock acquisitions, so that pair was **never** atomic; it only passed originally because the test held a lock across both. It now uses a single attribute write carrying a whole `DataValue`, which genuinely is atomic and is the path the server uses. A third issue was found and fixed before it shipped: the reader loops gated on `!writer.IsCompleted`, so a reader scheduled after the writer finished performed zero reads — failing two runs in three on `net48`. Converted to `do…while`; now five consecutive green runs. **Verified:** `Stack.State` 116 passed on `net10.0` *and* `net48`; the concurrency category green 5/5 consecutive on `net48`; `Opc.Ua.Types.Tests` 8,456 passed; the server node-manager suite 1,205 passed, including `NodeManagerLifecycleTests` and `AsyncCustomNodeManagerTests` which exercise the changed paths. ## Two corrections to my own published figures Both were found while starting implementation, and both are recorded on #4183. **`ForceDiagnosticsScan()` was overstated.** The analysis claimed the `DiagnosticsWriteLock` getter *"triggers a full address-space diagnostics rescan"*. It is in fact a single flag assignment (`DiagnosticsNodeManager.cs:700-703`) consumed by the read paths and the periodic timer. The defect stands — a side-effecting property getter with a racy write on a synchronisation path — but its cost was misstated. **The contract was 15 sites, not 19.** A naive search matched ten comment and `#pragma` lines that *mention* `lock(source)` while explaining it. The documents now carry a counting command that excludes comment matches. A third finding expanded scope the other way: the `object`-typed lock leak is **repo-wide**, with ten such members across `Opc.Ua.Core`, `Opc.Ua.Types` and `Opc.Ua.Server` — including `Node.DataLock => this` and `ApplicationConfiguration.PropertiesLock => m_properties`, the same defect class as `Subscription.DiagnosticsLock`. ## Caveats stated in the documents - **Nothing in the designs is implemented or benchmarked.** Every performance claim across all seven designs is a design intention, not a measurement. - The larger phases are a **major undertaking against a live plugin API**; the documents say so rather than letting the neatness of the proposals imply otherwise. - The recommended sequencing puts low-risk, self-contained steps first and notes they are **worth doing regardless** of which strategy is eventually chosen. - Round 2 notes the two option designers' cost estimates are **not independent of their assignments**. ## Review guidance The two analyses make factual claims checkable by running the included commands. The design explorations are exploratory and need not be agreed with to merge. The code change is small, test-backed, and independent of which delivery strategy is eventually chosen. Reasonable outcomes include merging as-is, merging only the analyses and the lock fix, or disagreeing with the recommendation while keeping the evidence. --- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041 Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63 |
||
|
Marc Schier
|
2b44fcad22 |
Robot intent: a task-level command API with a Part 10 lifecycle (#4165)
# Description Implement the draft **OPC UA — Robot Intent** companion model end to end: task-level motion verbs, a Part 10 lifecycle, a server hosting surface, a client, two samples and a live integration suite. It replaces the opt-in convention API merged in #4127 with an implementation generated from a companion information model. ## Why OPC 40010-1 *Robotics* describes robot **topology** in detail and defines **no motion verbs at all** — its entire actuation surface is `Start`, `Stop` and loading a named program. A conformant client can discover everything about a robot's construction and cannot ask it to move anywhere. #4127 filled that gap with ten verbs in an application-owned namespace, resolved by BrowseName, and the file said what it was: *"opt-in, explicitly non-normative industrial operation conventions"*. That contribution established a **vocabulary**. It established no **lifecycle**, and the lifecycle is the harder half. A motion takes seconds; a pick takes a minute. OPC 10000-4 §5.12.2 discards a method result when the Session ends *"independent of the task actually performed at the Server"* — so a synchronous motion method does not merely time out, it **loses the outcome of work that has already physically happened**. OPC 10000-10 §4.1 gives the OPC Foundation's own resolution: a Method performs a calculation, a **Program** runs a batch process or a machine tool part program. So `SubmitIntent` returns as soon as the intent is *admitted*, and what it returns is a NodeId — an `IntentOperationState` (a Part 10 program instance) the client subscribes to for progress and reads for the result. ## What changed **Model and shared code** — `Opc.Ua.RobotIntent.NodeSet2.xml` is source-generated in `Opc.Ua.Robotics`, so the enums, the polymorphic intent structures, `IntentOperationState : ProgramStateMachineState`, the method states and the typed clients derive from the model rather than being hand-written. Its only `RequiredModel` is the base UA namespace, so a server can adopt it without OPC 40010 or DI. `PoseMath` implements the specification's Annex C conversion between its unit quaternion and the core `ThreeDFrame` — including the `asin` clamp that stops a pole orientation becoming a domain error, and the non-negative-`w` representative that makes two servers agree on four numbers. `FrameTree` composes transforms along the frame tree. **Verbs are a DataType hierarchy, not one Method each.** A single submission and a mission step are then the same shape, and a new intent is a subtype rather than a new method — which is what `AddOperation<TRequest,TResponse>` was working around. **Server** — `IntentControllerHost` owns admission in the specification's order, the queue with PLCopen buffer modes, blending, cancellation *with the server's right to refuse*, missions with a committed base and a revisable horizon plus an IEC 61131-3 step graph, safety-aware refusals, real-time channel brokerage, and the capability declaration. All thirteen Methods are wired; command authority and channel leases are released when the holding Session closes; every client-supplied NodeId is validated against the controller being commanded (clause 11.3). `RobotIntentNodeManager`, the fluent `IIntentControllerBuilder` and `AddRobotIntent`/`ConfigureRobotIntent` make it reachable, and `RobotIntentFacetCalculator` computes which of the specification's facets an instance actually satisfies. **Client** — `RobotIntentClient` discovers `Server/RobotIntent/Controllers` and reads what a robot accepts; `IntentOperationHandle` subscribes and completes on a terminal state, recovering the result after a reconnect because clause 6.7 promises it survives; `CommandAuthorityLease` and `RealTimeChannelLease` manage the two exclusive resources; fluent builders cover every intent and mission. **Samples** — all robotics samples move under `samples/Robotics/`. `IntentEnabledRobot` is one UR5e-style offset-wrist arm on a bench, declared completely enough to show what a conformant server publishes, with a simulated safety source so the clause 10.4 refusals are demonstrable. `IntentViewerClient` turns a **click on a target prim in the OpenUSD viewport** into an intent and watches the arm execute it; it also runs headless, which is how CI exercises it. **Intents execute serially**, which satisfies every `BlockingMode` constraint by construction: the specification forbids beginning a `Single` or `Hard` intent while another executes and merely *permits* `None` and `Soft` to overlap. The convention API (`RoboticsOperationConventions`, `RoboticsOperationsBuilders`, `RoboticsOperationsClient`) is removed. ### On safety — what this deliberately does *not* claim The interface is **non-safety-rated**, and that is a property of the technology rather than a scoping choice. OPC 10000-15 carries cyclic safety data from a SafetyProvider to a SafetyConsumer; the consumer's `RequestSPDU` holds an identifier, a monitoring number and one octet of explicitly **non-safety** flags, so a caller has no channel through which to supply safety-rated arguments. Every safety fieldbus (PROFIsafe, CIP Safety, FSoE, openSAFETY) expresses a safety command as a **continuously asserted cyclic signal**, because the integrity argument rests on the fail-safe state that follows when assertion *stops* — and a Method call has no defined behaviour when it stops being called. What the host does instead is **observe and refuse**: `UpdateSafetyState` is how the application reports what the safety system is enforcing, and admission then refuses on the same values a client can read, so a refusal is explainable from the address space rather than from Server-internal state. It may never *instruct* the safety system, and no Method here commands a safe motion function, changes an operational mode or clears a stop. ## Defects this found in the specification, fixed upstream Implementing a specification is the only way to find out whether it can be implemented. Seven defects went back to [marcschier/opcua-drafts#47](https://github.com/marcschier/opcua-drafts/pull/47) rather than being worked around here: - **Refusal was unobservable.** Clause 6.2 orders six distinct refusals and clause 5.8 calls the failure set "small and diagnosable on purpose" — but `SubmitIntent` returned only `IntentId` and `Operation`, so a client could see *that* something was refused and never *which*. It now returns `Accepted`, `Failure` and `Message`, and clause 6.2 states that a refusal returns `Good` rather than substituting a Bad `StatusCode`. - **The Part 10 promotion was illegal.** `IntentOperationType.ProgramDiagnostic` was declared as a Property of `PropertyType`; OPC 10000-10 declares it a Variable of `ProgramDiagnostic2Type` reached by `HasComponent`. The declaration added a *second* member beside the inherited one instead of promoting it — which is precisely why an earlier version of this branch had deleted it. Correcting the TypeDefinition made it generate cleanly with **no change to the curated Core**. - **Clause 9 said nothing about the Method surface.** A server could declare `MissionsSupported` true and omit `SubmitMission` entirely, because the mission and channel Methods are Optional. This implementation did exactly that until an integration test caught it. A fourth honesty rule and a table now fix which Methods each declaration implies. - **Conformance was not machine-readable, and two implementations disagreed.** Clause 12 defined conformance in terms of facets and gave a Server nowhere to publish which ones it satisfied, so every client had to re-derive Table 12.2 from the address space. This repository proved why that fails: the server''s `RobotIntentFacetCalculator` and the client''s `RobotIntentRules.DeriveFacets` each implemented clause 12.2 independently and **disagreed on essentially every row** — the client''s `Blending` was `BlendingSupported` alone where the server also requires all four blending buffer modes, its `QueuedIntents` was `MaxQueueDepth > 0` where the server also requires `Buffered`. A client and server from the same release answered "does this controller conform?" differently, and both were reading the specification correctly, because several rows are behavioural and no amount of browsing settles them. `IntentCapabilitiesType` now carries **`SupportedFacets`**, exactly as OPC 10000-5 does with `ServerCapabilitiesType.ServerProfileArray`, RI-Base requires it, and clause 12.2 separates structural requirements a Server shall meet before listing a facet from attested ones governed by clause 9. - `WaitIntentDataType.Signal` was unbounded where clause 11.3 has to check it; §11.3 gained a table fixing the expected type of every NodeId-valued member; §5.7.0 gave `Ready`/`ActiveIntent`/`ActiveMission` normative meaning; §6.9 bounded `RequestedLease`; §6.5 and §6.4 now say what a server that cannot differentiate `StopMode` must do, and which stop a superseded intent gets. ## Bugs the tests and reviews caught Each of these would have been silent in the field: - **The execution engine was never wired into the public API.** `IntentControllerBuilder.RegisterAsync` built the address space and never started an `IntentControllerHost`, so through the documented `AddRobotIntent` → `ConfigureRobotIntent` path **every Method returned `BadNotImplemented`** and the registered `IIntentExecutor` was never resolved. A browsable address space is not a working robot interface, and only a live integration test showed the difference. - **NodeId scoping failed open on an empty index.** A controller advertising `CallProgramIntentDataType` with no published `ProgramType` accepted an *arbitrary* NodeId as `CallProgram.Program` — and `CallProgram` runs code the server holds. - **Blending completed the predecessor at submission time** and abandoned its motion, telling the client it succeeded and publishing an all-zero pose at the origin as a genuine `AchievedPose`. - **Mission base immutability compared only identifiers**, so a client could keep the ids and swap the *motion* of an already-committed step. - **`FinalResultData/Result` was never registered** with the node manager: Browse showed it, Read returned `BadNodeIdUnknown` — so clause 6.7's promise to a Part 10 client did not hold. - `DisposeAsync` disposed primitives out from under in-flight executor code; re-using a terminated `IntentId` destroyed the retained result and duplicated a NodeId; neither side registered the Robot Intent encodeables, so `Pose3DDataType` did not decode; host startup was hooked to a task that ran in only one hosting shape. - The result was published **after** the state went terminal; capabilities were resolved against a stale namespace table; an empty `ContentFilter` arrives as an empty element array rather than a null filter, so testing only for null made every unconditional mission transition silently untaken — a mission would run its first step and stop. ### Found by a later test-quality audit, after the above was written An audit of the test suite itself found five tests whose **names** asserted a normative requirement while their **code** asserted something weaker or nothing at all. The worst **fabricated the answer it checked**: a helper caught a decode failure and invented "5 supported intents, 6 axes, all facets true", so six capability-honesty assertions across four tests could pass while reading nothing from the server. The fallback is gone and the suite passes with honest reads. Chasing the facet gaps it reported led to the clause 12 defect above. Two further defects followed from completing that work: - **`RI-Interop-40010` was never computed**, and the code documented the omission as impossible to fix "because the calculator receives only a Robot Intent controller, not the linked OPC 40010 model". That was wrong: Annex B's structural requirement is an **inverse** `HasIntentController` reference, reachable from `IntentControllerState` — and the integration suite was already browsing exactly that. A server implementing Annex B could not claim Annex B. - **The published claim could go stale.** `SupportedFacets` was a snapshot taken during `RegisterAsync`, so a reference attached afterwards left it wrong. It is now bound through `BindRead` and recomputed on read, so the claim tracks the address space by construction rather than by discipline — which matters for anyone bolting Robot Intent onto an existing OPC 40010 node manager, where the attach ordering is often not theirs to choose. Two tests that **could not fail** were also replaced: one asserted `AccessLevel` equals `CurrentRead` to pin a read-only marking, but `BaseVariableState` initialises every variable to exactly that, so deleting the marking left it green; it is now a write expecting `BadNotWritable`, including a real OPC UA `Write` service call over a live session. Finally, a build break that a plain build hides: `RoboticsAotTests` used `out string?` in a project declaring `<Nullable>disable</Nullable>`. Building the project **without** `-f` suppresses `CS8632` through `NoWarn=nullable` and succeeds; building it **with** `-f net10.0` resolves `NoWarn` differently, `CS8632` fires, and `TreatWarningsAsErrors` makes it an error. CI builds per-TFM, so this would have failed there while passing every local build to that point. ## Testing and coverage | | | |---|---| | `dotnet build UA.slnx -t:Rebuild` | **0 errors, 2 warnings** — both pre-existing `CA1861` in `tests/Opc.Ua.Tools.Tests`, a file this PR does not touch | | **Whole `UA.slnx` test suite, net10.0** | **50 of 52 assemblies fully green** (measured at `40d9cf213`, before the `master` merge; the other two are explained below) | | `Opc.Ua.Robotics.Tests` | **467** net10.0, **447** net48 | | `Opc.Ua.Robotics.Intent.Tests` (live client <-> server) | **20** on net10.0 and net48, run repeatedly | | `Opc.Ua.OpenUsd.Tests` | **904** after merging `master` (664 before; the rest arrive with #4160 / #4162 / #4180) | | `Opc.Ua.Aot.Tests` | **121/121** | | `Opc.Ua.Di.Tests` (regression from the sample move) | **364/365**, one pre-existing skip | | **Changed-line coverage** | **90.7 %**, no file below 80 % — comfortably above the enforced band, which after #4158 applies only to patches larger than 100 changed lines | The two non-green assemblies are **not** caused by this branch. Rather than assert that, I ran the same selections at the merge-base `ae241b662` on the same machine: - `ServiceCallReassemblerTests.PrivateRequestSummaryFormatsKnownRequestKinds` expects `12.5` and gets `12,5` — a culture-sensitive assertion failing on a European locale. Fails 4/4 on **both** commits. - `CertRotationLiveTests.L2Cert2RotateCertificateDuringServerRestartRecoversSharedChannelAsync` fails on a teardown count. Fails 4/4 on **both** commits. - A third failure seen once, AOT `FindServersAsync` with `BadSecureChannelClosed`, did not reproduce: 4/4 on retest and 121/121 in the final run. Environmental. Two things are worth calling out because they would otherwise be invisible: - `tools/**` is **not** in the `coverage-thresholds.json` ignore list, and no test project referenced the OpenUSD tools assemblies — so their changed lines counted as uncovered and the real patch figure was 75 %, exactly the blocking threshold. Rather than widening the ignore list, the pure decisions in the viewport host (pointer-to-physical-pixel conversion, `RenderPickRequest` construction, stale-retry, backend discovery, pick-mode selection) were separated from the Avalonia/native shell and tested headless against a fake picking backend. - `dotnet build UA.slnx` **hides warnings on unchanged projects**; only `-t:Rebuild` surfaces them. That is how a `CA2263` introduced by a net48 portability fix survived several "zero warning" builds. The multi-target requirement earned its keep repeatedly: it caught a default interface implementation, `ArgumentNullException.ThrowIfNull`, `string.Create` with an interpolated handler, `ValueTask.FromResult`/`CompletedTask`, `Task.WaitAsync`, generic `Enum.GetValues` and generic `Enum.IsDefined`, and a non-generic `TaskCompletionSource` — none of which the net10.0 build complained about. A net10.0-**only** hang in the simulated executor and two load-sensitive flaky tests were also found and fixed at the cause. ### Merged with `master` `master` (through `e73e71184`) is merged in as `08205d81d`. Four conflicts, all where the two branches touched the same robot and OpenUSD surfaces, and none resolved by dropping a side: - #4162 added six files to `samples/MinimalRobotServer/` while this branch moved that directory to `samples/Robotics/MinimalRobotServer/`. They are kept at the moved path, and `UA.slnx` takes the union of both sides' new projects. - #4160 replaced `object` with a `UsdValue` union and rewrote `Flatten`; this branch had reformatted the same method and fixed a path in its doc comment. Git produced a hybrid that did not compile. The resolution keeps `master`'s `UsdValue` arm and `Flatten(UsdValue)` overload in this branch's block-bodied form, with the path fix retained. - `master` inserted `FindStageCamera` directly before two methods this branch had relocated, so the merge duplicated `WriteStageUsda` and `GetPrivateStateRoot`. Only `FindStageCamera` is genuinely new — `master`'s diff shows the other two as unmodified context — so the duplicates are dropped and this branch's `internal` versions kept, including the `GetPrivateStateRoot(string?)` overload the connector tests use to stay out of the real user profile. - `master` moved the OpenUSD stack to `0.4.0-alpha`; this branch pins `OpenUsd.Rendering` in the test project so the renderer-pick tests can reach the picking backend. Left at `0.1.0-alpha` that pin is below what the viewer now resolves transitively, which is `CS1705`. The pin follows `master`. CI was green on `baaf35e9` with **190 checks passing and none failing**, across every build leg (net472 / net48 / netstandard2.0 / netstandard2.1 / net8.0 / net9.0 / net10.0, Debug and Release) and the full test matrix on Windows and Linux. Since then `master` has been merged twice more and two CI-stability fixes landed (see below), so the current head is re-running. Locally, `dotnet build UA.slnx -t:Rebuild` gives **0 errors** and the 2 pre-existing `CA1861` warnings; Robotics **467** net10.0 / **447** net48, Robot Intent **20** on both, OpenUSD **904**, AOT **121/121**, source generation **3771**. ### Review round Five reviewers over disjoint areas found defects worth calling out, all fixed in `3ffec2c1`: - **A refused mission step was reported `Succeeded`.** The advance path returned the same `false` for "steps exhausted" and "step refused", so a protective stop between steps told a supervisory system the mission had completed while the robot stood halted with work unexecuted. - **Three attacker-controlled NodeIds reached the executor unvalidated** — `Pick.Tool`, `Place.Tool`, `ToolChange.DockStation`. `DockStation` drives a physical tool-exchange motion to a location the server never authorised. The replacement test enumerates NodeId members by reflection so the next one added cannot be missed. - **A lock-order inversion that loses a stop.** `CanCancel` and the mission condition evaluator ran under the host lock while the executor's own thread re-entered it via `ReportBlendBegin`. - **A non-terminal executor outcome killed the pump** from outside the `try/catch` that exists because the executor is untrusted — `Ready` stayed true and submissions kept being accepted while nothing moved. - **`Pause` published `Suspended` while the robot kept moving.** Now queue-only and honest about it; the specification gained the matching rule ([opcua-drafts `357a558`](https://github.com/marcschier/opcua-drafts/pull/47)). - **`RI-RealTimeChannel` and `RI-Safety` were false claims** — granted on an empty folder with no methods, and on the mere presence of a Mandatory node. - **The Part 10 promotions never promoted.** Namespace-1 BrowseNames declared second members beside the inherited ones; the conformance test could not see it because its lookup stripped the prefix. Fixed upstream and regenerated. - **Renderer picking never worked.** The reflective probe bound a renderer that is not a picking backend and throws off its owner thread. It now binds only `IRenderPickingBackend`, falls back immediately and loudly, and the docs say plainly that renderer picking is unavailable with this package version. - **The client lost command authority the instant it gained it** — monitored items report the current value first, and the lease treated every notification as a loss. ### CI stability fixes for master-side failures The CI split in #4158 turned on legs that had never run, and two of the failures they exposed were real bugs on `master` rather than anything this branch introduced. Both are fixed here because they were blocking every branch: - **ECC key generation was broken on macOS.** `ExportExplicitParameters` throws `PlatformNotSupportedException` there — Apple's SecurityTransforms supports named curves only — and it was called for exactly one value, the curve order `n`. The four curves OPC 10000-12 admits have published orders, so those are carried directly, with `CurveOrderTableMatchesThePlatform` checking each against the platform's own exported order wherever export works, so the constants cannot silently drift. This accounts for the two ECC failures in `test-macOS-latest-Server`. - **The certificate-leak detector raced asynchronous disposal.** It polled a fixed budget and reported whatever it saw, which cannot distinguish a genuine leak (count steady) from a suite still draining fire-and-forget disposals (count falling) — on a loaded agent that produced `Certificate leak detected: 2 instance(s)` with every test passing. It now waits for the count to **settle**, so a real leak is reported sooner than before while a slow drain gets as long as it keeps progressing. Shared by every suite that asserts no certificate leaks. The remaining macOS failures on `master` (`Gds`, `Tools`, `Redundancy.Samples`, `aot-macos`) have different root causes, are not reproducible without a macOS host, and are **not** addressed here. ### Merged with `master` again `master` through `416d619c4` (#4158, the CI matrix split and graduated coverage bands) is merged in as `08ae4528`. One conflict, in `docs/DeveloperGuide.md`, where both sides had deliberately rewritten the coverage-gate section: - `master`'s rewrite is kept in full — graduated `patch.bands`, Codecov being informational-only, and where the numbers surface per CI system. - This branch's paragraph about the `ignore` globs is kept too, re-sited ahead of those subsections. I re-checked it against the merged `coverage-thresholds.json` rather than assuming it still held: the list is still `tests/**`, `samples/**`, `obj`, `bin`, `*.g.cs`, with **`tools/**` absent** — so the warning that anything under `tools/` is measured like product code, and that an assembly no test project references contributes changed lines counted as *uncovered*, is still accurate and worth keeping. - `master`'s "reproduce a coverage failure locally" line supersedes this branch's near-identical one, so only `master`'s survives. Post-merge: `dotnet build UA.slnx -t:Rebuild` **0 errors**, 2 pre-existing `CA1861`; Robotics **467** net10.0 / **447** net48; Robot Intent **20** on both. ### Getting CI green The pipeline had never actually run on this branch until now (it was waiting on the fork-PR approval gate), so the first run surfaced four real problems, each fixed at source: - **Seven build legs failed** because `IntentEnabledRobot` pins `net10.0` for the OpenUSD payload but never opted into `RestrictForLegacyTfm`. On the legacy legs the libraries build as net472/net48, where `Opc.Ua.Types` exposes the `System.Threading.Lock` polyfill publicly, and that collides with the sample's own net10.0 BCL (`CS0433`). `Opc.Ua.OpenUsd.Connector.Viewer` was already using the opt-out for the same reason. - **A Linux-only test failure** turned out to be a real defect, not a platform quirk: `SetPose` published the *commanded* interpolated pose beside joint frames derived from the *solved* joints, so the two disagreed by the inverse-kinematics residual — about 8 mm, constant after any real motion. That is the lying-digital-twin defect one level deeper than the per-step IK fix reached. `SetPose` now derives both from the joint angles. - **Three source-generator tests** asserted the old emitted shape. The clause 6.2 Null-Variant fix was implemented in `ObjectTypeProxyGenerator` so every generated proxy benefits, which changed that shape; the tests are updated and strengthened to pin the `IsNull` branch as well. - **A CI-only timeout** in the simulated arm drain: the executor runs on a manual clock so the tests do not depend on machine speed, but the drain guarded itself with a wall-clock deadline, which put that dependence back. CI runs with coverage instrumentation, which is enough to take a per-step IK probe from ~0.6 s to over 5 s. The guard is now a simulated step budget. One failure was **not** from this PR and did not recur: a Roslyn `AccessViolationException` compiling `Opc.Ua.ISA95` on one leg, while the Debug leg of the same target passed on identical code and the Release build succeeds locally. ## Related Issues - Relates to #3827 — the request that prompted the robotics intent work. - Builds on #4127. The companion specification is drafted in the open at [marcschier/opcua-drafts#47](https://github.com/marcschier/opcua-drafts/pull/47) (`metaverse-specs/robot-intent/`), with the prior art and the reasoning behind each decision in the research document beside it. **Nothing in it is normative or endorsed by the OPC Foundation, and its NodeIds and namespace URI are provisional** — which is the main reason this is a draft. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. On the test checkbox, precisely: the whole `UA.slnx` suite now runs on net10.0 with **50 of 52 assemblies fully green**, and `Opc.Ua.Robotics.Tests` passes on **all five** target frameworks including net472, net8.0 and net9.0 which had never previously been exercised. The two remaining failures are reproduced identically at the merge-base and are detailed above, so I have ticked the box; if you would rather it stayed unticked until those two are green, say so and I will revert it. ## Points I would most like feedback on 1. **The namespace and NodeIds are provisional.** They come from a draft specification, not from the OPC Foundation. If this lands before the model is registered, the identifiers will move. 2. **The model lives in `Opc.Ua.Robotics`.** It is standalone on the base UA namespace and takes no dependency on OPC 40010, so a separate `Opc.Ua.RobotIntent` package would arguably be cleaner. Keeping it here was a deliberate call — this is where the robot-facing API already is — but it is the easiest thing to change now and the hardest later. 3. **Removing the convention API from #4127 is a breaking change** for anyone who adopted it. It was explicitly marked non-normative, so I have assumed that is acceptable — please say if a deprecation period is wanted instead. 4. **Serial execution** is a deliberate simplification. Concurrent `None`/`Soft` intents would be an optimisation, not a correction, but it is worth agreeing that reading. 5. **`samples/Robotics/` reorganisation** moves `MinimalRobotServer`. Happy to split that into its own commit if it makes review easier. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dab1302-19c5-4a9b-a50c-d97d389713aa Copilot-Session: a248a589-9a20-4372-868e-5d347e57001b |
||
|
Marc Schier
|
e73e71184d |
[WotCon] Add OPC UA WoT Connectivity 1.1 with the Types, source generation, Server and xRegistry work it builds on (#4128)
## Summary Adds OPC UA WoT Connectivity 1.1 to the stack, together with the Types, source generation, Server and xRegistry work it is built on. This branch is the merge point for the fourteen-PR stack that replaced integration PR #4093; every constituent PR was reviewed and approved separately and is listed below. 414 files changed, +108,936 / -3,199. Roughly 46k added lines of product code, 57k of tests, plus samples, tools and documentation. ## Types - Complete `ExtensionObject` raw-body decoding for binary, XML and JSON bodies by resolving the concrete type through the message context's encodeable factory, and simplify the `Variant` structure helpers by delegating to it (#4128). - Add `IAtomicFileReplace` as an optional `IFileSystem` capability so existing external implementations keep working, with atomic publish for `LocalFileSystem` via `File.Replace`/`File.Move` and for `VirtualFileSystem` by re-keying the in-memory entry (#4128). - Add lossless conversion between WoT documents and NodeSet2 (`Opc.Ua.Wot.WotNodeSetConverter`). A byte-exact `uav:nodeSet` envelope is preserved when requested, the structured `uav:nodes` projection is used when the readable vocabulary is incomplete, and NodeSet2 is otherwise synthesized from readable WoT terms. Unmapped JSON members survive a round trip as pointer-addressed residue (#4131). ## Source generation - Improve the NodeSet to ModelDesign conversion and the generated node state (#4132). - Generate OPC UA models directly from WoT Thing Description files (#4134). ## Server - Add live NodeManager shadow and immediate reload. A reload materializes a new generation beside the active one and switches atomically; the superseded generation is retired gracefully so existing MonitoredItems keep being served until they drain, or immediately when the caller asks for it. Includes the request admission and drain machinery that lets an orderly shutdown wait for admitted requests instead of tearing down underneath them (#4147). - Make subscription transfer between sessions transactional, so a failed transfer leaves neither session holding a partially moved subscription (#4150). ## xRegistry - Allow clients to use explicit registry roots rather than assuming a well-known location (#4130). ## WoT Connectivity - Add the WoT Connectivity 1.1 information model: a registry-first revision layered on the abstract xRegistry base model, incorporating the complete published OPC 10100-1 v1.02 surface as deprecated nodes in the same namespace so existing 1.02 clients keep working (#4139). - Add the protocol binding abstractions and planners that compile WoT forms into executable plans (#4140). - Add the HTTP, Modbus, OPC UA and MQTT binding executors (#4144). - Add the registry and its stores, including document versioning, validation, dependency resolution and refresh (#4146). - Add the registry client (#4142). - Materialize WoT Thing Descriptions into the server address space, deriving types from Thing Models and instances from Thing Descriptions (#4154). - Restore the WoT asset registry bridge so the deprecated 1.02 asset surface is backed by the registry (#4171). - Add the WoT Connectivity samples and complete the documentation (#4156), and restore the remaining coverage and docs (#4172). ## New projects `src/Opc.Ua.WotCon.Bindings`, `src/Opc.Ua.WotCon.Bindings.Mqtt`, `tests/Opc.Ua.WotCon.Bindings.Tests`, `tests/Opc.Ua.WotCon.Samples.Tests`, and the `samples/WotCon` AggregationServer, AggregationClient and FlatTagServer samples. ## Constituent pull requests Merged into this branch top-down, each independently approved: | PR | Title | | --- | --- | | #4128 | [Types] Complete ExtensionObject decoding and add an atomic file replace capability | | #4131 | [Types] Add lossless conversion between WoT documents and NodeSet2 | | #4132 | [SourceGeneration] Improve NodeSet to ModelDesign conversion and generated node state | | #4134 | [SourceGeneration] Generate OPC UA models from WoT Thing Description files | | #4147 | [Server] Add live NodeManager shadow and immediate reload | | #4150 | [Server] Make subscription transfer between sessions transactional | | #4130 | [XRegistry] Allow clients to use explicit registry roots | | #4139 | [WotCon] Add the WoT Connectivity 1.1 information model | | #4140 | [WotCon] Add the WoT protocol binding abstractions and planners | | #4144 | [WotCon] Add the HTTP, Modbus, OPC UA and MQTT binding executors | | #4146 | [WotCon] Add the WoT Connectivity registry and its stores | | #4142 | [WotCon] Add the WoT Connectivity registry client | | #4154 | [WotCon] Materialize WoT Thing Descriptions into the server address space | | #4156 | [WotCon] Add the WoT Connectivity samples and complete the documentation | | #4171 | Restore WoT asset registry bridge | | #4172 | Restore WoT coverage and docs | Supersedes #4093. ## Follow-up The WoT Connectivity and WoT Binding drafts have moved to 1.1-draft2 since this work was authored. Aligning with that revision - the removed group vocabulary, the new projection/View construct, the remaining model vocabulary terms and the two new portable-identity validation rules - is tracked separately and will follow in its own pull request. ## Validation Built for every target framework with zero warnings and zero errors, and validated at the tip of the stack with `Opc.Ua.WotCon.Tests` at 999 passed / 0 failed and `Opc.Ua.Server.Tests` at 4024 passed / 0 failed. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8 |
||
|
Marc Schier
|
724ae71f8a |
Make NodeId assignment during a node copy explicit (#4157)
Fixes #4137. ## The problem `NodeState.Initialize(ISystemContext, NodeState)` copies a node. Every child it materialises through `CreateChild` gets a per-instance NodeId from `ISystemContext.NodeIdFactory`, and is then initialised from its source on the very next statement - which overwrites that NodeId. The assignment is therefore *always* discarded: it only burns identifiers, and permanently leaks them for factories that track outstanding allocations. Booting the Robotics node manager left ten reservations behind that every later allocation skipped. #4123 fixed the symptom by hiding the factory for the duration of the copy, wrapping the context in one that reports `NodeIdFactory` as `null`. The review objection was fair: a context that lies about one property is hard to reason about, and it only works because that property is allowed to be null. ## What this does States the intent instead of faking the environment. NodeId assignment is now an argument of the two members that materialise children: ```csharp public virtual BaseInstanceState? CreateChild( ISystemContext context, QualifiedName browseName, bool assignInstanceNodeIds = true); protected virtual BaseInstanceState? FindChild( ISystemContext context, QualifiedName browseName, bool createOrReplace, BaseInstanceState? replacement, bool assignInstanceNodeIds = true); ``` A copy passes `assignInstanceNodeIds: false` and every node type - generated or hand-written - sees the real context. There is no capability property, no wrapper, and no second overload to keep in sync. `MethodState`, `BaseDataVariableState` and the source generator template each collapse to a single `FindChild` override that threads the argument into its `CreateOrReplace<Child>` helpers. ## Breaking change The four argument `FindChild` and the two argument `CreateChild` virtuals are gone. - **Call sites are unaffected.** The new parameter defaults to `true`, so `CreateChild(context, browseName)` and `FindChild(context, browseName, true, null)` keep compiling and keep the 1.5.378 behaviour. - **Overrides must add the parameter.** A node manager that overrides either member gets `CS0115` until it does. The compiler points at every site; `docs/migrate/2.0.x/node-states.md` shows the before/after. This is deliberate, per review: the stack ships all base types, and types generated from a model are regenerated for 2.0 anyway. ## Also fixed `MethodState.FindChild` returned `OutputArguments` when asked for `InputArguments` with `createOrReplace: false` - a pre-existing bug in the method being restructured. Covered by `FindChildReturnsTheRequestedArgumentsProperty`. ## Validation - Full `dotnet build UA.slnx`: **0 errors, 0 warnings**. - `Opc.Ua.Types.Tests`, `Opc.Ua.SourceGeneration.Core.Tests`, `Opc.Ua.SourceGeneration.Tests`, `Opc.Ua.Di.Tests`, `Opc.Ua.Robotics.Tests` - all passing. - Each new regression test was checked to fail without its corresponding fix. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485 Copilot-Session: f45109d9-59cb-4ffd-a4f7-16e1ea449ae4 |
||
|
Marc Schier
|
0357380335 |
Source generator: typed NodeSet method signatures and referenced model accessors (#4124)
**Stack 2/5** — base #4123. Part of the #4096 split; see #4096 for the overall picture. > Review after #4123. The diff shown against `master` will include #4123 until that merges. ## What this is Source generator only; no runtime library changes. ## Contents - **Typed NodeSet method signatures.** A method imported from a NodeSet now gets a typed signature. Methods that share a declared signature are grouped onto one generated `MethodType`, named after its owner when the plain name would collide, so two unrelated `Execute` methods no longer share a class. - **An override that keeps the signature it inherits keeps the declaration it overrides.** Pointing `MethodDeclarationNode` at the override itself made `MethodDeclarationId` self-referential. OPC 10000-4 lets a caller invoke a method with the NodeId of its declaration in the ObjectType *or any supertype*, and `MethodState.MethodDeclarationId` is what the server matches on, so `Call` stopped resolving and returned `Bad_MethodInvalid` for every alarm subtype. An override that genuinely changes its signature still becomes its own declaration. - **`StateNumbers` / `TransitionNumbers` were emitted empty for every NodeSet-sourced model**, because such children arrive as `VariableDesign` rather than `PropertyDesign` and were filtered out. Any companion model built from a NodeSet was writing `StateNumber = 0` on the wire — a Part 16 defect well beyond the models in this stack. The ModelDesign path is unchanged. - **Referenced models can contribute fluent accessors**, carried by a model dependency payload so a downstream model can extend an upstream one. ## Notes for the reviewer - `tests/Opc.Ua.SourceGeneration.Tests` is left at its `master` content here. Its new vendor-subtype test uses the Robotics NodeSet as its fixture, so it can only land with the Robotics change at the top of the stack (5/5). - `PumpInstanceNodeIdRegressionTests` gains six `InputArguments`/`OutputArguments` entries for argument-less standard condition methods. Typed method signatures materialise those properties from the `ConditionType` declaration even where the method takes none. That is a real deviation, pinned rather than hidden so it stays visible; it is worth a follow-up. ## Validation `dotnet build UA.slnx` — 0 errors. | Suite | Result | | --- | --- | | Opc.Ua.SourceGeneration.Core.Tests | 3739 / 0 | | Opc.Ua.SourceGeneration.Tests | 81 / 0 | | Opc.Ua.Di.Tests | 304 / 0 | | Opc.Ua.History.Tests | 506 / 0 | | Opc.Ua.Gds.Tests | 1095 / 0 | History and GDS are listed because they are what the method-declaration fix above is load-bearing for: without it they fail with 10 and 370 errors respectively. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485 |
||
|
Marc Schier
|
8a10139b8c |
Fix OPC UA address-space compliance in the device integration stack (#4117)
# Description
A live OPC UA compliance audit of `samples/PumpDeviceIntegrationServer`
against the **OPC 40223 Pumps** and **OPC 10000-100 (DI)** companion
specifications found **13 address-space violations** plus a
serialization bug in the MCP tool. Every fix lands at its **root cause
in the shared stack** rather than as a workaround in the sample, so all
servers benefit.
The sample now exposes **N fully simulated pumps** (`--pumps N`) and
advertises the DI conformance facets it actually satisfies.
> This branch has been merged with `master` including **#4116** ("Add
simulation to PumpDeviceIntegrationServer and Event modelling"), which
independently reworked the same pump simulation and the same fluent
alarm builders. See [Relationship to #4116](#relationship-to-4116) below
for how the two were reconciled.
## Findings and fixes
| # | Finding | Root cause | Fix |
|---|---------|-----------|-----|
| 1 | `OverTempAlarm` created by the fluent API did not exist in the
address space | `AttachAlarm` attached the alarm with `AddChild` but
never indexed it into the node manager | Register the node; the same gap
existed in `AddObject`, `CreateInstance` and the state-machine creators
|
| 2 | Alarms were unsubscribable — `EventNotifier = 0` everywhere, no
`HasNotifier` from `Server` | No event-source wiring at all | Set
`HasCondition`, initialise
`SourceNode`/`SourceName`/`ConditionName`/`InputNode`, promote
`EventNotifier` up the ancestor chain, publish `HasNotifier` from the
`Server` object |
| 3 | Every instance NodeId was minted in the **DI standard namespace**
(`ns=4;s=5001_Pump #1`) | `DiNodeManager.New` inherited the parent's
namespace index | New `DiNodeManager.InstanceNamespaceIndex`; instances
move to the server's application namespace |
| 4 | `Server/Namespaces` described only 2 of 6 namespaces | The
NodeSet2 importer drops nodes parented to namespace 0, so companion-spec
metadata objects never reached the address space — and no route covered
the server's own namespace | New `NamespaceMetadataPublisher` walks the
`NamespaceArray` and fills version/publication date from
`ModelDependencyAttribute` |
| 5 | Vendor BrowseNames (`Diagnostics`, `LastError`, …) sat in the DI
namespace | Builders defaulted to the parent's namespace | Default to
the server namespace; **spec-defined BrowseNames are unchanged** |
| 6 | `AccessLevel="5"` from the Pumps NodeSet was reported as `1` | The
ModelDesign `AccessLevel` enum is not `[Flags]` and cannot represent
`CurrentRead \| HistoryRead` | Carry the verbatim NodeSet2 bitmask and
emit named `AccessLevels` constants |
| 7 | `Historizing = true` while `HistoryRead` returned
`BadHistoryOperationUnsupported` | Nothing reconciled the declared
history surface with the wired historians | Startup clears the
advertisement (including masking the attribute read callbacks) when no
provider resolves; the sample wires a real historian |
| 8 | Mandatory `ProductInstanceUri` unset | Sample omission | Populated
on every pump |
| 9 | Mandatory `TrueState`/`FalseState` empty on every supervision
boolean | Absent in the official NodeSet; a server must still populate
them | Populated on every pump |
| 10 | Machinery `Machines` folder empty (OPC 40001-1 §9.2 says
**shall**) | `TryAddToMachinesFolder` used `AddChild`, which
*re-parents* the device away from `DeviceSet` | Add the `Organizes`
reference instead |
| 11 | BrowseName contained a space and `#` | Sample naming | `Pump_1`
with the readable label in `DisplayName` |
| 12 | `Maintenance` group materialised but empty | Sample omission |
Populated |
| 13 | Uninitialised values reported `Good` | No initial-value status |
`BadWaitingForInitialData` until the first simulation tick |
| — | `ServerProfileArray` advertised only `StandardUA2017` |
`DiNodeManager` did not implement the existing `IConformanceContributor`
| Contribute DI 1.05 server facets computed at runtime from what is
actually wired, merged with `StandardUA2017` |
| — | MCP tool serialised boolean `false` and numeric `0` as JSON `null`
| `Variant.Null` is `default`, so `Variant.Equals` adopts the non-null
type and compares against the default value | Dispatch on `BuiltInType`
and use typed `TryGetValue` accessors instead of the prohibited
`AsBoxedObject` |
## Relationship to #4116
#4116 landed on `master` while this branch was in flight and targeted
overlapping ground. The merge is a **union of both intents**, not a
fast-forward:
- **`AlarmBuilderExtensions`** — kept both sides. #4116 contributes the
parent-must-be-an-Object guard, `SetEnableState`, and the
`HasEventSource` references; this branch contributes the `HasCondition`
reference, condition source initialisation, **registration of the alarm
with the node manager so it is browsable**, notifier promotion up the
whole ancestor chain, and root-notifier registration.
- **`SupervisionBuilderExtensions`** — took #4116's `SetAlarmActive`. It
is more spec-correct than this branch's version: it honours
`EnabledState`, resets Acked/Confirmed on activation, and computes
`Retain` per OPC 10000-9 so an unacknowledged Condition stays retained
after going inactive. This branch's null guard was kept so a malformed
alarm cannot dereference a missing `EnabledState`.
- **`PumpNodeManager.Configure.cs`** — took #4116's **push-based**
simulation (`RegisterPumpSimulation`, `CreatePumpSimulation`,
`IValueUpdater<T>`, `Initialize`/`Advance`/`Publish`, deterministic
per-pump phase offsets) as the structure, then folded in this branch's
compliance work: configurable pump count, Maintenance group,
`TrueState`/`FalseState`, historian wiring, per-pump mandatory
Identification values, and `BadWaitingForInitialData` until the first
tick.
- **README** — union, keeping #4116's address-space validation workflow
section.
Two test adjustments were needed, both because the merge legitimately
changed behaviour the tests pinned:
- `PumpHostedReferenceTests` now resolves pump NodeIds from the browse
result instead of hard-coding the DI namespace, and waits for the first
published value. `PumpCreatedAfterStartupJoinsTheLiveSimulationAsync`
was **added** to restore #4116's coverage that a pump created after
startup through `ConfigureDevicesFor` joins the live simulation.
- `FluentAlarmRegistrationIntegrationTests` no longer asserts that
`Retain` clears when an alarm goes inactive — that pinned this branch's
weaker semantics, and #4116's Part 9 behaviour is correct.
## Breaking change
Runtime-created DI device instance NodeIds move from the companion-spec
namespace to the server's application namespace, e.g.
`ns=<DI>;s=5001_Pump #1` becomes `ns=<application>;s=5001_Pump #1`. This
invalidates NodeIds cached by clients from earlier 2.0 previews. It is
**not** a model change — standard DI/Machinery/Pumps type NodeIds and
spec-defined BrowseNames are untouched.
Device integration hosting is new in 2.0, so there is nothing to migrate
from 1.5.378 and this is deliberately *not* in `docs/MigrationGuide.md`.
Anyone tracking 2.0 previews should rediscover devices by browsing
`DeviceSet` rather than persisting NodeIds, and resolve namespace
indexes from `NamespaceArray` per connection.
## Verification
Compliance is pinned by regression tests that assert **through the
server's service surface** (`BrowseAsync` / `ReadAsync` /
`HistoryReadAsync`), not by inspecting `NodeState` objects in memory.
That distinction mattered: three defects survived in-memory assertions
and were only caught through the read path — most subtly, `Historizing`
was cleared while `AccessLevel` still advertised `HistoryRead`, because
attribute read callbacks re-applied the NodeSet bits.
Post-merge on net10.0:
| Suite | Result |
|---|---|
| `Opc.Ua.Di.Tests` | 326 passed |
| `Opc.Ua.Server.Tests` | 3846 passed, 5 skipped |
| `Opc.Ua.Tools.Tests` | 288 passed |
| `Opc.Ua.History.Tests` | 506 passed, 27 skipped |
| `Opc.Ua.SourceGeneration.Core.Tests` | 3722 passed, 8 skipped |
The full solution was also built and tested on **net48** before the
merge.
Review feedback from `copilot-pull-request-reviewer` has been addressed
in commit `badad31dc`: the shared `ServerObjectState` lock was removed
(the forward `HasNotifier` edge is published through the owning node
manager instead), MCP array serialization now preserves element types,
and the pump option guards report the specific rejected option. The only
failure is
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, a
pre-existing certificate-store issue that **passes when run in
isolation** and is unrelated to this change.
## Documentation
Updated `docs/DeviceIntegration.md`, `docs/MigrationGuide.md`,
`docs/HistoricalAccess.md`, `docs/DeveloperGuide.md`,
`docs/McpServer.md`, `docs/README.md`,
`samples/PumpDeviceIntegrationServer/README.md` and
`tools/Opc.Ua.Mcp/README.md`, and consolidated the four node-manager
documents into a single [`docs/NodeManagers.md`](docs/NodeManagers.md) —
overview, built-in managers (including `MasterNodeManager`), core vs
custom, registration, server address-space metadata, and source
generation — with a TOC and all inbound links repointed.
## Related Issues
No tracking issue exists for this work yet — it originated from an
ad-hoc compliance audit of the pump sample. Given the size of the change
and the runtime-breaking NodeId move, please open (or link) a tracking
issue so the design is recorded as an ADR before merging.
- Fixes #
## Checklist
_Put an `x` in the boxes that apply. You can complete these step by step
after opening the PR._
- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [x] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
|
||
|
Marc Schier
|
cfd15f08e1 |
Assign instance NodeIds in generated CreateOrReplace<Child> helpers (#4104)
# Description
The model source generator emits two families of instance helpers with
inconsistent NodeId behaviour:
* `ISystemContext.CreateInstanceOf<Type>(parent, browseName)` rebases
the whole materialised subtree onto per-instance NodeIds.
* `<Owner>.CreateOrReplace<Child>(context, replacement)` — the plumbing
behind `NodeState.CreateChild` / `ReplaceChild` — did not. The child it
produced was either bare (null NodeId, no browse name) or kept the
type-level NodeId of the replacement.
Any node manager mixing the two had to hand-roll a recursive re-stamp
before registration, or sibling instances collided on NodeIds — a
failure that only surfaces at registration time.
`samples/PumpDeviceIntegrationServer` did exactly this.
## What changed
**Generator** (`tools/Opc.Ua.SourceGeneration.Core`)
* `CreateOrReplace<Child>` gains `bool assignInstanceNodeIds = true`.
When enabled and the context supplies an `ISystemContext.NodeIdFactory`,
a child whose NodeId is null or still equal to its declaration NodeId —
and its descendants — are rebased through
`NodeInstanceExtensions.AssignInstanceNodeId` /
`AssignInstanceChildNodeIds`, with the owner as reference root. **A
NodeId the caller already assigned is never overwritten.**
* The helper also stamps the new child's `SymbolicName` / `BrowseName` /
`DisplayName`. Without an identity, a path-based `INodeIdFactory`
derives `{parent}_` for *every* child of a parent, so the assignment
would have introduced collisions rather than removed them. `BrowseName`
stamping is guarded on `context.NamespaceUris != null` — event-reporting
paths pass a context without a namespace table.
* All generated `Create*` factory call sites pass
`assignInstanceNodeIds: false`: they build declaration subtrees whose
NodeIds must stay at their type-level values, and the enclosing
`CreateInstanceOf<Type>` factory rebases the finished subtree in a
single pass.
* The `CreateInstanceOf<Type>` rebase guard drops the `parent != null`
requirement — supplying a `browseName` is what marks a dynamically
materialised instance. Calling it without one (as the generated
`NodeStateActivator`s do, and when replacing a well-known singleton)
still keeps the declaration NodeIds.
* `replacement` is now emitted as `BaseInstanceState?` on
`CreateOrReplace<Child>` and on the generated `FindChild` override,
matching the documented contract and the base `NodeState.FindChild`
signature.
**Hand-written twins** —
`MethodState.CreateOrReplace{Input,Output}Arguments` and
`BaseDataVariableState.CreateOrReplaceEnumStrings` gain the same
parameter and behaviour, sharing a new internal
`NodeInstanceExtensions.AssignNewChildInstanceNodeIds`. This is also
required so the generated `public new` overrides keep hiding a matching
base signature (otherwise `CS0109` with `TreatWarningsAsErrors`).
**Sample** — `PumpDeviceIntegrationServer` now passes the real
`DeviceSet` parent to the factory and drops `AssignChildNodeIds` plus
the manual root stamp.
**Docs** — new *"Materialising instances at runtime — NodeId
assignment"* section in `SourceGeneratedNodeManagers.md` and a refreshed
`DeviceIntegration.md`.
## Design note: why not the guard proposed in the issue
Gating on *"the owner already has an instance NodeId"* cannot be
implemented as `owner.NodeId != <ownerTypeNodeId>`. The generated child
factories call `<Owner>.CreateOrReplace<Child>` on **declaration**
instances — `CreatePumpType_Operational` runs on a
`FunctionalGroupState` whose NodeId is `ObjectIds.PumpType_Operational`,
not `ObjectTypeIds.FunctionalGroupType` — so the comparison would be
true while the *type template* is being materialised (`forInstance:
false`) and would rewrite the address space's declaration NodeIds. The
explicit opt-out separates the two callers unambiguously.
## Compatibility
Adding an optional parameter is source-compatible. Behaviour changes for
callers of `NodeState.CreateChild` / `ReplaceChild` when the context
carries a `NodeIdFactory`: the new child now carries a browse name and a
per-instance NodeId instead of nothing.
## Related Issues
- Fixes #4099
> [!NOTE]
> The issue asks to delete `AssignChildNodeIds` from
`samples/MinimalRobotServer`. That sample does not exist on `master` —
it only lives on the still-open #4096 branch and carries no such helper.
The equivalent boilerplate on `master` is
`PumpDeviceIntegrationServer/PumpNodeManager`, which this PR cleans up.
Details in [this
comment](https://github.com/OPCFoundation/UA-.NETStandard/issues/4099#issuecomment-5102275507).
> [!NOTE]
> One commit is an unrelated one-line-scope build-hygiene fix: two
`CA2000` warnings in `ConsoleReferenceClient` where subscription
ownership transfers to the session. Happy to split it out if preferred.
## Checklist
- [x] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [x] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [x] I have addressed **all** PR feedback received.
### Tests added
**Generator emission** (`NodeManagerGeneratorTests`) —
`CreateOrReplaceChildAssignsInstanceNodeIdsByDefault`,
`TypeFactoriesOptOutOfCreateOrReplaceNodeIdAssignment`,
`CreateInstanceOfFactoriesRebaseWithoutAnExplicitParent`.
**Helper behaviour** (`NodeInstanceExtensionsTests`) —
`CreateOrReplaceArgumentsAssignsInstanceNodeIds`,
`CreateOrReplaceArgumentsKeepsCallerAssignedNodeId`,
`CreateOrReplaceArgumentsHonoursTheAssignmentOptOut`,
`CreateOrReplaceEnumStringsAssignsInstanceNodeIds`.
**Pump DI server regression** (`PumpInstanceNodeIdRegressionTests`,
added on review feedback):
| Test | Asserts |
| --- | --- |
| `ConfiguredPumpExposesTheFullModelSurface` | The exact set of **77
model identifiers** a configured pump materialises. |
| `EveryPumpInstanceExposesTheSameGeneratedModelSurface` | A second pump
exposes the identical generated surface, minus what the fluent
`Configure` pass adds. |
| `MaterialisedPumpNodeIdsComeFromTheNodeIdFactory` | Every NodeId is
`{parentIdentifier}_{symbolicName}` in the parent's namespace — minted
by `PumpNodeManager.New`. |
| `MaterialisedPumpNodeIdsAreUniqueAcrossInstances` | No NodeId is null
or shared between two instances of the type. |
| `GeneratedFactoryAssignsInstanceNodeIdsBeforeRegistration` | The
factory output already carries per-instance NodeIds before
`AddPredefinedNodeAsync` applies its defensive rebase. |
| `AlarmSubtreeKeepsStandardDeclarationNodeIds` | Pins the alarm
subtree's standard-namespace ids — a pre-existing gap outside the
generated helpers. |
The 77-identifier baseline was captured by running the same subtree walk
against the pre-change commit and diffing it against the post-change
run: **identical** — same node set, same NodeIds.
### Local test runs
| Suite | net10.0 | net48 | net472 |
| --- | --- | --- | --- |
| `Opc.Ua.SourceGeneration.Core.Tests` | 3722 passed | 3721 passed | — |
| `Opc.Ua.SourceGeneration.Tests` | 81 passed | 81 passed | — |
| `Opc.Ua.SourceGeneration.Stack.Tests` | 90 passed | — | — |
| `Opc.Ua.Types.Tests` | 8083 passed | 8076 passed | — |
| `Opc.Ua.Di.Tests` | 303 passed | 303 passed | 277 passed |
| `Opc.Ua.Core.Tests` | 4134 passed | — | — |
| `Opc.Ua.Gds.Tests` | 1095 passed | — | — |
| `Opc.Ua.History.Tests` | 506 passed | — | — |
| `Opc.Ua.Server.Tests` | 3626 passed, 1 failed | 3574 passed, 1 failed
| — |
The single `Opc.Ua.Server.Tests` failure —
`ServerFluentApiHostingTests.ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`
— is pre-existing and unrelated; it reproduces on a stashed clean
`master` tree.
> [!NOTE]
> The source-generation test harness compiles generated model code
against a hand-written `Opc.Ua` API stub
(`tests/Opc.Ua.SourceGeneration.Core.Tests/CompilerUtils.cs`, shared by
three test projects). Its 73 `CreateOrReplace<Child>` declarations were
updated to the new signature — without that, every generated call site
passing `assignInstanceNodeIds: false` failed to bind.
`NodesetMethodArgumentTests` pinned the exact emitted call and was
updated to the new one for the same reason as the
`NodeStateGeneratorTests` expectations.
> [!NOTE]
> `master` has been merged into this branch. The only conflict was in
`NodeStateGeneratorTests.cs`, where master rewrapped the expected
call-site strings while this branch appended `assignInstanceNodeIds:
false` to them — kept master's wrapping and re-applied the argument.
Master's two new templates (`Create_MethodArguments`,
`Assign_MethodArgumentValues`) call
`CreateOrReplace{Input,Output}Arguments` while building a declaration
subtree, so they now pass `assignInstanceNodeIds: false` to preserve
their stated intent of leaving the property node's identity untouched.
> [!TIP]
> Changing a source generator leaves dependent projects' cached
generated output stale. If a local build reports `CS1739: ... does not
have a parameter named 'assignInstanceNodeIds'` or `CS0109` on a
generated `CreateOrReplace*`, run `dotnet build <project> -t:Rebuild`.
Clean CI builds are unaffected.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b019f3a2-f58d-4f7e-93db-c8a0dec656e2
|
||
|
Marc Schier
|
5b9bcedc62 |
Add OPC UA ISA-95 Common Model and Job Control support (#4089)
# Description Adds first-class support for the OPC UA ISA-95 companion specifications: - OPC-10030 Common Model. - OPC-10031-4 Job Control V1. - OPC-10031-4 Job Control V2. The change introduces `Opc.Ua.ISA95`, `Opc.Ua.ISA95.Client`, and `Opc.Ua.ISA95.Server`, together with a NativeAOT-compatible minimal server sample and comprehensive tests. ## Highlights - Compiles the three canonical NodeSets and identifier mapping CSV files through the source generator. - Adds NodeSet identifier mapping validation, typed method arguments/results, companion event records, and generated method argument nodes. - Provides typed Common Model builders with normative relationships and an injectable `GeoSpatialLocationType` provider. - Implements lossless V1/V2 Job Control providers over a shared version-neutral store, including state paths, status events, comments, response queries, catalog changes, and cohesive DI registration. - Adds subtype-aware typed clients, standalone encodeable registration, `IIsa95ClientFactory`, and V2 status-event streaming. - Adds the `MinimalIsa95Server` sample, package documentation, and an ISA-95 guide with conformance and HA guidance. - Tracks typed RSL/GPOS integration as follow-up issue #4095. ## Validation - `Opc.Ua.ISA95.Tests`: 137/137 passed on `net10.0` and `net48`. - Source-generator suites: 81/81 model, 90/90 stack, 3718 passed / 8 skipped core. - `EventRecordFieldReaders` feedback tests: 6/6 on `net10.0` and `net48`. - `EventRecordFieldReaders` changed-method patch coverage: 100% line coverage (36/36) and 100% branch coverage (22/22). - `Opc.Ua.ISA95`, `.Client`, and `.Server` build across all configured target frameworks without new diagnostics. - Cobertura line coverage: 88.08% model, 90.04% client, 91.19% server. - NativeAOT sample publishes and passes a live typed V2 Store/status-event validation. - The ISA-95 NativeAOT smoke test passes in the source-generated native test executable. - OPC UA KB NodeSet validation reports zero errors for Common, V1, and V2. The broader NativeAOT executable still reports unrelated existing GDS and complex-type test failures; the ISA-95-specific native test passes. ## Node state generator fixes Generating method arguments inline for imported nodesets regressed every model that already emits `InputArguments` / `OutputArguments` child node factories: - The inline block ran before the child factories. `MethodState.CreateOrReplaceInputArguments` keeps the property that was assigned first, so the child carrying the well-known NodeId was dropped and a NodeId-less placeholder remained in the address space. This broke browsing (reference descriptions without a target) and made method arguments unreachable at their well-known NodeIds. - The `MethodDeclarationId` fallback keyed on an autogenerated display name redirected every declared method of the standard address space to its own NodeId, which made method calls fail. The inline block is now emitted after the child factories and reduced to a value assignment when a child factory exists. The value is only re-emitted when an argument references a namespace other than OPC UA, because the value of an imported property is the verbatim XML of the source nodeset whose namespace indices do not survive the import. The `MethodDeclarationId` fallback is restricted to declarations without a resolvable NodeId. The generated OPC UA address space is byte-identical to `master` again. ## Related Issues - Fixes #4088 - Follow-up: #4095 |
||
|
Marc Schier
|
66be6731f4 |
Source generator: support compiled companion models and safe NodeState writes (#4063)
## Summary Combines the compiled companion-model support from #4062 with the NodeState emission fixes already in this PR. The model source generator can now consume compiled companion `NodeSet2` models more reliably, while generated factories consistently target base `NodeState` members even when a generated subclass shadows them. ## Changes ### Compiled companion NodeSets - Add `OpcUaStateTypeIndex` and wire it through `ModelSourceGenerator` / `ModelCompilation` so standard (`ns=0`) node-state and method-state class names are resolved from referenced `Opc.Ua.Core(.Types)` metadata. - Add the model-generation-scoped `StandardMethodStateFallback`, which degrades an unresolved standard typed `*MethodState` reference to `global::Opc.Ua.MethodState` only when the type is neither available in the compilation nor declared by the current generation pass. - Keep declaration names outside the reference-only fallback policy, propagate fallback state through the logical execution context, reuse the immutable availability index, and use named incremental-generator inputs instead of deep `Left` / `Right` chains. - Escape generated fluent-builder XML documentation where browse names can contain XML-sensitive characters. ### Safe NodeState factory emission - Declare one base-typed `global::Opc.Ua.NodeState nodeState = state;` local per generated factory. - Use `nodeState` for base-member reads and writes to `NodeId`, `SymbolicName`, `BrowseName`, `DisplayName`, `Description`, `WriteMask`, `UserWriteMask`, `AccessRestrictions`, `ReleaseStatus`, `Categories`, `Specification` and `IsPartOfTypeHierarchy`. - Base-qualify the optional-child `previousNodeId` read, NodeId comparison, and existing-child NodeId update. - `global::`-qualify emitted `NodeId` / `ExpandedNodeId` namespace prefixes to avoid namespace collisions. ### Supporting updates - Expose `Opc.Ua.Types` internals to `Opc.Ua.Core.Encoders.Tests`. - Update generator assertions and add focused fallback regression coverage. ## Why Compiled companion models can instantiate standard types whose curated Core implementation does not expose the conventionally named typed method-state class. The scoped fallback lets those models generate without weakening the Stack generator or masking classes emitted by the current model. Separately, generated subclasses can shadow base `NodeState` properties with strongly typed `new` properties. Routing base attributes through a `NodeState`-typed local guarantees initialization reaches the intended base member while keeping generated code smaller and clearer than repeated casts. ## Validation - Full `Opc.Ua.SourceGeneration.Core.Tests` on net10.0: **3718 passed / 8 skipped / 0 failed**. - Combined focused generator coverage on net48: **21 passed / 0 failed**. - Full `Opc.Ua.SourceGeneration.Tests`: **68 passed / 0 failed**. - Full `Opc.Ua.SourceGeneration.Stack.Tests`: **90 passed / 0 failed**. ## Context This PR supersedes #4062 and contains both source-generator change sets. The companion-model fallback remains the scoped interim workaround tracked by #4060; a future upstream name-resolution step can remove it. These changes also feed the experimental data-encodings work in #4007. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Marc Schier
|
84fa8e8906 |
CTT follow-up: monitored items, alarms, codecs, history, and model validation (#4002)
# Description Follow-up to #3978 based on the latest OPC UA CTT aggregate, matrix, Historical Access, Attribute, and Node Management runs. ## Aggregate run 16 - Ignore synthetic `BadBoundNotFound` raw-bound markers when feeding aggregate calculators. - Add direct and live historian regression coverage. ## Multi-dimensional Variant matrices - Fix `Variant.Copy()` so rank-2 Variant matrices preserve their payload and dimensions. - Enforce valid Variant matrix dimensions consistently across Binary, JSON, XML stream, and XML parser codecs. - Encoders return `BadEncodingError`; decoders return `BadDecodingError` for malformed matrix dimensions. - Add cross-codec valid/null/empty/malformed tests and a batched ReferenceServer Read regression. - Correct the previous CTT issue attribution: the invalid `Scalar_Static_Arrays2D_Variant` server encoding poisoned the batched CTT Read. ## Node Management AddNodes - Return `BadNodeIdRejected` for unsupported requested-NodeId namespaces. - Validate hierarchical ReferenceType source/target NodeClass constraints. - Document run-18 confirmation that all remaining 15 AddNodes errors are CTT project/script configuration issues. ## Historical Access and Attribute Services run 18 - Reject whitespace in `NumericRange` syntax with `BadIndexRangeInvalid`. - Add HistoryRead tests for operation-level invalid syntax and per-DataValue `BadIndexRangeNoData`. - Document CTT defects: - `012.js` expects `BadIndexRangeNoData` at the operation level instead of on returned DataValues; - `Err-012.js` uses a non-historizing node for an access-denied test; - generic Attribute array helpers omit NodeId[] and StatusCode[] conversion paths. ## Base Information model validation run 19 - Run the CTT main session as `sysadmin` over Basic256Sha256 SignAndEncrypt so SecurityAdmin RolePermissions and EncryptionRequired restrictions are both satisfied without weakening the server model. - Preserve predefined well-known NodeIds while making generated dynamic type and optional-child factories allocate compatible per-instance NodeIds, remap internal references, and update references from the owning subtree. - Materialize dynamic RoleType properties through generated helpers and keep role-child string NodeIds disjoint from numeric RoleManager root IDs. - Add net48/net10 regressions for generator output, NodeId allocation/reference remapping, DI mandatory children, dynamic roles, and low-privilege versus SecurityAdmin Browse visibility. - Document CTT defects for permission-unaware mandatory-child validation, the UA 1.04 reference model used against a UA 1.05 server, stale ServerDiagnostics cache, and the `ConformanceUnits` scalar/array mismatch. ## Related Issues - Follow-up to #3960 and #3978. ## Checklist - [x] I have signed the CLA and read the CONTRIBUTING document. - [x] Tests prove the fixes and improve coverage. - [x] Required CTT documentation is updated. - [x] Focused builds/tests introduce no warnings. - [x] Affected tests pass on .NET Framework and .NET 10. - [ ] CI and CodeQL are green. - [ ] All PR feedback is addressed. ## Latest CTT full run and A&C run 20 - Fix Monitor Value Change V2 `036.js`: an ambiguous `DataValue(StatusCode)` call encoded `BadIndexRangeNoData` as a Good value. - Prove all 19 configured matrix monitored items notify when the selected value actually changes; document the independent `042.js` script defects. - Rebase dynamic `CertificateExpired` / `TrustListOutOfDate` alarm roots and descendants, restore standard type declarations and ModellingRules, and expose the cross-node-manager references. - Deliver method-triggered events to every authorized Session, fixing the two-Session Confirm scenario. - Preserve locale-only `LocalizedText` values consistently across Binary, JSON, and XML and align Acknowledge/Confirm comment behavior and tests. - Document the remaining proven CTT defects for TransactionDiagnostics, SemanticChange, History `013`, PolicyId scope, A&C, durable subscriptions, and Subscription Minimum events. ## Base synchronization Merged `master` through `007cfaf90` (`#4010`, repository layout consolidation) and retained the matrix regression tests at their new `tests/` locations. ## Latest validation - Focused merged-tree tests pass on net10 and net48. - `Opc.Ua.Server` and `Quickstarts.Servers` Release builds complete with 0 warnings and 0 errors on net10 and net48. - The existing `DisableEnableViaTypeAndInstanceMethodNoEventsDuringDisabledAsync` timing test took its documented inconclusive timeout path after the layout merge; no test failure was reported. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <romanett98@gmail.com> |
||
|
Marc Schier
|
7cec2d614d | Move repository files to the consolidated layout |