fix(transport): import reads the template library once per session, batches instance lookups, and expands stale instances to derived/composing templates over one flatten session (review 2026-10-21 04/P4, 08/P1, 04/C2)

2026-10-22 09:36:34 -04:00
parent 39025b28fa
commit 7e08a1dbfb
8 changed files with 723 additions and 109 deletions
@@ -1505,6 +1505,111 @@ public sealed class BundleImporterApplyTests : IDisposable
        Assert.DoesNotContain(valveInstanceId, result.StaleInstanceIds);
    }

+    /// <summary>
+    /// Review 2026-10-21 04/C2: overwriting a feature MODULE changes the flattened
+    /// config of every template that composes it, so a deployed instance of the
+    /// composing template — which has no direct relationship to the overwritten
+    /// template id — must be reported stale.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task ApplyAsync_overwriting_a_composed_module_marks_instances_of_the_composing_template_stale()
+    {
+        await SeedSiteAsync();
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var module = new Template("Motor") { Description = "module" };
+            module.Attributes.Add(new TemplateAttribute("Speed") { DataType = DataType.Float, Value = "1.0" });
+            module.Attributes.Add(new TemplateAttribute("Torque") { DataType = DataType.Float, Value = "2.0" });
+            ctx.Templates.Add(module);
+            await ctx.SaveChangesAsync();
+            var station = new Template("Station") { Description = "composer" };
+            station.Compositions.Add(new TemplateComposition("motor1") { ComposedTemplateId = module.Id });
+            ctx.Templates.Add(station);
+            await ctx.SaveChangesAsync();
+        }
+        var sessionId = await ExportAndLoadAsync();
+
+        // Drop Torque from the target module, then deploy a Station instance against
+        // that shape; the Overwrite restores Torque inside the composed slot.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var module = await ctx.Templates.Include(x => x.Attributes).SingleAsync(x => x.Name == "Motor");
+            var torque = module.Attributes.Single(a => a.Name == "Torque");
+            module.Attributes.Remove(torque);
+            ctx.TemplateAttributes.Remove(torque);
+            await ctx.SaveChangesAsync();
+        }
+        var stationInstanceId = await SeedDeployedInstanceWithRealSnapshotAsync("Station", "Station-Deployed");
+
+        ImportResult result;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            result = await importer.ApplyAsync(sessionId,
+                new List<ImportResolution>
+                {
+                    new("Template", "Motor", ResolutionAction.Overwrite, null),
+                    new("Template", "Station", ResolutionAction.Skip, null),
+                },
+                user: "bob");
+        }
+
+        Assert.Contains(stationInstanceId, result.StaleInstanceIds);
+    }
+
+    /// <summary>
+    /// Review 2026-10-21 04/C2: overwriting a BASE template changes every derived
+    /// template's flattened config, so a deployed instance of a derived template must
+    /// be reported stale even though no instance uses the base directly.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task ApplyAsync_overwriting_a_base_template_marks_instances_of_derived_templates_stale()
+    {
+        await SeedSiteAsync();
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var baseT = new Template("Receiver") { Description = "base" };
+            baseT.Attributes.Add(new TemplateAttribute("Flow") { DataType = DataType.Float, Value = "1.0" });
+            baseT.Attributes.Add(new TemplateAttribute("Pressure") { DataType = DataType.Float, Value = "2.0" });
+            ctx.Templates.Add(baseT);
+            await ctx.SaveChangesAsync();
+            ctx.Templates.Add(new Template("Reactor") { Description = "derived", ParentTemplateId = baseT.Id });
+            await ctx.SaveChangesAsync();
+        }
+        var sessionId = await ExportAndLoadAsync();
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var baseT = await ctx.Templates.Include(x => x.Attributes).SingleAsync(x => x.Name == "Receiver");
+            var pressure = baseT.Attributes.Single(a => a.Name == "Pressure");
+            baseT.Attributes.Remove(pressure);
+            ctx.TemplateAttributes.Remove(pressure);
+            await ctx.SaveChangesAsync();
+        }
+        var reactorInstanceId = await SeedDeployedInstanceWithRealSnapshotAsync("Reactor", "Reactor-Deployed");
+
+        ImportResult result;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            result = await importer.ApplyAsync(sessionId,
+                new List<ImportResolution>
+                {
+                    new("Template", "Receiver", ResolutionAction.Overwrite, null),
+                    new("Template", "Reactor", ResolutionAction.Skip, null),
+                },
+                user: "bob");
+        }
+
+        Assert.Contains(reactorInstanceId, result.StaleInstanceIds);
+    }
+
    /// <summary>Verifies that a freshly imported NotDeployed instance is never reported as stale.</summary>
    /// <returns>A task that represents the asynchronous operation.</returns>
    [Fact]
@@ -0,0 +1,209 @@
+using System.Collections.Concurrent;
+using System.Reflection;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Diagnostics;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.DependencyInjection;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates;
+using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
+using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
+using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport;
+using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
+using ZB.MOM.WW.ScadaBridge.Commons.Types.Templates;
+using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
+using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase;
+using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories;
+using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Services;
+
+namespace ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests.Import;
+
+/// <summary>
+/// Review 2026-10-21 04/P4 + 08/P1: one import used to load the WHOLE template
+/// library (attributes, alarms, scripts with bodies, compositions — tracked) up to
+/// nine times, two of them inside per-template loops, and to look every bundled
+/// instance up one query at a time. These tests count repository calls through a
+/// recording proxy so the per-session budget is pinned.
+/// </summary>
+public sealed class ImportRoundTripBudgetTests : IDisposable
+{
+    private readonly ServiceProvider _provider;
+    private readonly ConcurrentDictionary<string, int> _calls = new(StringComparer.Ordinal);
+
+    /// <summary>Initializes the test class with a counting template repository over an in-memory database.</summary>
+    public ImportRoundTripBudgetTests()
+    {
+        var services = new ServiceCollection();
+        services.AddSingleton<Microsoft.AspNetCore.DataProtection.IDataProtectionProvider>(
+            new Microsoft.AspNetCore.DataProtection.EphemeralDataProtectionProvider());
+        services.AddSingleton<IConfiguration>(new ConfigurationBuilder().AddInMemoryCollection().Build());
+
+        var dbName = $"ImportRoundTripBudgetTests_{Guid.NewGuid()}";
+        services.AddDbContext<ScadaBridgeDbContext>(opts => opts
+            .UseInMemoryDatabase(dbName)
+            .ConfigureWarnings(w => w.Ignore(InMemoryEventId.TransactionIgnoredWarning)));
+
+        services.AddSingleton<ITemplateGraphWatermark, TemplateGraphWatermark>();
+        services.AddScoped<ITemplateEngineRepository>(sp => CountingProxy<ITemplateEngineRepository>.Wrap(
+            new TemplateEngineRepository(
+                sp.GetRequiredService<ScadaBridgeDbContext>(),
+                sp.GetRequiredService<ITemplateGraphWatermark>()),
+            _calls));
+        services.AddScoped<IExternalSystemRepository, ExternalSystemRepository>();
+        services.AddScoped<INotificationRepository, NotificationRepository>();
+        services.AddScoped<IInboundApiRepository, InboundApiRepository>();
+        services.AddScoped<ISiteRepository, SiteRepository>();
+        services.AddScoped<IAuditCorrelationContext, AuditCorrelationContext>();
+        services.AddScoped<IAuditService, AuditService>();
+        services.AddTransport();
+
+        _provider = services.BuildServiceProvider();
+    }
+
+    /// <summary>Releases the resources used by this instance.</summary>
+    public void Dispose() => _provider.Dispose();
+
+    private int Calls(string method) => _calls.TryGetValue(method, out var n) ? n : 0;
+
+    /// <summary>
+    /// Seeds a library of templates (a base, a module, and several composing /
+    /// derived templates) plus a site with instances, exports the site closure with
+    /// its templates, and returns the loaded session.
+    /// </summary>
+    private async Task<Guid> SeedExportAndLoadAsync(int instanceCount)
+    {
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var baseT = new Template("Base");
+            baseT.Attributes.Add(new TemplateAttribute("Flow") { DataType = DataType.Float, Value = "1.0" });
+            var module = new Template("Motor");
+            module.Attributes.Add(new TemplateAttribute("Speed") { DataType = DataType.Float, Value = "1.0" });
+            ctx.Templates.AddRange(baseT, module);
+            await ctx.SaveChangesAsync();
+
+            var pump = new Template("Pump") { ParentTemplateId = baseT.Id };
+            pump.Compositions.Add(new TemplateComposition("motor1") { ComposedTemplateId = module.Id });
+            ctx.Templates.Add(pump);
+            // Unrelated library templates: the import must not pay for them per item.
+            for (var i = 0; i < 5; i++) ctx.Templates.Add(new Template($"Other{i}"));
+            var site = new Site("Plant 1", "plant-1") { GrpcNodeAAddress = "http://plant-1-a:8083" };
+            ctx.Sites.Add(site);
+            await ctx.SaveChangesAsync();
+
+            for (var i = 0; i < instanceCount; i++)
+            {
+                ctx.Instances.Add(new Instance($"Pump-{i:00}") { TemplateId = pump.Id, SiteId = site.Id });
+            }
+            await ctx.SaveChangesAsync();
+        }
+
+        Stream bundle;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var siteIds = await ctx.Sites.Select(s => s.Id).ToListAsync();
+            bundle = await scope.ServiceProvider.GetRequiredService<IBundleExporter>().ExportAsync(
+                new ExportSelection(
+                    TemplateIds: Array.Empty<int>(),
+                    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: true,
+                    SiteIds: siteIds),
+                user: "alice", sourceEnvironment: "dev", passphrase: null,
+                cancellationToken: CancellationToken.None);
+        }
+
+        await using var loadScope = _provider.CreateAsyncScope();
+        return (await loadScope.ServiceProvider.GetRequiredService<IBundleImporter>()
+            .LoadAsync(bundle, passphrase: null)).SessionId;
+    }
+
+    /// <summary>
+    /// Verifies that preview + apply of a bundle carrying templates and instances
+    /// never loads the full template graph, loads the library projection at most once
+    /// per phase, and looks instances up in batches rather than per instance.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Preview_and_apply_load_the_template_library_once_per_phase_and_batch_instance_lookups()
+    {
+        var sessionId = await SeedExportAndLoadAsync(instanceCount: 6);
+        _calls.Clear();
+
+        ImportPreview preview;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            preview = await scope.ServiceProvider.GetRequiredService<IBundleImporter>().PreviewAsync(sessionId);
+        }
+        Assert.DoesNotContain(preview.Items, i => i.Kind == ConflictKind.Blocker);
+
+        Assert.Equal(0, Calls(nameof(ITemplateEngineRepository.GetAllTemplatesAsync)));
+        Assert.Equal(0, Calls(nameof(ITemplateEngineRepository.GetInstanceByUniqueNameAsync)));
+        Assert.Equal(0, Calls(nameof(ITemplateEngineRepository.GetTemplateByIdAsync)));
+        Assert.True(
+            Calls(nameof(ITemplateEngineRepository.GetAllTemplatesForAnalysisAsync))
+            + Calls(nameof(ITemplateEngineRepository.GetTemplateStubsAsync)) <= 2,
+            "preview should load the library projection at most once per shape");
+        _calls.Clear();
+
+        var resolutions = preview.Items
+            .Where(i => i.EntityType is "Template" or "Instance" or "Site")
+            .Select(i => new ImportResolution(i.EntityType, i.Name, ResolutionAction.Overwrite, null))
+            .ToList();
+        ImportResult result;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            result = await scope.ServiceProvider.GetRequiredService<IBundleImporter>()
+                .ApplyAsync(sessionId, resolutions, user: "bob");
+        }
+
+        Assert.True(result.Overwritten >= 6 + 3, $"expected the templates and instances to be overwritten, got {result.Overwritten}");
+        Assert.Equal(0, Calls(nameof(ITemplateEngineRepository.GetAllTemplatesAsync)));
+        Assert.Equal(0, Calls(nameof(ITemplateEngineRepository.GetInstanceByUniqueNameAsync)));
+        Assert.True(Calls(nameof(ITemplateEngineRepository.GetAllTemplatesForAnalysisAsync)) <= 1,
+            "apply should load the analysis library at most once");
+        Assert.True(Calls(nameof(ITemplateEngineRepository.GetInstancesByUniqueNamesAsync)) <= 1,
+            "apply should look the bundle's instances up in one batch");
+    }
+
+    /// <summary>Records every interface call by method name, then forwards it to the real implementation.</summary>
+    /// <typeparam name="T">The proxied interface.</typeparam>
+    public class CountingProxy<T> : DispatchProxy where T : class
+    {
+        private T _inner = null!;
+        private ConcurrentDictionary<string, int> _calls = null!;
+
+        /// <summary>Creates a counting proxy over <paramref name="inner"/>.</summary>
+        /// <param name="inner">The real implementation.</param>
+        /// <param name="calls">The shared call counter.</param>
+        /// <returns>The proxy.</returns>
+        public static T Wrap(T inner, ConcurrentDictionary<string, int> calls)
+        {
+            var proxy = Create<T, CountingProxy<T>>();
+            var self = (CountingProxy<T>)(object)proxy;
+            self._inner = inner;
+            self._calls = calls;
+            return proxy;
+        }
+
+        /// <inheritdoc />
+        protected override object? Invoke(MethodInfo? targetMethod, object?[]? args)
+        {
+            _calls.AddOrUpdate(targetMethod!.Name, 1, (_, n) => n + 1);
+            try
+            {
+                return targetMethod.Invoke(_inner, args);
+            }
+            catch (TargetInvocationException ex) when (ex.InnerException is not null)
+            {
+                throw ex.InnerException;
+            }
+        }
+    }
+}