fix(adminui): authoring must not accept a TagConfig enum the runtime rejects (#549)
MTConnectTagConfigModel read `dataType` with TagConfigJson.GetEnum, which falls
back to String on ANY unrecognised value. A misspelled name ("Strng") from a
hand-authored blob, a CSV import or the API was therefore accepted at authoring
and silently rewritten, while the driver's strict parse rejected it and the tag
died with BadNodeIdUnknown. Found by the G-7 parity tests' falsifiability
control, which had to use the digits-only shape to have a case that failed.
The rule is not restated in the AdminUI. MTConnectTagDefinitionFactory exposes
its parse as `IsAcceptedDataTypeName`, documented as the one authority the
driver, the deploy gate and the authoring model all consult; the model's
Validate() asks it about the raw value the blob carries. The old digits-only
message is NOT retained as a special case: the runtime draws no distinction
between a bare number and a typo — one predicate, one Inspect warning — so a
second message here would be a second statement of a rule this model does not
own. The raw value is captured as the JSON TOKEN, because the runtime
deserialises the key into a `string?` DTO field and an unquoted number throws
there too.
The survey found the same asymmetry at six more editors, contrary to the
assumption that those runtimes silently default: R2-11 Phase C moved every
driver's TagConfig enum read to TagConfigJson.TryReadEnumStrict, which REJECTS
the tag on a present-but-invalid string, while the editors kept loading them
with a silent fallback. Modbus (region/dataType/byteOrder), S7, AbCip, AbLegacy,
TwinCAT and FOCAS now check their enums in Validate() through the runtime's own
TagConfigJson.DescribeInvalidEnumText — added alongside a public
ClassifyEnumText that TryReadEnum now delegates to, so the JSON-token door and
the string door reach one implementation. Mqtt and Sql already carried
equivalent strict checks and are untouched.
Care taken NOT to become stricter than the runtime: a present-but-non-string
token stays accepted for those six (TryReadEnum calls it absent), as does an
absent key, each pinned by a test.
Also adds a direct ProjectReference to Core.Abstractions from the AdminUI — the
save gate now leans on a type it was consuming only transitively.
This commit is contained in:
@@ -52,7 +52,18 @@ public sealed class AbCipTagConfigModel
|
||||
}
|
||||
|
||||
/// <summary>Validation hook; returns an error message or null when the model is valid.</summary>
|
||||
/// <remarks>
|
||||
/// The <c>dataType</c> check reads the ORIGINAL key bag rather than the leniently-defaulted
|
||||
/// <see cref="DataType"/>, because <c>AbCipTagDefinitionFactory</c> reads it with
|
||||
/// <c>TryReadEnumStrict</c> — a present-but-invalid string rejects the whole tag at the driver
|
||||
/// (<c>BadNodeIdUnknown</c>). Without this the blob would be accepted here, silently rewritten to
|
||||
/// <c>DInt</c> on save, and — on the CSV-import and API paths, which save it verbatim — deployed
|
||||
/// into a tag the driver skips (Gitea #549).
|
||||
/// </remarks>
|
||||
/// <returns>An error message describing the validation failure, or <c>null</c> when valid.</returns>
|
||||
public string? Validate()
|
||||
=> string.IsNullOrWhiteSpace(TagPath) ? "Tag path is required." : null;
|
||||
{
|
||||
if (TagConfigJson.DescribeInvalidEnum<AbCipDataType>(_bag, "dataType") is { } dtError) { return dtError; }
|
||||
return string.IsNullOrWhiteSpace(TagPath) ? "Tag path is required." : null;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -55,6 +55,14 @@ public sealed class AbLegacyTagConfigModel
|
||||
/// <returns>An error message describing the validation failure, or <c>null</c> when valid.</returns>
|
||||
public string? Validate()
|
||||
{
|
||||
// Strict enum, read from the ORIGINAL key bag: AbLegacyTagDefinitionFactory reads "dataType" with
|
||||
// TryReadEnumStrict, so a present-but-invalid string rejects the whole tag at the driver
|
||||
// (BadNodeIdUnknown) while the lenient load above would default it to Int. Accepting it here would
|
||||
// let the CSV-import and API paths — which save the blob verbatim — deploy an unreadable tag
|
||||
// (Gitea #549).
|
||||
if (TagConfigJson.DescribeInvalidEnum<AbLegacyDataType>(_bag, "dataType") is { } dtError)
|
||||
return dtError;
|
||||
|
||||
if (string.IsNullOrWhiteSpace(Address))
|
||||
return "Address is required.";
|
||||
|
||||
|
||||
@@ -59,6 +59,13 @@ public sealed class FocasTagConfigModel
|
||||
/// </remarks>
|
||||
public string? Validate()
|
||||
{
|
||||
// Strict enum, read from the ORIGINAL key bag: FocasTagDefinitionFactory reads "dataType" with
|
||||
// TryReadEnumStrict, so a present-but-invalid string rejects the whole tag at the driver
|
||||
// (BadNodeIdUnknown) while the lenient load above defaults it to Int32. Accepting it here would
|
||||
// let the CSV-import and API paths — which save the blob verbatim — deploy an unreadable tag,
|
||||
// the same shape as the #540 address gap this method already closes (Gitea #549).
|
||||
if (TagConfigJson.DescribeInvalidEnum<FocasDataType>(_bag, "dataType") is { } dtError) return dtError;
|
||||
|
||||
if (string.IsNullOrWhiteSpace(Address)) return "Address is required.";
|
||||
return FocasAddress.TryParse(Address) is null ? DescribeUnparseableAddress(Address) : null;
|
||||
}
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
using System.Text.Json.Nodes;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.MTConnect;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors;
|
||||
|
||||
@@ -57,12 +58,17 @@ public sealed class MTConnectTagConfigModel
|
||||
/// consumed by the driver or the runtime.</summary>
|
||||
public string? MtComponent { get; set; }
|
||||
|
||||
// The as-loaded raw "dataType" JSON value, kept only to detect the ParseEnum numeric-text trap in
|
||||
// Validate() below — Enum.TryParse (which TagConfigJson.GetEnum uses) silently accepts a quoted
|
||||
// number ("8" -> Float64) exactly as readily as a name, so a config that was ever hand-authored or
|
||||
// migrated with a numeric dataType would otherwise load into a *valid-looking* enum value with no
|
||||
// signal that it isn't a name. ToJson() always re-writes DataType as a name, so this can only ever
|
||||
// be non-null on the FIRST load of an already-poisoned config, never after a save through this model.
|
||||
// The as-loaded raw "dataType" JSON token, kept so Validate() below can put it to the DRIVER's own
|
||||
// accept/reject rule (MTConnectTagDefinitionFactory.IsAcceptedDataTypeName) rather than to the
|
||||
// lenient load. TagConfigJson.GetEnum falls back to String on ANY unrecognised value, so without this
|
||||
// a misspelled name ("Strng") or a quoted number ("8" -> Float64, the ParseEnum numeric-text trap)
|
||||
// loads as a *valid-looking* enum value with no signal that it is not what the operator wrote — while
|
||||
// the strict runtime rejects the tag outright (Gitea #549). Captured as the raw TOKEN, not via
|
||||
// GetString: the runtime deserialises this key into a `string?` DTO field, so a NON-string token
|
||||
// (`"dataType": 8`) throws JsonException there and rejects the tag too, where GetString would report
|
||||
// it as simply absent. ToJson() always re-writes dataType as a name, so this can only ever be
|
||||
// poisoned on the FIRST load of a blob authored elsewhere (hand-edited, CSV import, API), never after
|
||||
// a save through this model.
|
||||
private string? _rawDataType;
|
||||
|
||||
private JsonObject _bag = new();
|
||||
@@ -96,7 +102,7 @@ public sealed class MTConnectTagConfigModel
|
||||
Units = TagConfigJson.GetString(o, "units"),
|
||||
MtDevice = TagConfigJson.GetString(o, "mtDevice"),
|
||||
MtComponent = TagConfigJson.GetString(o, "mtComponent"),
|
||||
_rawDataType = TagConfigJson.GetString(o, "dataType"),
|
||||
_rawDataType = ReadRawDataType(o),
|
||||
_bag = o,
|
||||
};
|
||||
}
|
||||
@@ -124,6 +130,15 @@ public sealed class MTConnectTagConfigModel
|
||||
}
|
||||
|
||||
/// <summary>Validation hook; returns an error message or null when the model is valid.</summary>
|
||||
/// <remarks>
|
||||
/// The <c>dataType</c> rule is not stated here — it is asked of
|
||||
/// <see cref="MTConnectTagDefinitionFactory.IsAcceptedDataTypeName"/>, the same predicate the driver
|
||||
/// and the deploy gate use. One authority, so this gate can never end up weaker than the runtime
|
||||
/// again (Gitea #549). Note that the previous, digits-only check is NOT retained as a tailored
|
||||
/// special case: the runtime draws no distinction between a bare number and a misspelled name —
|
||||
/// both fail that one predicate and both draw the same <c>Inspect</c> warning — so a second message
|
||||
/// here would be a second statement of a rule this model does not own.
|
||||
/// </remarks>
|
||||
/// <returns>An error message describing the validation failure, or <c>null</c> when the model is valid.</returns>
|
||||
public string? Validate()
|
||||
{
|
||||
@@ -132,16 +147,26 @@ public sealed class MTConnectTagConfigModel
|
||||
return "A DataItem id (fullName) is required.";
|
||||
}
|
||||
|
||||
// See the _rawDataType remarks: a quoted number parses as a "valid" enum today but is never
|
||||
// something this editor itself would have written, so surface it instead of silently accepting it.
|
||||
if (_rawDataType is { Length: > 0 } raw && raw.All(char.IsAsciiDigit))
|
||||
// See the _rawDataType remarks: the value the blob CARRIES is judged, not the leniently-defaulted
|
||||
// DataType above, because the CSV-import and API paths save the blob verbatim.
|
||||
if (!MTConnectTagDefinitionFactory.IsAcceptedDataTypeName(_rawDataType))
|
||||
{
|
||||
return $"dataType \"{raw}\" is a numeric value, not a named type — re-select a data type.";
|
||||
return $"dataType \"{_rawDataType}\" is not a valid DriverDataType name — re-select a data type. "
|
||||
+ $"Valid: {string.Join(", ", Enum.GetNames<DriverDataType>())}.";
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
// The as-loaded "dataType" TOKEN as text (see the _rawDataType remarks): a JSON string yields its
|
||||
// value, any other present token yields its JSON text so Validate() judges what was actually written,
|
||||
// and absent / JSON null yields null — the one shape the runtime treats as "no type declared".
|
||||
private static string? ReadRawDataType(JsonObject o)
|
||||
{
|
||||
if (!o.TryGetPropertyValue("dataType", out var node) || node is null) { return null; }
|
||||
return node is JsonValue v && v.TryGetValue<string>(out var s) ? s : node.ToJsonString();
|
||||
}
|
||||
|
||||
// Normalises a blank/whitespace-only string to null so TagConfigJson.Set omits the key rather than
|
||||
// persisting an empty string.
|
||||
private static string? Blank(string? value) => string.IsNullOrWhiteSpace(value) ? null : value;
|
||||
|
||||
@@ -68,6 +68,18 @@ public sealed class ModbusTagConfigModel
|
||||
}
|
||||
|
||||
/// <summary>Validation hook; returns an error message or null when the model is valid.</summary>
|
||||
/// <remarks>
|
||||
/// All three enums are read from the ORIGINAL key bag, not from the leniently-defaulted typed
|
||||
/// fields above: <c>ModbusTagDefinitionFactory</c> reads each with <c>TryReadEnumStrict</c>, so a
|
||||
/// present-but-invalid string rejects the whole tag at the driver (<c>BadNodeIdUnknown</c>) where
|
||||
/// the load silently substitutes <c>HoldingRegisters</c>/<c>Int16</c>/<c>BigEndian</c>. Accepting
|
||||
/// one here would let the CSV-import and API paths — which save the blob verbatim — deploy a tag
|
||||
/// the driver skips (Gitea #549). Also note a <b>wrong-but-valid</b> region is not this gate's
|
||||
/// business: only a spelling the runtime cannot read is rejected.
|
||||
/// </remarks>
|
||||
/// <returns>An error message describing the validation failure, or <c>null</c> when the model is valid.</returns>
|
||||
public string? Validate() => null;
|
||||
public string? Validate()
|
||||
=> TagConfigJson.DescribeInvalidEnum<ModbusRegion>(_bag, "region")
|
||||
?? TagConfigJson.DescribeInvalidEnum<ModbusDataType>(_bag, "dataType")
|
||||
?? TagConfigJson.DescribeInvalidEnum<ModbusByteOrder>(_bag, "byteOrder");
|
||||
}
|
||||
|
||||
@@ -53,6 +53,16 @@ public sealed class S7TagConfigModel
|
||||
|
||||
/// <summary>Validation hook; returns an error message or null when the model is valid.</summary>
|
||||
/// <returns>An error message describing the validation failure, or null when the model is valid.</returns>
|
||||
/// <remarks>
|
||||
/// The <c>dataType</c> check reads the ORIGINAL key bag rather than the leniently-defaulted
|
||||
/// <see cref="DataType"/>: <c>S7TagDefinitionFactory</c> reads it with <c>TryReadEnumStrict</c>,
|
||||
/// so a present-but-invalid string rejects the whole tag at the driver (<c>BadNodeIdUnknown</c>)
|
||||
/// where the load silently substitutes <c>Int16</c>. Accepting it here would let the CSV-import
|
||||
/// and API paths — which save the blob verbatim — deploy an unreadable tag (Gitea #549).
|
||||
/// </remarks>
|
||||
public string? Validate()
|
||||
=> string.IsNullOrWhiteSpace(Address) ? "Address is required." : null;
|
||||
{
|
||||
if (TagConfigJson.DescribeInvalidEnum<S7DataType>(_bag, "dataType") is { } dtError) { return dtError; }
|
||||
return string.IsNullOrWhiteSpace(Address) ? "Address is required." : null;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
using System.Text.Json;
|
||||
using System.Text.Json.Nodes;
|
||||
using RuntimeTagConfigJson = ZB.MOM.WW.OtOpcUa.Core.Abstractions.TagConfigJson;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors;
|
||||
|
||||
@@ -65,6 +66,33 @@ public static class TagConfigJson
|
||||
public static TEnum GetEnum<TEnum>(JsonObject o, string name, TEnum fallback) where TEnum : struct, Enum
|
||||
=> GetString(o, name) is { } s && Enum.TryParse<TEnum>(s, ignoreCase: true, out var v) ? v : fallback;
|
||||
|
||||
/// <summary>
|
||||
/// Describes a present-but-invalid enum field the way the RUNTIME parser judges it, or
|
||||
/// <c>null</c> when the field is absent, present-but-non-string, or valid.
|
||||
/// <para>
|
||||
/// <see cref="GetEnum{TEnum}"/> is deliberately lenient — it must be, because a model has to
|
||||
/// LOAD a poisoned blob to let an operator fix it — but leniency at the save gate is the
|
||||
/// Gitea #549 defect: every driver's runtime read is
|
||||
/// <c>TagConfigJson.TryReadEnumStrict</c>, which REJECTS the whole tag on a present-but-invalid
|
||||
/// string (R2-11 Phase C), so a typo the modal accepts and rewrites is a tag the driver would
|
||||
/// have skipped with <c>BadNodeIdUnknown</c> on the CSV-import and API paths, which save the
|
||||
/// blob verbatim. A model's <c>Validate()</c> therefore calls this on its ORIGINAL key bag.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The verdict and the wording both come from
|
||||
/// <see cref="RuntimeTagConfigJson.DescribeInvalidEnumText{TEnum}"/> — the same authority the
|
||||
/// runtime parsers and the deploy gate consult — rather than being restated here. Passing
|
||||
/// <see cref="GetString"/>'s null-for-non-string result reproduces the runtime's
|
||||
/// <c>Absent</c> treatment of a non-string token exactly.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <typeparam name="TEnum">The enum type expected.</typeparam>
|
||||
/// <param name="o">The model's preserved TagConfig key bag, as loaded.</param>
|
||||
/// <param name="name">The property name to check.</param>
|
||||
/// <returns>The error text, or <c>null</c> when the runtime would accept the field.</returns>
|
||||
public static string? DescribeInvalidEnum<TEnum>(JsonObject o, string name) where TEnum : struct, Enum
|
||||
=> RuntimeTagConfigJson.DescribeInvalidEnumText<TEnum>(name, GetString(o, name));
|
||||
|
||||
/// <summary>Sets a string/number/enum-name value (enums via ToString()). A null value REMOVES the key, so it is omitted from the serialised JSON.</summary>
|
||||
/// <param name="o">The JSON object to mutate.</param>
|
||||
/// <param name="name">The property name to set.</param>
|
||||
|
||||
@@ -53,6 +53,17 @@ public sealed class TwinCATTagConfigModel
|
||||
|
||||
/// <summary>Validation hook; returns an error message or null when the model is valid.</summary>
|
||||
/// <returns>An error message describing the validation failure, or null when the model is valid.</returns>
|
||||
/// <remarks>
|
||||
/// The <c>dataType</c> check reads the ORIGINAL key bag rather than the leniently-defaulted
|
||||
/// <see cref="DataType"/>: <c>TwinCATTagDefinitionFactory</c> reads it with
|
||||
/// <c>TryReadEnumStrict</c>, so a present-but-invalid string rejects the whole tag at the driver
|
||||
/// (<c>BadNodeIdUnknown</c>) where the load silently substitutes <c>DInt</c>. Accepting it here
|
||||
/// would let the CSV-import and API paths — which save the blob verbatim — deploy an unreadable
|
||||
/// tag (Gitea #549).
|
||||
/// </remarks>
|
||||
public string? Validate()
|
||||
=> string.IsNullOrWhiteSpace(SymbolPath) ? "Symbol path is required." : null;
|
||||
{
|
||||
if (TagConfigJson.DescribeInvalidEnum<TwinCATDataType>(_bag, "dataType") is { } dtError) { return dtError; }
|
||||
return string.IsNullOrWhiteSpace(SymbolPath) ? "Symbol path is required." : null;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -26,6 +26,12 @@
|
||||
<!-- #520: API-key minting + the reserved-principal rule live in Core.Authorization, because
|
||||
both ends need them — the OPC UA server resolves principals, the AdminUI authors keys. -->
|
||||
<ProjectReference Include="..\..\Core\ZB.MOM.WW.OtOpcUa.Core\ZB.MOM.WW.OtOpcUa.Core.csproj"/>
|
||||
<!-- #549: the typed tag-config editors ask Core.Abstractions' TagConfigJson whether an authored
|
||||
enum spelling is one the RUNTIME parsers accept, so authoring can never be weaker than the
|
||||
driver. That type is load-bearing for the save gate — referenced directly rather than
|
||||
consumed transitively through Core, so a future re-plumbing of Core cannot silently take it
|
||||
away. -->
|
||||
<ProjectReference Include="..\..\Core\ZB.MOM.WW.OtOpcUa.Core.Abstractions\ZB.MOM.WW.OtOpcUa.Core.Abstractions.csproj"/>
|
||||
<ProjectReference Include="..\..\Core\ZB.MOM.WW.OtOpcUa.Core.Scripting\ZB.MOM.WW.OtOpcUa.Core.Scripting.csproj"/>
|
||||
<ProjectReference Include="..\..\Core\ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions\ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions.csproj"/>
|
||||
</ItemGroup>
|
||||
|
||||
Reference in New Issue
Block a user