feat: optimization backlog — security, performance, PATCH verb, AOT, and test coverage#26
Merged
Merged
Conversation
- Add SemaphoreSlim double-check pattern to BearerTokenProvider to prevent concurrent token refresh races under high load - Add ConfigureAwait(false) to all awaits in authentication handlers - Use conditional compilation for HttpRequestMessage.Properties vs Options to suppress obsolete warnings on .NET 5+ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Only create StringContent when body is non-empty; prevents sending Content-Type: application/json on GET/DELETE with no body - Remove hardcoded HTTP/2 version; let HttpClient negotiate - Replace HttpClient.Timeout mutation with CancellationTokenSource to fix thread-unsafe per-request timeout handling - Add ConfigureAwait(false) to MessageResponseBuilder async paths Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove snapshot files for tests that no longer exist: - CanGenerate_Obsolete_GetEndpoint (never had a test) - CanGenerate_UsingAliases (typo/duplicate, actual test is CanGenerate_Using_Alias) These files were blocking CI with unapproved snapshots. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…gnostic - Add [Patch] attribute and wire up PATCH HTTP method support in generator and descriptor layer - Fix ClientEndpointGenerator incremental predicate to filter to InterfaceDeclarationSyntax, avoiding unnecessary semantic analysis - Extract duplicate interface/class naming logic to EndpointNaming helper shared by EndpointDescriptor and EndpointReferenceDescriptor - Add ECL008 diagnostic: [Body] parameter cannot be nullable Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update assertions for intentionally removed behaviors: - Replace Should_Use_HttpVersion_20 with Should_Not_Force_Http_Version - Replace Should_Set_Timeout_On_HttpClient with Should_Not_Mutate_HttpClient_Timeout (timeout is now applied via CancellationTokenSource, not HttpClient.Timeout) - Split Should_Use_ApplicationJson_As_MediaType into separate Accept and ContentType tests; add Should_Not_Set_Content_When_No_Body to cover the new conditional StringContent behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dling - Add EndpointResponseTests covering IsSuccess, StatusCode, Value, and error response behavior - Add PagedResponseTests covering pagination properties - Add multi-parameter and URL encoding tests to MessageRequestBuilderTest Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
HTTP verb attributes now carry default success status codes appropriate
for each verb:
- [Get] → 200
- [Post] → 200, 201
- [Put] → 200
- [Patch] → 200, 204
- [Delete] → 200, 204
Custom codes can be overridden: [Post("/items", 200, 202)]
The generator now emits AddSuccessResponse for each status code,
enabling proper response handling for 201 Created and 204 No Content
responses without requiring manual builder configuration.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a dedicated build step for Cabazure.Client.IntegrationTests with TreatWarningsAsErrors=true. The project already has IsAotCompatible=true which enables ILLink trim analysis; this step ensures any regressions in AOT compatibility are caught in CI rather than at deployment time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace static EndpointNaming helper class with an extension method on InterfaceDeclarationSyntax per project code style preference. EndpointNaming.GetNames(syntax) → syntax.GetEndpointNames() Rename file to InterfaceDeclarationSyntaxExtensions.cs to match the extension class convention. Update copilot-instructions.md to document the preference for extension methods over static helper classes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Consolidated 7 decision files from .squad/decisions/inbox/ into .squad/decisions.md - Deleted inbox files after merger - Documented optimization session outcomes: * Thread-safe token cache (SemaphoreSlim double-check locking) * HTTP semantics fixes (conditional StringContent, HTTP version negotiation) * Thread-safe timeout via CancellationTokenSource wrapper pattern * PATCH verb support (RFC 5789) * Per-verb success status code defaults (GET=200, POST=201, DELETE=204, etc.) * 49 new unit tests covering response wrappers, attributes, options, edge cases * AOT validation integrated into CI * Extension method refactoring per user directive - No breaking changes; 164 tests passing; ship-ready for 1.0 release Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Optimization pass across Cabazure.Client’s generator + runtime to improve HTTP semantics, thread-safety, generator performance, and expand supported REST patterns (notably PATCH and per-verb success status defaults), with corresponding test/CI updates.
Changes:
- Add PATCH verb support and extend generator output to emit per-verb success status handling + new nullable-body diagnostic (ECL008).
- Improve runtime correctness/safety (token refresh concurrency control, per-request timeouts without mutating shared
HttpClient.Timeout, avoid allocating request content when no body). - Add/adjust unit + integration tests and extend CI with an AOT-related validation step.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Cabazure.Client.Tests/ClientEndpointGeneratorTests.CanGenerate_UsingAliases#TestEndpoint.g.verified.cs | Removes orphan/empty snapshot artifact. |
| test/Cabazure.Client.Tests/ClientEndpointGeneratorTests.CanGenerate_PostEndpoint#TestEndpoint.g.verified.cs | Snapshot updated for POST success codes (adds 201 Created). |
| test/Cabazure.Client.Tests/ClientEndpointGeneratorTests.CanGenerate_DeleteEndpoint#TestEndpoint.g.verified.cs | Snapshot updated for DELETE success codes (adds 204 NoContent). |
| test/Cabazure.Client.Runtime.Tests/PagedResponseTests.cs | New unit tests for PagedResponse<T> behavior. |
| test/Cabazure.Client.Runtime.Tests/EndpointResponseTests.cs | New unit tests for EndpointResponse / EndpointResponse<T> helpers and conversions. |
| test/Cabazure.Client.Runtime.Tests/Builder/MessageRequestBuilderTest.cs | Updates existing assertions + adds coverage for encoding and multi-parameter scenarios. |
| test/Cabazure.Client.IntegrationTests/PostEndpointTests.cs | Updates timeout semantics assertion + success-code expectations. |
| test/Cabazure.Client.IntegrationTests/ListEndpointTests.cs | Updates timeout semantics assertion (no HttpClient.Timeout mutation). |
| test/Cabazure.Client.IntegrationTests/GetEndpointTests.cs | Updates timeout semantics assertion (no HttpClient.Timeout mutation). |
| test/Cabazure.Client.IntegrationTests/DeleteEndpointTests.cs | Updates success-code expectations for DELETE. |
| src/Cabazure.Client/TypeConstants.cs | Adds PatchAttribute fully-qualified name constant. |
| src/Cabazure.Client/Diagnostics/DiagnosticDescriptors.cs | Updates ECL003 message to include PATCH; adds ECL008. |
| src/Cabazure.Client/Descriptors/InterfaceDeclarationSyntaxExtensions.cs | Adds extension method to centralize endpoint naming logic. |
| src/Cabazure.Client/Descriptors/EndpointReferenceDescriptor.cs | Uses centralized endpoint naming extension. |
| src/Cabazure.Client/Descriptors/EndpointMethodDescriptor.cs | Adds success-code parsing, PATCH recognition, and nullable-body validation. |
| src/Cabazure.Client/Descriptors/EndpointDescriptor.cs | Uses centralized endpoint naming extension. |
| src/Cabazure.Client/ClientEndpointGenerator.cs | Narrows incremental predicate, adds PATCH HttpMethod emission, and emits multiple success responses. |
| src/Cabazure.Client.Runtime/PutAttribute.cs | Adds SuccessStatusCodes parameter/property with defaults. |
| src/Cabazure.Client.Runtime/PostAttribute.cs | Adds SuccessStatusCodes parameter/property with defaults. |
| src/Cabazure.Client.Runtime/PatchAttribute.cs | Introduces new PATCH verb attribute with default success codes. |
| src/Cabazure.Client.Runtime/HttpRequestMessageExtensions.cs | Uses HttpRequestMessage.Options on NET5+ (falls back to .Properties). |
| src/Cabazure.Client.Runtime/GetAttribute.cs | Adds SuccessStatusCodes parameter/property with defaults. |
| src/Cabazure.Client.Runtime/DeleteAttribute.cs | Adds SuccessStatusCodes parameter/property with defaults. |
| src/Cabazure.Client.Runtime/Builder/MessageResponseBuilder.cs | Adds ConfigureAwait(false) to async read paths. |
| src/Cabazure.Client.Runtime/Builder/MessageRequestBuilder.cs | Avoids creating content when no body; stops forcing HTTP/2. |
| src/Cabazure.Client.Runtime/Builder/HttpClientExtensions.cs | Introduces per-request timeout wrapper (HttpClientWithOptions). |
| src/Cabazure.Client.Runtime/Authentication/BearerTokenProvider.cs | Adds semaphore-based token refresh coordination + disposal. |
| src/Cabazure.Client.Runtime/Authentication/AzureAuthenticationHandler.cs | Adds ConfigureAwait(false) on async calls. |
| .squad/decisions.md | Documents optimization backlog decisions and executed items. |
| .squad/agents/ripley/history.md | Records security/API analysis context and findings. |
| .squad/agents/hudson/history.md | Records testing analysis and snapshot cleanup history. |
| .squad/agents/dallas/history.md | Records implementation analysis and applied patterns. |
| .github/workflows/ci.yml | Adds a build step labeled as AOT compatibility check. |
| .github/copilot-instructions.md | Documents preference for extension methods over static helpers. |
- Revert WithRequestOptions to return HttpClient (backward compatible) - Add SendAsync extension on HttpClient taking IRequestOptions? and CancellationToken, handling per-request timeout internally - Update generator to emit client.SendAsync(request, options, ct) directly instead of chaining WithRequestOptions().SendAsync() - Remove HttpClientWithOptions class from public API - Fix GetHttpStatusCodeExpression to return full expressions including cast (e.g. (HttpStatusCode)418) for non-mapped status codes - Regenerate all affected snapshots Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add ECL008 (BodyParameterCannotBeNullable) diagnostic test - Add value-type check for [Body] parameters: emit ECL005 when body param is a value type, since WithBody<T> requires T : class - Add [Patch] generator snapshot test with approved verified snapshot - Fix Should_URL_Encode_Special_Characters_In_Path_Parameters: use InlineData template param instead of hardcoded string - Fix decisions.md: PUT default status code is 200 (not 200+204) - Fix decisions.md: describe SendAsync extension approach (not wrapper) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rickykaare
force-pushed
the
feat/optimization-backlog
branch
from
9330497 to
27b9853
Compare
…BC-01) Add back the original single-argument constructor to all five HTTP verb attributes (Get, Post, Put, Delete, Patch) by chaining to the params constructor. This preserves the original IL signature that binary consumers depend on, preventing MissingMethodException in assemblies compiled against the previous API surface. Also fixes DeleteAttribute which had its params constructor and SuccessStatusCodes assignment commented out, leaving the property unset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…TTP/2 (BC-03) Add a nullable Version property to ClientRequestOptions so consumers can opt into HTTP/2 (or any version) per-request without the old hardcoded behaviour. When set, the version is applied to the HttpRequestMessage in ConfigureHttpRequest(); when unset, HttpClient version negotiation is unchanged. PagedRequestOptions inherits from ClientRequestOptions and gains the property automatically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…opt-in HTTP/2 (BC-03)" This reverts commit b2428ac.
Set HttpRequestMessage.Version = new Version(2, 0) unconditionally in MessageRequestBuilder.Build(). HttpRequestMessage.Version exists in netstandard2.0 — no platform guard required. HttpVersionPolicy.RequestVersionOrLower (the default on .NET 5+) ensures graceful fallback to HTTP/1.1 if the server does not support HTTP/2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive optimization pass addressing security vulnerabilities, performance bottlenecks, and API completeness gaps identified during the 1.0 audit. All changes are backward-compatible at both source and binary level.
Changes
🔒 Security & Thread-Safety
BearerTokenProvider— Replaces the naive cache check with a SemaphoreSlim double-check pattern: fast path reads the cache lock-free; slow path acquires the semaphore and double-checks before refreshing. Eliminates concurrent token-refresh storms under high load. ImplementsIDisposable. All async paths addConfigureAwait(false).HttpRequestMessageExtensions— GuardsHttpRequestMessage.Optionsvs.Propertieswith#if NET5_0_OR_GREATER, avoiding the obsolete.PropertiesAPI on .NET 5+ while preserving netstandard2.0 compatibility.HttpClient.Timeoutmutation (shared state, not thread-safe) and replaces it with aCancellationTokenSource.CancelAfter()wrapper per request.HttpClientis shared across concurrent calls; mutatingTimeoutmid-flight is a data race.⚡ Performance
StringContentallocation —MessageRequestBuildernow only createsStringContentwhen a[Body]parameter is actually present. Previously, every request allocated and sent an empty JSON body even for GET/DELETE, which some APIs reject.(_, _) => truetonode is InterfaceDeclarationSyntax. Non-interface syntax nodes no longer trigger the semantic analysis pipeline, reducing unnecessary work during incremental rebuilds.client.DefaultRequestVersion = new Version(2, 0)withHttpVersionPolicy.RequestVersionOrLowerin the generatedAddCabazureClient<TOptions>DI setup, guarded by#if NET5_0_OR_GREATER. This prefers HTTP/2 where available and gracefully falls back to HTTP/1.1, without hardcoding the version on eachHttpRequestMessage.🆕 Features
[Patch]verb — NewPatchAttributewith full generator support (descriptor parsing, code emission, snapshot test). PATCH is RFC 5789; users expect it for partial-update endpoints.[Body]parameter is declared nullable. Nullable body parameters compile fine but fail at runtime; catching this in the generator produces an actionable error message at build time.params int[] successStatusCodes. Defaults match HTTP semantics:GET→200,POST→200,201,PUT→200,PATCH→200,204,DELETE→200,204. The generator emits one.AddSuccessResponsecall per code, so response helpers work correctly out of the box.🧪 Tests
EndpointResponse<T>tests covering status helpers, typed body deserialization, and error pathsPagedResponse<T[]>tests covering pagination state and continuation token handling⚙️ CI
dotnet build /p:TreatWarningsAsErrors=trueon theIntegrationTestsproject. The project already declares<IsAotCompatible>true</IsAotCompatible>but had no CI gate; trim warnings now fail the build on any platform without requiring a fulldotnet publish --aot.🔧 Refactoring
EndpointNaming→InterfaceDeclarationSyntaxExtensions— TheEndpointNamingstatic helper class is replaced by extension methods onInterfaceDeclarationSyntax. Duplicate naming logic that existed in two descriptors is now in one place, and call sites read more naturally as fluent expressions.Backward Compatibility
Fully backward-compatible — no action required by consumers.
The verb attributes (
[Get],[Post],[Put],[Delete],[Patch]) added aparams int[] successStatusCodesoverload. The original single-argument constructors ((string routeTemplate)) are preserved via constructor chaining (Array.Empty<int>()), so:MissingMethodException.All other changes are implementation-internal (private fields, generated code shape, CI configuration).
Test Results
170 tests — all passing.
Cabazure.Client.Runtime.Tests)Cabazure.Client.IntegrationTests)Cabazure.Client.Tests)