fix(transport): reject bundle imports that would persist an inheritance/composition cycle

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
2026-07-09 15:40:14 -04:00
parent 854a6a8c0b
commit aae0a2db3e
2 changed files with 188 additions and 0 deletions
@@ -18,6 +18,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening;
 using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase;
+using ZB.MOM.WW.ScadaBridge.TemplateEngine;
 using ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation;
 using ZB.MOM.WW.ScadaBridge.Transport.Encryption;
 using ZB.MOM.WW.ScadaBridge.Transport.Serialization;
@@ -1044,6 +1045,19 @@ public sealed class BundleImporter : IBundleImporter
            await ResolveCompositionEdgesAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false);
            await ResolveInheritanceEdgesAsync(content.Templates, resolutionMap, user, ct).ConfigureAwait(false);

+            // ---- Import-time acyclicity guard over the merged template graph ----
+            // The two rewire passes above have STAGED (not yet saved) the bundle's
+            // inheritance (ParentTemplateId) and composition edges onto the tracked
+            // target templates. A bundle can still close a loop through a pre-existing
+            // target template — e.g. Overwrite a base so it now inherits from (or
+            // composes) one of its own descendants. The design-time save-gate enforces
+            // acyclicity on individual edits, but an import merges many edges at once,
+            // so re-check the whole merged graph here. On a cycle we throw and the
+            // catch below rolls the whole import back (ChangeTracker.Clear on the
+            // in-memory provider) BEFORE any of it is persisted; deliberately no
+            // SaveChanges precedes this check so nothing survives that rollback.
+            EnsureNoTemplateGraphCycles();
+
            // ---- Site/instance-scoped apply: instances LAST ----
            // The instance pass needs every FK target materialised first:
            //   • template id  — resolved by name against the just-flushed central
@@ -2178,6 +2192,60 @@ public sealed class BundleImporter : IBundleImporter
        }
    }

+    /// <summary>
+    /// Post-rewire acyclicity guard: reads the fully-merged template graph off the
+    /// change tracker (staged, not-yet-saved edges included) and rejects the import
+    /// if the bundle's inheritance or composition edges would persist a cycle —
+    /// e.g. an Overwrite that makes a base inherit from, or compose, its own
+    /// descendant. Delegates to the authoritative TemplateEngine
+    /// <see cref="CycleDetector"/>. Throws <see cref="SemanticValidationException"/>
+    /// on the first cycle found; the caller's catch rolls the whole import back.
+    /// </summary>
+    /// <remarks>
+    /// The template list is taken from the change tracker rather than a fresh query
+    /// so it reflects the staged edges without a premature flush. It is complete for
+    /// cycle detection: any edge that references a pre-existing target template forces
+    /// a full <c>GetAllTemplatesAsync</c> load in the rewire passes (tracking every
+    /// template), and any all-imported cycle consists solely of tracked (Local)
+    /// templates — so every node of any candidate cycle is present.
+    /// </remarks>
+    private void EnsureNoTemplateGraphCycles()
+    {
+        var templates = _dbContext.ChangeTracker.Entries<Template>()
+            .Where(e => e.State != EntityState.Deleted)
+            .Select(e => e.Entity)
+            .ToList();
+        if (templates.Count == 0) return;
+
+        foreach (var t in templates)
+        {
+            if (t.ParentTemplateId.HasValue)
+            {
+                var cycle = CycleDetector.DetectInheritanceCycle(t.Id, t.ParentTemplateId.Value, templates);
+                if (cycle is not null)
+                {
+                    throw new SemanticValidationException(new[]
+                    {
+                        $"Import would create a template inheritance cycle: {cycle}",
+                    });
+                }
+            }
+
+            foreach (var comp in t.Compositions)
+            {
+                if (_dbContext.Entry(comp).State == EntityState.Deleted) continue;
+                var cycle = CycleDetector.DetectCompositionCycle(t.Id, comp.ComposedTemplateId, templates);
+                if (cycle is not null)
+                {
+                    throw new SemanticValidationException(new[]
+                    {
+                        $"Import would create a template composition cycle: {cycle}",
+                    });
+                }
+            }
+        }
+    }
+
    private async Task ApplySharedScriptsAsync(
        IReadOnlyList<SharedScriptDto> dtos,
        Dictionary<(string, string), ImportResolution> map,
@@ -12,6 +12,7 @@ using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase;
 using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories;
 using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Services;
 using ZB.MOM.WW.ScadaBridge.Transport;
+using ZB.MOM.WW.ScadaBridge.Transport.Import;

 namespace ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests.Import;

@@ -80,6 +81,7 @@ public sealed class InheritanceImportTests : IDisposable
    {
        await using var scope = _provider.CreateAsyncScope();
        var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+        ctx.TemplateCompositions.RemoveRange(ctx.TemplateCompositions);
        ctx.Templates.RemoveRange(ctx.Templates);
        ctx.AuditLogEntries.RemoveRange(ctx.AuditLogEntries);
        await ctx.SaveChangesAsync();
@@ -201,4 +203,122 @@ public sealed class InheritanceImportTests : IDisposable
            Assert.Equal("WaterPump", row.EntityName);
        }
    }
+
+    /// <summary>Seed base "root" + derived "leaf" (leaf ParentTemplateId -> root).</summary>
+    private async Task SeedParentChildAsync(string rootName, string leafName)
+    {
+        await using var scope = _provider.CreateAsyncScope();
+        var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+        var root = new Template(rootName) { Description = "root" };
+        ctx.Templates.Add(root);
+        await ctx.SaveChangesAsync();
+        ctx.Templates.Add(new Template(leafName) { Description = "leaf", ParentTemplateId = root.Id });
+        await ctx.SaveChangesAsync();
+    }
+
+    [Fact]
+    public async Task Import_Overwrite_ThatClosesInheritanceCycle_Throws_AndRollsBack()
+    {
+        // Source: base "Alpha" (root) + derived "Beta" (parent Alpha), so the
+        // exported Beta DTO carries BaseTemplateName = "Alpha".
+        await SeedParentChildAsync("Alpha", "Beta");
+        var bundle = await ExportAllTemplatesAsync();
+        await WipeTemplatesAsync();
+
+        // Target: the MIRROR — "Beta" (root) + "Alpha" (parent Beta). Overwriting
+        // Beta so it inherits from Alpha closes Alpha -> Beta -> Alpha.
+        await SeedParentChildAsync("Beta", "Alpha");
+
+        var sessionId = await LoadAsync(bundle);
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            var resolutions = new List<ImportResolution>
+            {
+                new("Template", "Alpha", ResolutionAction.Skip, null),
+                new("Template", "Beta", ResolutionAction.Overwrite, null),
+            };
+            var ex = await Assert.ThrowsAsync<SemanticValidationException>(
+                () => importer.ApplyAsync(sessionId, resolutions, user: "bob"));
+            Assert.Contains("cycle", ex.Message, StringComparison.OrdinalIgnoreCase);
+        }
+
+        // Rolled back: target Beta's parent edge is untouched (still a root).
+        await using (var assert = _provider.CreateAsyncScope())
+        {
+            var ctx = assert.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var beta = await ctx.Templates.SingleAsync(t => t.Name == "Beta");
+            Assert.Null(beta.ParentTemplateId);
+        }
+    }
+
+    [Fact]
+    public async Task Import_Overwrite_ThatClosesCompositionCycle_Throws_AndRollsBack()
+    {
+        // Source: leaf "Gamma" + "Delta" that composes Gamma, so the exported
+        // Delta DTO carries a composition edge to "Gamma".
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.Templates.Add(new Template("Gamma") { Description = "leaf" });
+            await ctx.SaveChangesAsync();
+            var gamma = await ctx.Templates.SingleAsync(t => t.Name == "Gamma");
+            var delta = new Template("Delta") { Description = "composer" };
+            ctx.Templates.Add(delta);
+            await ctx.SaveChangesAsync();
+            delta.Compositions.Add(new TemplateComposition("g1")
+            {
+                TemplateId = delta.Id,
+                ComposedTemplateId = gamma.Id,
+            });
+            await ctx.SaveChangesAsync();
+        }
+
+        var bundle = await ExportAllTemplatesAsync();
+        await WipeTemplatesAsync();
+
+        // Target: the MIRROR — "Gamma" composes "Delta". Overwriting Delta so it
+        // composes Gamma closes the composition loop Gamma -> Delta -> Gamma.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.Templates.Add(new Template("Delta") { Description = "target-leaf" });
+            await ctx.SaveChangesAsync();
+            var delta = await ctx.Templates.SingleAsync(t => t.Name == "Delta");
+            var gamma = new Template("Gamma") { Description = "target-composer" };
+            ctx.Templates.Add(gamma);
+            await ctx.SaveChangesAsync();
+            gamma.Compositions.Add(new TemplateComposition("d1")
+            {
+                TemplateId = gamma.Id,
+                ComposedTemplateId = delta.Id,
+            });
+            await ctx.SaveChangesAsync();
+        }
+
+        var sessionId = await LoadAsync(bundle);
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            var resolutions = new List<ImportResolution>
+            {
+                new("Template", "Gamma", ResolutionAction.Skip, null),
+                new("Template", "Delta", ResolutionAction.Overwrite, null),
+            };
+            var ex = await Assert.ThrowsAsync<SemanticValidationException>(
+                () => importer.ApplyAsync(sessionId, resolutions, user: "bob"));
+            Assert.Contains("cycle", ex.Message, StringComparison.OrdinalIgnoreCase);
+        }
+
+        // Rolled back: target Delta still has NO composition rows (Overwrite did
+        // not persist the loop-closing edge).
+        await using (var assert = _provider.CreateAsyncScope())
+        {
+            var ctx = assert.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var delta = await ctx.Templates
+                .Include(t => t.Compositions)
+                .SingleAsync(t => t.Name == "Delta");
+            Assert.Empty(delta.Compositions);
+        }
+    }
 }