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; + } + } +}