From c1b9c7ec5cb97598724eaac8a9fc5f84748c8781 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:32:23 -0400 Subject: [PATCH] fix(sql): attribute a duplicated key to its own tags; key-value cap gets 2x headroom (R06:S-1 review I-1, m-3) One stray duplicate row used to fail every tag in a key-value group or none of them, depending on whether an unrelated key was absent. Duplicated keys are now refused per key: only that key's tags publish BadConfigurationError and the rest of the group reads normally. The key-value cap reads up to 2x the distinct keys (+1 to detect excess), so a few stray duplicates are attributed per key and only a gross excess (history/events table) fails the whole group. Duplicated-key refusals count as answered refusals for health. The row-cap, duplicate-key and multi-row warnings now use the per-plan limiter (m-3); the reader-wide limiter is removed. Unit tests pin both the all-keys-present and absent-key cases with Good controls; live SQL Server gate added (23/23). Refs #744 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/Sql.md | 27 +++-- .../ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs | 2 +- .../SqlGroupPlanner.cs | 10 +- .../SqlPollReader.cs | 104 ++++++++++-------- .../SqlQueryPlan.cs | 12 +- .../SqlServerReadTests.cs | 63 +++++++++++ .../SqlGroupPlannerTests.cs | 17 +-- .../SqlPollReaderTests.cs | 56 ++++++++-- 8 files changed, 206 insertions(+), 85 deletions(-) diff --git a/docs/drivers/Sql.md b/docs/drivers/Sql.md index ffa68922..120d41c1 100644 --- a/docs/drivers/Sql.md +++ b/docs/drivers/Sql.md @@ -166,16 +166,23 @@ emits a `SqlQueryPlan` per distinct group key. Grouping is deterministic (groups order of their first member, members keep their input order, parameters keep first-appearance order), so an unchanged configuration produces byte-identical SQL. -**Every key-value and where-pair query is row-capped (R06:S-1).** A key-value plan asks for one row more -than it has distinct keys (`TOP (n+1)` / `LIMIT n+1` / `FETCH FIRST n+1 ROWS ONLY`), a where-pair wide-row plan -for two, and the reader stops reading there even if a provider ignores the limit. Receiving the extra row -proves the selector is ambiguous — a history/events table, or a where column that is not unique — so every -member of that group publishes `BadConfigurationError` with a rate-limited warning naming the table and -selector, instead of the old behaviour of materialising every matching row on every poll and publishing an -arbitrary "last row" at Good. A duplicate that fits inside the cap (a duplicated key alongside an absent one) -still resolves last-row-wins. `topByTimestamp` keeps its single-row limit. An over-cap answer, like every -other refusal (absent row, NULL cell, uncoercible cell), is the database answering: it advances -`LastSuccessfulRead` and restores `Healthy` after a comms fault. +**Every key-value and where-pair query is row-capped (R06:S-1).** A key-value plan reads at most twice as +many rows as it has distinct keys and asks for one more (`TOP (2n+1)` / `LIMIT 2n+1` / `FETCH FIRST 2n+1 ROWS +ONLY`); a where-pair wide-row plan asks for two. The reader stops there even if a provider ignores the limit. + +- **A duplicated key is refused per key.** When a key comes back more than once, only the tags on THAT key + publish `BadConfigurationError`; every other key in the group reads normally, whatever else is present or + absent. (Before this, a duplicate was last-row-wins Good.) +- **A gross excess fails the group.** Receiving the extra row past the cap means a history/events table or a + non-unique where column, which cannot be attributed because the rest is unread, so every member of the group + publishes `BadConfigurationError`. For a where-pair any second row is this case. +- Each case logs a per-plan rate-limited warning naming the table and the selector (or an example key). + `topByTimestamp` keeps its single-row limit. +- **Upgrade note:** a deployment whose key-value source carries stray duplicate rows, or whose where column is + not unique, will see those tags go Bad where they used to read an arbitrary row at Good. + +Every such refusal, like the other refusals (absent row, NULL cell, uncoercible cell), is the database +answering: it advances `LastSuccessfulRead` and restores `Healthy` after a comms fault. ⚠️ **Group keys are case-sensitive on identifiers.** Two tags authored `dbo.T` and `DBO.T` plan as **two** groups — one extra round-trip, not a correctness problem, but worth knowing if a group count diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs index 7fbd99d7..d7212245 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs @@ -542,7 +542,7 @@ public sealed class SqlDriver try { var result = await _reader.ReadDetailedAsync(fullReferences, cancellationToken).ConfigureAwait(false); - ObservePollOutcome(result.Snapshots, result.RowCapExceededGroups, generation); + ObservePollOutcome(result.Snapshots, result.AnsweredRefusalGroups, generation); return result.Snapshots; } catch (OperationCanceledException) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs index 31c77351..915eb89c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs @@ -230,10 +230,12 @@ public static class SqlGroupPlanner values.Add(keyValue); } - // Row cap (R06:S-1): one row per distinct key is all this plan can use, so it asks for ONE more — enough - // to see a source breaking the one-row-per-key contract, never enough to pull a history table's every - // matching row into memory on every poll. - var rowCap = values.Count; + // Row cap (R06:S-1): one row per distinct key is all this plan can use. It reads up to TWICE that, so a + // handful of stray duplicates stays inside the cap and is attributed PER KEY by the reader (only the + // duplicated key's tags go BadConfigurationError) — and asks for one row more than that, so only a + // gross excess (a history/events table, unattributable by construction because the rest is unread) + // fails the whole group. Memory stays O(keys) either way. + var rowCap = values.Count * 2; var sql = new StringBuilder("SELECT ") .Append(dialect.RowLimitPrefix(rowCap + 1)) .Append(QuoteList(selected, dialect)) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs index fdb22ff0..ffeb3804 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs @@ -68,7 +68,6 @@ public sealed class SqlPollReader : IDisposable private int _disposed; - private long _lastContractWarningTicks; /// /// Per-key last-warned instant for the per-poll warnings (R06:S-6): a frozen group's BadTimeout and a @@ -215,7 +214,7 @@ public sealed class SqlPollReader : IDisposable results[i] = new DataValueSnapshot(null, SqlStatusCodes.BadNodeIdUnknown, null, readAt); } - var rowCapExceeded = shape.Definitions.Count > 0 + var answeredRefusals = shape.Definitions.Count > 0 ? await ReadGroupsAsync(shape, results, readAt, cancellationToken).ConfigureAwait(false) : 0; @@ -224,7 +223,7 @@ public sealed class SqlPollReader : IDisposable for (var i = 0; i < results.Length; i++) results[i] ??= new DataValueSnapshot(null, SqlStatusCodes.BadInternalError, null, readAt); - return new SqlPollResult(results, rowCapExceeded); + return new SqlPollResult(results, answeredRefusals); } /// How many times a reference list has been (re)planned. Test observation of the S-7 cache. @@ -301,7 +300,7 @@ public sealed class SqlPollReader : IDisposable /// /// Runs every planned group concurrently and slices the results back. Returns how many groups came - /// back over their row cap. + /// back with an answered refusal (over the row cap, or a duplicated key). /// private async Task ReadGroupsAsync( PollShape shape, @@ -334,14 +333,14 @@ public sealed class SqlPollReader : IDisposable // leaves a sibling group running against a connection nobody is watching. var outcomes = await Task.WhenAll(groups).ConfigureAwait(false); - var rowCapExceeded = 0; + var answeredRefusals = 0; for (var g = 0; g < plans.Count; g++) { - Slice(plans[g], outcomes[g], slots, results, readAt); - if (ReferenceEquals(outcomes[g], GroupResult.RowCapExceeded)) rowCapExceeded++; + var refusedSome = Slice(plans[g], outcomes[g], slots, results, readAt); + if (refusedSome || ReferenceEquals(outcomes[g], GroupResult.RowCapExceeded)) answeredRefusals++; } - return rowCapExceeded; + return answeredRefusals; } /// @@ -622,14 +621,16 @@ public sealed class SqlPollReader : IDisposable if (plan.RowCap is { } rowCap && rows.Count > rowCap) { - if (ShouldWarnAboutContract()) + // Per plan, not reader-wide: with two ambiguous groups the reader-wide limiter could name only the + // first table forever, and with health still Healthy this line is the operator's only pointer. + if (ShouldWarn("rowcap:" + plan.SqlText)) { var first = plan.Members[0]; var selector = plan.Model == SqlTagModel.KeyValue - ? string.Concat(first.KeyColumn, " IN (", rowCap.ToString(CultureInfo.InvariantCulture), " key(s))") + ? string.Concat(first.KeyColumn, " IN (", (rowCap / 2).ToString(CultureInfo.InvariantCulture), " key(s))") : string.Concat(first.RowSelectorColumn, " = ", first.RowSelectorValue); _logger.LogWarning( - "Sql poll source '{Table}' returned more rows than the {Model} selector '{Selector}' can use " + + "Sql poll source '{Table}' returned more rows than the {Model} selector '{Selector}' can attribute " + "(at most {Cap}) in one poll, so the selector is ambiguous — a history/events table, or a " + "where column that is not unique. Its {Members} tag(s) publish BadConfigurationError and " + "nothing past row {Limit} is read. Point the tag at a current-values table or a latest-row view.", @@ -658,8 +659,11 @@ public sealed class SqlPollReader : IDisposable } } - /// Maps one group's outcome onto every slot its members occupy in the caller's list. - private void Slice( + /// + /// Maps one group's outcome onto every slot its members occupy in the caller's list. Returns whether any + /// member was refused on an ANSWERED result (a duplicated key) — answer evidence the driver must count. + /// + private bool Slice( SqlQueryPlan plan, GroupResult outcome, Dictionary> slots, @@ -669,9 +673,11 @@ public sealed class SqlPollReader : IDisposable // KeyValue indexes rows by the key column; WideRow's members all read the one selected row, so the // where-column is not even in the result set (design §5.3). Both selections happen ONCE per group — // they are group-wide facts, and doing them per member would report a contract violation N times. + HashSet? duplicatedKeys = null; var byKey = outcome.Succeeded && plan.Model == SqlTagModel.KeyValue - ? IndexByKey(plan, outcome) + ? IndexByKey(plan, outcome, out duplicatedKeys) : null; + var refusedSome = false; var wideRow = outcome.Succeeded && plan.Model == SqlTagModel.WideRow ? SelectWideRow(plan, outcome) : null; @@ -681,26 +687,45 @@ public sealed class SqlPollReader : IDisposable // Members may repeat (two tags on one keyValue, or the same ref twice) — every occurrence is fed. if (!slots.TryGetValue(member, out var positions)) continue; - var snapshot = outcome.Succeeded - ? MapMember(plan, member, outcome, byKey, wideRow, readAt) - : new DataValueSnapshot(null, outcome.FailureStatus, null, readAt); + DataValueSnapshot snapshot; + if (!outcome.Succeeded) + { + snapshot = new DataValueSnapshot(null, outcome.FailureStatus, null, readAt); + } + else if (duplicatedKeys is not null && member.KeyValue is { } key && duplicatedKeys.Contains(key)) + { + // More than one row for THIS tag's key: its selector is ambiguous — the same rule a where-pair + // follows — so it is a configuration error for this key's tags only. Every other key in the + // group is unaffected, whatever else is present or absent (R06:S-1 review I-1: it used to be + // last-row-wins Good, or the whole group Bad, depending on an unrelated key's absence). + snapshot = new DataValueSnapshot(null, SqlStatusCodes.BadConfigurationError, null, readAt); + refusedSome = true; + } + else + { + snapshot = MapMember(plan, member, outcome, byKey, wideRow, readAt); + } foreach (var position in positions) results[position] = snapshot; } + + return refusedSome; } /// - /// Builds the key → row index a key-value plan slices through. Later rows win, so a source that - /// breaks the one-row-per-key contract (design §3.6) still yields a deterministic value; the - /// violation is reported through a rate-limited warning rather than silently. + /// Builds the key → row index a key-value plan slices through, and the set of keys that came back more + /// than once — a source breaking the one-row-per-key contract (design §3.6). Those keys' tags publish + /// ; the violation is also reported through a + /// per-plan rate-limited warning. /// - private Dictionary IndexByKey(SqlQueryPlan plan, GroupResult outcome) + private Dictionary IndexByKey( + SqlQueryPlan plan, GroupResult outcome, out HashSet? duplicatedKeys) { + duplicatedKeys = null; var index = new Dictionary(StringComparer.Ordinal); if (plan.KeyColumn is null || !outcome.ColumnIndex.TryGetValue(plan.KeyColumn, out var keyAt)) return index; - var duplicated = 0; foreach (var row in outcome.Rows) { // A NULL key cell indexes nothing: Convert.ToString would fold it to "", which would then be @@ -711,20 +736,16 @@ public sealed class SqlPollReader : IDisposable // (an INTEGER station id, say) — the provider coerces on the way in, so match on the way out. var key = Convert.ToString(keyCell, CultureInfo.InvariantCulture); if (key is null) continue; - if (!index.TryAdd(key, row)) - { - index[key] = row; - duplicated++; - } + if (!index.TryAdd(key, row)) (duplicatedKeys ??= new HashSet(StringComparer.Ordinal)).Add(key); } - if (duplicated > 0 && ShouldWarnAboutContract()) + if (duplicatedKeys is not null && ShouldWarn("dupkey:" + plan.SqlText)) { _logger.LogWarning( - "Sql poll source '{Table}' returned more than one row for {Count} key(s) in one poll; the " + - "last row wins. The key-value model requires one row per key — point the tag at a " + - "current-values table or an operator-authored latest-per-key view.", - plan.Members[0].Table, duplicated); + "Sql poll source '{Table}' returned more than one row for {Count} key(s) in one poll (e.g. " + + "'{Example}'); those keys' tags publish BadConfigurationError. The key-value model requires one " + + "row per key — point the tag at a current-values table or an operator-authored latest-per-key view.", + plan.Members[0].Table, duplicatedKeys.Count, duplicatedKeys.First()); } return index; @@ -759,7 +780,7 @@ public sealed class SqlPollReader : IDisposable private object?[]? SelectWideRow(SqlQueryPlan plan, GroupResult outcome) { if (outcome.Rows.Count == 0) return null; - if (outcome.Rows.Count > 1 && ShouldWarnAboutContract()) + if (outcome.Rows.Count > 1 && ShouldWarn("multirow:" + plan.SqlText)) { var first = plan.Members[0]; var selector = string.IsNullOrWhiteSpace(first.RowSelectorColumn) @@ -920,15 +941,6 @@ public sealed class SqlPollReader : IDisposable } } - /// Rate-limits the source-contract warning so a misconfigured table cannot flood the log. - private bool ShouldWarnAboutContract() - { - var now = Stopwatch.GetTimestamp(); - var last = Interlocked.Read(ref _lastContractWarningTicks); - if (last != 0 && Stopwatch.GetElapsedTime(last, now) < ContractWarningInterval) return false; - return Interlocked.CompareExchange(ref _lastContractWarningTicks, now, last) == last; - } - /// One group's outcome: either a materialised result set, or the status its members inherit. private sealed class GroupResult { @@ -993,11 +1005,11 @@ public sealed class SqlPollReader : IDisposable /// One poll's snapshots plus the answer evidence they cannot carry themselves. /// One snapshot per requested reference, at the same index. -/// -/// Groups the database answered with more rows than their plan can use; their members carry -/// . +/// +/// Groups the database ANSWERED but whose result the reader refused in whole or part — an over-cap result +/// set, or a duplicated key. Those members carry . /// -public sealed record SqlPollResult(IReadOnlyList Snapshots, int RowCapExceededGroups); +public sealed record SqlPollResult(IReadOnlyList Snapshots, int AnsweredRefusalGroups); /// /// The OPC UA status codes the Sql driver publishes, and the quality-class predicates its tests read diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs index 333a762c..ec32c5d1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs @@ -56,11 +56,13 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// reader's map from result-set ordinal to source column. /// /// -/// The most rows this plan can USE — its distinct key count (key-value) or 1 (a where-pair wide row); -/// when the statement already limits itself (topByTimestamp). -/// asks for RowCap + 1, and the reader stops there: receiving the extra row proves the selector is -/// ambiguous (a history table, a non-unique where column) and the group publishes -/// BadConfigurationError instead of materialising every matching row on every poll (R06:S-1). +/// The most rows the reader will attribute before refusing the whole group — twice the distinct key count +/// (key-value: headroom so stray duplicates are attributed per key) or 1 (a where-pair wide row: one +/// selector, so any second row is the group's ambiguity); when the statement +/// already limits itself (topByTimestamp). asks for RowCap + 1, and the +/// reader stops there: receiving the extra row means a history/events table or a non-unique where column, +/// and the group publishes BadConfigurationError instead of materialising every matching row on +/// every poll (R06:S-1). /// public sealed record SqlQueryPlan( SqlTagModel Model, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.IntegrationTests/SqlServerReadTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.IntegrationTests/SqlServerReadTests.cs index ff6271bc..73339481 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.IntegrationTests/SqlServerReadTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.IntegrationTests/SqlServerReadTests.cs @@ -116,6 +116,69 @@ public sealed class SqlServerReadTests } } + /// + /// Review I-1, live: one stray duplicate row in a key-value table fails ONLY that key's tag; the other + /// key in the same group (one capped SELECT TOP (5) … IN (@k0, @k1)) reads Good — the positive + /// control — and the driver stays Healthy. + /// + [Fact] + public async Task ReadAsync_realSqlServer_aDuplicatedKeyFailsOnlyItsOwnTag() + { + SkipUnlessLive(); + const string table = "RowCapCurrent"; + var qualified = $"{SqlPollServerFixture.Schema}.{table}"; + + await using (var arrange = await _sql.OpenInspectionConnectionAsync()) + { + await using var command = arrange.CreateCommand(); + command.CommandText = $""" + IF OBJECT_ID(N'{qualified}') IS NOT NULL DROP TABLE {qualified}; + CREATE TABLE {qualified} ( + {SqlPollServerFixture.KeyColumn} nvarchar(64) NOT NULL, + {SqlPollServerFixture.ValueColumn} float NULL, + {SqlPollServerFixture.TimestampColumn} datetime2 NOT NULL); + INSERT INTO {qualified} VALUES + (N'Pump7', 1.0, '2026-07-24T10:00:00'), + (N'Pump7', 2.0, '2026-07-24T10:00:01'), + (N'Pump8', 8.5, '2026-07-24T10:00:00'); + """; + await command.ExecuteNonQueryAsync(TestContext.Current.CancellationToken); + } + + try + { + await using var driver = await StartAsync( + KeyValueOn("Sql/Current/Pump7", "Pump7", table), + KeyValueOn("Sql/Current/Pump8", "Pump8", table)); + + var snapshots = await driver.ReadAsync( + ["Sql/Current/Pump7", "Sql/Current/Pump8"], TestContext.Current.CancellationToken); + + snapshots[0].StatusCode.ShouldBe(SqlStatusCodes.BadConfigurationError); + snapshots[1].StatusCode.ShouldBe(SqlStatusCodes.Good); + Convert.ToDouble(snapshots[1].Value).ShouldBe(8.5); + driver.GetHealth().State.ShouldBe(DriverState.Healthy); + } + finally + { + await using var cleanup = await _sql.OpenInspectionConnectionAsync(); + await using var drop = cleanup.CreateCommand(); + drop.CommandText = $"DROP TABLE IF EXISTS {qualified}"; + await drop.ExecuteNonQueryAsync(CancellationToken.None); + } + } + + private static RawTagEntry KeyValueOn(string rawPath, string key, string table) => Tag(rawPath, new JsonObject + { + ["driver"] = "Sql", + ["model"] = "KeyValue", + ["table"] = SqlPollServerFixture.Qualified(table), + ["keyColumn"] = SqlPollServerFixture.KeyColumn, + ["keyValue"] = key, + ["valueColumn"] = SqlPollServerFixture.ValueColumn, + ["timestampColumn"] = SqlPollServerFixture.TimestampColumn, + }); + // ---- key-value model ---- /// The plan's headline case: a seeded key-value row round-trips as a Good snapshot. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerTests.cs index 058ea516..ef7661b9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerTests.cs @@ -32,8 +32,9 @@ public class SqlGroupPlannerTests // ---- R06:S-1: the row cap ---- /// - /// A key-value plan asks for one row more than it has distinct keys, and a where-pair wide-row plan for - /// two: enough to SEE an ambiguous selector, never enough to materialise a history table every poll. + /// A key-value plan reads up to twice its distinct keys (so stray duplicates are attributed per key) and + /// asks for one row more; a where-pair wide-row plan asks for two. Enough to SEE an ambiguous selector, + /// never enough to materialise a history table every poll. /// The topByTimestamp form keeps its single-row limit and needs no cap. /// [Fact] @@ -49,8 +50,8 @@ public class SqlGroupPlannerTests ], new SqlServerDialect()).ToList(); var kv = plans.Single(p => p.Model == SqlTagModel.KeyValue); - kv.RowCap.ShouldBe(2); - kv.SqlText.ShouldStartWith("SELECT TOP (3) "); + kv.RowCap.ShouldBe(4); // 2 distinct keys, with 2x headroom for per-key attribution + kv.SqlText.ShouldStartWith("SELECT TOP (5) "); var wherePair = plans.Single(p => p.Parameters.Count == 1 && p.Model == SqlTagModel.WideRow); wherePair.RowCap.ShouldBe(1); @@ -90,7 +91,7 @@ public class SqlGroupPlannerTests new SqlServerDialect()); plans[0].SqlText.ShouldBe( - "SELECT TOP (2) [tag_name], [num_value], [sample_ts] FROM [dbo].[TagValues] WHERE [tag_name] IN (@k0)"); + "SELECT TOP (3) [tag_name], [num_value], [sample_ts] FROM [dbo].[TagValues] WHERE [tag_name] IN (@k0)"); plans[0].ParameterNames.ShouldBe(new[] { "@k0" }); plans[0].SelectedColumns.ShouldBe(new[] { "tag_name", "num_value", "sample_ts" }); plans[0].Model.ShouldBe(SqlTagModel.KeyValue); @@ -107,7 +108,7 @@ public class SqlGroupPlannerTests new SqlServerDialect()); plans[0].SqlText.ShouldBe( - "SELECT TOP (2) [tag_name], [num_value] FROM [dbo].[TagValues] WHERE [tag_name] IN (@k0)"); + "SELECT TOP (3) [tag_name], [num_value] FROM [dbo].[TagValues] WHERE [tag_name] IN (@k0)"); plans[0].TimestampColumn.ShouldBeNull(); } @@ -233,7 +234,7 @@ public class SqlGroupPlannerTests KvTag("B", "TagValues", "tag_name", "Line1.Temp", "num_value", null), ], new ColonMarkerDialect()).ToList(); - plans[0].SqlText.ShouldEndWith("IN (:k0, :k1) LIMIT 3"); + plans[0].SqlText.ShouldEndWith("IN (:k0, :k1) LIMIT 5"); plans[0].SqlText.ShouldNotContain("@"); plans[0].ParameterNames.ShouldBe(new[] { ":k0", ":k1" }); } @@ -456,7 +457,7 @@ public class SqlGroupPlannerTests // A column name is an identifier — it cannot be parameterized, so the guarantee is that it is // bracket-quoted with ] doubled, leaving one (nonexistent) identifier rather than executable SQL. plans[0].SqlText.ShouldBe( - "SELECT TOP (2) [tag_name], [v]] ; DROP TABLE Users --] FROM [dbo].[T] WHERE [tag_name] IN (@k0)"); + "SELECT TOP (3) [tag_name], [v]] ; DROP TABLE Users --] FROM [dbo].[T] WHERE [tag_name] IN (@k0)"); } // ---- contract edges ---- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs index d26bd0b6..19d28944 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs @@ -272,25 +272,59 @@ public sealed class SqlPollReaderTests snapshots[0].SourceTimestampUtc!.Value.ShouldBe(snapshots[0].ServerTimestampUtc); } + /// + /// R06:S-1 review I-1. One stray duplicate row is attributed to ITS key: that key's tags publish + /// and every other key in the group reads normally. + /// It used to be last-row-wins Good, or the whole group Bad, depending on whether an UNRELATED key was + /// absent. Both variants are pinned: every other key present, and another key absent. The other keys' + /// Good / BadNoData are the positive controls. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task ReadAsync_aDuplicatedKey_failsOnlyThatKeysTags_whateverElseIsAbsent(bool withAnAbsentKey) + { + using var fixture = new SqlitePollFixture(); + var logger = new RecordingLogger(); + fixture.Execute( + $"INSERT INTO {SqlitePollFixture.KeyValueTable} " + + $"({SqlitePollFixture.KeyColumn}, {SqlitePollFixture.ValueColumn}, {SqlitePollFixture.TimestampColumn}) " + + $"VALUES ('{SqlitePollFixture.PresentKey}', 99.0, '2026-07-24T10:00:09Z')"); + var tags = new List + { + KvTag("Speed", SqlitePollFixture.PresentKey), + KvTag("Speed2", SqlitePollFixture.PresentKey), // a second tag on the duplicated key + KvTag("Pressure", SqlitePollFixture.SecondPresentKey), + }; + if (withAnAbsentKey) tags.Add(KvTag("Gone", SqlitePollFixture.AbsentKey)); + var reader = Reader(fixture, logger, [.. tags]); + + var snapshots = await reader.ReadAsync([.. tags.Select(t => t.Name)], CancellationToken.None); + + snapshots[0].StatusCode.ShouldBe(SqlStatusCodes.BadConfigurationError); + snapshots[1].StatusCode.ShouldBe(SqlStatusCodes.BadConfigurationError); + snapshots[2].StatusCode.ShouldBe(SqlStatusCodes.Good); + snapshots[2].Value.ShouldBe(SqlitePollFixture.SecondPresentValue); + if (withAnAbsentKey) snapshots[3].StatusCode.ShouldBe(SqlStatusCodes.BadNoData); + + var warning = logger.Entries.ShouldHaveSingleItem(); + warning.Message.ShouldContain(SqlitePollFixture.PresentKey); + } + + /// The driver counts a duplicated-key refusal as the database answering, like an over-cap one. [Fact] - public async Task ReadAsync_whenASourceViolatesOneRowPerKey_takesTheLastRowDeterministically() + public async Task ReadDetailedAsync_aDuplicatedKey_isReportedAsAnAnsweredRefusal() { using var fixture = new SqlitePollFixture(); fixture.Execute( $"INSERT INTO {SqlitePollFixture.KeyValueTable} " + $"({SqlitePollFixture.KeyColumn}, {SqlitePollFixture.ValueColumn}, {SqlitePollFixture.TimestampColumn}) " + $"VALUES ('{SqlitePollFixture.PresentKey}', 99.0, '2026-07-24T10:00:09Z')"); - // Two keys, one absent: the duplicate fits inside the row cap (2), so it is not provably a history - // table — the last-row-wins rule still applies there (the cap test below covers the other side). - var reader = Reader(fixture, - KvTag("Speed", SqlitePollFixture.PresentKey), KvTag("Gone", SqlitePollFixture.AbsentKey)); + var reader = Reader(fixture, KvTag("Speed", SqlitePollFixture.PresentKey)); + var clean = Reader(fixture, KvTag("Pressure", SqlitePollFixture.SecondPresentKey)); - var snapshots = await reader.ReadAsync(["Speed", "Gone"], CancellationToken.None); - - // Design §3.6: the contract is one row per key; when a source breaks it the reader is at least - // deterministic — last occurrence in reader order — rather than silently arbitrary. - snapshots[0].Value.ShouldBe(99.0); - snapshots[1].StatusCode.ShouldBe(SqlStatusCodes.BadNoData); + (await reader.ReadDetailedAsync(["Speed"], CancellationToken.None)).AnsweredRefusalGroups.ShouldBe(1); + (await clean.ReadDetailedAsync(["Pressure"], CancellationToken.None)).AnsweredRefusalGroups.ShouldBe(0); } ///