Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User configured PreParsedDocumentProvider should be used for regular queries when APQ feature is enabled. #2128

Merged
merged 8 commits into from
Feb 24, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import org.springframework.boot.context.properties.EnableConfigurationProperties
import org.springframework.cache.CacheManager
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
import org.springframework.core.Ordered
import org.springframework.core.annotation.Order
import java.util.Optional

/**
Expand All @@ -38,6 +40,7 @@ open class DgsGraphQLMicrometerAutoConfiguration {
}

@Bean
@Order(Ordered.LOWEST_PRECEDENCE)
@ConditionalOnProperty(
prefix = "$AUTO_CONF_PREFIX.instrumentation",
name = ["enabled"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explained in response to earlier comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.and("gql.errorCode", "BAD_REQUEST")
.and("gql.path", "[hello]")
.and("outcome", "failure"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,8 @@ management:
exposure:
include:
- metrics
debug: true
debug: true
dgs:
graphql:
apq:
enabled: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright 2025 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.graphql.dgs.apq

import graphql.ExecutionInput
import graphql.execution.preparsed.PreparsedDocumentEntry
import graphql.execution.preparsed.PreparsedDocumentProvider
import graphql.execution.preparsed.persisted.ApolloPersistedQuerySupport
import graphql.execution.preparsed.persisted.PersistedQueryCache
import java.util.*
import java.util.concurrent.CompletableFuture
import java.util.function.Function

class DgsAPQPreParsedDocumentProviderWrapper(
persistedQueryCache: PersistedQueryCache,
private val preparsedDocumentProvider: Optional<PreparsedDocumentProvider>,
) : ApolloPersistedQuerySupport(persistedQueryCache) {
override fun getDocumentAsync(
executionInput: ExecutionInput,
parseAndValidateFunction: Function<ExecutionInput, PreparsedDocumentEntry>,
): CompletableFuture<PreparsedDocumentEntry> {
val queryId = getPersistedQueryId(executionInput)
if (queryId.isPresent) {
return super.getDocumentAsync(executionInput, parseAndValidateFunction)
}

if (preparsedDocumentProvider.isPresent) {
return preparsedDocumentProvider.get().getDocumentAsync(executionInput, parseAndValidateFunction)
}

return CompletableFuture.completedFuture(parseAndValidateFunction.apply(executionInput))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,28 @@ package com.netflix.graphql.dgs.apq
import com.github.benmanes.caffeine.cache.Cache
import com.github.benmanes.caffeine.cache.Caffeine
import com.github.benmanes.caffeine.cache.CaffeineSpec
import com.netflix.graphql.dgs.internal.QueryValueCustomizer
import com.netflix.graphql.dgs.springgraphql.autoconfig.DgsSpringGraphQLAutoConfiguration
import graphql.execution.preparsed.PreparsedDocumentEntry
import graphql.execution.preparsed.persisted.ApolloPersistedQuerySupport
import graphql.execution.preparsed.PreparsedDocumentProvider
import graphql.execution.preparsed.persisted.PersistedQueryCache
import io.micrometer.core.instrument.MeterRegistry
import io.micrometer.core.instrument.binder.cache.CaffeineCacheMetrics
import org.springframework.beans.factory.annotation.Qualifier
import org.springframework.boot.autoconfigure.AutoConfiguration
import org.springframework.boot.autoconfigure.AutoConfigureAfter
import org.springframework.boot.autoconfigure.condition.ConditionalOnBean
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
import org.springframework.boot.autoconfigure.graphql.GraphQlSourceBuilderCustomizer
import org.springframework.boot.context.properties.EnableConfigurationProperties
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
import java.time.Duration
import java.util.*

@AutoConfiguration
@AutoConfigureAfter(DgsSpringGraphQLAutoConfiguration::class)
@ConditionalOnProperty(
prefix = DgsAPQSupportProperties.PREFIX,
name = ["enabled"],
Expand All @@ -46,18 +50,17 @@ import java.time.Duration
@EnableConfigurationProperties(DgsAPQSupportProperties::class)
open class DgsAPQSupportAutoConfiguration {
@Bean
@ConditionalOnBean(PersistedQueryCache::class)
open fun apolloPersistedQuerySupport(persistedQueryCache: PersistedQueryCache): ApolloPersistedQuerySupport =
ApolloPersistedQuerySupport(persistedQueryCache)

@Bean
@ConditionalOnBean(ApolloPersistedQuerySupport::class)
open fun apolloAPQQueryValueCustomizer(): QueryValueCustomizer =
QueryValueCustomizer { query ->
if (query.isNullOrBlank()) {
ApolloPersistedQuerySupport.PERSISTED_QUERY_MARKER
} else {
query
open fun apqSourceBuilderCustomizer(
preparsedDocumentProvider: Optional<PreparsedDocumentProvider>,
persistedQueryCache: Optional<PersistedQueryCache>,
): GraphQlSourceBuilderCustomizer =
GraphQlSourceBuilderCustomizer { builder ->
builder.configureGraphQl { graphQlBuilder ->
// For non-APQ queries, the user specified PreparsedDocumentProvider should be used, so we configure the DgsAPQPreparsedDocumentProvider to
// wrap the user specified one and delegate appropriately since we can only have one PreParsedDocumentProvider bean
val apqPreParsedDocumentProvider =
DgsAPQPreParsedDocumentProviderWrapper(persistedQueryCache.get(), preparsedDocumentProvider)
graphQlBuilder.preparsedDocumentProvider(apqPreParsedDocumentProvider)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ import org.springframework.context.ApplicationContext
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
import org.springframework.core.DefaultParameterNameDiscoverer
import org.springframework.core.Ordered
import org.springframework.core.PriorityOrdered
import org.springframework.core.ReactiveAdapterRegistry
import org.springframework.core.annotation.Order
Expand Down Expand Up @@ -171,7 +172,9 @@ open class DgsSpringGraphQLAutoConfiguration(
graphQLContextContributors: ObjectProvider<GraphQLContextContributor>,
): Instrumentation = GraphQLContextContributorInstrumentation(graphQLContextContributors.orderedStream().toList())

// This instrumentation needs to run before MetricsInstrumentation
@Bean
@Order(Ordered.LOWEST_PRECEDENCE - 1)
@ConditionalOnProperty(
prefix = "${AUTO_CONF_PREFIX}.errors.classification",
name = ["enabled"],
Expand Down Expand Up @@ -491,12 +494,14 @@ open class DgsSpringGraphQLAutoConfiguration(
@Qualifier("query") providedQueryExecutionStrategy: Optional<ExecutionStrategy>,
@Qualifier("mutation") providedMutationExecutionStrategy: Optional<ExecutionStrategy>,
dataFetcherExceptionHandler: DataFetcherExceptionHandler,
environment: Environment,
): GraphQlSourceBuilderCustomizer =
GraphQlSourceBuilderCustomizer { builder ->
builder.configureGraphQl { graphQlBuilder ->
if (preparsedDocumentProvider.isPresent) {
graphQlBuilder
.preparsedDocumentProvider(preparsedDocumentProvider.get())
val apqEnabled = environment.getProperty("dgs.graphql.apq.enabled", Boolean::class.java, false)
// If apq is enabled, we will not use this preparsedDocumentProvider and use DgsAPQPreparsedDocumentProviderWrapper instead
if (preparsedDocumentProvider.isPresent && !apqEnabled) {
graphQlBuilder.preparsedDocumentProvider(preparsedDocumentProvider.get())
}

if (providedQueryExecutionStrategy.isPresent) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Copyright 2025 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.graphql.dgs.springgraphql.apq

import com.netflix.graphql.dgs.apq.DgsAPQPreParsedDocumentProviderWrapper
import graphql.ExecutionInput
import graphql.execution.preparsed.PreparsedDocumentEntry
import graphql.execution.preparsed.PreparsedDocumentProvider
import graphql.execution.preparsed.persisted.PersistedQueryCache
import graphql.execution.preparsed.persisted.PersistedQuerySupport.PERSISTED_QUERY_MARKER
import graphql.language.Document
import io.mockk.every
import io.mockk.impl.annotations.MockK
import io.mockk.junit5.MockKExtension
import io.mockk.mockk
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.extension.ExtendWith
import org.springframework.beans.factory.annotation.Autowired
import java.util.*
import java.util.concurrent.CompletableFuture

@ExtendWith(MockKExtension::class)
class DgsAPQPreParsedDocumentProviderWrapperTest {
@Autowired
private lateinit var dgsAPQPreParsedDocumentProvider: DgsAPQPreParsedDocumentProviderWrapper

@MockK
lateinit var preparsedDocumentProvider: PreparsedDocumentProvider

@MockK
lateinit var persistedQueryCache: PersistedQueryCache

@BeforeEach
fun setUp() {
dgsAPQPreParsedDocumentProvider =
DgsAPQPreParsedDocumentProviderWrapper(persistedQueryCache, Optional.of(preparsedDocumentProvider))
}

@Test
fun `APQ only queries with just the hash use the persisted query cache`() {
var count = 0
val document = mockk<Document>()
val computeFunction = { _: ExecutionInput ->
count++
PreparsedDocumentEntry(document)
}
val extensions: MutableMap<String, Any> = HashMap()
extensions["persistedQuery"] =
mapOf("version" to "1", "sha256Hash" to "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38")
val executionInput =
ExecutionInput
.Builder()
.query(PERSISTED_QUERY_MARKER)
.extensions(extensions)
.build()

every {
persistedQueryCache.getPersistedQueryDocumentAsync(any(), any(), any())
}.returns(CompletableFuture.completedFuture(computeFunction(executionInput)))

dgsAPQPreParsedDocumentProvider.getDocumentAsync(executionInput, computeFunction)
assertThat(count).isEqualTo(1)
}

@Test
fun `APQ queries with query and hash use the persisted query cache`() {
var count = 0
val document = mockk<Document>()
val computeFunction = { _: ExecutionInput ->
count++
PreparsedDocumentEntry(document)
}
val extensions: MutableMap<String, Any> = HashMap()
extensions["persistedQuery"] =
mapOf("version" to "1", "sha256Hash" to "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38")
val executionInput =
ExecutionInput
.Builder()
.query("{__typename}")
.extensions(extensions)
.build()

every {
persistedQueryCache.getPersistedQueryDocumentAsync(any(), any(), any())
}.returns(CompletableFuture.completedFuture(computeFunction(executionInput)))

dgsAPQPreParsedDocumentProvider.getDocumentAsync(executionInput, computeFunction)
assertThat(count).isEqualTo(1)
}

@Test
fun `Plain queries (non-APQ) use the user specified preparsed document provider`() {
var count = 0
val document = mockk<Document>()
val computeFunction = { _: ExecutionInput ->
count++
PreparsedDocumentEntry(document)
}
val extensions: MutableMap<String, Any> = HashMap()
extensions["persistedQuery"] =
mapOf("version" to "1", "sha256Hash" to "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38")
val executionInput = ExecutionInput.Builder().query("{__typename}").build()

every {
preparsedDocumentProvider.getDocumentAsync(executionInput, any())
}.returns(CompletableFuture.completedFuture(computeFunction(executionInput)))

dgsAPQPreParsedDocumentProvider.getDocumentAsync(executionInput, computeFunction)
assertThat(count).isEqualTo(1)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright 2024 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.graphql.dgs.springgraphql.autoconfig

import com.netflix.graphql.dgs.apq.DgsAPQSupportAutoConfiguration
import graphql.execution.preparsed.persisted.PersistedQueryCache
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import org.springframework.boot.autoconfigure.AutoConfigurations
import org.springframework.boot.autoconfigure.graphql.GraphQlAutoConfiguration
import org.springframework.boot.test.context.runner.ApplicationContextRunner

class DgsAPQAutoConfigurationTest {
private val autoConfigurations =
AutoConfigurations.of(
DgsSpringGraphQLAutoConfiguration::class.java,
DgsAPQSupportAutoConfiguration::class.java,
GraphQlAutoConfiguration::class.java,
)

@Test
fun shouldContributeAPQBeans() {
val contextRunner =
ApplicationContextRunner()
.withConfiguration(autoConfigurations)
.withPropertyValues("dgs.graphql.apq.enabled=true")

contextRunner.run { context ->
assertThat(context)
.hasBean("apqSourceBuilderCustomizer")
.hasBean("sourceBuilderCustomizer")
.hasSingleBean(PersistedQueryCache::class.java)
}
}

@Test
fun shouldNotContributeAPQBeans() {
val contextRunner =
ApplicationContextRunner()
.withConfiguration(autoConfigurations)

contextRunner.run { context ->
assertThat(context)
.doesNotHaveBean("apqSourceBuilderCustomizer")
.hasBean("sourceBuilderCustomizer")
.doesNotHaveBean(PersistedQueryCache::class.java)
}
}
}
Loading