DraftValidator/DraftSnapshot is dead code (no src/ caller) — possible publish-time enforcement gap #422

New Issue

2026-06-01T10:19:51-04:00

dohertj2 commented

2026-06-01 10:19:51 -04:00

Summary

DraftValidator (the managed "pre-publish validator per decision #91") has no live caller anywhere in src/. A repo-wide search finds its only references are its own source files and its test:

src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs
src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftSnapshot.cs
tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorTests.cs (15 tests)

Nothing constructs a DraftSnapshot or calls DraftValidator.Validate(...) / ValidateClusterTopology(...) outside that test. It is not registered in DI and is not invoked from the publish path.

$ grep -rn --include='*.cs' -e 'DraftValidator\.' -e 'new DraftSnapshot' src \
    | grep -v 'Validation/DraftValidator.cs\|Validation/DraftSnapshot.cs'
# (no results)

The enforced pre-publish draft validation actually runs DB-side in sp_ValidateDraft (src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs, Procs.ValidateDraft ~:157), as part of the Status='Draft' → sp_PublishGeneration generation lifecycle. So the managed DraftValidator is currently dormant complement code — and there are passing tests exercising a validator that production never calls.

Why this may be more than dead code (potential enforcement gap)

DraftValidator's own doc-comment (:7–13) says it "complements the structural checks in sp_ValidateDraft — this layer owns schema validation for JSON columns, UNS segment regex, EquipmentId derivation, cross-cluster checks, and anything else uncomfortable to express in T-SQL." But because it is not wired in, the rules it uniquely owns may be enforced nowhere at publish time.

Comparing its private rule methods against the sp_ValidateDraft RAISERROR checks:

`DraftValidator` rule	In `sp_ValidateDraft`?
`ValidateUnsSegments` (UNS segment regex)	No
`ValidatePathLength` (≤ 200)	No
`ValidateEquipmentIdDerivation`	No
`ValidateClusterTopology` (topology vs `RedundancyMode`)	No
`ValidateEquipmentUuidImmutability`	Yes (`:233`)
`ValidateSameClusterNamespaceBinding`	Yes (`:218`, `BadCrossClusterNamespaceBinding`)
`ValidateReservationPreflight` (ZTag/SAPID)	Yes (`:245`/`:257`)
`ValidateDriverNamespaceCompatibility`	Partial (driver-type→namespace-kind, #111)

So UNS-segment regex, path-length, EquipmentId derivation, and cluster-topology-vs-RedundancyMode appear to be guarded only by the uncalled C# validator. (I have not read the full stored-proc body or checked for DB constraints/triggers that might cover them — worth confirming before concluding they're truly unenforced.)

Suggested resolution — pick one

Re-wire DraftValidator as intended — invoke it in the publish path (alongside / before sp_ValidateDraft) so the JSON-schema / UNS-regex / EquipmentId-derivation / cluster-topology rules are actually enforced. Confirms the 15 existing tests guard live behavior.
Remove it as dead code — delete DraftValidator.cs, DraftSnapshot.cs, and DraftValidatorTests.cs, and confirm sp_ValidateDraft (plus any DB constraints) is the intended sole enforcement. Only safe if the four "No" rules above are genuinely covered elsewhere or deliberately dropped.

Option 1 is preferable if those rules matter at publish time; option 2 only after verifying nothing is lost.

Context

Surfaced while normalizing config-validation across the ZB.MOM.WW.* family (scadaproj → ZB.MOM.WW.Configuration). This is purely an OtOpcUa housekeeping/correctness question — unrelated to that shared options-validation library (draft/generation-content validation is correctly out of its scope either way). Filing here so it isn't lost.

Verified against main on 2026-06-01.

## Summary `DraftValidator` (the managed "pre-publish validator per decision #91") has **no live caller** anywhere in `src/`. A repo-wide search finds its only references are its own source files and its test: - `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs` - `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftSnapshot.cs` - `tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorTests.cs` (15 tests) Nothing constructs a `DraftSnapshot` or calls `DraftValidator.Validate(...)` / `ValidateClusterTopology(...)` outside that test. It is not registered in DI and is not invoked from the publish path. ``` $ grep -rn --include='*.cs' -e 'DraftValidator\.' -e 'new DraftSnapshot' src \ | grep -v 'Validation/DraftValidator.cs\|Validation/DraftSnapshot.cs' # (no results) ``` The enforced pre-publish draft validation actually runs **DB-side** in `sp_ValidateDraft` (`src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs`, `Procs.ValidateDraft` ~`:157`), as part of the `Status='Draft' → sp_PublishGeneration` generation lifecycle. So the managed `DraftValidator` is currently **dormant complement code** — and there are passing tests exercising a validator that production never calls. ## Why this may be more than dead code (potential enforcement gap) `DraftValidator`'s own doc-comment (`:7–13`) says it "complements the structural checks in `sp_ValidateDraft` — this layer owns schema validation for JSON columns, UNS segment regex, EquipmentId derivation, cross-cluster checks, and anything else uncomfortable to express in T-SQL." But because it is not wired in, the rules it **uniquely** owns may be enforced **nowhere** at publish time. Comparing its private rule methods against the `sp_ValidateDraft` RAISERROR checks: | `DraftValidator` rule | In `sp_ValidateDraft`? | |---|---| | `ValidateUnsSegments` (UNS segment regex) | **No** | | `ValidatePathLength` (≤ 200) | **No** | | `ValidateEquipmentIdDerivation` | **No** | | `ValidateClusterTopology` (topology vs `RedundancyMode`) | **No** | | `ValidateEquipmentUuidImmutability` | Yes (`:233`) | | `ValidateSameClusterNamespaceBinding` | Yes (`:218`, `BadCrossClusterNamespaceBinding`) | | `ValidateReservationPreflight` (ZTag/SAPID) | Yes (`:245`/`:257`) | | `ValidateDriverNamespaceCompatibility` | Partial (driver-type→namespace-kind, #111) | So **UNS-segment regex, path-length, EquipmentId derivation, and cluster-topology-vs-RedundancyMode** appear to be guarded only by the uncalled C# validator. (I have not read the full stored-proc body or checked for DB constraints/triggers that might cover them — worth confirming before concluding they're truly unenforced.) ## Suggested resolution — pick one 1. **Re-wire `DraftValidator` as intended** — invoke it in the publish path (alongside / before `sp_ValidateDraft`) so the JSON-schema / UNS-regex / EquipmentId-derivation / cluster-topology rules are actually enforced. Confirms the 15 existing tests guard live behavior. 2. **Remove it as dead code** — delete `DraftValidator.cs`, `DraftSnapshot.cs`, and `DraftValidatorTests.cs`, and confirm `sp_ValidateDraft` (plus any DB constraints) is the intended sole enforcement. Only safe if the four "No" rules above are genuinely covered elsewhere or deliberately dropped. Option 1 is preferable if those rules matter at publish time; option 2 only after verifying nothing is lost. ## Context Surfaced while normalizing config-validation across the `ZB.MOM.WW.*` family (`scadaproj` → `ZB.MOM.WW.Configuration`). This is purely an OtOpcUa housekeeping/correctness question — unrelated to that shared **options**-validation library (draft/generation-content validation is correctly out of its scope either way). Filing here so it isn't lost. _Verified against `main` on 2026-06-01._

dohertj2 commented

2026-07-15 08:18:40 -04:00

Stale — the issue's premise is now inverted. DraftValidator is LIVE-wired into the deploy gate; do NOT delete. (Verified on master @ 5908339d.)

When this was filed (2026-06-01) it was against a pre-v2 state. The v2 Akka deploy path (StartDeployment) was subsequently wired to call DraftValidator, so it is no longer dead code:

src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs:181 — DraftSnapshotFactory.FromConfigDbAsync(db)
:182 — DraftValidator.Validate(draft) (hard pre-seal reject gate)
:208 — DraftValidator.ValidateClusterTopology(cluster, nodes)

Reached live from AdminUI: POST /api/deployments → AdminOperationsClient.StartDeploymentAsync → StartDeployment msg → HandleStartDeploymentAsync. Any validation error rejects the deploy.

Corollary (the inversion): it is now sp_ValidateDraft / sp_PublishGeneration that have no C# caller — the generation-publish SQL path is dormant in the v2 Akka architecture, and the C# DraftValidator is the live publish gate. So the framing "the DB proc is what actually runs" is reversed.

The four "unique" rules are genuinely load-bearing (deletion would lose them):

ValidatePathLength (<=200) — enforced NOWHERE else; the OPC UA path budget would go unchecked.
UNS segment regex for Area/Line names — UnsTreeService create/update applies no regex to them (only Equipment has a bypassable client-side [RegularExpression] on EquipmentPage.razor). DraftValidator is the sole publish-time backstop.
Enabled-node topology-drift — the DB CHECK CK_ServerCluster_RedundancyMode_NodeCount validates the declared (NodeCount, RedundancyMode) pair but cannot see ClusterNode.Enabled; only ValidateClusterTopology catches nodes toggled off under Hot/Warm.
ValidateEquipmentIdDerivation — redundant-by-construction (write path sets EQ-<12hex> inline at UnsTreeService.cs:522/593), but a cheap backstop against hand-edited/imported rows.

The 15 DraftValidatorTests therefore guard live behavior. Closing as resolved (already wired; nothing to do).

One optional future cleanup (not blocking): DeriveEquipmentId is duplicated inline at UnsTreeService.cs:522/593 rather than calling the canonical method — a de-dup refactor, not a correctness gap.

**Stale — the issue's premise is now inverted. `DraftValidator` is LIVE-wired into the deploy gate; do NOT delete.** (Verified on `master` @ `5908339d`.) When this was filed (2026-06-01) it was against a pre-v2 state. The v2 Akka deploy path (`StartDeployment`) was subsequently wired to call `DraftValidator`, so it is no longer dead code: - `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs:181` — `DraftSnapshotFactory.FromConfigDbAsync(db)` - `:182` — `DraftValidator.Validate(draft)` (hard pre-seal reject gate) - `:208` — `DraftValidator.ValidateClusterTopology(cluster, nodes)` Reached live from AdminUI: `POST /api/deployments` → `AdminOperationsClient.StartDeploymentAsync` → `StartDeployment` msg → `HandleStartDeploymentAsync`. Any validation error rejects the deploy. **Corollary (the inversion):** it is now `sp_ValidateDraft` / `sp_PublishGeneration` that have **no C# caller** — the generation-publish SQL path is dormant in the v2 Akka architecture, and the C# `DraftValidator` is the live publish gate. So the framing "the DB proc is what actually runs" is reversed. **The four "unique" rules are genuinely load-bearing (deletion would lose them):** - `ValidatePathLength (<=200)` — enforced NOWHERE else; the OPC UA path budget would go unchecked. - UNS segment regex for **Area/Line** names — `UnsTreeService` create/update applies no regex to them (only Equipment has a bypassable client-side `[RegularExpression]` on `EquipmentPage.razor`). DraftValidator is the sole publish-time backstop. - Enabled-node topology-drift — the DB CHECK `CK_ServerCluster_RedundancyMode_NodeCount` validates the declared `(NodeCount, RedundancyMode)` pair but cannot see `ClusterNode.Enabled`; only `ValidateClusterTopology` catches nodes toggled off under Hot/Warm. - `ValidateEquipmentIdDerivation` — redundant-by-construction (write path sets `EQ-<12hex>` inline at `UnsTreeService.cs:522/593`), but a cheap backstop against hand-edited/imported rows. The 15 `DraftValidatorTests` therefore guard **live** behavior. Closing as resolved (already wired; nothing to do). _One optional future cleanup (not blocking): `DeriveEquipmentId` is duplicated inline at `UnsTreeService.cs:522/593` rather than calling the canonical method — a de-dup refactor, not a correctness gap._

dohertj2 closed this issue

2026-07-15 08:18:41 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#422