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

APQ seems to be partially implemented in Spring Server #2065

Closed
malaquf opened this issue Dec 19, 2024 · 4 comments
Closed

APQ seems to be partially implemented in Spring Server #2065

malaquf opened this issue Dec 19, 2024 · 4 comments
Labels
type: bug Something isn't working

Comments

@malaquf
Copy link
Contributor

malaquf commented Dec 19, 2024

Library Version
8.2.1

Describe the bug
We tried to enable APQ in spring server, which according to the documentation requires only setting automaticPersistedQueries.enabled: true, but unfortunately the APQ flow doesn't work as expected because the initial GET call fail in GraphQLServer with bad request without a body (I'd expect a PersistedQueryNotFound error be available in the payload, for eg). The AutomaticPersistedQueriesProvider is not even executed.

The request fails because the parseRequest returns null (no query parameter available):

open suspend fun execute(
        request: Request
    ): GraphQLServerResponse? =
        coroutineScope {
            requestParser.parseRequest(request)?.let { graphQLRequest ->
                val graphQLContext = contextFactory.generateContext(request)

                val customCoroutineContext = (graphQLContext.get<CoroutineContext>() ?: EmptyCoroutineContext)
                val graphQLExecutionScope = CoroutineScope(
                    coroutineContext + customCoroutineContext + SupervisorJob()
                )

                val graphQLContextWithCoroutineScope = graphQLContext + mapOf(
                    CoroutineScope::class to graphQLExecutionScope
                )

                requestHandler.executeRequest(graphQLRequest, graphQLContextWithCoroutineScope)
            }
        }

and therefore the router returns bad request here (GraphQLRoutesConfiguration):

@Bean
    fun graphQLRoutes() = coRouter {
        val isEndpointRequest = POST(config.endpoint) or GET(config.endpoint)
        val isNotWebSocketRequest = headers { isWebSocketHeaders(it) }.not()

        (isEndpointRequest and isNotWebSocketRequest).invoke { serverRequest ->
            try {
                val graphQLResponse = graphQLServer.execute(serverRequest)
                val acceptMediaType = serverRequest
                    .headers()
                    .accept()
                    .find { it != MediaType.ALL && it.includes(MediaType.APPLICATION_GRAPHQL_RESPONSE) }
                    ?.let { MediaType.APPLICATION_GRAPHQL_RESPONSE }
                    ?: MediaType.APPLICATION_JSON
                if (graphQLResponse != null) {
                    ok().contentType(acceptMediaType).bodyValueAndAwait(graphQLResponse)
                } else {
                    badRequest().buildAndAwait()
                }
            } catch (e: Exception) {
                badRequest().buildAndAwait()
            }
        }
    }

To Reproduce

We first tried it with Apollo Client and our own requests and saw the router not being able to resolve the request with 400s, and then we tried to reproduce it with simple examples from Apollo as described below:

1st GET request without query returns 400:

curl --get http://localhost:4000/graphql \
  --header 'content-type: application/json' \
  --data-urlencode 'extensions={"persistedQuery":{"version":1,"sha256Hash":"ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}'

2nd GET request with query returns 200:

  curl --get http://localhost:4000/graphql \
  --header 'content-type: application/json' \
  --data-urlencode 'query={__typename}' \
  --data-urlencode 'extensions={"persistedQuery":{"version":1,"sha256Hash":"ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}'

Send another GET request without the query and the cache is still not checked and we still receive 400:

curl --get http://localhost:4000/graphql \
  --header 'content-type: application/json' \
  --data-urlencode 'extensions={"persistedQuery":{"version":1,"sha256Hash":"ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}'

Expected behavior
The first call should not fail with bad request, and it should be able to resolve from cache after the result is properly cached.

Also, from the client implementation that's how it's supposed work (first request without a "query" parameter):

val apqRawResultWithoutQuery: String = when (automaticPersistedQueriesSettings.httpMethod) {

@malaquf malaquf added the type: bug Something isn't working label Dec 19, 2024
@malaquf
Copy link
Contributor Author

malaquf commented Dec 20, 2024

For info, I could make it work by overriding the default SpringGraphQLRequestParser as follows:

@Component
class CustomSpringGraphQLRequestParser(
	private val objectMapper: ObjectMapper
) : SpringGraphQLRequestParser(objectMapper) {

	private val mapTypeReference: MapType =
		TypeFactory.defaultInstance().constructMapType(HashMap::class.java, String::class.java, Any::class.java)

	override suspend fun parseRequest(request: ServerRequest): GraphQLServerRequest? {
		if (request.method() == HttpMethod.GET &&
			request.queryParam("extensions").isPresent &&
			request.queryParam("extensions").get().contains("persistedQuery")
		) {
			return getRequestFromGet(request)
		}
		return super.parseRequest(request)
	}

	private fun getRequestFromGet(serverRequest: ServerRequest): GraphQLServerRequest {
		val query = serverRequest.queryParam("query").orElse("")
		val operationName: String? = serverRequest.queryParam("operationName").orElseGet { null }
		val variables: String? = serverRequest.queryParam("variables").orElseGet { null }
		val graphQLVariables: Map<String, Any>? = variables?.let {
			objectMapper.readValue(it, mapTypeReference)
		}
		val extensions: Map<String, Any>? = serverRequest.queryParam("extensions").takeIf { it.isPresent }?.get()?.let {
			objectMapper.readValue(it, mapTypeReference)
		}

		return GraphQLRequest(
			query = query,
			operationName = operationName,
			variables = graphQLVariables,
			extensions = extensions
		)
	}

}

Notice the 'extensions' field is also not mapped in the standard class, which causes further problems with invalid query parsing afterwards.

I checked the Ktor server project, and it seems to have the same issue.

@samuelAndalon
Copy link
Contributor

the issue is with the GraphQLRequestParser specifically for GET requests, it was never implemented. if you can use POST, ill try to make some time to work on it, but it you prefer, contributions are always welcomed.

@malaquf
Copy link
Contributor Author

malaquf commented Dec 24, 2024

Great! I'll try submitting a PR when I get it done. Thanks for the fast reply.
Have a great holiday season!

@malaquf
Copy link
Contributor Author

malaquf commented Dec 30, 2024

Hello @samuelAndalon. For info, I submitted a PR here.
Thanks for looking into it!

samuelAndalon pushed a commit that referenced this issue Jan 5, 2025
… hash of query string (#2067)

### 📝 Description
According to the GraphQL APQ flow description, GET requests containing
only SHA-256 hash of the query should be checked in cache and respond
with PERSISTED_QUERY_NOT_FOUND error if request is not cached.
Both Ktor and Spring server implementations didn't handle this first
query without a query param.
I tried to implement the change without breaking existing behaviours, as
a query param is expected to take precedence over post body, for
example, as in one of the tests in RouteConfigurationIT.


### 🔗 Related Issues
#2065
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Development

No branches or pull requests

2 participants