Make a failed call's body readable, and stop sharing one builder per client - #30
Conversation
…client
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<T>.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<T>() 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 <noreply@anthropic.com>
📝 WalkthroughSummary
Risk: Security-sensitive areas: No direct security controls changed. Correlation-id header handling was adjusted to prevent duplicate or stale values; body deserialization remains non-throwing for invalid input. Test coverage: Added 13 regression tests covering typed body parsing, invalid payloads, null results, builder isolation, concurrent and sequential requests, and correlation-id transmission. Reported total: 19 passing tests. Operational concerns: No migration or deployment changes required. No data or configuration changes. Rollback is a standard code rollback; callers adopting the new extensions would need to revert those usages if compatibility with the prior behavior is required. WalkthroughChangesClient operation isolation
ApiResult body reading
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SW.HttpExtensions.UnitTests/ApiResultExtensionsTests.cs`:
- Around line 30-36: Update MatchesPropertiesRegardlessOfCasing to use a
genuinely different casing for the JSON ErrorCode property, such as errorcode or
ERRORCODE, rather than PascalCase. Keep the existing assertion and test flow
unchanged so it specifically validates case-insensitive property matching.
In `@SW.HttpExtensions/ApiOperationBuilder.cs`:
- Around line 29-32: The ApiOperationBuilder correlation-ID handling must stop
mutating shared HttpClient.DefaultRequestHeaders; carry the requestContext
correlation ID through the runner and apply it to each HttpRequestMessage,
omitting it for invalid contexts. In
SW.HttpExtensions/ApiOperationBuilder.cs#L29-L32 update the builder/runner flow
accordingly; in SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L113-L124 add
genuinely parallel builder construction and assert per-request headers; in
SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L144-L160 cover valid-context
reuse followed by an invalid context and assert no stale correlation ID is sent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: simplify9/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14efc684-7392-489b-bba5-7150394791ed
📒 Files selected for processing (5)
SW.HttpExtensions.UnitTests/ApiClientBaseTests.csSW.HttpExtensions.UnitTests/ApiResultExtensionsTests.csSW.HttpExtensions/ApiClientBase.csSW.HttpExtensions/ApiOperationBuilder.csSW.HttpExtensions/ApiResultExtensions.cs
📜 Review details
🔇 Additional comments (5)
SW.HttpExtensions/ApiClientBase.cs (1)
9-43: LGTM!SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs (1)
13-103: LGTM!Also applies to: 126-136
SW.HttpExtensions/ApiResultExtensions.cs (2)
1-37: LGTM!Also applies to: 60-68
48-49: 🎯 Functional CorrectnessNo change needed:
deserialized == nullis valid for an unconstrained generic.The null-literal equality operator is permitted for generic type parameters without a value-type constraint, so this does not need to be rewritten to
is nullorReferenceEquals.> Likely an incorrect or invalid review comment.SW.HttpExtensions.UnitTests/ApiResultExtensionsTests.cs (1)
1-28: LGTM!Also applies to: 39-49, 51-64, 66-79, 81-87
| /// <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); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the casing test use a genuinely different casing.
ErrorCode matches the property declaration exactly, so this test would pass even if case-insensitive matching broke. Use a value such as errorcode or ERRORCODE; the first test already covers camelCase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.HttpExtensions.UnitTests/ApiResultExtensionsTests.cs` around lines 30 -
36, Update MatchesPropertiesRegardlessOfCasing to use a genuinely different
casing for the JSON ErrorCode property, such as errorcode or ERRORCODE, rather
than PascalCase. Keep the existing assertion and test flow unchanged so it
specifically validates case-insensitive property matching.
| if (requestContext.IsValid) | ||
| { | ||
| httpClient.DefaultRequestHeaders.Remove(RequestContext.CorrelationIdHeaderName); | ||
| httpClient.DefaultRequestHeaders.Add(RequestContext.CorrelationIdHeaderName, requestContext.CorrelationId); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect target frameworks and all request/header construction paths.
fd -t f -e csproj -e props -e targets . -x sed -n '1,180p' {} \;
rg -n -C 3 'DefaultRequestHeaders|HttpRequestMessage|SendAsync\(|GetAsync\(|PostAsync\(' \
SW.HttpExtensions SW.HttpExtensions.UnitTestsRepository: simplify9/SW-HttpExtensions
Length of output: 22006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Operation and runner files =="
for f in SW.HttpExtensions/ApiOperationBuilder.cs SW.HttpExtensions/ApiOperationRunnerWrapped.cs SW.HttpExtensions/ApiOperationRunnerWrappedOfT.cs SW.HttpExtensions/ApiOperationRunnerTyped.cs SW.HttpExtensions/ApiClientBase.cs SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs; do
echo "-- $f --"
wc -l "$f"
sed -n '1,180p' "$f" | cat -n
done
echo "== RequestContext definitions/usages =="
rg -n -C 3 'class RequestContext|Record|CorrelationIdHeaderName|CorrelationId|DefaultRequestHeaders\.' SW.HttpExtensions SW.HttpExtensions.UnitTestsRepository: simplify9/SW-HttpExtensions
Length of output: 41328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate primitive types =="
git ls-files | rg 'RequestContext|PrimitiveTypes|SW\.PrimitiveTypes|json' | sed -n '1,200p'
echo "== Search correlation/request context in repo =="
rg -n -C 4 'class RequestContext|record RequestContext|record struct RequestContext|struct RequestContext|CorrelationIdHeaderName|CorrelationId|IsValid|Set\(|Set' \
. --glob '!bin/**' --glob '!obj/**' --glob '!node_modules/**' | sed -n '1,220p'Repository: simplify9/SW-HttpExtensions
Length of output: 12380
🌐 Web query:
SimplyWorks.PrimitiveTypes 8.1.2 RequestContext class CorrelationIdHeaderName CorrelationId IsValid
💡 Result:
In the SimplyWorks.PrimitiveTypes library (version 8.1.2 and related versions), the RequestContext class is used to maintain request-scoped information, including the CorrelationId for distributed tracing [1]. While specific internal implementation details for property names like CorrelationIdHeaderName or method names like IsValid may vary across versions of the Simplify9/SW-PrimitiveTypes repository, the library is designed for.NET 8.0 and provides foundational patterns for API and messaging development [2][1][3]. Based on typical implementations of RequestContext within the SimplyWorks ecosystem: - CorrelationId: This property is used to retrieve or set the unique identifier for tracking a request across service boundaries [1]. - RequestContext: This class is injected into services or message consumers to provide access to context-specific data, such as the current user or correlation ID [1][4]. - CorrelationIdHeaderName: This is typically a configuration constant or property used to identify the HTTP header key (e.g., "X-Correlation-ID") where the correlation ID is stored [5][6]. - IsValid: This is often implemented as a validation method or property within value objects or domain patterns provided by the library to ensure data integrity [2][3]. For the most accurate and up-to-date documentation regarding your specific version, please refer to the official repository at https://github.com/simplify9/SW-PrimitiveTypes [2][7]. If you are looking for configuration specific to your implementation, ensure you are referencing the namespace SW.PrimitiveTypes.Patterns.RequestContext [8].
Citations:
- 1: https://nugetprodusnc.azure-api.net/packages/SimplyWorks.Bus/
- 2: https://github.com/simplify9/SW-PrimitiveTypes
- 3: https://libraries.io/nuget/SimplyWorks.PrimitiveTypes
- 4: https://www.nuget.org/packages/SimplyWorks.Bus/8.1.7
- 5: https://bytecrate.dev/aspnet-core-correlation-id-middleware-logging/
- 6: https://cabinet.thluiz.com/Inbox/2024/05/5-Serilog-Best-Practices-For-Better-Structured-Logging/
- 7: https://nugetmusthaves.com/Package/SimplyWorks.PrimitiveTypes
- 8: simplify9/SW-PrimitiveTypes@2020.5.31.2...8.1.0
🌐 Web query:
SimplyWorks.PrimitiveTypes 8.1.2 RequestContext.CorrelationId "sw-correlation-id"
💡 Result:
The term sw-correlation-id is a custom identifier commonly used for distributed tracing and request correlation in.NET applications, particularly those utilizing libraries from the SimplyWorks ecosystem [1][2][3]. SimplyWorks.PrimitiveTypes is a foundational.NET library that provides essential types, domain patterns, and messaging contracts for building modern applications [4][3]. While specific, granular implementation details for every property in version 8.1.2 may not be documented in a single public reference manual, the library is part of the simplify9 open-source project, which facilitates building event-driven architectures and API handlers [2][3]. In such architectures, a Correlation ID (often passed via an HTTP header or messaging property like sw-correlation-id) is used to track a single request or transaction as it traverses various services, logs, and message buses [5][6]. Developers typically implement this by: 1. Generating or extracting the Correlation ID at the service entry point (e.g., an API middleware) [5][6]. 2. Storing this ID in a RequestContext or similar scoped container to make it accessible throughout the request lifecycle [5]. 3. Injecting the ID into outgoing HTTP requests or message headers (e.g., sw-correlation-id) to maintain the trace across downstream services [5]. If you are working with SimplyWorks.PrimitiveTypes, you can find the source code and documentation on the simplify9 GitHub repository to see how these primitives integrate with your messaging or API layer [2][7].
Citations:
- 1: https://www.nuget.org/packages/SimplyWorks.Bus.RabbitMqExtensions/8.1.6
- 2: https://github.com/simplify9/SW-PrimitiveTypes
- 3: https://libraries.io/nuget/SimplyWorks.PrimitiveTypes
- 4: https://www.nuget.org/profiles/simplify9
- 5: https://ankitvijay.net/2020/11/24/passing-correlation-id-across-requests/
- 6: https://oneuptime.com/blog/post/2026-01-30-request-trace-correlation/view
- 7: https://github.com/simplify9/PrimitiveTypes
🌐 Web query:
site:github.com/simplify9/PrimitiveTypes RequestContext.cs CorrelationIdHeaderName
💡 Result:
There is no standard or built-in.NET type, class, or property named CorrelationIdHeaderName within the.NET SDK or standard library. Furthermore, RequestContext.cs is not a standard file in.NET or ASP.NET Core framework projects. The term likely refers to a custom implementation within a specific third-party library or a private application-specific codebase [1]. If you are working with a codebase containing these names, they are custom identifiers defined by your project or the specific library you are using, rather than platform-provided defaults.
Citations:
🌐 Web query:
"simplify9" "PrimitiveTypes" "RequestContext" "CorrelationIdHeaderName"
💡 Result:
The terms "PrimitiveTypes," "RequestContext," and "CorrelationIdHeaderName" are associated with the "SimplyWorks" ecosystem developed by Simplify9 [1][2]. SimplyWorks.PrimitiveTypes is a foundational.NET 8.0 library [1][2] providing essential types, interfaces, and patterns for domain-driven design, value objects, messaging, and API development [1][3]. Within this ecosystem: 1. RequestContext: The library includes infrastructure for managing request-scoped data [4]. Files such as RequestContext.cs and IRequestContext.cs are part of the patterns provided to handle contextual information during request processing [4]. 2. CorrelationIdHeaderName: While "CorrelationIdHeaderName" is a common constant name in various.NET libraries (such as the ABP Framework [5][6] or community-developed correlation ID libraries [7][8]), within the context of the Simplify9 libraries, such constants are used to facilitate traceability across services in their messaging and API handlers [1][3][9]. The Simplify9 repository, specifically SW-PrimitiveTypes, serves as a core dependency for other tools in their ecosystem, including SimplyWorks.Bus (for messaging) and SimplyWorks.CqApi (for API generation) [1][2]. If you are looking for specific implementation details, you can find the library on GitHub under simplify9/SW-PrimitiveTypes [1][3].
Citations:
- 1: https://github.com/simplify9/SW-PrimitiveTypes
- 2: https://libraries.io/nuget/SimplyWorks.PrimitiveTypes
- 3: https://github.com/simplify9/PrimitiveTypes
- 4: simplify9/SW-PrimitiveTypes@2020.5.31.2...8.1.0
- 5: https://abp.io/docs/api/10.0/Volo.Abp.EventBus.EventBusConsts.html
- 6: https://github.com/abpframework/abp/blob/rel-10.1/docs/en/framework/infrastructure/correlation-id.md
- 7: https://github.com/BipulRaman/CorrelationId
- 8: https://mderriey.com/2016/11/18/correlation-id-with-asp-net-web-api/
- 9: https://www.nuget.org/packages/SimplyWorks.Bus.RabbitMqExtensions/8.1.7
Keep correlation IDs off shared default headers.
DefaultRequestHeaders is shared mutable HttpClient state. Parallel builder construction can race Remove/Add, and an invalid context skips removal so a previously installed correlation ID can be sent on later operations. Carry the ID through the runner and apply it per HttpRequestMessage.
SW.HttpExtensions/ApiOperationBuilder.cs#L29-L32: do not mutate sharedHttpClient.DefaultRequestHeadersfor request-scoped correlation.SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L113-L124: use genuinely parallel builder construction and assert sent request headers.SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L144-L160: cover a prior valid context followed by an invalid reuse; assert stale correlation ID is absent on the invalid request.
📍 Affects 2 files
SW.HttpExtensions/ApiOperationBuilder.cs#L29-L32(this comment)SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L113-L124SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L144-L160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.HttpExtensions/ApiOperationBuilder.cs` around lines 29 - 32, The
ApiOperationBuilder correlation-ID handling must stop mutating shared
HttpClient.DefaultRequestHeaders; carry the requestContext correlation ID
through the runner and apply it to each HttpRequestMessage, omitting it for
invalid contexts. In SW.HttpExtensions/ApiOperationBuilder.cs#L29-L32 update the
builder/runner flow accordingly; in
SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L113-L124 add genuinely
parallel builder construction and assert per-request headers; in
SW.HttpExtensions.UnitTests/ApiClientBaseTests.cs#L144-L160 cover valid-context
reuse followed by an invalid context and assert no stale correlation ID is sent.
3 participants
Two independent problems, both surfaced while wiring a service-to-service call that needed to tell an expected refusal ("not enough stock") apart from a fault.
1. A failed call's body is unreadable
ApiResult<TResponse>.Responseis only populated on a 2xx (ApiOperationRunnerWrappedOfT.cs:65-86); a non-2xx leaves the payload inBodyas an unparsed string. So an API that answers an expected, non-exceptional refusal with a4xxand a typed body — "out of stock", "already cancelled", a validation list — hands the caller a status code and nothing actionable, unless every call site hand-rolls its ownJsonConvertcall.The observable consequence is services answering
200 OKwith an error field purely because that's the only shape a caller can read. That's exactly what I ended up doing in Traxis Warehouse, and it has two costs: it hides a failure from every generic status-code check (logging, retries, dashboards), and it silently degrades if anyone later corrects the status to a 4xx — the caller stops seeing the reason and reports "couldn't reach the service" for an ordinary out-of-stock.Added
TryReadBody<TBody>/ReadBodyOrDefault<TBody>extensions onApiResult:Deliberately not populating
Responseon failure, though that was the other option I considered. Callers currently rely onResponsebeing null on a failed call — some use it as the success signal — so filling it in would change behaviour for every SDK in the org to no one's benefit. An extension gets the same capability with zero behavioural change.TryReadBodynever throws. That's the point, not politeness: a failure body is frequently not the shape you expect — an unhandled server exception, a proxy's HTML error page, a gateway timeout — and none of those should turn a failed call into a thrown exception at the call site.2. One builder shared by every method on a client
ApiClientBase.Builderwas a single instance built in the constructor, whileApiOperationBuildercarries mutable per-call state (the path). That made every client only accidentally safe.I initially thought
Task.WhenAll(client.A(), client.B())was broken by this. It isn't, and the test I wrote to prove it passed against the unfixed code — which is how I caught my own error.AsApiResultcopies the path into a fresh runner synchronously, before the first await, soA()'s path is captured beforeB()even begins. There's now a test pinning that down, because it's the reason this survived in production for as long as it did.What genuinely does not hold up:
Task.Run,Parallel.ForEach, a background fan-out): two threads racePath(...)and one operation is dispatched to the other's URL. Silently. The window is a couple of instructions, which makes it rare and horrible to diagnose.var b = Builder.Path(x);then usingbafter any other call on the same client. Not a race at all: deterministically wrong, becausebis the other call's builder.Buildernow returns a fresh instance per access.Which required making the correlation-id header idempotent
HttpHeaders.Addappends rather than replaces, so a builder per call would have sentsw-correlation-id: abc, abc, growing by one on every request. Now remove-then-add — which also keeps the value current, where a plain "add only if missing" guard would pin the first request's correlation id onto every later request that reuses the client.Verified in both directions
Regression tests are worthless if they pass before the fix, so I checked:
BuilderIsNotSharedBetweenAccessesandOperationsPreparedBeforeDispatchDoNotCrossPathsfailCorrelationIdIsSentOnceNoMatterHowManyCallsfails — "correlation id was sent 3 times"Compatibility
Source- and binary-compatible.
Builderkeeps its type, name and accessibility — it changes from an auto-property to a computed one, so consumers recompile and rebind unchanged. The extensions are purely additive, andResponse/Body/Successsemantics are untouched.The one behavioural change any consumer can observe:
Builderis no longer reference-equal across accesses. Nothing in-tree depended on that, and depending on it was the bug.nuget-publish.ymlruns on push tomain, so this releases a newSimplyWorks.HttpExtensions(currently8.1.x) that every SDK in the org eventually picks up. Worth a second reviewer on #2 specifically for that reason.Not in scope, but worth knowing
Three more things I found in the same read, left alone deliberately:
Jwt()setshttpClient.DefaultRequestHeaders.Authorization— shared client state rather than per-request. Typed clients are transient so it's fine today, but under any client reuse across users a leftover header is a token-leak vector. The fix is per-HttpRequestMessageheaders, which is a much larger change.Header(name, value)has noContainsguard (unlikeKey()), so repeated calls accumulate values on the sharedDefaultRequestHeaders. Pre-existing and unchanged by this PR.ApiOperationRunnerWrapped<T>.GetAsyncmutates its ownpathfield when appending the query string — harmless while runners are per-call, but a latent double-append.🤖 Generated with Claude Code