fix(transport): base-first template reference resolution on import (ScadaBridge#100 item 1)

TemplateLibrary.GetIdByNameAsync was first-row-wins, so a derived slot template could shadow a same-named base. Now: exact name, base first, derived-only name only when unique, otherwise unresolved (composition audit says ambiguous). Tracked-template indexes are base-first too. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-10 04:38:05 -05:00
parent 545d3e8ba0
commit a13aabc7c3
3 changed files with 135 additions and 10 deletions
@@ -305,7 +305,7 @@ Site identifiers and connection names are environment-specific, so a bundle expo

 **Template library reads (review 2026-10-21 04/P4 + 08/P1).** A preview and each apply attempt read the target template library **once**, through body-free projections (`GetTemplateStubsAsync` for id/name maps, `GetAllTemplatesForAnalysisAsync` — untracked, members and lock flags but no script bodies — where members or edges are needed), held on a per-session `TemplateLibrary` and threaded to every consumer: preview diff names, blocker and locked-override scans, apply-time validation, the composition/inheritance rewire fallbacks (formerly a full library reload *inside* the per-template loop), the instance pass's name→id map (overlaid with the change tracker's just-flushed templates), the acyclicity guard and the stale-instance expansion. The full tracked graph is loaded only for the templates the bundle **Overwrites** (`GetTemplatesWithChildrenAsync` by name), since their child syncs mutate it. The acyclicity guard reads the change tracker's staged templates overlaid on that snapshot — the untouched part of the graph has not changed during the apply. Instance lookups are batched by name set (`ITemplateEngineRepository.GetInstancesByUniqueNamesAsync`, one tracked + hydrated read for preview and one for the instance pass, Rename targets included), and the preview resolves an existing instance's template / site / area names from the library, one site list and one area list per site, and reads shared scripts and external systems once each.

-**Template second-pass rewires.** Bundles carry template-to-template references by **name**, not id (ids are environment-specific). After the templates are flushed, the importer runs three name-resolution passes over the imported templates so every cross-template FK points at the freshly-persisted target row: the **alarm on-trigger-script** links (`TemplateAlarm.OnTriggerScriptId`), the **composition** edges (`TemplateComposition`), and the **inheritance** edges (`Template.ParentTemplateId`, resolved from `BaseTemplateName` through the same rename/skip resolution map — a `null` base clears a stale edge on Overwrite). Each pass is best-effort: an unresolvable reference leaves the FK null and emits a warning audit row (`BundleImportAlarmScriptUnresolved` / `BundleImportCompositionUnresolved` / `BundleImportBaseTemplateUnresolved`) rather than aborting the import. Each pass indexes the change tracker's templates by name **once** (a `DbSet.Local` access runs a full `DetectChanges` over everything the import has staged, so the former per-template and per-edge scans made apply time quadratic in bundle size) and reads the children it rewires in **one** batched `GetTemplatesWithChildrenAsync`; the alarm pass loads only templates whose bundled alarms name an on-trigger script (Gitea #47 — measured on the in-memory provider, 800 composing, derived templates applied in 0.6 s instead of 26.5 s; `ImportApplyScalingTests` pins the cost as independent of bundle size).
+**Template second-pass rewires.** Bundles carry template-to-template references by **name**, not id (ids are environment-specific). After the templates are flushed, the importer runs three name-resolution passes over the imported templates so every cross-template FK points at the freshly-persisted target row: the **alarm on-trigger-script** links (`TemplateAlarm.OnTriggerScriptId`), the **composition** edges (`TemplateComposition`), and the **inheritance** edges (`Template.ParentTemplateId`, resolved from `BaseTemplateName` through the same rename/skip resolution map — a `null` base clears a stale edge on Overwrite). Each pass is best-effort: an unresolvable reference leaves the FK null and emits a warning audit row (`BundleImportAlarmScriptUnresolved` / `BundleImportCompositionUnresolved` / `BundleImportBaseTemplateUnresolved`) rather than aborting the import. **Reference rule against the target library (ScadaBridge#100 item 1, 2026-11-10):** a composed / base / instance template name that is not staged by this import resolves by exact name with the **base template first** (base names are unique through the filtered unique index — the same row an Overwrite targets); a name held only by derived composition-slot templates resolves only when exactly one holds it; several derived and no base is ambiguous and stays unresolved (the composition audit row says so) instead of first-row-wins binding to another composition's slot template. The tracked-template indexes are base-first likewise. Each pass indexes the change tracker's templates by name **once** (a `DbSet.Local` access runs a full `DetectChanges` over everything the import has staged, so the former per-template and per-edge scans made apply time quadratic in bundle size) and reads the children it rewires in **one** batched `GetTemplatesWithChildrenAsync`; the alarm pass loads only templates whose bundled alarms name an on-trigger script (Gitea #47 — measured on the in-memory provider, 800 composing, derived templates applied in 0.6 s instead of 26.5 s; `ImportApplyScalingTests` pins the cost as independent of bundle size).

 **Rename call-site limitation (load-bearing).** A `Rename` resolution renames the entity being imported (e.g. a SharedScript `Foo` → `Bar`), but it does **not** rewrite call sites inside importing scripts — a script body that calls `CallShared("Foo")` still says `Foo` after import. So the target DB can end up with scripts referencing the old name. This is deliberate: Pass 1 name resolution registers **both** the original and renamed names as resolvable (so the import isn't blocked), and the authoritative check is the target's own **deploy-time validation** (`ScriptCompiler.TryCompile` in the Template Engine deploy gate), which recompiles against the actual persisted catalog. Operators renaming a shared dependency must fix the calling scripts on the target (or re-export with the dependency included under its original name).

@@ -2402,6 +2402,29 @@ public sealed class BundleImporter : IBundleImporter
    internal static Dictionary<string, Template> ToTemplateLookup(IEnumerable<Template> rows) =>
        ToNameLookup(rows.OrderBy(t => t.IsDerived), t => t.Name);

+    /// <summary>The by-name reference rule of <c>TemplateLibrary.GetIdByNameAsync</c>
+    /// (ScadaBridge#100 item 1): exact name; base template first; a derived-only name resolves
+    /// only when unique; several derived and no base ⇒ omitted (ambiguous).</summary>
+    internal static Dictionary<string, int> ResolveTemplateReferenceIds(
+        IEnumerable<(int Id, string Name, bool IsDerived)> pairs)
+    {
+        var result = new Dictionary<string, int>(StringComparer.Ordinal);
+        foreach (var group in pairs.GroupBy(p => p.Name, StringComparer.Ordinal))
+        {
+            var bases = group.Where(p => !p.IsDerived).ToList();
+            if (bases.Count > 0)
+            {
+                result[group.Key] = bases.Min(p => p.Id);
+                continue;
+            }
+
+            var derived = group.ToList();
+            if (derived.Count == 1)
+                result[group.Key] = derived[0].Id;
+        }
+        return result;
+    }
+
    private async Task ApplyTemplatesAsync(
        IReadOnlyList<TemplateDto> dtos,
        Dictionary<(string, string), ImportResolution> map,
@@ -3367,7 +3390,10 @@ public sealed class BundleImporter : IBundleImporter
                            OwnerTemplateName = parent.Name,
                            compDto.InstanceName,
                            UnresolvedComposedTemplateName = compDto.ComposedTemplateName,
-                            Reason = "Composed template name not present in bundle or target.",
+                            Reason = (await library.GetAmbiguousNamesAsync(ct).ConfigureAwait(false))
+                                    .Contains(compDto.ComposedTemplateName)
+                                ? "Composed template name is held by several derived (composition-slot) templates in the target and by no base template; refusing to guess (ScadaBridge#100)."
+                                : "Composed template name not present in bundle or target.",
                        },
                        ct).ConfigureAwait(false);
                    continue;
@@ -3492,7 +3518,9 @@ public sealed class BundleImporter : IBundleImporter

        public LocalTemplateIndex(IReadOnlyList<Template> tracked)
        {
-            foreach (var template in tracked)
+            // Base first (ScadaBridge#100 item 1): a tracked derived slot template must not
+            // shadow a base template of the same name for a composed/base reference.
+            foreach (var template in tracked.OrderBy(t => t.IsDerived))
            {
                _exact.TryAdd(template.Name, template);
            }
@@ -3633,19 +3661,43 @@ public sealed class BundleImporter : IBundleImporter
            return _analysis;
        }

-        /// <summary>Target template name → id (first wins on a duplicate name).</summary>
+        /// <summary>
+        /// Target template name → id, used to resolve a bundle's by-name references (composed,
+        /// base and instance template names) against templates this import did not stage.
+        /// <para>
+        /// ScadaBridge#100 item 1 — the rule, keyed by exact name: a <b>base</b> template wins
+        /// (base names are unique through the filtered unique index, and the base is the row a
+        /// by-name reference means — the same rule <see cref="ToTemplateLookup"/> applies to
+        /// Overwrite targets); a name held only by derived (composition-slot) templates resolves
+        /// only when exactly ONE derived template holds it; a name held by several derived
+        /// templates and no base is <b>ambiguous</b> and is left out, so the caller records it
+        /// unresolved rather than binding to an arbitrary slot owned by another composition.
+        /// It used to be first-row-wins over the projection order, so a derived slot template
+        /// with a lower id silently shadowed a base template of the same name.
+        /// </para>
+        /// </summary>
        public async Task<IReadOnlyDictionary<string, int>> GetIdByNameAsync(CancellationToken ct)
        {
            if (_idByName is null)
            {
                var pairs = await GetPairsAsync(ct).ConfigureAwait(false);
-                _idByName = pairs
-                    .GroupBy(p => p.Name, StringComparer.Ordinal)
-                    .ToDictionary(g => g.Key, g => g.First().Id, StringComparer.Ordinal);
+                _idByName = ResolveTemplateReferenceIds(pairs);
            }
            return _idByName;
        }

+        /// <summary>Target names held only by two or more derived templates (no base) —
+        /// the references <see cref="GetIdByNameAsync"/> refuses to guess.</summary>
+        public async Task<IReadOnlySet<string>> GetAmbiguousNamesAsync(CancellationToken ct)
+        {
+            var pairs = await GetPairsAsync(ct).ConfigureAwait(false);
+            return pairs
+                .GroupBy(p => p.Name, StringComparer.Ordinal)
+                .Where(g => g.All(p => p.IsDerived) && g.Count() > 1)
+                .Select(g => g.Key)
+                .ToHashSet(StringComparer.Ordinal);
+        }
+
        /// <summary>Target template id → name.</summary>
        public async Task<IReadOnlyDictionary<int, string>> GetNameByIdAsync(CancellationToken ct)
        {
@@ -3659,14 +3711,14 @@ public sealed class BundleImporter : IBundleImporter

        // Id/name pairs from whichever projection is already in hand: the analysis
        // load when a caller has made it, else the lighter stub projection.
-        private async Task<IReadOnlyList<(int Id, string Name)>> GetPairsAsync(CancellationToken ct)
+        private async Task<IReadOnlyList<(int Id, string Name, bool IsDerived)>> GetPairsAsync(CancellationToken ct)
        {
            if (_analysis is not null)
            {
-                return _analysis.Select(t => (t.Id, t.Name)).ToList();
+                return _analysis.Select(t => (t.Id, t.Name, t.IsDerived)).ToList();
            }
            _stubs ??= await _repo.GetTemplateStubsAsync(ct).ConfigureAwait(false);
-            return _stubs.Select(t => (t.Id, t.Name)).ToList();
+            return _stubs.Select(t => (t.Id, t.Name, t.IsDerived)).ToList();
        }
    }

@@ -4844,6 +4896,9 @@ public sealed class BundleImporter : IBundleImporter
            await library.GetIdByNameAsync(ct).ConfigureAwait(false), StringComparer.Ordinal);
        foreach (var local in _dbContext.Templates.Local)
        {
+            // A tracked derived slot template never displaces a base of the same name
+            // (ScadaBridge#100 item 1 — instances are of base templates).
+            if (local.IsDerived && templateIdByName.ContainsKey(local.Name)) continue;
            templateIdByName[local.Name] = local.Id;
        }

@@ -0,0 +1,70 @@
+using ZB.MOM.WW.ScadaBridge.Transport.Import;
+
+namespace ZB.MOM.WW.ScadaBridge.Transport.Tests.Import;
+
+/// <summary>
+/// ScadaBridge#100 item 1: how a bundle's by-name template reference (composed, base or
+/// instance template name) resolves against target templates the import did not stage.
+/// Derived (composition-slot) templates carry their slot name and sit outside the filtered
+/// unique index, so a name can be held by a base template and any number of derived ones.
+/// </summary>
+public class TemplateReferenceResolutionTests
+{
+    /// <summary>A base template wins even when a same-named derived slot template has a lower id
+    /// (the old first-row-wins rule bound the reference to the derived row).</summary>
+    [Fact]
+    public void Base_template_wins_over_a_lower_id_derived_template_of_the_same_name()
+    {
+        var ids = BundleImporter.ResolveTemplateReferenceIds(new[]
+        {
+            (Id: 3, Name: "Motor", IsDerived: true),
+            (Id: 7, Name: "Motor", IsDerived: false),
+            (Id: 9, Name: "Motor", IsDerived: true),
+        });
+
+        Assert.Equal(7, ids["Motor"]);
+    }
+
+    /// <summary>A name held by exactly one derived template and no base still resolves to it —
+    /// composition targets may legitimately be derived templates.</summary>
+    [Fact]
+    public void A_unique_derived_only_name_resolves()
+    {
+        var ids = BundleImporter.ResolveTemplateReferenceIds(new[]
+        {
+            (Id: 4, Name: "Slot1", IsDerived: true),
+            (Id: 5, Name: "Pump", IsDerived: false),
+        });
+
+        Assert.Equal(4, ids["Slot1"]);
+        Assert.Equal(5, ids["Pump"]);
+    }
+
+    /// <summary>Several derived templates sharing a name with no base is ambiguous: the name is
+    /// left unresolved rather than bound to an arbitrary slot owned by another composition.</summary>
+    [Fact]
+    public void Several_derived_templates_and_no_base_is_ambiguous_and_unresolved()
+    {
+        var ids = BundleImporter.ResolveTemplateReferenceIds(new[]
+        {
+            (Id: 2, Name: "Motor", IsDerived: true),
+            (Id: 6, Name: "Motor", IsDerived: true),
+        });
+
+        Assert.False(ids.ContainsKey("Motor"));
+    }
+
+    /// <summary>References stay exact-name: a case variant is a different key (case-insensitive
+    /// matching of references is outside this rule).</summary>
+    [Fact]
+    public void Matching_is_exact_name()
+    {
+        var ids = BundleImporter.ResolveTemplateReferenceIds(new[]
+        {
+            (Id: 1, Name: "Pump", IsDerived: false),
+        });
+
+        Assert.False(ids.ContainsKey("pump"));
+        Assert.Equal(1, ids["Pump"]);
+    }
+}