Netflix/dgs-framework

bug: When trying to use Persisted queries I get exception

Ancient-Dragon opened this issue ยท 24 comments

Please read our contributor guide before
creating an issue.

Expected behavior

To be able to query for data while using persisted queries on the apollo client.

Actual behavior

Any requests with the extension (e.g. {"operationName":"getMarketForecasts","variables":{"effectiveDate":"2022-02-17"},"extensions":{"persistedQuery":{"version":1,"sha256Hash":"81edfa7234a38d8dee7b60971cae1c44d1b001b81376bdd66530552a64d5a0d8"}}}) for persisted queries and a hash give the error:
2022-03-04 14:20:57,627 ERROR [http-nio-8090-exec-6] org.apache.juli.logging.DirectJDKLog: Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Request processing failed; nested exception is java.lang.NullPointerException: null cannot be cast to non-null type kotlin.String] with root cause java.lang.NullPointerException: null cannot be cast to non-null type kotlin.String at com.netflix.graphql.dgs.mvc.DgsRestController$graphql$executionResult$1.invoke(DgsRestController.kt:161) at com.netflix.graphql.dgs.mvc.DgsRestController$graphql$executionResult$1.invoke(DgsRestController.kt:158) at com.netflix.graphql.dgs.internal.utils.TimeTracer.logTime(TimeTracer.kt:24) at com.netflix.graphql.dgs.mvc.DgsRestController.graphql(DgsRestController.kt:158) at jdk.internal.reflect.GeneratedMethodAccessor94.invoke(Unknown Source) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:190) at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:138) at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:105) at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:879) at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:793) at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87) at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1040) at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:943) at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1006) at org.springframework.web.servlet.FrameworkServlet.doPost(FrameworkServlet.java:909) at javax.servlet.http.HttpServlet.service(HttpServlet.java:660) at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:883) at javax.servlet.http.HttpServlet.service(HttpServlet.java:741) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:231) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:53) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.CorsFilter.doFilterInternal(CorsFilter.java:92) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.FormContentFilter.doFilterInternal(FormContentFilter.java:93) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at io.opentelemetry.javaagent.instrumentation.springwebmvc.HandlerMappingResourceNameFilter.doFilter(HandlerMappingResourceNameFilter.java:59) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.boot.actuate.metrics.web.servlet.WebMvcMetricsFilter.doFilterInternal(WebMvcMetricsFilter.java:93) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201) at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:202) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96) at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:541) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:92) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343) at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:373) at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65) at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:868) at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1590) at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) at java.base/java.lang.Thread.run(Thread.java:834)

Steps to reproduce

While using the package in the frontend:
"apollo-link-persisted-queries": "0.2.5", (https://www.npmjs.com/package/apollo-link-persisted-queries)
try and query a dgs graphql service and it should give a 500 response.
Note: A test case would be highly appreciated, but we understand that's not always possible

What DGS Framework version are you using

Hey, your question prompted me to double check that I had run a maven clean install (which it appears as though I hadn't, I had just rebuild using my IDE). I am using version 4.9.21, apologies. Now I don't get that error message anymore, however I do get the following error when making the request:

image

The response is:
{"errors":[{"message":"The query is null or empty.","locations":[],"extensions":{"errorType":"BAD_REQUEST"}}]}

Also here is my config for DGS:

dgs:
  graphql:
    path: /api2/graphql
    apq:
      enabled: true

And here is the code I am using to create my apollo client:

import { createPersistedQueryLink } from 'apollo-link-persisted-queries';
import { createHttpLink } from 'apollo-link-http';
import { ApolloClient } from 'apollo-client';
import { InMemoryCache } from 'apollo-cache-inmemory';

// Http endpoint
const httpEndpoint = process.env.VUE_APP_JAVA_GRAPHQL_HTTP || `${process.env.VUE_APP_BACKEND_API_URL}/graphql`;

  const httpLink = createPersistedQueryLink({ useGETForHashedQueries: false }).concat(createHttpLink({
    uri: httpEndpoint,
  }));

  const link = httpLink;

  const client = new ApolloClient({
    link,
    cache: new InMemoryCache({
      addTypename: false,
    }),
    ssrMode: false,
    connectToDevTools: true,
  });

I think you are missing the required sha256 function in createPersistedQueryLink:

https://www.apollographql.com/docs/react/api/link/persisted-queries/

If that does not work, can you provide a repurposable example?

According to the documentation here: https://www.npmjs.com/package/apollo-link-persisted-queries it suggests that there is a default implementation for generateHash. Is there a specific hashing algorithm that dgs uses that I need to use in my frontend?

You should try using the apollo client implementation instead

afbeelding

https://www.apollographql.com/docs/react/api/link/persisted-queries/

@Ancient-Dragon could you please follow whah @jord1e suggests and let us know.

PS @jord1e thanks for all your help! as always.

I don't think we can upgrade unfortunately. From what I can tell though the request seems to be pretty standard (as in we're not sending a query body because we're hashed it, which is what the error is suggesting), If I get some time over the next couple of days I will try to get a separate repo built to demonstrate our issue. If you already have a working example that would also be helpful.

If you have a repo, including the UI client, I could use to debug this behavior it will rely help.

Well, that was stupid (and somewhat time consuming)

Here is a complete (and working) example: https://github.com/jord1e/dgs-framework-apollo-automatic-persisted-query-support-example

You actually need to include the Caffeine cache library (or provide your own). I always add it to my projects by default, but you probably didn't have it installed.

Is it OK if I document this somewhere this week (and maybe add some WARN logging lines when dgs.graphql.apq.enabled=true and no PersistedQueryCache bean is present) @berngp

Source of the 'problem':

I just tested with the latest version of apollo and it wasn't working.

Ah just seen jord1e's post, I'll give it a try now as yeah we don't have Caffeine cache as a library.

I see everyone is awake from the Americas to Europe ๐Ÿ˜„

I think it would be good if there was some documentation on how to implement the required cache bean as well if the library is not providing one. (ignore this I got confused I misread assuming you were asking me to make a bean of the Persisted cache rather than the Cache class)

So after providing the following dependency:

<dependency>
    <groupId>com.github.ben-manes.caffeine</groupId>
    <artifactId>caffeine</artifactId>
    <version>3.0.5</version>
</dependency>

and making a bean of like:

import java.time.Duration;

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;

@Configuration
public class CachingConfiguration {

    @Bean
    Cache persistedQueryCache() {
        return Caffeine.newBuilder()
                .maximumSize(10_000)
                .expireAfterWrite(Duration.ofMinutes(5))
                .build();
    }

}

I still get the error message

Also sorry for the poor response times and getting examples, its not strictly something I'm working on at work so I have to do it in free time etc, and doing examples needs to be on personal PC

You do not need the bean (the dependency version is also not required because of spring version management)

Do you have dgs.graphql.apq.enabled=true

And are you on version 4.9.21 ?

Well, that was stupid (and somewhat time consuming)

Here is a complete (and working) example: https://github.com/jord1e/dgs-framework-apollo-automatic-persisted-query-support-example

You actually need to include the Caffeine cache library (or provide your own). I always add it to my projects by default, but you probably didn't have it installed.

Is it OK if I document this somewhere this week (and maybe add some WARN logging lines when dgs.graphql.apq.enabled=true and no PersistedQueryCache bean is present) @berngp

Source of the 'problem':

@jord1e thank you! Although it is ok to document I am now wondering why I decided to make the transitive to com.github.ben-manes.caffeine:caffeine as compileOnly. I think it would probably be better to express an implementation dependency and have it working by default. What do you think?

Well, that was stupid (and somewhat time consuming)
Here is a complete (and working) example: jord1e/dgs-framework-apollo-automatic-persisted-query-support-example
You actually need to include the Caffeine cache library (or provide your own). I always add it to my projects by default, but you probably didn't have it installed.
Is it OK if I document this somewhere this week (and maybe add some WARN logging lines when dgs.graphql.apq.enabled=true and no PersistedQueryCache bean is present) @berngp
Source of the 'problem':

@jord1e thank you! Although it is ok to document I am now wondering why I decided to make the transitive to com.github.ben-manes.caffeine:caffeine as compileOnly. I think it would probably be better to express an implementation dependency and have it working by default. What do you think?

I think not to be honest, it would require all users to pull in Caffeine, even when not using APQ. (although Caffeine is a great library ๐Ÿ‘€)

In my opinion a WARN message (or even a FATAL when no impl/bean+the config option is true) + docs under the advanced section would be better.

... Or a default (ConcurrentHash)Map<K, V> implementation (we should still INFO in this case)

I don't want to maintain a simple implementation that folks then use in prod by mistake. I like the idea of FATAL and have a Failure Analyzer that describes how to fix it. Note that the module is optional, so folks need to explicitly depend on it. This probably makes it a bit better if we decide to also now have an implementation dependency on Caffeine.

Note that the module is optional, so folks need to explicitly depend on it.

I presume you mean that dgs.graphql.apq.enabled=true is optional and it will thus be OK to FATAL in case of a missing impl (which the default Caffeine cache is)?

I agree with this, it is probably the best option.

I don't want to maintain a simple implementation that folks then use in prod by mistake.

Good call

You are right in the points above! So, I think I have it clear (sorry a bit slow)

  1. If dgs.graphql.apq.enabled=true and there is no implementation of PersistedQueryCache, exit explaining the user what to do. Probably throw a specific error and have a failure analyzer capture it.
  2. Optionally offer a starter module for APQ that has Caffeine added as an implementation dependency. You should be able to just import the starter, along with either graphql-dgs-spring-boot-starter or graphql-dgs-webflux-starter, and use APQ out of the box.

PS/ Full disclosure, for some reason I thought I had expressed the APQ Feature as an optional module and therefore I thought it was not a bad idea to consider an implementation dependency on Caffeine. This is not the case and therefore the suggestion of the starter above.

I was thinking of this:

Untitled Diagram drawio(1)

This would:

  1. Properly inform and guide library consumers
  2. Have a easily-accesible implementation in the form of a Cafffeine module (the config option for Caffeine specifically will be moved there)
  3. Give users the option to provide their own impl (a simple i.e. InMemoryPersistedQueryCache by GraphQL Java) without using caffeine, and without importing yet another module.

WDYT?

Sorry for hijacking your issue @Ancient-Dragon ๐Ÿ˜…

Ok, so narrowing it down a little bit. I was able to setup a demo which worked fine. I've copied all my dependencies from our project into this demo application and now its failing so I must have a conflict somewhere. I will post back when I find out what the conflict is.

This was the dependency that has caused this whole issue:

        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-actuator</artifactId>
            <version>2.3.0.RELEASE</version>
        </dependency>

Once I removed this (and added caffeine dependency) it works as described.

Closing this particular issue since you were able to work through it. @jord1e has a separate PR out to add the starter module with Caffeine dependency.