From 151a591dd77c9c610ca80b2bdb18109b475450d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:01:33 +0300 Subject: [PATCH] Make a failed call's body readable, and stop sharing one builder per client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent problems, both found while wiring a service-to-service call that needed to tell an expected refusal ("not enough stock") apart from a fault. ApiResult.Response is only populated on a 2xx, so a non-2xx leaves the payload in Body as an unparsed string. An API that answers an expected, non-exceptional refusal with a 4xx and a typed body therefore hands the caller a status code and nothing actionable, unless every call site hand-rolls a JsonConvert call. The observable consequence is services answering 200 OK with an error field purely because that's the only shape a caller can read — which hides a failure from every generic status-code check, and silently degrades to "couldn't reach the service" if anyone later corrects the status. TryReadBody/ReadBodyOrDefault make the failure body readable without touching Response's semantics, so no existing caller changes behaviour. ApiClientBase.Builder was one instance built in the constructor and shared by every method, while ApiOperationBuilder carries mutable per-call state (the path). That was only accidentally safe. The usual Builder.Path(x).AsApiResult() shape holds up, because AsApiResult copies the path into a fresh runner synchronously, before the first await — so even Task.WhenAll of two operations has the first path captured before the second starts. There's a test pinning that down, since it's why this survived in production. What did not hold up: genuinely parallel use of one client instance, where two threads race Path() and one operation is sent to the other's URL; and holding a configured builder before dispatching it, which isn't a race but deterministically wrong, because it IS the other call's builder. Builder now returns a fresh instance per access. That in turn required the correlation-id header to become idempotent: HttpHeaders.Add appends rather than replaces, so a builder per call would have sent "sw-correlation-id: abc, abc", growing by one every call. Remove-then-add also keeps the value current, which an "add only if missing" guard would not. Verified both directions: the three new behavioural tests fail against the previous code (2 on the shared builder, 1 on the correlation header once the builder is per-call) and pass after. 19/19 green. Source- and binary-compatible: Builder keeps its type and accessibility, and the extensions are additive. Co-Authored-By: Claude Opus 5 --- .../ApiClientBaseTests.cs | 163 ++++++++++++++++++ .../ApiResultExtensionsTests.cs | 89 ++++++++++ SW.HttpExtensions/ApiClientBase.cs | 29 +++- SW.HttpExtensions/ApiOperationBuilder.cs | 9 + SW.HttpExtensions/ApiResultExtensions.cs | 70 ++++++++ 5 files changed, 358 insertions(+), 2 deletions(-) create mode 100644 SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs create mode 100644 SW.HttpExtensions.UnitTests/ApiResultExtensionsTests.cs create mode 100644 SW.HttpExtensions/ApiResultExtensions.cs diff --git a/SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs b/SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs new file mode 100644 index 0000000..073c400 --- /dev/null +++ b/SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs @@ -0,0 +1,163 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using SW.PrimitiveTypes; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Security.Claims; +using System.Threading; +using System.Threading.Tasks; + +namespace SW.HttpExtensions.UnitTests +{ + [TestClass] + public class ApiClientBaseTests + { + private class TestOptions : ApiClientOptionsBase + { + public override string ConfigurationSection => "Test"; + } + + /// Records every path it is asked for, and answers 200 with an empty object. + private class RecordingHandler : HttpMessageHandler + { + private readonly List paths = new List(); + private readonly int delayMs; + + public RecordingHandler(int delayMs = 0) => this.delayMs = delayMs; + + public IReadOnlyList Paths + { + get { lock (paths) return paths.ToList(); } + } + + protected override async Task SendAsync(HttpRequestMessage request, + CancellationToken cancellationToken) + { + lock (paths) paths.Add(request.RequestUri.PathAndQuery); + if (delayMs > 0) await Task.Delay(delayMs, cancellationToken); + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{}", System.Text.Encoding.UTF8, "application/json") + }; + } + } + + /// Two operations on one client, exactly as a generated SDK writes them. + private class TestClient : ApiClientBase + { + public TestClient(HttpClient httpClient, RequestContext requestContext, TestOptions options) + : base(httpClient, requestContext, options) { } + + public Task> Alpha() => + Builder.Path("alpha").AsApiResult().GetAsync(); + + public Task> Beta() => + Builder.Path("beta").AsApiResult().GetAsync(); + + public ApiOperationBuilder ExposedBuilder => Builder; + + /// + /// Prepares both operations before dispatching either — the shape that breaks + /// deterministically when every method shares one builder instance. + /// + public async Task TwoPreparedOperations() + { + var first = Builder.Path("alpha"); + var second = Builder.Path("beta"); + await first.AsApiResult().GetAsync(); + await second.AsApiResult().GetAsync(); + } + } + + private static TestClient CreateClient(HttpMessageHandler handler, RequestContext requestContext = null) + { + var httpClient = new HttpClient(handler) { BaseAddress = new System.Uri("http://localhost/") }; + return new TestClient(httpClient, requestContext ?? new RequestContext(), new TestOptions()); + } + + [TestMethod] + public void BuilderIsNotSharedBetweenAccesses() + { + var client = CreateClient(new RecordingHandler()); + Assert.AreNotSame(client.ExposedBuilder, client.ExposedBuilder); + } + + /// + /// The bug a per-access builder exists to prevent, in its deterministic form: with one shared + /// builder the second Path(...) overwrites the first, so BOTH requests go to "beta" and the + /// first operation silently runs as the second. + /// + [TestMethod] + public async Task OperationsPreparedBeforeDispatchDoNotCrossPaths() + { + var handler = new RecordingHandler(); + var client = CreateClient(handler); + + await client.TwoPreparedOperations(); + + CollectionAssert.AreEqual( + new[] { "/alpha", "/beta" }, + handler.Paths.ToList(), + $"expected one request per operation, got: {string.Join(", ", handler.Paths)}"); + } + + /// + /// Guards the case that was ALREADY safe, so it stays that way: awaiting two async operations + /// together is fine even with a shared builder, because AsApiResult copies the path into a + /// fresh runner synchronously — before the first await — so Alpha's path is captured before + /// Beta starts. Worth pinning down; it's the reason the old shared builder survived in + /// production for as long as it did. + /// + [TestMethod] + public async Task AwaitingTwoOperationsTogetherKeepsTheirOwnPaths() + { + var handler = new RecordingHandler(delayMs: 30); + var client = CreateClient(handler); + + await Task.WhenAll(client.Alpha(), client.Beta()); + + CollectionAssert.AreEquivalent( + new[] { "/alpha", "/beta" }, + handler.Paths.ToList(), + $"expected one request per operation, got: {string.Join(", ", handler.Paths)}"); + } + + [TestMethod] + public async Task SequentialCallsOnOneClientEachKeepTheirOwnPath() + { + var handler = new RecordingHandler(); + var client = CreateClient(handler); + + await client.Alpha(); + await client.Beta(); + + CollectionAssert.AreEqual(new[] { "/alpha", "/beta" }, handler.Paths.ToList()); + } + + /// + /// A builder per call means the constructor's correlation-id header runs per call too, and + /// HttpHeaders.Add appends rather than replaces — so without the remove-then-add this sends a + /// header that grows a duplicate value on every request. + /// + [TestMethod] + public async Task CorrelationIdIsSentOnceNoMatterHowManyCalls() + { + var requestContext = new RequestContext(); + requestContext.Set(new ClaimsPrincipal(new ClaimsIdentity("testauth"))); + + var handler = new RecordingHandler(); + var httpClient = new HttpClient(handler) { BaseAddress = new System.Uri("http://localhost/") }; + var client = new TestClient(httpClient, requestContext, new TestOptions()); + + await client.Alpha(); + await client.Beta(); + await client.Alpha(); + + Assert.IsTrue( + httpClient.DefaultRequestHeaders.TryGetValues(RequestContext.CorrelationIdHeaderName, out var values), + "correlation id header was not set at all"); + Assert.AreEqual(1, values.Count(), $"correlation id was sent {values.Count()} times"); + } + } +} diff --git a/SW.HttpExtensions.UnitTests/ApiResultExtensionsTests.cs b/SW.HttpExtensions.UnitTests/ApiResultExtensionsTests.cs new file mode 100644 index 0000000..ccd9f21 --- /dev/null +++ b/SW.HttpExtensions.UnitTests/ApiResultExtensionsTests.cs @@ -0,0 +1,89 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using SW.PrimitiveTypes; + +namespace SW.HttpExtensions.UnitTests +{ + [TestClass] + public class ApiResultExtensionsTests + { + private class Refusal + { + public string ErrorCode { get; set; } + public string ErrorMessage { get; set; } + } + + [TestMethod] + public void ReadsAStructuredBodyOffAFailedResult() + { + var result = new ApiResult + { + Success = false, + StatusCode = 400, + Body = "{\"errorCode\":\"InsufficientQuantity\",\"errorMessage\":\"only 3 left\"}" + }; + + Assert.IsTrue(result.TryReadBody(out var refusal)); + Assert.AreEqual("InsufficientQuantity", refusal.ErrorCode); + Assert.AreEqual("only 3 left", refusal.ErrorMessage); + } + + /// Property casing differs between the wire (camel) and C# (pascal). + [TestMethod] + public void MatchesPropertiesRegardlessOfCasing() + { + var result = new ApiResult { Body = "{\"ErrorCode\":\"Pascal\"}" }; + Assert.IsTrue(result.TryReadBody(out var refusal)); + Assert.AreEqual("Pascal", refusal.ErrorCode); + } + + /// + /// A failure body is often not the shape the caller expects — a proxy's HTML error page, a + /// gateway timeout, a bare exception string. None of those may throw at the call site. + /// + [TestMethod] + public void ReturnsFalseRatherThanThrowingOnABodyThatIsNotJson() + { + var result = new ApiResult { StatusCode = 502, Body = "Bad Gateway" }; + Assert.IsFalse(result.TryReadBody(out var refusal)); + Assert.IsNull(refusal); + } + + [TestMethod] + public void ReturnsFalseOnJsonOfTheWrongShape() + { + var result = new ApiResult { Body = "[1,2,3]" }; + Assert.IsFalse(result.TryReadBody(out _)); + } + + [TestMethod] + public void ReturnsFalseOnAnEmptyOrAbsentBody() + { + Assert.IsFalse(new ApiResult { Body = null }.TryReadBody(out _)); + Assert.IsFalse(new ApiResult { Body = "" }.TryReadBody(out _)); + Assert.IsFalse(new ApiResult { Body = " " }.TryReadBody(out _)); + } + + /// Literal JSON null deserializes to null — absent, not a value. + [TestMethod] + public void ReturnsFalseOnLiteralJsonNull() + { + Assert.IsFalse(new ApiResult { Body = "null" }.TryReadBody(out _)); + } + + [TestMethod] + public void ToleratesANullResult() + { + ApiResult result = null; + Assert.IsFalse(result.TryReadBody(out _)); + Assert.IsNull(result.ReadBodyOrDefault()); + } + + [TestMethod] + public void ReadBodyOrDefaultReturnsTheBodyOrTheDefault() + { + Assert.AreEqual("X", + new ApiResult { Body = "{\"errorCode\":\"X\"}" }.ReadBodyOrDefault().ErrorCode); + Assert.IsNull(new ApiResult { Body = "nonsense" }.ReadBodyOrDefault()); + } + } +} diff --git a/SW.HttpExtensions/ApiClientBase.cs b/SW.HttpExtensions/ApiClientBase.cs index 42fd47a..24605c0 100644 --- a/SW.HttpExtensions/ApiClientBase.cs +++ b/SW.HttpExtensions/ApiClientBase.cs @@ -6,16 +6,41 @@ namespace SW.HttpExtensions { public abstract class ApiClientBase where TApiClientOptions : ApiClientOptionsBase { + private readonly RequestContext requestContext; + protected ApiClientBase(HttpClient httpClient, RequestContext requestContext, TApiClientOptions options) { HttpClient = httpClient; Options = options; - Builder = new ApiOperationBuilder(httpClient, requestContext, Options); + this.requestContext = requestContext; } protected HttpClient HttpClient { get; } protected TApiClientOptions Options { get; } - protected ApiOperationBuilder Builder { get; } + + /// + /// A FRESH builder per access, because + /// carries mutable per-call state — most importantly the path. + /// + /// + /// This used to be one instance built in the constructor and shared by every method on the + /// client, which made the client only accidentally safe. The usual + /// Builder.Path(x).AsApiResult<T>().GetAsync() shape happens to hold up, because + /// AsApiResult copies the path into a fresh runner synchronously — before the first + /// await — so even Task.WhenAll(client.A(), client.B()) has A's path captured before B + /// starts. Two things did NOT hold up: + /// + /// + /// Genuinely parallel use of one client instance (Task.Run, Parallel.ForEach, + /// a background fan-out). Two threads racing Path(...) can send one operation to the + /// other's URL, silently — the window is small, which makes it rare and awful to diagnose. + /// Holding a configured builder before dispatching it — var b = Builder.Path(x); + /// then using b after any other call on the same client. That one isn't a race at all; + /// it's deterministically wrong, because b IS the other call's builder. + /// + /// + protected ApiOperationBuilder Builder => + new ApiOperationBuilder(HttpClient, requestContext, Options); //protected void AddApiKey() //{ diff --git a/SW.HttpExtensions/ApiOperationBuilder.cs b/SW.HttpExtensions/ApiOperationBuilder.cs index e8e943c..674e97c 100644 --- a/SW.HttpExtensions/ApiOperationBuilder.cs +++ b/SW.HttpExtensions/ApiOperationBuilder.cs @@ -20,8 +20,17 @@ public ApiOperationBuilder(HttpClient httpClient, RequestContext requestContext, this.requestContext = requestContext; this.options = options; + // Remove-then-add rather than a bare Add: HttpHeaders.Add APPENDS a second value for an + // existing name, so building more than one builder over the same HttpClient (which is now + // once per call — see ApiClientBase.Builder) would otherwise send + // "sw-correlation-id: abc, abc", growing by one on every call. Remove-then-add is also + // what keeps the header CURRENT: a plain "add only if missing" guard would pin the first + // request's correlation id onto every later request that reuses the client. if (requestContext.IsValid) + { + httpClient.DefaultRequestHeaders.Remove(RequestContext.CorrelationIdHeaderName); httpClient.DefaultRequestHeaders.Add(RequestContext.CorrelationIdHeaderName, requestContext.CorrelationId); + } } public ApiOperationBuilder Path(string path) diff --git a/SW.HttpExtensions/ApiResultExtensions.cs b/SW.HttpExtensions/ApiResultExtensions.cs new file mode 100644 index 0000000..4e37a1c --- /dev/null +++ b/SW.HttpExtensions/ApiResultExtensions.cs @@ -0,0 +1,70 @@ +using Newtonsoft.Json; +using SW.PrimitiveTypes; + +namespace SW.HttpExtensions +{ + /// + /// Reads a structured body off an — in practice, off a FAILED one. + /// + /// + /// is only ever populated for a 2xx (see + /// ); a non-2xx leaves the payload sitting in + /// as an unparsed string. So an API that answers an expected, + /// non-exceptional refusal with a 4xx and a typed body — "out of stock", "already cancelled", a + /// validation list — gives the caller a status code and nothing it can act on, unless every call + /// site hand-rolls its own call. + /// + /// + /// The observable consequence is services answering 200 OK with an error field instead, + /// purely because that's the only shape a caller can actually read. That works, but it hides a + /// failure from every generic status-code check (logging, retries, dashboards) and silently + /// degrades if anyone later corrects the status to a 4xx. + /// + /// + public static class ApiResultExtensions + { + /// + /// Deserializes into . + /// + /// Never throws: a null result, an empty body, or a body that isn't JSON of that shape all + /// return false with left at its default. That matters because + /// a failure body is frequently NOT the shape you expect — an unhandled server exception, an + /// HTML error page from a proxy, or a gateway timeout — and none of those should turn a failed + /// call into a thrown exception at the call site. + /// + /// Note this reads , not : + /// on a successful typed call the body is not retained, so this is for the failure path. + /// + public static bool TryReadBody(this ApiResult apiResult, out TBody body) + { + body = default; + + if (string.IsNullOrWhiteSpace(apiResult?.Body)) + return false; + + try + { + var deserialized = JsonConvert.DeserializeObject(apiResult.Body); + if (deserialized == null) + return false; + + body = deserialized; + return true; + } + catch (JsonException) + { + return false; + } + } + + /// + /// for when the absent case needs no distinct handling — + /// returns the default when the body is missing or isn't . + /// + public static TBody ReadBodyOrDefault(this ApiResult apiResult) + { + apiResult.TryReadBody(out var body); + return body; + } + } +}