mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-25 03:43:05 +08:00
fix/encoder-stack-depth-json-flake
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 romanettandClaude Opus 5.5
|
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> |
||
|
Marc SchierandCopilot
|
4e0b82af5f |
Add IServerContext and move server startup tasks onto it (#4193)
## Stacked on #4188
Base is `marcschier/server-seam-analysis`, not `master`.
`IServerInternal.cs` and
`ServerInternalData.cs` both diverge from master on that branch (chunk
4c removed the two
`object` locks), so branching off master would conflict on both files
*and* design
`IServerContext` against a stale interface. GitHub retargets the base to
`master`
automatically when #4188 merges. **Review #4188 first.**
Closes part of #4184.
## What this does
`IServerInternal` is a service locator: 56 members, 30-odd of which hand
out one of 20+
subsystems. A component that only needs to observe the server and act on
it has to take the
whole thing, and no signature tells a reader which parts a component
actually touches.
This adds `IServerContext` — the ambient view of a running server — and
moves
`IServerStartupTask` onto it.
```csharp
public interface IServerContext
{
ServerState CurrentState { get; }
ServerObjectState ServerObject { get; }
IServiceMessageContext MessageContext { get; }
ServerSystemContext DefaultSystemContext { get; }
ServerSystemContext CreateSystemContext(ISession session);
T? FindPredefinedNode<T>(NodeId nodeId) where T : NodeState;
IEnumerable<T> FindNodeManagers<T>() where T : class;
ValueTask ReportEventAsync(IFilterTarget e, CancellationToken ct = default);
ValueTask CloseSessionAsync(OperationContext context, NodeId sessionId,
bool deleteSubscriptions, CancellationToken ct = default);
ValueTask DeleteSubscriptionAsync(uint subscriptionId, CancellationToken ct = default);
void UpdateServerDiagnostics(Action<ServerDiagnosticsSummaryDataType> update);
}
```
Nothing on it returns a subsystem. A component that needs one takes it
as a constructor
dependency, which states the requirement instead of hiding it behind an
ambient handle.
`IServerInternal : IServerContext`, so every existing handle already
satisfies the new seam
and consumers migrate one at a time.
## The member set was measured, not guessed
Every member of `IServerInternal` was counted by read site across `src`,
`samples` and
`tests`, split by area. `Ext` is non-test production consumers, which is
what decides
whether something belongs on an ambient interface.
| Member | InServer | OtherSrc | Sample | Test | **Ext** | On
`ISystemContext`? |
|---|---:|---:|---:|---:|---:|---|
| `NamespaceUris` | 63 | 50 | 34 | 131 | **84** | yes |
| `Telemetry` | 58 | 23 | 35 | 5 | **58** | yes |
| `MessageContext` | 2 | 8 | 7 | 11 | **15** | no |
| `TypeTree` | 42 | 3 | 11 | 10 | **14** | yes, as `TypeTable` |
| `NodeManager` | 66 | 7 | 6 | 74 | **13** | no |
| `DefaultSystemContext` | 21 | 4 | 1 | 7 | **5** | no |
| `IdentityRegistry` | 1 | 4 | 1 | 1 | **5** | no |
| `SubscriptionManager` | 23 | 0 | 0 | 17 | **0** | no |
| `SessionManager` | 21 | 0 | 0 | 15 | **0** | no |
| `RequestManager` | 10 | 0 | 0 | 23 | **0** | no |
| `AggregateManager` | 12 | 0 | 0 | 2 | **0** | no |
| `ModellingRulesManager`, `ConformanceUnitsManager`,
`DiagnosticsEnabled` | 0 | 0 | 0 | 0 | **0** | no |
Two things fall out. The highest-traffic members are already carried by
`ISystemContext`,
which `ServerSystemContext` copies off the server at construction — so
most of those 278
`NamespaceUris` reads are callers reaching through the server for
something they already
hold. And the subsystems with zero external demand are exactly the ones
that should be
injected rather than published.
The check that matters: the eight `IServerStartupTask` implementations
in
`Opc.Ua.Redundancy.Server` are the heaviest real consumer of the ambient
handle, and
between them they touch **eight distinct members**. That is the shape
the interface is cut
to.
## Commits
### `e4a912eb` Remove seven unused members from `IServerInternal`
The deletion test, applied before anything is designed against the
interface.
| Removed | Evidence |
|---|---|
| `CloseSession(OperationContext, NodeId, bool)` | zero call sites;
sync-over-async; already obsolete |
| `ReadServerDiagnostics<TResult>` | zero production sites — **added one
commit earlier in #4188 for symmetry and never called** |
| `ModellingRulesManager` | property never read; `ServerInternalData`
owns the lifetime |
| `ConformanceUnitsManager` | same |
| `DiagnosticsEnabled` | façade over
`IDiagnosticsNodeManager.DiagnosticsEnabled`, which is what callers use
|
| `Status` | one site, itself inside an obsolete method |
| `ServerDiagnostics` | only reachable through the diagnostics methods
that replaced it |
`EndpointAddresses` was kept — `Opc.Ua.Di.Tests` uses it to resolve an
endpoint URL. Thin,
but real.
The only build fallout was three lines in one test assembly.
`Opc.Ua.Redundancy.Server`,
`Opc.Ua.Redundancy.Kubernetes`, `Opc.Ua.Gds.Server.Common`,
`Opc.Ua.WotCon.Server`,
`Opc.Ua.PubSub.Server`, `Opc.Ua.Di.Server`, `ConsoleReferenceServer` and
`Quickstarts.Servers` all built untouched. That is the evidence the
members were dead.
### `ffcc80b9` Add `IServerContext`
Additive. `ServerInternalData` implements it; no caller changes.
### `e8c4e15f` Hand startup tasks `IServerContext`
Nine implementations, `OpcUaServerHostedService`, and the test call
sites.
### `1b4de97a` Drop the `IServerStartupTask` migration-guide section
Review feedback. `IServerStartupTask` does not exist in `master378`, so
it is a 2.0-only
concept and nobody migrating from 1.5.378 can own an implementation of
it. The **Removed
members on `IServerInternal`** section stays - those members *are* in
master378 - and is now
self-contained.
### `2ab7d689` Merge `marcschier/server-seam-analysis`
Picks up the base branch including its merge of master. Textually clean,
no conflicts.
### `6a650f12` Migrate `RobotIntentHostStartupTask` to `IServerContext`
The merge was textually clean but **semantically broken**: master's
Robot Intent feature
(#4165) added a tenth `IServerStartupTask` implementation written
against the old
`IServerInternal` signature, so it no longer satisfied the interface.
Its body walked
`server.NodeManager.AsyncNodeManagers` and type-tested each entry for
`RobotIntentNodeManager` - exactly the pattern `FindNodeManagers<T>()`
replaces - so it
migrated the same way as the other nine. `FindNodeManagers` also
searches the synchronous
registry and de-dupes by reference, so a manager registered on both
faces still starts its
intent controller hosts exactly once.
## Three findings from migrating real consumers
**`MessageContext` is not derivable.** The design had callers use
`DefaultSystemContext.AsMessageContext()`, on the reasoning that a
message context can be
built from a system context. It can't: that conversion carries the
namespace tables and the
factory but **not** `MaxStringLength`, `MaxArrayLength` or
`MaxByteStringLength`. Four
replication paths in `Opc.Ua.Redundancy.Server` would have silently
fallen back to default
decoding limits — a component quietly accepting more than the server is
configured to. It
is a real member, and the interface documents why.
**`new`-shadowed members split the mock surface.** `IServerInternal`
first redeclared
`DefaultSystemContext` with a narrower type and `CurrentState` with a
setter. That
compiles, but a shadowed member is a *second interface slot*: production
code calling
through `IServerContext` read a slot that every existing
`Mock<IServerInternal>` leaves
unstubbed. Seventeen tests went from passing to `NullReferenceException`
**while still
compiling**. The narrower typing bought no encapsulation anyway — the
object really is a
`ServerSystemContext` and any caller can cast — so both shadows are
gone. `CurrentState` is
read-only on the ambient interface and the one site that assigns it uses
the concrete type,
as `GetStatus` already does.
**`IServiceProvider` is deliberately *not* on `OnServerStartedAsync`.**
The plan sketched
it. All nine implementations already receive their dependencies through
their constructors
from the builder extensions, so a container on the callback would have
replaced one service
locator with another.
## `FindNodeManagers<T>`
Two tasks walked `server.NodeManager.NodeManagers` and type-tested each
entry for
`ILocalAddressSpaceSource`; two more reached for a specific manager to
cast it to a
capability. Asking for the capability says what the caller wants; asking
for the registry
says how to find it.
```csharp
// before
foreach (INodeManager nodeManager in server.NodeManager.NodeManagers)
{
if (nodeManager is not ILocalAddressSpaceSource source) { continue; }
...
}
// after
foreach (ILocalAddressSpaceSource source in server.FindNodeManagers<ILocalAddressSpaceSource>())
{
...
}
```
It searches both the asynchronous and synchronous registries:
`ToSyncNodeManager()` returns
`this` when a manager already implements `INodeManager`, so the two
lists overlap and a
capability may sit on either face.
## Tests
Mock harnesses are **wired, not stubbed** — they return the node
managers and contexts the
tests already build, so the tests keep asserting against real behaviour
rather than against
a mock that returns whatever is convenient. Four new tests cover
`CreateSystemContext`
(identity/locale propagation, null guard), the explicit
`DefaultSystemContext` identity,
and `FindPredefinedNode` before the address space is bound.
| Suite | net10.0 | net48 |
|---|---|---|
| `Opc.Ua.Redundancy.Server.Tests` | 573 passed | 573 passed |
| `Opc.Ua.Redundancy.Kubernetes.Tests` | 97 passed | n/a (net8/net10
only) |
| `Opc.Ua.Server.Tests` | 4028 passed, 1 failed | 225 passed, 1 failed
(filtered) |
The single failure is
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`,
a local certificate-store problem. Verified it reproduces unchanged at
HEAD with these
changes stashed — it is not caused by this PR.
All twelve dependent assemblies and samples build clean on both TFMs,
zero warnings.
### Re-verified after the base merge (`6a650f12`)
`Opc.Ua.Server`, `Redundancy.Server`, `Redundancy.Kubernetes`,
`Robotics.Server`,
`WotCon.Server`, `Di.Server`, `PubSub.Server`, `Gds.Server.Common` and
`XRegistry.Server` all build clean on net10.0. `Opc.Ua.Robotics.Tests`
491 passed,
`Opc.Ua.Redundancy.Server.Tests` 573 passed,
`Opc.Ua.Redundancy.Kubernetes.Tests` 97 passed.
### CI status: 119 passed, 3 failed - none caused by this PR
| Failing job | Why it is not this PR |
|---|---|
| `SourceGeneration_Core_Tests_windows` (net48) |
`GenerateAndCompileDesignFileAsync(..., CSharp11, ...)` fails with
`CS9058: Feature 'Lock object' is not available in C# 11.0` because
generated `Opc.Ua.NodeStates.g.cs` emits `System.Threading.Lock`.
**Identical failure on base #4188 (build 16737)**, which contains none
of these commits. This PR touches no source-generation file. |
| `SourceGeneration_Core_Tests_windows` (net10.0) | Same root cause;
also failing on base #4188. |
| `PubSub_Eth_Tests_windows` (net48) | Agent DNS outage:
`NuGetToolInstaller` died with `ERR:getaddrinfo ENOTFOUND
dist.nuget.org`. The job aborted before restore/build, so **the test
step never executed**. Infra flake; needs a re-run, not a code change. |
Both `SourceGeneration.Core` failures belong to the base branch and
should be fixed there
rather than papered over here.
## Correction to the recorded plan
Chunk 7 (the frozen bind phase) was recorded as the largest mechanical
footprint remaining
and the one most likely to stall. It isn't: **each of the 12 `Set*`
mutators has exactly one
call site**, in the `StandardServer` wiring, plus one sample. It is a
~13 call-site change.
The 268-file figure that produced that assessment counted files
*referencing*
`IServerInternal`, which chunk 7 never has to touch, because the
interface survives and only
the mutators move off it.
## Docs
`docs/MigrationGuide.md` gains one section: the removed
`IServerInternal` members with their
replacements, plus the note that `MessageContext`,
`DefaultSystemContext`, `CurrentState`,
`ServerObject`, `ReportEventAsync`, `CloseSessionAsync`,
`DeleteSubscriptionAsync` and
`UpdateServerDiagnostics` moved *down* to `IServerContext` and stay
reachable through
`IServerInternal` unchanged, so no call site has to move.
An earlier revision also documented the `IServerStartupTask` signature
change there. That
was dropped after review (
|
||
|
Marc Schier
|
7cec2d614d | Move repository files to the consolidated layout |