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
This commit is contained in:
@@ -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 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).
|
**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) =>
|
internal static Dictionary<string, Template> ToTemplateLookup(IEnumerable<Template> rows) =>
|
||||||
ToNameLookup(rows.OrderBy(t => t.IsDerived), t => t.Name);
|
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(
|
private async Task ApplyTemplatesAsync(
|
||||||
IReadOnlyList<TemplateDto> dtos,
|
IReadOnlyList<TemplateDto> dtos,
|
||||||
Dictionary<(string, string), ImportResolution> map,
|
Dictionary<(string, string), ImportResolution> map,
|
||||||
@@ -3367,7 +3390,10 @@ public sealed class BundleImporter : IBundleImporter
|
|||||||
OwnerTemplateName = parent.Name,
|
OwnerTemplateName = parent.Name,
|
||||||
compDto.InstanceName,
|
compDto.InstanceName,
|
||||||
UnresolvedComposedTemplateName = compDto.ComposedTemplateName,
|
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);
|
ct).ConfigureAwait(false);
|
||||||
continue;
|
continue;
|
||||||
@@ -3492,7 +3518,9 @@ public sealed class BundleImporter : IBundleImporter
|
|||||||
|
|
||||||
public LocalTemplateIndex(IReadOnlyList<Template> tracked)
|
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);
|
_exact.TryAdd(template.Name, template);
|
||||||
}
|
}
|
||||||
@@ -3633,19 +3661,43 @@ public sealed class BundleImporter : IBundleImporter
|
|||||||
return _analysis;
|
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)
|
public async Task<IReadOnlyDictionary<string, int>> GetIdByNameAsync(CancellationToken ct)
|
||||||
{
|
{
|
||||||
if (_idByName is null)
|
if (_idByName is null)
|
||||||
{
|
{
|
||||||
var pairs = await GetPairsAsync(ct).ConfigureAwait(false);
|
var pairs = await GetPairsAsync(ct).ConfigureAwait(false);
|
||||||
_idByName = pairs
|
_idByName = ResolveTemplateReferenceIds(pairs);
|
||||||
.GroupBy(p => p.Name, StringComparer.Ordinal)
|
|
||||||
.ToDictionary(g => g.Key, g => g.First().Id, StringComparer.Ordinal);
|
|
||||||
}
|
}
|
||||||
return _idByName;
|
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>
|
/// <summary>Target template id → name.</summary>
|
||||||
public async Task<IReadOnlyDictionary<int, string>> GetNameByIdAsync(CancellationToken ct)
|
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
|
// Id/name pairs from whichever projection is already in hand: the analysis
|
||||||
// load when a caller has made it, else the lighter stub projection.
|
// 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)
|
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);
|
_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);
|
await library.GetIdByNameAsync(ct).ConfigureAwait(false), StringComparer.Ordinal);
|
||||||
foreach (var local in _dbContext.Templates.Local)
|
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;
|
templateIdByName[local.Name] = local.Id;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+70
@@ -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"]);
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user