From 17130d9c8299f5001b1fe052165f7e6b8a2a89da Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 02:56:05 -0400 Subject: [PATCH] test(parity): consolidate the browse-commit guard onto one list; key browsers off DriverTypeNames (#558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two guards enumerated the same RawBrowseCommitMapper.BuildTagConfig seam against two hand-authored lists, which can only drift. The older one's list was also misnamed: DriverDispatchMapParityTests.NonBrowsableDriverTypes really meant "types that produce the generic key" — AbLegacy is genuinely non-browsable yet correctly absent from it, because it has a typed branch. Retire the older fact. RawBrowseCommitMapperParityTests.GenericKeyExemptions is now the single hand-authored list for this seam: named for what it holds, one written reason per entry, and checked against a DI-derived browsable set rather than a second list. The one assertion the retired fact made that nothing here covered — set EQUALITY, i.e. an exemption must still DESCRIBE the mapper — moves across as Every_generic_key_exemption_still_commits_to_the_generic_key. Without it, a driver that gained a typed branch would keep a silently inert exemption. Item 2: OpcUaClientDriverBrowser and GalaxyDriverBrowser returned string literals ("OpcUaClient", "GalaxyMxGateway") where Sql/Mqtt use DriverTypeNames. The values matched, and nothing guarded that. Repoint both at the constants — DriverTypeNames.Galaxy carries the identical value — and add Every_registered_browser_is_keyed_by_a_declared_driver_type_name over the same AddAdminUI() read-back. Compared ORDINALLY: BrowserSessionService indexes on the exact persisted string, and case-only drift (TwinCat/TwinCAT) is what DriverTypeNames exists to end. Browser projects are out of reach of DriverTypeNamesGuardTests, which pins the driver FACTORIES' names. Falsified, four ways, each red naming the offender and green on revert: - #555 re-run: disabling the Sql branch reddens the two enumeration facts ("...no dedicated BuildTagConfig branch...: Sql"), the same catch the retired fact made. - giving Modbus a typed branch reddens ONLY the new stale-exemption fact, proving it is not redundant with the four that were already there. - SqlDriverBrowser.DriverType => "SQL" and Galaxy => "GalaxyMXGateway" each redden the new browser-key guard by name — case-only typos, which an OrdinalIgnoreCase compare would have waved through. --- .../GalaxyDriverBrowser.cs | 13 +++- .../OpcUaClientDriverBrowser.cs | 9 ++- .../Uns/RawBrowseCommitMapper.cs | 4 +- .../Uns/DriverDispatchMapParityTests.cs | 68 ++++------------- .../Uns/RawBrowseCommitMapperParityTests.cs | 73 +++++++++++++++++++ 5 files changed, 110 insertions(+), 57 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs index b3c67ec5..8722086e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs @@ -4,6 +4,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.MxGateway.Client; using ZB.MOM.WW.OtOpcUa.Commons.Browsing; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config; using ZB.MOM.WW.Secrets.Abstractions; @@ -47,9 +48,15 @@ public sealed class GalaxyDriverBrowser : IDriverBrowser } /// - // Hardcoded literal: this project references Driver.Galaxy.Contracts, not Driver.Galaxy, - // so GalaxyDriverFactoryExtensions.DriverTypeName isn't available here. - public string DriverType => "GalaxyMxGateway"; + /// + /// Sourced from the shared set, reached through + /// Commons. This project references Driver.Galaxy.Contracts, not Driver.Galaxy, so + /// GalaxyDriverFactoryExtensions.DriverTypeName is not available here — but that was never a + /// reason to hand-write the string, which is what this line used to do (Gitea #558): + /// holds the same value and is the constant + /// DriverTypeNamesGuardTests already pins to the factory's registration. + /// + public string DriverType => DriverTypeNames.Galaxy; // Deserializes a GalaxyDriverOptions blob, opens a transient GalaxyRepositoryClient // against the configured gateway endpoint, and returns a browse session over it. The diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs index 69805ce4..9a9d822e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs @@ -6,6 +6,7 @@ using Opc.Ua; using Opc.Ua.Client; using Opc.Ua.Configuration; using ZB.MOM.WW.OtOpcUa.Commons.Browsing; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; // OpcUaClientDriverOptions + NamespaceMap namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser; @@ -41,7 +42,13 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser } /// - public string DriverType => "OpcUaClient"; + /// + /// Sourced from the shared set rather than the hand-written literal + /// this line used to carry (Gitea #558). The value is unchanged; the constant is what + /// DriverTypeNamesGuardTests pins to the factory's own registration, so a rename on either + /// side can no longer leave this browser indexed under a key nothing dispatches to. + /// + public string DriverType => DriverTypeNames.OpcUaClient; /// public async Task OpenAsync(string configJson, CancellationToken cancellationToken) 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 024c251a..97b50e60 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs @@ -271,7 +271,9 @@ public static class RawBrowseCommitMapper // 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. + // - any type that does reach here must be named in that test's GenericKeyExemptions with a reason, + // and every name on that list must still reach here (Gitea #558) — an exemption for a driver + // that has since gained a branch above is a stale claim, and a silently inert one. // 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). diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DriverDispatchMapParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DriverDispatchMapParityTests.cs index 0cb9f9ff..2b778e1b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DriverDispatchMapParityTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DriverDispatchMapParityTests.cs @@ -1,16 +1,28 @@ using Shouldly; using Xunit; -using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; /// -/// Parity guards for the two remaining per-driver dispatch maps (deferment.md G-5, G-6). -/// Both were already testable structures — unlike the two Razor @switch blocks that needed -/// extracting first — so nothing but the tests was missing. +/// Parity guards for the per-driver CSV column dispatch map (deferment.md G-5). It was +/// already a testable structure — unlike the two Razor @switch blocks that needed extracting +/// first — so nothing but the tests was missing. /// +/// +/// The browse-commit seam (G-6) is guarded next door, not here (Gitea #558). This class used to +/// also hold Browse_commit_falls_back_to_the_generic_address_key_for_exactly_the_non_browsable_drivers, +/// over the same RawBrowseCommitMapper.BuildTagConfig seam +/// now enumerates. Two guards over one seam means two +/// hand-authored lists of "drivers that legitimately reach the generic key", and the pair can only +/// drift — this one's was additionally MISNAMED (NonBrowsableDriverTypes really meant "types +/// that produce the generic key": AbLegacy is genuinely non-browsable yet correctly absent, +/// because it has a typed branch). The surviving list is +/// RawBrowseCommitMapperParityTests.GenericKeyExemptions, which is named for what it holds, +/// carries a written reason per entry, and is checked against a derived browsable set rather +/// than a second hand-authored one. +/// [Trait("Category", "Unit")] public sealed class DriverDispatchMapParityTests { @@ -47,52 +59,4 @@ public sealed class DriverDispatchMapParityTests stale.ShouldBeEmpty($"CsvColumnMap has entries for undeclared driver types: {string.Join(", ", stale)}"); } - - /// - /// Every driver type either produces a TYPED blob from browse-commit, or is on the documented - /// non-browsable list. The generic {"address": …} fallback opens EMPTY in a typed editor and - /// blanks the field on save — the MTConnect branch was added for exactly that, and Sql had the same - /// bug unnoticed (deferment.md G-6) because the fallback's comment claimed "browsable drivers - /// are all handled above", which stopped being true the moment SqlDriverBrowser registered. - /// Asserted in both directions on purpose. A one-way check would let a newly-browsable - /// driver be quietly added to the exclusion list instead of getting a branch. Requiring the two sets - /// to match exactly means a driver gaining or losing a browser forces this list to be revisited. - /// - [Fact] - public void Browse_commit_falls_back_to_the_generic_address_key_for_exactly_the_non_browsable_drivers() - { - // Address fields as a browser supplies them; a driver whose browser states none must still produce - // something its typed editor can read from the leaf name alone. - var addressFields = new Dictionary(StringComparer.Ordinal) - { - ["schema"] = "dbo", - ["table"] = "Readings", - ["columnName"] = "Speed", - }; - - var fellBack = DriverTypeNames.All - .Where(t => RawBrowseCommitMapper.BuildTagConfig(t, "Speed", addressFields) == """{"address":"Speed"}""") - .OrderBy(t => t, StringComparer.Ordinal) - .ToList(); - - fellBack.ShouldBe( - NonBrowsableDriverTypes.OrderBy(t => t, StringComparer.Ordinal).ToList(), - $"the set of driver types committing to the generic \"address\" key drifted. Extra entries are " - + $"browsable drivers whose typed editor will open EMPTY and blank the field on save; missing " - + $"entries gained a typed branch and should leave this list. Got: {string.Join(", ", fellBack)}"); - } - - /// - /// Driver types with no browser, for which the generic address key is the correct commit. - /// - /// Modbus — register addresses are numeric coordinates, not a browsable namespace. - /// Calculation — a pseudo-driver over other tags' RawPaths; there is no device. - /// - /// Adding a type here is a claim that it cannot be browsed. If it can, give it a branch instead. - /// - private static readonly string[] NonBrowsableDriverTypes = - [ - DriverTypeNames.Modbus, - DriverTypeNames.Calculation, - ]; } 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 index b8a33313..9d1771f1 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperParityTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperParityTests.cs @@ -30,6 +30,15 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; /// class walks , so a newly-registered driver is triaged the first time /// the suite runs — the idiom DriverFormMapParityTests / DriverDispatchMapParityTests use. /// +/// +/// The only guard over this seam (Gitea #558). DriverDispatchMapParityTests briefly held a +/// second one, enumerating the same mapper against its own hand-authored +/// NonBrowsableDriverTypes. Two lists for one question can only drift, so it was retired into +/// this class: is now the single hand-authored list for browse-commit, +/// and the one assertion the old fact made that nothing here covered — that an exemption still describes +/// the mapper — came with it as +/// . +/// /// /// /// @@ -138,6 +147,70 @@ public sealed class RawBrowseCommitMapperParityTests + "Remove the exemption and give the driver a BuildTagConfig branch."); } + /// + /// The other direction: an exemption must still DESCRIBE the mapper. A driver that has since gained + /// a typed BuildTagConfig branch no longer reaches the generic key, so its exemption is a + /// stale claim — and worse, a silently inert one, since the enumeration above only ever flags + /// drivers that are missing from the list. + /// + /// + /// Inherited from DriverDispatchMapParityTests's retired browse-commit fact, which asserted + /// set EQUALITY against its own hand-authored list and so covered this direction as a side effect + /// (Gitea #558). Consolidating onto one list would have dropped the assertion had it not moved here. + /// Keeping it also keeps the list honest as a piece of documentation: every reason written in + /// is a live description of what the mapper does today, not an + /// archived one. + /// + [Fact] + public void Every_generic_key_exemption_still_commits_to_the_generic_key() + { + var superseded = GenericKeyExemptions + .Where(e => !CommitsToTheGenericAddressKey(e.DriverType)) + .OrderBy(e => e.DriverType, StringComparer.Ordinal) + .ToList(); + + superseded.ShouldBeEmpty( + "these driver types are exempted from needing a typed branch, but RawBrowseCommitMapper now " + + $"gives them one: {string.Join(", ", superseded.Select(e => e.DriverType))}. The exemption is " + + "stale — delete it, so the list keeps meaning what it says."); + } + + /// + /// Every registered browser is indexed under a value, exactly. + /// BrowserSessionService looks a browser up by the driver type persisted on the device, so a + /// browser whose key is anything else is registered but unreachable — the picker reports "no browser + /// for this driver" and the operator falls back to typing the address by hand. + /// + /// + /// Added with Gitea #558, alongside repointing OpcUaClientDriverBrowser and + /// GalaxyDriverBrowser off string literals ("OpcUaClient" / "GalaxyMxGateway") + /// onto the constants. The values matched, so nothing was broken — but nothing prevented the next + /// browser from hand-writing its key either, and a browser project cannot be reached by + /// DriverTypeNamesGuardTests, which pins the driver FACTORIES' names rather than the + /// browsers'. This is the seam where the two meet: the same DI read-back the guards above use + /// already has every browser instance in hand. + /// + /// Compared ORDINALLY, not case-insensitively: BrowserSessionService's index and the + /// persisted DriverType must agree on the exact string, and "differs only in case" is + /// precisely the TwinCat/TwinCAT drift exists to end. + /// + /// + [Fact] + public void Every_registered_browser_is_keyed_by_a_declared_driver_type_name() + { + var declared = DriverTypeNames.All.ToHashSet(StringComparer.Ordinal); + + var unreachable = RegisteredBespokeBrowserDriverTypes() + .Where(t => !declared.Contains(t)) + .OrderBy(t => t, StringComparer.Ordinal) + .ToList(); + + unreachable.ShouldBeEmpty( + "these IDriverBrowser registrations are keyed by a string that is not a DriverTypeNames member, " + + $"so BrowserSessionService can never match a device to them: {string.Join(", ", unreachable)}. " + + "Use the DriverTypeNames constant."); + } + /// 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()