Repository navigation
Report the reason for rejected batch requests to server diagnostics - #10372
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new diagnostic errors for “batching disabled” are emitted without passing through session.Handle(...), making diagnostics inconsistent with other request errors and bypassing custom IErrorHandler behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves server diagnostics for rejected batch requests by emitting IServerDiagnosticEvents.HttpRequestError with a more specific error explaining why batching was rejected, while keeping the client-facing response unchanged for the “batching disabled” cases.
Changes:
- Add dedicated error messages for “variable batching disabled” and “request batching disabled” via
ErrorHelper+ resource strings. - Emit
HttpRequestErrorwhen variable batching is disabled and when variable batch size exceedsMaxBatchSize. - Reorder/adjust operation-batch handling in
HttpPostMiddlewareBaseto report the correct rejection reason, and add tests validating diagnostic output and response body.
File summaries
| File | Description |
|---|---|
| src/HotChocolate/AspNetCore/test/AspNetCore.Tests/HttpPostMiddlewareTests.cs | Adds coverage asserting diagnostic messages and unchanged response bodies for batching rejection scenarios. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Utilities/ErrorHelper.cs | Introduces new IError factories for batching-disabled diagnostic reasons. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.resx | Adds localized resource strings for the new batching-disabled messages. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.Designer.cs | Updates generated resource accessors for new resource keys. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/HttpPostMiddlewareBase.cs | Emits diagnostic reasons for request/operation batching disabled and adjusts condition ordering. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/ExecutorSession.cs | Emits diagnostics for variable batching disabled and max variable batch size exceeded. |
Review details
Files not reviewed (1)
- src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.Designer.cs: Generated file
Suppressed comments (1)
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/HttpPostMiddlewareBase.cs:202
- The diagnostic error for disabled request batching is emitted without applying session.Handle(...). For consistency with other request errors (and to allow custom IErrorHandler implementations to affect diagnostics), pass the error through session.Handle before calling HttpRequestError.
session.DiagnosticEvents.HttpRequestError(
context,
ErrorHelper.RequestBatchingDisabled());
- Files reviewed: 5/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Patch coverage100.0% of changed lines covered (27/27)
Project coverage: 57.9% (288200/497786 lines) |
Summary
IServerDiagnosticEvents.HttpRequestErrorwith an error that names the disabled mode and points atGraphQLServerOptions.Batching. The client response is unchanged and still saysInvalid GraphQL Request.with codeHC0009.ExecutorSession.ExecuteSingleAsyncpreviously returned without raising any diagnostic event, both for disabled batching and for an exceededMaxBatchSize. Both now raiseHttpRequestError, so listeners such as the OpenTelemetry instrumentation record the reason on the request span.HttpPostMiddlewareBasechecks the batching flag before thebatchOperationsvalue, so the reported reason is the check that rejected the request. A missing or unparseablebatchOperationsvalue keeps reporting the generic error.Test plan
HttpPostMiddlewareTestsuse a recordingServerDiagnosticEventListenerto assert the reported message and the unchanged response body for a variable batch, a request batch, and an operation batch with batching disabled, a variable batch overMaxBatchSize, and an operation batch with an invalidbatchOperationsvalue.HttpPostMiddlewareTestsandGraphQLOverHttpSpecTestspass.