Return 400 on JSON parse failure for application/json#9784
Open
glen-84 wants to merge 4 commits into
Open
Conversation
glen-84
changed the title
application/json (latest)application/json
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates HotChocolate’s latest GraphQL-over-HTTP transport behavior to return HTTP 400 (instead of 200) when a request body cannot be parsed as JSON and the client accepts application/json, aligning with the graphql-over-http spec guidance while keeping legacy transport behavior unchanged for backward compatibility.
Changes:
- Honor middleware-proposed HTTP status codes for
application/jsonresponses on the latest transport (legacy remains “always 200” forapplication/json). - Distinguish document syntax errors from request-interpretation failures so document syntax errors can keep
application/jsonat 200 whileapplication/graphql-response+jsonstays 400. - Update spec tests to assert 400 for latest +
application/jsonwhen the request body is unparseable JSON.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/HotChocolate/AspNetCore/test/AspNetCore.Tests/GraphQLOverHttpSpecTests.cs | Updates expectations for status codes in the spec matrix tests (latest + application/json invalid JSON now 400). |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/HttpPostMiddlewareBase.cs | Differentiates request-interpretation failures vs document syntax errors when proposing status codes. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Formatters/DefaultHttpResponseFormatter.cs | Applies proposed status codes for application/json on latest transport, preserving legacy behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
BCF8("SHOULD use 400 status code on JSON parsing failure when accepting application/json") fired because HotChocolate returned 200 instead of 400 when a request body is unparseable JSON and the client acceptsapplication/json.DefaultHttpResponseFormatternow honors a proposed400for theapplication/jsonresponse content-type on the latest transport (the §6.4.1.1.1 / §6.4.1.1.2 "cannot interpret the request" cases). Every other case, including field errors and unexpected server errors, stays200per §6.4.1. TheLegacytransport keeps its pre-spec "always 200" behavior for backward compatibility.HttpPostMiddlewareBasenow distinguishes request-interpretation failures (invalid JSON, missing query) which propose 400 per §6.4.1.1.1, from GraphQL document syntax errors, which leave the status unset soapplication/jsonstays 200 (§6.4.1.1.3) andapplication/graphql-response+jsonstays 400.Behavior matrix
application/jsonapplication/jsonapplication/graphql-response+jsonapplication/jsonTest plan
HotChocolate.AspNetCore.Testson net10.0: 411 passed, 2 skipped, 0 failed.Query_No_Bodyupdated so only theLatest+application/jsonrow asserts 400; allLegacyrows remain 200.ValidationError/ValidationError2unchanged and green.