Add helpers to detect Nats-Service-Error on responses - #1152
Conversation
|
Claude finished @mtmk's task in 3m 32s —— View job PR Review
SummaryThe implementation is clean and well-structured. The 1.
2. Tests bypass All test methods that set up service-error headers hard-code the string literals 3. Using |
![synadia-claude-reviewer[bot]](https://avatars.githubusercontent.com/u/132956691?s=60&v=4){kind=small}
| /// <param name="msg">The response message to inspect.</param> | ||
| /// <returns><c>true</c> if the response is a service success; otherwise <c>false</c>.</returns> | ||
| public static bool IsServiceSuccess<T>(this NatsMsg<T> msg) | ||
| => msg.Headers is null || !msg.Headers.ContainsKey(NatsSvcConstants.ServiceErrorHeader); |
There was a problem hiding this comment.
API inconsistency with GetServiceStatus().IsSuccess
IsServiceSuccess() returns true when HasNoResponders is set because there's no service error header. But GetServiceStatus().IsSuccess returns false for the same message (no-responders is explicitly treated as non-success).
A user calling if (msg.IsServiceSuccess()) on a no-responders message will enter the success branch and try to use msg.Data, which is meaningless. The existing NatsMsg<T>.EnsureSuccess() treats no-responders as a failure (throws), so this diverges from established convention.
Consider either:
- Renaming to
IsServiceError()(returnstrueonly when an error header is present — no semantic claim about no-responders) - Or having
IsServiceSuccess()also check!HasNoResponders, consistent withGetServiceStatus().IsSuccess
The throwOnNoResponders overload covers the explicit opt-in path, but the zero-argument overload will silently treat no-responders as success.
There was a problem hiding this comment.
Fixed in d92ad90 by flipping the default: all three helpers (IsServiceSuccess, EnsureServiceSuccess, GetServiceStatus) now throw NatsNoRespondersException on no-responders by default, with a throwOnNoResponders: false opt-out for load-balancing / failover callers. The common path raises instead of silently returning a misleading bool or struct.
|
|
||
| var code = 0; | ||
| if (headers.TryGetValue(NatsSvcConstants.ServiceErrorCodeHeader, out var codeValue) | ||
| && int.TryParse(codeValue.ToString(), out var parsed)) |
There was a problem hiding this comment.
StringValues.ToString() joins multiple values with ", " when there are multiple header values. If a response carries duplicate Nats-Service-Error-Code headers, int.TryParse will silently fail and code stays 0. This is safe (defaulting to 0 is documented), but using codeValue[0] (first value) is slightly more explicit about the intent and avoids the join allocation in the multi-value edge case:
| && int.TryParse(codeValue.ToString(), out var parsed)) | |
| if (headers.TryGetValue(NatsSvcConstants.ServiceErrorCodeHeader, out var codeValue) | |
| && int.TryParse(codeValue[0], out var parsed)) |
There was a problem hiding this comment.
Took a different angle in d92ad90: switched both header reads to last-value-wins via a LastValueOrEmpty helper. While digging into this I checked nats-server source first and confirmed the server doesn't touch these headers at all (the Services protocol is purely client-side), so the original framework-appends-after-user reasoning didn't apply. The realistic multi-value path is the wire parser (NatsHeaderParser.cs:233) using StringValues.Concat on duplicate header lines, so last-wins defends against a malformed reply emitting Nats-Service-Error twice. Skipped the bare [0] form because StringValues' indexer returns string? which trips TreatWarningsAsErrors on netstandard 2.0; the helper handles that and an empty StringValues safely.
| { | ||
| var headers = new NatsHeaders | ||
| { | ||
| { "Nats-Service-Error", "boom" }, |
There was a problem hiding this comment.
Tests use hard-coded string literals instead of NatsSvcConstants.ServiceErrorHeader / NatsSvcConstants.ServiceErrorCodeHeader. One of the goals of NatsSvcConstants is to be the single source of truth for these header names, but tests bypass it. If the constant values were ever changed, these tests would not catch the regression.
| { "Nats-Service-Error", "boom" }, | |
| { NatsSvcConstants.ServiceErrorHeader, "boom" }, | |
| { NatsSvcConstants.ServiceErrorCodeHeader, "500" }, |
Same applies to the other test methods that hard-code these strings (GetServiceStatus_returns_code_and_message_when_present, EnsureServiceSuccess_throws_with_code_and_message, etc.).
There was a problem hiding this comment.
Switched my new unit tests to NatsSvcConstants in d92ad90. The pre-existing integration tests in ServicesTests.cs and CheckNativeAot/Program.cs keep the literals on purpose: those pin the on-wire format end-to-end, so if a constant ever drifted from the spec they should fail loudly.
2 participants
Service handlers signal errors using
Nats-Service-Error/Nats-Service-Error-Coderesponse headers, but until now the requester side had no way to surface that without reading headers manually. AddsIsServiceSuccess(),EnsureServiceSuccess(), andGetServiceStatus()extensions onNatsMsg<T>, plus aNatsSvcStatusstruct and aNatsSvcConstantsclass that centralizes the header names previously duplicated across the producer side.The shape mirrors
HttpResponseMessage:IsServiceSuccess~IsSuccessStatusCode,EnsureServiceSuccess~EnsureSuccessStatusCode, andGetServiceStatusreturns a struct that rollsStatusCode+ReasonPhraseinto one value.Fixes #496