Support sync interface members & enum names#2068
Conversation
Allow synchronous return types only for generated/non-public interface members by building a generated sync function path in RequestBuilderImplementation (BuildGeneratedSyncFuncForMethod, DeserializeSyncResponse). Update RestMethodInfo to enforce the new rule. Enhance SystemTextJsonContentSerializer enum converter to map serialized names (including JsonStringEnumMemberName on .NET 9+) both ways and prefer annotated/serialized names when reading/writing. Add tests (guarded by NET9_0_OR_GREATER) to validate JsonStringEnumMemberName behavior and a SyncCapableMockHttpMessageHandler for sync test scenarios. Add a Blazor WebAssembly example (BlazorWasmIssue2065) demonstrating Refit usage and include it in the solution.
Enhance the System.Text.Json enum converter to attempt case-insensitive name lookups by adding a second names-to-values map using StringComparer.OrdinalIgnoreCase and making GetNamesToValues accept a comparer. This makes deserialization resilient to case differences in incoming JSON. Also update the BlazorWasm example project file: replace NoPack with IsPackable=false, bump Microsoft.AspNetCore.Components.WebAssembly packages from 10.0.0 to 10.0.5, and adjust file encoding.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2068 +/- ##
==========================================
- Coverage 82.48% 81.16% -1.32%
==========================================
Files 38 38
Lines 2780 2878 +98
Branches 449 476 +27
==========================================
+ Hits 2293 2336 +43
- Misses 379 428 +49
- Partials 108 114 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses two reported regressions/feature gaps: (1) enabling synchronous return shapes only for generated/non-public Refit interface members (avoiding problematic sync waits for normal/public APIs), and (2) improving System.Text.Json enum (de)serialization to support serialized/annotated enum member names (including JsonStringEnumMemberName on .NET 9+) with added case-insensitive matching. It also adds tests and a Blazor WASM example project demonstrating the scenarios from #2065 and #2067.
Changes:
- Update Refit’s return-type enforcement to allow sync return types only for explicit interface members or non-public methods, and add a new sync execution path in
RequestBuilderImplementation. - Enhance
SystemTextJsonContentSerializerenum converter to round-trip annotated/serialized enum names and add case-insensitive name matching. - Add tests for
JsonStringEnumMemberNamebehavior (NET9+) and add a Blazor WASM example project to the solution.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/BlazorWasmIssue2065/** | New Blazor WASM example demonstrating the reported issues and expected behavior. |
| Refit/SystemTextJsonContentSerializer.cs | Enum converter updated to support serialized/annotated names and case-insensitive lookup. |
| Refit/RestMethodInfo.cs | Enforces sync return types only for explicit interface members or non-public methods. |
| Refit/RequestBuilderImplementation.cs | Adds a new “generated sync” execution path for allowed sync members. |
| Refit.Tests/SerializedContentTests.cs | Adds NET9+ tests validating JsonStringEnumMemberName round-tripping and default converter usage. |
| Refit.Tests/ExplicitInterfaceRefitTests.cs | Adjusts tests to support sync execution scenarios. |
| Refit.sln | Includes the new Blazor WASM example in the solution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| readonly Dictionary<string, object> namesToValues = GetNamesToValues( | ||
| enumType, | ||
| StringComparer.Ordinal | ||
| ); | ||
| readonly Dictionary<string, object> namesToValuesIgnoreCase = GetNamesToValues( | ||
| enumType, | ||
| StringComparer.OrdinalIgnoreCase | ||
| ); |
There was a problem hiding this comment.
CamelCaseStringEnumConverter now adds a case-insensitive lookup map (namesToValuesIgnoreCase), but there are no tests covering the new behavior. Adding a test that deserializes an enum value with different casing (and asserting the correct member is chosen) would help prevent regressions, especially for enums where members differ only by case.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@ChrisPulman I've opened a new pull request, #2069, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@ChrisPulman I've opened a new pull request, #2070, to work on those changes. Once the pull request is ready, I'll request review from you. |
…zation (#2069) * Initial plan * Add case-insensitive enum deserialization tests for CamelCaseStringEnumConverter Co-authored-by: ChrisPulman <4910015+ChrisPulman@users.noreply.github.com> Agent-Logs-Url: https://github.com/reactiveui/refit/sessions/457806a0-8430-40b3-8e4e-26f6590217ff --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ChrisPulman <4910015+ChrisPulman@users.noreply.github.com>
…ApiResponse, disposal) (#2070) * Initial plan * Fix sync pipeline to replicate full BuildCancellableTaskFuncForMethod behavior - Rewrite BuildGeneratedSyncFuncForMethod as a dispatcher that handles void return separately and delegates non-void to a new generic method. - Add BuildGeneratedSyncFuncForMethodGeneric<T,TBody> that mirrors the full async pipeline: runs ExceptionFactory (skipped for HttpResponseMessage), honours IsApiResponse, uses ShouldDisposeResponse for cleanup, and surfaces DeserializationExceptionFactory on deserialization errors. - Add DeserializeContentSync<T> handling all content types: HttpResponseMessage, HttpContent, Stream, string, and deserialised T. - Fix RestMethodInfo sync branch to properly decompose IApiResponse<T> / ApiResponse<T> return types, setting DeserializedResultType to the inner type just like the async Task<IApiResponse<T>> path does. - Add SyncVoid to ReturnTypeInfo enum and handle it in Parser.cs and Emitter.cs so void-returning sync stub methods emit a plain call instead of the invalid 'return (void)...' pattern. - Add comprehensive tests covering error responses, HttpResponseMessage, HttpContent, Stream, IApiResponse<T>, and void sync methods. Co-authored-by: ChrisPulman <4910015+ChrisPulman@users.noreply.github.com> Agent-Logs-Url: https://github.com/reactiveui/refit/sessions/7e184030-9d18-44ec-aff4-305b2c7e973d * Fix spelling: 'occured' -> 'occurred' in new sync error messages Co-authored-by: ChrisPulman <4910015+ChrisPulman@users.noreply.github.com> Agent-Logs-Url: https://github.com/reactiveui/refit/sessions/7e184030-9d18-44ec-aff4-305b2c7e973d --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ChrisPulman <4910015+ChrisPulman@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var isExplicitInterfaceMember = restMethod.MethodInfo.Name.IndexOf('.') >= 0; | ||
| var isNonPublic = !restMethod.MethodInfo.IsPublic; | ||
| if (isExplicitInterfaceMember || isNonPublic) | ||
| { | ||
| return BuildGeneratedSyncFuncForMethod(restMethod); |
There was a problem hiding this comment.
BuildRestResultFuncForMethod only has branches for Task, Task, and IObservable. Interfaces returning ValueTask (which RestMethodInfo explicitly allows and the exception message advertises) will currently fall through into the sync/explicit-member branch and either throw or be treated as a sync method. Add an explicit ValueTask branch (likely by reusing the Task pipeline and wrapping/unwrapping as needed) so ValueTask return types behave as supported.
| var factory = BuildRequestFactoryForMethod( | ||
| restMethod, | ||
| client.BaseAddress.AbsolutePath, | ||
| paramsContainsCancellationToken: false | ||
| ); |
There was a problem hiding this comment.
The generated sync path always calls BuildRequestFactoryForMethod(..., paramsContainsCancellationToken: false) and later calls SendAsync without a CancellationToken. If a (non-public / explicit) sync method includes a CancellationToken parameter, the token won’t be stripped from paramList (so it may get bound into query/body) and cancellation won’t be honored for SendAsync or AuthorizationHeaderValueGetter. Mirror the async path by using restMethod.CancellationToken != null, extracting the token from paramList, and passing it through to SendAsync and the auth-header getter.
| var factory = BuildRequestFactoryForMethod( | ||
| restMethod, | ||
| client.BaseAddress.AbsolutePath, | ||
| paramsContainsCancellationToken: false | ||
| ); | ||
| using var rq = factory(paramList); | ||
| using var resp = client | ||
| .SendAsync(rq, HttpCompletionOption.ResponseHeadersRead) | ||
| .GetAwaiter() | ||
| .GetResult(); |
There was a problem hiding this comment.
The sync-void branch hard-codes paramsContainsCancellationToken: false and calls SendAsync without a CancellationToken. If a generated/non-public void method includes a CancellationToken parameter, it won’t cancel and the token may be treated as a normal argument when building the request. Consider matching BuildVoidTaskFuncForMethod by stripping/passing the CancellationToken when restMethod.CancellationToken != null.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Refactors request building/execution to centralize async logic and surface correct request metadata. Introduces RunSynchronous helpers and async implementations (ExecuteRequestAsync, ExecuteVoidRequestAsync, BuildRequestMessageForMethodAsync) so synchronous funcs invoke the same async pipeline. AuthorizationHeaderValueGetter is now awaited when building requests. ApiResponse creation now preserves the original HttpRequestMessage/RequestUri and fixes disposal/error/deserialization handling (including improved Exception/DeserializationExceptionFactory usage). Adds tests: awaiting auth header getter and ensuring generated sync ApiResponse preserves RequestMessage; updates expectations in explicit-interface tests to use HasResponseError and assert request properties.
What kind of change does this PR introduce?
fixes #2065 and #2067
What is the current behavior?
#2065
#2067
What is the new behavior?
Allow synchronous return types only for generated/non-public interface members by building a generated sync function path in RequestBuilderImplementation (BuildGeneratedSyncFuncForMethod, DeserializeSyncResponse).
Update RestMethodInfo to enforce the new rule. Enhance SystemTextJsonContentSerializer enum converter to map serialized names (including JsonStringEnumMemberName on .NET 9+) both ways and prefer annotated/serialized names when reading/writing.
Add tests (guarded by NET9_0_OR_GREATER) to validate JsonStringEnumMemberName behavior and a SyncCapableMockHttpMessageHandler for sync test scenarios.
Add a Blazor WebAssembly example (BlazorWasmIssue2065) demonstrating Refit usage and include it in the solution.
Enhance the System.Text.Json enum converter to attempt case-insensitive name lookups by adding a second names-to-values map using StringComparer.OrdinalIgnoreCase and making GetNamesToValues accept a comparer.
This makes deserialization resilient to case differences in incoming JSON.
Also update the BlazorWasm example project file: mark with IsPackable=false, bump Microsoft.AspNetCore.Components.WebAssembly packages from 10.0.0 to 10.0.5, and adjust file encoding.
What might this PR break?
Please check if the PR fulfills these requirements
Other information: