From 92e713f1a178390bc6e69eb7e28b59b5dbd39f5f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 10 Jul 2026 02:22:44 -0400 Subject: [PATCH] =?UTF-8?q?fix(transport):=20template=20diffs=20resolve=20?= =?UTF-8?q?folder/base/composition=20names=20=E2=80=94=20unchanged=20struc?= =?UTF-8?q?tured=20templates=20classify=20Identical=20again?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 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 --- .../Import/ArtifactDiff.cs | 43 +++++++++++------ .../Import/BundleImporter.cs | 9 +++- .../Import/BundleImporterPreviewTests.cs | 45 ++++++++++++++++++ .../Import/ArtifactDiffTests.cs | 46 +++++++++++++++++++ 4 files changed, 128 insertions(+), 15 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs index 4e08dd27..b8aa9266 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs @@ -50,8 +50,19 @@ public sealed class ArtifactDiff /// /// The incoming template from the bundle. /// The existing template in the database, or null if new. + /// Optional folder-id→name map so an existing template's + /// FolderId resolves to the same name the bundle carries; without it, folder + /// identity falls back to a <id:N> placeholder that never matches a bundle + /// name (spurious Modified on every re-import — #05-T23). + /// Optional template-id→name map resolving the existing + /// template's ParentTemplateId (base) and each composition's ComposedTemplateId + /// to names, with the same placeholder fallback semantics. /// An import preview item describing the conflict type and differences. - public ImportPreviewItem CompareTemplate(TemplateDto incoming, Template? existing) + public ImportPreviewItem CompareTemplate( + TemplateDto incoming, + Template? existing, + IReadOnlyDictionary? folderNameById = null, + IReadOnlyDictionary? templateNameById = null) { ArgumentNullException.ThrowIfNull(incoming); if (existing is null) @@ -61,8 +72,8 @@ public sealed class ArtifactDiff var changes = new List(); AddIfDifferent(changes, "Description", existing.Description, incoming.Description); - AddIfDifferent(changes, "FolderName", FolderNameOf(existing), incoming.FolderName); - AddIfDifferent(changes, "BaseTemplateName", BaseTemplateNameOf(existing), incoming.BaseTemplateName); + AddIfDifferent(changes, "FolderName", FolderNameOf(existing, folderNameById), incoming.FolderName); + AddIfDifferent(changes, "BaseTemplateName", BaseTemplateNameOf(existing, templateNameById), incoming.BaseTemplateName); // 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 @@ -104,7 +115,7 @@ public sealed class ArtifactDiff incoming.Compositions, e => e.InstanceName, i => i.InstanceName, - (e, i) => CompositionTargetNameOf(e) == i.ComposedTemplateName, + (e, i) => CompositionTargetNameOf(e, templateNameById) == i.ComposedTemplateName, "Compositions", changes); @@ -665,24 +676,28 @@ public sealed class ArtifactDiff && e.ParameterDefinitions == i.ParameterDefinitions && e.ReturnDefinition == i.ReturnDefinition; - private static string? FolderNameOf(Template t) + private static string? FolderNameOf(Template t, IReadOnlyDictionary? folderNameById) { // 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 - // CompareTemplate scope, so fall back to "" when only the id is - // known. The PreviewAsync caller can pass a hydrated Template (via - // GetTemplateWithChildrenAsync) and the folder name typically isn't on - // it — this branch is a deliberate best-effort. - return t.FolderId is null ? null : $""; + // it to a name. When PreviewAsync supplies a folder-id→name map (#05-T23) + // the id resolves to the real name so an unchanged folder assignment reads + // Identical; without the map (or on a miss) we fall back to "", which + // keeps the older unit-test callers valid but never matches a bundle name. + if (t.FolderId is null) return null; + if (folderNameById != null && folderNameById.TryGetValue(t.FolderId.Value, out var name)) return name; + return $""; } - private static string? BaseTemplateNameOf(Template t) + private static string? BaseTemplateNameOf(Template t, IReadOnlyDictionary? templateNameById) { - return t.ParentTemplateId is null ? null : $""; + if (t.ParentTemplateId is null) return null; + if (templateNameById != null && templateNameById.TryGetValue(t.ParentTemplateId.Value, out var name)) return name; + return $""; } - private static string CompositionTargetNameOf(TemplateComposition comp) + private static string CompositionTargetNameOf(TemplateComposition comp, IReadOnlyDictionary? templateNameById) { + if (templateNameById != null && templateNameById.TryGetValue(comp.ComposedTemplateId, out var name)) return name; return $""; } diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index bf134164..ab70ad58 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -392,10 +392,17 @@ public sealed class BundleImporter : IBundleImporter .ConfigureAwait(false); var hydratedByName = hydratedTemplates .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 "" 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) { hydratedByName.TryGetValue(tDto.Name, out var existing); - items.Add(_diff.CompareTemplate(tDto, existing)); + items.Add(_diff.CompareTemplate(tDto, existing, folderNameById, templateNameById)); } // ---- SharedScripts ---- diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs index 0630be71..01960ed4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterPreviewTests.cs @@ -330,6 +330,51 @@ public sealed class BundleImporterPreviewTests : IDisposable 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 "" 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(); + 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(); + 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] public async Task PreviewAsync_emits_Warning_when_template_script_dependency_missing() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs index 5fa731a3..8a5306a8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs @@ -150,6 +150,52 @@ public sealed class ArtifactDiffTests 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 "" + // 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 { [5] = "Machines" }; + var templateNameById = new Dictionary { [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 { [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 ============ [Fact]