fix(templateengine+centralui): resolve follow-ups #3 (derived-template collisions) and #7 (sandbox batch/wait surface)
#3 — CollisionDetector counted a derived template's IsInherited placeholder rows as a distinct origin from the parent members the inheritance walk re-adds, reporting a spurious "Naming collision" for every inherited row and blocking any attribute/composition add to a derived template. CollectDirectMembers now skips IsInherited rows on the direct-template and inherited-parent walks; it keeps them for the composed-module walk, where placeholders are the sole representation of a derived module's inherited members (that walk does not climb the composed template's parent chain). #7 — SandboxAttributeAccessor (Central UI Test-Run host) omitted WriteBatchAndWaitAsync / WaitAsync / WaitForAsync, so the editor false-flagged valid instance scripts with CS1061 even though `template validate` and the deploy gate accept them. Added the five overloads mirroring the runtime AttributeAccessor; they throw a labelled ScriptSandboxException if run in Test Run (the central sandbox has no device-batch / event-waiter transport). Tests: +3 CollisionDetector unit + 1 end-to-end TemplateService (derived add now succeeds); +2 ScriptAnalysisService diagnose-clean. Each new test verified to fail without its fix with the exact user-facing symptom. Full suites green (TemplateEngine.Tests 438, CentralUI.Tests 866). Docs: Component-TemplateEngine.md (inherited-placeholder collision rule), Component-ScriptAnalysis.md (third sandbox surface + its compile-clean guard), known-issues tracker #3/#7 marked resolved and the minor note promoted to #8.
This commit is contained in:
Joseph Doherty
+42
@@ -677,4 +677,46 @@ public class ScriptAnalysisServiceTests
|
||||
Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS"));
|
||||
Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA"));
|
||||
}
|
||||
|
||||
// ── Instance-script batch/wait surface (follow-up #7) ─────────────────
|
||||
|
||||
[Fact]
|
||||
public void InstanceScript_BatchAndWaitHelpers_DiagnoseClean()
|
||||
{
|
||||
// Follow-up #7: SandboxAttributeAccessor was missing WriteBatchAndWaitAsync /
|
||||
// WaitAsync / WaitForAsync, so the editor false-flagged valid instance scripts
|
||||
// with CS1061 even though `template validate` and the deploy gate accept them.
|
||||
// The sandbox surface must mirror the runtime AttributeAccessor member-for-member.
|
||||
var code =
|
||||
"var data = new System.Collections.Generic.Dictionary<string, object?> { { \"Cmd\", 1 } };\n" +
|
||||
"var done = await Attributes.WriteBatchAndWaitAsync(data, \"Flag\", true, \"Done\", true, System.TimeSpan.FromSeconds(5));\n" +
|
||||
"var hit = await Attributes.WaitAsync(\"Done\", true, System.TimeSpan.FromSeconds(5));\n" +
|
||||
"var pred = await Attributes.WaitAsync(\"Done\", v => v != null, System.TimeSpan.FromSeconds(5));\n" +
|
||||
"var res = await Attributes.WaitForAsync(\"Done\", true, System.TimeSpan.FromSeconds(5));\n" +
|
||||
"return done && hit && pred && res.Matched;";
|
||||
var resp = _svc.Diagnose(new DiagnoseRequest(code));
|
||||
|
||||
Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS"));
|
||||
Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ChildInstanceScript_WriteBatchAndWait_DiagnoseClean()
|
||||
{
|
||||
// The same helper on a child composition
|
||||
// (Children["X"].Attributes.WriteBatchAndWaitAsync) — the exact shape used by
|
||||
// the base MESReceiver MoveIn/MoveOut scripts — must also resolve cleanly
|
||||
// through the sandbox composition accessor.
|
||||
var code =
|
||||
"var data = new System.Collections.Generic.Dictionary<string, object?> { { \"MoveInType\", \"Run\" } };\n" +
|
||||
"var ok = await Children[\"RightMESReceiver\"].Attributes.WriteBatchAndWaitAsync(" +
|
||||
"data, \"MoveInFlag\", true, \"MoveInCompleteFlag\", true, System.TimeSpan.FromSeconds(25));\n" +
|
||||
"return ok;";
|
||||
var resp = _svc.Diagnose(new DiagnoseRequest(
|
||||
Code: code,
|
||||
Children: new[] { Comp("RightMESReceiver", attrs: new[] { Attr("MoveInType") }) }));
|
||||
|
||||
Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS"));
|
||||
Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -111,4 +111,81 @@ public class CollisionDetectorTests
|
||||
Assert.NotEmpty(collisions);
|
||||
Assert.Contains(collisions, c => c.Contains("modA.Temp"));
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
// Follow-up #3: inherited placeholder rows must not self-collide with the
|
||||
// parent rows the inheritance walk re-adds. Before the fix, every derived
|
||||
// template reported a collision for each IsInherited row (child origin vs
|
||||
// "parent 'X'" origin) — so there was no supported path to add an attribute
|
||||
// or composition to a derived template.
|
||||
// ========================================================================
|
||||
|
||||
[Fact]
|
||||
public void DetectCollisions_DerivedTemplateWithInheritedPlaceholders_NoFalseCollision()
|
||||
{
|
||||
// Base carries the real attributes.
|
||||
var parent = new Template("ReactorSide") { Id = 7 };
|
||||
parent.Attributes.Add(new TemplateAttribute("DeltaVac") { Id = 70, TemplateId = 7, DataType = DataType.Float });
|
||||
parent.Attributes.Add(new TemplateAttribute("ResultType") { Id = 71, TemplateId = 7, DataType = DataType.String });
|
||||
|
||||
// Derived child stores IsInherited placeholder copies of the base attrs
|
||||
// PLUS a newly-added direct attribute (the member an author is trying to add).
|
||||
var child = new Template("LeftReactorSide") { Id = 8, ParentTemplateId = 7 };
|
||||
child.Attributes.Add(new TemplateAttribute("DeltaVac") { Id = 80, TemplateId = 8, DataType = DataType.Float, IsInherited = true });
|
||||
child.Attributes.Add(new TemplateAttribute("ResultType") { Id = 81, TemplateId = 8, DataType = DataType.String, IsInherited = true });
|
||||
child.Attributes.Add(new TemplateAttribute("SideName") { Id = 82, TemplateId = 8, DataType = DataType.String });
|
||||
|
||||
var all = new List<Template> { parent, child };
|
||||
var collisions = CollisionDetector.DetectCollisions(child, all);
|
||||
|
||||
// The inherited placeholders are NOT genuine cross-origin duplicates.
|
||||
Assert.Empty(collisions);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void DetectCollisions_MultiLevelInheritedPlaceholders_NoFalseCollision()
|
||||
{
|
||||
// Grandparent → parent → child, with placeholder rows materialized at every
|
||||
// level. The inherited-parent walk must skip the parent's own placeholders
|
||||
// too, or the grandparent's real row collides with the parent's copy.
|
||||
var grandparent = new Template("Root") { Id = 1 };
|
||||
grandparent.Attributes.Add(new TemplateAttribute("Common") { Id = 10, TemplateId = 1, DataType = DataType.Float });
|
||||
|
||||
var parent = new Template("Mid") { Id = 2, ParentTemplateId = 1 };
|
||||
parent.Attributes.Add(new TemplateAttribute("Common") { Id = 20, TemplateId = 2, DataType = DataType.Float, IsInherited = true });
|
||||
|
||||
var child = new Template("Leaf") { Id = 3, ParentTemplateId = 2 };
|
||||
child.Attributes.Add(new TemplateAttribute("Common") { Id = 30, TemplateId = 3, DataType = DataType.Float, IsInherited = true });
|
||||
|
||||
var all = new List<Template> { grandparent, parent, child };
|
||||
var collisions = CollisionDetector.DetectCollisions(child, all);
|
||||
|
||||
Assert.Empty(collisions);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void DetectCollisions_DerivedTemplate_GenuineCollisionStillDetected_DespiteInheritedPlaceholder()
|
||||
{
|
||||
// Guard against over-suppression. The parent declares a real direct member
|
||||
// "dup.Shared"; the child both (a) carries an IsInherited placeholder copy of
|
||||
// it and (b) composes a module "dup" whose "Shared" member resolves to the
|
||||
// same canonical name "dup.Shared". That is a genuine cross-origin collision
|
||||
// (composed module vs inherited parent member) and must still be reported —
|
||||
// even though the child's own placeholder row is now skipped.
|
||||
var module = new Template("Module") { Id = 3 };
|
||||
module.Attributes.Add(new TemplateAttribute("Shared") { Id = 30, TemplateId = 3, DataType = DataType.Float });
|
||||
|
||||
var parent = new Template("Base") { Id = 1 };
|
||||
parent.Attributes.Add(new TemplateAttribute("dup.Shared") { Id = 10, TemplateId = 1, DataType = DataType.Float });
|
||||
|
||||
var child = new Template("Derived") { Id = 2, ParentTemplateId = 1 };
|
||||
child.Attributes.Add(new TemplateAttribute("dup.Shared") { Id = 20, TemplateId = 2, DataType = DataType.Float, IsInherited = true });
|
||||
child.Compositions.Add(new TemplateComposition("dup") { Id = 1, TemplateId = 2, ComposedTemplateId = 3 });
|
||||
|
||||
var all = new List<Template> { parent, child, module };
|
||||
var collisions = CollisionDetector.DetectCollisions(child, all);
|
||||
|
||||
Assert.NotEmpty(collisions);
|
||||
Assert.Contains(collisions, c => c.Contains("dup.Shared"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -220,6 +220,37 @@ public class TemplateServiceTests
|
||||
Assert.Equal(1, result.Value.TemplateId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task AddAttribute_ToDerivedTemplateWithInheritedPlaceholders_Succeeds()
|
||||
{
|
||||
// Follow-up #3 (end-to-end). Adding a member to a derived template used to
|
||||
// fail: CloneTemplateWithNewAttribute carries the child's IsInherited
|
||||
// placeholder rows into DetectCollisions, which counted each one twice (child
|
||||
// origin + the parent the inheritance walk re-adds) and reported a spurious
|
||||
// "Naming collision" for every inherited attribute — there was no supported
|
||||
// API path to extend a derived template. The new attribute must now save.
|
||||
var parent = new Template("ReactorSide") { Id = 7 };
|
||||
parent.Attributes.Add(new TemplateAttribute("DeltaVac") { Id = 70, TemplateId = 7, DataType = DataType.Float });
|
||||
parent.Attributes.Add(new TemplateAttribute("ResultType") { Id = 71, TemplateId = 7, DataType = DataType.String });
|
||||
|
||||
var child = new Template("LeftReactorSide") { Id = 8, ParentTemplateId = 7 };
|
||||
child.Attributes.Add(new TemplateAttribute("DeltaVac") { Id = 80, TemplateId = 8, DataType = DataType.Float, IsInherited = true });
|
||||
child.Attributes.Add(new TemplateAttribute("ResultType") { Id = 81, TemplateId = 8, DataType = DataType.String, IsInherited = true });
|
||||
|
||||
_repoMock.Setup(r => r.GetTemplateByIdAsync(8, It.IsAny<CancellationToken>())).ReturnsAsync(child);
|
||||
_repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny<CancellationToken>()))
|
||||
.ReturnsAsync(new List<Template> { parent, child });
|
||||
|
||||
var attr = new TemplateAttribute("MoveInType") { DataType = DataType.String, Value = "" };
|
||||
var result = await _service.AddAttributeAsync(8, attr, "admin");
|
||||
|
||||
// Guard the message: Result.Error throws on a success result, so only read
|
||||
// it when the add actually failed (which is the case this test regresses).
|
||||
Assert.True(result.IsSuccess, result.IsFailure ? result.Error : null);
|
||||
Assert.Equal("MoveInType", result.Value.Name);
|
||||
Assert.Equal(8, result.Value.TemplateId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task AddAttribute_DuplicateName_Fails()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user