From 04ac4c0491ed0752bd85cfc2989896e570bf1324 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Thu, 22 Oct 2026 09:24:31 -0400
Subject: [PATCH] fix(transport): export reads instance children once, site
 connections once per site, composed templates in batches (review 2026-10-21
 04/P5)

---
 docs/requirements/Component-Transport.md      |   2 +
 .../Export/DependencyResolver.cs              | 145 ++++++++++++------
 .../Export/DependencyResolverTests.cs         | 117 +++++++++++++-
 3 files changed, 214 insertions(+), 50 deletions(-)

diff --git a/docs/requirements/Component-Transport.md b/docs/requirements/Component-Transport.md
index b1ef7fdc..f86f9513 100644
--- a/docs/requirements/Component-Transport.md
+++ b/docs/requirements/Component-Transport.md
@@ -159,6 +159,8 @@ A **Sites & Instances** section (T18) adds a flat list of sites, each row expand
 
 The user can toggle "include all dependencies" off (with a warning that the bundle may produce an invalid import).
 
+**Resolver round trips (review 2026-10-21 04/P5).** Instances are read with their override/binding collections in the same query (`GetInstanceByIdAsync` / the template repository's `GetInstancesBySiteIdAsync` both `Include` them — there is no per-collection reload); a site's data connections are read once per export and shared between the site seed and the per-instance closure; and missing templates (instance templates, then each level of the composition closure) are fetched in one batch through `GetTemplatesWithChildrenAsync` keyed by an id→name map from `GetTemplateStubsAsync` — a lone missing template still uses the by-id accessor. Exporting one 500-instance site is a few dozen queries rather than ~5 000.
+
 **Step 3 — Encryption.** Passphrase and confirm. Strength meter. Explicit warning lists every secret field being encrypted. An "Export without encryption" path is logged with the `UnencryptedBundleExport` audit flag.
 
 **Step 4 — Download.** Generated filename pattern: `scadabundle-{sourceEnv}-{yyyy-MM-dd-HHmmss}.scadabundle`. SHA-256 displayed for out-of-band verification.
diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Export/DependencyResolver.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Export/DependencyResolver.cs
index b7c7374e..ce8e0ef0 100644
--- a/src/ZB.MOM.WW.ScadaBridge.Transport/Export/DependencyResolver.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Export/DependencyResolver.cs
@@ -136,6 +136,10 @@ public sealed class DependencyResolver
         var dataConnections = new Dictionary<int, DataConnection>();
         var instances = new Dictionary<int, Instance>();
 
+        // Review 2026-10-21 04/P5: one GetDataConnectionsBySiteIdAsync per site per
+        // export — the site seed and the per-instance closure below share this cache.
+        var siteConnectionCache = new Dictionary<int, IReadOnlyList<DataConnection>>();
+
         // Selecting a SITE pulls its data connections and all of its instances. The
         // instance's overrides/bindings are loaded eagerly so the serializer can read
         // them off the entity navigation collections.
@@ -145,16 +149,17 @@ public sealed class DependencyResolver
             if (site is null) continue;
             sites[site.Id] = site;
 
-            foreach (var conn in await _siteRepository.GetDataConnectionsBySiteIdAsync(site.Id, ct).ConfigureAwait(false))
+            foreach (var conn in await GetSiteConnectionsAsync(site.Id, siteConnectionCache, ct).ConfigureAwait(false))
             {
                 dataConnections[conn.Id] = conn;
             }
 
-            foreach (var siteInstance in await _siteRepository.GetInstancesBySiteIdAsync(site.Id, ct).ConfigureAwait(false))
+            // The template repository's GetInstancesBySiteIdAsync Includes the four
+            // override/binding collections (the site repository's does not), so the
+            // site-loaded entity is used as-is — no per-instance reload (04/P5).
+            foreach (var siteInstance in await _templates.GetInstancesBySiteIdAsync(site.Id, ct).ConfigureAwait(false))
             {
-                if (instances.ContainsKey(siteInstance.Id)) continue;
-                var loaded = await LoadInstanceWithChildrenAsync(siteInstance.Id, ct).ConfigureAwait(false);
-                if (loaded is not null) instances[loaded.Id] = loaded;
+                instances.TryAdd(siteInstance.Id, siteInstance);
             }
         }
 
@@ -164,7 +169,9 @@ public sealed class DependencyResolver
         foreach (var instanceId in selection.InstanceIds.Distinct())
         {
             if (instances.ContainsKey(instanceId)) continue;
-            var loaded = await LoadInstanceWithChildrenAsync(instanceId, ct).ConfigureAwait(false);
+            // GetInstanceByIdAsync Includes the four override/binding collections, so
+            // no per-collection reload is needed (04/P5).
+            var loaded = await _templates.GetInstanceByIdAsync(instanceId, ct).ConfigureAwait(false);
             if (loaded is not null) instances[loaded.Id] = loaded;
         }
 
@@ -175,9 +182,11 @@ public sealed class DependencyResolver
             // binds, and (by feeding instance.TemplateId into the template dictionary)
             // its template — so the template/shared-script/external-system closure below
             // expands transitively over instance templates too.
-            await ExpandSiteInstanceClosureAsync(instances, sites, dataConnections, templates, ct).ConfigureAwait(false);
+            var templateIdByName = new TemplateNameLookup(_templates);
+            await ExpandSiteInstanceClosureAsync(
+                instances, sites, dataConnections, templates, siteConnectionCache, templateIdByName, ct).ConfigureAwait(false);
 
-            await ExpandTemplateClosureAsync(templates, ct).ConfigureAwait(false);
+            await ExpandTemplateClosureAsync(templates, templateIdByName, ct).ConfigureAwait(false);
             await ExpandSharedScriptsFromTemplatesAsync(templates, sharedScripts, ct).ConfigureAwait(false);
             await ExpandSharedScriptsFromApiMethodsAsync(apiMethods, sharedScripts, ct).ConfigureAwait(false);
             await ExpandExternalSystemsFromTemplatesAsync(templates, externalSystems, ct).ConfigureAwait(false);
@@ -272,25 +281,71 @@ public sealed class DependencyResolver
         };
     }
 
-    // ---- Instance child loading ----
-    // Loads an instance and attaches its overrides/bindings onto the entity's own
-    // navigation collections, so the serializer reads children off the entity (the
-    // same shape central-config entities use). Returns null if the instance is gone.
-    private async Task<Instance?> LoadInstanceWithChildrenAsync(int instanceId, CancellationToken ct)
+    // ---- Batched loads (review 2026-10-21 04/P5) ----
+
+    // Reads a site's data connections at most once per ResolveAsync call.
+    private async Task<IReadOnlyList<DataConnection>> GetSiteConnectionsAsync(
+        int siteId, Dictionary<int, IReadOnlyList<DataConnection>> cache, CancellationToken ct)
     {
-        var instance = await _templates.GetInstanceByIdAsync(instanceId, ct).ConfigureAwait(false);
-        if (instance is null) return null;
+        if (!cache.TryGetValue(siteId, out var connections))
+        {
+            connections = await _siteRepository.GetDataConnectionsBySiteIdAsync(siteId, ct).ConfigureAwait(false);
+            cache[siteId] = connections;
+        }
+        return connections;
+    }
 
-        var attributeOverrides = await _templates.GetOverridesByInstanceIdAsync(instance.Id, ct).ConfigureAwait(false);
-        var alarmOverrides = await _templates.GetAlarmOverridesByInstanceIdAsync(instance.Id, ct).ConfigureAwait(false);
-        var nativeAlarmOverrides = await _templates.GetNativeAlarmSourceOverridesByInstanceIdAsync(instance.Id, ct).ConfigureAwait(false);
-        var bindings = await _templates.GetBindingsByInstanceIdAsync(instance.Id, ct).ConfigureAwait(false);
+    // Lazily-loaded id → name map over the whole template library (a body-free
+    // projection), so a set of template ids can be fetched with children through the
+    // by-name bulk accessor in one round trip instead of one by-id load each.
+    private sealed class TemplateNameLookup
+    {
+        private readonly ITemplateEngineRepository _repo;
+        private Dictionary<int, string>? _nameById;
 
-        instance.AttributeOverrides = attributeOverrides.ToList();
-        instance.AlarmOverrides = alarmOverrides.ToList();
-        instance.NativeAlarmSourceOverrides = nativeAlarmOverrides.ToList();
-        instance.ConnectionBindings = bindings.ToList();
-        return instance;
+        public TemplateNameLookup(ITemplateEngineRepository repo) => _repo = repo;
+
+        public async Task<IReadOnlyDictionary<int, string>> GetAsync(CancellationToken ct)
+        {
+            if (_nameById is null)
+            {
+                var stubs = await _repo.GetTemplateStubsAsync(ct).ConfigureAwait(false);
+                _nameById = stubs.ToDictionary(t => t.Id, t => t.Name);
+            }
+            return _nameById;
+        }
+    }
+
+    // Adds every template in `ids` not yet in `templates`, with children. A single
+    // missing id keeps the by-id accessor (no library projection needed); two or more
+    // go through one batched by-name load. Returns the newly added templates.
+    private async Task<List<Template>> LoadMissingTemplatesAsync(
+        IEnumerable<int> ids,
+        Dictionary<int, Template> templates,
+        TemplateNameLookup lookup,
+        CancellationToken ct)
+    {
+        var missing = ids.Where(id => !templates.ContainsKey(id)).Distinct().ToList();
+        var added = new List<Template>();
+        if (missing.Count == 0) return added;
+
+        if (missing.Count == 1)
+        {
+            var single = await _templates.GetTemplateWithChildrenAsync(missing[0], ct).ConfigureAwait(false);
+            if (single is not null && templates.TryAdd(single.Id, single)) added.Add(single);
+            return added;
+        }
+
+        var nameById = await lookup.GetAsync(ct).ConfigureAwait(false);
+        var names = missing.Where(nameById.ContainsKey).Select(id => nameById[id]).ToList();
+        if (names.Count == 0) return added;
+
+        var missingSet = missing.ToHashSet();
+        foreach (var t in await _templates.GetTemplatesWithChildrenAsync(names, ct).ConfigureAwait(false))
+        {
+            if (missingSet.Contains(t.Id) && templates.TryAdd(t.Id, t)) added.Add(t);
+        }
+        return added;
     }
 
     // ---- Site/instance closure ----
@@ -304,8 +359,15 @@ public sealed class DependencyResolver
         Dictionary<int, Site> sites,
         Dictionary<int, DataConnection> dataConnections,
         Dictionary<int, Template> templates,
+        Dictionary<int, IReadOnlyList<DataConnection>> siteConnectionCache,
+        TemplateNameLookup templateLookup,
         CancellationToken ct)
     {
+        // 2. Templates → the existing template closure picks them up below. Loaded
+        //    in one batch across every gathered instance (04/P5).
+        await LoadMissingTemplatesAsync(
+            instances.Values.Select(i => i.TemplateId), templates, templateLookup, ct).ConfigureAwait(false);
+
         // Snapshot the instances: the loop only grows sites/dataConnections/templates,
         // never instances, so iterating a snapshot keeps the dictionary stable.
         foreach (var instance in instances.Values.ToList())
@@ -317,13 +379,6 @@ public sealed class DependencyResolver
                 if (site is not null) sites[site.Id] = site;
             }
 
-            // 2. Template → the existing template closure picks it up below.
-            if (!templates.ContainsKey(instance.TemplateId))
-            {
-                var template = await _templates.GetTemplateWithChildrenAsync(instance.TemplateId, ct).ConfigureAwait(false);
-                if (template is not null) templates[template.Id] = template;
-            }
-
             // 3. Data connections the instance references, resolved within its site.
             //    Connection bindings reference connections by id; native-alarm-source
             //    overrides reference them by name — resolve both against the site set.
@@ -341,8 +396,7 @@ public sealed class DependencyResolver
 
             if (bindingConnectionIds.Count == 0 && referencedConnectionNames.Count == 0) continue;
 
-            var siteConnections = await _siteRepository
-                .GetDataConnectionsBySiteIdAsync(instance.SiteId, ct)
+            var siteConnections = await GetSiteConnectionsAsync(instance.SiteId, siteConnectionCache, ct)
                 .ConfigureAwait(false);
 
             foreach (var conn in siteConnections)
@@ -360,22 +414,19 @@ public sealed class DependencyResolver
         siteIdentifierById.TryGetValue(siteId, out var id) ? id : siteId.ToString();
 
     // ---- Template composition closure ----
-    private async Task ExpandTemplateClosureAsync(Dictionary<int, Template> templates, CancellationToken ct)
+    // Breadth-first by level: every module composed by the current frontier is
+    // loaded in one batch (04/P5), then becomes the next frontier.
+    private async Task ExpandTemplateClosureAsync(
+        Dictionary<int, Template> templates, TemplateNameLookup templateLookup, CancellationToken ct)
     {
-        var queue = new Queue<Template>(templates.Values);
-        while (queue.Count > 0)
+        IReadOnlyCollection<Template> frontier = templates.Values.ToList();
+        while (frontier.Count > 0)
         {
-            var current = queue.Dequeue();
-            foreach (var comp in current.Compositions)
-            {
-                if (templates.ContainsKey(comp.ComposedTemplateId)) continue;
-                var composed = await _templates
-                    .GetTemplateWithChildrenAsync(comp.ComposedTemplateId, ct)
-                    .ConfigureAwait(false);
-                if (composed is null) continue;
-                templates[composed.Id] = composed;
-                queue.Enqueue(composed);
-            }
+            frontier = await LoadMissingTemplatesAsync(
+                frontier.SelectMany(t => t.Compositions).Select(c => c.ComposedTemplateId),
+                templates,
+                templateLookup,
+                ct).ConfigureAwait(false);
         }
     }
 
diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Export/DependencyResolverTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Export/DependencyResolverTests.cs
index 7101eb2c..44c0bba7 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Export/DependencyResolverTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Export/DependencyResolverTests.cs
@@ -7,6 +7,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Scripts;
 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.Types.Templates;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
 using ZB.MOM.WW.ScadaBridge.Transport.Export;
 
@@ -76,9 +77,28 @@ public sealed class DependencyResolverTests
         IncludeDependencies: true,
         SmsConfigurationIds: ids);
 
+    // Templates registered via StubTemplate. Backs both the by-id accessor and the
+    // batched stub + by-name accessors the resolver uses for closure expansion
+    // (review 2026-10-21 04/P5).
+    private readonly List<Template> _stubbedTemplates = new();
+
     private void StubTemplate(Template t)
     {
         _templates.GetTemplateWithChildrenAsync(t.Id, Arg.Any<CancellationToken>()).Returns(t);
+        if (_stubbedTemplates.Count == 0)
+        {
+            _templates.GetTemplateStubsAsync(Arg.Any<CancellationToken>()).Returns(_ =>
+                (IReadOnlyList<TemplateStub>)_stubbedTemplates
+                    .Select(x => new TemplateStub(x.Id, x.Name, x.ParentTemplateId, x.FolderId, x.IsDerived, x.OwnerCompositionId))
+                    .ToList());
+            _templates.GetTemplatesWithChildrenAsync(Arg.Any<IEnumerable<string>>(), Arg.Any<CancellationToken>())
+                .Returns(ci =>
+                {
+                    var names = ci.Arg<IEnumerable<string>>().ToHashSet(StringComparer.Ordinal);
+                    return (IReadOnlyList<Template>)_stubbedTemplates.Where(x => names.Contains(x.Name)).ToList();
+                });
+        }
+        _stubbedTemplates.Add(t);
     }
 
     private void StubAllSharedScripts(params SharedScript[] scripts)
@@ -114,7 +134,7 @@ public sealed class DependencyResolverTests
 
     private void StubSiteInstances(int siteId, params Instance[] instances)
     {
-        _sites.GetInstancesBySiteIdAsync(siteId, Arg.Any<CancellationToken>()).Returns(instances);
+        _templates.GetInstancesBySiteIdAsync(siteId, Arg.Any<CancellationToken>()).Returns(instances);
     }
 
     // Stubs the per-instance child getters on the template repo so the resolver can
@@ -442,8 +462,9 @@ public sealed class DependencyResolverTests
 
         Assert.Single(result.Instances, i => i.UniqueName == "Mixer01");
         Assert.Single(result.Sites, s => s.SiteIdentifier == "site-e");
-        // The instance was loaded exactly once (site path), not re-loaded via the instance path.
-        await _templates.Received(1).GetInstanceByIdAsync(instance.Id, Arg.Any<CancellationToken>());
+        // The instance came from the site load (which already includes its children)
+        // and was not re-loaded via the instance path (review 2026-10-21 04/P5).
+        await _templates.DidNotReceive().GetInstanceByIdAsync(instance.Id, Arg.Any<CancellationToken>());
     }
 
     /// <summary>Verifies that resolving orders sites, connections and instances deterministically.</summary>
@@ -524,4 +545,94 @@ public sealed class DependencyResolverTests
         Assert.Contains("Site:site-n", instEntry.DependsOn);
         Assert.Contains("DataConnection:site-n/opc-1", instEntry.DependsOn);
     }
+
+    // ─────────────────────────────────────────────────────────────────────
+    // Review 2026-10-21 04/P5 — export round trips.
+    // ─────────────────────────────────────────────────────────────────────
+
+    /// <summary>
+    /// Verifies that a directly-selected instance is loaded once, with its children
+    /// from the same query, and the four per-collection child loads are not issued.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Resolve_instance_does_not_reload_its_child_collections()
+    {
+        var site = new Site("Plant", "site-p") { Id = 5 };
+        var instance = new Instance("Pump01") { Id = 70, SiteId = site.Id, TemplateId = 0 };
+        StubSite(site);
+        StubInstance(instance);
+        StubAllSharedScripts();
+        StubAllExternalSystems();
+        StubAllFolders();
+
+        var result = await Sut().ResolveAsync(SelectInstances(new[] { instance.Id }), CancellationToken.None);
+
+        Assert.Single(result.Instances);
+        await _templates.Received(1).GetInstanceByIdAsync(instance.Id, Arg.Any<CancellationToken>());
+        await _templates.DidNotReceive().GetOverridesByInstanceIdAsync(Arg.Any<int>(), Arg.Any<CancellationToken>());
+        await _templates.DidNotReceive().GetAlarmOverridesByInstanceIdAsync(Arg.Any<int>(), Arg.Any<CancellationToken>());
+        await _templates.DidNotReceive().GetNativeAlarmSourceOverridesByInstanceIdAsync(Arg.Any<int>(), Arg.Any<CancellationToken>());
+        await _templates.DidNotReceive().GetBindingsByInstanceIdAsync(Arg.Any<int>(), Arg.Any<CancellationToken>());
+    }
+
+    /// <summary>
+    /// Verifies that a site's data connections are read once per export, not once per
+    /// instance that binds one.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Resolve_site_reads_its_data_connections_once()
+    {
+        var site = new Site("Plant", "site-p") { Id = 6 };
+        var conn = new DataConnection("opc", "OpcUa", site.Id) { Id = 61 };
+        var instances = Enumerable.Range(0, 3).Select(i =>
+        {
+            var inst = new Instance($"Pump{i}") { Id = 600 + i, SiteId = site.Id, TemplateId = 0 };
+            inst.ConnectionBindings.Add(new InstanceConnectionBinding("flow") { InstanceId = inst.Id, DataConnectionId = conn.Id });
+            return inst;
+        }).ToArray();
+        StubSite(site);
+        StubSiteConnections(site.Id, conn);
+        StubSiteInstances(site.Id, instances);
+        StubAllSharedScripts();
+        StubAllExternalSystems();
+        StubAllFolders();
+
+        var result = await Sut().ResolveAsync(SelectSites(new[] { site.Id }), CancellationToken.None);
+
+        Assert.Equal(3, result.Instances.Count);
+        Assert.Single(result.DataConnections);
+        await _sites.Received(1).GetDataConnectionsBySiteIdAsync(site.Id, Arg.Any<CancellationToken>());
+    }
+
+    /// <summary>
+    /// Verifies that the modules a template composes are loaded in one batch rather
+    /// than one by-id load each.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Resolve_batches_composed_template_loads()
+    {
+        var root = new Template("Root") { Id = 1 };
+        var modules = Enumerable.Range(0, 3).Select(i => new Template($"Module{i}") { Id = 10 + i }).ToArray();
+        foreach (var m in modules)
+        {
+            root.Compositions.Add(new TemplateComposition($"slot{m.Id}") { TemplateId = root.Id, ComposedTemplateId = m.Id });
+        }
+        StubTemplate(root);
+        foreach (var m in modules) StubTemplate(m);
+        StubAllSharedScripts();
+        StubAllExternalSystems();
+        StubAllFolders();
+
+        var result = await Sut().ResolveAsync(SelectTemplates(root.Id), CancellationToken.None);
+
+        Assert.Equal(4, result.Templates.Count);
+        foreach (var m in modules)
+        {
+            await _templates.DidNotReceive().GetTemplateWithChildrenAsync(m.Id, Arg.Any<CancellationToken>());
+        }
+        await _templates.Received(1).GetTemplatesWithChildrenAsync(Arg.Any<IEnumerable<string>>(), Arg.Any<CancellationToken>());
+    }
 }