Added ability to cache the whole graphql response #247
Conversation
donbeave
changed the title
|
Can the cache also take into consideration say, a HTTP header? I'm thinking for example, two clients could see different field values based on some auth data, e.g. inside a JWT ? |
|
@pstarritt-evooq yeah, I have added |
There was a problem hiding this comment.
First off, thanks for the contribution! Sorry it took me awhile to get around to reviewing it. Pretty busy and it's a bigger PR to review.
I've added some comments here mostly regarding applying some patterns to try to make the code a bit cleaner in my opinion. In addition to this could you add a sample to the samples project and/or add a section to the documentation to showcase usage?
| @@ -34,6 +41,16 @@ public void write(HttpServletRequest request, HttpServletResponse response) thro | |||
|
|
|||
| String responseContent = responseBuilder.toString(); | |||
| byte[] contentBytes = responseContent.getBytes(StandardCharsets.UTF_8); | |||
|
|
|||
| if (responseCache != null && responseCache.isCacheable(request, invocationInput)) { | |||
There was a problem hiding this comment.
This block or something nearly identical is repeated in every QueryResponseWriter implementation. Instead it would be better if you'd apply the decorator pattern. If somebody enabled caching then have the QueryResponseWriter factory method create some CachingResponseWriter (which should implement QueryResponseWriter too obviously) wrapping the other implementations. That way you have one class with this logic and you don't pollute the other classes with caching logic. Ideally they're not aware of that at all.
| try { | ||
| responseCache.put(request, invocationInput, CachedResponse.ofContent(contentBytes)); | ||
| } catch (Throwable t) { | ||
| log.warn(t.getMessage(), t); |
There was a problem hiding this comment.
I'd loose this log statement and just add the stacktrace to the log statement on the next line.
| QueryResponseWriter queryResponseWriter = QueryResponseWriter.createWriter(queryResult, configuration.getObjectMapper(), | ||
| configuration.getSubscriptionTimeout()); | ||
| queryResponseWriter.write(request, response); | ||
| QueryResponseWriter queryResponseWriter = QueryResponseWriter.createWriter( |
There was a problem hiding this comment.
I'd add the ResponseCache in this factory method instead of adding it to the method signature of the writer.
| @@ -49,11 +52,31 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr | |||
| private void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request, | |||
| HttpServletResponse response) { | |||
| try { | |||
| if (configuration.getResponseCache() != null) { | |||
There was a problem hiding this comment.
Bit too much logic here imo. Would be better if you'd move this part into a separate class and just call its method here. If you let it return a boolean then you can get rid of that return; at line 67 too and it'll be a bit easier to follow.
|
@oliemansm Thank you! I'll add fixes based on your comments and also add an example project and improve documentation. I Will back here again after a few days. |
|
@oliemansm please check the changes and let me know if I need to fix or change something else. |
graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java
Outdated
Show resolved
Hide resolved
graphql-java-servlet/src/main/java/graphql/kickstart/servlet/QueryResponseWriter.java
Show resolved
Hide resolved
|
@oliemansm how about these changes, is it looks close to your expectations? |
|
@oliemansm sorry for pushing you, do you have time to take a look at this PR? I want to be sure this approach is ok and I can start to do tests, documentation, and an example project. |
|
@donbeave Apologies for the delay, very busy period. PR looking good overall. If you could resolve the merge conflicts I can merge it. |
…aphql-java-servlet into graphql-java-kickstart-master
|
@oliemansm done, please check. |
|
@donbeave Great, thanks! Could you add a page in the documentation when you have time to explain usage? We're notoriously bad in documentation, so I'd like to try to keep up from now on. |
|
I would very much like to explore this feature. Is the documentation added? I can't find it on the main page. |
|
@ubarua123 You can check this video tutorial: https://www.youtube.com/watch?v=HpagmudtUBM, it covers all aspects of this feature. |
Hello!
First of all, thank you for creating this library. We are using it heavily in @scentbird. I found out this library has a lack of features, which so important for our applications. Sometimes we want to cache the whole GraphQL response, not just the part of it. Based on the input information, we have a service that determines what will be retrieved from the cache and whatnot. This PR contains a basic interface that provides the ability to retrieve the response text from content and cache response text to the cache if one implementation is provided. This interface is not locked to any specific cache technology and can also use multiple cache managers inside, it depends on implementation.
I hope you guys like this idea.
P.S. Later I'll share also some cache implementations in a separate project.