diff --git a/code-reviews/README.md b/code-reviews/README.md index 22911eb3..1aec089c 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,10 +40,10 @@ module file and counted in **Total**. | Severity | Open findings | |----------|---------------| | Critical | 0 | -| High | 10 | +| High | 0 | | Medium | 46 | | Low | 90 | -| **Total** | **146** | +| **Total** | **136** | ## Module Status @@ -54,24 +54,24 @@ module file and counted in **Total**. | [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 33 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/4 | 4 | 14 | | [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/6 | 9 | 23 | -| [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/1/5 | 7 | 22 | +| [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/5 | 6 | 22 | | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/4/5 | 9 | 24 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | -| [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/1/5 | 7 | 24 | -| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/2/3 | 6 | 23 | +| [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/5 | 6 | 24 | +| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 23 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 22 | -| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/3/4 | 8 | 25 | +| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/4 | 7 | 25 | | [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 23 | | [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 10 | -| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/2/3 | 6 | 25 | +| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 25 | | [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 6 | -| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/2/6 | 9 | 23 | +| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/6 | 8 | 23 | | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 26 | -| [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/3/3 | 7 | 24 | -| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/4/1 | 6 | 22 | -| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/2/2/4 | 8 | 12 | +| [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/3 | 6 | 24 | +| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/4/1 | 5 | 22 | +| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 12 | ## Pending Findings @@ -84,20 +84,9 @@ description, location, recommendation — lives in the module's `findings.md`. _None open._ -### High (10) +### High (0) -| ID | Module | Title | -|----|--------|-------| -| Communication-016 | [Communication](Communication/findings.md) | `HandleConnectionStateChanged` is dead code — the documented disconnect-cleanup workflow never fires | -| DeploymentManager-018 | [DeploymentManager](DeploymentManager/findings.md) | Reconciliation force-sets `Enabled`, overwriting an intentional `Disabled` after central failover | -| ExternalSystemGateway-018 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `DeliverBufferedAsync` lets `JsonException` propagate, turning a corrupt buffered row into a permanent retry-forever poison message | -| InboundAPI-022 | [InboundAPI](InboundAPI/findings.md) | `IActiveNodeGate` has no production registration in Host — standby-node gating is silently disabled in production | -| NotificationService-019 | [NotificationService](NotificationService/findings.md) | `NotificationDeliveryService` and `INotificationDeliveryService` are orphaned by the central-only redesign | -| SiteEventLogging-016 | [SiteEventLogging](SiteEventLogging/findings.md) | `From`/`To` filters compare non-normalised ISO 8601 strings against UTC-stored timestamps | -| StoreAndForward-018 | [StoreAndForward](StoreAndForward/findings.md) | Notification corrupt-payload parks the buffered message, contradicting the "notifications do not park" design invariant | -| TemplateEngine-017 | [TemplateEngine](TemplateEngine/findings.md) | Revision hash and diff both ignore `Description` and `Connections`, defeating staleness detection for real deployment changes | -| Transport-001 | [Transport](Transport/findings.md) | Template Overwrite never syncs attributes / alarms / scripts | -| Transport-002 | [Transport](Transport/findings.md) | ExternalSystem Overwrite never syncs methods | +_None open._ ### Medium (46) diff --git a/code-reviews/Transport/findings.md b/code-reviews/Transport/findings.md index 22751c65..e48d4ab6 100644 --- a/code-reviews/Transport/findings.md +++ b/code-reviews/Transport/findings.md @@ -53,9 +53,23 @@ entry-count / per-entry decompression cap). |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/Import/BundleImporter.cs:844-851` | +**Resolution** — Extended `ApplyTemplatesAsync`'s Overwrite branch with three +new private diff-and-merge helpers (`SyncTemplateAttributesAsync`, +`SyncTemplateAlarmsAsync`, `SyncTemplateScriptsAsync`) that compare the bundle +DTO's children against the tracked existing template's collections by name and +stage add / update / delete via the audited repository methods. Each detected +change emits one of the per-field audit events the design doc enumerates +(`TemplateAttributeAdded` / `TemplateAttributeUpdated` / +`TemplateAttributeDeleted` and the alarm / script analogues); the existing +`ResolveAlarmScriptLinksAsync` and `ResolveCompositionEdgesAsync` passes rewire +the alarm→script FK and composition graph against the post-merge state with no +changes — Overwrite-on-alarms resets `OnTriggerScriptId` so Pass A is +authoritative. Regression test: +`BundleImporterApplyTests.ApplyAsync_Overwrite_synchronises_attributes_alarms_and_scripts_to_bundle`. + **Description** The `ResolutionAction.Overwrite` branch in `ApplyTemplatesAsync` only writes @@ -80,19 +94,24 @@ composition rewire passes against the post-merge state. Emit the per-field audit rows the design doc enumerates. Add an integration test that overwrites a template whose Scripts / Attributes / Alarms differ. -**Resolution** - -_Unresolved._ - ### Transport-002 — ExternalSystem Overwrite never syncs methods | | | |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/Import/BundleImporter.cs:1213-1221` | +**Resolution** — Added a private `SyncExternalSystemMethodsAsync` helper to +`BundleImporter` modeled on the T-001 `SyncTemplate*Async` helpers (dictionary- +by-name diff, repo Add / Update / Delete, scalar-field-compare gating, one +audit row per change). The `ApplyExternalSystemsAsync` Overwrite branch now +calls it after the parent scalar update; the helper emits +`ExternalSystemMethodAdded` / `ExternalSystemMethodUpdated` / +`ExternalSystemMethodDeleted` per change. Covered by +`ApplyAsync_Overwrite_synchronises_external_system_methods_to_bundle`. + **Description** `ApplyExternalSystemsAsync` Overwrite path writes `EndpointUrl`, `AuthType`, and @@ -111,10 +130,6 @@ diff classification in `ArtifactDiff.CompareExternalSystem`) and emit the per-method audit rows. Add a test that overwrites an external system whose methods differ. -**Resolution** - -_Unresolved._ - ### Transport-003 — Unlock lockout is enforced only client-side; server session is never marked Locked | | | diff --git a/src/ScadaLink.Transport/Import/BundleImporter.cs b/src/ScadaLink.Transport/Import/BundleImporter.cs index 4e9d3cb4..022fd652 100644 --- a/src/ScadaLink.Transport/Import/BundleImporter.cs +++ b/src/ScadaLink.Transport/Import/BundleImporter.cs @@ -975,6 +975,17 @@ public sealed class BundleImporter : IBundleImporter await _templateRepo.UpdateTemplateAsync(ex, ct).ConfigureAwait(false); await _auditService.LogAsync(user, "Update", "Template", ex.Id.ToString(), ex.Name, new { ex.Name, ex.Description, ex.FolderId }, ct).ConfigureAwait(false); + // T-001: Overwrite must also synchronise child collections — + // attributes / alarms / scripts diverging between the bundle + // and the target must round-trip. Composition rewire is + // handled by ResolveCompositionEdgesAsync after the global + // flush; alarm→script FKs are rewired by + // ResolveAlarmScriptLinksAsync. The helpers below stage the + // child diffs (add / update / delete) onto the tracked + // entity and emit one audit row per detected change. + await SyncTemplateAttributesAsync(ex, dto, user, ct).ConfigureAwait(false); + await SyncTemplateAlarmsAsync(ex, dto, user, ct).ConfigureAwait(false); + await SyncTemplateScriptsAsync(ex, dto, user, ct).ConfigureAwait(false); summary.Overwritten++; break; case ResolutionAction.Add: @@ -1055,6 +1066,329 @@ public sealed class BundleImporter : IBundleImporter return t; } + /// + /// T-001 — Overwrite child sync (attributes). Diffs the DTO's + /// Attributes against the existing template's attribute collection + /// by name and stages add / update / delete on the tracked entity. Emits + /// one TemplateAttributeAdded / TemplateAttributeUpdated / + /// TemplateAttributeDeleted audit row per detected change — the + /// per-field rows the design doc's Configuration Audit Trail table + /// enumerates for the "Template overwritten" action. + /// + /// Update detection compares every scalar field (Value, DataType, + /// IsLocked, Description, DataSourceReference) — no field change → no + /// audit row, so an idempotent overwrite produces no noise. + /// + /// + private async Task SyncTemplateAttributesAsync( + Template ex, + TemplateDto dto, + string user, + CancellationToken ct) + { + var existingByName = ex.Attributes.ToDictionary(a => a.Name, a => a, StringComparer.Ordinal); + var dtoByName = dto.Attributes.ToDictionary(a => a.Name, a => a, StringComparer.Ordinal); + + // Deletes — attributes present on the target but not in the bundle. + foreach (var existing in existingByName.Values.ToList()) + { + if (dtoByName.ContainsKey(existing.Name)) continue; + await _templateRepo.DeleteTemplateAttributeAsync(existing.Id, ct).ConfigureAwait(false); + ex.Attributes.Remove(existing); + await _auditService.LogAsync( + user, + "TemplateAttributeDeleted", + "TemplateAttribute", + existing.Id.ToString(), + $"{ex.Name}.{existing.Name}", + new { TemplateName = ex.Name, AttributeName = existing.Name }, + ct).ConfigureAwait(false); + } + + // Adds + Updates. + foreach (var attrDto in dto.Attributes) + { + if (existingByName.TryGetValue(attrDto.Name, out var current)) + { + // Update only if any field actually changed. + bool changed = + !string.Equals(current.Value, attrDto.Value, StringComparison.Ordinal) || + current.DataType != attrDto.DataType || + current.IsLocked != attrDto.IsLocked || + !string.Equals(current.Description, attrDto.Description, StringComparison.Ordinal) || + !string.Equals(current.DataSourceReference, attrDto.DataSourceReference, StringComparison.Ordinal); + if (!changed) continue; + + current.Value = attrDto.Value; + current.DataType = attrDto.DataType; + current.IsLocked = attrDto.IsLocked; + current.Description = attrDto.Description; + current.DataSourceReference = attrDto.DataSourceReference; + await _templateRepo.UpdateTemplateAttributeAsync(current, ct).ConfigureAwait(false); + await _auditService.LogAsync( + user, + "TemplateAttributeUpdated", + "TemplateAttribute", + current.Id.ToString(), + $"{ex.Name}.{current.Name}", + new + { + TemplateName = ex.Name, + AttributeName = current.Name, + current.Value, + current.DataType, + current.IsLocked, + current.Description, + current.DataSourceReference, + }, + ct).ConfigureAwait(false); + } + else + { + var newAttr = new TemplateAttribute(attrDto.Name) + { + Value = attrDto.Value, + DataType = attrDto.DataType, + IsLocked = attrDto.IsLocked, + Description = attrDto.Description, + DataSourceReference = attrDto.DataSourceReference, + }; + ex.Attributes.Add(newAttr); + await _auditService.LogAsync( + user, + "TemplateAttributeAdded", + "TemplateAttribute", + "0", + $"{ex.Name}.{newAttr.Name}", + new + { + TemplateName = ex.Name, + AttributeName = newAttr.Name, + newAttr.Value, + newAttr.DataType, + newAttr.IsLocked, + newAttr.Description, + newAttr.DataSourceReference, + }, + ct).ConfigureAwait(false); + } + } + } + + /// + /// T-001 — Overwrite child sync (alarms). Mirrors + /// for the alarm collection. + /// Updated / added alarms have their OnTriggerScriptId cleared so + /// the post-flush pass re-binds + /// the FK from the DTO's OnTriggerScriptName against the synced + /// script collection. Audit rows: TemplateAlarmAdded / + /// TemplateAlarmUpdated / TemplateAlarmDeleted. + /// + private async Task SyncTemplateAlarmsAsync( + Template ex, + TemplateDto dto, + string user, + CancellationToken ct) + { + var existingByName = ex.Alarms.ToDictionary(a => a.Name, a => a, StringComparer.Ordinal); + var dtoByName = dto.Alarms.ToDictionary(a => a.Name, a => a, StringComparer.Ordinal); + + foreach (var existing in existingByName.Values.ToList()) + { + if (dtoByName.ContainsKey(existing.Name)) continue; + await _templateRepo.DeleteTemplateAlarmAsync(existing.Id, ct).ConfigureAwait(false); + ex.Alarms.Remove(existing); + await _auditService.LogAsync( + user, + "TemplateAlarmDeleted", + "TemplateAlarm", + existing.Id.ToString(), + $"{ex.Name}.{existing.Name}", + new { TemplateName = ex.Name, AlarmName = existing.Name }, + ct).ConfigureAwait(false); + } + + foreach (var alarmDto in dto.Alarms) + { + if (existingByName.TryGetValue(alarmDto.Name, out var current)) + { + bool changed = + !string.Equals(current.Description, alarmDto.Description, StringComparison.Ordinal) || + current.PriorityLevel != alarmDto.PriorityLevel || + current.TriggerType != alarmDto.TriggerType || + !string.Equals(current.TriggerConfiguration, alarmDto.TriggerConfiguration, StringComparison.Ordinal) || + current.IsLocked != alarmDto.IsLocked; + if (!changed) + { + // Always reset the script FK on Overwrite so the post-flush + // resolve pass owns the binding (the DTO's script name is + // the authoritative reference); leaving a stale FK would + // silently survive Overwrite when the user expected the + // bundle to be the source of truth. + if ((current.OnTriggerScriptId is not null) || + !string.IsNullOrEmpty(alarmDto.OnTriggerScriptName)) + { + current.OnTriggerScriptId = null; + await _templateRepo.UpdateTemplateAlarmAsync(current, ct).ConfigureAwait(false); + } + continue; + } + + current.Description = alarmDto.Description; + current.PriorityLevel = alarmDto.PriorityLevel; + current.TriggerType = alarmDto.TriggerType; + current.TriggerConfiguration = alarmDto.TriggerConfiguration; + current.IsLocked = alarmDto.IsLocked; + current.OnTriggerScriptId = null; // re-resolved post-flush. + await _templateRepo.UpdateTemplateAlarmAsync(current, ct).ConfigureAwait(false); + await _auditService.LogAsync( + user, + "TemplateAlarmUpdated", + "TemplateAlarm", + current.Id.ToString(), + $"{ex.Name}.{current.Name}", + new + { + TemplateName = ex.Name, + AlarmName = current.Name, + current.Description, + current.PriorityLevel, + current.TriggerType, + current.TriggerConfiguration, + current.IsLocked, + OnTriggerScriptName = alarmDto.OnTriggerScriptName, + }, + ct).ConfigureAwait(false); + } + else + { + var newAlarm = new TemplateAlarm(alarmDto.Name) + { + Description = alarmDto.Description, + PriorityLevel = alarmDto.PriorityLevel, + TriggerType = alarmDto.TriggerType, + TriggerConfiguration = alarmDto.TriggerConfiguration, + IsLocked = alarmDto.IsLocked, + }; + ex.Alarms.Add(newAlarm); + await _auditService.LogAsync( + user, + "TemplateAlarmAdded", + "TemplateAlarm", + "0", + $"{ex.Name}.{newAlarm.Name}", + new + { + TemplateName = ex.Name, + AlarmName = newAlarm.Name, + newAlarm.Description, + newAlarm.PriorityLevel, + newAlarm.TriggerType, + newAlarm.TriggerConfiguration, + newAlarm.IsLocked, + OnTriggerScriptName = alarmDto.OnTriggerScriptName, + }, + ct).ConfigureAwait(false); + } + } + } + + /// + /// T-001 — Overwrite child sync (scripts). Mirrors + /// for the script collection. + /// Audit rows: TemplateScriptAdded / TemplateScriptUpdated / + /// TemplateScriptDeleted. + /// + private async Task SyncTemplateScriptsAsync( + Template ex, + TemplateDto dto, + string user, + CancellationToken ct) + { + var existingByName = ex.Scripts.ToDictionary(s => s.Name, s => s, StringComparer.Ordinal); + var dtoByName = dto.Scripts.ToDictionary(s => s.Name, s => s, StringComparer.Ordinal); + + foreach (var existing in existingByName.Values.ToList()) + { + if (dtoByName.ContainsKey(existing.Name)) continue; + await _templateRepo.DeleteTemplateScriptAsync(existing.Id, ct).ConfigureAwait(false); + ex.Scripts.Remove(existing); + await _auditService.LogAsync( + user, + "TemplateScriptDeleted", + "TemplateScript", + existing.Id.ToString(), + $"{ex.Name}.{existing.Name}", + new { TemplateName = ex.Name, ScriptName = existing.Name }, + ct).ConfigureAwait(false); + } + + foreach (var scriptDto in dto.Scripts) + { + if (existingByName.TryGetValue(scriptDto.Name, out var current)) + { + bool changed = + !string.Equals(current.Code, scriptDto.Code, StringComparison.Ordinal) || + !string.Equals(current.TriggerType, scriptDto.TriggerType, StringComparison.Ordinal) || + !string.Equals(current.TriggerConfiguration, scriptDto.TriggerConfiguration, StringComparison.Ordinal) || + !string.Equals(current.ParameterDefinitions, scriptDto.ParameterDefinitions, StringComparison.Ordinal) || + !string.Equals(current.ReturnDefinition, scriptDto.ReturnDefinition, StringComparison.Ordinal) || + current.IsLocked != scriptDto.IsLocked; + if (!changed) continue; + + current.Code = scriptDto.Code; + current.TriggerType = scriptDto.TriggerType; + current.TriggerConfiguration = scriptDto.TriggerConfiguration; + current.ParameterDefinitions = scriptDto.ParameterDefinitions; + current.ReturnDefinition = scriptDto.ReturnDefinition; + current.IsLocked = scriptDto.IsLocked; + await _templateRepo.UpdateTemplateScriptAsync(current, ct).ConfigureAwait(false); + await _auditService.LogAsync( + user, + "TemplateScriptUpdated", + "TemplateScript", + current.Id.ToString(), + $"{ex.Name}.{current.Name}", + new + { + TemplateName = ex.Name, + ScriptName = current.Name, + current.TriggerType, + current.TriggerConfiguration, + current.IsLocked, + }, + ct).ConfigureAwait(false); + } + else + { + var newScript = new TemplateScript(scriptDto.Name, scriptDto.Code) + { + TriggerType = scriptDto.TriggerType, + TriggerConfiguration = scriptDto.TriggerConfiguration, + ParameterDefinitions = scriptDto.ParameterDefinitions, + ReturnDefinition = scriptDto.ReturnDefinition, + IsLocked = scriptDto.IsLocked, + }; + ex.Scripts.Add(newScript); + await _auditService.LogAsync( + user, + "TemplateScriptAdded", + "TemplateScript", + "0", + $"{ex.Name}.{newScript.Name}", + new + { + TemplateName = ex.Name, + ScriptName = newScript.Name, + newScript.TriggerType, + newScript.TriggerConfiguration, + newScript.IsLocked, + }, + ct).ConfigureAwait(false); + } + } + } + /// /// FU-B / remainder of #37 — Pass A of the post-template-flush rewire. /// For every imported template (Add / Overwrite / Rename) whose bundle DTO @@ -1345,6 +1679,12 @@ public sealed class BundleImporter : IBundleImporter await _externalRepo.UpdateExternalSystemAsync(existing, ct).ConfigureAwait(false); await _auditService.LogAsync(user, "Update", "ExternalSystem", existing.Id.ToString(), existing.Name, new { existing.Name, existing.EndpointUrl }, ct).ConfigureAwait(false); + // T-002: Overwrite must also synchronise the Methods child + // collection — added / removed / modified methods on the + // bundle DTO must round-trip. Mirrors the T-001 template + // child-sync helpers (attributes / alarms / scripts): each + // helper emits one audit row per detected change. + await SyncExternalSystemMethodsAsync(existing, dto, user, ct).ConfigureAwait(false); summary.Overwritten++; break; case ResolutionAction.Add: @@ -1371,6 +1711,114 @@ public sealed class BundleImporter : IBundleImporter return sys; } + /// + /// T-002 — Overwrite child sync (ExternalSystem methods). Mirrors the + /// T-001 SyncTemplate*Async helpers: name-keyed diff between the + /// bundle DTO and the persisted children, then add / update / delete via + /// the repository with one audit row per detected change. Methods are + /// NOT a navigation on (the FK + /// runs from + /// to the parent) so the helper drives the repo directly rather than + /// mutating a tracked collection like the template helpers do. + /// + /// Audit rows: ExternalSystemMethodAdded / + /// ExternalSystemMethodUpdated / ExternalSystemMethodDeleted. + /// Idempotent: scalar-field comparison gates the Update audit row, so an + /// Overwrite against an already-matching method produces no noise. + /// + /// + private async Task SyncExternalSystemMethodsAsync( + ExternalSystemDefinition ex, + ExternalSystemDto dto, + string user, + CancellationToken ct) + { + var existingMethods = await _externalRepo + .GetMethodsByExternalSystemIdAsync(ex.Id, ct) + .ConfigureAwait(false); + var existingByName = existingMethods.ToDictionary(m => m.Name, m => m, StringComparer.Ordinal); + var dtoByName = dto.Methods.ToDictionary(m => m.Name, m => m, StringComparer.Ordinal); + + // Deletes — methods present on the target but not in the bundle. + foreach (var existing in existingByName.Values.ToList()) + { + if (dtoByName.ContainsKey(existing.Name)) continue; + await _externalRepo.DeleteExternalSystemMethodAsync(existing.Id, ct).ConfigureAwait(false); + await _auditService.LogAsync( + user, + "ExternalSystemMethodDeleted", + "ExternalSystemMethod", + existing.Id.ToString(), + $"{ex.Name}.{existing.Name}", + new { ExternalSystemName = ex.Name, MethodName = existing.Name }, + ct).ConfigureAwait(false); + } + + // Adds + Updates. + foreach (var methodDto in dto.Methods) + { + if (existingByName.TryGetValue(methodDto.Name, out var current)) + { + // Update only if any field actually changed — mirrors the + // ArtifactDiff.ExternalSystemMethodsEqual comparator. + bool changed = + !string.Equals(current.HttpMethod, methodDto.HttpMethod, StringComparison.Ordinal) || + !string.Equals(current.Path, methodDto.Path, StringComparison.Ordinal) || + !string.Equals(current.ParameterDefinitions, methodDto.ParameterDefinitions, StringComparison.Ordinal) || + !string.Equals(current.ReturnDefinition, methodDto.ReturnDefinition, StringComparison.Ordinal); + if (!changed) continue; + + current.HttpMethod = methodDto.HttpMethod; + current.Path = methodDto.Path; + current.ParameterDefinitions = methodDto.ParameterDefinitions; + current.ReturnDefinition = methodDto.ReturnDefinition; + await _externalRepo.UpdateExternalSystemMethodAsync(current, ct).ConfigureAwait(false); + await _auditService.LogAsync( + user, + "ExternalSystemMethodUpdated", + "ExternalSystemMethod", + current.Id.ToString(), + $"{ex.Name}.{current.Name}", + new + { + ExternalSystemName = ex.Name, + MethodName = current.Name, + current.HttpMethod, + current.Path, + current.ParameterDefinitions, + current.ReturnDefinition, + }, + ct).ConfigureAwait(false); + } + else + { + var newMethod = new ExternalSystemMethod(methodDto.Name, methodDto.HttpMethod, methodDto.Path) + { + ExternalSystemDefinitionId = ex.Id, + ParameterDefinitions = methodDto.ParameterDefinitions, + ReturnDefinition = methodDto.ReturnDefinition, + }; + await _externalRepo.AddExternalSystemMethodAsync(newMethod, ct).ConfigureAwait(false); + await _auditService.LogAsync( + user, + "ExternalSystemMethodAdded", + "ExternalSystemMethod", + "0", + $"{ex.Name}.{newMethod.Name}", + new + { + ExternalSystemName = ex.Name, + MethodName = newMethod.Name, + newMethod.HttpMethod, + newMethod.Path, + newMethod.ParameterDefinitions, + newMethod.ReturnDefinition, + }, + ct).ConfigureAwait(false); + } + } + } + private async Task ApplyDatabaseConnectionsAsync( IReadOnlyList dtos, Dictionary<(string, string), ImportResolution> map, diff --git a/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs index 2aab5a40..c9e8b2d0 100644 --- a/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs +++ b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs @@ -2,11 +2,13 @@ using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using ScadaLink.Commons.Entities.ExternalSystems; using ScadaLink.Commons.Entities.Scripts; using ScadaLink.Commons.Entities.Templates; using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Interfaces.Services; using ScadaLink.Commons.Interfaces.Transport; +using ScadaLink.Commons.Types.Enums; using ScadaLink.Commons.Types.Transport; using ScadaLink.ConfigurationDatabase; using ScadaLink.ConfigurationDatabase.Repositories; @@ -86,10 +88,11 @@ public sealed class BundleImporterApplyTests : IDisposable var ctx = scope.ServiceProvider.GetRequiredService(); var templateIds = await ctx.Templates.Select(t => t.Id).ToListAsync(); var sharedScriptIds = await ctx.SharedScripts.Select(s => s.Id).ToListAsync(); + var externalSystemIds = await ctx.ExternalSystemDefinitions.Select(e => e.Id).ToListAsync(); var selection = new ExportSelection( TemplateIds: templateIds, SharedScriptIds: sharedScriptIds, - ExternalSystemIds: Array.Empty(), + ExternalSystemIds: externalSystemIds, DatabaseConnectionIds: Array.Empty(), NotificationListIds: Array.Empty(), SmtpConfigurationIds: Array.Empty(), @@ -476,4 +479,272 @@ public sealed class BundleImporterApplyTests : IDisposable Assert.Null(row.BundleImportId); } } + + [Fact] + public async Task ApplyAsync_Overwrite_synchronises_attributes_alarms_and_scripts_to_bundle() + { + // T-001 regression. The Overwrite branch used to write only Description + // / FolderId on the existing template; the bundle's Attributes / Alarms + // / Scripts collections were silently dropped on the floor. This test + // seeds a template with one shape, exports it, mutates the target to a + // divergent shape, then asserts that Overwrite restores every child + // collection AND emits per-field audit rows. + // + // Bundle shape (exported from "Pump"): + // Attributes: [SetPoint (Float, 50.0), Pressure (Float, 100.0)] + // Alarms: [HiAlarm (PriorityLevel=1)] + // Scripts: [Init (Code="return 1;")] + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var t = new Template("Pump") { Description = "from-bundle" }; + t.Attributes.Add(new TemplateAttribute("SetPoint") { DataType = DataType.Float, Value = "50.0" }); + t.Attributes.Add(new TemplateAttribute("Pressure") { DataType = DataType.Float, Value = "100.0" }); + t.Alarms.Add(new TemplateAlarm("HiAlarm") { PriorityLevel = 1, TriggerType = AlarmTriggerType.ValueMatch }); + t.Scripts.Add(new TemplateScript("Init", "return 1;")); + ctx.Templates.Add(t); + await ctx.SaveChangesAsync(); + } + + var sessionId = await ExportAndLoadAsync(); + + // Mutate the target so every child collection diverges from the bundle. + // Attributes: SetPoint value mutated, Pressure DELETED, NewAttr ADDED + // Alarms: HiAlarm PriorityLevel mutated, ExtraAlarm ADDED + // Scripts: Init code mutated, ExtraScript ADDED + // Description also mutated so the scalar field still flips. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var pump = await ctx.Templates + .Include(t => t.Attributes) + .Include(t => t.Alarms) + .Include(t => t.Scripts) + .SingleAsync(t => t.Name == "Pump"); + pump.Description = "target-mutated"; + var setPoint = pump.Attributes.Single(a => a.Name == "SetPoint"); + setPoint.Value = "999.0"; // mutated + var pressure = pump.Attributes.Single(a => a.Name == "Pressure"); + pump.Attributes.Remove(pressure); + ctx.TemplateAttributes.Remove(pressure); + pump.Attributes.Add(new TemplateAttribute("NewAttr") + { + DataType = DataType.String, + Value = "should-be-deleted-by-overwrite", + }); + var hiAlarm = pump.Alarms.Single(a => a.Name == "HiAlarm"); + hiAlarm.PriorityLevel = 99; // mutated + pump.Alarms.Add(new TemplateAlarm("ExtraAlarm") + { + PriorityLevel = 5, + TriggerType = AlarmTriggerType.RangeViolation, + }); + var initScript = pump.Scripts.Single(s => s.Name == "Init"); + initScript.Code = "return 999;"; // mutated + pump.Scripts.Add(new TemplateScript("ExtraScript", "return 0;")); + await ctx.SaveChangesAsync(); + } + + // Capture the audit baseline so we can scope assertions to rows + // emitted by THIS apply. + int beforeMaxAuditId; + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + beforeMaxAuditId = await ctx.AuditLogEntries.MaxAsync(a => (int?)a.Id) ?? 0; + } + + // Act — apply Overwrite. + ImportResult result; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + result = await importer.ApplyAsync(sessionId, + new List { new("Template", "Pump", ResolutionAction.Overwrite, null) }, + user: "bob"); + } + + // Assert — children mirror the bundle. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var pump = await ctx.Templates + .Include(t => t.Attributes) + .Include(t => t.Alarms) + .Include(t => t.Scripts) + .SingleAsync(t => t.Name == "Pump"); + + Assert.Equal("from-bundle", pump.Description); + + // Attributes — exactly { SetPoint, Pressure }, values restored. + Assert.Equal(2, pump.Attributes.Count); + var setPoint = pump.Attributes.Single(a => a.Name == "SetPoint"); + Assert.Equal("50.0", setPoint.Value); + Assert.Equal(DataType.Float, setPoint.DataType); + var pressure = pump.Attributes.Single(a => a.Name == "Pressure"); + Assert.Equal("100.0", pressure.Value); + Assert.DoesNotContain(pump.Attributes, a => a.Name == "NewAttr"); + + // Alarms — exactly { HiAlarm }, PriorityLevel restored. + Assert.Single(pump.Alarms); + var hi = pump.Alarms.Single(); + Assert.Equal("HiAlarm", hi.Name); + Assert.Equal(1, hi.PriorityLevel); + Assert.DoesNotContain(pump.Alarms, a => a.Name == "ExtraAlarm"); + + // Scripts — exactly { Init }, code restored. + Assert.Single(pump.Scripts); + var init = pump.Scripts.Single(); + Assert.Equal("Init", init.Name); + Assert.Equal("return 1;", init.Code); + Assert.DoesNotContain(pump.Scripts, s => s.Name == "ExtraScript"); + + // Per-field audit rows — design doc enumerates Added / Updated / + // Deleted shapes; all of these should appear, all stamped with + // the BundleImportId from the result. + var newRows = await ctx.AuditLogEntries + .Where(a => a.Id > beforeMaxAuditId) + .ToListAsync(); + Assert.All(newRows, r => Assert.Equal(result.BundleImportId, r.BundleImportId)); + + // Attribute audit events. + Assert.Contains(newRows, r => r.Action == "TemplateAttributeUpdated" && r.EntityName == "Pump.SetPoint"); + Assert.Contains(newRows, r => r.Action == "TemplateAttributeAdded" && r.EntityName == "Pump.Pressure"); + Assert.Contains(newRows, r => r.Action == "TemplateAttributeDeleted" && r.EntityName == "Pump.NewAttr"); + + // Alarm audit events. + Assert.Contains(newRows, r => r.Action == "TemplateAlarmUpdated" && r.EntityName == "Pump.HiAlarm"); + Assert.Contains(newRows, r => r.Action == "TemplateAlarmDeleted" && r.EntityName == "Pump.ExtraAlarm"); + + // Script audit events. + Assert.Contains(newRows, r => r.Action == "TemplateScriptUpdated" && r.EntityName == "Pump.Init"); + Assert.Contains(newRows, r => r.Action == "TemplateScriptDeleted" && r.EntityName == "Pump.ExtraScript"); + } + Assert.Equal(1, result.Overwritten); + } + + [Fact] + public async Task ApplyAsync_Overwrite_synchronises_external_system_methods_to_bundle() + { + // T-002 regression. The ExternalSystem Overwrite branch used to write + // only EndpointUrl / AuthType / AuthConfiguration on the existing + // definition; the bundle's Methods collection was silently dropped on + // the floor. This test seeds an external system with one method + // shape, exports it, mutates the target's methods to diverge, then + // asserts that Overwrite restores every method AND emits per-field + // audit rows. + // + // Bundle shape (exported from "Erp"): + // Methods: [ + // GetUser (GET /users/{id}, param=A, return=R), + // PostJob (POST /jobs, param=B, return=S), + // ] + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = new ExternalSystemDefinition("Erp", "https://erp.example", "ApiKey"); + ctx.ExternalSystemDefinitions.Add(sys); + await ctx.SaveChangesAsync(); + ctx.ExternalSystemMethods.Add(new ExternalSystemMethod("GetUser", "GET", "/users/{id}") + { + ExternalSystemDefinitionId = sys.Id, + ParameterDefinitions = "A", + ReturnDefinition = "R", + }); + ctx.ExternalSystemMethods.Add(new ExternalSystemMethod("PostJob", "POST", "/jobs") + { + ExternalSystemDefinitionId = sys.Id, + ParameterDefinitions = "B", + ReturnDefinition = "S", + }); + await ctx.SaveChangesAsync(); + } + + var sessionId = await ExportAndLoadAsync(); + + // Mutate the target so the methods diverge from the bundle: + // GetUser — Path mutated (UPDATE expected on Overwrite) + // PostJob — DELETED (ADD expected on Overwrite to restore) + // ExtraOp — ADDED (DELETE expected on Overwrite to remove) + // EndpointUrl / AuthType also mutated so the scalar update still fires. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp"); + sys.EndpointUrl = "https://wrong.example"; + sys.AuthType = "Basic"; + var getUser = await ctx.ExternalSystemMethods + .SingleAsync(m => m.ExternalSystemDefinitionId == sys.Id && m.Name == "GetUser"); + getUser.Path = "/wrong/path"; + var postJob = await ctx.ExternalSystemMethods + .SingleAsync(m => m.ExternalSystemDefinitionId == sys.Id && m.Name == "PostJob"); + ctx.ExternalSystemMethods.Remove(postJob); + ctx.ExternalSystemMethods.Add(new ExternalSystemMethod("ExtraOp", "DELETE", "/extra") + { + ExternalSystemDefinitionId = sys.Id, + ParameterDefinitions = "X", + ReturnDefinition = "Y", + }); + await ctx.SaveChangesAsync(); + } + + // Capture the audit baseline so we can scope assertions to rows + // emitted by THIS apply. + int beforeMaxAuditId; + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + beforeMaxAuditId = await ctx.AuditLogEntries.MaxAsync(a => (int?)a.Id) ?? 0; + } + + // Act — apply Overwrite on the external system. + ImportResult result; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + result = await importer.ApplyAsync(sessionId, + new List { new("ExternalSystem", "Erp", ResolutionAction.Overwrite, null) }, + user: "bob"); + } + + // Assert — methods mirror the bundle exactly. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp"); + Assert.Equal("https://erp.example", sys.EndpointUrl); + + var methods = await ctx.ExternalSystemMethods + .Where(m => m.ExternalSystemDefinitionId == sys.Id) + .ToListAsync(); + Assert.Equal(2, methods.Count); + + var getUser = methods.Single(m => m.Name == "GetUser"); + Assert.Equal("GET", getUser.HttpMethod); + Assert.Equal("/users/{id}", getUser.Path); + Assert.Equal("A", getUser.ParameterDefinitions); + Assert.Equal("R", getUser.ReturnDefinition); + + var postJob = methods.Single(m => m.Name == "PostJob"); + Assert.Equal("POST", postJob.HttpMethod); + Assert.Equal("/jobs", postJob.Path); + Assert.Equal("B", postJob.ParameterDefinitions); + Assert.Equal("S", postJob.ReturnDefinition); + + Assert.DoesNotContain(methods, m => m.Name == "ExtraOp"); + + // Per-field audit rows — design doc enumerates Added / Updated / + // Deleted shapes; all of these should appear, all stamped with + // the BundleImportId from the result. + var newRows = await ctx.AuditLogEntries + .Where(a => a.Id > beforeMaxAuditId) + .ToListAsync(); + Assert.All(newRows, r => Assert.Equal(result.BundleImportId, r.BundleImportId)); + + Assert.Contains(newRows, r => r.Action == "ExternalSystemMethodUpdated" && r.EntityName == "Erp.GetUser"); + Assert.Contains(newRows, r => r.Action == "ExternalSystemMethodAdded" && r.EntityName == "Erp.PostJob"); + Assert.Contains(newRows, r => r.Action == "ExternalSystemMethodDeleted" && r.EntityName == "Erp.ExtraOp"); + } + Assert.Equal(1, result.Overwritten); + } }