Skip to content

Commit 4b4edfe

Browse files
donbeavedonbeave
committed
Code review changes.
1 parent 6c95ef3 commit 4b4edfe

12 files changed

+303
-129
lines changed

‎graphql-java-servlet/src/main/java/graphql/kickstart/servlet/BatchedQueryResponseWriter.java‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,6 @@
22

33
import graphql.ExecutionResult;
44
import graphql.kickstart.execution.GraphQLObjectMapper;
5-
import graphql.kickstart.execution.input.GraphQLInvocationInput;
6-
import graphql.kickstart.servlet.cache.CachedResponse;
7-
import graphql.kickstart.servlet.cache.GraphQLResponseCache;
85
import lombok.RequiredArgsConstructor;
96
import lombok.extern.slf4j.Slf4j;
107

@@ -21,10 +18,9 @@ class BatchedQueryResponseWriter implements QueryResponseWriter {
2118

2219
private final List<ExecutionResult> results;
2320
private final GraphQLObjectMapper graphQLObjectMapper;
24-
private final GraphQLInvocationInput invocationInput;
2521

2622
@Override
27-
public void write(HttpServletRequest request, HttpServletResponse response, GraphQLResponseCache responseCache) throws IOException {
23+
public void write(HttpServletRequest request, HttpServletResponse response) throws IOException {
2824
response.setContentType(HttpRequestHandler.APPLICATION_JSON_UTF8);
2925
response.setStatus(HttpRequestHandler.STATUS_OK);
3026

@@ -42,15 +38,6 @@ public void write(HttpServletRequest request, HttpServletResponse response, Grap
4238
String responseContent = responseBuilder.toString();
4339
byte[] contentBytes = responseContent.getBytes(StandardCharsets.UTF_8);
4440

45-
if (responseCache != null && responseCache.isCacheable(request, invocationInput)) {
46-
try {
47-
responseCache.put(request, invocationInput, CachedResponse.ofContent(contentBytes));
48-
} catch (Throwable t) {
49-
log.warn(t.getMessage(), t);
50-
log.warn("Ignore read from cache, unexpected error happened");
51-
}
52-
}
53-
5441
response.setContentLength(contentBytes.length);
5542
response.getOutputStream().write(contentBytes);
5643
}

‎graphql-java-servlet/src/main/java/graphql/kickstart/servlet/ErrorQueryResponseWriter.java‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,31 +3,16 @@
33
import java.io.IOException;
44
import javax.servlet.http.HttpServletRequest;
55
import javax.servlet.http.HttpServletResponse;
6-
7-
import graphql.kickstart.execution.input.GraphQLInvocationInput;
8-
import graphql.kickstart.servlet.cache.CachedResponse;
9-
import graphql.kickstart.servlet.cache.GraphQLResponseCache;
106
import lombok.RequiredArgsConstructor;
11-
import lombok.extern.slf4j.Slf4j;
127

13-
@Slf4j
148
@RequiredArgsConstructor
159
class ErrorQueryResponseWriter implements QueryResponseWriter {
1610

1711
private final int statusCode;
1812
private final String message;
19-
private final GraphQLInvocationInput invocationInput;
2013

2114
@Override
22-
public void write(HttpServletRequest request, HttpServletResponse response, GraphQLResponseCache responseCache) throws IOException {
23-
if (responseCache != null && responseCache.isCacheable(request, invocationInput)) {
24-
try {
25-
responseCache.put(request, invocationInput, CachedResponse.ofError(statusCode, message));
26-
} catch (Throwable t) {
27-
log.warn(t.getMessage(), t);
28-
log.warn("Ignore read from cache, unexpected error happened");
29-
}
30-
}
15+
public void write(HttpServletRequest request, HttpServletResponse response) throws IOException {
3116
response.sendError(statusCode, message);
3217
}
3318

‎graphql-java-servlet/src/main/java/graphql/kickstart/servlet/GraphQLConfiguration.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import graphql.kickstart.execution.GraphQLObjectMapper;
55
import graphql.kickstart.execution.GraphQLQueryInvoker;
66
import graphql.kickstart.execution.context.ContextSetting;
7-
import graphql.kickstart.servlet.cache.GraphQLResponseCache;
7+
import graphql.kickstart.servlet.cache.GraphQLResponseCacheManager;
88
import graphql.kickstart.servlet.config.DefaultGraphQLSchemaServletProvider;
99
import graphql.kickstart.servlet.config.GraphQLSchemaServletProvider;
1010
import graphql.kickstart.servlet.context.GraphQLServletContextBuilder;
@@ -37,13 +37,13 @@ public class GraphQLConfiguration {
3737
@Getter
3838
private final long asyncTimeout;
3939
private final ContextSetting contextSetting;
40-
private final GraphQLResponseCache responseCache;
40+
private final GraphQLResponseCacheManager responseCacheManager;
4141

4242
private GraphQLConfiguration(GraphQLInvocationInputFactory invocationInputFactory,
4343
GraphQLQueryInvoker queryInvoker,
4444
GraphQLObjectMapper objectMapper, List<GraphQLServletListener> listeners, boolean asyncServletModeEnabled,
4545
Executor asyncExecutor, long subscriptionTimeout, long asyncTimeout, ContextSetting contextSetting,
46-
Supplier<BatchInputPreProcessor> batchInputPreProcessor, GraphQLResponseCache responseCache) {
46+
Supplier<BatchInputPreProcessor> batchInputPreProcessor, GraphQLResponseCacheManager responseCacheManager) {
4747
this.invocationInputFactory = invocationInputFactory;
4848
this.queryInvoker = queryInvoker;
4949
this.graphQLInvoker = queryInvoker.toGraphQLInvoker();
@@ -55,7 +55,7 @@ private GraphQLConfiguration(GraphQLInvocationInputFactory invocationInputFactor
5555
this.asyncTimeout = asyncTimeout;
5656
this.contextSetting = contextSetting;
5757
this.batchInputPreProcessor = batchInputPreProcessor;
58-
this.responseCache = responseCache;
58+
this.responseCacheManager = responseCacheManager;
5959
}
6060

6161
public static GraphQLConfiguration.Builder with(GraphQLSchema schema) {
@@ -116,8 +116,8 @@ public BatchInputPreProcessor getBatchInputPreProcessor() {
116116
return batchInputPreProcessor.get();
117117
}
118118

119-
public GraphQLResponseCache getResponseCache() {
120-
return responseCache;
119+
public GraphQLResponseCacheManager getResponseCacheManager() {
120+
return responseCacheManager;
121121
}
122122

123123
public static class Builder {
@@ -133,7 +133,7 @@ public static class Builder {
133133
private long asyncTimeout = 30;
134134
private ContextSetting contextSetting = ContextSetting.PER_QUERY_WITH_INSTRUMENTATION;
135135
private Supplier<BatchInputPreProcessor> batchInputPreProcessorSupplier = NoOpBatchInputPreProcessor::new;
136-
private GraphQLResponseCache responseCache;
136+
private GraphQLResponseCacheManager responseCacheManager;
137137

138138
private Builder(GraphQLInvocationInputFactory.Builder invocationInputFactoryBuilder) {
139139
this.invocationInputFactoryBuilder = invocationInputFactoryBuilder;
@@ -217,8 +217,8 @@ public Builder with(Supplier<BatchInputPreProcessor> batchInputPreProcessor) {
217217
return this;
218218
}
219219

220-
public Builder with(GraphQLResponseCache responseCache) {
221-
this.responseCache = responseCache;
220+
public Builder with(GraphQLResponseCacheManager responseCache) {
221+
this.responseCacheManager = responseCache;
222222
return this;
223223
}
224224

@@ -234,7 +234,7 @@ public GraphQLConfiguration build() {
234234
asyncTimeout,
235235
contextSetting,
236236
batchInputPreProcessorSupplier,
237-
responseCache
237+
responseCacheManager
238238
);
239239
}
240240

‎graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java‎

Lines changed: 13 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,7 @@
66
import graphql.kickstart.execution.input.GraphQLBatchedInvocationInput;
77
import graphql.kickstart.execution.input.GraphQLInvocationInput;
88
import graphql.kickstart.execution.input.GraphQLSingleInvocationInput;
9-
import graphql.kickstart.servlet.cache.CacheResponseWriter;
10-
import graphql.kickstart.servlet.cache.CachedResponse;
9+
import graphql.kickstart.servlet.cache.CacheReader;
1110
import graphql.kickstart.servlet.input.BatchInputPreProcessResult;
1211
import graphql.kickstart.servlet.input.BatchInputPreProcessor;
1312
import lombok.extern.slf4j.Slf4j;
@@ -52,31 +51,20 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr
5251
private void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request,
5352
HttpServletResponse response) {
5453
try {
55-
if (configuration.getResponseCache() != null) {
56-
CachedResponse cachedResponse = null;
57-
try {
58-
cachedResponse = configuration.getResponseCache().get(request, invocationInput);
59-
} catch (Throwable t) {
60-
log.warn(t.getMessage(), t);
61-
log.warn("Ignore read from cache, unexpected error happened");
62-
}
54+
// try to return value from cache if cache manager was set, otherwise processed the query
55+
if (configuration.getResponseCacheManager() != null &&
56+
!CacheReader.responseFromCache(invocationInput, request, response, configuration.getResponseCacheManager())) {
57+
GraphQLQueryResult queryResult = invoke(invocationInput, request, response);
6358

64-
if (cachedResponse != null) {
65-
CacheResponseWriter cacheResponseWriter = new CacheResponseWriter();
66-
cacheResponseWriter.write(request, response, cachedResponse);
67-
return;
68-
}
59+
QueryResponseWriter queryResponseWriter = QueryResponseWriter.createWriter(
60+
queryResult,
61+
configuration.getObjectMapper(),
62+
configuration.getSubscriptionTimeout(),
63+
invocationInput,
64+
configuration.getResponseCacheManager()
65+
);
66+
queryResponseWriter.write(request, response);
6967
}
70-
71-
GraphQLQueryResult queryResult = invoke(invocationInput, request, response);
72-
73-
QueryResponseWriter queryResponseWriter = QueryResponseWriter.createWriter(
74-
queryResult,
75-
configuration.getObjectMapper(),
76-
configuration.getSubscriptionTimeout(),
77-
invocationInput
78-
);
79-
queryResponseWriter.write(request, response, configuration.getResponseCache());
8068
} catch (Throwable t) {
8169
response.setStatus(STATUS_BAD_REQUEST);
8270
log.info("Bad GET request: path was not \"/schema.json\" or no query variable named \"query\" given");

‎graphql-java-servlet/src/main/java/graphql/kickstart/servlet/QueryResponseWriter.java‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,33 +3,43 @@
33
import graphql.kickstart.execution.GraphQLQueryResult;
44
import graphql.kickstart.execution.GraphQLObjectMapper;
55
import graphql.kickstart.execution.input.GraphQLInvocationInput;
6-
import graphql.kickstart.servlet.cache.GraphQLResponseCache;
6+
import graphql.kickstart.servlet.cache.CachingQueryResponseWriter;
7+
import graphql.kickstart.servlet.cache.GraphQLResponseCacheManager;
78

89
import java.io.IOException;
910
import java.util.Objects;
1011
import javax.servlet.http.HttpServletRequest;
1112
import javax.servlet.http.HttpServletResponse;
1213

13-
interface QueryResponseWriter {
14+
public interface QueryResponseWriter {
1415

1516
static QueryResponseWriter createWriter(
1617
GraphQLQueryResult result,
1718
GraphQLObjectMapper graphQLObjectMapper,
1819
long subscriptionTimeout,
19-
GraphQLInvocationInput invocationInput
20+
GraphQLInvocationInput invocationInput,
21+
GraphQLResponseCacheManager responseCache
2022
) {
2123
Objects.requireNonNull(result, "GraphQL query result cannot be null");
2224

25+
QueryResponseWriter writer;
26+
2327
if (result.isBatched()) {
24-
return new BatchedQueryResponseWriter(result.getResults(), graphQLObjectMapper, invocationInput);
28+
writer = new BatchedQueryResponseWriter(result.getResults(), graphQLObjectMapper);
2529
} else if (result.isAsynchronous()) {
26-
return new SingleAsynchronousQueryResponseWriter(result.getResult(), graphQLObjectMapper, subscriptionTimeout, invocationInput);
30+
writer = new SingleAsynchronousQueryResponseWriter(result.getResult(), graphQLObjectMapper, subscriptionTimeout);
2731
} else if (result.isError()) {
28-
return new ErrorQueryResponseWriter(result.getStatusCode(), result.getMessage(), invocationInput);
32+
writer = new ErrorQueryResponseWriter(result.getStatusCode(), result.getMessage());
33+
} else {
34+
writer = new SingleQueryResponseWriter(result.getResult(), graphQLObjectMapper);
35+
}
36+
37+
if (responseCache != null) {
38+
writer = new CachingQueryResponseWriter(writer, responseCache, invocationInput, result.isError());
2939
}
30-
return new SingleQueryResponseWriter(result.getResult(), graphQLObjectMapper, invocationInput);
40+
return writer;
3141
}
3242

33-
void write(HttpServletRequest request, HttpServletResponse response, GraphQLResponseCache responseCache) throws IOException;
43+
void write(HttpServletRequest request, HttpServletResponse response) throws IOException;
3444

3545
}

‎graphql-java-servlet/src/main/java/graphql/kickstart/servlet/SingleAsynchronousQueryResponseWriter.java‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,31 +10,21 @@
1010
import javax.servlet.AsyncContext;
1111
import javax.servlet.http.HttpServletRequest;
1212
import javax.servlet.http.HttpServletResponse;
13-
14-
import graphql.kickstart.execution.input.GraphQLInvocationInput;
15-
import graphql.kickstart.servlet.cache.GraphQLResponseCache;
1613
import lombok.Getter;
1714
import lombok.RequiredArgsConstructor;
18-
import lombok.extern.slf4j.Slf4j;
1915
import org.reactivestreams.Publisher;
2016
import org.reactivestreams.Subscription;
2117

22-
@Slf4j
2318
@RequiredArgsConstructor
2419
class SingleAsynchronousQueryResponseWriter implements QueryResponseWriter {
2520

2621
@Getter
2722
private final ExecutionResult result;
2823
private final GraphQLObjectMapper graphQLObjectMapper;
2924
private final long subscriptionTimeout;
30-
private final GraphQLInvocationInput invocationInput;
3125

3226
@Override
33-
public void write(HttpServletRequest request, HttpServletResponse response, GraphQLResponseCache responseCache) {
34-
if (responseCache != null) {
35-
log.warn("Response cache for asynchronous query are not implemented yet");
36-
}
37-
27+
public void write(HttpServletRequest request, HttpServletResponse response) {
3828
Objects.requireNonNull(request, "Http servlet request cannot be null");
3929
response.setContentType(HttpRequestHandler.APPLICATION_EVENT_STREAM_UTF8);
4030
response.setStatus(HttpRequestHandler.STATUS_OK);

‎graphql-java-servlet/src/main/java/graphql/kickstart/servlet/SingleQueryResponseWriter.java‎

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,42 +2,26 @@
22

33
import graphql.ExecutionResult;
44
import graphql.kickstart.execution.GraphQLObjectMapper;
5-
import graphql.kickstart.execution.input.GraphQLInvocationInput;
6-
import graphql.kickstart.servlet.cache.CachedResponse;
7-
import graphql.kickstart.servlet.cache.GraphQLResponseCache;
85
import lombok.RequiredArgsConstructor;
9-
import lombok.extern.slf4j.Slf4j;
106

11-
import java.io.IOException;
12-
import java.nio.charset.StandardCharsets;
137
import javax.servlet.http.HttpServletRequest;
148
import javax.servlet.http.HttpServletResponse;
9+
import java.io.IOException;
10+
import java.nio.charset.StandardCharsets;
1511

16-
@Slf4j
1712
@RequiredArgsConstructor
1813
class SingleQueryResponseWriter implements QueryResponseWriter {
1914

2015
private final ExecutionResult result;
2116
private final GraphQLObjectMapper graphQLObjectMapper;
22-
private final GraphQLInvocationInput invocationInput;
2317

2418
@Override
25-
public void write(HttpServletRequest request, HttpServletResponse response, GraphQLResponseCache responseCache) throws IOException {
19+
public void write(HttpServletRequest request, HttpServletResponse response) throws IOException {
2620
response.setContentType(HttpRequestHandler.APPLICATION_JSON_UTF8);
2721
response.setStatus(HttpRequestHandler.STATUS_OK);
2822
response.setCharacterEncoding(StandardCharsets.UTF_8.name());
2923
String responseContent = graphQLObjectMapper.serializeResultAsJson(result);
3024
byte[] contentBytes = responseContent.getBytes(StandardCharsets.UTF_8);
31-
32-
if (responseCache != null && responseCache.isCacheable(request, invocationInput)) {
33-
try {
34-
responseCache.put(request, invocationInput, CachedResponse.ofContent(contentBytes));
35-
} catch (Throwable t) {
36-
log.warn(t.getMessage(), t);
37-
log.warn("Ignore read from cache, unexpected error happened");
38-
}
39-
}
40-
4125
response.setContentLength(contentBytes.length);
4226
response.getOutputStream().write(contentBytes);
4327
}

0 commit comments

Comments
 (0)