Joseph Doherty
33f7472002
No functional defects found. One Low/Open record-only finding: Inspect() double-parses its TagConfig blob (once directly, once via TryReadTagConfig) by deliberate design of the module's single-authority split — not pursued as a fix since it is deploy-time-only (not a hot path) and widening TryReadTagConfig's signature to avoid it touches consumers outside this module (Driver.Calculation, AdminUI).
5.6 KiB
Code Review — Driver.Calculation.Contracts
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts |
| Reviewer | Claude (automated review agent) |
| Review date | 2026-08-10 |
| Commit reviewed | b990adae |
| Status | Reviewed |
| Open findings | 1 |
Checklist coverage
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | No issues found. TryReadTagConfig's field-by-field reading (scriptId/scriptSource/changeTriggered/timerIntervalMs) matches its own documented lenient-defaulting rules exactly, and Inspect's warnings track the same rules one-for-one — verified by cross-reading both against TagConfigNonEnumParityTests.cs in AdminUI.Tests. |
| 2 | OtOpcUa conventions | No issues found. This is a pure JSON-reading leaf with no transport, no gateway, and no Galaxy-specific surface — the relevant conventions here are the deploy-time TagConfig-gate architecture (G-7: one authority shared by the runtime read and the Inspect gate), which this module satisfies by construction (CalculationTagDefinition.TryFromTagConfig, Driver.Calculation, calls CalculationTagDefinitionFactory.TryReadTagConfig, this module, rather than re-implementing the blob read). |
| 3 | Concurrency & thread safety | No issues found. Purely functional (JsonDocument.Parse + local reads), no shared mutable state. |
| 4 | Error handling & resilience | No issues found. Both TryReadTagConfig and Inspect are documented never to throw and both live up to it: every JsonDocument.Parse call is wrapped in try/catch (JsonException), and a malformed blob is a false/warning result rather than an exception escaping to the deploy pipeline or the runtime driver. |
| 5 | Security | No issues found. No secrets, no I/O beyond parsing an in-memory string; nothing here can be used for injection (script execution is a Roslyn-sandboxed concern owned by Core.Scripting, not this module). |
| 6 | Performance & resource management | 1 finding (Driver.Calculation.Contracts-001), Open, Low — a deliberate double-parse in Inspect, at deploy-time only (not a runtime hot path). |
| 7 | Design-document adherence | No issues found. The class-level remarks explicitly state the "one authority" split (this module owns what the BLOB says; Driver.Calculation owns identity + dependency-ref extraction) and the "Inspect is never stricter than TryReadTagConfig" invariant (pinned by a falsification-control test in AdminUI.Tests, per the project memory on this exact pattern) — both hold under inspection of the actual code. |
| 8 | Code organization & conventions | No issues found. Single-file, single-responsibility leaf; the split rationale (why NamespaceMap-style SDK-typed helpers don't belong here, why the dependency-ref extraction stays in Driver.Calculation) is documented inline and matches the actual dependency graph. |
| 9 | Testing coverage | No issues found. There is no dedicated Driver.Calculation.Contracts.Tests project, but this matches the fleet-wide convention — no *.Contracts driver library anywhere in the repo has one (ls tests/Drivers | grep -i Contracts is empty). TryReadTagConfig is exercised indirectly via CalculationTagDefinitionTests in Driver.Calculation.Tests, and Inspect is directly tested in tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs (A_Calculation_blob_that_draws_an_advisory_but_parses_clean_is_accepted_at_authoring), which specifically pins the "Inspect never stricter than the runtime read" invariant with a falsification control. |
| 10 | Documentation & comments | No issues found. The XML docs are accurate and unusually explicit about WHY the design is shaped the way it is (the authority split, the deliberately-weaker-than-AdminUI Inspect semantics called out in the review brief). |
Findings
Driver.Calculation.Contracts-001
| Field | Value |
|---|---|
| Severity | Low |
| Category | Performance & resource management |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts/CalculationTagDefinitionFactory.cs:107-160 (Inspect) |
| Status | Open |
Description: Inspect parses reference with JsonDocument.Parse once directly (to check the root
kind and read changeTriggered/timerIntervalMs for the "present-but-discarded" warnings), and then —
when the object parsed cleanly — calls TryReadTagConfig(reference, out _) (line 133), which parses the
SAME string a second time from scratch to decide whether a usable scriptId is present. This is a
deliberate consequence of the module's own "one authority" design (Inspect must consult
TryReadTagConfig, not reimplement its scriptId rule), so it is not a bug, but it is a real double-parse
of every calc tag's TagConfig blob on every deploy-time gate pass.
Recommendation: If this ever needs to scale beyond a deploy-time gate (e.g. if Inspect starts being
called from a hot path, which it is not today), consider giving TryReadTagConfig an internal overload
that accepts an already-parsed JsonElement so Inspect can pass its own root through without a
second JsonDocument.Parse. Not pursued in this review: TryReadTagConfig's public string-based
signature is consumed by other modules (Driver.Calculation's CalculationTagDefinition.TryFromTagConfig,
and AdminUI's CalculationTagConfigModel.cs), so widening it safely is a small cross-module change, not
a same-file one-liner, and the current cost is one extra JSON parse per calc tag at deploy time — not a
runtime cost.
Resolution: (Open — Low severity, record-only per review policy; deploy-time only, not a hot path.)