Repository navigation
[OpenAPI] Avoid double PipeReader.AdvanceTo when reading @body - #9961
tobias-tengler merged 12 commits into
Conversation
…Body DynamicEndpointMiddleware.BuildVariablesAsync advanced the request body PipeReader on every loop iteration and then advanced it a second time after the loop, with no intervening ReadAsync. Kestrel's request pipe reader enforces the PipeReader contract and throws InvalidOperationException ("No reading operation to complete."), which the middleware's catch-all turns into an empty 500. This broke every OpenAPI endpoint that binds the request body via @Body (POST/PUT/PATCH). Restructure the read loop so AdvanceTo is only called inside the loop while more data is pending, then exactly once after parsing the completed buffer. The existing integration tests do not catch this because they run on ASP.NET TestServer, whose in-memory body PipeReader tolerates the redundant AdvanceTo; only a real Kestrel host reproduces it.
Adds POST and PUT @Body endpoint tests that run on a real Kestrel host. These fail before the fix (empty 500 from the double PipeReader.AdvanceTo) and pass after it. The existing HttpEndpointIntegrationTests run on TestServer, whose in-memory body PipeReader tolerates the redundant AdvanceTo, so they cannot catch this regression.
tobias-tengler
left a comment
There was a problem hiding this comment.
Thank you for your contribution @hemed :)
I left a few comments. Once addressed we can get this in!
Addresses review feedback: use braces on the guard clauses, and move the final AdvanceTo into a finally block so the reader is advanced even when the empty-body guard or the JSON parse throws — otherwise Kestrel violates the PipeReader contract while draining the request body.
|
Thanks for the review @tobias-tengler! Addressed all three:
I re-verified on a real Kestrel host that normal, empty, large (multi-read), and parse-failure request bodies all behave correctly (no "No reading operation to complete."). |
|
@hemed Thanks :) |
The PUT route is /users/{userId:$user.id}; including id in the body trips the
adapter's 'Unknown field' guard (route-param fields must not also be in the
body). Removing it lets the route segment supply user.id.
|
@tobias-tengler Can you try running the test again?. Fixed the failing |
Problem
Every OpenAPI-adapter endpoint that binds the request body via
@body(
POST/PUT/PATCH) returns an empty HTTP 500 when hosted on Kestrel.GETendpoints (no@body) are unaffected.DynamicEndpointMiddleware.BuildVariablesAsyncreads the body like this:The loop already calls
AdvanceTofor the final (completed)ReadResult, thenthe post-loop
AdvanceTo(result.Buffer.End)advances a second time with nointervening
ReadAsync. That violates thePipeReadercontract, so Kestrelthrows:
The middleware's
catch { ... Results.InternalServerError() ... }swallows it,so the caller sees a bare 500 with nothing logged.
There is also a latent correctness issue:
Parse(result.Buffer)ran afterAdvanceTohad already been called for that buffer.Reproduced on
HotChocolate.Adapters.OpenApi16.1.4, 16.2.2 and 16.3.0-p.1 — themethod is identical in all three.
How to reproduce
Host any GraphQL operation that has an
@bodyvariable over POST on Kestrel(not
TestServer— see below) and call it with a JSON body:Confirming it is adapter-side and not the operation/schema:
POST /graphql(200).GETendpoint on the same schema works.Content-Type→ 415 and an empty body → 400, so the failure is afterrequest validation, inside the body read.
JsonValueParser.Parse→ValueJsonFormatter.Formatover the samebytes with a plain
ReadOnlySequence<byte>succeeds — the throw only happensagainst a real Kestrel
PipeReader.Fix
Keep the
do/while, but only advance while more data is pending. The final(completed) read is left un-advanced inside the loop, so the single post-loop
AdvanceToafter parsing is the only advance for it:I verified the read pattern in isolation against a real Kestrel host: the
original (double
AdvanceTo) endpoint returns 500, the new pattern returns200 (checked with both a small body and a multi-read 5 MB body).
Tests
Added
Endpoints/KestrelHttpEndpointIntegrationTests.cswith POST and PUT@bodyregression tests that run on a real Kestrel host (WebHostBuilder+UseKestrel, dynamic port). They fail before the fix (empty 500) and pass after.Why the existing tests didn't catch this:
HttpEndpointIntegrationTests(including the
POST /usersbody test) run on ASP.NETTestServer, whosein-memory request-body
PipeReadertolerates the redundantAdvanceTo. Only areal Kestrel host enforces the
PipeReadercontract and throws, so a Kestrel-hosted test is required to guard this.