Joseph Doherty
1c67d019e5
The comments and docs that described Driver.MTConnect-003 as live at HEAD would
have been wrong the moment 64be41d8 landed:
- code-reviews/Driver.MTConnect/findings.md — Driver.MTConnect-003 Resolved,
with the fixing commit; module open-finding count 5 -> 4.
- code-reviews/Driver.MTConnect.Contracts/findings.md — the one clause inside
Contracts-001's prose that asserted 003 was Open.
- docs/drivers/MTConnect.md — the coercion-precedence warning now states that a
browse-committed tag resolves at step 1 CARRYING the step-3 answer, and that
step 3 remains the rescue for a hand-authored / CSV / API blob with no type.
- MTConnectDriver.BuildBinding, MTConnectDataTypeInference and
MTConnectDriverOptions.RawTags remarks — same correction, each keeping a short
historical note of what the defect was so a reader meeting the old blobs still
understands them.
Comment-only in src; the driver suite (538) and AdminUI suite (1474) re-run green.
77 lines
8.2 KiB
Markdown
77 lines
8.2 KiB
Markdown
# Code Review — Driver.MTConnect.Contracts
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts` |
|
||
| Reviewer | Claude (per-module review) |
|
||
| Review date | 2026-08-10 |
|
||
| Commit reviewed | `b990adae` |
|
||
| Status | Complete |
|
||
| Open findings | 0 |
|
||
|
||
## Checklist coverage
|
||
|
||
A comprehensive review completes every category, recording "No issues found" where
|
||
a category produced nothing rather than leaving it blank.
|
||
|
||
| # | Category | Result |
|
||
|---|---|---|
|
||
| 1 | Correctness & logic bugs | No issues found. `TryFromTagConfig` cannot throw for any input: the only reachable failure of `JsonSerializer.Deserialize` over this all-scalar DTO is `JsonException`, which is caught, and the null/blank input is guarded before it. `SeparatorInsensitiveComparer.Equals` and `GetHashCode` hash and compare exactly the same characters (the classic bucket-mismatch bug is explicitly avoided and commented). `Infer`'s `(uint)sampleCount.Value` cannot overflow — `sampleCount` is an `int` the probe parser already constrained, and the cast is guarded by `> 0`. The `DATA_SET`/`TABLE` demotion is correctly checked **before** the category, so a `DATA_SET` `SAMPLE` cannot escape as `Float64`. |
|
||
| 2 | OtOpcUa conventions | No issues found. Zero-dependency leaf over `Core.Abstractions` only, mirroring `Driver.Modbus.Contracts` / `Driver.AbCip.Contracts`; net10.0, nullable enabled, `TreatWarningsAsErrors`. No `JsonStringEnumConverter` on `JsonOptions` — the type-carrying field stays `string?` and is parsed by NAME, with a digits-only pre-check that rejects the numerically-serialized-enum shape before `Enum.TryParse` can accept `"8"` as an ordinal. The absent `GenerateDocumentationFile` matches every other `*.Contracts` project in the fleet (checked against Modbus/AbCip/S7/Mqtt), so it is convention rather than an omission. |
|
||
| 3 | Concurrency & thread safety | No issues found. Every type here is immutable or stateless: two `sealed record`s, a `readonly record struct`, a `FrozenSet` built once in a static initialiser, and a stateless singleton comparer. No mutable static state, no I/O, no locks needed. |
|
||
| 4 | Error handling & resilience | No issues found. `TryFromTagConfig` is total (never throws, returns a verdict), `Inspect` is total, and `IsAcceptedDataTypeName` treats blank as accepted so an unauthored type falls through to the Agent's own declaration rather than rejecting a tag. A present-but-invalid type rejects rather than silently defaulting — the sibling drivers' strict-enum posture, and the fix for Gitea #549. |
|
||
| 5 | Security | No issues found. No I/O, no credentials, no logging, no reflection over untrusted input; the JSON reader is bounded by a fixed DTO shape. `Inspect`'s messages echo the operator's own authored value and the valid enum names, which is the intended operator-facing detail and carries nothing secret. |
|
||
| 6 | Performance & resource management | No issues found. `MTConnectInferredType` is a `readonly record struct` so the discovery hot path allocates nothing; `NumericEventTypes` is a `FrozenSet` consulted through a comparer rather than by normalising the input, so the lookup needs no temporary string; `Infer` trims via `ReadOnlySpan<char>`. Nothing here is disposable. |
|
||
| 7 | Design-document adherence | 1 finding — `Driver.MTConnect.Contracts-001`. The accept-gate-first contract from the #549–#559 arc is correctly implemented: `Inspect` asks `TryFromTagConfig` for the verdict and only explains a rejection, so it can never be stricter than the runtime; `IsAcceptedDataTypeName` is the single authority both the runtime and `MTConnectTagConfigModel.Validate` consult; and `FirstNonBlank(DriverDataType, DataType)` is deliberate, mirrored by the AdminUI model's load order and its normalise-onto-`dataType`-and-drop-the-alias save (#550). The `reference.TrimStart().FirstOrDefault() != '{'` early return in `Inspect` is likewise fleet convention — all ten driver factories carry the identical guard — so it was reviewed and not raised. |
|
||
| 8 | Code organization & conventions | No issues found. The parse lives here as one authority rather than being mirrored into the AdminUI, which is exactly the drift the arc's design argues against; the DTO is private to the factory; the Options pattern is honoured and every collection defaults to empty rather than null. |
|
||
| 9 | Testing coverage | No issues found. The project has no test project of its own (fleet convention for `*.Contracts`); it is covered from `Driver.MTConnect.Tests` (`MTConnectDataTypeInferenceTests` is a golden table including a probe-vs-streams spelling-parity case; `MTConnectRawTagBindingTests` and `MTConnectDriverOptionsTests` drive the factory) and from `AdminUI.Tests`, whose TagConfig parity corpus consumes `IsAcceptedDataTypeName` / `Inspect` through `MTConnectTagConfigModel`. Both suites were run after this review's changes: 538 and 1466 pass. |
|
||
| 10 | Documentation & comments | 1 finding — `Driver.MTConnect.Contracts-001`. The XML documentation is otherwise excellent and records the *reason* for each rule — why the digits-only pre-check exists, why the alias out-ranks the canonical key, why the comparer is separator-insensitive (the probe document's `PART_COUNT` vs the streams document's `PartCount`), and why blank is accepted. |
|
||
|
||
## Findings
|
||
|
||
<!-- One #### entry per finding. IDs are Driver.MTConnect.Contracts-NNN, sequential within the
|
||
module, never reused. Findings are never deleted — close them by changing Status and
|
||
completing Resolution. -->
|
||
|
||
### Driver.MTConnect.Contracts-001
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Documentation & comments |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs:31` · `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs:86` |
|
||
| Status | Resolved |
|
||
|
||
**Description:** Both public types documented a call graph that no longer exists, and in one case the
|
||
false premise concealed a live defect rather than merely misinforming a reader.
|
||
|
||
1. `MTConnectDataTypeInference`'s class remarks justify living in `.Contracts` because "three call
|
||
sites must agree byte-for-byte": `ITagDiscovery.DiscoverAsync`, "the universal browser's commit
|
||
path (writes the browsed leaf into `TagConfig`)", and "the AdminUI typed tag editor (shows the
|
||
inferred type and offers an override)". At HEAD a `grep` for `MTConnectDataTypeInference` outside
|
||
this project returns only the driver: `RawBrowseCommitMapper` never calls it, and
|
||
`MTConnectTagConfigModel` explicitly documents that it does **not** recompute the inference. Two
|
||
of the three named callers do not exist.
|
||
2. `MTConnectDriverOptions.RawTags` describes the coercion-type precedence and states that the
|
||
`/probe`-inference step "is what makes a browse-committed tag work: `RawBrowseCommitMapper` writes
|
||
only the address into the blob, so without it every value from the address picker would publish as
|
||
text under a numerically-typed OPC UA node." That premise is false at HEAD — the mapper writes a
|
||
full `MTConnectTagConfigModel` blob whose `dataType` defaults to `String` — and the sentence
|
||
therefore asserts as *prevented* precisely the outcome that now happens on every browse commit.
|
||
|
||
The second is what makes this worth recording rather than a tidy-up: a reader auditing the browse
|
||
path would have taken that remark as evidence the case was handled. The underlying defect is
|
||
recorded as `Driver.MTConnect-003` (Resolved `64be41d8`, 2026-08-10, in `RawBrowseCommitMapper`).
|
||
|
||
**Recommendation:** Correct both remarks to describe the callers that exist, and — rather than
|
||
deleting the precedence rule, which is still correct for hand-authored, CSV-imported and API-authored
|
||
blobs — state explicitly that the step is not reached by a browse-committed tag today, with a pointer
|
||
to the finding that tracks the cause.
|
||
|
||
**Resolution:** Fixed `6c63bf8f`, 2026-08-10 — `MTConnectDataTypeInference` now names its two real
|
||
callers (`DiscoverAsync` and `MTConnectDriver.InferDefinition`) and carries a warning that the two
|
||
it used to name do not call it; `MTConnectDriverOptions.RawTags` carries a warning that the
|
||
`/probe` step is unreachable for a browse-committed tag at HEAD. Both point at
|
||
`Driver.MTConnect-003`. Comment-only: `Driver.MTConnect.Tests` (538) and `AdminUI.Tests` (1466) both
|
||
re-run green.
|