Repository navigation
Refuse an empty variable batch in the HTTP transport - #10441
Merged
Merged
Conversation
github-actions
Bot
added
📚 documentation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical resource-leak issue remains, along with missing persisted-operation and multipart regression coverage.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This pull request rejects empty HTTP variable batches before execution, preserves in-process guards, and updates related tests and documentation.
Changes:
- Validates
variables: []across HTTP parsing paths. - Adds Core, Fusion, and transport regression coverage.
- Documents status-code and migration behavior.
| File | Reviewed changes |
|---|---|
website/content/docs/hotchocolate/server/batching.md |
Documents empty-batch rejection; legacy JSON status wording needs qualification. |
website/content/docs/hotchocolate/migrating/migrate-from-16-6-to-16-7.md |
Documents migration behavior; should mention the legacy JSON 200 response. |
website/content/docs/fusion/cost-analysis.md |
Updates cost-analysis context. |
src/HotChocolate/Fusion/test/Fusion.Execution.Tests/Execution/Pipeline/OperationExecutionMiddlewareTests.cs |
Covers Fusion execution guards. |
src/HotChocolate/Fusion/test/Fusion.AspNetCore.Tests/VariableBatchingTests.cs |
Covers Fusion HTTP variable batching. |
src/HotChocolate/Fusion/test/Fusion.AspNetCore.Tests/CostReportingTests.cs |
Updates cost-reporting coverage. |
src/HotChocolate/Core/test/Execution.Tests/Pipeline/OperationExecutionMiddlewareTests.cs |
Covers Core execution guards. |
src/HotChocolate/AspNetCore/test/AspNetCore.Tests/GraphQLOverHttpSpecTests.cs |
Covers POST, GET, and request batches; one test name needs correction. |
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Utilities/ErrorHelper.cs |
Adds the request error helper. |
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.resx |
Adds localized error text. |
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.Designer.cs |
Adds the generated resource accessor. |
src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/ExecutorSession.cs |
Adds parsing validation; disposal handling and persisted/multipart coverage require changes. |
Files not reviewed (1)
- src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.Designer.cs: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Patch coverage100.0% of changed lines covered (53/53)
Project coverage: 58.8% (298926/508115 lines) |
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Add HTTP QUERY coverage and Legacy application/json coverage for empty variable batches.
Review effort: Lite
Findings: None
Resolved since last review (2)
Files not reviewed (1)
- src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.Designer.cs: Generated file
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
Files not reviewed (1)
- src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Properties/AspNetCorePipelineResources.Designer.cs: Generated file
This was referenced Oct 25, 2026
Merged
This was referenced Oct 31, 2026
Closed
[nuget][SUI_Matcher]- Bump the sui-package-updates group with 21 updates
DFE-Digital/SUI_Matcher#424
Open
Bump HotChocolate.AspNetCore.Pipeline and HotChocolate.Fusion.AspNetCore
feddi-dev/feddi-gateway#109
Open
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
variablesfield is an empty array before any interceptor or execution runs, whether or not the operation declares variables. It answers like an empty request batch: 400 underDraft20250508and 422 underDraft20260903, with "A variable batch request must contain at least one variable set." andHC0009. An operation that declares no variables previously executed once."variables": null,"variables": {}, and an omittedvariablesstay single requests.ExecutorSessionparse method, so POST, GET, multipart, HTTP QUERY, request batches, and the persisted-operation routes all refuse it, and a request batch with one such entry is refused as a whole. WebSocket subscriptions do not use these methods.Test plan
GraphQLOverHttpSpecTestspins the POST answer for operations with and without declared variables under both transport versions, the GET answer, and the refused request batch.VariableBatchingTestspins the same answers for the Fusion gateway.OperationExecutionMiddlewareTestsinExecution.TestsandFusion.Execution.Testspins each execution guard; removing either guard fails its test.AspNetCore.TestsandFusion.AspNetCore.Testspass in full.