From feeae1371eec924c6da428e32b19de62b5ad1b37 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 16 Jun 2026 18:25:42 -0400 Subject: [PATCH] fix(multivalue): NJ-3/NJ-4/NJ-5 review fixes - NJ-3: widen per-row catch to Exception (an STJ encode failure can't abort startup); drop dead null-guard already excluded by the SQL filter - NJ-4: capture logger/instanceName in locals for the fire-and-forget normalize continuation (match the sibling pattern in this actor) - NJ-5: emit a warn-log when a malformed List value is imported verbatim; thread an optional ILogger to the sync re-import site --- .../ListValueNormalizer.cs | 19 +++++++------------ .../Actors/InstanceActor.cs | 8 +++++--- .../Import/BundleImporter.cs | 8 ++++++-- .../Serialization/ImportValueNormalizer.cs | 16 ++++++++++++++-- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ListValueNormalizer.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ListValueNormalizer.cs index abac3825..2d8659b6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ListValueNormalizer.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ListValueNormalizer.cs @@ -54,10 +54,13 @@ public static class ListValueNormalizer rewritten++; } } - catch (FormatException ex) + catch (Exception ex) { + // Never abort startup for a single bad row. FormatException from Decode is the + // expected case; the broad catch also covers an unexpected serialize failure + // (e.g. a JsonException on a non-finite value) so one poison row can't crash boot. logger?.LogWarning(ex, - "List value normalizer: skipping unparseable list value for TemplateAttribute {Id}.", + "List value normalizer: skipping unprocessable list value for TemplateAttribute {Id}.", a.Id); } } @@ -71,14 +74,6 @@ public static class ListValueNormalizer foreach (var o in overrideRows) { - if (o.ElementDataType is null) - { - logger?.LogDebug( - "List value normalizer: skipping InstanceAttributeOverride {Id} with no element type.", - o.Id); - continue; - } - try { var native = AttributeValueCodec.Encode( @@ -89,10 +84,10 @@ public static class ListValueNormalizer rewritten++; } } - catch (FormatException ex) + catch (Exception ex) { logger?.LogWarning(ex, - "List value normalizer: skipping unparseable list value for InstanceAttributeOverride {Id}.", + "List value normalizer: skipping unprocessable list value for InstanceAttributeOverride {Id}.", o.Id); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs index 64c65d3c..55b4e781 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs @@ -966,10 +966,12 @@ public class InstanceActor : ReceiveActor if (native != kvp.Value) // stored value was old-form → normalize on disk { var key = kvp.Key; - _storage.SetStaticOverrideAsync(_instanceUniqueName, key, native!) - .ContinueWith(t => _logger.LogWarning(t.Exception?.GetBaseException(), + var logger = _logger; + var instanceName = _instanceUniqueName; + _storage.SetStaticOverrideAsync(instanceName, key, native!) + .ContinueWith(t => logger.LogWarning(t.Exception?.GetBaseException(), "Failed to normalize static override {Instance}.{Attr} to native JSON", - _instanceUniqueName, key), + instanceName, key), TaskContinuationOptions.OnlyOnFaulted); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index f05852d9..43750632 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -1,6 +1,7 @@ using System.IO.Compression; using System.Security.Cryptography; using System.Text.Json; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems; using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; @@ -70,6 +71,7 @@ public sealed class BundleImporter : IBundleImporter private readonly IOptions _options; private readonly TimeProvider _timeProvider; private readonly SemanticValidator _semanticValidator; + private readonly ILogger? _logger; /// /// Initializes a new with all required dependencies. @@ -106,7 +108,8 @@ public sealed class BundleImporter : IBundleImporter IAuditService auditService, IAuditCorrelationContext correlationContext, ScadaBridgeDbContext dbContext, - SemanticValidator semanticValidator) + SemanticValidator semanticValidator, + ILogger? logger = null) { _bundleSerializer = bundleSerializer ?? throw new ArgumentNullException(nameof(bundleSerializer)); _manifestValidator = manifestValidator ?? throw new ArgumentNullException(nameof(manifestValidator)); @@ -124,6 +127,7 @@ public sealed class BundleImporter : IBundleImporter _correlationContext = correlationContext ?? throw new ArgumentNullException(nameof(correlationContext)); _dbContext = dbContext ?? throw new ArgumentNullException(nameof(dbContext)); _semanticValidator = semanticValidator ?? throw new ArgumentNullException(nameof(semanticValidator)); + _logger = logger; } /// @@ -1120,7 +1124,7 @@ public sealed class BundleImporter : IBundleImporter // stores natively — otherwise an idempotent re-import of an old-form // bundle would spuriously report a Value change. var normalizedValue = ImportValueNormalizer.NormalizeListValue( - attrDto.Value, attrDto.DataType, attrDto.ElementDataType); + attrDto.Value, attrDto.DataType, attrDto.ElementDataType, _logger, attrDto.Name); if (existingByName.TryGetValue(attrDto.Name, out var current)) { // Update only if any field actually changed. diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/ImportValueNormalizer.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/ImportValueNormalizer.cs index e4211ae1..2fcf9772 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/ImportValueNormalizer.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/ImportValueNormalizer.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging; using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; @@ -29,7 +30,14 @@ internal static class ImportValueNormalizer /// The attribute value as carried by the bundle DTO. /// The attribute's declared data type. /// The List element type (null for scalars). - public static string? NormalizeListValue(string? value, DataType dataType, DataType? elementType) + /// Optional logger; a warning is emitted when a malformed value is left as-is. + /// Optional attribute name for the diagnostic message. + public static string? NormalizeListValue( + string? value, + DataType dataType, + DataType? elementType, + ILogger? logger = null, + string? attributeName = null) { if (dataType != DataType.List || string.IsNullOrEmpty(value)) { @@ -41,10 +49,14 @@ internal static class ImportValueNormalizer return AttributeValueCodec.Encode( AttributeValueCodec.Decode(value, DataType.List, elementType)); } - catch (FormatException) + catch (FormatException ex) { // Leave malformed values exactly as imported; the DB normalizer is // the backstop. Never abort the import for a single bad value. + logger?.LogWarning(ex, + "Bundle import: could not normalize List value for attribute {Attribute}; " + + "importing verbatim (the central DB normalizer is the backstop).", + attributeName ?? "(unknown)"); return value; } }