-
Notifications
You must be signed in to change notification settings - Fork 656
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
Support send HttpGetMethod for Persisted Queries #1376
Support send HttpGetMethod for Persisted Queries #1376
Conversation
@@ -20,15 +20,27 @@ | |||
|
|||
private final ApolloLogger logger; | |||
private volatile boolean disposed; | |||
private boolean useHttpGetMethodForQueries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this flag can be local in interceptAsync
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
InterceptorRequest newRequest = request.toBuilder() | ||
.sendQueryDocument(false) | ||
.autoPersistQueries(true) | ||
.useHttpGetMethodForQueries(useHttpGetMethodForQueries || useHttpGetMethodForPersistedQueries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: think can be replaced with:
.useHttpGetMethodForQueries(request.useHttpGetMethodForQueries || useHttpGetMethodForPersistedQueries)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced, thanks!
return Optional.of(request.toBuilder() | ||
.sendQueryDocument(true) | ||
.autoPersistQueries(true) | ||
.useHttpGetMethodForQueries(useHttpGetMethodForQueries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we don't need this as request
is our original request (not modified). So it will have already set this flag.
See line with:
chain.proceedAsync(newRequest, dispatcher, new CallBack() {
@Override public void onResponse(@NotNull InterceptorResponse response) {
if (disposed) return;
Optional<InterceptorRequest> retryRequest = handleProtocolNegotiation(request, response);
request
is original request not modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you r right, as original request is preserved, can reuse that one. Fixed as suggestion above.
Beware that we still need to send the http body with extension.persistedQuery
for creating PQ cache in server for later request. So we need .sendQueryDocument(true) and .autoPersistQueries(true)
and guarantee original request has those value on.
And its value now depends in RealApolloCall.java line. 139
ApolloInterceptor.InterceptorRequest request = ApolloInterceptor.InterceptorRequest.builder(operation)
.cacheHeaders(cacheHeaders)
.requestHeaders(requestHeaders)
.fetchFromCache(false)
.optimisticUpdates(optimisticUpdates)
.useHttpGetMethodForQueries(useHttpGetMethodForQueries)
.autoPersistQueries(enableAutoPersistedQueries)
.build();
interceptorChain.proceedAsync(request, dispatcher, interceptorCallbackProxy());
}
56bbd15
to
b35c6b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Ref. Integrate APQs with CDN
Implemented
useHttpGetMethodForPersistedQueries
which sends with HttpGetMethod when it's a persisted query request, otherwise send HttpGetMethod or HttpPostMethod depends on global setting ofuseHttpGetMethodForQueries
.Example
ref. #1317 #1055