GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-19 07:02:16 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,497 Commits 105 Branches 105 Tags
copilot/fix-github-actions-job-validation
Commit Graph

3 Commits

Author SHA1 Message Date
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
 2026-07-28 21:22:14 +02:00
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>
 2026-07-18 05:01:03 +02:00
Marc Schier 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00