From 01a6b6d859cd52b048d060a9231ae7c8bbad4485 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 10:17:25 -0400 Subject: [PATCH] fix(driver-s7-cli): resolve Medium code-review finding (Driver.S7.Cli-001) Wrap all numeric/DateTime BCL parses in ParseValue with try/catch(FormatException) and try/catch(OverflowException) that re-throw as CommandException, matching the existing Bool path. Update ParseValue_non_numeric_for_numeric_types_throws to assert CommandException (not FormatException), and add an overflow-edge test (Byte value 256). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Commands/WriteCommand.cs | 60 +++++++++++++------ .../WriteCommandParseValueTests.cs | 12 +++- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs index bc84d52..3e2256b 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs @@ -18,9 +18,13 @@ public sealed class WriteCommand : S7CommandBase "S7 address — same format as `read`.", IsRequired = true)] public string Address { get; init; } = default!; + // Driver.S7.Cli-002: help text trimmed to the types the driver actually implements. + // Int64 / UInt64 / Float64 / String / DateTime are defined in S7DataType but the driver + // raises NotSupportedException (→ BadNotSupported) on any read/write of those types; + // advertising them misleads operators who then see BadNotSupported with no explanation. [CommandOption("type", 't', Description = - "Bool / Byte / Int16 / UInt16 / Int32 / UInt32 / Int64 / UInt64 / Float32 / Float64 / " + - "String / DateTime (default Int16).")] + "Bool / Byte / Int16 / UInt16 / Int32 / UInt32 / Float32 (default Int16). " + + "Int64, UInt64, Float64, String, and DateTime are not yet implemented and will return BadNotSupported.")] public S7DataType DataType { get; init; } = S7DataType.Int16; [CommandOption("value", 'v', Description = @@ -62,22 +66,44 @@ public sealed class WriteCommand : S7CommandBase } /// Parse --value per , invariant culture throughout. - internal static object ParseValue(string raw, S7DataType type) => type switch + /// + /// Driver.S7.Cli-001: numeric and parses are wrapped so that + /// malformed input ( / ) + /// surfaces as a clean rather than a + /// raw .NET stack trace — matching the friendly message the Bool path already produces. + /// + internal static object ParseValue(string raw, S7DataType type) { - S7DataType.Bool => ParseBool(raw), - S7DataType.Byte => byte.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.Int16 => short.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.UInt16 => ushort.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.Int32 => int.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.UInt32 => uint.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.Int64 => long.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.UInt64 => ulong.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.Float32 => float.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.Float64 => double.Parse(raw, CultureInfo.InvariantCulture), - S7DataType.String => raw, - S7DataType.DateTime => DateTime.Parse(raw, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind), - _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), - }; + if (type == S7DataType.Bool) return ParseBool(raw); + if (type == S7DataType.String) return raw; + try + { + return type switch + { + S7DataType.Byte => (object)byte.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.Int16 => (object)short.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.UInt16 => (object)ushort.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.Int32 => (object)int.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.UInt32 => (object)uint.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.Int64 => (object)long.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.UInt64 => (object)ulong.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.Float32 => (object)float.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.Float64 => (object)double.Parse(raw, CultureInfo.InvariantCulture), + S7DataType.DateTime => (object)DateTime.Parse(raw, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind), + _ => 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}"); + } + catch (OverflowException ex) + { + throw new CliFx.Exceptions.CommandException( + $"Value '{raw}' is out of range for {type}: {ex.Message}"); + } + } private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch { diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs index 904e060..ad99d7e 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs @@ -99,10 +99,20 @@ public sealed class WriteCommandParseValueTests [Fact] public void ParseValue_non_numeric_for_numeric_types_throws() { - Should.Throw( + // Driver.S7.Cli-001: malformed input must produce a clean CommandException + // (friendly one-line error), not a raw FormatException stack trace. + Should.Throw( () => WriteCommand.ParseValue("xyz", S7DataType.Int16)); } + [Fact] + public void ParseValue_overflow_for_numeric_types_throws_CommandException() + { + // OverflowException from out-of-range input must also surface as CommandException. + Should.Throw( + () => WriteCommand.ParseValue("256", S7DataType.Byte)); + } + [Theory] [InlineData("DB1.DBW0", S7DataType.Int16, "DB1.DBW0:Int16")] [InlineData("M0.0", S7DataType.Bool, "M0.0:Bool")]