scadalink-design/code-reviews/TemplateEngine/findings.md

Code Review — TemplateEngine

Field	Value
Module	src/ScadaLink.TemplateEngine
Design doc	docs/requirements/Component-TemplateEngine.md
Status	Reviewed
Last reviewed	2026-05-16
Reviewer	claude-agent
Commit reviewed	9c60592
Open findings	14

Summary

The Template Engine is a pure central-side modeling library: stateless services over ITemplateEngineRepository plus four static helper classes (collision, cycle, lock, resolver). It has no Akka actors and no direct concurrency, so the Akka and thread-safety categories produce nothing of substance. The code is generally well-structured and the cascade-based composition model (derived templates owned by composition slots) is consistently applied. However the review surfaced several real correctness gaps. The most serious are in flattening: composed alarms and scripts nested below the first level are silently dropped, derived templates omit base alarms entirely (breaking per-slot alarm override), and the alarm-on-trigger-script resolution step is an empty placeholder so that whole validation rule is dead. Validation has two security-relevant weaknesses — the forbidden-API scan is a naive substring match and the brace-balance "compile" check mispredicts on verbatim / interpolated / raw string literals. Several documented behaviours (collision check on create, optimistic concurrency on instance state) are claimed but not implemented. Themes: validation that is weaker than the design promises, and asymmetric handling of attributes vs. alarms vs. scripts throughout the resolve/flatten/derive paths.

Checklist coverage

#	Category	Examined	Notes
1	Correctness & logic bugs	✓	Multiple real bugs: deep composed-member loss, derived alarms omitted, granularity bypass, no-op create-time collision block.
2	Akka.NET conventions	✓	No actors in this module (AddTemplateEngineActors is an empty placeholder). Nothing to assess.
3	Concurrency & thread safety	✓	Services are stateless, scoped per request; static helpers hold no mutable state. Design says template editing is last-write-wins; that is honoured. See TemplateEngine-010 re: a doc claim of optimistic concurrency that is not implemented.
4	Error handling & resilience	✓	Result<T> used consistently; repository nulls guarded. FlatteningService wraps in try/catch. No store-and-forward or failover surface in this module.
5	Security	✓	No auth checks in-module (delegated to callers per design). Script trust-model enforcement is weak — see TemplateEngine-006 and TemplateEngine-007.
6	Performance & resource management	✓	GetAllTemplatesAsync reloaded on most member edits; one genuine N+1 in TemplateDeletionService (TemplateEngine-009). No IDisposable leaks (JsonDocument/streams disposed).
7	Design-document adherence	✓	Drift found: recursive composition not fully implemented in flattening; DataType enum naming differs from doc; optimistic-concurrency claim.
8	Code organization & conventions	✓	POCO entities in Commons, repo interfaces in Commons, Options pattern N/A (no options here). Duplicate deletion logic (TemplateEngine-014).
9	Testing coverage	✓	Tests exist for every file, but the dead/placeholder paths (TemplateEngine-004, 005) and deep nesting (TemplateEngine-001) are not exercised.
10	Documentation & comments	✓	Mostly accurate; a misleading converter comment (TemplateEngine-011) and a stale enum/doc mismatch (TemplateEngine-012).

Findings

TemplateEngine-001 — Deeply nested composed members are dropped during flattening

Severity	High
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:211, src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:535, src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:609

Description

The design doc states composition supports "recursive nesting of feature modules" and that nested paths extend as [Outer].[Inner].[Member]. ResolveComposedAttributes only descends one level of nesting: it resolves the directly-composed module, then its immediate child compositions, and stops. A module composed three or more levels deep contributes no attributes to the flattened configuration. ResolveComposedAlarms and ResolveComposedScripts are worse — they handle only the first (direct) level and do not descend at all, so any alarm or script in a nested composed module is dropped entirely. CollisionDetector and TemplateResolver recurse fully, so collision detection and the authoring UI will show members that the deployed configuration silently lacks.

Recommendation

Replace the hand-unrolled one/two-level loops with a single recursive walk (carrying the accumulated path prefix) for attributes, alarms, and scripts, matching the recursion already in TemplateResolver.AddComposedMembers and CollisionDetector.CollectComposedMembers.

Resolution

Unresolved.

TemplateEngine-002 — Derived templates omit all base alarms; composed alarms cannot be overridden per slot

Severity	High
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:799

Description

BuildDerivedTemplate copies the base template's Attributes and Scripts into the new derived template as IsInherited = true placeholder rows so they can be overridden per composition slot, but there is no loop for Alarms. The derived template therefore has zero alarm rows. The TemplateAlarm entity also has no IsInherited or LockedInDerived fields (unlike TemplateAttribute / TemplateScript), so even if a copy loop were added there is no mechanism to mark a copied alarm as inherited or to override one. The design's Override Granularity section explicitly requires composed alarm fields (Priority, Trigger thresholds, Description, On-Trigger Script) to be overridable. As written, a composed module's alarms cannot be tuned for the slot they are used in.

Recommendation

Add an alarm copy loop to BuildDerivedTemplate and add IsInherited / LockedInDerived fields to TemplateAlarm, mirroring TemplateAttribute. Update UpdateAlarmAsync to honour them as UpdateAttributeAsync / UpdateScriptAsync already do.

Resolution

Unresolved.

TemplateEngine-003 — UpdateAttributeAsync lets a non-locked attribute change its fixed DataType / DataSourceReference

Severity	High
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:285

Description

LockEnforcer.ValidateAttributeOverride correctly rejects a change to DataType or DataSourceReference (both "fixed by the defining level" per the design). But the caller only honours that error when the attribute is already locked:

var granularityError = LockEnforcer.ValidateAttributeOverride(existing, proposed);
if (granularityError != null && existing.IsLocked)
    return Result<TemplateAttribute>.Failure(granularityError);

Lines 293-294 then unconditionally apply existing.DataType = proposed.DataType and existing.DataSourceReference = proposed.DataSourceReference. For the common case of an unlocked attribute, the fixed-field guard is dead and both fields are silently mutable, violating the override-granularity rule. (The lock-error branch of the same helper is also redundant — a locked attribute already returns earlier inside the helper.)

Recommendation

Remove the && existing.IsLocked condition so the granularity error is always returned, and stop assigning DataType / DataSourceReference from proposed in the apply block.

Resolution

Unresolved.

TemplateEngine-004 — Alarm on-trigger script references are never resolved (empty placeholder)

Severity	High
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:695

Description

ResolveAlarmScriptReferences is invoked as Step 7 of Flatten but its body is empty — only a comment describing what it should do. Consequently every ResolvedAlarm.OnTriggerScriptCanonicalName stays null. This has two downstream effects: (1) SemanticValidator's "on-trigger script must exist" check (SemanticValidator.cs:209) can never fire, so the design-mandated validation of alarm on-trigger script references is silently absent; (2) RevisionHashService and DiffService both hash/compare OnTriggerScriptCanonicalName, so a change to which script an alarm triggers never affects the revision hash and is invisible to the diff — a real staleness-detection gap.

Recommendation

Implement the resolution: map each alarm's OnTriggerScriptId (set on TemplateAlarm) to the canonical name of the corresponding resolved script, accounting for composition prefixes. If the design intends scripts to be referenced by name within scope, document and implement that consistently.

Resolution

Unresolved.

TemplateEngine-005 — Collision validation is skipped when creating a child template

Severity	High
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:56

Description

CreateTemplateAsync contains a block guarded by if (parentTemplateId.HasValue) that loads GetAllTemplatesAsync and then does nothing but hold a comment — it never runs a collision check. A child template created with a parent inherits the parent's members; if the child is later given members (via AddAttributeAsync etc.) those calls do run CollisionDetector, but the create path itself performs no naming-collision validation and UpdateTemplateAsync only validates collisions on a name change. The design states naming collisions are design-time errors that must block a save. The dead block is also confusing and allocates an unused full-table read.

Recommendation

Either run a real collision check on the to-be-created template (including its inherited members) or delete the dead block and its unused query. If create-time collisions are genuinely impossible because a fresh template has no members, document that explicitly instead of leaving a no-op.

Resolution

Unresolved.

TemplateEngine-006 — Forbidden-API enforcement is a naive substring scan (bypassable and false-positive prone)

Severity	Medium
Category	Security
Status	Open
Location	src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:21, src/ScadaLink.TemplateEngine/Validation/ValidationService.cs:318

Description

ScriptCompiler.ForbiddenPatterns is checked with code.Contains(pattern). This is both under- and over-inclusive against the script trust model:

• Bypass: using System.IO; followed by File.ReadAllText(...) contains no System.IO. token; using static System.IO.File;, namespace aliases, and global::System.IO.File all evade the literal patterns.
• False positive: a string literal, comment, or attribute name containing the text System.IO. is flagged as a forbidden API even though it is inert.

The same patterns are reused for trigger-expression validation (CheckExpressionSyntax), inheriting the same weakness. The file comment acknowledges this is interim until Roslyn is wired in, but the trust model is security-relevant and the gap should be tracked.

Recommendation

Defer real enforcement to the Roslyn-based compiler (semantic symbol analysis of referenced types/namespaces) rather than text matching. Until then, document the limitation prominently and treat the substring scan as advisory, not authoritative.

Resolution

Unresolved.

TemplateEngine-007 — Brace-balance "compilation" misjudges verbatim / interpolated / raw strings

Severity	Medium
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:54, src/ScadaLink.TemplateEngine/SharedScriptService.cs:124

Description

ScriptCompiler.TryCompile tracks string state with a single inString flag toggled on " and an escaped-quote check of code[i-1] != '\\'. It does not understand verbatim strings (@"..." where "" is the escape and \ is literal), interpolated strings ($"{...}" whose braces are code, not text), raw string literals ("""..."""), or char literals. A script with a verbatim string containing a brace, an interpolated string, or a '}' char literal will be wrongly rejected as having mismatched braces — blocking a valid script from deployment. SharedScriptService.ValidateSyntax is even cruder: it counts braces/brackets/parens with no string or comment awareness at all, so any string literal containing one of those characters produces a false syntax error.

Recommendation

Once the Roslyn compiler is available, parse with CSharpSyntaxTree.ParseText and inspect diagnostics instead of hand-rolling a tokenizer. If an interim check must remain, at minimum handle verbatim/interpolated/char literals or scope the check down to something that cannot false-positive.

Resolution

Unresolved.

TemplateEngine-008 — SetAlarmOverrideAsync accepts overrides for unknown / composed alarms with no validation

Severity	Medium
Category	Error handling & resilience
Status	Open
Location	src/ScadaLink.TemplateEngine/Services/InstanceService.cs:178

Description

SetAlarmOverrideAsync looks up the alarm by name among the template's direct alarms only. When the lookup returns null — which is the case for every composed (path-qualified) alarm as well as for a genuinely non-existent name — the method skips the lock check and proceeds to persist the override. This means: (1) an override can be created for an alarm that does not exist (a silent dead record), and (2) a composed alarm that is IsLocked at the template level can be overridden, bypassing the lock rule. SetAttributeOverrideAsync by contrast rejects unknown attribute names. The inline comment acknowledges the gap but the behaviour is inconsistent and risky.

Recommendation

Resolve the full effective alarm set (via the resolver / flattening) so composed alarms are found, reject overrides whose canonical name is not in that set, and apply the lock check to composed alarms too.

Resolution

Unresolved.

TemplateEngine-009 — N+1 query in TemplateDeletionService.CanDeleteTemplateAsync

Severity	Medium
Category	Performance & resource management
Status	Open
Location	src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs:75

Description

Check 3 ("other templates compose it directly") loads all templates and then issues a separate GetCompositionsByTemplateIdAsync call inside a loop over every template — one round-trip per template in the database. The composition information needed is already reachable via t.Compositions on the templates returned by GetAllTemplatesAsync (which TemplateService.DeleteTemplateAsync uses for the equivalent check at line 162). The loop scales linearly with the template count on every delete-precheck and every actual delete.

Recommendation

Use the Compositions navigation already loaded by GetAllTemplatesAsync, or add a single repository call that returns all compositions, rather than querying per template.

Resolution

Unresolved.

TemplateEngine-010 — InstanceService documents optimistic concurrency that is not implemented

Severity	Medium
Category	Documentation & comments
Status	Open
Location	src/ScadaLink.TemplateEngine/Services/InstanceService.cs:9

Description

The class summary states instances support "Enabled/disabled state with optimistic concurrency". EnableAsync, DisableAsync, AssignToAreaAsync and the override/binding mutators all perform a plain read-modify-write with no version token, RowVersion, or concurrency check. Two concurrent enable/disable requests last-writer-wins with no detection. Either the doc is stale (the design's optimistic-concurrency decision applies to deployment status records, not instance state) or a concurrency token was intended and is missing.

Recommendation

If last-write-wins is acceptable for instance state, correct the XML doc. If optimistic concurrency is required, add a concurrency token to Instance and surface a conflict result.

Resolution

Unresolved.

TemplateEngine-011 — SortedPropertiesConverterFactory is dead code with a misleading comment

Severity	Low
Category	Documentation & comments
Status	Open
Location	src/ScadaLink.TemplateEngine/Flattening/RevisionHashService.cs:136

Description

SortedPropertiesConverterFactory.CanConvert always returns false and CreateConverter always returns null, so the factory registered in CanonicalJsonOptions does nothing. The class comment claims it "ensures properties are serialized in alphabetical order for deterministic output", and the options comment says "Ensure consistent ordering" — both are false. Determinism actually relies entirely on the Hashable* records being hand-declared with alphabetically-ordered properties (plus camelCase). That works today but is fragile: a future contributor adding a property out of alphabetical order silently changes every revision hash, and the dead converter gives false confidence that ordering is enforced programmatically.

Recommendation

Either implement the converter to genuinely sort properties, or delete it and replace the comments with an explicit note that determinism depends on the manual property ordering of the Hashable* records (ideally enforced by a test).

Resolution

Unresolved.

TemplateEngine-012 — DataType enum naming diverges from the design doc

Severity	Low
Category	Design-document adherence
Status	Open
Location	src/ScadaLink.TemplateEngine/Validation/SemanticValidator.cs:18

Description

The design doc (Attribute section) lists data types as "Boolean, Integer, Float, String". The actual DataType enum is Boolean, Int32, Float, Double, DateTime, Binary. SemanticValidator.NumericDataTypes correctly hard-codes the real names (Int32, Float, Double), so the code is internally consistent, but the design doc is stale — it omits Double, DateTime, Binary and calls the integer type "Integer". This makes the doc an unreliable reference for which trigger-operand types are numeric.

Recommendation

Update docs/requirements/Component-TemplateEngine.md to list the actual enum members, or rename the enum to match the doc if "Integer" is the intended canonical name.

Resolution

Unresolved.

TemplateEngine-013 — ToDictionary(t => t.Id) throws on duplicate IDs; cycle detectors overload Id 0 as a sentinel

Severity	Low
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/CycleDetector.cs:30, src/ScadaLink.TemplateEngine/CycleDetector.cs:38

Description

Across the static helpers, allTemplates.ToDictionary(t => t.Id) is used freely; if the caller ever passes a list containing two templates with the same Id (e.g. a not-yet-saved template assigned Id == 0, or duplicated input) the call throws an unhandled ArgumentException rather than returning a Result failure. Separately, CycleDetector uses 0 as the "no parent" sentinel (currentId != 0, ParentTemplateId ?? 0) and DetectInheritanceCycle / DetectCrossGraphCycle ignore a proposed parent/composed id of 0. EF identity keys start at 1 so this is currently benign, but the overload is fragile — an in-memory or test template with Id == 0 would be treated as "no template" and cycle checks would be silently skipped.

Recommendation

Guard the dictionary builds (or use a grouping/ToLookup) and validate input, and use int?/-1 rather than 0 as the no-parent sentinel so a real id of 0 is never special.

Resolution

Unresolved.

TemplateEngine-014 — Template-deletion constraint logic is duplicated and divergent

Severity	Low
Category	Code organization & conventions
Status	Open
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:109, src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs:27

Description

TemplateService.DeleteTemplateAsync and TemplateDeletionService.CanDeleteTemplateAsync both implement the "can this template be deleted" rules (instances, child templates, derived templates, composing templates). The two implementations have already drifted: TemplateService reads composing templates from the in-memory t.Compositions navigation while TemplateDeletionService issues per-template GetCompositionsByTemplateIdAsync calls (see TemplateEngine-009), they format error messages differently, and TemplateService returns on the first failing category while TemplateDeletionService accumulates all of them. A future rule change must be made in two places or behaviour will diverge further.

Recommendation

Make TemplateService.DeleteTemplateAsync delegate to TemplateDeletionService (or vice versa) so the constraint logic lives in exactly one place.

Resolution

Unresolved.