fix(transport): case-insensitive remaining library lookups; base-only template reads for import targets (ScadaBridge#95)

- Preview matches template folders, shared scripts, external systems, SMTP (by Host) and SMS (by AccountSid) with the #93 ToNameLookup comparer (OrdinalIgnoreCase, first wins, never throws), matching SQL Server's case-insensitive collation, so a differently-cased target row is reported as a conflict instead of New. - Apply-side in-memory lookups do the same: ApplyTemplateFoldersAsync, the template folder-id map, ResolveFolderParentEdgesAsync, ApplySmtpConfigsAsync and ApplySmsConfigsAsync. An Overwrite now updates the existing row instead of adding a duplicate (folder names are only unique per parent, so the old ToDictionary could also throw). - New ITemplateEngineRepository.GetBaseTemplatesWithChildrenAsync filters IsDerived in SQL. Preview and the Overwrite load in ApplyTemplatesAsync use it, so a derived slot template that shares a bundled name is never the import target. GetTemplatesWithChildrenAsync is unchanged for the id-keyed callers (export closure, rewire passes), which can need derived rows. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 14:29:37 -05:00
parent 3a31dd2de7
commit 7848a3fe7c
6 changed files with 222 additions and 19 deletions
@@ -31,6 +31,16 @@ public interface ITemplateEngineRepository
    /// <param name="cancellationToken">Cancellation token.</param>
    /// <returns>A task that resolves to a read-only list of matched templates with children loaded.</returns>
    Task<IReadOnlyList<Template>> GetTemplatesWithChildrenAsync(IEnumerable<string> names, CancellationToken cancellationToken = default);
+    /// <summary>
+    /// <see cref="GetTemplatesWithChildrenAsync(IEnumerable{string}, CancellationToken)"/> restricted to
+    /// BASE templates (<see cref="Template.IsDerived"/> is false). Derived templates carry their
+    /// composition slot name, so a by-name read meant for the user-authored template the name
+    /// identifies (a bundle import's target) would otherwise also match derived rows (ScadaBridge#95).
+    /// </summary>
+    /// <param name="names">Template names to load. Duplicate / null / empty names are filtered out.</param>
+    /// <param name="cancellationToken">Cancellation token.</param>
+    /// <returns>A task that resolves to the matched base templates with children loaded.</returns>
+    Task<IReadOnlyList<Template>> GetBaseTemplatesWithChildrenAsync(IEnumerable<string> names, CancellationToken cancellationToken = default);
    /// <summary>Retrieves all templates.</summary>
    /// <param name="cancellationToken">Cancellation token.</param>
    /// <returns>A task that resolves to a read-only list of all templates.</returns>
@@ -53,8 +53,17 @@ public class TemplateEngineRepository : ITemplateEngineRepository
    }

    /// <inheritdoc />
-    public async Task<IReadOnlyList<Template>> GetTemplatesWithChildrenAsync(
+    public Task<IReadOnlyList<Template>> GetTemplatesWithChildrenAsync(
        IEnumerable<string> names, CancellationToken cancellationToken = default)
+        => GetTemplatesWithChildrenAsync(names, baseOnly: false, cancellationToken);
+
+    /// <inheritdoc />
+    public Task<IReadOnlyList<Template>> GetBaseTemplatesWithChildrenAsync(
+        IEnumerable<string> names, CancellationToken cancellationToken = default)
+        => GetTemplatesWithChildrenAsync(names, baseOnly: true, cancellationToken);
+
+    private async Task<IReadOnlyList<Template>> GetTemplatesWithChildrenAsync(
+        IEnumerable<string> names, bool baseOnly, CancellationToken cancellationToken)
    {
        // Bulk lookup replaces the per-name N+1 in
        // BundleImporter.PreviewAsync. Filter out null / empty / duplicate
@@ -67,7 +76,7 @@ public class TemplateEngineRepository : ITemplateEngineRepository
        if (distinct.Length == 0) return Array.Empty<Template>();

        return await _context.Templates
-            .Where(t => distinct.Contains(t.Name))
+            .Where(t => distinct.Contains(t.Name) && (!baseOnly || !t.IsDerived))
            .Include(t => t.Attributes)
            .Include(t => t.Alarms)
            .Include(t => t.Scripts)
@@ -465,7 +465,11 @@ public sealed class BundleImporter : IBundleImporter

        // ---- TemplateFolders ----
        var allFolders = await _templateRepo.GetAllFoldersAsync(ct).ConfigureAwait(false);
-        var folderByName = allFolders.ToDictionary(f => f.Name, f => f, StringComparer.Ordinal);
+        // Every library lookup below matches the way SQL Server's case-insensitive
+        // collation (and the unique indexes it enforces) does, so a target row that differs
+        // only in case is reported as the conflict it will be on apply, not as New
+        // (ScadaBridge#93, #95).
+        var folderByName = ToNameLookup(allFolders, f => f.Name);
        var folderNameById = allFolders.ToDictionary(f => f.Id, f => f.Name);
        foreach (var fDto in content.TemplateFolders)
        {
@@ -479,8 +483,10 @@ public sealed class BundleImporter : IBundleImporter
        // name (classic N+1). The bulk variant fetches every matching template
        // with children eager-loaded in a single round-trip.
        var bundleTemplateNames = content.Templates.Select(t => t.Name);
+        // Base templates only: a derived template carries its composition slot name and is
+        // never what a bundled template of that name targets (ScadaBridge#95).
        var hydratedTemplates = await _templateRepo
-            .GetTemplatesWithChildrenAsync(bundleTemplateNames, ct)
+            .GetBaseTemplatesWithChildrenAsync(bundleTemplateNames, ct)
            .ConfigureAwait(false);
        var hydratedByName = ToTemplateLookup(hydratedTemplates);
        var templateNameById = await library.GetNameByIdAsync(ct).ConfigureAwait(false);
@@ -494,9 +500,7 @@ public sealed class BundleImporter : IBundleImporter
        // One read of each library (04/P4); DetectBlockersAsync reuses the same lists
        // for its known-resolvable name sets instead of reading them again.
        var allSharedScripts = await _templateRepo.GetAllSharedScriptsAsync(ct).ConfigureAwait(false);
-        var sharedScriptByName = allSharedScripts
-            .GroupBy(x => x.Name, StringComparer.Ordinal)
-            .ToDictionary(g => g.Key, g => g.First(), StringComparer.Ordinal);
+        var sharedScriptByName = ToNameLookup(allSharedScripts, x => x.Name);
        foreach (var s in content.SharedScripts)
        {
            sharedScriptByName.TryGetValue(s.Name, out var existing);
@@ -505,9 +509,7 @@ public sealed class BundleImporter : IBundleImporter

        // ---- ExternalSystems (+ their methods) ----
        var allExternalSystems = await _externalRepo.GetAllExternalSystemsAsync(ct).ConfigureAwait(false);
-        var externalSystemByName = allExternalSystems
-            .GroupBy(x => x.Name, StringComparer.Ordinal)
-            .ToDictionary(g => g.Key, g => g.First(), StringComparer.Ordinal);
+        var externalSystemByName = ToNameLookup(allExternalSystems, x => x.Name);
        foreach (var es in content.ExternalSystems)
        {
            externalSystemByName.TryGetValue(es.Name, out var existing);
@@ -543,7 +545,7 @@ public sealed class BundleImporter : IBundleImporter

        // ---- SmtpConfigurations (no by-host lookup — scan GetAll) ----
        var allSmtp = await _notificationRepo.GetAllSmtpConfigurationsAsync(ct).ConfigureAwait(false);
-        var smtpByHost = allSmtp.ToDictionary(s => s.Host, s => s, StringComparer.Ordinal);
+        var smtpByHost = ToNameLookup(allSmtp, s => s.Host);
        foreach (var sm in content.SmtpConfigs)
        {
            smtpByHost.TryGetValue(sm.Host, out var existing);
@@ -552,7 +554,7 @@ public sealed class BundleImporter : IBundleImporter

        // ---- SmsConfigurations (no by-AccountSid lookup — scan GetAll) ----
        var allSms = await _notificationRepo.GetAllSmsConfigurationsAsync(ct).ConfigureAwait(false);
-        var smsBySid = allSms.ToDictionary(s => s.AccountSid, s => s, StringComparer.Ordinal);
+        var smsBySid = ToNameLookup(allSms, s => s.AccountSid);
        foreach (var sms in content.SmsConfigs)
        {
            smsBySid.TryGetValue(sms.AccountSid, out var existing);
@@ -2281,7 +2283,7 @@ public sealed class BundleImporter : IBundleImporter
    {
        if (dtos.Count == 0) return;
        var existing = await _templateRepo.GetAllFoldersAsync(ct).ConfigureAwait(false);
-        var byName = existing.ToDictionary(f => f.Name, f => f, StringComparer.Ordinal);
+        var byName = ToNameLookup(existing, f => f.Name);

        foreach (var dto in dtos)
        {
@@ -2366,9 +2368,11 @@ public sealed class BundleImporter : IBundleImporter
            .ToList();
        // The by-name query runs under SQL Server's case-insensitive collation, so the
        // in-memory lookup must match case-insensitively too (ScadaBridge#93).
+        // Base templates only (ScadaBridge#95): an Overwrite must never land on a derived row
+        // that happens to carry the bundled template's name.
        var byName = ToTemplateLookup(overwriteNames.Count == 0
            ? Array.Empty<Template>()
-            : await _templateRepo.GetTemplatesWithChildrenAsync(overwriteNames, ct).ConfigureAwait(false));
+            : await _templateRepo.GetBaseTemplatesWithChildrenAsync(overwriteNames, ct).ConfigureAwait(false));

        // Folder ids must be materialised before we wire templates' FolderId
        // FKs. ApplyTemplateFoldersAsync staged the rows via AddFolderAsync
@@ -2379,7 +2383,8 @@ public sealed class BundleImporter : IBundleImporter
        // unit. The cost is one extra round-trip per import, negligible.
        await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false);
        var folders = await _templateRepo.GetAllFoldersAsync(ct).ConfigureAwait(false);
-        var folderIdByName = folders.ToDictionary(f => f.Name, f => f.Id, StringComparer.Ordinal);
+        var folderIdByName = ToNameLookup(folders, f => f.Name).ToDictionary(
+            kv => kv.Key, kv => kv.Value.Id, StringComparer.OrdinalIgnoreCase);

        // Honour folder renames -- a TemplateFolder resolved as Rename was
        // written under its new name; templates that reference the old name in
@@ -3816,7 +3821,7 @@ public sealed class BundleImporter : IBundleImporter
    {
        if (dtos.Count == 0) return;
        var allFolders = await _templateRepo.GetAllFoldersAsync(ct).ConfigureAwait(false);
-        var byName = allFolders.ToDictionary(f => f.Name, f => f, StringComparer.Ordinal);
+        var byName = ToNameLookup(allFolders, f => f.Name);

        foreach (var dto in dtos)
        {
@@ -4066,7 +4071,7 @@ public sealed class BundleImporter : IBundleImporter
    {
        if (dtos.Count == 0) return;
        var all = await _notificationRepo.GetAllSmtpConfigurationsAsync(ct).ConfigureAwait(false);
-        var byHost = all.ToDictionary(s => s.Host, s => s, StringComparer.Ordinal);
+        var byHost = ToNameLookup(all, s => s.Host);

        foreach (var dto in dtos)
        {
@@ -4152,7 +4157,7 @@ public sealed class BundleImporter : IBundleImporter
    {
        if (dtos.Count == 0) return;
        var all = await _notificationRepo.GetAllSmsConfigurationsAsync(ct).ConfigureAwait(false);
-        var bySid = all.ToDictionary(s => s.AccountSid, s => s, StringComparer.Ordinal);
+        var bySid = ToNameLookup(all, s => s.AccountSid);

        foreach (var dto in dtos)
        {
@@ -221,6 +221,27 @@ public class TemplateEngineRepositoryTests : IDisposable
        Assert.Equal("Alpha", result[0].Name);
    }

+    /// <summary>
+    /// ScadaBridge#95: the base-only bulk variant skips derived templates that share a
+    /// requested name, while the general variant still returns them.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task GetBaseTemplatesWithChildrenAsync_SkipsDerivedRowsSharingTheName()
+    {
+        var baseT = new Template("Alpha");
+        var derived = new Template("Alpha") { IsDerived = true };
+        var derivedOnly = new Template("Beta") { IsDerived = true };
+        _context.Templates.AddRange(baseT, derived, derivedOnly);
+        await _context.SaveChangesAsync();
+
+        var baseOnly = await _repository.GetBaseTemplatesWithChildrenAsync(new[] { "Alpha", "Beta" });
+        var all = await _repository.GetTemplatesWithChildrenAsync(new[] { "Alpha", "Beta" });
+
+        Assert.Equal(baseT.Id, Assert.Single(baseOnly).Id);
+        Assert.Equal(3, all.Count);
+    }
+
    /// <summary>Verifies that the parent template id is preserved so the inheritance chain can be walked.</summary>
    /// <returns>A task that represents the asynchronous operation.</returns>
    [Fact]
@@ -149,7 +149,8 @@ public sealed class ImportApplyScalingTests
        return new ApplyCost(
            detectChanges,
            target.CallsTo(nameof(ITemplateEngineRepository.GetTemplateWithChildrenAsync)),
-            target.CallsTo(nameof(ITemplateEngineRepository.GetTemplatesWithChildrenAsync)),
+            target.CallsTo(nameof(ITemplateEngineRepository.GetTemplatesWithChildrenAsync))
+                + target.CallsTo(nameof(ITemplateEngineRepository.GetBaseTemplatesWithChildrenAsync)),
            result);
    }

@@ -6,6 +6,7 @@ using Microsoft.Extensions.DependencyInjection;
 using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems;
 using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi;
 using ZB.MOM.WW.ScadaBridge.Commons.Entities.Notifications;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.Scripts;
 using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates;
 using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
 using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
@@ -173,4 +174,160 @@ public sealed class ImportNameLookupTests : IDisposable
        var lookup = BundleImporter.ToNameLookup(new[] { a, b }, x => x.Name);
        Assert.Same(a, lookup["list"]);
    }
+    // ---- ScadaBridge#95: the remaining libraries + derived templates ------------------
+
+    private async Task<Guid> ExportAndLoadAsync(Func<ScadaBridgeDbContext, Task<ExportSelection>> select)
+    {
+        Stream bundle;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            bundle = await scope.ServiceProvider.GetRequiredService<IBundleExporter>().ExportAsync(
+                await select(ctx), user: "alice", sourceEnvironment: "dev", passphrase: null,
+                cancellationToken: CancellationToken.None);
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            return (await scope.ServiceProvider.GetRequiredService<IBundleImporter>()
+                .LoadAsync(bundle, passphrase: null)).SessionId;
+        }
+    }
+
+    private async Task<ImportPreview> PreviewAsync(Guid sessionId)
+    {
+        await using var scope = _provider.CreateAsyncScope();
+        return await scope.ServiceProvider.GetRequiredService<IBundleImporter>().PreviewAsync(sessionId);
+    }
+
+    /// <summary>
+    /// Folders, shared scripts, external systems, SMTP and SMS configurations whose target
+    /// spelling differs only in case are the SAME rows under SQL Server's collation: preview
+    /// must report them as conflicts (not New), and an Overwrite must update them in place
+    /// instead of adding a second row.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Preview_and_apply_match_the_remaining_libraries_case_insensitively()
+    {
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var folder = new TemplateFolder("pumps");
+            ctx.TemplateFolders.Add(folder);
+            await ctx.SaveChangesAsync();
+            ctx.Templates.Add(new Template("Pump") { FolderId = folder.Id });
+            ctx.SharedScripts.Add(new SharedScript("Helper", "return 1;"));
+            ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Erp", "http://erp", "None"));
+            ctx.SmtpConfigurations.Add(new SmtpConfiguration("mail.local", "None", "a@b.c"));
+            ctx.SmsConfigurations.Add(new SmsConfiguration("AC123"));
+            await ctx.SaveChangesAsync();
+        }
+
+        var sessionId = await ExportAndLoadAsync(async ctx => new ExportSelection(
+            TemplateIds: await ctx.Templates.Select(x => x.Id).ToListAsync(),
+            SharedScriptIds: await ctx.SharedScripts.Select(x => x.Id).ToListAsync(),
+            ExternalSystemIds: await ctx.ExternalSystemDefinitions.Select(x => x.Id).ToListAsync(),
+            DatabaseConnectionIds: Array.Empty<int>(),
+            NotificationListIds: Array.Empty<int>(),
+            SmtpConfigurationIds: await ctx.SmtpConfigurations.Select(x => x.Id).ToListAsync(),
+            ApiMethodIds: Array.Empty<int>(),
+            IncludeDependencies: false,
+            SmsConfigurationIds: await ctx.SmsConfigurations.Select(x => x.Id).ToListAsync()));
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            (await ctx.TemplateFolders.SingleAsync()).Name = "PUMPS";
+            (await ctx.SharedScripts.SingleAsync()).Name = "HELPER";
+            (await ctx.ExternalSystemDefinitions.SingleAsync()).Name = "ERP";
+            (await ctx.SmtpConfigurations.SingleAsync()).Host = "MAIL.LOCAL";
+            (await ctx.SmsConfigurations.SingleAsync()).AccountSid = "ac123";
+            await ctx.SaveChangesAsync();
+        }
+
+        var preview = await PreviewAsync(sessionId);
+        string[] types = ["TemplateFolder", "SharedScript", "ExternalSystem", "SmtpConfiguration", "SmsConfiguration"];
+        foreach (var type in types)
+        {
+            var item = Assert.Single(preview.Items, i => i.EntityType == type);
+            Assert.NotEqual(ConflictKind.New, item.Kind);
+        }
+
+        var resolutions = preview.Items
+            .Where(i => i.EntityType is "TemplateFolder" or "SmtpConfiguration" or "SmsConfiguration")
+            .Select(i => new ImportResolution(i.EntityType, i.Name, ResolutionAction.Overwrite, null))
+            .Concat(preview.Items
+                .Where(i => i.EntityType is not ("TemplateFolder" or "SmtpConfiguration" or "SmsConfiguration"))
+                .Select(i => new ImportResolution(i.EntityType, i.Name, ResolutionAction.Skip, null)))
+            .ToList();
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            await scope.ServiceProvider.GetRequiredService<IBundleImporter>()
+                .ApplyAsync(sessionId, resolutions, user: "bob");
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            Assert.Equal(1, await ctx.TemplateFolders.CountAsync());
+            Assert.Equal(1, await ctx.SmtpConfigurations.CountAsync());
+            Assert.Equal(1, await ctx.SmsConfigurations.CountAsync());
+        }
+    }
+
+    /// <summary>
+    /// A derived template carries its composition slot name; a bundled template of that name
+    /// targets a BASE template, so a target whose only row of that name is derived must see
+    /// the bundled template as New, and applying it must add a base template rather than
+    /// overwrite the derived one.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task A_derived_template_sharing_the_bundled_name_is_not_the_import_target()
+    {
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.Templates.Add(new Template("Motor"));
+            await ctx.SaveChangesAsync();
+        }
+
+        var sessionId = await ExportAndLoadAsync(async ctx => new ExportSelection(
+            TemplateIds: await ctx.Templates.Select(x => x.Id).ToListAsync(),
+            SharedScriptIds: Array.Empty<int>(),
+            ExternalSystemIds: Array.Empty<int>(),
+            DatabaseConnectionIds: Array.Empty<int>(),
+            NotificationListIds: Array.Empty<int>(),
+            SmtpConfigurationIds: Array.Empty<int>(),
+            ApiMethodIds: Array.Empty<int>(),
+            IncludeDependencies: false));
+
+        // The target's only "Motor" is a derived slot template.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            (await ctx.Templates.SingleAsync()).IsDerived = true;
+            await ctx.SaveChangesAsync();
+        }
+
+        var preview = await PreviewAsync(sessionId);
+        Assert.Equal(ConflictKind.New, Assert.Single(preview.Items, i => i.EntityType == "Template").Kind);
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            await scope.ServiceProvider.GetRequiredService<IBundleImporter>().ApplyAsync(
+                sessionId,
+                [new ImportResolution("Template", "Motor", ResolutionAction.Overwrite, null)],
+                user: "bob");
+        }
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var motors = await ctx.Templates.Where(t => t.Name == "Motor").ToListAsync();
+            Assert.Equal(2, motors.Count);
+            Assert.Single(motors, t => !t.IsDerived);
+        }
+    }
 }