User configured PreParsedDocumentProvider should be used for regular queries when APQ feature is enabled. #2128
Conversation
srinivasankavitha
requested review from
paulbakker,
kilink,
iuliiasobolevska,
kzwang,
asibross,
iparadiso and
jjacobs44
as code owners
srinivasankavitha
force-pushed
the
kavitha/apq-enhancements
branch
from
cafde75 to
022d23b
Compare
srinivasankavitha
changed the title
| @@ -40,7 +40,6 @@ open class DgsGraphQLMicrometerAutoConfiguration { | |||
| } | |||
|
|
|||
| @Bean | |||
| @Order(PriorityOrdered.LOWEST_PRECEDENCE) | |||
There was a problem hiding this comment.
What does this fix? The @Order is just bean creation order, so I want to make sure what the intent is.
There was a problem hiding this comment.
I added this is a previous commit, which I now removed because it doesn't do anything. The micrometer test started failing because the order in which the instrumentations run seems to have changed in my branch. If the DgsGraphQlJavaErrorInstrumentation runs before the MetricsInstrumentation, the tag value is set properly (which is in this branch). However, if it's the other way around, the metrics add the tag for gql.error before the DgsGraphQlJavaErrorInstrumentation runs and that's why it passes in master branch.
Basically need a way to ensure the MetricsInstrumentation runs last, but not sure what the best way to do that is, so looking into it.
There was a problem hiding this comment.
Fixed by specifying order on both instrumentation classes. That ensures the correct relative ordering.
| @@ -387,7 +387,7 @@ class MicrometerServletSmokeTest { | |||
| .and("gql.operation.name", "anonymous") | |||
| .and("gql.query.complexity", "none") | |||
| .and("gql.query.sig.hash", "none") | |||
| .and("gql.errorDetail", "none") | |||
| .and("gql.errorDetail", "INVALID_ARGUMENT") | |||
There was a problem hiding this comment.
explained in response to earlier comment
There was a problem hiding this comment.
The micrometer test started failing because the order in which the instrumentations run seems to have changed in my branch. If the DgsGraphQlJavaErrorInstrumentation runs before the MetricsInstrumentation, the tag value is set properly (which is in this branch). However, if it's the other way around, the metrics add the tag for gql.error before the DgsGraphQlJavaErrorInstrumentation runs and that's why it passes in master branch.
The correct order should be DgsGraphQlJavaErrorInstrumentation first and then DgsMetricsInstrumentation so the metrics get the correct tag value as fixed in this PR.
Basically need a way to ensure the MetricsInstrumentation runs last, but not sure what the best way to do that is, so looking into it.
| import java.util.concurrent.CompletableFuture | ||
| import java.util.function.Function | ||
|
|
||
| class DgsAPQPreParsedDocumentProvider( |
There was a problem hiding this comment.
Could be a more descriptive name, e.g. it should communicate that this is really a wrapper
| graphQlBuilder | ||
| .preparsedDocumentProvider(preparsedDocumentProvider.get()) | ||
| val apqEnabled = environment.getProperty("dgs.graphql.apq.enabled", Boolean::class.java, false) | ||
| // if apq is enabled use the APQPreparsedDocumentProvider instead |
There was a problem hiding this comment.
Comment and if statement don't seem to match? The if statement checks for apq NOT being enabled?
There was a problem hiding this comment.
Will clarify the comment - basically we don't want the code in the block to be used if apq is enabled.
| }.returns(CompletableFuture.completedFuture(computeFunction(executionInput))) | ||
|
|
||
| dgsAPQPreParsedDocumentProvider.getDocumentAsync(executionInput, computeFunction) | ||
| assertThat(count == 1) |
There was a problem hiding this comment.
assertThat(count).isEqualTo(1) for better error reporting
Pull Request type
Changes in this PR
Support for Automated Persisted Queries are implemented in graphql-java using a PreParsedDocumentProvider : https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/execution/preparsed/persisted/PersistedQuerySupport.java
The current setup allows us to configure only one PreParsedDocumentProvider. Therefore, if we enable APQ, we can only use a PreParsedDocumentProvider for APQ queries. Non-APQ or regular queries without a query hash will not be able to take advantage of a PreParsedDocumentProvider. Typically, users are able set up their own PreParsedDocumentProvider as a bean to speed up the request processing, especially critical for high scale applications.
This PR provides a wrapper to delegate to the appropriate PreParsedDocumentProvider based on whether the incoming query has an APQ hash or not. Also added some tests for the same.
Long term, we can work with spring or graphql-java to make this configurable.
Testing this feature:
dgs.graphql.apq.enabledfor the spring-graphql exampl app and start itExample of regular query: