Skip to content

Fix missing pipeline state answering 400 or 422 instead of 500 - #10430

Merged
glen-84 merged 2 commits into
mainfrom
gai/pipeline-state-errors-return-500
Sep 24, 2026
Merged

glen-84 merged 2 commits into
mainfrom
gai/pipeline-state-errors-return-500

Conversation

@glen-84

@glen-84 glen-84 commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Summary

  • A pipeline step that finds the request state an earlier step provides missing now answers 500: no document at validation, no validated document at compilation, and no operation or no coerced variables at execution. These StateInvalid* results had no status pin, so the formatter's fallback answered 400 under Draft20250508 and 422 under Draft20260903. They now carry the InternalServerError pin that RequestTimeout and the cost-analysis state errors already use. The document validation middleware is shared with the Fusion gateway, so its guard answers the same way there.
  • Client input that reaches the same guards stays a request error. A request that carries only a document ID or hash, which neither the document cache nor persisted operation storage resolved, keeps its current answer. An empty variable batch that no cost analyzer rejected keeps its status and gets its own message, "A variable batch request must contain at least one variable set."
  • The status moves for application/graphql-response+json under every transport version and for application/json under Draft20260903. application/json under Legacy and Draft20250508 stays 200.

Test plan

  • New GraphQLOverHttpSpecTests cover each guard over HTTP under Draft20250508 and Draft20260903. A request middleware registered before the guarded middleware removes the state that middleware checks, so the real guard produces the result. At validation this covers a document parsed from the request, one loaded from persisted operation storage, and one taken from the document cache.
  • The request-error cases are covered by an ID-only request to a schema without persisted operations and by an empty variable batch with the cost analyzer skipped.
  • HotChocolate.AspNetCore.Tests, HotChocolate.Execution.Tests, HotChocolate.CostAnalysis.Tests, HotChocolate.Fusion.AspNetCore.Tests, and the four HotChocolate.PersistedOperations.*.Tests projects pass on net10.0 with -p:EnforceCodeStyleInBuild=true.

Follow-ups

  • Fusion: an empty variable batch on a gateway that skips cost analysis most likely reaches context.VariableValues[0] in Fusion's OperationExecutionMiddleware and answers 500. This is read from the code, not probed.
  • Core: an empty variable batch for an operation that declares no variables executes once and answers 200, because variable coercion assigns one empty variable set in that case.

Copilot AI lite review requested due to automatic review settings September 24, 2026 15:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

One moderate document-state classification issue and two test-name nits remain.

Review effort: Lite
Findings: None

What changed in this PR

Updates GraphQL pipeline error classification so internal state failures return HTTP 500 while client errors retain transport-specific statuses.

Changes:

  • Adds internal-server status pins for invalid pipeline state.
  • Handles empty variable batches with a dedicated request error.
  • Adds transport-version HTTP integration coverage.
File Summary
src/​HotChocolate/​Core/​src/​Types/​Properties/​Resources.resx Adds the empty variable-batch message.
src/​HotChocolate/​Core/​src/​Types/​Properties/​Resources.Designer.cs Exposes the generated resource accessor.
src/​HotChocolate/​Core/​src/​Types/​Execution/​Pipeline/​OperationExecutionMiddleware.cs Detects empty variable batches.
src/​HotChocolate/​Core/​src/​Types/​Execution/​ErrorHelper.cs Adds status pins and batch error handling.
src/​HotChocolate/​Core/​src/​Execution.Pipeline/​ErrorHelper.cs Separates request and internal document errors.
src/​HotChocolate/​Core/​src/​Execution.Pipeline/​DocumentValidationMiddleware.cs Classifies unresolved document requests.
src/​HotChocolate/​AspNetCore/​test/​AspNetCore.Tests/​GraphQLOverHttpSpecTests.cs Covers transport status behavior.
Files not reviewed (1)
  • src/HotChocolate/Core/src/Types/Properties/Resources.Designer.cs: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Patch coverage

100.0% of changed lines covered (42/42)

File Covered Changed Patch %
…/Core/src/Execution.Pipeline/DocumentValidationMiddleware.cs 8 8 100.0% 🟢
src/HotChocolate/Core/src/Execution.Pipeline/ErrorHelper.cs 7 7 100.0% 🟢
src/HotChocolate/Core/src/Types/Execution/ErrorHelper.cs 22 22 100.0% 🟢
…/src/Types/Execution/Pipeline/OperationExecutionMiddleware.cs 5 5 100.0% 🟢

Project coverage: 58.9% (300028/509797 lines)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Files not reviewed (1)
  • src/HotChocolate/Core/src/Types/Properties/Resources.Designer.cs: Generated file

@glen-84
glen-84 merged commit 2959f63 into main Sep 24, 2026
157 checks passed
@glen-84
glen-84 deleted the gai/pipeline-state-errors-return-500 branch September 24, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants