Make a failed call's body readable, and stop sharing one builder per client

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";
+        }
+
+        /// <summary>Records every path it is asked for, and answers 200 with an empty object.</summary>
+        private class RecordingHandler : HttpMessageHandler
+        {
+            private readonly List<string> paths = new List<string>();
+            private readonly int delayMs;
+
+            public RecordingHandler(int delayMs = 0) => this.delayMs = delayMs;
+
+            public IReadOnlyList<string> Paths
+            {
+                get { lock (paths) return paths.ToList(); }
+            }
+
+            protected override async Task<HttpResponseMessage> 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")
+                };
+            }
+        }
+
+        /// <summary>Two operations on one client, exactly as a generated SDK writes them.</summary>
+        private class TestClient : ApiClientBase<TestOptions>
+        {
+            public TestClient(HttpClient httpClient, RequestContext requestContext, TestOptions options)
+                : base(httpClient, requestContext, options) { }
+
+            public Task<ApiResult<object>> Alpha() =>
+                Builder.Path("alpha").AsApiResult<object>().GetAsync();
+
+            public Task<ApiResult<object>> Beta() =>
+                Builder.Path("beta").AsApiResult<object>().GetAsync();
+
+            public ApiOperationBuilder<TestOptions> ExposedBuilder => Builder;
+
+            /// <summary>
+            /// Prepares both operations before dispatching either — the shape that breaks
+            /// deterministically when every method shares one builder instance.
+            /// </summary>
+            public async Task TwoPreparedOperations()
+            {
+                var first = Builder.Path("alpha");
+                var second = Builder.Path("beta");
+                await first.AsApiResult<object>().GetAsync();
+                await second.AsApiResult<object>().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);
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [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)}");
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [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());
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [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");
+        }
+    }
+}

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<object>
+            {
+                Success = false,
+                StatusCode = 400,
+                Body = "{\"errorCode\":\"InsufficientQuantity\",\"errorMessage\":\"only 3 left\"}"
+            };
+
+            Assert.IsTrue(result.TryReadBody<Refusal>(out var refusal));
+            Assert.AreEqual("InsufficientQuantity", refusal.ErrorCode);
+            Assert.AreEqual("only 3 left", refusal.ErrorMessage);
+        }
+
+        /// <summary>Property casing differs between the wire (camel) and C# (pascal).</summary>
+        [TestMethod]
+        public void MatchesPropertiesRegardlessOfCasing()
+        {
+            var result = new ApiResult { Body = "{\"ErrorCode\":\"Pascal\"}" };
+            Assert.IsTrue(result.TryReadBody<Refusal>(out var refusal));
+            Assert.AreEqual("Pascal", refusal.ErrorCode);
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [TestMethod]
+        public void ReturnsFalseRatherThanThrowingOnABodyThatIsNotJson()
+        {
+            var result = new ApiResult { StatusCode = 502, Body = "<html><body>Bad Gateway</body></html>" };
+            Assert.IsFalse(result.TryReadBody<Refusal>(out var refusal));
+            Assert.IsNull(refusal);
+        }
+
+        [TestMethod]
+        public void ReturnsFalseOnJsonOfTheWrongShape()
+        {
+            var result = new ApiResult { Body = "[1,2,3]" };
+            Assert.IsFalse(result.TryReadBody<Refusal>(out _));
+        }
+
+        [TestMethod]
+        public void ReturnsFalseOnAnEmptyOrAbsentBody()
+        {
+            Assert.IsFalse(new ApiResult { Body = null }.TryReadBody<Refusal>(out _));
+            Assert.IsFalse(new ApiResult { Body = "" }.TryReadBody<Refusal>(out _));
+            Assert.IsFalse(new ApiResult { Body = "   " }.TryReadBody<Refusal>(out _));
+        }
+
+        /// <summary>Literal JSON null deserializes to null — absent, not a value.</summary>
+        [TestMethod]
+        public void ReturnsFalseOnLiteralJsonNull()
+        {
+            Assert.IsFalse(new ApiResult { Body = "null" }.TryReadBody<Refusal>(out _));
+        }
+
+        [TestMethod]
+        public void ToleratesANullResult()
+        {
+            ApiResult result = null;
+            Assert.IsFalse(result.TryReadBody<Refusal>(out _));
+            Assert.IsNull(result.ReadBodyOrDefault<Refusal>());
+        }
+
+        [TestMethod]
+        public void ReadBodyOrDefaultReturnsTheBodyOrTheDefault()
+        {
+            Assert.AreEqual("X",
+                new ApiResult { Body = "{\"errorCode\":\"X\"}" }.ReadBodyOrDefault<Refusal>().ErrorCode);
+            Assert.IsNull(new ApiResult { Body = "nonsense" }.ReadBodyOrDefault<Refusal>());
+        }
+    }
+}

SW.HttpExtensions/ApiClientBase.cs

@@ -6,16 +6,41 @@ namespace SW.HttpExtensions
 {
     public abstract class ApiClientBase<TApiClientOptions> where TApiClientOptions : ApiClientOptionsBase
     {
+        private readonly RequestContext requestContext;
+
         protected ApiClientBase(HttpClient httpClient, RequestContext requestContext, TApiClientOptions options)
         {
             HttpClient = httpClient;
             Options = options;
-            Builder = new ApiOperationBuilder<TApiClientOptions>(httpClient, requestContext, Options);
+            this.requestContext = requestContext;
         }
 
         protected HttpClient HttpClient { get; }
         protected TApiClientOptions Options { get; }
-        protected ApiOperationBuilder<TApiClientOptions> Builder { get; }
+
+        /// <summary>
+        /// A FRESH builder per access, because <see cref="ApiOperationBuilder{TApiClientOptions}"/>
+        /// carries mutable per-call state — most importantly the path.
+        ///
+        /// <para>
+        /// 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
+        /// <c>Builder.Path(x).AsApiResult&lt;T&gt;().GetAsync()</c> shape happens to hold up, because
+        /// <c>AsApiResult</c> copies the path into a fresh runner synchronously — before the first
+        /// await — so even <c>Task.WhenAll(client.A(), client.B())</c> has A's path captured before B
+        /// starts. Two things did NOT hold up:
+        /// </para>
+        /// <list type="bullet">
+        /// <item>Genuinely parallel use of one client instance (<c>Task.Run</c>, <c>Parallel.ForEach</c>,
+        /// a background fan-out). Two threads racing <c>Path(...)</c> can send one operation to the
+        /// other's URL, silently — the window is small, which makes it rare and awful to diagnose.</item>
+        /// <item>Holding a configured builder before dispatching it — <c>var b = Builder.Path(x);</c>
+        /// then using <c>b</c> after any other call on the same client. That one isn't a race at all;
+        /// it's deterministically wrong, because <c>b</c> IS the other call's builder.</item>
+        /// </list>
+        /// </summary>
+        protected ApiOperationBuilder<TApiClientOptions> Builder =>
+            new ApiOperationBuilder<TApiClientOptions>(HttpClient, requestContext, Options);
 
         //protected void AddApiKey()
         //{

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<TApiClientOptions> Path(string path)

SW.HttpExtensions/ApiResultExtensions.cs

@@ -0,0 +1,70 @@
+using Newtonsoft.Json;
+using SW.PrimitiveTypes;
+
+namespace SW.HttpExtensions
+{
+    /// <summary>
+    /// Reads a structured body off an <see cref="ApiResult"/> — in practice, off a FAILED one.
+    ///
+    /// <para>
+    /// <see cref="ApiResult{TResponse}.Response"/> is only ever populated for a 2xx (see
+    /// <see cref="ApiOperationRunnerWrapped{TResponse}"/>); a non-2xx leaves the payload sitting in
+    /// <see cref="ApiResult.Body"/> 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 <see cref="JsonConvert"/> call.
+    /// </para>
+    /// <para>
+    /// The observable consequence is services answering <c>200 OK</c> 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.
+    /// </para>
+    /// </summary>
+    public static class ApiResultExtensions
+    {
+        /// <summary>
+        /// Deserializes <see cref="ApiResult.Body"/> into <typeparamref name="TBody"/>.
+        ///
+        /// Never throws: a null result, an empty body, or a body that isn't JSON of that shape all
+        /// return <c>false</c> with <paramref name="body"/> 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 <see cref="ApiResult.Body"/>, not <see cref="ApiResult{TResponse}.Response"/>:
+        /// on a successful typed call the body is not retained, so this is for the failure path.
+        /// </summary>
+        public static bool TryReadBody<TBody>(this ApiResult apiResult, out TBody body)
+        {
+            body = default;
+
+            if (string.IsNullOrWhiteSpace(apiResult?.Body))
+                return false;
+
+            try
+            {
+                var deserialized = JsonConvert.DeserializeObject<TBody>(apiResult.Body);
+                if (deserialized == null)
+                    return false;
+
+                body = deserialized;
+                return true;
+            }
+            catch (JsonException)
+            {
+                return false;
+            }
+        }
+
+        /// <summary>
+        /// <see cref="TryReadBody{TBody}"/> for when the absent case needs no distinct handling —
+        /// returns the default when the body is missing or isn't <typeparamref name="TBody"/>.
+        /// </summary>
+        public static TBody ReadBodyOrDefault<TBody>(this ApiResult apiResult)
+        {
+            apiResult.TryReadBody<TBody>(out var body);
+            return body;
+        }
+    }
+}