Resolve Contracts-016..017
Contracts-016 (Conventions): QueryActiveAlarmsRequest.session_id header replaced with the unambiguous "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." Removes the ambiguity that the prior "reserved for future use" wording introduced. Contracts-017 (Documentation): The rpc QueryActiveAlarms comment now includes the alarm_filter_prefix description: "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." Both are proto-comment-only changes — no wire-format impact, no field renumbering, and the regenerated MxaccessGateway.cs / MxaccessGatewayGrpc.cs carry only the doc-comment delta. Added the additive-only regression guard QueryActiveAlarmsRequest_PinsFieldNumbersAndRoundTripsPrefixFilter to ProtobufContractRoundTripTests — pins session_id=1 / client_correlation_id=2 / alarm_filter_prefix=3 by descriptor lookup and round-trips the message with and without the filter populated. Verification: dotnet build src/ZB.MOM.WW.MxGateway.slnx clean; ProtobufContractRoundTripTests 40/40 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,7 +7,7 @@
|
|||||||
| Review date | 2026-05-24 |
|
| Review date | 2026-05-24 |
|
||||||
| Commit reviewed | `d692232` |
|
| Commit reviewed | `d692232` |
|
||||||
| Status | Reviewed |
|
| Status | Reviewed |
|
||||||
| Open findings | 2 |
|
| Open findings | 0 |
|
||||||
|
|
||||||
## Checklist coverage
|
## Checklist coverage
|
||||||
|
|
||||||
@@ -284,13 +284,13 @@ Python and Go descriptors. No fields renumbered or repurposed.
|
|||||||
| Severity | Low |
|
| Severity | Low |
|
||||||
| Category | Code organization & conventions |
|
| Category | Code organization & conventions |
|
||||||
| Location | `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:31-41` (`QueryActiveAlarmsRequest`) |
|
| 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.
|
**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.
|
**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
|
### Contracts-017
|
||||||
|
|
||||||
@@ -299,10 +299,10 @@ Python and Go descriptors. No fields renumbered or repurposed.
|
|||||||
| Severity | Low |
|
| Severity | Low |
|
||||||
| Category | Documentation & comments |
|
| Category | Documentation & comments |
|
||||||
| Location | `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:23-29` (the `rpc QueryActiveAlarms` block) |
|
| 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.
|
**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.
|
**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).
|
||||||
|
|||||||
@@ -735,9 +735,10 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto {
|
|||||||
|
|
||||||
#region Messages
|
#region Messages
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Public request shape for QueryActiveAlarms. session_id is currently unused
|
/// Public request shape for QueryActiveAlarms.
|
||||||
/// (the snapshot is session-less) but reserved so a future per-session view
|
/// Clients may leave `session_id` empty; the gateway currently ignores it and
|
||||||
/// can be added without a wire break.
|
/// serves the session-less central-monitor cache. A future version may use it
|
||||||
|
/// to scope the snapshot to one session.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
[global::System.Diagnostics.DebuggerDisplayAttribute("{ToString(),nq}")]
|
[global::System.Diagnostics.DebuggerDisplayAttribute("{ToString(),nq}")]
|
||||||
public sealed partial class QueryActiveAlarmsRequest : pb::IMessage<QueryActiveAlarmsRequest>
|
public sealed partial class QueryActiveAlarmsRequest : pb::IMessage<QueryActiveAlarmsRequest>
|
||||||
|
|||||||
@@ -196,6 +196,9 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto {
|
|||||||
/// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
/// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
||||||
/// have been missed during a transport blip. Streamed so callers can
|
/// have been missed during a transport blip. Streamed so callers can
|
||||||
/// begin processing without buffering the full set.
|
/// 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.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <param name="request">The request received from the client.</param>
|
/// <param name="request">The request received from the client.</param>
|
||||||
/// <param name="responseStream">Used for sending responses back to the client.</param>
|
/// <param name="responseStream">Used for sending responses back to the client.</param>
|
||||||
@@ -364,6 +367,9 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto {
|
|||||||
/// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
/// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
||||||
/// have been missed during a transport blip. Streamed so callers can
|
/// have been missed during a transport blip. Streamed so callers can
|
||||||
/// begin processing without buffering the full set.
|
/// 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.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <param name="request">The request to send to the server.</param>
|
/// <param name="request">The request to send to the server.</param>
|
||||||
/// <param name="headers">The initial metadata to send with the call. This parameter is optional.</param>
|
/// <param name="headers">The initial metadata to send with the call. This parameter is optional.</param>
|
||||||
@@ -381,6 +387,9 @@ namespace ZB.MOM.WW.MxGateway.Contracts.Proto {
|
|||||||
/// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
/// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
||||||
/// have been missed during a transport blip. Streamed so callers can
|
/// have been missed during a transport blip. Streamed so callers can
|
||||||
/// begin processing without buffering the full set.
|
/// 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.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <param name="request">The request to send to the server.</param>
|
/// <param name="request">The request to send to the server.</param>
|
||||||
/// <param name="options">The options for the call.</param>
|
/// <param name="options">The options for the call.</param>
|
||||||
|
|||||||
@@ -31,12 +31,16 @@ service MxAccessGateway {
|
|||||||
// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
// reconnect to seed Part 9 client state, or to reconcile alarms that may
|
||||||
// have been missed during a transport blip. Streamed so callers can
|
// have been missed during a transport blip. Streamed so callers can
|
||||||
// begin processing without buffering the full set.
|
// 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);
|
rpc QueryActiveAlarms(QueryActiveAlarmsRequest) returns (stream ActiveAlarmSnapshot);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Public request shape for QueryActiveAlarms. session_id is currently unused
|
// Public request shape for QueryActiveAlarms.
|
||||||
// (the snapshot is session-less) but reserved so a future per-session view
|
// Clients may leave `session_id` empty; the gateway currently ignores it and
|
||||||
// can be added without a wire break.
|
// serves the session-less central-monitor cache. A future version may use it
|
||||||
|
// to scope the snapshot to one session.
|
||||||
message QueryActiveAlarmsRequest {
|
message QueryActiveAlarmsRequest {
|
||||||
string session_id = 1;
|
string session_id = 1;
|
||||||
string client_correlation_id = 2;
|
string client_correlation_id = 2;
|
||||||
|
|||||||
@@ -437,6 +437,46 @@ public sealed class ProtobufContractRoundTripTests
|
|||||||
Assert.Equal(withFilter, StreamAlarmsRequest.Parser.ParseFrom(withFilter.ToByteArray()));
|
Assert.Equal(withFilter, StreamAlarmsRequest.Parser.ParseFrom(withFilter.ToByteArray()));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Verifies that <c>QueryActiveAlarmsRequest</c> pins the additive-only field numbering
|
||||||
|
/// (<c>session_id = 1</c>, <c>client_correlation_id = 2</c>, <c>alarm_filter_prefix = 3</c>)
|
||||||
|
/// advertised in its proto comment, that the message round-trips with the optional
|
||||||
|
/// <c>alarm_filter_prefix</c> populated (the filter semantic the public RPC comment
|
||||||
|
/// documents), and that <c>QueryActiveAlarms</c> remains on the public service surface.
|
||||||
|
/// </summary>
|
||||||
|
[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);
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>Verifies that an MxValue carrying a raw_value bytes payload round-trips.</summary>
|
/// <summary>Verifies that an MxValue carrying a raw_value bytes payload round-trips.</summary>
|
||||||
[Fact]
|
[Fact]
|
||||||
public void MxValue_RoundTripsRawValueBytesPayload()
|
public void MxValue_RoundTripsRawValueBytesPayload()
|
||||||
|
|||||||
Reference in New Issue
Block a user