Repository navigation
Fixed automatic persisted query requests being answered with the schema - #633
Open
xperiandri wants to merge 3 commits into
Open
xperiandri wants to merge 3 commits into
xperiandri wants to merge 3 commits into
Conversation
Apollo Client's persisted query link first sends only a hash of the query in extensions.persistedQuery and falls back to the full query once the server answers PersistedQueryNotSupported. The HTTP handler ignored the extension: such a GET request was answered with the introspection result, a POST request without a query with a 400 "Invalid JSON body" problem, and a request carrying both was executed, so the client never received the error it expects. - Read the extensions through an internal GQLRequestEnvelope before binding GQLRequestContent, so the public request type is unchanged, and check the extensions query string parameter of GET requests. - Answer a request whose extensions.persistedQuery is truthy, with or without a query, as Apollo Server 5.5.1 does with persisted queries disabled (requestPipeline.ts, internalErrorClasses.ts): HTTP 200, Cache-Control "private, no-cache, must-revalidate", a single PersistedQueryNotSupported error with extensions.code PERSISTED_QUERY_NOT_SUPPORTED and no data. - Left graphql-transport-ws subscribe payloads unchecked: they must carry the query, are never answered with introspection, and GraphQLWsLink sends the full query together with the operation's extensions. - Breaking change: a request with both a query and a persisted query hash is now rejected instead of executed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…g back
- The answer to a persisted query request is now {"errors": [...]} alone,
as Apollo Server writes it: GQLResponse.RequestError added documentId, and
Apollo Client 4 takes a body with a top-level member other than data,
errors and extensions for no GraphQL result, so its persisted query link
neither fell back to the full query nor disabled persisted queries.
- The extensions of a request are no longer bound as a JsonElement, which
copied them: a client could make every request allocate a copy of a body
as large as the server accepts (20 898 KB per request for a 20 MB body,
now 416 KB, as for any unknown member). PersistedQueryRequest and its
converter evaluate the truthiness of persistedQuery from the reader and
skip every other value; the GET extensions parameter goes through the same
converter.
- Wrapped the doc comments with tags in <summary>.
- Tests now check that the answer has no other member than errors, the
truthiness of more values, duplicate and nested members, and GET
extensions that do not ask for a persisted query.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The implementation deserializes every non-GET request twice, adding avoidable CPU and multipart allocation overhead.
1 open finding
What changed in this PR
Adds Apollo APQ detection so unsupported persisted-query requests receive the expected fallback response.
Changes:
- Detects truthy
extensions.persistedQueryvalues across GET, JSON, and multipart requests. - Returns Apollo-compatible error status, body, and cache header.
- Adds coverage and release notes for the behavior.
| File | Description |
|---|---|
GraphQLRequestHandler.fs |
Implements APQ detection and rejection. |
RequestHandlerExtensibilityTests.fs |
Tests APQ and normal request behavior. |
RELEASE_NOTES.md |
Documents the fix and breaking behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| let checkAnonymousFieldsOnly (ctx : HttpContext) = taskResult { | ||
| // Read ahead of the request itself, which cannot be bound without the query a persisted query request leaves out | ||
| let! envelope = ctx.TryBindJsonAsync<GQLRequestEnvelope>(GQLRequestContent.expectedJSON) |
Test Results 9 files 9 suites 14m 44s ⏱️ Results for commit e5c058e. ♻️ This comment has been updated with latest results. |
The tests take the error message, its code, the cache directive and the query string parameter from `PersistedQueries` and the media type from `MediaTypeNames` instead of spelling them out again, and assert with `Assert.Equal` and `Assert.Single` instead of `Assert.True` on a comparison. The error extensions are built with `kvpObj` and without an upcast. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
Problem
Apollo Client's persisted query link first sends only a hash of the query in
extensions.persistedQuery. It falls back to the full query once the server answersPersistedQueryNotSupported. The HTTP handler ignored the extension, so the client never got the error it expects:Changes
The answer. A request whose
extensions.persistedQueryis truthy, with or without a query, is answered as Apollo Server 5.5.1 answers with persisted queries disabled (requestPipeline.ts,internalErrorClasses.ts):Cache-Control: private, no-cache, must-revalidate;{"errors":[{"message":"PersistedQueryNotSupported","extensions":{"code":"PERSISTED_QUERY_NOT_SUPPORTED"}}]}.The body has no
documentId: Apollo Client 4 does not fall back from a result with any other top-level member thandata,errorsandextensions.Where it is checked: in a JSON body, in a multipart
operationsfield, and in theextensionsquery string parameter of a GET request. Truthiness follows JavaScript, as Apollo Server tests it, and the last of duplicate members counts.No copy of the extensions. An internal envelope reads them before
GQLRequestContent, so the public request type is unchanged. A converter evaluatespersistedQuerystraight from the JSON reader and skips every other value. A 20 MBextensionsvalue costs 416 KB per request, about the same as any unknown member; binding it as aJsonElementcost 20 898 KB.WebSocket.
graphql-transport-wssubscribepayloads are not checked: they must carry the query, they are never answered with introspection, andGraphQLWsLinksends the full query along with the hash.Breaking: a request with both a query and a persisted query hash is now rejected instead of executed, as Apollo Server does.
Tests
The tests are xUnit, in
RequestHandlerExtensibilityTests.fs:persistedQueryvalues, with duplicate and nested members;extensionsthat ask for nothing or are not JSON, still answered with the introspection result;An independent review checked the following:
dev, including bodies up to 20 MB, size limits and file uploads.The only differences from
devwere the intended ones.Verification:
🤖 Generated with Claude Code