Support null query when running APQ request #4049
Conversation
…g APQ request Support null query when running APQ request Full issue described in graphql-java#4008 ---- #### AI description (iteration 1) #### PR Classification Bug fix: Enhance ExecutionInput to properly handle null queries in automatic persisted query (APQ) requests. #### PR Summary This pull request modifies the ExecutionInput logic to return a persisted query marker when a null or empty query is provided along with a persistedQuery extension, ensuring graceful handling of APQ requests. - `src/main/java/graphql/ExecutionInput.java`: Introduces a new static method (`assertQuery`) to check for null/empty queries and returns a persisted query marker if appropriate; also removes the strict non-null assertion in the builder. - `src/test/groovy/graphql/ExecutionInputTest.groovy`: Adds a test case validating that a null query with the persistedQuery extension returns the expected persisted query marker. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #355548 # Conflicts: # src/test/groovy/graphql/ExecutionInputTest.groovy
|
Hello wanted to say, thanks so much for yet another PR! Thanks for all your time and effort |
| @@ -48,6 +49,14 @@ private ExecutionInput(Builder builder) { | |||
| this.cancelled = builder.cancelled; | |||
| } | |||
|
|
|||
| private static String assertQuery(Builder builder) { | |||
| if ((builder.query == null || builder.query.isEmpty()) && builder.extensions.containsKey("persistedQuery")) { | |||
There was a problem hiding this comment.
We need a well known constant on ExecutionInput for this string value. With Javadoc on it on how it needs to be set to allow PQs say
There was a problem hiding this comment.
OR we reuse- graphql.execution.preparsed.persisted.PersistedQuerySupport#PERSISTED_QUERY_MARKER as the well know value in the builder exentions
| def "uses persisted query marker when query is empty and extensions contains persistedQuery"() { | ||
| when: | ||
| def executionInput = ExecutionInput.newExecutionInput() | ||
| .extensions([persistedQuery: "any"]) |
There was a problem hiding this comment.
Maybe rather than a "map entry" in extentions. Could we do this
ExcutionInput.newExcutionINput().persistentQuery().build()
and under the covers this sets the query to graphql.execution.preparsed.persisted.PersistedQuerySupport#PERSISTED_QUERY_MARKER say
No need for a special extensions entry say
There was a problem hiding this comment.
So the question then is
should we provide a helper builder method (that sets the query for you to the magic value) or use a "map entry" in extentions.
I feel like the former is better but would love to why you chose "extensions" - was there a client reason ?
There was a problem hiding this comment.
This fix was originally from a colleague to unblock our production usage. The idea behind making it implicit in ExecutionInput is that that it would ensure that framework extensions like GraphQL kickstarter, DGS, etc don't need to handle it explicitely as most persisted query implementations today are passed via extensions.
Looking into it further it seems that GraphQL-Java abstracts Persisted Queries which makes sense given there are a few implementations in practice (Apollo APQ, etc). This makes this change not ideal as it is tailored specifially for Apollo APQ implementation.
Based on that I think we some how need to incorporate the check to leverage the PersistedQuerySupport implementation, for example: isPersistedQuery(ExecutionInput input). Ideally instead of just providing a PreparsedDocumentProvider, consumers would explictely pass in the PersistedQuerySupport implementation in addition to the PreparsedDocumentProvider.
If that was implemented as of today, then we would need to remove the assertions in the ExecutionInput that query is not null, and defer that check to later if persisted queries are not enabled assert(query != null).
| def "uses persisted query marker when query is empty and extensions contains persistedQuery"() { | ||
| when: | ||
| def executionInput = ExecutionInput.newExecutionInput() | ||
| .extensions([persistedQuery: "any"]) |
There was a problem hiding this comment.
So the question then is
should we provide a helper builder method (that sets the query for you to the magic value) or use a "map entry" in extentions.
I feel like the former is better but would love to why you chose "extensions" - was there a client reason ?
Support null query when running APQ request. This is upstream merge of our commit made by @joshjcarrier
#4008
This pull request modifies the ExecutionInput logic to return a persisted query marker when a null or empty query is provided along with a persistedQuery extension, ensuring graceful handling of APQ requests.
src/main/java/graphql/ExecutionInput.java: Introduces a new static method (assertQuery) to check for null/empty queries and returns a persisted query marker if appropriate; also removes the strict non-null assertion in the builder.src/test/groovy/graphql/ExecutionInputTest.groovy: Adds a test case validating that a null query with the persistedQuery extension returns the expected persisted query marker.