test(raw): write the browse-commit parity guard the mapper's comment cites (#555)
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.
This commit is contained in:
Joseph Doherty
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// The guard <see cref="RawBrowseCommitMapper.BuildTagConfig"/>'s fallback comment cites (Gitea #555):
|
||||
/// every driver type in <see cref="DriverTypeNames.All"/> either gets a <b>dedicated</b> branch in
|
||||
/// <c>BuildTagConfig</c>, or appears in <see cref="GenericKeyExemptions"/> with a written reason.
|
||||
/// <para>
|
||||
/// <b>Why the generic key is a defect for a browsable driver.</b> The fallback writes
|
||||
/// <c>{"address": "<leaf>"}</c>. A driver whose typed editor reads a different key — MTConnect's
|
||||
/// <c>fullName</c>, OPC UA Client's <c>nodeId</c>, Sql's <c>table</c>/<c>columnName</c>/<c>rowSelector</c>
|
||||
/// — then opens that committed tag with an EMPTY address field and <em>blanks it on save</em>. That is
|
||||
/// exactly why the MTConnect branch was added, and Sql shipped with the same bug unnoticed
|
||||
/// (<c>deferment.md</c> G-6) because the fallback's comment asserted "every BROWSABLE driver is handled
|
||||
/// above" while <c>SqlDriverBrowser</c> was already registered.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Enumeration, not InlineData.</b> <c>RawTagConfigGateTests</c> 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 <see cref="DriverTypeNames.All"/>, so a newly-registered driver is triaged the first time
|
||||
/// the suite runs — the idiom <c>DriverFormMapParityTests</c> / <c>DriverDispatchMapParityTests</c> use.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>Browsability derivation, and its residual trust.</b> Production decides browsability in
|
||||
/// <c>BrowserSessionService.CanBrowse</c>, which is a disjunction of two arms:
|
||||
/// </para>
|
||||
/// <list type="number">
|
||||
/// <item>
|
||||
/// a <b>bespoke</b> <see cref="IDriverBrowser"/> registered for the driver type — derived here for
|
||||
/// real, by running the production <c>AddAdminUI()</c> registration and reading back every
|
||||
/// registered browser's <see cref="IDriverBrowser.DriverType"/>. 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.
|
||||
/// </item>
|
||||
/// <item>
|
||||
/// the <b>universal</b> <c>DiscoveryDriverBrowser</c>, whose <c>CanBrowse</c> constructs the driver
|
||||
/// through <c>IDriverFactory</c> and asks whether it is
|
||||
/// <c>ITagDiscovery { SupportsOnlineDiscovery: true }</c>. That arm is <b>not derivable from this
|
||||
/// assembly</b>: the AdminUI references driver <i>Contracts</i> and <i>Browser</i> projects, never
|
||||
/// the runtime drivers, and <c>IDriverFactory</c> 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.
|
||||
/// </item>
|
||||
/// </list>
|
||||
/// <para>
|
||||
/// So the universal arm is covered the only honest way available here: by requiring <b>every</b> 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. <see cref="No_generic_key_exemption_covers_a_driver_type_that_has_a_registered_browser"/>
|
||||
/// then mechanically refutes any exemption the derived arm contradicts — which is the failure mode that
|
||||
/// actually occurred with Sql.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class RawBrowseCommitMapperParityTests
|
||||
{
|
||||
/// <summary>The browse leaf's reference, as the commit path passes it (<c>BrowseNode.NodeId</c>).</summary>
|
||||
private const string Reference = "Speed";
|
||||
|
||||
/// <summary>
|
||||
/// THE #555 guard, over the arm that can be derived: a driver type with a registered bespoke
|
||||
/// browser must commit to a typed blob.
|
||||
/// </summary>
|
||||
[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.");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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.");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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.");
|
||||
}
|
||||
|
||||
/// <summary>An exemption for a driver type that no longer exists is stale bookkeeping.</summary>
|
||||
[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 ------------------------------------------------------------------
|
||||
|
||||
/// <summary>
|
||||
/// Runs the production <c>AddAdminUI()</c> registration and reads back the driver type of every
|
||||
/// <see cref="IDriverBrowser"/> it registers — the same set <c>BrowserSessionService</c> indexes.
|
||||
/// </summary>
|
||||
/// <returns>The driver types with a bespoke browse session, as production registers them.</returns>
|
||||
/// <remarks>
|
||||
/// Resolved through a real provider rather than read off the <c>ServiceDescriptor</c>s, because
|
||||
/// <see cref="IDriverBrowser.DriverType"/> is an instance member — <c>GalaxyDriverBrowser</c> takes a
|
||||
/// required <see cref="ISecretResolver"/>, so the two collaborators it needs are supplied here.
|
||||
/// Nothing in the graph is exercised beyond construction; no browse is opened.
|
||||
/// </remarks>
|
||||
private static IReadOnlyList<string> RegisteredBespokeBrowserDriverTypes()
|
||||
{
|
||||
var services = new ServiceCollection();
|
||||
services.AddLogging();
|
||||
services.AddSingleton<ISecretResolver, AbsentSecretResolver>();
|
||||
services.AddAdminUI();
|
||||
|
||||
using var provider = services.BuildServiceProvider();
|
||||
return [.. provider.GetServices<IDriverBrowser>().Select(b => b.DriverType)];
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Does the mapper drop this driver type onto the unknown-driver fallback — the single-key
|
||||
/// <c>{"address": …}</c> blob <c>WriteSingleKey</c> writes?
|
||||
/// </summary>
|
||||
/// <param name="driverType">The driver type to map a leaf for.</param>
|
||||
/// <returns><see langword="true"/> when the commit produced the generic fallback blob.</returns>
|
||||
/// <remarks>
|
||||
/// 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 <c>address</c> 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.
|
||||
/// </remarks>
|
||||
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<string>() == Reference;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The address fields a browse session states for a driver whose binding is a TUPLE rather than a
|
||||
/// single reference string; <see langword="null"/> for every driver whose address IS the leaf's node id.
|
||||
/// </summary>
|
||||
/// <param name="driverType">The driver type being mapped.</param>
|
||||
/// <returns>The stated fields, or <see langword="null"/> when the driver states none.</returns>
|
||||
/// <remarks>
|
||||
/// 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 <c>table</c> its branch deliberately returns the generic blob,
|
||||
/// so a Sql entry here is what makes its typed branch observable at all.
|
||||
/// <para>
|
||||
/// The browsed native <c>dataType</c> (Gitea #536) is deliberately not restated — it changes a field
|
||||
/// inside an already-typed blob, never which key the address lands under, and
|
||||
/// <c>RawBrowseCommitMapperTests</c> pins it per driver.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private static IReadOnlyDictionary<string, string>? StatedAddressFields(string driverType) => driverType switch
|
||||
{
|
||||
DriverTypeNames.Mqtt => new Dictionary<string, string>(StringComparer.Ordinal)
|
||||
{
|
||||
[MqttTagConfigKeys.GroupId] = "OtOpcUaSim",
|
||||
[MqttTagConfigKeys.EdgeNodeId] = "EdgeA",
|
||||
[MqttTagConfigKeys.MetricName] = Reference,
|
||||
},
|
||||
DriverTypeNames.Sql => new Dictionary<string, string>(StringComparer.Ordinal)
|
||||
{
|
||||
[SqlBrowseAddressFields.Schema] = "dbo",
|
||||
[SqlBrowseAddressFields.Table] = "Readings",
|
||||
[SqlBrowseAddressFields.ColumnName] = Reference,
|
||||
[SqlBrowseAddressFields.TopByTimestamp] = "SampledAt",
|
||||
},
|
||||
_ => null,
|
||||
};
|
||||
|
||||
/// <summary>One driver type deliberately left on the generic <c>address</c> key, and why.</summary>
|
||||
/// <param name="DriverType">The exempted <see cref="DriverTypeNames"/> value.</param>
|
||||
/// <param name="Reason">Why the generic key is the right commit for it — a claim, open to refutation.</param>
|
||||
private sealed record GenericKeyExemption(string DriverType, string Reason);
|
||||
|
||||
/// <summary>
|
||||
/// Driver types with no browse path at all, for which the generic <c>address</c> key is correct.
|
||||
/// Adding an entry here is a claim that the driver cannot be browsed — by a bespoke
|
||||
/// <see cref="IDriverBrowser"/> (mechanically refuted by
|
||||
/// <see cref="No_generic_key_exemption_covers_a_driver_type_that_has_a_registered_browser"/>) nor by
|
||||
/// the universal discovery browser (which needs the driver to be
|
||||
/// <c>ITagDiscovery { SupportsOnlineDiscovery: true }</c>). If it can be browsed, write a branch instead.
|
||||
/// </summary>
|
||||
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."),
|
||||
];
|
||||
|
||||
/// <summary>
|
||||
/// An <see cref="ISecretResolver"/> that resolves every secret to absent, for constructing
|
||||
/// <c>GalaxyDriverBrowser</c>. 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.
|
||||
/// </summary>
|
||||
private sealed class AbsentSecretResolver : ISecretResolver
|
||||
{
|
||||
/// <inheritdoc />
|
||||
public Task<string?> GetAsync(SecretName name, CancellationToken ct) => Task.FromResult<string?>(null);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user