From 11950b0a8ef4b68f178dccfecbea11b5a749a763 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 28 May 2026 08:48:44 -0400 Subject: [PATCH] =?UTF-8?q?fix(correctness):=20close=20Theme=2010=20?= =?UTF-8?q?=E2=80=94=205=20data-integrity=20/=20serialisation=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final themed batch. 5 well-localised correctness fixes. Serialisation precision: - ESG-020: DatabaseGateway.JsonElementToParameterValue probes TryGetInt64 → TryGetDecimal → GetDouble, so a script's high-precision decimal SQL parameter survives the cached-write retry round-trip without silent precision loss. 3 new regression tests. Template engine correctness: - TE-018: DiffService gains ComputeConnectionsDiff over FlattenedConfiguration.Connections, mirroring the existing entity-diff shape and pairing with the Theme 1 TE-017 hash-coverage fix. A ConfigurationDiff record extension in Commons is flagged as a follow-up. - TE-019: TemplateResolver.BuildInheritanceChain now walks via the int? ParentTemplateId directly — only null means "no parent". A real Id of 0 (the prior special-cased sentinel) now walks the chain like any other node, matching the TemplateEngine-013 CycleDetector fix. Regression of TE-013 closed. - TE-020: All 5 Create* paths in TemplateService + SharedScriptService re-ordered to save-first → log-with-real-Id → save-audit (matching the InstanceService pattern). Create* audit rows no longer carry a literal "0" EntityId. Doc deferral: - Transport-012: Component-Transport.md §Audit Trail now spells out that the BundleImportId repository filter IS wired (in CentralUiRepository), but the Audit-Log-Viewer UI dropdown + summary-row hyperlink are a deferred CentralUI follow-up. CLI workaround documented (audit query --bundle-import-id). 11+ new regression tests (3 ESG, 4 DiffService, 3 TemplateResolver, 4 TemplateService, 1 SharedScriptService). Build clean; ESG 72/72, TemplateEngine 324/324. README regenerated: 1 pending of 481 total. Session-to-date: 135 of 136 originally-open Theme findings closed across 10 themes in 10 commits. --- .../ExternalSystemGateway/findings.md | 6 +- code-reviews/README.md | 24 +-- code-reviews/TemplateEngine/findings.md | 14 +- code-reviews/Transport/findings.md | 12 +- docs/requirements/Component-Transport.md | 2 +- .../DatabaseGateway.cs | 17 +- .../Flattening/DiffService.cs | 53 +++++- .../SharedScriptService.cs | 8 +- .../TemplateResolver.cs | 18 +- .../TemplateService.cs | 30 ++- .../DatabaseGatewayTests.cs | 61 ++++++ .../Flattening/DiffServiceTests.cs | 118 ++++++++++++ .../SharedScriptServiceTests.cs | 44 +++++ .../TemplateResolverTests.cs | 76 ++++++++ .../TemplateServiceTests.cs | 179 +++++++++++++++++- 15 files changed, 620 insertions(+), 42 deletions(-) diff --git a/code-reviews/ExternalSystemGateway/findings.md b/code-reviews/ExternalSystemGateway/findings.md index 9c105f06..5389cc16 100644 --- a/code-reviews/ExternalSystemGateway/findings.md +++ b/code-reviews/ExternalSystemGateway/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -1118,9 +1118,11 @@ and produces a `"Timeout calling..."` (not `"Connection error to..."`) error. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs:185-193` | +**Resolution (2026-05-28):** `JsonElementToParameterValue` now probes `TryGetInt64` → `TryGetDecimal` → `GetDouble`, so a JSON number that fits in `decimal` materialises as a `decimal` (preserving the script's authored precision on cached-write retries) and only genuinely out-of-decimal-range values fall through to `double`. Regression test `JsonElementToParameterValue_DecimalShapedNumber_PreservesPrecisionViaDecimal` round-trips `1234567890.1234567890` through a `JsonElement` and asserts the result is a `decimal` carrying the original precision; companion tests guard the long-fast-path and the out-of-range-double fallback. + **Description** `DatabaseGateway.JsonElementToParameterValue` materialises the buffered cached-write diff --git a/code-reviews/README.md b/code-reviews/README.md index 16a09ab9..8d8a1b67 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -41,9 +41,9 @@ module file and counted in **Total**. |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 5 | -| Low | 1 | -| **Total** | **6** | +| Medium | 1 | +| Low | 0 | +| **Total** | **1** | ## Module Status @@ -58,7 +58,7 @@ module file and counted in **Total**. | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 24 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 24 | -| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 23 | +| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 25 | @@ -70,8 +70,8 @@ module file and counted in **Total**. | [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 26 | | [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 24 | -| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/0 | 3 | 22 | -| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 12 | +| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | +| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 12 | ## Pending Findings @@ -88,18 +88,12 @@ _None open._ _None open._ -### Medium (5) +### Medium (1) | ID | Module | Title | |----|--------|-------| | AuditLog-001 | [AuditLog](AuditLog/findings.md) | Combined-telemetry transport is plumbed end-to-end but never invoked in production | -| ExternalSystemGateway-020 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `JsonElementToParameterValue` silently downcasts non-Int64 JSON numbers to `double`, losing precision for `decimal` SQL parameters on retry | -| TemplateEngine-018 | [TemplateEngine](TemplateEngine/findings.md) | `DiffService` reports no entries for added/removed/changed connections | -| TemplateEngine-019 | [TemplateEngine](TemplateEngine/findings.md) | `TemplateResolver.BuildInheritanceChain` still uses the `0`-as-no-parent sentinel that was removed from `CycleDetector` | -| TemplateEngine-020 | [TemplateEngine](TemplateEngine/findings.md) | `Create*` audit entries are written with `EntityId = "0"` before `SaveChangesAsync` populates the real key | -### Low (1) +### Low (0) -| ID | Module | Title | -|----|--------|-------| -| Transport-012 | [Transport](Transport/findings.md) | "Bundle Import" filter promised in design doc not surfaced in Configuration Audit Log Viewer UI | +_None open._ diff --git a/code-reviews/TemplateEngine/findings.md b/code-reviews/TemplateEngine/findings.md index 0e12fd5b..751cf176 100644 --- a/code-reviews/TemplateEngine/findings.md +++ b/code-reviews/TemplateEngine/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 | +| Open findings | 1 | ## Summary @@ -913,9 +913,11 @@ TemplateEngine-018. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Flattening/DiffService.cs:19` | +**Resolution (2026-05-28):** Added `DiffService.ComputeConnectionsDiff(oldConfig, newConfig)`, which mirrors the existing attribute/alarm/script `ComputeEntityDiff` shape over the `FlattenedConfiguration.Connections` map and emits `DiffEntry` Added / Removed / Changed entries keyed by connection name (delegating equality to the existing `ConnectionsEqual` helper). Kept the new diff as a parallel method on `DiffService` rather than extending `ConfigurationDiff` (which lives in `ScadaLink.Commons`, outside this fix's scope); the public-record extension and Central UI plumbing are a paired Commons follow-up. Regression tests: `ComputeConnectionsDiff_NewBindingAdded_ReportedAsAdded`, `ComputeConnectionsDiff_BindingCleared_ReportedAsRemoved`, `ComputeConnectionsDiff_EndpointEdit_ReportedAsChanged`, `ComputeConnectionsDiff_IdenticalConnections_NoEntries`. + **Description** `DiffService.ComputeDiff` returns a `ConfigurationDiff` with `AttributeChanges`, @@ -954,9 +956,11 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/TemplateResolver.cs:117`, `src/ScadaLink.TemplateEngine/TemplateResolver.cs:123` | +**Resolution (2026-05-28):** `BuildInheritanceChain` now walks the parent chain via the `int?` `ParentTemplateId` directly — only a missing (`null`) value means "no parent", so a real template Id of 0 walks the chain like any other node (matching the duplicate-tolerant `BuildLookup` and the TemplateEngine-013 `CycleDetector` fix). Regression tests: `BuildInheritanceChain_RealIdZero_IsTreatedAsParentReferenceNotAsNoParent`, `BuildInheritanceChain_ParentChainThroughIdZero_DoesNotTruncateChainAtZero`, and the end-to-end `ResolveAllMembers_TemplateWithRealIdZero_StillResolvesItsMembers`. + **Description** TemplateEngine-013 removed the `0`-as-no-parent sentinel from `CycleDetector` @@ -1014,9 +1018,11 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/TemplateService.cs:77`, `src/ScadaLink.TemplateEngine/TemplateService.cs:256`, `src/ScadaLink.TemplateEngine/TemplateService.cs:407`, `src/ScadaLink.TemplateEngine/TemplateService.cs:556`, `src/ScadaLink.TemplateEngine/TemplateService.cs:734`, `src/ScadaLink.TemplateEngine/SharedScriptService.cs:71` | +**Resolution (2026-05-28):** Every `Create*` path in `TemplateService` (`CreateTemplateAsync`, `AddAttributeAsync`, `AddAlarmAsync`, `AddScriptAsync`, `AddCompositionAsync`) and `SharedScriptService.CreateSharedScriptAsync` now follows the `InstanceService.CreateInstanceAsync` shape — save the entity first so EF Core populates the auto-generated key, then log the audit row with the real `entity.Id`, then save the audit row. `AddCompositionAsync` already saved the composition row inside `CreateCascadedCompositionAsync` before returning, so only its `LogAsync` call needed to switch from `"0"` to `composition.Id.ToString()`. Regression tests assert the captured audit `entityId` equals the post-save id (not `"0"`): `CreateTemplate_AuditRowCarriesRealTemplateIdNotLiteralZero`, `AddAttribute_AuditRowCarriesRealAttributeIdNotLiteralZero`, `AddAlarm_AuditRowCarriesRealAlarmIdNotLiteralZero`, `AddScript_AuditRowCarriesRealScriptIdNotLiteralZero`, and `CreateSharedScript_AuditRowCarriesRealScriptIdNotLiteralZero`. + **Description** `IAuditService.LogAsync` takes a `string entityId` argument and `TemplateService diff --git a/code-reviews/Transport/findings.md b/code-reviews/Transport/findings.md index d3ef9a36..a96ad103 100644 --- a/code-reviews/Transport/findings.md +++ b/code-reviews/Transport/findings.md @@ -499,7 +499,7 @@ bundle prompt is surfaced AFTER the manifest+hash check, and that a dedicated |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `docs/requirements/Component-Transport.md` §Audit Trail, `src/ScadaLink.ConfigurationDatabase/Repositories/CentralUiRepository.cs:148` | **Description** @@ -518,6 +518,12 @@ it's reasonable to flag. Either implement the filter dropdown + summary-row link in the Configuration Audit Log Viewer, or note the deferral in the design doc. -**Resolution** +**Resolution (2026-05-28):** -_Unresolved._ +Took the "note the deferral in the design doc" path. `docs/requirements/Component-Transport.md` +§Audit Trail's "Correlation:" paragraph now spells out that the repository filter on +`BundleImportId` IS wired (in `CentralUiRepository.QueryConfigurationAuditAsync`) +but the Audit-Log-Viewer UI surface — the dropdown + `BundleImported` hyperlink — +is a deferred UI follow-up. Operators have a workaround via the existing +`audit query --bundle-import-id` CLI flag. The UI work belongs in the CentralUI +backlog; implementing it here would expand scope beyond a doc fix. diff --git a/docs/requirements/Component-Transport.md b/docs/requirements/Component-Transport.md index 2aa2950f..64ec73e5 100644 --- a/docs/requirements/Component-Transport.md +++ b/docs/requirements/Component-Transport.md @@ -257,7 +257,7 @@ Import flows through the same audited repository methods the UI and CLI use, so | Imported alarm references missing on-trigger script | `BundleImportAlarmScriptUnresolved` (warning; alarm FK left null) | | Imported template's composition references missing target template | `BundleImportCompositionUnresolved` (warning; composition row not written) | -**Correlation:** every per-entity row written during an import carries a new optional `BundleImportId` column (the GUID of the parent `BundleImported` summary row). The existing Configuration Audit Log Viewer gains a **Bundle Import** filter that surfaces all rows for a given import. The `BundleImported` summary row links to the filtered view. +**Correlation:** every per-entity row written during an import carries a new optional `BundleImportId` column (the GUID of the parent `BundleImported` summary row). The repository layer (`CentralUiRepository.QueryConfigurationAuditAsync`) already accepts a `BundleImportId` filter parameter. The Configuration Audit Log Viewer UI surface — a "Bundle Import" filter dropdown and a hyperlink on the `BundleImported` summary row that pre-populates that filter — is a deferred UI follow-up (tracked under Transport-012 in the code review backlog). Until then, an operator can drive the same filter via the CLI `audit query --bundle-import-id `. **Schema change:** one EF migration adds: diff --git a/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs b/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs index 78b28aca..d35a3584 100644 --- a/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs +++ b/src/ScadaLink.ExternalSystemGateway/DatabaseGateway.cs @@ -201,10 +201,23 @@ public class DatabaseGateway : IDatabaseGateway return true; } - private static object JsonElementToParameterValue(JsonElement element) => element.ValueKind switch + // ExternalSystemGateway-020: a JSON number that does not fit in Int64 must + // prefer decimal over double — a script's decimal SQL parameter is + // serialised as JSON without a type tag, and downcasting it to double on + // the cached-write retry path silently loses precision (e.g. + // 1234567890.1234567890 -> 1234567890.1234567 as a binary float). Probe + // long first (whole-number fast path), then decimal (preserves authored + // precision for typical money/measurement values), and only fall through + // to double for genuinely out-of-decimal-range values (very large + // scientific-notation floats). + internal static object JsonElementToParameterValue(JsonElement element) => element.ValueKind switch { JsonValueKind.String => (object?)element.GetString() ?? DBNull.Value, - JsonValueKind.Number => element.TryGetInt64(out var l) ? l : element.GetDouble(), + JsonValueKind.Number => element.TryGetInt64(out var l) + ? l + : element.TryGetDecimal(out var dec) + ? dec + : element.GetDouble(), JsonValueKind.True => true, JsonValueKind.False => false, JsonValueKind.Null or JsonValueKind.Undefined => DBNull.Value, diff --git a/src/ScadaLink.TemplateEngine/Flattening/DiffService.cs b/src/ScadaLink.TemplateEngine/Flattening/DiffService.cs index 731907aa..08238964 100644 --- a/src/ScadaLink.TemplateEngine/Flattening/DiffService.cs +++ b/src/ScadaLink.TemplateEngine/Flattening/DiffService.cs @@ -139,10 +139,7 @@ public class DiffService /// Compares two instances for equality across /// the fields that travel in the deployment package: protocol, primary and /// backup configuration JSON, and failover retry count. Used by callers that - /// need to detect connection-endpoint drift; the public diff shape only - /// exposes attribute / alarm / script changes today (see TemplateEngine-018 - /// for the diff-shape extension that surfaces added / removed / changed - /// connections in the UI). + /// need to detect connection-endpoint drift. /// /// First connection configuration. /// Second connection configuration. @@ -157,4 +154,52 @@ public class DiffService a.BackupConfigurationJson == b.BackupConfigurationJson && a.FailoverRetryCount == b.FailoverRetryCount; } + + /// + /// TemplateEngine-018: produces a per-connection diff between two flattened + /// configurations, emitting Added / Removed / Changed entries keyed by the + /// connection name. Mirrors the existing + /// shape used for attributes / alarms / scripts but is exposed as a separate + /// method because in + /// ScadaLink.Commons does not yet carry a ConnectionChanges + /// slot — the public diff record will be extended in a paired Commons change + /// (this file is the only one in this fix's scope). A null + /// Connections dictionary on either side is treated as the empty map. + /// + /// The previously deployed configuration, or null + /// for first-time deployment. + /// The current flattened configuration. + /// Added / Removed / Changed entries keyed by connection name, + /// sorted ordinally by . + public IReadOnlyList> ComputeConnectionsDiff( + FlattenedConfiguration? oldConfig, + FlattenedConfiguration newConfig) + { + ArgumentNullException.ThrowIfNull(newConfig); + + // Project both sides to (name, config) pairs. A null Connections + // dictionary models the no-connections case (first deploy, or all + // bindings cleared) — treat it as empty so the diff still reports the + // counterpart side's entries as Added / Removed rather than throwing. + var oldPairs = (oldConfig?.Connections ?? new Dictionary()) + .Select(kvp => (kvp.Key, kvp.Value)) + .ToList(); + var newPairs = (newConfig.Connections ?? new Dictionary()) + .Select(kvp => (kvp.Key, kvp.Value)) + .ToList(); + + return ComputeEntityDiff( + oldPairs, + newPairs, + pair => pair.Key, + (a, b) => ConnectionsEqual(a.Value, b.Value)) + .Select(entry => new DiffEntry + { + CanonicalName = entry.CanonicalName, + ChangeType = entry.ChangeType, + OldValue = entry.OldValue.Value, + NewValue = entry.NewValue.Value, + }) + .ToList(); + } } diff --git a/src/ScadaLink.TemplateEngine/SharedScriptService.cs b/src/ScadaLink.TemplateEngine/SharedScriptService.cs index 7ee89f65..9025c393 100644 --- a/src/ScadaLink.TemplateEngine/SharedScriptService.cs +++ b/src/ScadaLink.TemplateEngine/SharedScriptService.cs @@ -67,8 +67,14 @@ public class SharedScriptService ReturnDefinition = returnDefinition }; + // TemplateEngine-020: save the entity first so EF Core populates the + // auto-generated key, then write the audit row with the real + // script.Id, then save the audit row. The pre-fix order logged + // EntityId = "0" because the audit row was queued before + // SaveChangesAsync ran. await _repository.AddSharedScriptAsync(script, cancellationToken); - await _auditService.LogAsync(user, "Create", "SharedScript", "0", name, script, cancellationToken); + await _repository.SaveChangesAsync(cancellationToken); + await _auditService.LogAsync(user, "Create", "SharedScript", script.Id.ToString(), name, script, cancellationToken); await _repository.SaveChangesAsync(cancellationToken); return Result.Success(script); diff --git a/src/ScadaLink.TemplateEngine/TemplateResolver.cs b/src/ScadaLink.TemplateEngine/TemplateResolver.cs index 7313bc8e..07c56bef 100644 --- a/src/ScadaLink.TemplateEngine/TemplateResolver.cs +++ b/src/ScadaLink.TemplateEngine/TemplateResolver.cs @@ -103,6 +103,16 @@ public static class TemplateResolver /// /// Gets the inheritance chain from root ancestor to the specified template. /// + /// + /// TemplateEngine-019: the parent walk uses the + /// directly — only a missing + /// (null) value means "no parent". The legacy 0-as-no-parent + /// sentinel that was removed from CycleDetector in the + /// TemplateEngine-013 fix had silently truncated chains whenever a real + /// template id of 0 appeared (e.g. import-staging / not-yet-saved rows); + /// the duplicate-tolerant BuildLookup upstream means an Id of 0 is + /// a valid node here and must walk the chain like any other. + /// /// The template ID to build the chain for. /// A dictionary mapping template IDs to templates. /// A read-only list of templates from root to the specified template. @@ -111,16 +121,16 @@ public static class TemplateResolver IReadOnlyDictionary lookup) { var chain = new List