diff --git a/code-reviews/Contracts/findings.md b/code-reviews/Contracts/findings.md index 240d8f3..cfd6fd9 100644 --- a/code-reviews/Contracts/findings.md +++ b/code-reviews/Contracts/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-18 | | Commit reviewed | `88915c3` | | Status | Re-reviewed | -| Open findings | 3 | +| Open findings | 0 | ## Checklist coverage @@ -510,7 +510,7 @@ and `docs/WorkerConversion.md` (section "Sparse array expansion"). | Severity | Low | | Category | Testing coverage | | Location | `src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** No round-trip test or descriptor pin exists for the new `MxSparseArray` message, `MxSparseElement` message, or `MxValue.KindOneofCase.SparseArrayValue` (field number 19). A future renumber or type-narrowing of `sparse_array_value = 19`, or of `MxSparseArray`'s field numbers (1/2/3) or `MxSparseElement`'s field numbers (1/2), would not be caught at the contract level. This is the same gap class as Contracts-007 (`MxValue.raw_value`), Contracts-010 (bulk write/read), Contracts-018 (alarm-provider fallback), and Contracts-022 (`ReplayGap`) — all of which were resolved by adding focused round-trip tests. @@ -518,6 +518,8 @@ Additionally, the `MxSparseElement.value` field is typed `MxValue` (the full val **Recommendation:** Add round-trip / descriptor-pin tests to `ProtobufContractRoundTripTests`: (a) pin `MxValue.SparseArrayValueFieldNumber == 19` via the generated constant; (b) round-trip an `MxSparseArray` with `element_data_type`, `total_length`, and at least one `MxSparseElement` (covering `index` and a scalar `value`), embedded in an `MxValue` with `KindCase == SparseArrayValue`; (c) assert the `MxSparseArray` field numbers by name via `MxSparseArray.Descriptor.Fields` (1 = `element_data_type`, 2 = `total_length`, 3 = `elements`) and `MxSparseElement.Descriptor.Fields` (1 = `index`, 2 = `value`). Optionally add a second test with an empty `elements` list (valid all-defaults case) to pin that zero elements is not a proto-level error. +**Resolution:** _(2026-06-18)_ Confirmed all three gaps against the proto and generated constants. Added `ProtobufContractRoundTripTests.MxValue_RoundTripsSparseArrayValueAndPinsFieldNumbers` to `ProtobufContractRoundTripTests.cs`. The test: (a) pins `MxValue.SparseArrayValueFieldNumber == 19` via the generated constant; (b) pins all five field numbers by name + number via the descriptor (`MxSparseArray` fields 1/2/3 and `MxSparseElement` fields 1/2); (c) round-trips an `MxValue` with `KindCase == SparseArrayValue` carrying a populated `MxSparseArray` (one `MxSparseElement` with a scalar float value at index 2); (d) verifies an all-defaults `MxSparseArray` with no elements is not a proto-level error. The full `ProtobufContractRoundTrip|GatewayContractInfo` filter is 54/54 green. + ### Contracts-024 | Field | Value | @@ -525,12 +527,14 @@ Additionally, the `MxSparseElement.value` field is typed `MxValue` (the full val | Severity | Low | | Category | Documentation & comments | | Location | `docs/Contracts.md:9-11` | -| Status | Open | +| Status | Resolved | **Description:** `docs/Contracts.md` lists `MxValue`, `MxArray`, and `MxStatusProxy` as the types defined in `mxaccess_gateway.proto`, and documents both bulk subscription and bulk write/read command families in detail. The new `MxSparseArray` value arm (`sparse_array_value = 19`) — a public-facing addition to the `MxValue` oneof that changes the write API available to every command variant — is not mentioned anywhere in `docs/Contracts.md`. The CLAUDE.md rule "Update docs in the same change as the source. When public APIs, contracts, configuration … change, the affected docs … must change in the same commit" was not satisfied for this addition; `docs/Contracts.md` now undercounts the public `MxValue` surface. `gateway.md` and `docs/WorkerConversion.md` were updated, but `docs/Contracts.md` — the canonical contracts document linked from the client generation doc — was not. **Recommendation:** Extend `docs/Contracts.md` to describe `MxSparseArray`: the write-only `sparse_array_value = 19` arm on `MxValue`, the two messages (`MxSparseArray` with `element_data_type`, `total_length`, `elements`; `MxSparseElement` with `index`, `value`), the default-fill-not-preserve semantics for unmentioned indices, and the fact that it is accepted by every write variant (`Write`, `Write2`, `WriteSecured`, `WriteSecured2`, and each `*BulkEntry` entry) but rejected on read/event paths. Cross-reference `gateway.md` for the validation rules and expansion details rather than restating them. +**Resolution:** _(2026-06-18)_ Confirmed `docs/Contracts.md` had no mention of `MxSparseArray` / `MxSparseElement` / `sparse_array_value = 19`. Added a new paragraph in the "Files" section immediately after the `mxaccess_gateway.proto` intro sentence (before the bulk-subscription commands section): names `MxSparseArray` alongside `MxValue`, `MxArray`, and `MxStatusProxy` in the intro line; explains that `sparse_array_value = 19` is the `MxValue.kind` oneof arm for write-only partial-array writes; documents both messages with their fields and field numbers; states the default-fill-not-preserve semantics; and enumerates every write variant that accepts it plus the read/event rejection. Cross-references `gateway.md` for expansion rules and validation constraints. + ### Contracts-025 | Field | Value | @@ -538,8 +542,10 @@ Additionally, the `MxSparseElement.value` field is typed `MxValue` (the full val | Severity | Low | | Category | Documentation & comments | | Location | `src/ZB.MOM.WW.MxGateway.Tests/Contracts/GatewayContractInfoTests.cs:14-25` | -| Status | Open | +| Status | Resolved | **Description:** The XML summary on `GatewayContractInfoTests.GatewayProtocolVersion_IsVersionThree` (updated under Contracts-013 resolution to enumerate the alarm and bulk write/read extensions shipped under version 3) does not mention the new `MxSparseArray` / `sparse_array_value = 19` addition, which is also a strictly additive contract change shipped under version 3 without a bump. A reader checking whether a new additive contract feature requires a `GatewayProtocolVersion` bump will look at this test for precedent; finding only the alarm and bulk write/read examples, they cannot tell whether the sparse array addition was also additive-under-3 or was simply omitted by mistake. This is the same class of stale-summary issue as Contracts-013 (which noted the bulk write/read extension was not mentioned after the alarm-only summary). **Recommendation:** Extend the XML summary to list the `MxSparseArray` write ergonomics extension (`MxSparseArray` / `MxSparseElement` + `sparse_array_value = 19` on `MxValue`, plus the suffix-normalization behavior) alongside the alarm and bulk write/read extensions as a third example of a strictly additive change that shipped under version 3 without a bump. Comment-only change; no test logic or version constant changes. + +**Resolution:** _(2026-06-18)_ Confirmed the XML summary on `GatewayProtocolVersion_IsVersionThree` enumerated only the alarm and bulk write/read extensions; the sparse-array addition was missing. Extended the summary to list all three additive-under-version-3 extensions as an ordered enumeration: (1) alarm proto extension; (2) bulk write/read command family; (3) sparse-array write ergonomics (`MxSparseArray` / `MxSparseElement` messages plus `sparse_array_value = 19` on the `MxValue` oneof). Comment-only change; test logic and version constant are unchanged. The full `ProtobufContractRoundTrip|GatewayContractInfo` filter is 54/54 green. diff --git a/code-reviews/Server/findings.md b/code-reviews/Server/findings.md index 2718076..ef23c12 100644 --- a/code-reviews/Server/findings.md +++ b/code-reviews/Server/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-18 | | Commit reviewed | `88915c3` | | Status | Re-reviewed | -| Open findings | 2 | +| Open findings | 0 | ## Checklist coverage @@ -1109,13 +1109,13 @@ Additionally, `GatewayAlarmMonitor.ApplyProviderModeChangeAsync` increments the | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs:976-1000` (`NormalizeOutboundCommand`), `:1085-1095` (`MapCommand` tracking), `gateway.md` (array-write ergonomics section), `clients/*/README.md` | -| Status | Open | +| Status | Resolved | **Description:** The array-suffix `[]` normalization runs only for the single-add commands `AddItem` and `AddItem2` — `NormalizeOutboundCommand` has no case for `AddItemBulk` (or `AddBufferedItem`), and the `MapCommand` tracking switch likewise normalizes only the `AddItem`/`AddItem2` arms (`AddItemBulk` flows through `TrackBulkItems`/`AddBufferedItem` with the raw address). MXAccess requires the `[]` suffix on the AddItem address for an array attribute to register a *write-capable* handle. A client that registers a bare array address via `AddItemBulk` therefore binds a non-write-capable handle, and a later `Write`/`WriteSecured`/sparse write against that handle silently lands on a read-only-ish handle — the exact failure mode this feature fixes for the single-add path. The behavior is inconsistent across the add family, and `gateway.md` / the client READMEs describe normalization as happening "at AddItem time" and explicitly carve out only `ReadBulk`, giving no signal that `AddItemBulk` is excluded. `AddBufferedItem` is lower-risk (buffered/historical read items are not normally written) but is the same gap. **Recommendation:** Either (a) extend `NormalizeOutboundCommand` and the `MapCommand` tracking path to normalize each `AddItemBulk.TagAddresses` entry (and `AddBufferedItem.ItemDefinition`) the same `IsArray`-gated way, keeping the constraint check, the worker bind, and the stored `SessionItemRegistration.TagAddress` consistent; or (b) if bulk-add normalization is intentionally out of scope for this feature, state that explicitly in `gateway.md` and the client READMEs (alongside the existing `ReadBulk` carve-out) so clients know bulk-added array handles must carry the `[]` suffix themselves to be writable. -**Resolution:** _(empty until closed)_ +**Resolution:** 2026-06-18 — Took option (a). Root cause confirmed: `NormalizeOutboundCommand` had no `AddItemBulk`/`AddBufferedItem` case, so the worker bound bare (non-write-capable) array handles for those paths while single-add was already fixed. Added `AddItemBulk` (normalizes each `TagAddresses` entry in place) and `AddBufferedItem` (normalizes `ItemDefinition`) cases to `NormalizeOutboundCommand`; added the matching `AddBufferedItem` normalization to the `TrackCommandReply`/`MapCommand` tracking path (its registration keys off the command's `ItemDefinition`). `AddItemBulk` tracking needs no change — the worker echoes the already-suffixed address back in each `SubscribeResult.TagAddress`, which `TrackBulkItems` stores. Authz is unchanged and consistent: `FilterTagBulkAsync` checks the bare address through `ConstraintEnforcer.ResolveTarget`'s `[]` fallback, mirroring single-add. Updated `gateway.md` and all five client READMEs (dotnet/go/python/rust/java) so the add-family normalization no longer reads as AddItem-only; the `ReadBulk` carve-out stays. Regression tests: `GatewayArrayWriteWiringTests.AddItemBulk_BareArrayAddress_NormalizedOnWireAndInRegistration`, `.AddBufferedItem_BareArrayAddress_NormalizedOnWireAndInRegistration`. ### Server-058 @@ -1124,10 +1124,10 @@ Additionally, `GatewayAlarmMonitor.ApplyProviderModeChangeAsync` increments the | Severity | Low | | Category | Testing coverage | | Location | `src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** The new `ConstraintEnforcer.ResolveTarget` `[]`-suffix fallback is security-sensitive: it turns a bare-array address that previously missed the Galaxy index (→ spurious `tag_metadata` deny) into a real scope/classification decision. The added tests cover the in-scope-allow case (`CheckReadTagAsync_WithBareArrayName_ResolvesViaArraySuffixFallback`) and the missing/non-array negative case (`CheckReadTagAsync_WithMissingNonArrayName_StillFailsToResolve`), but **no test asserts that a bare-array name which now resolves via the fallback is still denied when it is out of the key's read/write scope** — i.e. that the fallback widened resolution but not authorization. There is also no `CheckWriteHandleAsync` test exercising an array handle whose `SessionItemRegistration.TagAddress` is the suffixed form (`Obj.Arr[]`) resolving directly through `ResolveTarget` for the write-scope and `MaxWriteClassification` checks. These are the precise paths a regression could silently widen. **Recommendation:** Add a `CheckReadTagAsync` (and a `CheckWriteHandleAsync`) case where the bare/suffixed array attribute resolves but the configured `ReadTagGlobs`/`WriteSubtrees` exclude it, asserting a `read_scope`/`write_scope` `ConstraintFailure` is still returned; and a `CheckWriteHandleAsync` case asserting `MaxWriteClassification` is enforced against the array attribute's `SecurityClassification` via the suffixed registration address. -**Resolution:** _(empty until closed)_ +**Resolution:** 2026-06-18 — Added three `ConstraintEnforcerTests` cases (the test fixture gained a second array attribute `Pump_001.Setpoints[]` with `SecurityClassification = 2` to exercise the classification path): `CheckReadTagAsync_WithBareArrayName_OutOfScope_StillDeniedReadScope` (bare array resolves via the `[]` fallback but is denied `read_scope` when out of `ReadTagGlobs` — guards that the fallback widened resolution, not authorization), `CheckWriteHandleAsync_WithSuffixedArrayRegistration_OutOfScope_StillDeniedWriteScope` (an array handle whose registration `TagAddress` is the suffixed `Pump_001.Levels[]` resolves through `ResolveTarget` and is denied `write_scope`), and `CheckWriteHandleAsync_WithSuffixedArrayRegistration_ClassificationTooHigh_StillDenied` (in-scope suffixed array handle denied `max_write_classification` when the attribute's `SecurityClassification` exceeds `MaxWriteClassification`). Tests-only change; no production code touched. diff --git a/code-reviews/Tests/findings.md b/code-reviews/Tests/findings.md index 865f747..a65a497 100644 --- a/code-reviews/Tests/findings.md +++ b/code-reviews/Tests/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-18 | | Commit reviewed | `88915c3` | | Status | Re-reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -749,8 +749,10 @@ The cancellation tests for `WorkerClient` in `WorkerClientTests` *do* exercise t | Severity | Low | | Category | Testing coverage | | Location | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/GatewayArrayWriteWiringTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** `GatewayArrayWriteWiringTests` covers two of eight sparse-write arms in `GatewaySession.NormalizeOutboundCommand`: `Write` (single) and `WriteBulk`. The remaining six arms — `WriteSecured`, `Write2`, `WriteSecured2`, `Write2Bulk`, `WriteSecuredBulk`, `WriteSecured2Bulk` — all call `ExpandValue(entry.Value)` through the same switch, but no wiring test exercises them through `GatewaySession.InvokeAsync`. The class summary says it covers "the single outbound choke point" and that "no sparse value is ever forwarded"; the claim is accurate for the two covered variants but unverified for the other six. A regression accidentally dropping one of the six remaining `case` arms, or shifting a brace so a case falls through to the default (no-op), would pass the entire `GatewayArrayWriteWiringTests` suite. **Recommendation:** Add one wiring test per uncovered variant (or a single `[Theory]` over the six command kinds), constructing the matching command type with a `SparseArrayValue` and asserting `worker.LastCommand!.Command..Value.KindCase == MxValue.KindOneofCase.ArrayValue` after `session.InvokeAsync`. The `SparseArrayExpanderTests` already pin the expander logic exhaustively; the wiring tests need only check that the choke point invokes expansion for each variant, not the expansion semantics themselves. The four secured variants (`WriteSecured`, `Write2`, `WriteSecured2`, `WriteSecured2Bulk`) can reuse the same `CapturingWorkerClient` stub. + +**Resolution:** 2026-06-18 — root cause confirmed: the six arms (`WriteSecured`, `Write2`, `WriteSecured2`, `Write2Bulk`, `WriteSecuredBulk`, `WriteSecured2Bulk`) each had a `case` in `NormalizeOutboundCommand` calling `ExpandValue` but no wiring test. Server-057's additions (`AddItemBulk`, `AddBufferedItem`) covered address-normalization tests only, not the missing write-expansion variants. Added six tests to `GatewayArrayWriteWiringTests.cs` — one per uncovered arm — each constructing the matching command with a 4-element `SparseArrayValue` (Integer, single element set), driving it through `GatewaySession.InvokeAsync`, and asserting `worker.LastCommand.Command..Value.KindCase == ArrayValue` and the expected element positions. Tests: `WriteSecured_SparseArrayValue_ExpandedBeforeReachingWorker`, `Write2_SparseArrayValue_ExpandedBeforeReachingWorker`, `WriteSecured2_SparseArrayValue_ExpandedBeforeReachingWorker`, `Write2Bulk_SparseArrayEntryValue_ExpandedBeforeReachingWorker`, `WriteSecuredBulk_SparseArrayEntryValue_ExpandedBeforeReachingWorker`, `WriteSecured2Bulk_SparseArrayEntryValue_ExpandedBeforeReachingWorker`. All 13 `GatewayArrayWriteWiring` tests pass (7 pre-existing + 6 new). diff --git a/docs/Contracts.md b/docs/Contracts.md index 0d19ee6..1690ab7 100644 --- a/docs/Contracts.md +++ b/docs/Contracts.md @@ -8,7 +8,24 @@ recreated by the contracts project build. `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto` defines the public `MxAccessGateway` gRPC service, command payloads, command replies, event DTOs, -`MxValue`, `MxArray`, and `MxStatusProxy`. +`MxValue`, `MxArray`, `MxSparseArray`, and `MxStatusProxy`. + +`MxValue` carries a `kind` oneof with arms for all scalar and array types. One +arm is `sparse_array_value = 19` (field number 19), which carries an +`MxSparseArray`. `MxSparseArray` is a write-only value type: the gateway accepts +it on every write variant (`Write`, `Write2`, `WriteSecured`, `WriteSecured2`, +and the corresponding `*BulkEntry` shapes), expands it into a full, +default-filled `MxArray` before forwarding to the worker, and rejects it on +read or event paths. The worker never receives or produces it. + +`MxSparseArray` has three fields: `element_data_type` (1, the `MxDataType` of +every element), `total_length` (2, the length of the expanded full array), and +`elements` (3, `repeated MxSparseElement`). Each `MxSparseElement` has `index` +(1, zero-based position in the expanded array) and `value` (2, a scalar +`MxValue`). Indices not mentioned in `elements` take the element type's default +value — they are reset, not preserved. See `gateway.md` section +"MxSparseArray — default-fill partial array writes" for the expansion rules, +validation constraints, and the scope requirements per write variant. The public command model includes bulk subscription command kinds for `AddItemBulk`, `AdviseItemBulk`, `RemoveItemBulk`, `UnAdviseItemBulk`, diff --git a/gateway.md b/gateway.md index dd3c706..ddad5f7 100644 --- a/gateway.md +++ b/gateway.md @@ -1124,14 +1124,17 @@ Known important parity areas from existing captures: representation (unmentioned indices → type default) before sending the whole-array write to the worker. - Array attribute addresses require the `[]` body suffix to be write-capable. - The gateway normalizes bare-name addresses at `AddItem` time: when Galaxy - metadata confirms `is_array`, the gateway appends `[]` before registering the - handle with the worker. When metadata is unavailable or the address is not - recognized as an array, the address is forwarded unchanged so existing - behavior is not regressed. The normalized address is stored in - `SessionItemRegistration.TagAddress` and applies consistently to all - subsequent writes on that handle. `ReadBulk` is unaffected — it uses raw - address strings with its own ephemeral registration. + The gateway normalizes bare-name addresses at add-item time across the whole + add family — single `AddItem`/`AddItem2`, the batched `AddItemBulk`, and + `AddBufferedItem`: when Galaxy metadata confirms `is_array`, the gateway + appends `[]` before registering the handle with the worker. When metadata is + unavailable or the address is not recognized as an array, the address is + forwarded unchanged so existing behavior is not regressed. The normalized + address is stored in `SessionItemRegistration.TagAddress` (for `AddItemBulk` + the worker echoes the suffixed address it bound back in each + `SubscribeResult.TagAddress`) and applies consistently to all subsequent + writes on that handle. `ReadBulk` is unaffected — it uses raw address strings + with its own ephemeral registration. The gateway should not "fix" these behaviors unless the client explicitly opts into a non-parity mode. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs index 67127e7..25d96a3 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs @@ -987,6 +987,19 @@ public sealed class GatewaySession break; case MxCommand.PayloadOneofCase.AddItem2: command.AddItem2.ItemDefinition = NormalizeAddress(command.AddItem2.ItemDefinition); + break; + case MxCommand.PayloadOneofCase.AddBufferedItem: + command.AddBufferedItem.ItemDefinition = NormalizeAddress(command.AddBufferedItem.ItemDefinition); + break; + case MxCommand.PayloadOneofCase.AddItemBulk: + // Normalize each bare array address in place so the worker binds a write-capable handle + // for every array tag in the batch (the same IsArray-gated rewrite the single-add path + // applies). Scalar addresses pass through unchanged. + for (int i = 0; i < command.AddItemBulk.TagAddresses.Count; i++) + { + command.AddItemBulk.TagAddresses[i] = NormalizeAddress(command.AddItemBulk.TagAddresses[i]); + } + break; case MxCommand.PayloadOneofCase.Write: ExpandValue(command.Write.Value); @@ -1089,9 +1102,16 @@ public sealed class GatewaySession TrackItem(command.AddItem2.ServerHandle, reply.AddItem2.ItemHandle, NormalizeAddress(command.AddItem2.ItemDefinition)); break; case MxCommandKind.AddBufferedItem when reply.AddBufferedItem is not null: - TrackItem(command.AddBufferedItem.ServerHandle, reply.AddBufferedItem.ItemHandle, command.AddBufferedItem.ItemDefinition); + // The reply carries no address, so tracking keys off the command's ItemDefinition; + // re-apply the array-suffix normalization (the tracking copy is a separate, un-mutated + // instance from the one forwarded at the InvokeAsync choke point) so the registration + // matches the write-capable handle the worker bound. + TrackItem(command.AddBufferedItem.ServerHandle, reply.AddBufferedItem.ItemHandle, NormalizeAddress(command.AddBufferedItem.ItemDefinition)); break; case MxCommandKind.AddItemBulk when reply.AddItemBulk is not null: + // The worker echoes back the (already-normalized) address it bound in each + // SubscribeResult.TagAddress, so TrackBulkItems stores the suffixed array address + // without re-normalizing here. TrackBulkItems(reply.AddItemBulk); break; case MxCommandKind.SubscribeBulk when reply.SubscribeBulk is not null: diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Contracts/GatewayContractInfoTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Contracts/GatewayContractInfoTests.cs index cf371e5..2e0da2b 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Contracts/GatewayContractInfoTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Contracts/GatewayContractInfoTests.cs @@ -13,15 +13,18 @@ public sealed class GatewayContractInfoTests /// /// Pins the current - /// constant at 3. Both the alarm proto extension (`AcknowledgeAlarm` / - /// `QueryActiveAlarms` RPCs, the `OnAlarmTransitionEvent` body, and the - /// alarm command/reply payload cases) and the bulk write/read command - /// family extension (`WriteBulk` / `Write2Bulk` / `WriteSecuredBulk` / - /// `WriteSecured2Bulk` / `ReadBulk` plus their `BulkWriteReply` and - /// `BulkReadReply` payloads) shipped under version 3 — both were strictly - /// additive contract changes, so neither required a further bump. A - /// future breaking contract change should bump this constant and update - /// this test in lock-step. + /// constant at 3. All three of the following extensions shipped under + /// version 3 as strictly additive contract changes that did not require a + /// further bump: (1) the alarm proto extension (AcknowledgeAlarm / + /// QueryActiveAlarms RPCs, the OnAlarmTransitionEvent body, + /// and the alarm command/reply payload cases); (2) the bulk write/read + /// command family (WriteBulk / Write2Bulk / + /// WriteSecuredBulk / WriteSecured2Bulk / ReadBulk + /// plus their BulkWriteReply and BulkReadReply payloads); + /// (3) the sparse-array write ergonomics extension (MxSparseArray / + /// MxSparseElement messages plus the sparse_array_value = 19 + /// arm on the MxValue oneof). A future breaking contract change + /// should bump this constant and update this test in lock-step. /// [Fact] public void GatewayProtocolVersion_IsVersionThree() diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs index 1bb9069..411dc3a 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs @@ -1588,4 +1588,87 @@ public sealed class ProtobufContractRoundTripTests Assert.Equal(150UL, parsed.ReplayGap.RequestedAfterSequence); Assert.Equal(200UL, parsed.ReplayGap.OldestAvailableSequence); } + + /// + /// Pins the wire field number for MxValue.sparse_array_value (19) + /// via the generated constant, pins the and + /// field numbers by name + number against the + /// descriptor, and round-trips an carrying a + /// with at least one + /// so a future renumber or type-narrowing is caught at the contract level. + /// Also verifies that an all-defaults (no elements) + /// is not a proto-level error. See Contracts-023. + /// + [Fact] + public void MxValue_RoundTripsSparseArrayValueAndPinsFieldNumbers() + { + // Pin MxValue.sparse_array_value wire field number == 19. + Assert.Equal(19, MxValue.SparseArrayValueFieldNumber); + + // Pin MxSparseArray field numbers by name + number. + var sparseArrayFields = MxSparseArray.Descriptor.Fields; + Assert.Equal(1, sparseArrayFields[MxSparseArray.ElementDataTypeFieldNumber].FieldNumber); + Assert.Equal("element_data_type", sparseArrayFields[MxSparseArray.ElementDataTypeFieldNumber].Name); + Assert.Equal(2, sparseArrayFields[MxSparseArray.TotalLengthFieldNumber].FieldNumber); + Assert.Equal("total_length", sparseArrayFields[MxSparseArray.TotalLengthFieldNumber].Name); + Assert.Equal(3, sparseArrayFields[MxSparseArray.ElementsFieldNumber].FieldNumber); + Assert.Equal("elements", sparseArrayFields[MxSparseArray.ElementsFieldNumber].Name); + + // Pin MxSparseElement field numbers by name + number. + var sparseElementFields = MxSparseElement.Descriptor.Fields; + Assert.Equal(1, sparseElementFields[MxSparseElement.IndexFieldNumber].FieldNumber); + Assert.Equal("index", sparseElementFields[MxSparseElement.IndexFieldNumber].Name); + Assert.Equal(2, sparseElementFields[MxSparseElement.ValueFieldNumber].FieldNumber); + Assert.Equal("value", sparseElementFields[MxSparseElement.ValueFieldNumber].Name); + + // Round-trip an MxValue with a populated MxSparseArray (one scalar element). + var original = new MxValue + { + DataType = MxDataType.Float, + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Float, + TotalLength = 4, + Elements = + { + new MxSparseElement + { + Index = 2, + Value = new MxValue + { + DataType = MxDataType.Float, + FloatValue = 3.14f, + VariantType = "VT_R4", + }, + }, + }, + }, + }; + + var parsed = MxValue.Parser.ParseFrom(original.ToByteArray()); + + Assert.Equal(original, parsed); + Assert.Equal(MxValue.KindOneofCase.SparseArrayValue, parsed.KindCase); + Assert.NotNull(parsed.SparseArrayValue); + Assert.Equal(MxDataType.Float, parsed.SparseArrayValue.ElementDataType); + Assert.Equal(4u, parsed.SparseArrayValue.TotalLength); + var element = Assert.Single(parsed.SparseArrayValue.Elements); + Assert.Equal(2u, element.Index); + Assert.Equal(3.14f, element.Value.FloatValue); + + // An all-defaults MxSparseArray (no elements) is not a proto-level error. + var empty = new MxValue + { + DataType = MxDataType.Float, + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Float, + TotalLength = 10, + }, + }; + var parsedEmpty = MxValue.Parser.ParseFrom(empty.ToByteArray()); + Assert.Equal(empty, parsedEmpty); + Assert.Equal(MxValue.KindOneofCase.SparseArrayValue, parsedEmpty.KindCase); + Assert.Empty(parsedEmpty.SparseArrayValue.Elements); + } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/GatewayArrayWriteWiringTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/GatewayArrayWriteWiringTests.cs index 36424da..208e0e5 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/GatewayArrayWriteWiringTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/GatewayArrayWriteWiringTests.cs @@ -247,6 +247,440 @@ public sealed class GatewayArrayWriteWiringTests Assert.Equal(new[] { 0, 7, 0, 0 }, forwarded.ArrayValue.Int32Values.Values); } + /// + /// A bare array address added via AddItemBulk is normalized to its writable array form + /// on the wire (so the worker registers a write-capable handle), while a scalar address in the + /// same batch is forwarded unchanged. Tracking the worker's echoed reply lands the normalized + /// address in the . + /// + [Fact] + public async Task AddItemBulk_BareArrayAddress_NormalizedOnWireAndInRegistration() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.AddItemBulk, + AddItemBulk = new AddItemBulkCommand + { + ServerHandle = 1, + TagAddresses = { "Obj.Arr", "Obj.Scalar" }, + }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + // The array address gains the writable "[]" suffix; the scalar passes through unchanged. + Assert.NotNull(worker.LastCommand); + Assert.Equal( + new[] { "Obj.Arr[]", "Obj.Scalar" }, + worker.LastCommand!.Command.AddItemBulk.TagAddresses); + + // The real worker echoes back the (suffixed) address it bound; tracking the reply must land the + // normalized address in the registration so a later write resolves the write-capable handle. + MxCommand trackingCopy = new() + { + Kind = MxCommandKind.AddItemBulk, + AddItemBulk = new AddItemBulkCommand + { + ServerHandle = 1, + TagAddresses = { "Obj.Arr", "Obj.Scalar" }, + }, + }; + MxCommandReply reply = new() + { + Kind = MxCommandKind.AddItemBulk, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + AddItemBulk = new BulkSubscribeReply + { + Results = + { + new SubscribeResult + { + ServerHandle = 1, + ItemHandle = 50, + TagAddress = "Obj.Arr[]", + WasSuccessful = true, + }, + new SubscribeResult + { + ServerHandle = 1, + ItemHandle = 51, + TagAddress = "Obj.Scalar", + WasSuccessful = true, + }, + }, + }, + }; + session.TrackCommandReply(trackingCopy, reply); + + Assert.True(session.TryGetItemRegistration(1, 50, out SessionItemRegistration arrayRegistration)); + Assert.Equal("Obj.Arr[]", arrayRegistration.TagAddress); + Assert.True(session.TryGetItemRegistration(1, 51, out SessionItemRegistration scalarRegistration)); + Assert.Equal("Obj.Scalar", scalarRegistration.TagAddress); + } + + /// + /// A sparse-array value is expanded to a full, + /// default-filled before reaching the worker; the secured variant's + /// case arm in NormalizeOutboundCommand is wired to the expander. + /// + [Fact] + public async Task WriteSecured_SparseArrayValue_ExpandedBeforeReachingWorker() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.WriteSecured, + WriteSecured = new WriteSecuredCommand + { + ServerHandle = 1, + ItemHandle = 42, + Value = new MxValue + { + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Integer, + TotalLength = 4, + Elements = + { + new MxSparseElement + { + Index = 2, + Value = new MxValue { Int32Value = 9 }, + }, + }, + }, + }, + }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + MxValue forwarded = worker.LastCommand!.Command.WriteSecured.Value; + Assert.Equal(MxValue.KindOneofCase.ArrayValue, forwarded.KindCase); + Assert.Equal(new[] { 0, 0, 9, 0 }, forwarded.ArrayValue.Int32Values.Values); + } + + /// + /// A sparse-array (timestamped) value is expanded to a full, + /// default-filled before reaching the worker; the Write2 variant's + /// case arm in NormalizeOutboundCommand is wired to the expander. + /// + [Fact] + public async Task Write2_SparseArrayValue_ExpandedBeforeReachingWorker() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.Write2, + Write2 = new Write2Command + { + ServerHandle = 1, + ItemHandle = 42, + Value = new MxValue + { + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Integer, + TotalLength = 4, + Elements = + { + new MxSparseElement + { + Index = 3, + Value = new MxValue { Int32Value = 5 }, + }, + }, + }, + }, + }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + MxValue forwarded = worker.LastCommand!.Command.Write2.Value; + Assert.Equal(MxValue.KindOneofCase.ArrayValue, forwarded.KindCase); + Assert.Equal(new[] { 0, 0, 0, 5 }, forwarded.ArrayValue.Int32Values.Values); + } + + /// + /// A sparse-array (timestamped secured) value is expanded + /// to a full, default-filled before reaching the worker; the + /// WriteSecured2 variant's case arm in NormalizeOutboundCommand is wired to + /// the expander. + /// + [Fact] + public async Task WriteSecured2_SparseArrayValue_ExpandedBeforeReachingWorker() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.WriteSecured2, + WriteSecured2 = new WriteSecured2Command + { + ServerHandle = 1, + ItemHandle = 42, + Value = new MxValue + { + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Integer, + TotalLength = 4, + Elements = + { + new MxSparseElement + { + Index = 0, + Value = new MxValue { Int32Value = 3 }, + }, + }, + }, + }, + }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + MxValue forwarded = worker.LastCommand!.Command.WriteSecured2.Value; + Assert.Equal(MxValue.KindOneofCase.ArrayValue, forwarded.KindCase); + Assert.Equal(new[] { 3, 0, 0, 0 }, forwarded.ArrayValue.Int32Values.Values); + } + + /// + /// A sparse-array entry in a (timestamped) is expanded to a + /// full, default-filled before reaching the worker; the Write2Bulk + /// variant's case arm in NormalizeOutboundCommand is wired to the expander. + /// + [Fact] + public async Task Write2Bulk_SparseArrayEntryValue_ExpandedBeforeReachingWorker() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.Write2Bulk, + Write2Bulk = new Write2BulkCommand + { + ServerHandle = 1, + Entries = + { + new Write2BulkEntry + { + ItemHandle = 42, + Value = new MxValue + { + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Integer, + TotalLength = 4, + Elements = + { + new MxSparseElement + { + Index = 1, + Value = new MxValue { Int32Value = 11 }, + }, + }, + }, + }, + }, + }, + }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + MxValue forwarded = worker.LastCommand!.Command.Write2Bulk.Entries[0].Value; + Assert.Equal(MxValue.KindOneofCase.ArrayValue, forwarded.KindCase); + Assert.Equal(new[] { 0, 11, 0, 0 }, forwarded.ArrayValue.Int32Values.Values); + } + + /// + /// A sparse-array entry in a is expanded to a full, + /// default-filled before reaching the worker; the WriteSecuredBulk + /// variant's case arm in NormalizeOutboundCommand is wired to the expander. + /// + [Fact] + public async Task WriteSecuredBulk_SparseArrayEntryValue_ExpandedBeforeReachingWorker() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.WriteSecuredBulk, + WriteSecuredBulk = new WriteSecuredBulkCommand + { + ServerHandle = 1, + Entries = + { + new WriteSecuredBulkEntry + { + ItemHandle = 42, + Value = new MxValue + { + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Integer, + TotalLength = 4, + Elements = + { + new MxSparseElement + { + Index = 2, + Value = new MxValue { Int32Value = 13 }, + }, + }, + }, + }, + }, + }, + }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + MxValue forwarded = worker.LastCommand!.Command.WriteSecuredBulk.Entries[0].Value; + Assert.Equal(MxValue.KindOneofCase.ArrayValue, forwarded.KindCase); + Assert.Equal(new[] { 0, 0, 13, 0 }, forwarded.ArrayValue.Int32Values.Values); + } + + /// + /// A sparse-array entry in a (timestamped secured) is + /// expanded to a full, default-filled before reaching the worker; the + /// WriteSecured2Bulk variant's case arm in NormalizeOutboundCommand is wired to + /// the expander. + /// + [Fact] + public async Task WriteSecured2Bulk_SparseArrayEntryValue_ExpandedBeforeReachingWorker() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.WriteSecured2Bulk, + WriteSecured2Bulk = new WriteSecured2BulkCommand + { + ServerHandle = 1, + Entries = + { + new WriteSecured2BulkEntry + { + ItemHandle = 42, + Value = new MxValue + { + SparseArrayValue = new MxSparseArray + { + ElementDataType = MxDataType.Integer, + TotalLength = 4, + Elements = + { + new MxSparseElement + { + Index = 3, + Value = new MxValue { Int32Value = 17 }, + }, + }, + }, + }, + }, + }, + }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + MxValue forwarded = worker.LastCommand!.Command.WriteSecured2Bulk.Entries[0].Value; + Assert.Equal(MxValue.KindOneofCase.ArrayValue, forwarded.KindCase); + Assert.Equal(new[] { 0, 0, 0, 17 }, forwarded.ArrayValue.Int32Values.Values); + } + + /// + /// A bare array address added via AddBufferedItem is normalized to its writable array + /// form on the wire and in the tracked . + /// + [Fact] + public async Task AddBufferedItem_BareArrayAddress_NormalizedOnWireAndInRegistration() + { + CapturingWorkerClient worker = new(); + GatewaySession session = CreateReadySession(worker); + + WorkerCommand command = new() + { + Command = new MxCommand + { + Kind = MxCommandKind.AddBufferedItem, + AddBufferedItem = new AddBufferedItemCommand + { + ServerHandle = 1, + ItemDefinition = "Obj.Arr", + }, + }, + }; + + worker.NextReply = new WorkerCommandReply + { + Reply = new MxCommandReply + { + Kind = MxCommandKind.AddBufferedItem, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + AddBufferedItem = new AddBufferedItemReply { ItemHandle = 60 }, + }, + }; + + await session.InvokeAsync(command, CancellationToken.None); + + Assert.NotNull(worker.LastCommand); + Assert.Equal("Obj.Arr[]", worker.LastCommand!.Command.AddBufferedItem.ItemDefinition); + + // AddBufferedItem tracking keys off the command's ItemDefinition (the reply carries no address), + // so the tracking-path normalization must run for the registration to match the bound handle. + MxCommand trackingCopy = new() + { + Kind = MxCommandKind.AddBufferedItem, + AddBufferedItem = new AddBufferedItemCommand + { + ServerHandle = 1, + ItemDefinition = "Obj.Arr", + }, + }; + session.TrackCommandReply(trackingCopy, worker.NextReply.Reply); + + Assert.True(session.TryGetItemRegistration(1, 60, out SessionItemRegistration registration)); + Assert.Equal("Obj.Arr[]", registration.TagAddress); + } + private static GatewaySession CreateReadySession(IWorkerClient workerClient) { GatewaySession session = new( diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs index 69ee056..b79df1e 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs @@ -263,6 +263,116 @@ public sealed class ConstraintEnforcerTests Assert.Equal("tag_metadata", unknown.ConstraintName); } + /// + /// The []-suffix fallback widened *resolution* of a bare array name, not *authorization*: + /// a bare array attribute that resolves through the fallback but is outside the key's read scope + /// must still be denied with a read_scope failure (Server-058). + /// + [Fact] + public async Task CheckReadTagAsync_WithBareArrayName_OutOfScope_StillDeniedReadScope() + { + ConstraintEnforcer enforcer = CreateEnforcer(out _); + ApiKeyIdentity identity = CreateIdentity(ApiKeyConstraints.Empty with + { + // Scope covers a different object, so the resolved array attribute is out of scope. + ReadTagGlobs = ["Other_001.*"], + }); + + ConstraintFailure? failure = await enforcer.CheckReadTagAsync( + identity, + "Pump_001.Levels", + CancellationToken.None); + + Assert.NotNull(failure); + Assert.Equal("read_scope", failure.ConstraintName); + } + + /// + /// A write against an array handle whose registration carries the suffixed form ("Pump_001.Levels[]") + /// resolves through ResolveTarget and is denied with a write_scope failure when the + /// array attribute is outside the key's write scope (Server-058). + /// + [Fact] + public async Task CheckWriteHandleAsync_WithSuffixedArrayRegistration_OutOfScope_StillDeniedWriteScope() + { + ConstraintEnforcer enforcer = CreateEnforcer(out _); + ApiKeyIdentity identity = CreateIdentity(ApiKeyConstraints.Empty with + { + WriteSubtrees = ["Other/*"], + }); + GatewaySession session = CreateSession(); + session.TrackCommandReply( + new MxCommand + { + Kind = MxCommandKind.AddItem, + AddItem = new AddItemCommand + { + ServerHandle = 12, + // The worker-bound, suffixed array address is what lands in the registration. + ItemDefinition = "Pump_001.Levels[]", + }, + }, + new MxCommandReply + { + ProtocolStatus = ZB.MOM.WW.MxGateway.Server.Grpc.MxAccessGrpcMapper.Ok(), + AddItem = new AddItemReply { ItemHandle = 70 }, + }); + + ConstraintFailure? failure = await enforcer.CheckWriteHandleAsync( + identity, + session, + serverHandle: 12, + itemHandle: 70, + CancellationToken.None); + + Assert.NotNull(failure); + Assert.Equal("write_scope", failure.ConstraintName); + } + + /// + /// A write against an in-scope array handle whose registration carries the suffixed form is still + /// denied when the array attribute's SecurityClassification exceeds the key's + /// MaxWriteClassification, resolved through ResolveTarget via the suffixed address + /// (Server-058). + /// + [Fact] + public async Task CheckWriteHandleAsync_WithSuffixedArrayRegistration_ClassificationTooHigh_StillDenied() + { + ConstraintEnforcer enforcer = CreateEnforcer(out _); + ApiKeyIdentity identity = CreateIdentity(ApiKeyConstraints.Empty with + { + // In scope for the array attribute, but the array's classification (2) exceeds the cap (1). + WriteTagGlobs = ["Pump_001.*"], + MaxWriteClassification = 1, + }); + GatewaySession session = CreateSession(); + session.TrackCommandReply( + new MxCommand + { + Kind = MxCommandKind.AddItem, + AddItem = new AddItemCommand + { + ServerHandle = 12, + ItemDefinition = "Pump_001.Setpoints[]", + }, + }, + new MxCommandReply + { + ProtocolStatus = ZB.MOM.WW.MxGateway.Server.Grpc.MxAccessGrpcMapper.Ok(), + AddItem = new AddItemReply { ItemHandle = 71 }, + }); + + ConstraintFailure? failure = await enforcer.CheckWriteHandleAsync( + identity, + session, + serverHandle: 12, + itemHandle: 71, + CancellationToken.None); + + Assert.NotNull(failure); + Assert.Equal("max_write_classification", failure.ConstraintName); + } + private static ConstraintEnforcer CreateEnforcer(out FakeAuditWriter auditWriter) { auditWriter = new FakeAuditWriter(); @@ -340,6 +450,16 @@ public sealed class ConstraintEnforcerTests FullTagReference = "Pump_001.Levels[]", IsArray = true, }, + new GalaxyAttribute + { + // A second array attribute carrying a non-zero security classification so + // the MaxWriteClassification check can be exercised via a suffixed + // registration address resolving through ResolveTarget. + AttributeName = "Setpoints", + FullTagReference = "Pump_001.Setpoints[]", + IsArray = true, + SecurityClassification = 2, + }, }, }, new GalaxyObject