fix(transport): template diffs resolve folder/base/composition names — unchanged structured templates classify Identical again
#05-T23. ArtifactDiff.CompareTemplate gains optional folderNameById / templateNameById maps; FolderNameOf / BaseTemplateNameOf / CompositionTargetNameOf resolve the existing template's FolderId / ParentTemplateId / composition ComposedTemplateId to real names (falling back to the <id:N> placeholder only when a map is absent or misses, keeping existing unit-test callers valid). PreviewAsync builds templateNameById from GetAllTemplatesAsync (parent/composition targets can be any template, not just bundle-named ones) and threads both maps through. Fixes spurious Modified on every re-import of a foldered/derived/composed template. End-to-end preview test + two ArtifactDiff unit tests. Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
This commit is contained in:
@@ -50,8 +50,19 @@ public sealed class ArtifactDiff
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
/// <param name="incoming">The incoming template from the bundle.</param>
|
/// <param name="incoming">The incoming template from the bundle.</param>
|
||||||
/// <param name="existing">The existing template in the database, or null if new.</param>
|
/// <param name="existing">The existing template in the database, or null if new.</param>
|
||||||
|
/// <param name="folderNameById">Optional folder-id→name map so an existing template's
|
||||||
|
/// <c>FolderId</c> resolves to the same name the bundle carries; without it, folder
|
||||||
|
/// identity falls back to a <c><id:N></c> placeholder that never matches a bundle
|
||||||
|
/// name (spurious Modified on every re-import — #05-T23).</param>
|
||||||
|
/// <param name="templateNameById">Optional template-id→name map resolving the existing
|
||||||
|
/// template's <c>ParentTemplateId</c> (base) and each composition's <c>ComposedTemplateId</c>
|
||||||
|
/// to names, with the same placeholder fallback semantics.</param>
|
||||||
/// <returns>An import preview item describing the conflict type and differences.</returns>
|
/// <returns>An import preview item describing the conflict type and differences.</returns>
|
||||||
public ImportPreviewItem CompareTemplate(TemplateDto incoming, Template? existing)
|
public ImportPreviewItem CompareTemplate(
|
||||||
|
TemplateDto incoming,
|
||||||
|
Template? existing,
|
||||||
|
IReadOnlyDictionary<int, string>? folderNameById = null,
|
||||||
|
IReadOnlyDictionary<int, string>? templateNameById = null)
|
||||||
{
|
{
|
||||||
ArgumentNullException.ThrowIfNull(incoming);
|
ArgumentNullException.ThrowIfNull(incoming);
|
||||||
if (existing is null)
|
if (existing is null)
|
||||||
@@ -61,8 +72,8 @@ public sealed class ArtifactDiff
|
|||||||
|
|
||||||
var changes = new List<FieldChange>();
|
var changes = new List<FieldChange>();
|
||||||
AddIfDifferent(changes, "Description", existing.Description, incoming.Description);
|
AddIfDifferent(changes, "Description", existing.Description, incoming.Description);
|
||||||
AddIfDifferent(changes, "FolderName", FolderNameOf(existing), incoming.FolderName);
|
AddIfDifferent(changes, "FolderName", FolderNameOf(existing, folderNameById), incoming.FolderName);
|
||||||
AddIfDifferent(changes, "BaseTemplateName", BaseTemplateNameOf(existing), incoming.BaseTemplateName);
|
AddIfDifferent(changes, "BaseTemplateName", BaseTemplateNameOf(existing, templateNameById), incoming.BaseTemplateName);
|
||||||
|
|
||||||
// Bundles reference the alarm on-trigger script by name; resolve the
|
// Bundles reference the alarm on-trigger script by name; resolve the
|
||||||
// persisted FK back to a name over this template's own scripts so the
|
// persisted FK back to a name over this template's own scripts so the
|
||||||
@@ -104,7 +115,7 @@ public sealed class ArtifactDiff
|
|||||||
incoming.Compositions,
|
incoming.Compositions,
|
||||||
e => e.InstanceName,
|
e => e.InstanceName,
|
||||||
i => i.InstanceName,
|
i => i.InstanceName,
|
||||||
(e, i) => CompositionTargetNameOf(e) == i.ComposedTemplateName,
|
(e, i) => CompositionTargetNameOf(e, templateNameById) == i.ComposedTemplateName,
|
||||||
"Compositions",
|
"Compositions",
|
||||||
changes);
|
changes);
|
||||||
|
|
||||||
@@ -665,24 +676,28 @@ public sealed class ArtifactDiff
|
|||||||
&& e.ParameterDefinitions == i.ParameterDefinitions
|
&& e.ParameterDefinitions == i.ParameterDefinitions
|
||||||
&& e.ReturnDefinition == i.ReturnDefinition;
|
&& e.ReturnDefinition == i.ReturnDefinition;
|
||||||
|
|
||||||
private static string? FolderNameOf(Template t)
|
private static string? FolderNameOf(Template t, IReadOnlyDictionary<int, string>? folderNameById)
|
||||||
{
|
{
|
||||||
// Templates carry only a FK to the folder; the EntitySerializer projects
|
// Templates carry only a FK to the folder; the EntitySerializer projects
|
||||||
// it to a name. The diff doesn't have access to a folder lookup at
|
// it to a name. When PreviewAsync supplies a folder-id→name map (#05-T23)
|
||||||
// CompareTemplate scope, so fall back to "<id:N>" when only the id is
|
// the id resolves to the real name so an unchanged folder assignment reads
|
||||||
// known. The PreviewAsync caller can pass a hydrated Template (via
|
// Identical; without the map (or on a miss) we fall back to "<id:N>", which
|
||||||
// GetTemplateWithChildrenAsync) and the folder name typically isn't on
|
// keeps the older unit-test callers valid but never matches a bundle name.
|
||||||
// it — this branch is a deliberate best-effort.
|
if (t.FolderId is null) return null;
|
||||||
return t.FolderId is null ? null : $"<id:{t.FolderId}>";
|
if (folderNameById != null && folderNameById.TryGetValue(t.FolderId.Value, out var name)) return name;
|
||||||
|
return $"<id:{t.FolderId}>";
|
||||||
}
|
}
|
||||||
|
|
||||||
private static string? BaseTemplateNameOf(Template t)
|
private static string? BaseTemplateNameOf(Template t, IReadOnlyDictionary<int, string>? templateNameById)
|
||||||
{
|
{
|
||||||
return t.ParentTemplateId is null ? null : $"<id:{t.ParentTemplateId}>";
|
if (t.ParentTemplateId is null) return null;
|
||||||
|
if (templateNameById != null && templateNameById.TryGetValue(t.ParentTemplateId.Value, out var name)) return name;
|
||||||
|
return $"<id:{t.ParentTemplateId}>";
|
||||||
}
|
}
|
||||||
|
|
||||||
private static string CompositionTargetNameOf(TemplateComposition comp)
|
private static string CompositionTargetNameOf(TemplateComposition comp, IReadOnlyDictionary<int, string>? templateNameById)
|
||||||
{
|
{
|
||||||
|
if (templateNameById != null && templateNameById.TryGetValue(comp.ComposedTemplateId, out var name)) return name;
|
||||||
return $"<id:{comp.ComposedTemplateId}>";
|
return $"<id:{comp.ComposedTemplateId}>";
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -392,10 +392,17 @@ public sealed class BundleImporter : IBundleImporter
|
|||||||
.ConfigureAwait(false);
|
.ConfigureAwait(false);
|
||||||
var hydratedByName = hydratedTemplates
|
var hydratedByName = hydratedTemplates
|
||||||
.ToDictionary(t => t.Name, t => t, StringComparer.Ordinal);
|
.ToDictionary(t => t.Name, t => t, StringComparer.Ordinal);
|
||||||
|
// #05-T23: a template's ParentTemplateId (base) and each composition's
|
||||||
|
// ComposedTemplateId point at ANY template in the DB (not just the ones
|
||||||
|
// whose names the bundle carries), so resolve names through the full
|
||||||
|
// template set. Without this map the diff compares "<id:N>" placeholders
|
||||||
|
// against real bundle names and reports spurious Modified.
|
||||||
|
var allTemplateStubs = await _templateRepo.GetAllTemplatesAsync(ct).ConfigureAwait(false);
|
||||||
|
var templateNameById = allTemplateStubs.ToDictionary(t => t.Id, t => t.Name);
|
||||||
foreach (var tDto in content.Templates)
|
foreach (var tDto in content.Templates)
|
||||||
{
|
{
|
||||||
hydratedByName.TryGetValue(tDto.Name, out var existing);
|
hydratedByName.TryGetValue(tDto.Name, out var existing);
|
||||||
items.Add(_diff.CompareTemplate(tDto, existing));
|
items.Add(_diff.CompareTemplate(tDto, existing, folderNameById, templateNameById));
|
||||||
}
|
}
|
||||||
|
|
||||||
// ---- SharedScripts ----
|
// ---- SharedScripts ----
|
||||||
|
|||||||
+45
@@ -330,6 +330,51 @@ public sealed class BundleImporterPreviewTests : IDisposable
|
|||||||
Assert.Null(pumpItem.FieldDiffJson);
|
Assert.Null(pumpItem.FieldDiffJson);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task PreviewAsync_unchanged_structured_template_classifies_Identical()
|
||||||
|
{
|
||||||
|
// #05-T23 end-to-end: a template with a base (ParentTemplateId) and a
|
||||||
|
// composition, re-imported against an UNCHANGED target, must classify
|
||||||
|
// Identical. Before T23, PreviewAsync compared "<id:N>" placeholders for
|
||||||
|
// BaseTemplateName / composition target against the bundle's real names,
|
||||||
|
// so it always read Modified — defeating the wizard's auto-skip contract.
|
||||||
|
await using (var scope = _provider.CreateAsyncScope())
|
||||||
|
{
|
||||||
|
var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
|
||||||
|
ctx.Templates.Add(new Template("BasePump") { Description = "base" });
|
||||||
|
ctx.Templates.Add(new Template("FeatureX") { Description = "feature" });
|
||||||
|
await ctx.SaveChangesAsync();
|
||||||
|
var basePump = await ctx.Templates.SingleAsync(t => t.Name == "BasePump");
|
||||||
|
var feature = await ctx.Templates.SingleAsync(t => t.Name == "FeatureX");
|
||||||
|
|
||||||
|
var pump = new Template("Pump") { Description = "derived", ParentTemplateId = basePump.Id };
|
||||||
|
ctx.Templates.Add(pump);
|
||||||
|
await ctx.SaveChangesAsync();
|
||||||
|
pump.Compositions.Add(new TemplateComposition("mod1")
|
||||||
|
{
|
||||||
|
TemplateId = pump.Id,
|
||||||
|
ComposedTemplateId = feature.Id,
|
||||||
|
});
|
||||||
|
await ctx.SaveChangesAsync();
|
||||||
|
}
|
||||||
|
|
||||||
|
// Export everything; do NOT wipe — the target is unchanged.
|
||||||
|
var bundleStream = await ExportTemplatesAsync();
|
||||||
|
var bytes = await StreamToBytes(bundleStream);
|
||||||
|
|
||||||
|
ImportPreview preview;
|
||||||
|
await using (var scope = _provider.CreateAsyncScope())
|
||||||
|
{
|
||||||
|
var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
|
||||||
|
var session = await importer.LoadAsync(new MemoryStream(bytes), passphrase: null);
|
||||||
|
preview = await importer.PreviewAsync(session.SessionId);
|
||||||
|
}
|
||||||
|
|
||||||
|
var pumpItem = Assert.Single(preview.Items,
|
||||||
|
i => i.EntityType == "Template" && i.Name == "Pump");
|
||||||
|
Assert.Equal(ConflictKind.Identical, pumpItem.Kind);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task PreviewAsync_emits_Warning_when_template_script_dependency_missing()
|
public async Task PreviewAsync_emits_Warning_when_template_script_dependency_missing()
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -150,6 +150,52 @@ public sealed class ArtifactDiffTests
|
|||||||
Assert.Contains(("add", "new2"), hunks);
|
Assert.Contains(("add", "new2"), hunks);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ============ #05-T23: placeholder-identity resolution via name maps ============
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void CompareTemplate_MatchingFolderBaseAndComposition_IsIdentical()
|
||||||
|
{
|
||||||
|
// A structured template (FolderId, ParentTemplateId, one composition) that
|
||||||
|
// the incoming bundle matches by NAME must classify Identical. Before T23,
|
||||||
|
// FolderNameOf/BaseTemplateNameOf/CompositionTargetNameOf returned "<id:N>"
|
||||||
|
// placeholders that never equalled the incoming names, so every re-import
|
||||||
|
// of an unchanged structured template spuriously read Modified.
|
||||||
|
var existing = MakeTemplate("Pump");
|
||||||
|
existing.FolderId = 5;
|
||||||
|
existing.ParentTemplateId = 7;
|
||||||
|
existing.Compositions.Add(new TemplateComposition("Mod1") { ComposedTemplateId = 9 });
|
||||||
|
|
||||||
|
var folderNameById = new Dictionary<int, string> { [5] = "Machines" };
|
||||||
|
var templateNameById = new Dictionary<int, string> { [7] = "BasePump", [9] = "FeatureX" };
|
||||||
|
|
||||||
|
var incoming = MakeTemplateDto("Pump") with
|
||||||
|
{
|
||||||
|
FolderName = "Machines",
|
||||||
|
BaseTemplateName = "BasePump",
|
||||||
|
Compositions = new[] { new TemplateCompositionDto("Mod1", "FeatureX") },
|
||||||
|
};
|
||||||
|
|
||||||
|
var item = _diff.CompareTemplate(incoming, existing, folderNameById, templateNameById);
|
||||||
|
|
||||||
|
Assert.Equal(ConflictKind.Identical, item.Kind);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void CompareTemplate_DivergentBaseTemplateName_IsModified()
|
||||||
|
{
|
||||||
|
// Sanity: the maps must not mask a REAL change. Existing base = "BasePump",
|
||||||
|
// incoming names a different base → Modified.
|
||||||
|
var existing = MakeTemplate("Pump");
|
||||||
|
existing.ParentTemplateId = 7;
|
||||||
|
var templateNameById = new Dictionary<int, string> { [7] = "BasePump" };
|
||||||
|
|
||||||
|
var incoming = MakeTemplateDto("Pump") with { BaseTemplateName = "DifferentBase" };
|
||||||
|
|
||||||
|
var item = _diff.CompareTemplate(incoming, existing, folderNameById: null, templateNameById: templateNameById);
|
||||||
|
|
||||||
|
Assert.Equal(ConflictKind.Modified, item.Kind);
|
||||||
|
}
|
||||||
|
|
||||||
// ============ #05-T6: single-source child equality — diff sees the full field set ============
|
// ============ #05-T6: single-source child equality — diff sees the full field set ============
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
|
|||||||
Reference in New Issue
Block a user