merge: #555 — RawBrowseCommitMapperParityTests exists now: bespoke-browser types derived from the real AddAdminUI registration must be typed; generic-key fallthrough demands a documented exemption
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