diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index 0c4a53a..6435d38 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-17 | | Reviewer | claude-agent | | Commit reviewed | `39d737e` | -| Open findings | 2 | +| Open findings | 0 | ## Summary @@ -587,7 +587,7 @@ each pinned under `de-DE`). |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Types/DynamicJsonElement.cs:40-54` | **Description** @@ -611,7 +611,18 @@ single integer before the bounds check. Add a regression test indexing with a `l **Resolution** -_Unresolved._ +Resolved 2026-05-17 — confirmed the `int`-only guard, and during fixing found the +underlying cause was wider than the finding text: `Wrap`'s `TryGetInt64 ? long : double` +ternary unified both branches to `double`, so integral JSON numbers were never actually +boxed as `long` — `obj.count` always surfaced as `double`. Fixed at the root: `Wrap` +delegates to a new `WrapNumber` helper that boxes integral numbers as `long` and +non-integral as `double` with separate typed returns; `TryGetIndex` now accepts any +integral index via `TryGetIntegralIndex` (`int`/`long`/`short`/`byte`/`sbyte`/`ushort`/ +`uint`/`ulong`, plus whole-valued `double`/`decimal`) normalized to `long` before the +bounds check. Internal-only change — no public-API or message-contract change. Regression +tests added in `DynamicJsonElementTests` (`IndexAccess_WithLongIndex_Works`, +`IndexAccess_WithIndexDerivedFromWrappedJsonNumber_Works`, +`IndexAccess_WithWideningIntegralIndex_Works`, `IndexAccess_WithLongIndexOutOfRange_Throws`). ### Commons-014 — `OpcUaEndpointConfigSerializer.Deserialize` can mislabel a corrupt typed row as `Legacy` @@ -619,7 +630,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs:107-131` | **Description** @@ -651,4 +662,15 @@ field. **Resolution** -_Unresolved._ +Resolved 2026-05-17 — confirmed the swallow-and-fall-through against the source. Split +`Deserialize` into a shape-detection phase and a materialization phase: it first parses +the document to decide whether the row is the current typed shape (root object carrying +`endpointUrl`); only a non-typed-shape row falls through to `LoadLegacy`. A row that *is* +the typed shape but fails `JsonSerializer.Deserialize` (invalid enum, wrong-typed field) +is now classified `Malformed` instead of being mislabelled `Legacy` with the offending +field silently dropped. The `OpcUaConfigParseResult` shape and `(Config, IsLegacy)` +deconstruction are unchanged, so existing callers are unaffected. XML docs updated to +describe the corrupt-typed-row branch. Regression tests added in +`OpcUaEndpointConfigSerializerTests` (`Deserialize_TypedShapeWithInvalidEnum_ReportsMalformedNotLegacy`, +`Deserialize_TypedShapeWithWrongTypeField_ReportsMalformedNotLegacy`, +`Deserialize_ValidTypedRow_StillReportsTyped`). diff --git a/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs b/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs index b5cdd0f..b66bef0 100644 --- a/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs +++ b/src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs @@ -68,9 +68,11 @@ public readonly record struct OpcUaConfigParseResult /// /// 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 reports so the form can prompt the -/// user to re-save. +/// On read, falls back to the legacy flat string-dict shape only for rows that are not +/// the current typed shape (no endpointUrl property), reporting +/// so the form can prompt the user to +/// re-save. A row that is the typed shape but fails to deserialize is reported +/// , never . /// public static class OpcUaEndpointConfigSerializer { @@ -94,9 +96,13 @@ public static class OpcUaEndpointConfigSerializer /// — 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. + /// unparseable JSON, or it is the current typed shape (it has an + /// endpointUrl property) but typed deserialization failed — e.g. an + /// enum-valued field holding an unrecognised string or a wrong-typed field. Such a + /// corrupt typed row is reported + /// rather than being mislabelled , so the + /// offending field is not silently dropped. The config is an empty default and the + /// caller should surface an error rather than treating it as the user's saved data. /// /// public static OpcUaConfigParseResult Deserialize(string? json) @@ -104,22 +110,42 @@ public static class OpcUaEndpointConfigSerializer if (string.IsNullOrWhiteSpace(json)) return new OpcUaConfigParseResult(new OpcUaEndpointConfig(), OpcUaConfigParseStatus.Typed); + // First decide which shape the row is — without yet trying to materialize it. + // A root JSON object carrying "endpointUrl" IS the current typed shape; anything + // else (no endpointUrl) is treated as a candidate legacy flat-dict row. + bool isTypedShape; try { using var doc = JsonDocument.Parse(json); - if (doc.RootElement.ValueKind == JsonValueKind.Object - && doc.RootElement.TryGetProperty("endpointUrl", out _)) + isTypedShape = doc.RootElement.ValueKind == JsonValueKind.Object + && doc.RootElement.TryGetProperty("endpointUrl", out _); + } + catch (JsonException) + { + // Could not even parse the document: genuinely malformed input. + return new OpcUaConfigParseResult(new OpcUaEndpointConfig(), OpcUaConfigParseStatus.Malformed); + } + + if (isTypedShape) + { + // The row is the current typed shape. If typed deserialization fails the row + // is a corrupt current-shape row (e.g. an invalid enum or wrong-typed field) — + // it must NOT fall through to the legacy path and be mislabelled Legacy, which + // would silently drop the offending field. Report Malformed instead. + try { var typed = JsonSerializer.Deserialize(json, JsonOpts); if (typed != null) return new OpcUaConfigParseResult(typed, OpcUaConfigParseStatus.Typed); } + catch (JsonException) { /* corrupt typed row — classified Malformed below */ } + + return new OpcUaConfigParseResult(new OpcUaEndpointConfig(), OpcUaConfigParseStatus.Malformed); } - catch (JsonException) { /* fall through to legacy */ } try { - return new OpcUaConfigParseResult(LoadLegacy(json!), OpcUaConfigParseStatus.Legacy); + return new OpcUaConfigParseResult(LoadLegacy(json), OpcUaConfigParseStatus.Legacy); } catch (JsonException) { diff --git a/src/ScadaLink.Commons/Types/DynamicJsonElement.cs b/src/ScadaLink.Commons/Types/DynamicJsonElement.cs index 27c939a..f1b4ec2 100644 --- a/src/ScadaLink.Commons/Types/DynamicJsonElement.cs +++ b/src/ScadaLink.Commons/Types/DynamicJsonElement.cs @@ -6,6 +6,8 @@ namespace ScadaLink.Commons.Types; /// /// Wraps a JsonElement as a dynamic object for convenient property access in scripts. /// Supports property access (obj.name), indexing (obj.items[0]), and ToString(). +/// Array indexing accepts any integral index type (int, long, short, byte, ...), so an +/// index derived from another wrapped JSON number — which Wrap surfaces as long — works. /// /// /// The element passed to the constructor is cloned @@ -39,13 +41,16 @@ public class DynamicJsonElement : DynamicObject public override bool TryGetIndex(GetIndexBinder binder, object[] indexes, out object? result) { + // Accept any integral index, not just int. DynamicJsonElement surfaces JSON + // numbers as long (see Wrap), so an index computed from another wrapped value + // (e.g. obj.items[obj.count - 1]) arrives as a long; byte/short widen too. if (_element.ValueKind == JsonValueKind.Array && - indexes.Length == 1 && indexes[0] is int index) + indexes.Length == 1 && TryGetIntegralIndex(indexes[0], out var index)) { var arrayLength = _element.GetArrayLength(); if (index >= 0 && index < arrayLength) { - result = Wrap(_element[index]); + result = Wrap(_element[(int)index]); return true; } } @@ -53,6 +58,28 @@ public class DynamicJsonElement : DynamicObject return false; } + private static bool TryGetIntegralIndex(object? value, out long index) + { + switch (value) + { + case int i: index = i; return true; + case long l: index = l; return true; + case short s: index = s; return true; + case byte b: index = b; return true; + case sbyte sb: index = sb; return true; + case ushort us: index = us; return true; + case uint ui: index = ui; return true; + case ulong ul when ul <= long.MaxValue: index = (long)ul; return true; + // Whole-valued floating-point indices are accepted too: arithmetic on + // wrapped JSON numbers can yield a double/decimal even for an integer result. + case double d when d >= long.MinValue && d <= long.MaxValue && d == Math.Floor(d): + index = (long)d; return true; + case decimal m when m == Math.Floor(m): + index = (long)m; return true; + default: index = 0; return false; + } + } + public override bool TryConvert(ConvertBinder binder, out object? result) { // Conversion to object (or dynamic): never null out a present value. Return the @@ -102,11 +129,23 @@ public class DynamicJsonElement : DynamicObject JsonValueKind.Object => new DynamicJsonElement(element), JsonValueKind.Array => new DynamicJsonElement(element), JsonValueKind.String => element.GetString(), - JsonValueKind.Number => element.TryGetInt64(out var l) ? l : element.GetDouble(), + JsonValueKind.Number => WrapNumber(element), JsonValueKind.True => true, JsonValueKind.False => false, JsonValueKind.Null => null, _ => element.GetRawText() }; } + + private static object WrapNumber(JsonElement element) + { + // An integral JSON number must box as long, not double. A ternary + // (TryGetInt64 ? long : double) would unify both branches to double and + // silently widen the long, so an integral index read back out of the wrapper + // (e.g. obj.items[obj.count - 1]) would arrive as a double. Box each case + // with its own typed return so the runtime type is preserved. + if (element.TryGetInt64(out var l)) + return l; + return element.GetDouble(); + } } diff --git a/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs b/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs index 61425a1..34f258d 100644 --- a/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs +++ b/tests/ScadaLink.Commons.Tests/Types/DataConnections/OpcUaEndpointConfigSerializerTests.cs @@ -168,6 +168,47 @@ public class OpcUaEndpointConfigSerializerTests Assert.True(result.IsLegacy); } + // ── Commons-014 regression: a corrupt typed row must not be mislabelled Legacy ── + + [Fact] + public void Deserialize_TypedShapeWithInvalidEnum_ReportsMalformedNotLegacy() + { + // The row IS the current typed shape (it has endpointUrl) but an enum-valued + // field holds an unrecognised string. Typed deserialization throws JsonException; + // it must NOT fall through to the legacy path and be reported as Legacy. + var json = """{"endpointUrl":"opc.tcp://x:4840","securityMode":"NotARealMode"}"""; + + var result = OpcUaEndpointConfigSerializer.Deserialize(json); + + Assert.Equal(OpcUaConfigParseStatus.Malformed, result.Status); + Assert.False(result.IsLegacy); + } + + [Fact] + public void Deserialize_TypedShapeWithWrongTypeField_ReportsMalformedNotLegacy() + { + // endpointUrl present (typed shape) but a numeric field holds a non-numeric token. + var json = """{"endpointUrl":"opc.tcp://x:4840","sessionTimeoutMs":"not-a-number"}"""; + + var result = OpcUaEndpointConfigSerializer.Deserialize(json); + + Assert.Equal(OpcUaConfigParseStatus.Malformed, result.Status); + Assert.False(result.IsLegacy); + } + + [Fact] + public void Deserialize_ValidTypedRow_StillReportsTyped() + { + // Guard: a clean typed row is still classified Typed after the Commons-014 fix. + var json = """{"endpointUrl":"opc.tcp://x:4840","securityMode":"sign"}"""; + + var result = OpcUaEndpointConfigSerializer.Deserialize(json); + + Assert.Equal(OpcUaConfigParseStatus.Typed, result.Status); + Assert.Equal("opc.tcp://x:4840", result.Config.EndpointUrl); + Assert.Equal(OpcUaSecurityMode.Sign, result.Config.SecurityMode); + } + [Fact] public void Deserialize_TwoElementDeconstruction_StillWorks() { diff --git a/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs b/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs index 9c6731b..cd500d2 100644 --- a/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs +++ b/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs @@ -144,4 +144,48 @@ public class DynamicJsonElementTests Assert.False(strWrapper.TryConvert(convBinder, out var result)); Assert.Null(result); } + + // ── Commons-013 regression: integral index values other than int must work ── + + [Fact] + public void IndexAccess_WithLongIndex_Works() + { + // DynamicJsonElement.Wrap surfaces JSON numbers as long; an index computed + // from a wrapped JSON number (obj.items[obj.count - 1]) arrives as a long. + dynamic obj = Wrap("""{ "items": [ "a", "b", "c" ] }"""); + long idx = 1L; + + Assert.Equal("b", obj.items[idx]); + } + + [Fact] + public void IndexAccess_WithIndexDerivedFromWrappedJsonNumber_Works() + { + // The exact failing case from Commons-013: count is a wrapped JSON number + // (unwrapped as long), so count - 1 is a long. + dynamic obj = Wrap("""{ "items": [ "a", "b", "c" ], "count": 3 }"""); + + Assert.Equal("c", obj.items[obj.count - 1]); + } + + [Theory] + [InlineData((byte)0, "a")] + [InlineData((short)1, "b")] + public void IndexAccess_WithWideningIntegralIndex_Works(object index, string expected) + { + dynamic obj = Wrap("""{ "items": [ "a", "b", "c" ] }"""); + + Assert.Equal(expected, obj.items[index]); + } + + [Fact] + public void IndexAccess_WithLongIndexOutOfRange_Throws() + { + // An out-of-range long index is still rejected (binder surfaces the error). + dynamic obj = Wrap("""{ "items": [ "a" ] }"""); + long idx = 5L; + + Assert.Throws( + () => { var _ = obj.items[idx]; }); + } }