From 6bb971c040fd966c8d2c4055eabfe17b9584d6dc Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:15:19 -0400 Subject: [PATCH] fix(driver-ablegacy-cli): resolve Medium code-review finding (Driver.AbLegacy.Cli-001) WriteCommand.ParseValue wraps FormatException/OverflowException as CliFx CommandException so a bad --value yields a clean one-line CLI error naming the value and target type instead of a raw .NET stack trace. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.AbLegacy.Cli/findings.md | 6 +-- .../Commands/WriteCommand.cs | 41 ++++++++++++++----- .../WriteCommandParseValueTests.cs | 16 +++++++- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/code-reviews/Driver.AbLegacy.Cli/findings.md b/code-reviews/Driver.AbLegacy.Cli/findings.md index 972978a..cff5fc8 100644 --- a/code-reviews/Driver.AbLegacy.Cli/findings.md +++ b/code-reviews/Driver.AbLegacy.Cli/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 7 | +| Open findings | 6 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | Medium | | Category | Error handling & resilience | | Location | `Commands/WriteCommand.cs:46`, `Commands/WriteCommand.cs:62-72` | -| Status | Open | +| Status | Resolved | **Description:** `WriteCommand.ExecuteAsync` calls `ParseValue(Value, DataType)` at line 46, *before* the `try` block and outside any catch. `ParseValue` uses @@ -59,7 +59,7 @@ type (mirroring the existing `Bit` and unsupported-type branches). Either catch `FormatException`/`OverflowException` inside `ParseValue` and rethrow as `CommandException`, or use `TryParse` and throw `CommandException` on failure. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — wrapped numeric parses in `ParseValue` with `try/catch` for `FormatException`/`OverflowException`, rethrowing as `CommandException` with a message naming the offending value and type; updated test to assert `CommandException` and added overflow regression test. ### Driver.AbLegacy.Cli-002 diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/Commands/WriteCommand.cs index 4e71bf5..4ce6695 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/Commands/WriteCommand.cs @@ -59,17 +59,38 @@ public sealed class WriteCommand : AbLegacyCommandBase } /// Parse --value per , invariant culture. - internal static object ParseValue(string raw, AbLegacyDataType type) => type switch + /// + /// Thrown when cannot be parsed as the requested type (malformed + /// input or out-of-range value) so CliFx renders a clean one-line error instead of a raw + /// stack trace. + /// + internal static object ParseValue(string raw, AbLegacyDataType type) { - AbLegacyDataType.Bit => ParseBool(raw), - AbLegacyDataType.Int or AbLegacyDataType.AnalogInt => short.Parse(raw, CultureInfo.InvariantCulture), - AbLegacyDataType.Long => int.Parse(raw, CultureInfo.InvariantCulture), - AbLegacyDataType.Float => float.Parse(raw, CultureInfo.InvariantCulture), - AbLegacyDataType.String => raw, - AbLegacyDataType.TimerElement or AbLegacyDataType.CounterElement - or AbLegacyDataType.ControlElement => int.Parse(raw, CultureInfo.InvariantCulture), - _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), - }; + try + { + return type switch + { + AbLegacyDataType.Bit => ParseBool(raw), + AbLegacyDataType.Int or AbLegacyDataType.AnalogInt => short.Parse(raw, CultureInfo.InvariantCulture), + AbLegacyDataType.Long => int.Parse(raw, CultureInfo.InvariantCulture), + AbLegacyDataType.Float => float.Parse(raw, CultureInfo.InvariantCulture), + AbLegacyDataType.String => raw, + AbLegacyDataType.TimerElement or AbLegacyDataType.CounterElement + or AbLegacyDataType.ControlElement => int.Parse(raw, CultureInfo.InvariantCulture), + _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), + }; + } + catch (FormatException ex) + { + throw new CliFx.Exceptions.CommandException( + $"Value '{raw}' is not a valid {type}: {ex.Message}", innerException: ex); + } + catch (OverflowException ex) + { + throw new CliFx.Exceptions.CommandException( + $"Value '{raw}' is out of range for {type}: {ex.Message}", innerException: ex); + } + } private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch { diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs index 5c4049b..51b54e3 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs @@ -72,12 +72,24 @@ public sealed class WriteCommandParseValueTests } [Fact] - public void ParseValue_non_numeric_for_numeric_types_throws() + public void ParseValue_non_numeric_for_numeric_types_throws_CommandException() { - Should.Throw( + // Bad input must surface as CommandException (clean one-line error), not a raw FormatException. + Should.Throw( () => WriteCommand.ParseValue("xyz", AbLegacyDataType.Int)); } + [Theory] + [InlineData("99999", AbLegacyDataType.Int)] // short range is ±32767 + [InlineData("-99999", AbLegacyDataType.Int)] + [InlineData("-99999", AbLegacyDataType.AnalogInt)] + public void ParseValue_out_of_range_throws_CommandException(string raw, AbLegacyDataType type) + { + // Out-of-range values must surface as CommandException, not OverflowException. + Should.Throw( + () => WriteCommand.ParseValue(raw, type)); + } + [Theory] [InlineData("N7:0", AbLegacyDataType.Int, "N7:0:Int")] [InlineData("B3:0/3", AbLegacyDataType.Bit, "B3:0/3:Bit")]