dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(commons): resolve Commons-005..007,009..012 — OPC UA parse status, TryConvert correctness, Result null guard, invariant formatting, doc refresh

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 22:04:21 -04:00
parent 746ab90444
commit c07f524ca4
12 changed files with 602 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files

110
code-reviews/Commons/findings.md
View File

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 8 |
| Open findings | 1 |
## Summary
@@ -229,7 +229,7 @@ SiteRuntime all build clean against the change. Regression tests added in
|--|--|
| Severity | Low |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs:25-51` |
**Description**
@@ -252,7 +252,19 @@ empty form. Update the XML doc to describe the failure branch.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending) — confirmed the over-reporting and silent data-loss
branch. `Deserialize` now returns an `OpcUaConfigParseResult` (a `readonly record struct`)
carrying an explicit `OpcUaConfigParseStatus` (`Typed` / `Legacy` / `Malformed`); genuinely
unparseable input is classified `Malformed` with `IsLegacy == false` instead of being
mislabelled as a recoverable legacy row. The struct keeps a custom two-element
`Deconstruct(out Config, out bool isLegacy)` so the existing SiteRuntime caller
(`var (config, _) = ...`) is unaffected — verified by a green SiteRuntime build. The XML
doc now describes all three outcomes. Throwing was rejected because the sole consumer
(`DeploymentManagerActor.FlattenConnectionConfig`) does not wrap the call and is
out-of-scope to change. Regression tests added in `OpcUaEndpointConfigSerializerTests`
(`Deserialize_Malformed_ReportsMalformedNotLegacy`, `Deserialize_LegacyParsed_StatusIsLegacy`,
`Deserialize_ObjectWithoutEndpointUrl_ParsesAsLegacy`,
`Deserialize_TwoElementDeconstruction_StillWorks`).
### Commons-006 — `DynamicJsonElement.TryConvert` reports success for unconvertible target types
@@ -260,7 +272,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/DynamicJsonElement.cs:47-51`, `:66-76` |
**Description**
@@ -282,7 +294,16 @@ For the `object` target, return the element itself (or `Wrap(_element)`) rather
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending) — confirmed by a direct `TryConvert` regression test
that the original code returned `(result: null, true)` for an `object` conversion of a
present value. `TryConvert` now handles the `object` target explicitly: it returns
`Wrap(_element)` (the unwrapped scalar, or the wrapper for objects/arrays) and yields
`null` only for a genuine `JsonValueKind.Null`. Non-`object` targets keep the correct
behavior — a `null` from `ConvertTo` now reports `false` (the dead `|| typeof(object)`
clause is removed) so the binder surfaces a real binding error for unconvertible pairs.
Regression tests added in `DynamicJsonElementTests` (`TryConvert_ObjectTarget_OnPresentValue_ReturnsNonNull`,
`TryConvert_ObjectTarget_OnJsonNull_ReturnsNull`,
`TryConvert_NonObjectTarget_OnUnconvertibleValue_ReportsFailure`).
### Commons-007 — Several Commons types carry non-trivial logic, stretching REQ-COM-6
@@ -290,7 +311,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/ScriptParameters.cs`, `src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs`, `src/ScadaLink.Commons/Validators/OpcUaEndpointConfigValidator.cs`, `src/ScadaLink.Commons/Types/StaleTagMonitor.cs`, `src/ScadaLink.Commons/Types/ScriptArgs.cs` |
**Description**
@@ -317,7 +338,18 @@ them. Tighten the architectural test if the rule is meant to be enforced.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending) — design-document decision. This is drift, not a
defect: the helpers are pure, dependency-light (no Akka/ASP.NET/EF, no I/O), and shared
across components, so the intentional choice is to keep them in Commons rather than
duplicate the logic. REQ-COM-6 in `docs/requirements/Component-Commons.md` was amended
with an explicit "pure-helper carve-out" — stateless, side-effect-free helpers that only
transform/format/parse/validate Commons' own data types are now permitted, with the
qualifying conditions (no I/O dependency, no shared mutable cross-call state, no
orchestration) and the current examples (`Result<T>`, `ScriptParameters`, `ScriptArgs`,
`ValueFormatter`, `DynamicJsonElement`, `StaleTagMonitor`, `OpcUaEndpointConfigSerializer`,
`OpcUaEndpointConfigValidator`) spelled out. No regression test — this is a documentation
policy decision. The `ArchitecturalConstraintTests` heuristic was left as-is since the
carve-out makes these types compliant by design.
### Commons-008 — `SetConnectionBindingsCommand` uses `ValueTuple` in a wire message contract
@@ -347,7 +379,18 @@ Replace the tuple with a small named record, e.g.
**Resolution**
_Unresolved._
_Open — deferred to a coordinated multi-module change._ The finding is confirmed valid:
the `ValueTuple` is the only positional/tuple element in `Messages/` and is unfriendly to
REQ-COM-5a additive evolution. However, `SetConnectionBindingsCommand.Bindings` is not a
Commons-internal type — its `IReadOnlyList<(string,int)>` shape is part of a cross-module
contract consumed by `ScadaLink.CLI` (`InstanceCommands.TryParseBindings` builds a
`List<(string,int)>` and passes it to the constructor), `ScadaLink.ManagementService`
(`ManagementActor` forwards `cmd.Bindings`), and `ScadaLink.TemplateEngine`
(`InstanceService.SetConnectionBindingsAsync` takes an `IReadOnlyList<(string AttributeName,
int DataConnectionId)>` parameter). Introducing a `ConnectionBinding` record therefore
requires editing those three modules in lock-step, which is outside the scope of this
Commons-only review pass (the constraint forbids touching other modules' source). Left
Open and flagged for a follow-up cross-module change; the fix itself is unambiguous.
### Commons-009 — `Component-Commons.md` is stale relative to the actual file set
@@ -355,7 +398,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Documentation & comments |
| Status | Open |
| Status | Resolved |
| Location | `docs/requirements/Component-Commons.md:61-198` |
**Description**
@@ -385,7 +428,18 @@ folders.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending) — `docs/requirements/Component-Commons.md` was
refreshed against the actual file set. REQ-COM-3 now lists `TemplateFolder`,
`InstanceAlarmOverride` and `DeployedConfigSnapshot` and drops the non-existent
`SiteDataConnectionAssignment` (no such entity in the code). REQ-COM-4 adds
`ISiteRepository` (eight repositories). REQ-COM-4a documents `IDatabaseGateway`,
`IExternalSystemClient`, `IInstanceLocator` and `INotificationDeliveryService` alongside
`IAuditService`, and corrects the `IAuditService` owner to the Configuration Database
component. The REQ-COM-5b folder tree was rewritten to include the previously-absent
`Types/DataConnections`, `Types/Flattening`, `Types/Scripts`, the helper types under
`Types/`, the `Messages/DataConnection|Instance|Integration|InboundApi|RemoteQuery|Management`
namespaces, and the `Serialization/` and `Validators/` folders. Documentation-only; no
regression test.
### Commons-010 — Behavior-bearing Commons types have no unit tests
@@ -393,7 +447,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Testing coverage |
| Status | Open |
| Status | Resolved |
| Location | `tests/ScadaLink.Commons.Tests/` |
**Description**
@@ -422,7 +476,16 @@ round-trip.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending) — all the listed types now have unit tests.
`DynamicJsonElement`, `ManagementCommandRegistry`, `Result<T>` and the OPC UA serializer
were already covered by tests added in earlier finding fixes (Commons-002/004) and this
pass extended them (Commons-005/006). New focused test files added this pass:
`ValueFormatterTests` (scalar/collection/null formatting plus the Commons-012
culture-invariance regression), `ScriptArgsTests` (dictionary / read-only-dictionary /
non-generic-`IDictionary` / anonymous-object / primitive-rejection paths of
`ScriptArgs.Normalize`), and `FlatteningAndScriptScopeTests` (`ConfigurationDiff.HasChanges`
and `ScriptScope.HasParent` computed-property logic). The module suite is green at 224
tests (up from 196).
### Commons-011 — `Result<T>.Failure` accepts a null error string
@@ -430,7 +493,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/Result.cs:15-20`, `:30-32`, `:36` |
**Description**
@@ -449,7 +512,13 @@ Throw `ArgumentNullException` (or `ArgumentException` for empty/whitespace) in
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending) — the private failure constructor now calls
`ArgumentException.ThrowIfNullOrWhiteSpace(error)`, so a failed `Result<T>` always carries
a usable message (`ArgumentNullException` for null, `ArgumentException` for empty/
whitespace). The `Failure` factory XML doc records both exceptions. A repo-wide scan of
`.Failure(...)` call sites confirmed every caller passes a non-empty literal or
interpolated string, so no consumer is broken. Regression tests added in `ResultTests`
(`Failure_WithNullError_ShouldThrow`, `Failure_WithBlankError_ShouldThrow`).
### Commons-012 — `ValueFormatter` uses current-culture formatting without documenting it
@@ -457,7 +526,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Documentation & comments |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/ValueFormatter.cs:20-27` |
**Description**
@@ -479,4 +548,13 @@ overload). Either way, document the culture behavior on the method.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending) — confirmed `ValueFormatter` is *not* UI-only: its
sole consumer is `StreamRelayActor`, which formats attribute values into outbound gRPC
`SiteStreamEvent` messages (a wire context), so culture-dependent output was a latent
bug. `FormatDisplayValue` now formats `IFormattable` scalars and collection elements with
`IFormattable.ToString(null, CultureInfo.InvariantCulture)`; a new `FormatElement` helper
applies the same invariant rule per collection element (previously elements went through
the parameterless `ToString()`). The XML doc gained a remarks block stating the
culture-invariant contract and why. Regression tests added in `ValueFormatterTests`
(`FormatDisplayValue_Double_UsesInvariantCulture_*`, `_DateTime_*`, `_CollectionOfDoubles_*`,
each pinned under `de-DE`).