From acb3772426419b4daa46a9861dd90a9787b3be99 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 03:45:14 -0400 Subject: [PATCH] fix(s7): validate span-read options at parse time; document the over-PDU degradation --- .../S7DriverOptions.cs | 16 +++- .../S7DriverFactoryExtensions.cs | 33 +++++-- .../S7SpanOptionValidationTests.cs | 96 +++++++++++++++++++ 3 files changed, 135 insertions(+), 10 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs index 4b6912e8..a5771e2e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs @@ -88,10 +88,11 @@ public sealed class S7DriverOptions /// /// /// - /// Any value <= -1 disables span grouping entirely — every tag is read - /// on its own, exactly as the driver behaved before grouping existed. That is the - /// escape hatch for a CPU that dislikes wide block reads, and the control arm the - /// parity tests read through. + /// -1 disables span grouping entirely — every tag is read on its own, + /// exactly as the driver behaved before grouping existed. That is the escape hatch for a + /// CPU that dislikes wide block reads, and the control arm the parity tests read through. + /// -1 is the floor: S7DriverFactoryExtensions.ParseOptions rejects anything + /// below it, so a config cannot express "disabled" by accident with an arbitrary negative. /// /// /// Grouping cannot make a read fail where the per-tag reads would have succeeded: a @@ -121,6 +122,13 @@ public sealed class S7DriverOptions /// one enormous request that is mostly padding. The 220-byte default sits just under the /// usable payload of the common 240-byte S7-300/400 PDU, so a typical span is one request /// on the wire. + /// + /// There is deliberately no upper bound: the negotiated PDU size is opaque inside + /// S7.Net, so a value above it cannot be detected — it stays legal and simply degrades to + /// multi-request chunking inside S7.Net, costing extra wire round-trips per span rather + /// than failing. S7DriverFactoryExtensions.ParseOptions therefore validates only the + /// lower bound (>= 1); a non-positive cap can never admit a single block read. + /// /// [Display(Name = "Span read max span (bytes)", Description = "Largest single planned block read. Bounds wasted-gap amplification, not correctness.", diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs index 9548caf9..36d754c0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs @@ -56,6 +56,26 @@ public static class S7DriverFactoryExtensions throw new InvalidOperationException( $"S7 driver config for '{driverInstanceId}' missing required Host"); + // Block-read grouping knobs. There is no IValidateOptions machinery for S7, so this parse is + // the single choke point every deployed config passes through — validate here or a nonsensical + // value is accepted at deploy time and only misbehaves later, inside the read loop. + var spanReadMaxGapBytes = dto.SpanReadMaxGapBytes ?? S7DriverOptions.DefaultSpanReadMaxGapBytes; + var spanReadMaxSpanBytes = dto.SpanReadMaxSpanBytes ?? S7DriverOptions.DefaultSpanReadMaxSpanBytes; + + if (spanReadMaxSpanBytes < 1) + throw new InvalidOperationException( + $"S7 driver config for '{driverInstanceId}' has SpanReadMaxSpanBytes {spanReadMaxSpanBytes}; " + + "must be >= 1 — it caps a single planned block read, so a non-positive cap admits none. " + + "There is no upper bound: a value above the negotiated PDU is legal and degrades to " + + "multi-request chunking inside S7.Net."); + + // -1 is the documented escape hatch (span grouping disabled — S7Driver enables spans on >= 0) + // and 0 means "tolerate no gap", i.e. merge only strictly adjacent tags. Both stay legal. + if (spanReadMaxGapBytes < -1) + throw new InvalidOperationException( + $"S7 driver config for '{driverInstanceId}' has SpanReadMaxGapBytes {spanReadMaxGapBytes}; " + + "must be >= -1 — -1 disables span grouping and 0 merges only strictly adjacent tags."); + return new S7DriverOptions { Host = dto.Host!, @@ -69,11 +89,11 @@ public static class S7DriverFactoryExtensions // tags. The driver maps each RawTagEntry's TagConfig blob through S7TagDefinitionFactory // at Initialize; the legacy pre-declared DTO tag path (Name/Address → BuildTag) is retired. RawTags = dto.RawTags is { Count: > 0 } ? [.. dto.RawTags] : [], - // Block-read grouping knobs. Mapped explicitly like every other key on this DTO — the - // options object is never deserialised directly — so omitting them here would leave the - // documented "-1 disables grouping" escape hatch unreachable from a deployed config. - SpanReadMaxGapBytes = dto.SpanReadMaxGapBytes ?? S7DriverOptions.DefaultSpanReadMaxGapBytes, - SpanReadMaxSpanBytes = dto.SpanReadMaxSpanBytes ?? S7DriverOptions.DefaultSpanReadMaxSpanBytes, + // Mapped explicitly like every other key on this DTO — the options object is never + // deserialised directly — so omitting them here would leave the documented + // "-1 disables grouping" escape hatch unreachable from a deployed config. + SpanReadMaxGapBytes = spanReadMaxGapBytes, + SpanReadMaxSpanBytes = spanReadMaxSpanBytes, Probe = new S7ProbeOptions { Enabled = dto.Probe?.Enabled ?? true, @@ -128,7 +148,8 @@ public static class S7DriverFactoryExtensions /// Gets the probe configuration. public S7ProbeDto? Probe { get; init; } /// Gets the largest hole the read planner reads through when merging tags into one - /// block request; any value below zero disables block-read grouping. Null keeps the default. + /// block request; -1 disables block-read grouping and anything below it is rejected. + /// Null keeps the default. public int? SpanReadMaxGapBytes { get; init; } /// Gets the largest single planned block read in bytes. Null keeps the default. public int? SpanReadMaxSpanBytes { get; init; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs new file mode 100644 index 00000000..c565cda6 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs @@ -0,0 +1,96 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// Config-time guard on the two span-read knobs. There is no IValidateOptions machinery +/// for S7 — is the single choke point every +/// deployed config passes through — so a nonsensical value used to be accepted at deploy time and +/// only misbehave later, in the read loop, on a driver that had already reported healthy. +/// +/// +/// The legal-value cases matter as much as the rejections: -1 on the gap is the documented +/// escape hatch that disables grouping entirely (S7Driver enables spans on >= 0), +/// and 0 means "tolerate no gap at all", i.e. merge only strictly adjacent tags. A guard +/// written as < 0 would break both. +/// +public sealed class S7SpanOptionValidationTests +{ + /// A driver-config blob carrying the given span knob, host included (it is required). + private static string Config(string knob, int value) => + "{\"host\":\"10.0.0.1\",\"" + knob + "\":" + value.ToString(System.Globalization.CultureInfo.InvariantCulture) + "}"; + + /// A span cap below one can never admit a single planned block read, so it is rejected. + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(-220)] + public void A_span_cap_below_one_is_rejected(int maxSpanBytes) + { + var ex = Should.Throw(() => S7DriverFactoryExtensions.ParseOptions( + "s7-cfg", Config("spanReadMaxSpanBytes", maxSpanBytes))); + + ex.Message.ShouldContain("SpanReadMaxSpanBytes"); + ex.Message.ShouldContain("s7-cfg"); + } + + /// A gap budget below the grouping-disabled sentinel is rejected. + [Theory] + [InlineData(-2)] + [InlineData(-16)] + public void A_gap_budget_below_minus_one_is_rejected(int maxGapBytes) + { + var ex = Should.Throw(() => S7DriverFactoryExtensions.ParseOptions( + "s7-cfg", Config("spanReadMaxGapBytes", maxGapBytes))); + + ex.Message.ShouldContain("SpanReadMaxGapBytes"); + ex.Message.ShouldContain("s7-cfg"); + } + + /// + /// The two legal edge values survive: -1 (grouping disabled) and 0 (no gap + /// tolerated). Both are documented behaviour, not degenerate input. + /// + [Theory] + [InlineData(-1)] + [InlineData(0)] + public void The_documented_gap_edge_values_are_accepted(int maxGapBytes) + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxGapBytes", maxGapBytes)); + + parsed.SpanReadMaxGapBytes.ShouldBe(maxGapBytes); + } + + /// A one-byte span cap is the smallest legal cap and must not trip the guard. + [Fact] + public void The_smallest_legal_span_cap_is_accepted() + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxSpanBytes", 1)); + + parsed.SpanReadMaxSpanBytes.ShouldBe(1); + } + + /// + /// An over-PDU span cap is legal — S7.Net transparently chunks an oversized ReadBytes + /// into several wire requests — so the guard must not invent an upper bound it cannot know + /// (the negotiated PDU size is opaque inside S7.Net). + /// + [Fact] + public void An_over_pdu_span_cap_is_accepted_because_S7Net_chunks_it() + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxSpanBytes", 4096)); + + parsed.SpanReadMaxSpanBytes.ShouldBe(4096); + } + + /// Omitting both knobs keeps the shipped defaults, which must themselves pass the guard. + [Fact] + public void The_shipped_defaults_pass_the_guard() + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", "{\"host\":\"10.0.0.1\"}"); + + parsed.SpanReadMaxGapBytes.ShouldBe(S7DriverOptions.DefaultSpanReadMaxGapBytes); + parsed.SpanReadMaxSpanBytes.ShouldBe(S7DriverOptions.DefaultSpanReadMaxSpanBytes); + } +}