From 241318e7e5d1049ca66ccc026e6d6a8450c01498 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 18:08:39 -0500 Subject: [PATCH] feat(deploy): reject Sql Query tags naming undeclared queries; warn on Writable/writable disagreement (lmxopcua#781) Item 5: DraftValidator now emits SqlQueryNotDeclared (always blocking) for a Query tag whose query is absent from the merged driver config's queries map, via the driver's own tag parse. Item 3: StartDeployment appends a non-blocking warning to the accepted message when the Tag entity's AccessLevel and the Sql blob's writable flag disagree. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- .../Validation/DraftValidator.cs | 1 + .../Validation/SqlTagDeployChecks.cs | 107 ++++++++++++++++++ .../ZB.MOM.WW.OtOpcUa.Configuration.csproj | 3 + .../AdminOperations/AdminOperationsActor.cs | 7 ++ .../DraftValidatorSqlQueryTests.cs | 76 +++++++++++++ .../AdminOperationsActorTagConfigGateTests.cs | 45 ++++++++ 6 files changed, 239 insertions(+) create mode 100644 src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlTagDeployChecks.cs create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlQueryTests.cs diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs index 96a25da58..ef76b5da2 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs @@ -41,6 +41,7 @@ public static class DraftValidator ValidateEquipReferenceResolution(draft, errors); ValidateCalculationTags(draft, errors); ValidateSqlConnectionStringNotPersisted(draft, errors); + errors.AddRange(SqlTagDeployChecks.UndeclaredQueryErrors(draft)); ValidateMTConnectSingleDevice(draft, errors); ValidateNodeAclScopes(draft, errors); return errors; diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlTagDeployChecks.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlTagDeployChecks.cs new file mode 100644 index 000000000..bdabcc4ad --- /dev/null +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlTagDeployChecks.cs @@ -0,0 +1,107 @@ +using System.Text.Json; +using ZB.MOM.WW.OtOpcUa.Commons.Types; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; + +namespace ZB.MOM.WW.OtOpcUa.Configuration.Validation; + +/// +/// Deploy-gate checks for Sql tags that need a fact outside the tag's own blob (lmxopcua#781 items 3 and 5). +/// Both go through — the driver's own runtime parse — so a +/// blob the driver would refuse anyway (already reported by the per-tag inspector) is never double-reported +/// here, and nothing here can be stricter than the runtime. +/// +public static class SqlTagDeployChecks +{ + /// Error code for a Query tag naming a query its driver instance does not declare. + public const string UndeclaredQueryCode = "SqlQueryNotDeclared"; + + /// + /// Item 5. A Query-model tag whose query is not a key of the driver instance's + /// queries map is skipped by the driver at Initialize (BadNodeIdUnknown, reason on /hosts). The + /// queries map is read from the same merged config the driver binds + /// (: DriverConfig plus, for a single-device instance, the + /// DeviceConfig), keys matched ordinally as the driver does. + /// + /// The snapshot being validated. + /// One error per offending tag; empty when every Query tag names a declared query. + public static IReadOnlyList UndeclaredQueryErrors(DraftSnapshot draft) + { + var errors = new List(); + foreach (var (driver, devices) in SqlInstances(draft)) + { + HashSet? declared = null; + foreach (var tag in draft.Tags) + { + if (!devices.Any(d => string.Equals(d.DeviceId, tag.DeviceId, StringComparison.Ordinal))) continue; + if (!SqlEquipmentTagParser.TryParse(tag.TagConfig, tag.Name, out var def) + || def.Model != SqlTagModel.Query) continue; + declared ??= DeclaredQueries(driver, devices); + if (declared.Contains(def.QueryName!)) continue; + errors.Add(new(UndeclaredQueryCode, + $"Sql tag '{tag.TagId}' reads named query '{def.QueryName}', which Sql driver instance " + + $"'{driver.DriverInstanceId}' does not declare under 'queries' — the driver would skip the tag " + + "and it would read BadNodeIdUnknown. Declare the query in the driver config or fix the tag's 'query'.", + tag.TagId)); + } + } + + return errors; + } + + /// + /// Item 3. The node's write permission needs BOTH the Tag entity's AccessLevel (ReadWrite) and the + /// Sql blob's "writable": true; when they disagree the tag is silently read-only. A warning, never + /// an error: both read-only combinations are legal states, just usually not the intended one. + /// + /// The snapshot being validated. + /// One human-readable warning per disagreeing tag. + public static IReadOnlyList WritableMismatchWarnings(DraftSnapshot draft) + { + var warnings = new List(); + foreach (var (_, devices) in SqlInstances(draft)) + { + foreach (var tag in draft.Tags) + { + if (!devices.Any(d => string.Equals(d.DeviceId, tag.DeviceId, StringComparison.Ordinal))) continue; + if (!SqlEquipmentTagParser.TryParse(tag.TagConfig, tag.Name, out var def)) continue; + var entityWritable = tag.AccessLevel == TagAccessLevel.ReadWrite; + if (entityWritable == def.Writable) continue; + warnings.Add(entityWritable + ? $"tag '{tag.TagId}': Writable flag is on but the Sql TagConfig lacks \"writable\": true — the driver will refuse writes to it." + : $"tag '{tag.TagId}': Sql TagConfig says \"writable\": true but the tag's Writable flag is off — the node will be read-only."); + } + } + + return warnings; + } + + private static IEnumerable<(DriverInstance Driver, List Devices)> SqlInstances(DraftSnapshot draft) + { + foreach (var driver in draft.DriverInstances) + { + if (!string.Equals(driver.DriverType, DriverTypeNames.Sql, StringComparison.Ordinal)) continue; + yield return (driver, draft.Devices + .Where(d => string.Equals(d.DriverInstanceId, driver.DriverInstanceId, StringComparison.Ordinal)) + .ToList()); + } + } + + private static HashSet DeclaredQueries(DriverInstance driver, List devices) + { + var set = new HashSet(StringComparer.Ordinal); + var merged = DriverDeviceConfigMerger.Merge( + driver.DriverConfig, devices.Select(d => new DriverDeviceConfigMerger.DeviceRow(d.Name, d.DeviceConfig)).ToList(), []); + using var doc = JsonDocument.Parse(merged); + foreach (var p in doc.RootElement.EnumerateObject()) + { + if (!string.Equals(p.Name, "queries", StringComparison.OrdinalIgnoreCase) + || p.Value.ValueKind != JsonValueKind.Object) continue; + foreach (var q in p.Value.EnumerateObject()) set.Add(q.Name); + } + + return set; + } +} diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/ZB.MOM.WW.OtOpcUa.Configuration.csproj b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/ZB.MOM.WW.OtOpcUa.Configuration.csproj index cea5ae4bc..e153ea358 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/ZB.MOM.WW.OtOpcUa.Configuration.csproj +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/ZB.MOM.WW.OtOpcUa.Configuration.csproj @@ -44,6 +44,9 @@ + + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs index 69e878db4..d4de355e3 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs @@ -465,6 +465,12 @@ public sealed class AdminOperationsActor : ReceiveActor _log.Warning("StartDeployment: TagConfig warning (Warn mode, non-blocking): {Warning}", x.Text); } + // lmxopcua#781 item 3: always non-blocking (independent of TagConfigValidationMode) — the Tag + // entity's Writable flag and the Sql blob's "writable" disagree. + var writableWarnings = SqlTagDeployChecks.WritableMismatchWarnings(draft); + foreach (var w in writableWarnings) + _log.Warning("StartDeployment: Sql writable mismatch (non-blocking): {Warning}", w); + if (errors.Count > 0) { var summary = string.Join("; ", errors.Select(e => $"[{e.Code}] {e.Message}")); @@ -547,6 +553,7 @@ public sealed class AdminOperationsActor : ReceiveActor var messageParts = new List(); if (advisory is not null) messageParts.Add(advisory); messageParts.AddRange(tagConfigWarnings.Select(x => x.Text)); + messageParts.AddRange(writableWarnings); var acceptedMessage = messageParts.Count > 0 ? string.Join("; ", messageParts) : null; return new ComposeOutcome( diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlQueryTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlQueryTests.cs new file mode 100644 index 000000000..62254d053 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlQueryTests.cs @@ -0,0 +1,76 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Configuration.Validation; + +namespace ZB.MOM.WW.OtOpcUa.Configuration.Tests; + +/// lmxopcua#781 items 5 and 3: Sql deploy checks that need the driver config / Tag entity. +[Trait("Category", "Unit")] +public sealed class DraftValidatorSqlQueryTests +{ + private const string Code = SqlTagDeployChecks.UndeclaredQueryCode; + private const string Declared = + """{"queries":{"line":{"sql":"SELECT 1 AS v","keyColumns":[]}}}"""; + + private static string QueryTag(string query, bool? writable = null) => + $$"""{"model":"Query","query":"{{query}}","column":"v"{{(writable is null ? "" : $",\"writable\":{writable.ToString()!.ToLowerInvariant()}")}}}"""; + + private static DraftSnapshot Draft(string driverConfig, string tagConfig, + TagAccessLevel access = TagAccessLevel.Read, string deviceConfig = "{}") => new() + { + GenerationId = 1, ClusterId = "c", + DriverInstances = [new DriverInstance + { DriverInstanceId = "di", ClusterId = "c", Name = "sql", DriverType = "Sql", DriverConfig = driverConfig }], + Devices = [new Device { DeviceId = "dev", DriverInstanceId = "di", Name = "d", DeviceConfig = deviceConfig }], + Tags = [new Tag { TagId = "t1", DeviceId = "dev", Name = "t", DataType = "Int32", + AccessLevel = access, TagConfig = tagConfig }], + }; + + [Fact] + public void Query_tag_naming_an_undeclared_query_is_a_deploy_error() + { + var errors = DraftValidator.Validate(Draft(Declared, QueryTag("nope"))); + errors.ShouldContain(e => e.Code == Code && e.Context == "t1" && e.Message.Contains("'nope'")); + } + + [Fact] + public void Query_tag_with_no_queries_map_at_all_is_a_deploy_error() + => DraftValidator.Validate(Draft("{}", QueryTag("line"))).ShouldContain(e => e.Code == Code); + + [Fact] + public void Positive_control_declared_query_is_quiet() + => DraftValidator.Validate(Draft(Declared, QueryTag("line"))).ShouldNotContain(e => e.Code == Code); + + [Fact] + public void Query_declared_on_the_single_device_config_counts_as_declared() + => DraftValidator.Validate(Draft("{}", QueryTag("line"), deviceConfig: Declared)) + .ShouldNotContain(e => e.Code == Code); + + [Fact] + public void Query_names_are_ordinal() + => DraftValidator.Validate(Draft(Declared, QueryTag("LINE"))).ShouldContain(e => e.Code == Code); + + [Fact] + public void Non_query_models_are_not_checked() + => DraftValidator.Validate(Draft("{}", + """{"model":"KeyValue","table":"t","keyColumn":"k","keyValue":"a","valueColumn":"v"}""")) + .ShouldNotContain(e => e.Code == Code); + + [Theory] + [InlineData(TagAccessLevel.ReadWrite, false, true)] + [InlineData(TagAccessLevel.Read, true, true)] + [InlineData(TagAccessLevel.ReadWrite, true, false)] + [InlineData(TagAccessLevel.Read, false, false)] + public void Writable_flag_disagreement_warns_and_agreement_is_quiet(TagAccessLevel access, bool writable, bool warns) + { + var warnings = SqlTagDeployChecks.WritableMismatchWarnings(Draft(Declared, QueryTag("line", writable), access)); + (warnings.Count == 1).ShouldBe(warns); + if (warns) warnings[0].ShouldContain("t1"); + } + + [Fact] + public void Writable_mismatch_is_never_a_validation_error() + => DraftValidator.Validate(Draft(Declared, QueryTag("line", true), TagAccessLevel.Read)).ShouldBeEmpty(); +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorTagConfigGateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorTagConfigGateTests.cs index f9689d47c..8c078542b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorTagConfigGateTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorTagConfigGateTests.cs @@ -124,4 +124,49 @@ public sealed class AdminOperationsActorTagConfigGateTests : ControlPlaneActorTe coordinator.ExpectMsg(PresenceBudget); ExpectMsg(PresenceBudget).Outcome.ShouldBe(StartDeploymentOutcome.Accepted); } + + // lmxopcua#781 item 3 — through the actor, in Error mode too: the writable mismatch is a warning, never a reject. + private const string SqlQueries = "{\"queries\":{\"line\":{\"sql\":\"SELECT 1 AS v\",\"keyColumns\":[]}}}"; + + private StartDeploymentResult DeploySql(bool writableInBlob, TagAccessLevel access) + { + var dbFactory = NewInMemoryDbFactory(); + using (var db = dbFactory.CreateDbContext()) + { + db.DriverInstances.Add(new DriverInstance + { DriverInstanceId = "d-1", ClusterId = "", Name = "drv", DriverType = "Sql", DriverConfig = SqlQueries }); + db.Devices.Add(new Device { DeviceId = "dev-1", DriverInstanceId = "d-1", Name = "dev", DeviceConfig = "{}" }); + db.Tags.Add(new Tag + { + TagId = "tag-1", DeviceId = "dev-1", Name = "flow", DataType = "Int32", AccessLevel = access, + TagConfig = "{\"model\":\"Query\",\"query\":\"line\",\"column\":\"v\",\"writable\":" + + (writableInBlob ? "true" : "false") + "}", + }); + db.SaveChanges(); + } + var coordinator = CreateTestProbe("coord"); + var actor = Sys.ActorOf(AdminOperationsActor.Props( + dbFactory, coordinator.Ref, Enumerable.Empty(), TagConfigValidationMode.Error)); + actor.Tell(new StartDeployment("joe", CorrelationId.NewId())); + coordinator.ExpectMsg(PresenceBudget); + return ExpectMsg(PresenceBudget); + } + + [Fact] + public void Sql_writable_mismatch_is_accepted_with_a_warning_even_in_error_mode() + { + var reply = DeploySql(writableInBlob: true, TagAccessLevel.Read); + reply.Outcome.ShouldBe(StartDeploymentOutcome.Accepted); + reply.Message.ShouldNotBeNull(); + reply.Message.ShouldContain("tag-1"); + reply.Message.ShouldContain("writable"); + } + + [Fact] + public void Sql_writable_agreement_adds_no_warning() + { + var reply = DeploySql(writableInBlob: true, TagAccessLevel.ReadWrite); + reply.Outcome.ShouldBe(StartDeploymentOutcome.Accepted); + reply.Message.ShouldBeNull(); + } }