diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index ad5e1f0..c1b33a2 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -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`, `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` 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.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` 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`). diff --git a/docs/requirements/Component-Commons.md b/docs/requirements/Component-Commons.md index 22e7905..4f3967c 100644 --- a/docs/requirements/Component-Commons.md +++ b/docs/requirements/Component-Commons.md @@ -60,26 +60,28 @@ Commons must define persistence-ignorant POCO entity classes for all configurati Entity classes are organized by domain area: -- **Template & Modeling**: `Template`, `TemplateAttribute`, `TemplateAlarm`, `TemplateScript`, `TemplateComposition`, `Instance`, `InstanceAttributeOverride`, `InstanceConnectionBinding`, `Area`. +- **Template & Modeling**: `Template`, `TemplateAttribute`, `TemplateAlarm`, `TemplateScript`, `TemplateComposition`, `TemplateFolder`. +- **Instances**: `Instance`, `InstanceAttributeOverride`, `InstanceConnectionBinding`, `InstanceAlarmOverride`, `Area`. - **Shared Scripts**: `SharedScript`. -- **Sites & Data Connections**: `Site`, `DataConnection`, `SiteDataConnectionAssignment`. +- **Sites & Data Connections**: `Site`, `DataConnection`. - **External Systems & Database Connections**: `ExternalSystemDefinition`, `ExternalSystemMethod`, `DatabaseConnectionDefinition`. - **Notifications**: `NotificationList`, `NotificationRecipient`, `SmtpConfiguration`. - **Inbound API**: `ApiKey`, `ApiMethod`. - **Security**: `LdapGroupMapping`, `SiteScopeRule`. -- **Deployment**: `DeploymentRecord`, `SystemArtifactDeploymentRecord`. +- **Deployment**: `DeploymentRecord`, `SystemArtifactDeploymentRecord`, `DeployedConfigSnapshot`. - **Audit**: `AuditLogEntry`. ### REQ-COM-4: Per-Component Repository Interfaces Commons must define repository interfaces that consuming components use for data access. Each interface is tailored to the data needs of its consuming component: -- `ITemplateEngineRepository` — Templates, attributes, alarms, scripts, compositions, instances, overrides, connection bindings, areas. +- `ITemplateEngineRepository` — Templates, attributes, alarms, scripts, compositions, template folders, instances, overrides, alarm overrides, connection bindings, areas. - `IDeploymentManagerRepository` — Deployment records, deployed configuration snapshots, system-wide artifact deployment records. - `ISecurityRepository` — LDAP group mappings, site scoping rules. - `IInboundApiRepository` — API keys, API method definitions. - `IExternalSystemRepository` — External system definitions, method definitions, database connection definitions. - `INotificationRepository` — Notification lists, recipients, SMTP configuration. +- `ISiteRepository` — Sites, data connections, and their site assignments. - `ICentralUiRepository` — Read-oriented queries spanning multiple domain areas for display purposes. All repository interfaces must: @@ -93,7 +95,13 @@ Implementations of these interfaces are owned by the Configuration Database comp Commons must define service interfaces for cross-cutting concerns that multiple components consume: -- **`IAuditService`**: Provides a single method for components to log audit entries: `LogAsync(user, action, entityType, entityId, entityName, afterState)`. The implementation (owned by the Audit Logging component) serializes the state as JSON and adds the audit entry to the current unit-of-work transaction. Defined in Commons so any central component can call it without depending on the Audit Logging component directly. +- **`IAuditService`**: Provides a single method for components to log audit entries: `LogAsync(user, action, entityType, entityId, entityName, afterState)`. The implementation (owned by the Configuration Database component) serializes the state as JSON and adds the audit entry to the current unit-of-work transaction. Defined in Commons so any central component can call it without depending on the Configuration Database component directly. +- **`IDatabaseGateway`**: Provides script-facing ADO.NET database access via named database connections. Implemented by the External System Gateway, consumed by the Site Runtime's script runtime context. +- **`IExternalSystemClient`**: Provides script-facing invocation of external system HTTP APIs (synchronous `Call` and store-and-forward `CachedCall`). Implemented by the External System Gateway, consumed by the script runtime context. +- **`IInstanceLocator`**: Resolves an instance unique name to its site identifier. Used by the Inbound API's `Route.To()` to determine the destination site. +- **`INotificationDeliveryService`**: Sends notifications to a named notification list, routing transient failures to store-and-forward. Implemented by the Notification Service, consumed by the script runtime context. + +These interfaces are defined in Commons so that consuming components depend only on the abstraction, not on the implementing component. ### REQ-COM-5: Cross-Component Message Contracts @@ -126,20 +134,23 @@ All types in Commons are organized by **category** and **domain area** using a c ``` ScadaLink.Commons/ ├── Types/ # REQ-COM-1: Shared data types -│ ├── DataType.cs -│ ├── RetryPolicy.cs │ ├── Result.cs -│ └── Enums/ -│ ├── InstanceState.cs -│ ├── DeploymentStatus.cs -│ ├── AlarmState.cs -│ ├── AlarmTriggerType.cs -│ └── ConnectionHealth.cs +│ ├── RetryPolicy.cs +│ ├── ScriptArgs.cs # script-call parameter normalization helper +│ ├── ScriptParameters.cs # typed script-parameter access helper +│ ├── StaleTagMonitor.cs # heartbeat staleness watchdog +│ ├── ValueFormatter.cs # culture-invariant value-to-string helper +│ ├── DynamicJsonElement.cs # dynamic JSON wrapper for scripts +│ ├── Enums/ # InstanceState, DeploymentStatus, AlarmState, +│ │ # AlarmLevel, AlarmTriggerType, ConnectionHealth, +│ │ # DataType, StoreAndForwardCategory, +│ │ # StoreAndForwardMessageStatus +│ ├── DataConnections/ # OPC UA endpoint config value objects + enums +│ ├── Flattening/ # FlattenedConfiguration, ConfigurationDiff, +│ │ # DeploymentPackage, ValidationResult +│ └── Scripts/ # AlarmContext, ScriptScope ├── Interfaces/ # Shared interfaces by concern -│ ├── Protocol/ # REQ-COM-2: Protocol abstraction -│ │ ├── IDataConnection.cs -│ │ ├── TagValue.cs -│ │ └── SubscriptionCallback.cs +│ ├── Protocol/ # REQ-COM-2: Protocol abstraction (IDataConnection, etc.) │ ├── Repositories/ # REQ-COM-4: Per-component repository interfaces │ │ ├── ITemplateEngineRepository.cs │ │ ├── IDeploymentManagerRepository.cs @@ -147,55 +158,46 @@ ScadaLink.Commons/ │ │ ├── IInboundApiRepository.cs │ │ ├── IExternalSystemRepository.cs │ │ ├── INotificationRepository.cs +│ │ ├── ISiteRepository.cs │ │ └── ICentralUiRepository.cs │ └── Services/ # REQ-COM-4a: Cross-cutting service interfaces -│ └── IAuditService.cs +│ ├── IAuditService.cs +│ ├── IDatabaseGateway.cs +│ ├── IExternalSystemClient.cs +│ ├── IInstanceLocator.cs +│ └── INotificationDeliveryService.cs ├── Entities/ # REQ-COM-3: Domain entity POCOs, by domain area -│ ├── Templates/ -│ │ ├── Template.cs -│ │ ├── TemplateAttribute.cs -│ │ ├── TemplateAlarm.cs -│ │ ├── TemplateScript.cs -│ │ └── TemplateComposition.cs -│ ├── Instances/ -│ │ ├── Instance.cs -│ │ ├── InstanceAttributeOverride.cs -│ │ ├── InstanceConnectionBinding.cs -│ │ └── Area.cs -│ ├── Sites/ -│ │ ├── Site.cs -│ │ ├── DataConnection.cs -│ │ └── SiteDataConnectionAssignment.cs -│ ├── ExternalSystems/ -│ │ ├── ExternalSystemDefinition.cs -│ │ ├── ExternalSystemMethod.cs -│ │ └── DatabaseConnectionDefinition.cs -│ ├── Notifications/ -│ │ ├── NotificationList.cs -│ │ ├── NotificationRecipient.cs -│ │ └── SmtpConfiguration.cs -│ ├── InboundApi/ -│ │ ├── ApiKey.cs -│ │ └── ApiMethod.cs -│ ├── Security/ -│ │ ├── LdapGroupMapping.cs -│ │ └── SiteScopeRule.cs +│ ├── Templates/ # Template, TemplateAttribute, TemplateAlarm, +│ │ # TemplateScript, TemplateComposition, TemplateFolder +│ ├── Instances/ # Instance, InstanceAttributeOverride, +│ │ # InstanceConnectionBinding, InstanceAlarmOverride, Area +│ ├── Sites/ # Site, DataConnection +│ ├── ExternalSystems/ # ExternalSystemDefinition, ExternalSystemMethod, +│ │ # DatabaseConnectionDefinition +│ ├── Notifications/ # NotificationList, NotificationRecipient, SmtpConfiguration +│ ├── InboundApi/ # ApiKey, ApiMethod +│ ├── Security/ # LdapGroupMapping, SiteScopeRule +│ ├── Deployment/ # DeploymentRecord, SystemArtifactDeploymentRecord, +│ │ # DeployedConfigSnapshot +│ ├── Scripts/ # SharedScript +│ └── Audit/ # AuditLogEntry +├── Messages/ # REQ-COM-5: Cross-component message contracts, by concern │ ├── Deployment/ -│ │ ├── DeploymentRecord.cs -│ │ └── SystemArtifactDeploymentRecord.cs -│ ├── Scripts/ -│ │ └── SharedScript.cs -│ └── Audit/ -│ └── AuditLogEntry.cs -└── Messages/ # REQ-COM-5: Cross-component message contracts, by concern - ├── Deployment/ - ├── Lifecycle/ - ├── Health/ - ├── Communication/ - ├── Streaming/ - ├── DebugView/ - ├── ScriptExecution/ - └── Artifacts/ +│ ├── Lifecycle/ +│ ├── Health/ +│ ├── Communication/ +│ ├── Streaming/ +│ ├── DebugView/ +│ ├── ScriptExecution/ +│ ├── Artifacts/ +│ ├── DataConnection/ # data-connection subscribe/write/health messages +│ ├── Instance/ # attribute get/set request/command messages +│ ├── Integration/ # external-integration call request/response +│ ├── InboundApi/ # Route.To() request messages +│ ├── RemoteQuery/ # event-log and parked-message query messages +│ └── Management/ # HTTP/ClusterClient management commands + registry +├── Serialization/ # OpcUaEndpointConfigSerializer (typed↔legacy JSON) +└── Validators/ # OpcUaEndpointConfigValidator ``` **Naming rules**: @@ -205,7 +207,7 @@ ScadaLink.Commons/ - Message contracts are named as commands, events, or responses: `DeployInstanceCommand`, `DeploymentStatusResponse`, `AttributeValueChanged`. - Enums use singular names: `AlarmState`, not `AlarmStates`. -### REQ-COM-6: No Business Logic +### REQ-COM-6: No Business Logic; Pure Helpers Permitted Commons must contain only: @@ -213,8 +215,17 @@ Commons must contain only: - Interfaces - Enums - Constants +- **Pure, stateless helpers** — see the carve-out below. -It must **not** contain any business logic, service implementations, actor definitions, or orchestration code. Any method bodies must be limited to trivial data-access logic (e.g., factory methods, validation of invariants in constructors). +It must **not** contain any business logic that orchestrates other components, service implementations that perform I/O (database, network, file system), actor definitions, or orchestration code. + +**Pure-helper carve-out.** Commons *may* contain stateless, side-effect-free helper types whose behavior is confined to transforming, formatting, parsing, or validating the data types Commons already defines, provided they: + +- have no dependency on Akka.NET, ASP.NET Core, EF Core, or any I/O surface (consistent with REQ-COM-7); +- hold no shared mutable state across calls (a self-contained instance helper such as `StaleTagMonitor`, which owns only its own timer, is acceptable); +- do not call into other components or perform orchestration. + +Examples currently in Commons that fall under this carve-out: `Result`, `ScriptParameters` and `ScriptArgs` (script-parameter shaping), `ValueFormatter` (value-to-string formatting), `DynamicJsonElement` (dynamic JSON access), `StaleTagMonitor` (a self-contained heartbeat watchdog), `OpcUaEndpointConfigSerializer` (typed↔legacy JSON conversion of a Commons value object) and `OpcUaEndpointConfigValidator` (rule checks over a Commons value object). These are intentionally placed in Commons so every consuming component shares one implementation rather than duplicating the logic. Anything that would require an I/O dependency, mutable cross-call state, or knowledge of another component's behavior does **not** qualify and must live in the owning component. ### REQ-COM-7: Minimal Dependencies diff --git a/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs b/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs index d281bd1..b5cdd0f 100644 --- a/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs +++ b/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs @@ -4,11 +4,73 @@ using ScadaLink.Commons.Types.DataConnections; namespace ScadaLink.Commons.Serialization; +/// +/// Outcome classification for . +/// +public enum OpcUaConfigParseStatus +{ + /// The stored JSON parsed cleanly as the current typed shape. + Typed, + + /// + /// The stored JSON parsed as the legacy flat string-dict shape. The returned + /// config is usable; the caller may prompt the user to re-save in the new shape. + /// + Legacy, + + /// + /// The stored JSON could not be parsed at all (genuinely malformed). The returned + /// config is an empty default and the original string was lost — the caller should + /// surface an error rather than presenting the empty config as the user's data. + /// + Malformed +} + +/// +/// Result of . Carries the parsed +/// config plus an explicit distinguishing a recoverable legacy row +/// from genuinely unparseable input. Deconstructs into (Config, IsLegacy) for +/// backward compatibility with callers that only need those two values. +/// +public readonly record struct OpcUaConfigParseResult +{ + public OpcUaConfigParseResult(OpcUaEndpointConfig config, OpcUaConfigParseStatus status) + { + Config = config; + Status = status; + } + + /// The parsed config (an empty default when is Malformed). + public OpcUaEndpointConfig Config { get; } + + /// Classification of the parse outcome. + public OpcUaConfigParseStatus Status { get; } + + /// True when the source parsed as the legacy flat-dict shape. + public bool IsLegacy => Status == OpcUaConfigParseStatus.Legacy; + + /// True when the source could not be parsed at all. + public bool IsMalformed => Status == OpcUaConfigParseStatus.Malformed; + + /// + /// Two-element deconstruction kept for backward compatibility. Note that + /// IsLegacy is false for both + /// and ; callers that need to tell those + /// apart should read directly. + /// + public void Deconstruct(out OpcUaEndpointConfig config, out bool isLegacy) + { + config = Config; + isLegacy = IsLegacy; + } +} + /// /// Serializes to/from the typed nested JSON /// shape stored in DataConnection.PrimaryConfiguration / BackupConfiguration. /// On read, falls back to the legacy flat string-dict shape for pre-refactor rows -/// and returns IsLegacy=true so the form can prompt the user to re-save. +/// and reports so the form can prompt the +/// user to re-save. /// public static class OpcUaEndpointConfigSerializer { @@ -22,10 +84,25 @@ public static class OpcUaEndpointConfigSerializer public static string Serialize(OpcUaEndpointConfig config) => JsonSerializer.Serialize(config, JsonOpts); - public static (OpcUaEndpointConfig Config, bool IsLegacy) Deserialize(string? json) + /// + /// Parses stored OPC UA endpoint JSON. Tries the current typed shape first, then the + /// legacy flat string-dict shape. The returned + /// distinguishes three outcomes: + /// + /// — clean parse of the current shape + /// (also returned for null/blank input, which yields a default config). + /// — parsed as a legacy flat object; + /// the config is usable and the caller may prompt a re-save. + /// — the input is genuinely + /// unparseable JSON. The config is an empty default and the original string is lost; + /// the caller should surface an error rather than treating the empty config as the + /// user's saved data. + /// + /// + public static OpcUaConfigParseResult Deserialize(string? json) { if (string.IsNullOrWhiteSpace(json)) - return (new OpcUaEndpointConfig(), false); + return new OpcUaConfigParseResult(new OpcUaEndpointConfig(), OpcUaConfigParseStatus.Typed); try { @@ -35,18 +112,21 @@ public static class OpcUaEndpointConfigSerializer { var typed = JsonSerializer.Deserialize(json, JsonOpts); if (typed != null) - return (typed, false); + return new OpcUaConfigParseResult(typed, OpcUaConfigParseStatus.Typed); } } catch (JsonException) { /* fall through to legacy */ } try { - return (LoadLegacy(json!), IsLegacy: true); + return new OpcUaConfigParseResult(LoadLegacy(json!), OpcUaConfigParseStatus.Legacy); } catch (JsonException) { - return (new OpcUaEndpointConfig(), IsLegacy: true); + // Genuinely malformed input: not a recoverable legacy row. Report Malformed + // (not Legacy) so the caller can surface an error instead of presenting an + // empty config as if it were the user's saved configuration. + return new OpcUaConfigParseResult(new OpcUaEndpointConfig(), OpcUaConfigParseStatus.Malformed); } } diff --git a/src/ScadaLink.Commons/Types/DynamicJsonElement.cs b/src/ScadaLink.Commons/Types/DynamicJsonElement.cs index 1124178..27c939a 100644 --- a/src/ScadaLink.Commons/Types/DynamicJsonElement.cs +++ b/src/ScadaLink.Commons/Types/DynamicJsonElement.cs @@ -55,8 +55,19 @@ public class DynamicJsonElement : DynamicObject public override bool TryConvert(ConvertBinder binder, out object? result) { + // Conversion to object (or dynamic): never null out a present value. Return the + // unwrapped value for scalars, this wrapper for objects/arrays, and null only + // when the element is genuinely JSON null. + if (binder.Type == typeof(object)) + { + result = _element.ValueKind == JsonValueKind.Null ? null : Wrap(_element); + return true; + } + result = ConvertTo(binder.Type); - return result != null || binder.Type == typeof(object); + // A non-object target with a null result means ConvertTo could not handle the + // element/type pair — report failure so the binder surfaces a binding error. + return result != null; } public override string ToString() diff --git a/src/ScadaLink.Commons/Types/Result.cs b/src/ScadaLink.Commons/Types/Result.cs index 791ef32..742351e 100644 --- a/src/ScadaLink.Commons/Types/Result.cs +++ b/src/ScadaLink.Commons/Types/Result.cs @@ -14,6 +14,9 @@ public sealed class Result private Result(string error) { + // A failed Result must always carry a usable message — Result is the + // system-wide error-handling type, and consumers log/display Error directly. + ArgumentException.ThrowIfNullOrWhiteSpace(error); _value = default; _error = error; IsSuccess = false; @@ -33,6 +36,11 @@ public sealed class Result public static Result Success(T value) => new(value); + /// + /// Creates a failed result carrying the given error message. + /// + /// is null. + /// is empty or whitespace. public static Result Failure(string error) => new(error); public TResult Match(Func onSuccess, Func onFailure) => diff --git a/src/ScadaLink.Commons/Types/ValueFormatter.cs b/src/ScadaLink.Commons/Types/ValueFormatter.cs index 9e70d4e..2f430c2 100644 --- a/src/ScadaLink.Commons/Types/ValueFormatter.cs +++ b/src/ScadaLink.Commons/Types/ValueFormatter.cs @@ -1,4 +1,5 @@ using System.Collections; +using System.Globalization; namespace ScadaLink.Commons.Types; @@ -9,21 +10,36 @@ namespace ScadaLink.Commons.Types; public static class ValueFormatter { /// - /// Formats a value for display as a string. Returns the value's natural - /// string representation for scalars, and comma-separated elements for - /// array/collection types. + /// Formats a value as a string. Returns the value's string representation for + /// scalars and comma-separated elements for array/collection types. /// + /// + /// Formatting is culture-invariant: + /// numbers and values render the same regardless of the + /// server/thread locale. This is required because the formatter feeds non-UI + /// contexts (gRPC stream events, logs, diff display) where locale-dependent + /// output (decimal separators, date order) would be inconsistent. + /// public static string FormatDisplayValue(object? value) { if (value is null) return ""; if (value is string s) return s; - if (value is IFormattable) return value.ToString() ?? ""; + if (value is IFormattable formattable) + return formattable.ToString(null, CultureInfo.InvariantCulture) ?? ""; if (value is IEnumerable enumerable) { - return string.Join(",", enumerable.Cast().Select(e => e?.ToString() ?? "")); + return string.Join(",", enumerable.Cast().Select(FormatElement)); } return value.ToString() ?? ""; } + + private static string FormatElement(object? element) => element switch + { + null => "", + string str => str, + IFormattable f => f.ToString(null, CultureInfo.InvariantCulture) ?? "", + _ => element.ToString() ?? "" + }; } diff --git a/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs b/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs index 04a90ba..61425a1 100644 --- a/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs +++ b/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs @@ -135,19 +135,47 @@ public class OpcUaEndpointConfigSerializerTests Assert.Equal(1000, config.PublishingIntervalMs); } + [Fact] + public void Deserialize_LegacyParsed_StatusIsLegacy() + { + var (_, isLegacy) = OpcUaEndpointConfigSerializer.Deserialize("""{"endpoint":"opc.tcp://x:4840"}"""); + Assert.True(isLegacy); + + var result = OpcUaEndpointConfigSerializer.Deserialize("""{"endpoint":"opc.tcp://x:4840"}"""); + Assert.Equal(OpcUaConfigParseStatus.Legacy, result.Status); + } + [Theory] [InlineData("not json at all")] [InlineData("[1,2,3]")] - [InlineData("{\"foo\":123}")] [InlineData("\"just a string\"")] - public void Deserialize_Malformed_ReturnsDefaultsAsLegacy(string input) + public void Deserialize_Malformed_ReportsMalformedNotLegacy(string input) { - var (config, isLegacy) = OpcUaEndpointConfigSerializer.Deserialize(input); + var result = OpcUaEndpointConfigSerializer.Deserialize(input); - Assert.True(isLegacy); - Assert.Equal("", config.EndpointUrl); - Assert.Equal(60000, config.SessionTimeoutMs); - Assert.Null(config.Heartbeat); + // Genuinely unparseable input must NOT be reported as a recoverable legacy row. + Assert.Equal(OpcUaConfigParseStatus.Malformed, result.Status); + Assert.False(result.IsLegacy); + Assert.Equal("", result.Config.EndpointUrl); + } + + [Fact] + public void Deserialize_ObjectWithoutEndpointUrl_ParsesAsLegacy() + { + // A flat object with unrecognized keys is still a parseable legacy row, not malformed. + var result = OpcUaEndpointConfigSerializer.Deserialize("{\"foo\":123}"); + Assert.Equal(OpcUaConfigParseStatus.Legacy, result.Status); + Assert.True(result.IsLegacy); + } + + [Fact] + public void Deserialize_TwoElementDeconstruction_StillWorks() + { + // Backward-compat: existing callers deconstruct into (Config, IsLegacy). + var (config, isLegacy) = OpcUaEndpointConfigSerializer.Deserialize( + """{"endpointUrl":"opc.tcp://x:4840"}"""); + Assert.False(isLegacy); + Assert.Equal("opc.tcp://x:4840", config.EndpointUrl); } [Fact] diff --git a/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs b/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs index f3e75ca..9c6731b 100644 --- a/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs +++ b/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs @@ -1,3 +1,4 @@ +using System.Dynamic; using System.Text.Json; using ScadaLink.Commons.Types; @@ -100,4 +101,47 @@ public class DynamicJsonElementTests Assert.Throws( () => { var _ = obj.doesNotExist; }); } + + // ── Commons-006 regression: TryConvert(object) must never null out a present value ── + + [Fact] + public void TryConvert_ObjectTarget_OnPresentValue_ReturnsNonNull() + { + // Directly exercise the DynamicObject.TryConvert contract for an `object` + // target: a present JSON object/array/string must not convert to null. + using var objDoc = JsonDocument.Parse("""{ "x": 1 }"""); + var objWrapper = new DynamicJsonElement(objDoc.RootElement); + var convBinder = (ConvertBinder)Microsoft.CSharp.RuntimeBinder.Binder.Convert( + Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags.None, typeof(object), typeof(DynamicJsonElementTests)); + + Assert.True(objWrapper.TryConvert(convBinder, out var result)); + Assert.NotNull(result); + } + + [Fact] + public void TryConvert_ObjectTarget_OnJsonNull_ReturnsNull() + { + // Only a genuinely null JSON value converts to a null object. + using var doc = JsonDocument.Parse("""{ "v": null }"""); + var nullWrapper = new DynamicJsonElement(doc.RootElement.GetProperty("v")); + var convBinder = (ConvertBinder)Microsoft.CSharp.RuntimeBinder.Binder.Convert( + Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags.None, typeof(object), typeof(DynamicJsonElementTests)); + + Assert.True(nullWrapper.TryConvert(convBinder, out var result)); + Assert.Null(result); + } + + [Fact] + public void TryConvert_NonObjectTarget_OnUnconvertibleValue_ReportsFailure() + { + // Requesting int from a JSON string is genuinely unconvertible: TryConvert + // must report false rather than a null success. + using var doc = JsonDocument.Parse("""{ "s": "not-a-number" }"""); + var strWrapper = new DynamicJsonElement(doc.RootElement.GetProperty("s")); + var convBinder = (ConvertBinder)Microsoft.CSharp.RuntimeBinder.Binder.Convert( + Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags.None, typeof(int), typeof(DynamicJsonElementTests)); + + Assert.False(strWrapper.TryConvert(convBinder, out var result)); + Assert.Null(result); + } } diff --git a/tests/ScadaLink.Commons.Tests/Types/FlatteningAndScriptScopeTests.cs b/tests/ScadaLink.Commons.Tests/Types/FlatteningAndScriptScopeTests.cs new file mode 100644 index 0000000..4a9aca4 --- /dev/null +++ b/tests/ScadaLink.Commons.Tests/Types/FlatteningAndScriptScopeTests.cs @@ -0,0 +1,50 @@ +using ScadaLink.Commons.Types.Flattening; +using ScadaLink.Commons.Types.Scripts; + +namespace ScadaLink.Commons.Tests.Types; + +/// +/// Commons-010: coverage for the small computed-property logic on +/// and . +/// +public class FlatteningAndScriptScopeTests +{ + [Fact] + public void ConfigurationDiff_NoChanges_HasChangesIsFalse() + { + var diff = new ConfigurationDiff { InstanceUniqueName = "inst-1" }; + + Assert.False(diff.HasChanges); + } + + [Fact] + public void ConfigurationDiff_WithAttributeChange_HasChangesIsTrue() + { + var diff = new ConfigurationDiff + { + InstanceUniqueName = "inst-1", + AlarmChanges = new[] + { + new DiffEntry { CanonicalName = "HiAlarm", ChangeType = DiffChangeType.Added } + } + }; + + Assert.True(diff.HasChanges); + } + + [Fact] + public void ScriptScope_Root_HasNoParent() + { + Assert.False(ScriptScope.Root.HasParent); + Assert.Null(ScriptScope.Root.ParentPath); + } + + [Fact] + public void ScriptScope_WithParentPath_HasParentIsTrue() + { + var scope = new ScriptScope("Pump1.Motor", "Pump1"); + + Assert.True(scope.HasParent); + Assert.Equal("Pump1", scope.ParentPath); + } +} diff --git a/tests/ScadaLink.Commons.Tests/Types/ResultTests.cs b/tests/ScadaLink.Commons.Tests/Types/ResultTests.cs index 17c286f..c33f7d9 100644 --- a/tests/ScadaLink.Commons.Tests/Types/ResultTests.cs +++ b/tests/ScadaLink.Commons.Tests/Types/ResultTests.cs @@ -72,4 +72,20 @@ public class ResultTests Assert.True(result.IsSuccess); Assert.Equal("hello", result.Value); } + + // ── Commons-011 regression: a failed Result must always carry a message ── + + [Fact] + public void Failure_WithNullError_ShouldThrow() + { + Assert.Throws(() => Result.Failure(null!)); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void Failure_WithBlankError_ShouldThrow(string error) + { + Assert.Throws(() => Result.Failure(error)); + } } diff --git a/tests/ScadaLink.Commons.Tests/Types/ScriptArgsTests.cs b/tests/ScadaLink.Commons.Tests/Types/ScriptArgsTests.cs new file mode 100644 index 0000000..2d74275 --- /dev/null +++ b/tests/ScadaLink.Commons.Tests/Types/ScriptArgsTests.cs @@ -0,0 +1,81 @@ +using System.Collections; +using ScadaLink.Commons.Types; + +namespace ScadaLink.Commons.Tests.Types; + +/// +/// Commons-010: coverage for — the script-call +/// parameter normalizer (dictionary / anonymous-object / primitive-rejection paths). +/// +public class ScriptArgsTests +{ + [Fact] + public void Normalize_Null_ReturnsNull() + { + Assert.Null(ScriptArgs.Normalize(null)); + } + + [Fact] + public void Normalize_ReadOnlyDictionary_ReturnedAsIs() + { + IReadOnlyDictionary input = + new Dictionary { ["a"] = 1 }; + + var result = ScriptArgs.Normalize(input); + + Assert.Same(input, result); + } + + [Fact] + public void Normalize_PlainDictionary_ReturnedAsIs() + { + // Dictionary implements IReadOnlyDictionary, so it matches the + // first switch arm and is returned by reference (no defensive copy). + var input = new Dictionary { ["a"] = 1 }; + + var result = ScriptArgs.Normalize(input); + + Assert.Same(input, result); + Assert.Equal(1, result!["a"]); + } + + [Fact] + public void Normalize_NonGenericDictionary_KeysStringified() + { + IDictionary raw = new Hashtable { [42] = "answer" }; + + var result = ScriptArgs.Normalize(raw); + + Assert.Equal("answer", result!["42"]); + } + + [Fact] + public void Normalize_AnonymousObject_PropertiesBecomeEntries() + { + var result = ScriptArgs.Normalize(new { name = "Bob", count = 3 }); + + Assert.Equal("Bob", result!["name"]); + Assert.Equal(3, result["count"]); + } + + [Theory] + [InlineData(42)] + [InlineData(true)] + [InlineData(3.14)] + public void Normalize_Primitive_Throws(object primitive) + { + Assert.Throws(() => ScriptArgs.Normalize(primitive)); + } + + [Fact] + public void Normalize_String_Throws() + { + Assert.Throws(() => ScriptArgs.Normalize("hello")); + } + + [Fact] + public void Normalize_Decimal_Throws() + { + Assert.Throws(() => ScriptArgs.Normalize(9.99m)); + } +} diff --git a/tests/ScadaLink.Commons.Tests/Types/ValueFormatterTests.cs b/tests/ScadaLink.Commons.Tests/Types/ValueFormatterTests.cs new file mode 100644 index 0000000..3ef74dc --- /dev/null +++ b/tests/ScadaLink.Commons.Tests/Types/ValueFormatterTests.cs @@ -0,0 +1,80 @@ +using System.Globalization; +using ScadaLink.Commons.Types; + +namespace ScadaLink.Commons.Tests.Types; + +/// +/// Tests for . Includes the Commons-012 regression: +/// formatting must be culture-invariant because the formatter feeds non-UI contexts +/// (gRPC stream events, logs) where locale-dependent output would be inconsistent. +/// +public class ValueFormatterTests +{ + [Fact] + public void FormatDisplayValue_Null_ReturnsEmptyString() + { + Assert.Equal("", ValueFormatter.FormatDisplayValue(null)); + } + + [Fact] + public void FormatDisplayValue_String_ReturnsValueUnchanged() + { + Assert.Equal("hello", ValueFormatter.FormatDisplayValue("hello")); + } + + [Fact] + public void FormatDisplayValue_Collection_JoinsWithComma() + { + Assert.Equal("1,2,3", ValueFormatter.FormatDisplayValue(new[] { 1, 2, 3 })); + } + + // ── Commons-012 regression: culture-invariant numeric/date formatting ── + + [Fact] + public void FormatDisplayValue_Double_UsesInvariantCulture_RegardlessOfThreadCulture() + { + var original = CultureInfo.CurrentCulture; + try + { + // German uses a comma as the decimal separator; invariant uses a dot. + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + Assert.Equal("3.14", ValueFormatter.FormatDisplayValue(3.14)); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [Fact] + public void FormatDisplayValue_DateTime_UsesInvariantCulture_RegardlessOfThreadCulture() + { + var original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + var dt = new DateTime(2026, 5, 16, 0, 0, 0, DateTimeKind.Utc); + var invariant = dt.ToString(CultureInfo.InvariantCulture); + Assert.Equal(invariant, ValueFormatter.FormatDisplayValue(dt)); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [Fact] + public void FormatDisplayValue_CollectionOfDoubles_UsesInvariantCulture() + { + var original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + Assert.Equal("1.5,2.5", ValueFormatter.FormatDisplayValue(new[] { 1.5, 2.5 })); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } +}