fix(driver-abcip-cli): resolve Medium code-review findings (Driver.AbCip.Cli-001, -002)
Driver.AbCip.Cli-001: WriteCommand.ParseValue wraps FormatException/ OverflowException as CommandException so bad --value input yields a clean CLI error instead of a raw stack trace. Driver.AbCip.Cli-002: probe/read/subscribe commands reject Structure types up front (RejectStructure helper), matching the write guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,7 +7,7 @@
|
|||||||
| Review date | 2026-05-22 |
|
| Review date | 2026-05-22 |
|
||||||
| Commit reviewed | `76d35d1` |
|
| Commit reviewed | `76d35d1` |
|
||||||
| Status | Reviewed |
|
| Status | Reviewed |
|
||||||
| Open findings | 8 |
|
| Open findings | 6 |
|
||||||
|
|
||||||
## Checklist coverage
|
## Checklist coverage
|
||||||
|
|
||||||
@@ -40,7 +40,7 @@ a category produced nothing rather than leaving it blank.
|
|||||||
| Severity | Medium |
|
| Severity | Medium |
|
||||||
| Category | Error handling & resilience |
|
| Category | Error handling & resilience |
|
||||||
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs:70-85` |
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs:70-85` |
|
||||||
| Status | Open |
|
| Status | Resolved |
|
||||||
|
|
||||||
**Description:** `ParseValue` parses every numeric Logix type with the BCL `*.Parse`
|
**Description:** `ParseValue` parses every numeric Logix type with the BCL `*.Parse`
|
||||||
methods (`sbyte.Parse`, `short.Parse`, `int.Parse`, `float.Parse`, ...). These throw
|
methods (`sbyte.Parse`, `short.Parse`, `int.Parse`, `float.Parse`, ...). These throw
|
||||||
@@ -59,7 +59,7 @@ one-line error. CliFx only formats `CommandException` cleanly.
|
|||||||
rethrows as a `CommandException` with the raw value, the target `--type`, and the
|
rethrows as a `CommandException` with the raw value, the target `--type`, and the
|
||||||
valid range — mirroring the `ParseBool` failure message.
|
valid range — mirroring the `ParseBool` failure message.
|
||||||
|
|
||||||
**Resolution:** _(open)_
|
**Resolution:** Resolved 2026-05-22 — wrapped the `ParseValue` switch in `try/catch (FormatException or OverflowException)` that rethrows as `CommandException` with the raw value and type; updated the previously-passing `ParseValue_non_numeric_for_numeric_types_throws` test to assert `CommandException` and added two new tests covering overflow and actionable message content.
|
||||||
|
|
||||||
### Driver.AbCip.Cli-002
|
### Driver.AbCip.Cli-002
|
||||||
|
|
||||||
@@ -68,7 +68,7 @@ valid range — mirroring the `ParseBool` failure message.
|
|||||||
| Severity | Medium |
|
| Severity | Medium |
|
||||||
| Category | Correctness & logic bugs |
|
| Category | Correctness & logic bugs |
|
||||||
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs:21-23`; `Commands/ReadCommand.cs:24-25`; `Commands/SubscribeCommand.cs:20-22` |
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs:21-23`; `Commands/ReadCommand.cs:24-25`; `Commands/SubscribeCommand.cs:20-22` |
|
||||||
| Status | Open |
|
| Status | Resolved |
|
||||||
|
|
||||||
**Description:** `ProbeCommand`, `ReadCommand`, and `SubscribeCommand` expose
|
**Description:** `ProbeCommand`, `ReadCommand`, and `SubscribeCommand` expose
|
||||||
`--type` as a free `AbCipDataType` enum option with no exclusion of
|
`--type` as a free `AbCipDataType` enum option with no exclusion of
|
||||||
@@ -87,7 +87,7 @@ are out of scope here", but the code does not enforce it.
|
|||||||
pattern `WriteCommand` uses, or factor a shared `RejectStructure(DataType)` guard
|
pattern `WriteCommand` uses, or factor a shared `RejectStructure(DataType)` guard
|
||||||
into `AbCipCommandBase`.
|
into `AbCipCommandBase`.
|
||||||
|
|
||||||
**Resolution:** _(open)_
|
**Resolution:** Resolved 2026-05-22 — added `RejectStructure(AbCipDataType)` static helper to `AbCipCommandBase` that throws `CommandException` for `Structure`; called at the top of `ExecuteAsync` in `ProbeCommand`, `ReadCommand`, and `SubscribeCommand`.
|
||||||
|
|
||||||
### Driver.AbCip.Cli-003
|
### Driver.AbCip.Cli-003
|
||||||
|
|
||||||
|
|||||||
@@ -56,4 +56,19 @@ public abstract class AbCipCommandBase : DriverCommandBase
|
|||||||
/// multiple gateways in parallel can distinguish the logs.
|
/// multiple gateways in parallel can distinguish the logs.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
protected string DriverInstanceId => $"abcip-cli-{Gateway}";
|
protected string DriverInstanceId => $"abcip-cli-{Gateway}";
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Guards against <see cref="AbCipDataType.Structure"/> being passed to a command
|
||||||
|
/// that does not support UDT layouts. Call at the top of <c>ExecuteAsync</c> for any
|
||||||
|
/// command that accepts <c>--type</c> but cannot handle memberless Structure tags.
|
||||||
|
/// Throws a <see cref="CliFx.Exceptions.CommandException"/> if <paramref name="type"/>
|
||||||
|
/// is <see cref="AbCipDataType.Structure"/>.
|
||||||
|
/// </summary>
|
||||||
|
protected static void RejectStructure(AbCipDataType type)
|
||||||
|
{
|
||||||
|
if (type == AbCipDataType.Structure)
|
||||||
|
throw new CliFx.Exceptions.CommandException(
|
||||||
|
"Structure (UDT) reads are out of scope for this command — those need an explicit " +
|
||||||
|
"member layout, which belongs in a real driver config.");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ public sealed class ProbeCommand : AbCipCommandBase
|
|||||||
public override async ValueTask ExecuteAsync(IConsole console)
|
public override async ValueTask ExecuteAsync(IConsole console)
|
||||||
{
|
{
|
||||||
ConfigureLogging();
|
ConfigureLogging();
|
||||||
|
RejectStructure(DataType);
|
||||||
var ct = console.RegisterCancellationHandler();
|
var ct = console.RegisterCancellationHandler();
|
||||||
|
|
||||||
var probeTag = new AbCipTagDefinition(
|
var probeTag = new AbCipTagDefinition(
|
||||||
|
|||||||
@@ -27,6 +27,7 @@ public sealed class ReadCommand : AbCipCommandBase
|
|||||||
public override async ValueTask ExecuteAsync(IConsole console)
|
public override async ValueTask ExecuteAsync(IConsole console)
|
||||||
{
|
{
|
||||||
ConfigureLogging();
|
ConfigureLogging();
|
||||||
|
RejectStructure(DataType);
|
||||||
var ct = console.RegisterCancellationHandler();
|
var ct = console.RegisterCancellationHandler();
|
||||||
|
|
||||||
var tagName = SynthesiseTagName(TagPath, DataType);
|
var tagName = SynthesiseTagName(TagPath, DataType);
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ public sealed class SubscribeCommand : AbCipCommandBase
|
|||||||
public override async ValueTask ExecuteAsync(IConsole console)
|
public override async ValueTask ExecuteAsync(IConsole console)
|
||||||
{
|
{
|
||||||
ConfigureLogging();
|
ConfigureLogging();
|
||||||
|
RejectStructure(DataType);
|
||||||
var ct = console.RegisterCancellationHandler();
|
var ct = console.RegisterCancellationHandler();
|
||||||
|
|
||||||
var tagName = ReadCommand.SynthesiseTagName(TagPath, DataType);
|
var tagName = ReadCommand.SynthesiseTagName(TagPath, DataType);
|
||||||
|
|||||||
@@ -66,23 +66,40 @@ public sealed class WriteCommand : AbCipCommandBase
|
|||||||
/// <summary>
|
/// <summary>
|
||||||
/// Parse the operator's <c>--value</c> string into the CLR type the driver expects
|
/// Parse the operator's <c>--value</c> string into the CLR type the driver expects
|
||||||
/// for the declared <see cref="AbCipDataType"/>. Invariant culture everywhere.
|
/// for the declared <see cref="AbCipDataType"/>. Invariant culture everywhere.
|
||||||
|
/// Bad input (non-numeric text, out-of-range value) is caught and rethrown as a
|
||||||
|
/// <see cref="CliFx.Exceptions.CommandException"/> so CliFx renders a clean one-line
|
||||||
|
/// error rather than a full .NET stack trace.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
internal static object ParseValue(string raw, AbCipDataType type) => type switch
|
internal static object ParseValue(string raw, AbCipDataType type)
|
||||||
{
|
{
|
||||||
AbCipDataType.Bool => ParseBool(raw),
|
try
|
||||||
AbCipDataType.SInt => sbyte.Parse(raw, CultureInfo.InvariantCulture),
|
{
|
||||||
AbCipDataType.Int => short.Parse(raw, CultureInfo.InvariantCulture),
|
return type switch
|
||||||
AbCipDataType.DInt or AbCipDataType.Dt => int.Parse(raw, CultureInfo.InvariantCulture),
|
{
|
||||||
AbCipDataType.LInt => long.Parse(raw, CultureInfo.InvariantCulture),
|
AbCipDataType.Bool => ParseBool(raw),
|
||||||
AbCipDataType.USInt => byte.Parse(raw, CultureInfo.InvariantCulture),
|
AbCipDataType.SInt => sbyte.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
AbCipDataType.UInt => ushort.Parse(raw, CultureInfo.InvariantCulture),
|
AbCipDataType.Int => short.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
AbCipDataType.UDInt => uint.Parse(raw, CultureInfo.InvariantCulture),
|
AbCipDataType.DInt or AbCipDataType.Dt => int.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
AbCipDataType.ULInt => ulong.Parse(raw, CultureInfo.InvariantCulture),
|
AbCipDataType.LInt => long.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
AbCipDataType.Real => float.Parse(raw, CultureInfo.InvariantCulture),
|
AbCipDataType.USInt => byte.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
AbCipDataType.LReal => double.Parse(raw, CultureInfo.InvariantCulture),
|
AbCipDataType.UInt => ushort.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
AbCipDataType.String => raw,
|
AbCipDataType.UDInt => uint.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
_ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."),
|
AbCipDataType.ULInt => ulong.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
};
|
AbCipDataType.Real => float.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
|
AbCipDataType.LReal => double.Parse(raw, CultureInfo.InvariantCulture),
|
||||||
|
AbCipDataType.String => raw,
|
||||||
|
_ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
catch (Exception ex) when (ex is FormatException or OverflowException)
|
||||||
|
{
|
||||||
|
throw new CliFx.Exceptions.CommandException(
|
||||||
|
$"Cannot parse '{raw}' as {type}. " +
|
||||||
|
$"Check the value is within the valid range for {type} and uses invariant-culture " +
|
||||||
|
$"decimal notation (e.g. '3.14', not '3,14').",
|
||||||
|
innerException: ex);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch
|
private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -81,12 +81,33 @@ public sealed class WriteCommandParseValueTests
|
|||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void ParseValue_non_numeric_for_numeric_types_throws()
|
public void ParseValue_non_numeric_for_numeric_types_throws_CommandException()
|
||||||
{
|
{
|
||||||
Should.Throw<FormatException>(
|
Should.Throw<CliFx.Exceptions.CommandException>(
|
||||||
() => WriteCommand.ParseValue("xyz", AbCipDataType.DInt));
|
() => WriteCommand.ParseValue("xyz", AbCipDataType.DInt));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void ParseValue_out_of_range_throws_CommandException()
|
||||||
|
{
|
||||||
|
// sbyte max is 127; 999 overflows it.
|
||||||
|
Should.Throw<CliFx.Exceptions.CommandException>(
|
||||||
|
() => WriteCommand.ParseValue("999", AbCipDataType.SInt));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
[InlineData("12x", AbCipDataType.Int)]
|
||||||
|
[InlineData("3.14", AbCipDataType.DInt)]
|
||||||
|
[InlineData("99999999999", AbCipDataType.Int)]
|
||||||
|
public void ParseValue_bad_input_CommandException_message_is_actionable(
|
||||||
|
string raw, AbCipDataType type)
|
||||||
|
{
|
||||||
|
var ex = Should.Throw<CliFx.Exceptions.CommandException>(
|
||||||
|
() => WriteCommand.ParseValue(raw, type));
|
||||||
|
ex.Message.ShouldContain(raw);
|
||||||
|
ex.Message.ShouldContain(type.ToString());
|
||||||
|
}
|
||||||
|
|
||||||
[Theory]
|
[Theory]
|
||||||
[InlineData("Motor01_Speed", AbCipDataType.Real, "Motor01_Speed:Real")]
|
[InlineData("Motor01_Speed", AbCipDataType.Real, "Motor01_Speed:Real")]
|
||||||
[InlineData("Program:Main.Counter", AbCipDataType.DInt, "Program:Main.Counter:DInt")]
|
[InlineData("Program:Main.Counter", AbCipDataType.DInt, "Program:Main.Counter:DInt")]
|
||||||
|
|||||||
Reference in New Issue
Block a user