From 9043f0089b07bc2d8aecfe0170643a27832dd69b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 19:33:09 -0400 Subject: [PATCH] =?UTF-8?q?fix(configuration-database):=20resolve=20Config?= =?UTF-8?q?urationDatabase-001=20=E2=80=94=20remove=20dead=20child-templat?= =?UTF-8?q?e=20query=20in=20GetTemplateWithChildrenAsync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ConfigurationDatabase/findings.md | 27 +++++- .../Repositories/TemplateEngineRepository.cs | 16 ++-- .../TemplateEngineRepositoryTests.cs | 90 +++++++++++++++++++ 3 files changed, 121 insertions(+), 12 deletions(-) create mode 100644 tests/ScadaLink.ConfigurationDatabase.Tests/TemplateEngineRepositoryTests.cs diff --git a/code-reviews/ConfigurationDatabase/findings.md b/code-reviews/ConfigurationDatabase/findings.md index ec3ff38..515ddd7 100644 --- a/code-reviews/ConfigurationDatabase/findings.md +++ b/code-reviews/ConfigurationDatabase/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 11 | +| Open findings | 10 | ## Summary @@ -60,7 +60,7 @@ repositories (`TemplateEngineRepository`, `DeploymentManagerRepository`, |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs:30-41` | **Description** @@ -84,7 +84,28 @@ explicit result tuple/DTO so the loaded data reaches the caller. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``). Root cause confirmed against source: the +method ran a `Where(t => t.ParentTemplateId == id)` query, assigned the result to a +local `children` variable, and never used it — a misleading no-op that also issued an +extra database round-trip per call. + +Triage of the three callers (`FlatteningPipeline.BuildTemplateChainAsync`, +`ManagementActor.HandleGetTemplate`, `ManagementActor.HandleValidateTemplate`) showed +none consume derived/sub-templates; they all need the template's *member* collections +(Attributes/Alarms/Scripts/Compositions), which `GetTemplateByIdAsync` already +eager-loads. The `Template` entity has no child-templates navigation collection, and +adding one (plus changing the interface signature) would require editing +`ScadaLink.Commons`, which is outside this module's scope. + +Fix applied the recommendation's secondary option: removed the dead query so the +method no longer misleads or wastes a round-trip, and added an XML doc comment +clarifying that "children" means the template's member collections. The method now +honestly delegates to `GetTemplateByIdAsync`. Regression tests added in +`TemplateEngineRepositoryTests.cs`: +`GetTemplateWithChildrenAsync_ReturnsTemplateWithAllMemberCollectionsPopulated`, +`GetTemplateWithChildrenAsync_PreservesParentTemplateId_ForInheritanceChainWalk`, and +`GetTemplateWithChildrenAsync_ReturnsNull_WhenTemplateDoesNotExist` — pinning the +template-aggregate contract the callers depend on. ### ConfigurationDatabase-002 — Hardcoded `sa` connection string with embedded password literal diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs index f494153..ea36949 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs @@ -27,17 +27,15 @@ public class TemplateEngineRepository : ITemplateEngineRepository .FirstOrDefaultAsync(t => t.Id == id, cancellationToken); } + /// + /// Loads a template together with its child members — Attributes, Alarms, + /// Scripts and Compositions — eager-loaded so callers get the full template + /// aggregate in a single round-trip. "Children" here refers to the template's + /// member collections, not derived/sub templates. + /// public async Task GetTemplateWithChildrenAsync(int id, CancellationToken cancellationToken = default) { - var template = await GetTemplateByIdAsync(id, cancellationToken); - if (template == null) return null; - - // Load all templates that have this template as parent - var children = await _context.Templates - .Where(t => t.ParentTemplateId == id) - .ToListAsync(cancellationToken); - - return template; + return await GetTemplateByIdAsync(id, cancellationToken); } public async Task> GetAllTemplatesAsync(CancellationToken cancellationToken = default) diff --git a/tests/ScadaLink.ConfigurationDatabase.Tests/TemplateEngineRepositoryTests.cs b/tests/ScadaLink.ConfigurationDatabase.Tests/TemplateEngineRepositoryTests.cs new file mode 100644 index 0000000..00b1fb3 --- /dev/null +++ b/tests/ScadaLink.ConfigurationDatabase.Tests/TemplateEngineRepositoryTests.cs @@ -0,0 +1,90 @@ +using Microsoft.EntityFrameworkCore; +using ScadaLink.Commons.Entities.Templates; +using ScadaLink.ConfigurationDatabase; +using ScadaLink.ConfigurationDatabase.Repositories; + +namespace ScadaLink.ConfigurationDatabase.Tests; + +public class TemplateEngineRepositoryTests : IDisposable +{ + private readonly ScadaLinkDbContext _context; + private readonly TemplateEngineRepository _repository; + + public TemplateEngineRepositoryTests() + { + var options = new DbContextOptionsBuilder() + .UseSqlite("DataSource=:memory:") + .Options; + + _context = new ScadaLinkDbContext(options); + _context.Database.OpenConnection(); + _context.Database.EnsureCreated(); + _repository = new TemplateEngineRepository(_context); + } + + public void Dispose() + { + _context.Database.CloseConnection(); + _context.Dispose(); + } + + [Fact] + public async Task GetTemplateWithChildrenAsync_ReturnsTemplateWithAllMemberCollectionsPopulated() + { + // Arrange: a template with one attribute, one alarm, one script and one composition. + var composed = new Template("ComposedTemplate"); + _context.Templates.Add(composed); + await _context.SaveChangesAsync(); + + var template = new Template("ParentTemplate"); + template.Attributes.Add(new TemplateAttribute("Attr1")); + template.Alarms.Add(new TemplateAlarm("Alarm1")); + template.Scripts.Add(new TemplateScript("Script1", "return 1;")); + template.Compositions.Add(new TemplateComposition("Slot1") { ComposedTemplateId = composed.Id }); + _context.Templates.Add(template); + await _context.SaveChangesAsync(); + + // Act + var loaded = await _repository.GetTemplateWithChildrenAsync(template.Id); + + // Assert: the method must deliver the template's child members to the caller, + // not silently drop them. Regression guard for ConfigurationDatabase-001. + Assert.NotNull(loaded); + Assert.Equal(template.Id, loaded!.Id); + Assert.Single(loaded.Attributes); + Assert.Equal("Attr1", loaded.Attributes.First().Name); + Assert.Single(loaded.Alarms); + Assert.Equal("Alarm1", loaded.Alarms.First().Name); + Assert.Single(loaded.Scripts); + Assert.Equal("Script1", loaded.Scripts.First().Name); + Assert.Single(loaded.Compositions); + Assert.Equal("Slot1", loaded.Compositions.First().InstanceName); + } + + [Fact] + public async Task GetTemplateWithChildrenAsync_ReturnsNull_WhenTemplateDoesNotExist() + { + var loaded = await _repository.GetTemplateWithChildrenAsync(9999); + + Assert.Null(loaded); + } + + [Fact] + public async Task GetTemplateWithChildrenAsync_PreservesParentTemplateId_ForInheritanceChainWalk() + { + // FlatteningPipeline.BuildTemplateChainAsync walks ParentTemplateId upward. + // The result of GetTemplateWithChildrenAsync must carry that link intact. + var baseTemplate = new Template("BaseTemplate"); + _context.Templates.Add(baseTemplate); + await _context.SaveChangesAsync(); + + var derived = new Template("DerivedTemplate") { ParentTemplateId = baseTemplate.Id }; + _context.Templates.Add(derived); + await _context.SaveChangesAsync(); + + var loaded = await _repository.GetTemplateWithChildrenAsync(derived.Id); + + Assert.NotNull(loaded); + Assert.Equal(baseTemplate.Id, loaded!.ParentTemplateId); + } +}