dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity
Files
74aae53500f0889cbda7ea25732806a2cca1e699
scadalink-design/code-reviews/TemplateEngine/findings.md

530 lines
24 KiB
Markdown
Raw Blame History

# 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 | 10 |
## 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 | Resolved |
| 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**
Resolved 2026-05-16 (commit `<pending>`): replaced the hand-unrolled
one/two-level composition loops in `ResolveComposedAttributes`,
`ResolveComposedAlarms`, and `ResolveComposedScripts` with single recursive
walks (`*Recursive` helpers) carrying the accumulated path prefix and a
`visited` set, so composed members at arbitrary nesting depth are resolved.
Regression tests: `Flatten_ThreeLevelComposition_AttributesAlarmsScriptsAllResolved`,
`Flatten_NestedComposedAlarm_TriggerAttributePrefixed`.
### 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 (re-triaged 2026-05-16)._ Partially mis-stated and out of the
current fix scope. Correction to the description: composed/inherited alarms
are **not** dropped from the flattened deployment output — `FlatteningService`
resolves alarms from the entire inheritance chain (`ResolveInheritedAlarms`
walks `templateChain`, which includes the base of a derived template), so an
instance of a derived template still receives the base template's alarms. The
real, valid gap is narrower: there is no per-slot **alarm override**
mechanism. The fix genuinely requires adding `IsInherited` / `LockedInDerived`
fields to the `TemplateAlarm` entity, which lives in `ScadaLink.Commons`
(a different module). Adding an alarm copy loop to `BuildDerivedTemplate`
without those fields would be actively harmful: copied alarm rows on the
derived template would shadow the live base alarm with stale data during
flattening (`ResolveInheritedAlarms` has no `IsInherited` skip for alarms,
unlike attributes/scripts). Resolving this safely is a cross-module change
(`Commons` + `TemplateEngine`) and must be scheduled as a coordinated edit;
left **Open** pending that.
### TemplateEngine-003 — `UpdateAttributeAsync` lets a non-locked attribute change its fixed DataType / DataSourceReference
| | |
|--|--|
| Severity | High |
| Category | Correctness & logic bugs |
| Status | Resolved |
| 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:
```csharp
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**
Resolved 2026-05-16 (commit `<pending>`): removed the `&& existing.IsLocked`
guard in `UpdateAttributeAsync` so the fixed-field granularity error is always
honoured, and removed the unconditional `existing.DataType` /
`existing.DataSourceReference` assignments from the apply block. Regression
tests: `UpdateAttribute_UnlockedAttribute_DataTypeChangeRejected`,
`UpdateAttribute_UnlockedAttribute_DataSourceReferenceChangeRejected`.
### TemplateEngine-004 — Alarm on-trigger script references are never resolved (empty placeholder)
| | |
|--|--|
| Severity | High |
| Category | Correctness & logic bugs |
| Status | Resolved |
| 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**
Resolved 2026-05-16 (commit `<pending>`): implemented `ResolveAlarmScriptReferences`.
Alarm resolution now records each resolved alarm's `OnTriggerScriptId` keyed by
canonical name, and script resolution records each resolved `TemplateScript.Id`
keyed by its canonical name (both honour composition path prefixes). Step 7
joins the two maps to set `ResolvedAlarm.OnTriggerScriptCanonicalName`, so the
revision hash, diff, and `SemanticValidator` on-trigger-script-exists check now
all see the reference. Regression tests:
`Flatten_AlarmOnTriggerScript_ResolvedToCanonicalName`,
`Flatten_ComposedAlarmOnTriggerScript_ResolvedWithPrefix`.
### TemplateEngine-005 — Collision validation is skipped when creating a child template
| | |
|--|--|
| Severity | High |
| Category | Correctness & logic bugs |
| Status | Resolved |
| 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**
Resolved 2026-05-16 (commit `<pending>`): deleted the dead `if
(parentTemplateId.HasValue)` block and its unused `GetAllTemplatesAsync`
read in `CreateTemplateAsync`. A create-time collision check on a child is a
guaranteed no-op — a freshly created template has no members of its own, the
parent's members were already collision-validated on every member-mutating
call, and a new child cannot be an ancestor of its parent. Replaced the no-op
with an explanatory comment documenting that collision detection is enforced
on `AddAttribute`/`AddAlarm`/`AddScript`/`AddComposition` and on rename.
Regression test: `CreateTemplate_WithParent_DoesNotRunDeadCollisionQuery`.
### 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._
Reference in New Issue View Git Blame Copy Permalink