From d6221419c66250a6266dd1e9a47b100fd5d24dd3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 17 May 2026 03:18:41 -0400 Subject: [PATCH] =?UTF-8?q?fix(template-engine):=20resolve=20TemplateEngin?= =?UTF-8?q?e-015,016=20=E2=80=94=20cascade-rename=20nested=20derived=20tem?= =?UTF-8?q?plates,=20correct=20composed-script=20ParentPath?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/TemplateEngine/findings.md | 24 ++++++-- .../Flattening/FlatteningService.cs | 14 +++-- .../TemplateService.cs | 50 +++++++++++++-- .../Flattening/FlatteningServiceTests.cs | 42 +++++++++++++ .../TemplateServiceTests.cs | 61 +++++++++++++++++++ 5 files changed, 177 insertions(+), 14 deletions(-) diff --git a/code-reviews/TemplateEngine/findings.md b/code-reviews/TemplateEngine/findings.md index 9413f07..ae18a6f 100644 --- a/code-reviews/TemplateEngine/findings.md +++ b/code-reviews/TemplateEngine/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-17 | | Reviewer | claude-agent | | Commit reviewed | `39d737e` | -| Open findings | 2 | +| Open findings | 0 | ## Summary @@ -674,7 +674,7 @@ verifies all three constraint categories are surfaced together. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/TemplateService.cs:680` | **Description** @@ -719,7 +719,14 @@ two-level cascade rename. **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit `pending`): `RenameCompositionAsync` now recurses +into `derived.Compositions` via a new `CollectCascadeRenamesAsync` helper +(mirroring `CascadeDeleteDerivedAsync`), re-deriving each cascaded inner derived +template's name from its renamed parent and slot instance name, and runs the +same-name collision pre-check across every name in the cascade before any row +mutates. Regression tests: +`RenameComposition_CascadesRenameToNestedDerivedTemplates`, +`RenameComposition_NestedCascadeNameCollision_Fails`. ### TemplateEngine-016 — Composed-script `ScriptScope.ParentPath` is always empty, breaking `Parent.X` resolution for nested modules @@ -727,7 +734,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:750` | **Description** @@ -765,4 +772,11 @@ two-level composed script. **Resolution** -_Unresolved._ +Resolved 2026-05-17 (commit `pending`): threaded a `parentPath` parameter +through `ResolveComposedScriptsRecursive` — the top-level caller passes `""` +(a depth-1 composition's parent is the root template) and each nested call +passes the enclosing module's `prefix` — and the `ScriptScope` now sets +`ParentPath` to that value instead of a hard-coded `""`, so a depth-2 script's +`SelfPath = "Outer.Inner"` pairs with `ParentPath = "Outer"` and `Parent.X` +resolves against the real parent module. Regression test: +`Flatten_NestedComposedScript_ScopeCarriesCorrectParentPath`. diff --git a/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs b/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs index 9f331cf..7164106 100644 --- a/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs +++ b/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs @@ -714,7 +714,7 @@ public class FlatteningService foreach (var composition in compositions) ResolveComposedScriptsRecursive( - composition, composition.InstanceName, + composition, composition.InstanceName, parentPath: "", compositionMap, composedTemplateChains, scripts, scriptCanonicalById, new HashSet()); } @@ -723,11 +723,17 @@ public class FlatteningService /// /// Recursively resolves the scripts of a composed module and every module /// nested inside it, path-qualifying each canonical name with the - /// accumulated . + /// accumulated . is + /// the path of the enclosing module — empty for a depth-1 composition + /// (parent is the root template) and the enclosing module's + /// prefix for deeper nesting — and is carried into each script's + /// so a nested script's Parent.X + /// resolves against its real parent module. /// private static void ResolveComposedScriptsRecursive( TemplateComposition composition, string prefix, + string parentPath, IReadOnlyDictionary> compositionMap, IReadOnlyDictionary> composedTemplateChains, Dictionary scripts, @@ -747,7 +753,7 @@ public class FlatteningService { CanonicalName = canonicalName, Source = "Composed", - Scope = new Commons.Types.Scripts.ScriptScope(SelfPath: prefix, ParentPath: "") + Scope = new Commons.Types.Scripts.ScriptScope(SelfPath: prefix, ParentPath: parentPath) }; } } @@ -762,7 +768,7 @@ public class FlatteningService foreach (var nested in nestedCompositions) ResolveComposedScriptsRecursive( - nested, $"{prefix}.{nested.InstanceName}", + nested, $"{prefix}.{nested.InstanceName}", parentPath: prefix, compositionMap, composedTemplateChains, scripts, scriptCanonicalById, visited); } } diff --git a/src/ScadaLink.TemplateEngine/TemplateService.cs b/src/ScadaLink.TemplateEngine/TemplateService.cs index 11d621e..fcb74c1 100644 --- a/src/ScadaLink.TemplateEngine/TemplateService.cs +++ b/src/ScadaLink.TemplateEngine/TemplateService.cs @@ -705,12 +705,28 @@ public class TemplateService { var newDerivedName = $"{owner.Name}.{newInstanceName}"; var allTemplates = await _repository.GetAllTemplatesAsync(cancellationToken); - if (allTemplates.Any(t => t.Id != derived.Id && t.Name == newDerivedName)) - return Result.Failure( - $"Cannot rename derived template to '{newDerivedName}': a template with that name already exists."); - derived.Name = newDerivedName; - await _repository.UpdateTemplateAsync(derived, cancellationToken); + // The cascade of derived templates created by AddComposition follows a + // dotted path (Pump.TempSensor and the nested Pump.TempSensor.Probe1). + // Renaming the slot must rename every derived template in that cascade + // so the dotted-path naming invariant holds — pre-check every new name + // the cascade will introduce before any row mutates. + var renames = new List<(Template Template, string NewName)>(); + await CollectCascadeRenamesAsync(derived, newDerivedName, renames, cancellationToken); + + var renamedIds = renames.Select(r => r.Template.Id).ToHashSet(); + foreach (var (_, newName) in renames) + { + if (allTemplates.Any(t => !renamedIds.Contains(t.Id) && t.Name == newName)) + return Result.Failure( + $"Cannot rename derived template to '{newName}': a template with that name already exists."); + } + + foreach (var (template, newName) in renames) + { + template.Name = newName; + await _repository.UpdateTemplateAsync(template, cancellationToken); + } } composition.InstanceName = newInstanceName; @@ -747,6 +763,30 @@ public class TemplateService return Result.Success(true); } + /// + /// Recursively collects the (template, new name) pairs for a renamed derived + /// template and every cascaded inner derived template beneath it. Each inner + /// derived's new name is re-derived from its renamed parent and the slot's + /// instance name (mirroring the cascade + /// builds and the recursion in ). + /// + private async Task CollectCascadeRenamesAsync( + Template derived, + string newName, + List<(Template Template, string NewName)> renames, + CancellationToken cancellationToken) + { + renames.Add((derived, newName)); + + foreach (var child in derived.Compositions.ToList()) + { + var childDerived = await _repository.GetTemplateByIdAsync(child.ComposedTemplateId, cancellationToken); + if (childDerived != null && childDerived.IsDerived && childDerived.OwnerCompositionId == child.Id) + await CollectCascadeRenamesAsync( + childDerived, $"{newName}.{child.InstanceName}", renames, cancellationToken); + } + } + /// /// Recursively deletes a derived template along with the cascade of inner /// derived templates the compose flow created. Each composition row on the diff --git a/tests/ScadaLink.TemplateEngine.Tests/Flattening/FlatteningServiceTests.cs b/tests/ScadaLink.TemplateEngine.Tests/Flattening/FlatteningServiceTests.cs index b9b4d88..2c2c72d 100644 --- a/tests/ScadaLink.TemplateEngine.Tests/Flattening/FlatteningServiceTests.cs +++ b/tests/ScadaLink.TemplateEngine.Tests/Flattening/FlatteningServiceTests.cs @@ -624,4 +624,46 @@ public class FlatteningServiceTests var alarm = result.Value.Alarms.First(a => a.CanonicalName == "MainPump.PumpFault"); Assert.Equal("MainPump.PumpAlarmHandler", alarm.OnTriggerScriptCanonicalName); } + + // ── TemplateEngine-016: composed-script ScriptScope.ParentPath ───────── + + [Fact] + public void Flatten_NestedComposedScript_ScopeCarriesCorrectParentPath() + { + // Station composes Pump (level 1); Pump composes Motor (level 2). + // The depth-1 script's parent is the root template (ParentPath ""); + // the depth-2 script's parent is the Pump module (ParentPath "MainPump"). + var motor = CreateTemplate(3, "Motor"); + motor.Scripts.Add(new TemplateScript("MonitorMotor", "// m") { Id = 70 }); + + var pump = CreateTemplate(2, "Pump"); + pump.Scripts.Add(new TemplateScript("MonitorPump", "// p") { Id = 71 }); + + var station = CreateTemplate(1, "Station"); + + var compositions = new Dictionary> + { + [1] = new List { new("MainPump") { ComposedTemplateId = 2 } }, + [2] = new List { new("DriveMotor") { ComposedTemplateId = 3 } }, + }; + var composedChains = new Dictionary> + { + [2] = [pump], [3] = [motor], + }; + + var instance = CreateInstance(); + var result = _sut.Flatten(instance, [station], compositions, composedChains, + new Dictionary()); + + Assert.True(result.IsSuccess); + + var depth1 = result.Value.Scripts.First(s => s.CanonicalName == "MainPump.MonitorPump"); + Assert.Equal("MainPump", depth1.Scope.SelfPath); + Assert.Equal("", depth1.Scope.ParentPath); + + var depth2 = result.Value.Scripts.First(s => s.CanonicalName == "MainPump.DriveMotor.MonitorMotor"); + Assert.Equal("MainPump.DriveMotor", depth2.Scope.SelfPath); + // Parent module of a depth-2 script is the enclosing Pump module. + Assert.Equal("MainPump", depth2.Scope.ParentPath); + } } diff --git a/tests/ScadaLink.TemplateEngine.Tests/TemplateServiceTests.cs b/tests/ScadaLink.TemplateEngine.Tests/TemplateServiceTests.cs index c556050..bd1f10c 100644 --- a/tests/ScadaLink.TemplateEngine.Tests/TemplateServiceTests.cs +++ b/tests/ScadaLink.TemplateEngine.Tests/TemplateServiceTests.cs @@ -466,6 +466,67 @@ public class TemplateServiceTests Assert.Equal("Pump.NewSlot", derived.Name); } + [Fact] + public async Task RenameComposition_CascadesRenameToNestedDerivedTemplates() + { + // Pump.TempSensor is the slot-owned derived; Pump.TempSensor.Probe1 is a + // cascaded inner derived under it. Renaming the TempSensor slot to + // MainSensor must rename BOTH derived templates so the dotted-path + // naming invariant holds: Pump.MainSensor and Pump.MainSensor.Probe1. + var innerComp = new TemplateComposition("Probe1") { Id = 51, TemplateId = 77, ComposedTemplateId = 78 }; + var innerDerived = new Template("Pump.TempSensor.Probe1") { Id = 78, IsDerived = true, OwnerCompositionId = 51, ParentTemplateId = 11 }; + var composition = new TemplateComposition("TempSensor") { Id = 50, TemplateId = 1, ComposedTemplateId = 77 }; + var owner = new Template("Pump") { Id = 1 }; + owner.Compositions.Add(composition); + var derived = new Template("Pump.TempSensor") { Id = 77, IsDerived = true, OwnerCompositionId = 50, ParentTemplateId = 2 }; + derived.Compositions.Add(innerComp); + + _repoMock.Setup(r => r.GetTemplateCompositionByIdAsync(50, It.IsAny())).ReturnsAsync(composition); + _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny())).ReturnsAsync(owner); + _repoMock.Setup(r => r.GetTemplateByIdAsync(77, It.IsAny())).ReturnsAsync(derived); + _repoMock.Setup(r => r.GetTemplateByIdAsync(78, It.IsAny())).ReturnsAsync(innerDerived); + _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny())) + .ReturnsAsync(new List