From 0e9d977f9a7d9182592c04073fccda2c3bef5a16 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 01:38:13 -0400 Subject: [PATCH] test(raw): write the browse-commit parity guard the mapper's comment cites (#555) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RawBrowseCommitMapper.BuildTagConfig's unknown-driver fallback claimed to be guarded by RawBrowseCommitMapperParityTests, which did not exist anywhere under tests/. The claim it made — "every BROWSABLE driver is handled above" — is the exact claim that stopped being true when SqlDriverBrowser registered (deferment.md G-6), so the comment was asserting the invariant instead of pinning it. The guard now exists and enumerates DriverTypeNames.All: - Browsability's bespoke arm is DERIVED from production, not hand-listed: the test runs AddAdminUI() and reads back every registered IDriverBrowser's DriverType — the same set BrowserSessionService.CanBrowse indexes. A driver that gains or loses a bespoke browser moves the guard with it. - The universal arm (DiscoveryDriverBrowser -> ITagDiscovery) is NOT derivable from the AdminUI assembly: it references driver Contracts/Browser projects, never the runtime drivers, and IDriverFactory is composed in the Host. Covered instead by requiring every declared driver type to be typed or explicitly exempted with a reason (Modbus, Calculation), with a third fact mechanically refuting any exemption the derived arm contradicts — the Sql failure mode. Falsified three ways, each red naming the offender: disabling the Sql branch trips the derived-arm fact; disabling the MTConnect branch trips the enumeration fact (the universal-arm half, which no bespoke registration would have caught); adding Sql to the exemption list trips the contradiction fact. Comment corrected to state what the test actually asserts, including the residual trust the universal arm leaves. --- .../Uns/RawBrowseCommitMapper.cs | 11 +- .../Uns/RawBrowseCommitMapperParityTests.cs | 273 ++++++++++++++++++ 2 files changed, 281 insertions(+), 3 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperParityTests.cs diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs index 17e72aad..024c251a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs @@ -267,9 +267,14 @@ public static class RawBrowseCommitMapper return BuildSqlTagConfig(address, addressFields); // Unknown/flat-address driver (e.g. Modbus is not browsable): record the reference under a generic - // "address" key so nothing is lost. Every BROWSABLE driver is handled above — guarded by - // RawBrowseCommitMapperParityTests, which enumerates DriverTypeNames.All rather than trusting - // this comment. + // "address" key so nothing is lost. RawBrowseCommitMapperParityTests enumerates DriverTypeNames.All + // and holds this line to two claims rather than to the assertion the comment used to make: + // - a driver type with a bespoke IDriverBrowser registered by AddAdminUI() — read back from the + // real registration, not a list — must NOT reach here; and + // - any type that does reach here must be named in that test's GenericKeyExemptions with a reason. + // The universal DiscoveryDriverBrowser's arm is not derivable from the AdminUI assembly (it needs + // the runtime drivers + the Host's IDriverFactory), so it is covered by the exemption list rather + // than mechanically — see the test's remarks for the residual trust that leaves (Gitea #555). return WriteSingleKey("address", address); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperParityTests.cs new file mode 100644 index 00000000..b8a33313 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperParityTests.cs @@ -0,0 +1,273 @@ +using System.Text.Json.Nodes; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; +using ZB.MOM.WW.OtOpcUa.Commons.Browsing; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Mqtt; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; +using ZB.MOM.WW.Secrets.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; + +/// +/// The guard 's fallback comment cites (Gitea #555): +/// every driver type in either gets a dedicated branch in +/// BuildTagConfig, or appears in with a written reason. +/// +/// Why the generic key is a defect for a browsable driver. The fallback writes +/// {"address": "<leaf>"}. A driver whose typed editor reads a different key — MTConnect's +/// fullName, OPC UA Client's nodeId, Sql's table/columnName/rowSelector +/// — then opens that committed tag with an EMPTY address field and blanks it on save. That is +/// exactly why the MTConnect branch was added, and Sql shipped with the same bug unnoticed +/// (deferment.md G-6) because the fallback's comment asserted "every BROWSABLE driver is handled +/// above" while SqlDriverBrowser was already registered. +/// +/// +/// Enumeration, not InlineData. RawTagConfigGateTests covers the same mapper through +/// hand-written per-driver cases, which cannot notice a driver type that nobody wrote a case for. This +/// class walks , so a newly-registered driver is triaged the first time +/// the suite runs — the idiom DriverFormMapParityTests / DriverDispatchMapParityTests use. +/// +/// +/// +/// +/// Browsability derivation, and its residual trust. Production decides browsability in +/// BrowserSessionService.CanBrowse, which is a disjunction of two arms: +/// +/// +/// +/// a bespoke registered for the driver type — derived here for +/// real, by running the production AddAdminUI() registration and reading back every +/// registered browser's . Nothing is hand-listed, so a +/// driver that gains a bespoke browser starts demanding a branch on the next run, and one that +/// loses its browser stops. +/// +/// +/// the universal DiscoveryDriverBrowser, whose CanBrowse constructs the driver +/// through IDriverFactory and asks whether it is +/// ITagDiscovery { SupportsOnlineDiscovery: true }. That arm is not derivable from this +/// assembly: the AdminUI references driver Contracts and Browser projects, never +/// the runtime drivers, and IDriverFactory is composed in the Host — so the types that answer +/// "yes" (AbCip, TwinCAT, FOCAS, MTConnect) have no representative in this project's output at all. +/// Dragging the Host + every runtime driver into an AdminUI unit-test project to reach them would +/// trade this residual trust for a project-reference list that is just as hand-authored. +/// +/// +/// +/// So the universal arm is covered the only honest way available here: by requiring every driver +/// type to be typed or explicitly exempted, and by making each exemption a positive claim ("this driver +/// cannot be browsed") that a reader can falsify. +/// then mechanically refutes any exemption the derived arm contradicts — which is the failure mode that +/// actually occurred with Sql. +/// +/// +[Trait("Category", "Unit")] +public sealed class RawBrowseCommitMapperParityTests +{ + /// The browse leaf's reference, as the commit path passes it (BrowseNode.NodeId). + private const string Reference = "Speed"; + + /// + /// THE #555 guard, over the arm that can be derived: a driver type with a registered bespoke + /// browser must commit to a typed blob. + /// + [Fact] + public void Every_registered_bespoke_browser_driver_type_gets_a_typed_tag_config() + { + var browsable = RegisteredBespokeBrowserDriverTypes(); + + // Falsifiability floor: an empty set would make this pass while asserting nothing, which is how a + // broken registration read would look. Four browsers are registered today (OpcUaClient, Galaxy, + // Sql, Mqtt). + browsable.ShouldNotBeEmpty( + "no IDriverBrowser came back from AddAdminUI() — the registration read is broken, not the mapper"); + + var untyped = browsable + .Where(CommitsToTheGenericAddressKey) + .OrderBy(t => t, StringComparer.Ordinal) + .ToList(); + + untyped.ShouldBeEmpty( + "these driver types have a registered browse session but no dedicated RawBrowseCommitMapper." + + "BuildTagConfig branch, so a browse-committed tag lands under the generic \"address\" key: " + + $"{string.Join(", ", untyped)}. Their typed tag editor reads a different key, so the tag " + + "opens with an empty address and is blanked on save (deferment.md G-6). Add a branch."); + } + + /// + /// The enumeration the fallback's comment claims: no driver type reaches the generic key by + /// accident. Covers the universal-browse arm, which cannot be derived from this assembly. + /// + [Fact] + public void Every_declared_driver_type_is_typed_or_carries_a_documented_generic_key_exemption() + { + var exempt = GenericKeyExemptions.Select(e => e.DriverType).ToHashSet(StringComparer.OrdinalIgnoreCase); + + var undocumented = DriverTypeNames.All + .Where(t => CommitsToTheGenericAddressKey(t) && !exempt.Contains(t)) + .OrderBy(t => t, StringComparer.Ordinal) + .ToList(); + + undocumented.ShouldBeEmpty( + "these driver types commit to the generic \"address\" key with nothing recorded about why: " + + $"{string.Join(", ", undocumented)}. If the driver can be browsed — a bespoke IDriverBrowser, " + + "or an ITagDiscovery driver the universal DiscoveryDriverBrowser can open — give it a " + + "BuildTagConfig branch. If it genuinely cannot, add it to GenericKeyExemptions with the " + + "reason, so the next reader inherits the argument instead of the silence."); + } + + /// + /// Refutes an exemption the derivable arm contradicts. This is the check that would have caught Sql: + /// a registered browser is proof the driver IS browsable, whatever the exemption claims. + /// + [Fact] + public void No_generic_key_exemption_covers_a_driver_type_that_has_a_registered_browser() + { + var browsable = RegisteredBespokeBrowserDriverTypes().ToHashSet(StringComparer.OrdinalIgnoreCase); + + var contradicted = GenericKeyExemptions + .Where(e => browsable.Contains(e.DriverType)) + .OrderBy(e => e.DriverType, StringComparer.Ordinal) + .ToList(); + + contradicted.ShouldBeEmpty( + "these exemptions claim a driver cannot be browsed, but AdminUI registers an IDriverBrowser for " + + $"it: {string.Join("; ", contradicted.Select(e => $"{e.DriverType} — \"{e.Reason}\""))}. " + + "Remove the exemption and give the driver a BuildTagConfig branch."); + } + + /// An exemption for a driver type that no longer exists is stale bookkeeping. + [Fact] + public void Every_generic_key_exemption_names_a_declared_driver_type() + { + var declared = DriverTypeNames.All.ToHashSet(StringComparer.OrdinalIgnoreCase); + + var stale = GenericKeyExemptions + .Select(e => e.DriverType) + .Where(t => !declared.Contains(t)) + .OrderBy(t => t, StringComparer.Ordinal) + .ToList(); + + stale.ShouldBeEmpty($"GenericKeyExemptions names undeclared driver types: {string.Join(", ", stale)}"); + } + + // ---- derivation + fixtures ------------------------------------------------------------------ + + /// + /// Runs the production AddAdminUI() registration and reads back the driver type of every + /// it registers — the same set BrowserSessionService indexes. + /// + /// The driver types with a bespoke browse session, as production registers them. + /// + /// Resolved through a real provider rather than read off the ServiceDescriptors, because + /// is an instance member — GalaxyDriverBrowser takes a + /// required , so the two collaborators it needs are supplied here. + /// Nothing in the graph is exercised beyond construction; no browse is opened. + /// + private static IReadOnlyList RegisteredBespokeBrowserDriverTypes() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSingleton(); + services.AddAdminUI(); + + using var provider = services.BuildServiceProvider(); + return [.. provider.GetServices().Select(b => b.DriverType)]; + } + + /// + /// Does the mapper drop this driver type onto the unknown-driver fallback — the single-key + /// {"address": …} blob WriteSingleKey writes? + /// + /// The driver type to map a leaf for. + /// when the commit produced the generic fallback blob. + /// + /// Judged on the produced JSON rather than on which branch ran, because the JSON is what the typed + /// editor later reads. A driver whose own model happens to emit only address would read back + /// identically and is correctly not flagged — every typed model in the tree emits more keys than + /// that, so the two shapes stay distinguishable in practice. + /// + private static bool CommitsToTheGenericAddressKey(string driverType) + { + var json = RawBrowseCommitMapper.BuildTagConfig(driverType, Reference, StatedAddressFields(driverType)); + var o = JsonNode.Parse(json)!.AsObject(); + + return o.Count == 1 + && o.TryGetPropertyValue("address", out var value) + && value?.GetValue() == Reference; + } + + /// + /// The address fields a browse session states for a driver whose binding is a TUPLE rather than a + /// single reference string; for every driver whose address IS the leaf's node id. + /// + /// The driver type being mapped. + /// The stated fields, or when the driver states none. + /// + /// Keyed off the drivers' own published key constants, not string literals, and null is the default + /// ON PURPOSE: a future tuple-address driver that lands with no entry here maps from nothing, falls + /// to the generic key, and fails the guard by name — which is the direction to fail in. Sql is the + /// proof that matters: with no stated table its branch deliberately returns the generic blob, + /// so a Sql entry here is what makes its typed branch observable at all. + /// + /// The browsed native dataType (Gitea #536) is deliberately not restated — it changes a field + /// inside an already-typed blob, never which key the address lands under, and + /// RawBrowseCommitMapperTests pins it per driver. + /// + /// + private static IReadOnlyDictionary? StatedAddressFields(string driverType) => driverType switch + { + DriverTypeNames.Mqtt => new Dictionary(StringComparer.Ordinal) + { + [MqttTagConfigKeys.GroupId] = "OtOpcUaSim", + [MqttTagConfigKeys.EdgeNodeId] = "EdgeA", + [MqttTagConfigKeys.MetricName] = Reference, + }, + DriverTypeNames.Sql => new Dictionary(StringComparer.Ordinal) + { + [SqlBrowseAddressFields.Schema] = "dbo", + [SqlBrowseAddressFields.Table] = "Readings", + [SqlBrowseAddressFields.ColumnName] = Reference, + [SqlBrowseAddressFields.TopByTimestamp] = "SampledAt", + }, + _ => null, + }; + + /// One driver type deliberately left on the generic address key, and why. + /// The exempted value. + /// Why the generic key is the right commit for it — a claim, open to refutation. + private sealed record GenericKeyExemption(string DriverType, string Reason); + + /// + /// Driver types with no browse path at all, for which the generic address key is correct. + /// Adding an entry here is a claim that the driver cannot be browsed — by a bespoke + /// (mechanically refuted by + /// ) nor by + /// the universal discovery browser (which needs the driver to be + /// ITagDiscovery { SupportsOnlineDiscovery: true }). If it can be browsed, write a branch instead. + /// + private static readonly GenericKeyExemption[] GenericKeyExemptions = + [ + new(DriverTypeNames.Modbus, + "Register addresses are numeric coordinates in a fixed map, not a namespace anything can " + + "enumerate: ModbusDriver implements no ITagDiscovery and has no bespoke browser. The generic " + + "key is also the key ModbusTagConfigModel reads, so nothing is lost even if a leaf arrives."), + new(DriverTypeNames.Calculation, + "A pseudo-driver computing over other tags' RawPaths — there is no device to browse, and its " + + "binding is a scriptId rather than an address."), + ]; + + /// + /// An that resolves every secret to absent, for constructing + /// GalaxyDriverBrowser. Nothing here opens a session, so no secret is ever requested — and + /// resolving to absent means a browser that started requesting one fails closed rather than + /// silently succeeding against a fake value. + /// + private sealed class AbsentSecretResolver : ISecretResolver + { + /// + public Task GetAsync(SecretName name, CancellationToken ct) => Task.FromResult(null); + } +}