mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-27 21:55:54 +08:00
copilot/fix-code-review-comment
11
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 MrAlaskan
|
8602d40cff |
fix(alias): compare PortableNodeId identifiers by NodeId semantics (#4037)
### Summary This PR fixes `AliasNamePubSubRefreshStrategy` category matching for string NodeIds whose identifier contains semicolons and adds a regression test covering that scenario. ### Problem `AliasNamePubSubRefreshStrategy` is supposed to match the publisher's `PortableNodeId.Identifier` against the local alias category `NodeId` while ignoring namespace index differences. However, the previous implementation did that by converting both NodeIds to text and stripping everything before the first `;`. That happened to work for simple identifiers such as `ns=1;s=MyCategory`, but it broke for valid string NodeIds whose identifier itself contains semicolons. For example, when the local category is `ns=1;s=A;B`, the previous logic transformed the local value into `s=A;B` but transformed the already namespace-free portable form `s=A;B` into just `B`. As a result, both sides represented the same identifier semantically, yet the refresh strategy could never match them and would silently ignore legitimate alias update messages for that category. ### Changes - Replace the string-prefix stripping comparison in `AliasNamePubSubRefreshStrategy` with namespace-insensitive `NodeId` equality, preserving the original identifier type and value. - Add a regression test that verifies a PubSub alias update for a category such as `s=A;B` still invalidates the local cache. |
||
|
MrAlaskan
|
8f4087605a |
Fix unordered server endpoint validation (#4029)
### Summary This PR makes `Session.ValidateServerEndpoints()` compare `ServerEndpoints` and `UserIdentityTokens` as unordered sets and adds regression tests that verify `OpenAsync()` no longer rejects legal servers that return the same endpoint data in a different order. ### Problem According to OPC UA Part 4, the client validates the endpoint set returned by `CreateSessionResponse.ServerEndpoints` against the endpoint set observed during discovery. The specification describes these values as a set of endpoint descriptions filtered by the relevant transport profile, not as an ordered array that must preserve the exact discovery-time enumeration order. Previously, `Session.ValidateServerEndpoints()` first checked the endpoint count and then compared `m_discoveryServerEndpoints[ii]` against `serverEndpoints[ii]` by index. It also compared `UserIdentityTokens[jj]` by index within each endpoint. As a result, a server that returned the same legal endpoints in a different order, or returned the same token policies in a different order within an endpoint, could be rejected with `BadSecurityChecksFailed` even though the endpoint data itself had not changed. ### Changes - Replace index-based `ServerEndpoints` validation with unordered matching based on the same endpoint fields already validated by the client. - Compare `UserIdentityTokens` as an unordered multiset instead of requiring the server to preserve the original token enumeration order. - Extend the client session test scaffolding so discovery endpoints can be supplied explicitly. - Add regression tests that verify `OpenAsync()` accepts reordered `ServerEndpoints` and reordered `UserIdentityTokens`. |
||
|
MrAlaskan
|
565fc02535 |
fix(subscription): fail transfer synchronization when deleting stale monitored items (#4031)
### Summary This PR makes `MonitoredItemManager.TrySynchronizeHandlesAsync()` treat `DeleteMonitoredItems` failures as transfer-synchronization failures and adds a regression test that verifies the client no longer reports success when the server rejects deletion of stale monitored items. ### Problem OPC UA Part 4 requires clients to evaluate service results at two levels: `ResponseHeader.ServiceResult` reports the outcome of the service call as a whole. `Results[]` reports the outcome of each individual operation in that service. Even when the service-level result is `Good`, the service may still have only partially succeeded. The client therefore still needs to inspect the response payload, especially the per-operation `StatusCode` values, before concluding that the requested work actually completed. In `TrySynchronizeHandlesAsync()`, the transfer/recovery path deletes monitored items that still exist on the server but no longer exist locally. Previously, this delete path treated `DeleteMonitoredItemsAsync()` as successful as long as the call itself did not throw, and it did not validate `ResponseHeader.ServiceResult`, `Results[]`, or the associated diagnostic information. As a result, the client could report transfer synchronization success even when the server explicitly returned failures for one or more stale monitored items. ### Changes - Validate the `DeleteMonitoredItemsAsync()` response inside `TrySynchronizeHandlesAsync()` instead of treating any non-throwing call as success. - Return `false` when the delete operation reports a bad service-level result or any bad per-item result, so synchronization only succeeds after stale server-side items are actually removed. - Add a regression test that verifies `TryCompleteTransferAsync()` returns `false` when deletion of an extraneous server-side monitored item is rejected. |
||
|
MrAlaskan
|
b105eaae02 |
fix(subscription): do not acknowledge keep-alive future sequence numbers (#4030)
### Summary This PR stops the V2 subscription message processor from acknowledging keep-alive sequence numbers and adds a regression test that verifies acknowledgements are only queued for notification messages that actually contain data. ### Problem According to OPC UA Part 4 publish semantics, a keep-alive `NotificationMessage` does not acknowledge delivery of a real notification. Its `SequenceNumber` identifies the next notification message that will be sent, not a notification payload that the client has already received. Previously, `MessageProcessor.ProcessMessageAsync()` correctly treated `NotificationData.Count == 0` as a keep-alive and intentionally avoided advancing the data-message deduplication gate. However, `OnNotificationReceivedAsync()` still unconditionally queued a `SubscriptionAcknowledgement` for every message, including empty keep-alives. As a result, the V2 client could acknowledge a future sequence number before the corresponding data/event notification had actually been received. This could produce spurious acknowledgement noise against strict servers and made the V2 path inconsistent with the existing classic subscription engine, which only acknowledges messages that contain notification data. ### Changes - Stop queueing acknowledgements for keep-alive messages in `MessageProcessor.OnNotificationReceivedAsync()`. - Preserve the existing keep-alive dispatch path and continue acknowledging notification messages that carry real data. - Extend the message processor tests to verify that keep-alives do not enqueue acknowledgements and that the subsequent real notification still does. |
||
|
MrAlaskan
|
12f770b3b6 |
Fix managed session failover target selection (#4025)
### Summary This PR makes `ManagedSession` select failover targets relative to the currently active endpoint after a successful failover and adds a regression test that verifies subsequent failover decisions no longer use the original connection target as the current server. ### Problem According to OPC UA Part 4 redundancy semantics, once a client has failed over to a different redundant server, subsequent target selection must be based on the server that is currently hosting the active session. The current server is not permanently tied to the endpoint that was used for the initial connection. Previously, `ManagedSession` stored the constructor-supplied `ConfiguredEndpoint` in a read-only property and always passed that outer value into `IServerRedundancyHandler.SelectFailoverTarget()`. However, the inner `Session` updates its own `ConfiguredEndpoint` during `RecreateInPlaceAsync()` when failover succeeds. After the first failover, the outer `ManagedSession` and inner `Session` could therefore disagree about which redundant member was currently active, causing later failover decisions to be evaluated against the original endpoint instead of the active one. ### Changes - Make `ManagedSession.HandleFailoverAsync()` prefer the inner session's current `ConfiguredEndpoint` when selecting a failover target, while keeping the outer endpoint as a fallback before the session is available. - Preserve the existing failover flow and scope the fix to target selection only. - Add a regression test that verifies `HandleFailoverAsync()` passes the inner session's current endpoint to `IServerRedundancyHandler.SelectFailoverTarget()`. |
||
|
|
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> |
||
|
|
b76022d13d |
Add MinimalClient - lightweight OPC UA console client with fluent API (#4006)
# Description
Adds `MinimalClient`, a compact console OPC UA application demonstrating
the unified dependency-injection surface, secure endpoint discovery,
managed sessions, fluent V2 subscriptions, and Alarms & Conditions
support.
## Changes
- Adds `ConfigureApplication(options => ...)` to build and validate one
shared `ApplicationConfiguration` for combined client/server hosts.
- Extends `OpcUaClientOptions` so the existing `AddClient(options =>
...)` callback accepts application identity, PKI, and
certificate-validation settings like `AddServer(...)`.
- Composes root `ConfigureApplication(...)` values with client defaults
in either registration order; shared values take precedence and client
values fill gaps.
- Keeps server samples on their established `AddServer(...)`
configuration pattern.
- Adds `MinimalClient` as a .NET 10 Native AOT console application using
top-level statements and `HostApplicationBuilder`.
- Discovers a `SignAndEncrypt` / `Basic256Sha256` endpoint by default;
`--insecure` and `--auto-accept` are explicit opt-ins with warnings.
- Demonstrates fluent `AddSubscription(...)`,
`TryAddMonitoredItem(...)`, and `AlarmClientFactory` usage.
- Adds targeted configuration, client, and combined client/server tests.
## Example
```csharp
builder.Services
.AddOpcUa()
.AddClient(options =>
{
options.ApplicationName = "MinimalClient";
options.ApplicationUri = "urn:localhost:OPCFoundation:MinimalClient";
options.ProductUri = "uri:opcfoundation.org:MinimalClient";
options.Session = new ManagedSessionOptions
{
SessionName = "MinimalClient",
SessionTimeout = TimeSpan.FromSeconds(60)
};
})
.AddDiscoveryAndConnect(options =>
{
options.DiscoveryUrl = discoveryUrl;
options.SecurityMode = MessageSecurityMode.SignAndEncrypt;
options.SecurityPolicyUri = SecurityPolicies.Basic256Sha256;
})
.AddSubscriptions()
.AddAlarms();
```
## Related Issues
_If there is no issue yet, open one and link it 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.
- [ ] 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-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: romanett <romanett98@gmail.com>
Co-authored-by: Marc Schier <marcschier@hotmail.com>
|
||
|
Marc Schier
|
41a648c9db |
Fail fast on reverse connect listener startup (#4009)
# Description Fail reverse-connect listener startup immediately instead of logging bind failures and surfacing a later connection timeout. - Validate and stage configured and programmatic listener endpoints before replacing active configuration. - Open all listeners atomically and report endpoint-specific failures with `BadTcpEndpointUrlInvalid`, the transport-specific status, or `BadNoCommunication`. - Roll back listeners opened by a failed attempt without reacquiring the manager lock, including forced listener disposal when normal close fails. - Preserve meaningful `ServiceResultException` status codes through both `StartService` overloads. - Dispose replaced configuration watchers and reject callbacks from stale watcher generations. - Keep the intended shared-listener behavior: one long-lived `ReverseConnectManager` can accept connections from multiple Servers on the same Client URL. - Expand `Docs/ReverseConnect.md` with listener lifetime, shared-manager usage, diagnostics, and specification references. ## Validation - `ReverseConnectManagerTests`: net10.0 and net48. - `ReverseConnectHostTests`: net10.0 and net48. - Ordered reverse-connect listener reuse integration tests: net10.0. ## Related Issues - Relates to #3985. ## 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. |
||
|
Marc Schier
|
12ebfba603 |
Add ISA95 JobControl and ManagedSession regression coverage (#3998)
## Summary - Adds the official ISA95 JobControl 2.0.0 NodeSet2 as a complete source-generator regression fixture. - Extends the lightweight generated-stack test stub with `NamespaceMetadataState`, required by the complete model. - Adds a deterministic ManagedSession cancellation regression proving that a started connect attempt is stopped. The ManagedSession cleanup implementation and Quickstarts utility coverage now come from `master`; this PR retains the additional deterministic cleanup regression. ## ISA95 fixture provenance and integrity The fixture is byte-for-byte the official file: - Source: `OPCFoundation/UA-Nodeset@e0b5d80cfff698f0276393e28df138a7fbc4b541` - Path: `ISA95-JOBCONTROL/opc.ua.isa95-jobcontrol.nodeset2.xml` - Git blob: `82f008cd126ade54e5f2c737bcc116621a8c3087` - SHA-256: `52327e736beb604c7253a5a5393d1ad525b9ecf35b71fbacb4f429cacb334064` The fixture test retains the OPC Foundation MIT header and verifies the 2.0.0 model/date, 258 nodes (including 134 variables), representative late variables, and that every `ns=1` reference target resolves locally. ## Validation - ManagedSession tests: 17 passing on net10.0; 15 passing and 2 framework-gated skips on net48. - Quickstarts server utility tests: 3 passing on net10.0 and net48. - ISA95 generator and fixture integrity regression: 5 passing on net10.0. |
||
|
Marc Schier
|
8538f531e7 | Update paths for the consolidated repository layout | ||
|
Marc Schier
|
7cec2d614d | Move repository files to the consolidated layout |