[csharp][restsharp] add throwOnAnyError option to surface client errors#23663
Open
mjansrud wants to merge 4 commits intoOpenAPITools:masterfrom
Open
[csharp][restsharp] add throwOnAnyError option to surface client errors#23663mjansrud wants to merge 4 commits intoOpenAPITools:masterfrom
mjansrud wants to merge 4 commits intoOpenAPITools:masterfrom
Conversation
By default, RestSharp swallows deserialization and transport exceptions into RestResponse.ErrorException, and the generated ToApiResponse<T> in this template only carries ErrorText — the actual exception is dropped. Combined with a generated GetXxxAsync that returns Data directly, callers silently receive null on any deserialization failure (e.g. a required property missing in the upstream JSON). The error never reaches application logs or APM. Add an opt-in `throwOnAnyError` switch (default false, restsharp library only) that sets `ThrowOnAnyError = true` on RestClientOptions, making RestSharp rethrow the original ApiException(500, ...) that the generated deserializer already throws. The exception then propagates to the caller and into normal application error handling. Default kept off to preserve backwards compatibility — opt in when you want bugs to surface instead of silently producing null/[].
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/csharp/ApiClient.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/csharp/ApiClient.mustache:471">
P2: `throwOnAnyError` is lost when retry policies are enabled: Polly captures the exception, the fallback response only stores `ErrorException`, and `ToApiResponse` does not surface it, so the generated `ApiResponse` still hides the client error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…gured Polly's ExecuteAndCapture catches the rethrown ApiException, so without this change the option is silently neutralized whenever RetryConfiguration.RetryPolicy != null: the exception ends up in RestResponse.ErrorException, which ToApiResponse discards. When throwOnAnyError is enabled, rethrow PolicyResult.FinalException from DeserializeRestResponseFromPolicyAsync so the contract is consistent across both the no-retry and retry paths. Default-off branch is byte-identical to the previous output.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/csharp/ApiClient.v790.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/csharp/ApiClient.v790.mustache:562">
P2: `throwOnAnyError` can throw `null` for handled-result policy failures, causing a `NullReferenceException` instead of surfacing the actual policy failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Polly's PolicyResult.FinalException is null when Outcome is Failure but the failure type is ResultHandledByThisPolicy (e.g. a retry policy configured with .HandleResult(...) that gives up after N retries). Throwing it directly would NRE; fall back to InvalidOperationException so the option still surfaces an error. Also restores the trailing whitespace on the if line so existing restsharp samples don't all need regenerating.
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.
PR checklist
Description
By default the C#
restsharpclient silently drops deserialization failures. The chain:Deserializer.Deserialize<T>throwsApiException(500, e.Message)on parse failure.Execute<T>catches the exception and stuffs it intoresponse.ErrorException, leavingresponse.Data = null.ToApiResponse<T>carriesErrorTextbut discardsErrorException.ExceptionFactoryonly fires on non-2xx HTTP status, so a 200 with a deserialization failure passes through silently.GetXxxAsyncreturnslocalVarResponse.Datadirectly — callers receivenullwith no signal in logs or APM.We hit this in production when an upstream service started returning JSON missing a property our schema marked as
required. Endpoints quietly returned empty lists — clients rendered nothing — there was no error to investigate. Discovering the cause required adding ad-hoc logging to the generated code.Fix
This PR adds an opt-in switch
throwOnAnyError(restsharp library only, defaultfalse) that setsRestClientOptions.ThrowOnAnyError = true. RestSharp then rethrows the originalApiExceptioninstead of swallowing it. No template logic changes, no behaviour change for existing users.Verification
Tested locally on 14 generated clients in our monorepo. Before:
GET /candidates→200 [](silent — required field missing in upstream JSON)After enabling
throwOnAnyError=true:GET /candidates→500withRequired property 'track' not found in JSON. Path '[0]'ApiException→ propagates to APM.Why default off
Keeping the default
falseto preserve backwards compatibility for users who may rely on the current null-on-deserialization-failure behaviour. Opt-in via--additional-properties throwOnAnyError=true.Summary by cubic
Adds an opt-in
throwOnAnyErroroption for C#restsharpclients to rethrow deserialization and transport errors so client failures surface instead of returning null. Default is false; existing behavior is unchanged.New Features
throwOnAnyError(C#restsharp) that setsRestClientOptions.ThrowOnAnyError = trueso exceptions bubble up.PolicyResult.FinalException(orInvalidOperationExceptionif it’s null) fromDeserializeRestResponseFromPolicyAsyncfor consistent behavior.Migration
--additional-properties throwOnAnyError=true(orconfigOptions).Written for commit 65b15cc. Summary will update on new commits. Review in cubic