diff --git a/code-reviews/Contracts/findings.md b/code-reviews/Contracts/findings.md index 9536a6c..5353879 100644 --- a/code-reviews/Contracts/findings.md +++ b/code-reviews/Contracts/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-24 | | Commit reviewed | `d692232` | | Status | Reviewed | -| Open findings | 2 | +| Open findings | 0 | ## Checklist coverage @@ -284,13 +284,13 @@ Python and Go descriptors. No fields renumbered or repurposed. | Severity | Low | | Category | Code organization & conventions | | Location | `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:31-41` (`QueryActiveAlarmsRequest`) | -| Status | Open | +| Status | Resolved | **Description:** The new public message `QueryActiveAlarmsRequest` (added in commit `397d3c5`) has `session_id = 1` with a comment "session_id is currently unused (the snapshot is session-less) but reserved so a future per-session view can be added without a wire break." The field is not marked `reserved` and clients are not told whether to populate it. As shipped today the server-side implementation (`MxAccessGatewayService.QueryActiveAlarms`) ignores it, but a Java/Rust/Go/Python client author reading the proto alone can't tell whether to leave it empty or write the caller's session id. **Recommendation:** Either (a) tighten the comment to "Clients may leave `session_id` empty; the gateway currently ignores it and serves the session-less central-monitor cache. A future version may use it to scope the snapshot to one session." — making the "currently-ignored" semantic unambiguous — or (b) remove `session_id` and use `reserved 1; reserved "session_id";` until the per-session view actually exists. Option (a) is cheaper and preserves a forward-compat hint. -**Resolution:** _(empty until closed)_ +**Resolution:** _(2026-05-24)_ Took the finding's recommended option (a) — comment-only, no wire-format change. Confirmed the ambiguity by reading lines 37-39 of `mxaccess_gateway.proto` at `d2d2e5f`: the prior wording said the field was "reserved" while remaining a populated `string session_id = 1` (the protobuf `reserved` keyword was never used), so a client author could reasonably read it either way. Reworded the header comment on `QueryActiveAlarmsRequest` to verbatim "Clients may leave `session_id` empty; the gateway currently ignores it and serves the session-less central-monitor cache. A future version may use it to scope the snapshot to one session." — making the "currently-ignored" semantic unambiguous, removing the misleading use of "reserved" (which now only describes the protobuf keyword in this file), and preserving the forward-compatibility hint. The field, its number (1), and `client_correlation_id = 2` / `alarm_filter_prefix = 3` are unchanged — the additive-only contract rule is honoured. Added regression test `ProtobufContractRoundTripTests.QueryActiveAlarmsRequest_PinsFieldNumbersAndRoundTripsPrefixFilter` which pins the three field numbers (`session_id = 1`, `client_correlation_id = 2`, `alarm_filter_prefix = 3`) by name + number against the descriptor, ensuring a future editor cannot quietly renumber. `dotnet build src/ZB.MOM.WW.MxGateway.slnx` is green (0 warnings, 0 errors); the new test plus the existing 39 `ProtobufContractRoundTripTests` are 40 / 40 green. ### Contracts-017 @@ -299,10 +299,10 @@ Python and Go descriptors. No fields renumbered or repurposed. | Severity | Low | | Category | Documentation & comments | | Location | `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:23-29` (the `rpc QueryActiveAlarms` block) | -| Status | Open | +| Status | Resolved | **Description:** The RPC comment on `QueryActiveAlarms` describes the stream order ("Point-in-time snapshot of the currently-active alarm set served from the gateway's always-on alarm monitor cache") and the session-less semantic, but does not mention that `QueryActiveAlarmsRequest.alarm_filter_prefix` narrows the snapshot by a `StartsWith(reference)` match on `alarm_full_reference`. A client author reading the RPC comment alone cannot discover the filter capability without inspecting the request message. **Recommendation:** Extend the RPC comment with one line: "`QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the snapshot to alarms whose `alarm_full_reference` starts with the given prefix; an empty prefix returns the full set." Comment-only. -**Resolution:** _(empty until closed)_ +**Resolution:** _(2026-05-24)_ Confirmed against `mxaccess_gateway.proto` at `d2d2e5f`: lines 29-34 of the `rpc QueryActiveAlarms` block describe the snapshot order and session-less origin but never mention `alarm_filter_prefix`, which is only documented on the request message itself two lines below — a client author reading the RPC comment alone cannot discover the filter capability. Extended the RPC comment with the finding's recommended one-line addition verbatim: "`QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the snapshot to alarms whose `alarm_full_reference` starts with the given prefix; an empty prefix returns the full set." Comment-only change; the RPC signature, request/reply types, and field numbers are unchanged. The same regression test added under Contracts-016 (`ProtobufContractRoundTripTests.QueryActiveAlarmsRequest_PinsFieldNumbersAndRoundTripsPrefixFilter`) also exercises a round-trip of `QueryActiveAlarmsRequest` with `alarm_filter_prefix` populated and pins `QueryActiveAlarms` on the public `MxAccessGateway` service surface, so the RPC + filter field combination the new comment documents is wire-stable. `dotnet build src/ZB.MOM.WW.MxGateway.slnx` is green (0 warnings, 0 errors). diff --git a/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGateway.cs b/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGateway.cs index a337eda..fc3df94 100644 --- a/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGateway.cs +++ b/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGateway.cs @@ -735,9 +735,10 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto { #region Messages /// - /// Public request shape for QueryActiveAlarms. session_id is currently unused - /// (the snapshot is session-less) but reserved so a future per-session view - /// can be added without a wire break. + /// Public request shape for QueryActiveAlarms. + /// Clients may leave `session_id` empty; the gateway currently ignores it and + /// serves the session-less central-monitor cache. A future version may use it + /// to scope the snapshot to one session. /// [global::System.Diagnostics.DebuggerDisplayAttribute("{ToString(),nq}")] public sealed partial class QueryActiveAlarmsRequest : pb::IMessage diff --git a/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGatewayGrpc.cs b/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGatewayGrpc.cs index 21fefc2..24c98bc 100644 --- a/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGatewayGrpc.cs +++ b/src/ZB.MOM.WW.MxGateway.Contracts/Generated/MxaccessGatewayGrpc.cs @@ -196,6 +196,9 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto { /// reconnect to seed Part 9 client state, or to reconcile alarms that may /// have been missed during a transport blip. Streamed so callers can /// begin processing without buffering the full set. + /// `QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the + /// snapshot to alarms whose `alarm_full_reference` starts with the given + /// prefix; an empty prefix returns the full set. /// /// The request received from the client. /// Used for sending responses back to the client. @@ -364,6 +367,9 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto { /// reconnect to seed Part 9 client state, or to reconcile alarms that may /// have been missed during a transport blip. Streamed so callers can /// begin processing without buffering the full set. + /// `QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the + /// snapshot to alarms whose `alarm_full_reference` starts with the given + /// prefix; an empty prefix returns the full set. /// /// The request to send to the server. /// The initial metadata to send with the call. This parameter is optional. @@ -381,6 +387,9 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto { /// reconnect to seed Part 9 client state, or to reconcile alarms that may /// have been missed during a transport blip. Streamed so callers can /// begin processing without buffering the full set. + /// `QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the + /// snapshot to alarms whose `alarm_full_reference` starts with the given + /// prefix; an empty prefix returns the full set. /// /// The request to send to the server. /// The options for the call. diff --git a/src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto b/src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto index b1c099b..eeb3b1c 100644 --- a/src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto +++ b/src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto @@ -31,12 +31,16 @@ service MxAccessGateway { // reconnect to seed Part 9 client state, or to reconcile alarms that may // have been missed during a transport blip. Streamed so callers can // begin processing without buffering the full set. + // `QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the + // snapshot to alarms whose `alarm_full_reference` starts with the given + // prefix; an empty prefix returns the full set. rpc QueryActiveAlarms(QueryActiveAlarmsRequest) returns (stream ActiveAlarmSnapshot); } -// Public request shape for QueryActiveAlarms. session_id is currently unused -// (the snapshot is session-less) but reserved so a future per-session view -// can be added without a wire break. +// Public request shape for QueryActiveAlarms. +// Clients may leave `session_id` empty; the gateway currently ignores it and +// serves the session-less central-monitor cache. A future version may use it +// to scope the snapshot to one session. message QueryActiveAlarmsRequest { string session_id = 1; string client_correlation_id = 2; diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs index b940644..7a30081 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs @@ -437,6 +437,46 @@ public sealed class ProtobufContractRoundTripTests Assert.Equal(withFilter, StreamAlarmsRequest.Parser.ParseFrom(withFilter.ToByteArray())); } + /// + /// Verifies that QueryActiveAlarmsRequest pins the additive-only field numbering + /// (session_id = 1, client_correlation_id = 2, alarm_filter_prefix = 3) + /// advertised in its proto comment, that the message round-trips with the optional + /// alarm_filter_prefix populated (the filter semantic the public RPC comment + /// documents), and that QueryActiveAlarms remains on the public service surface. + /// + [Fact] + public void QueryActiveAlarmsRequest_PinsFieldNumbersAndRoundTripsPrefixFilter() + { + var service = Assert.Single( + MxaccessGatewayReflection.Descriptor.Services, + descriptor => descriptor.Name == "MxAccessGateway"); + Assert.Contains(service.Methods, method => method.Name == "QueryActiveAlarms"); + + var fields = QueryActiveAlarmsRequest.Descriptor.Fields; + Assert.Equal(1, fields[QueryActiveAlarmsRequest.SessionIdFieldNumber].FieldNumber); + Assert.Equal(2, fields[QueryActiveAlarmsRequest.ClientCorrelationIdFieldNumber].FieldNumber); + Assert.Equal(3, fields[QueryActiveAlarmsRequest.AlarmFilterPrefixFieldNumber].FieldNumber); + Assert.Equal("session_id", fields[QueryActiveAlarmsRequest.SessionIdFieldNumber].Name); + Assert.Equal("client_correlation_id", fields[QueryActiveAlarmsRequest.ClientCorrelationIdFieldNumber].Name); + Assert.Equal("alarm_filter_prefix", fields[QueryActiveAlarmsRequest.AlarmFilterPrefixFieldNumber].Name); + + var withoutFilter = new QueryActiveAlarmsRequest + { + ClientCorrelationId = "client-correlation-10", + }; + + var withFilter = new QueryActiveAlarmsRequest + { + ClientCorrelationId = "client-correlation-11", + AlarmFilterPrefix = "Tank01.", + }; + + Assert.Equal(withoutFilter, QueryActiveAlarmsRequest.Parser.ParseFrom(withoutFilter.ToByteArray())); + var parsedWithFilter = QueryActiveAlarmsRequest.Parser.ParseFrom(withFilter.ToByteArray()); + Assert.Equal(withFilter, parsedWithFilter); + Assert.Equal("Tank01.", parsedWithFilter.AlarmFilterPrefix); + } + /// Verifies that an MxValue carrying a raw_value bytes payload round-trips. [Fact] public void MxValue_RoundTripsRawValueBytesPayload()