change preparsedDocumentProvider return value to CompletableFuture<PreparsedDocumentEntry> by heoYH · Pull Request #2612 · graphql-java/graphql-java

heoYH · 2021-11-15T01:22:51Z

change pre parsed Document Provider return value to CompletableFuture<Preparsed DocumentEntry>

because

we use graphql-java on Spring webflux

if we use mongodb or redis for persistent query db then preparsedDocumentProvider.getDocument() call is blocking

…to CompletableFuture<PreparsedDocumentEntry>

bbakerman · 2021-11-15T06:14:32Z

src/main/java/graphql/execution/preparsed/persisted/PersistedQueryCache.java

     * @throws graphql.execution.preparsed.persisted.PersistedQueryNotFound if the query id is not know at all and you have no query text
     */
-    PreparsedDocumentEntry getPersistedQueryDocument(Object persistedQueryId, ExecutionInput executionInput, PersistedQueryCacheMiss onCacheMiss) throws PersistedQueryNotFound;
+    CompletableFuture<PreparsedDocumentEntry> getPersistedQueryDocument(Object persistedQueryId, ExecutionInput executionInput, PersistedQueryCacheMiss onCacheMiss) throws PersistedQueryNotFound;


This is indeed a great idea. This should indeed be an async method because its likely to go to a DB etc...for most implementations.

However as implemented this is a hard breaking change.

We can avoid this in this case.

We can

deprecate the PreparsedDocumentEntry getPersistedQueryDocument( method

put in a default CompletableFuture<PreparsedDocumentEntry> getPersistedQueryDocumentAsync( method

make the default call the getPersistedQueryDocument and wrap it in a completed CF

make all the calls sites called CompletableFuture<PreparsedDocumentEntry> getPersistedQueryDocumentAsync( and hence it will invoke the sync version for every one else.

bbakerman

We need to make this a non breaking API change because we can. We sometimes make breaking changes when we cant avoid them but in this case we can and we should avoid it.

Otherwise thank you very much for this PR

bbakerman · 2021-11-15T06:19:03Z

src/main/java/graphql/execution/preparsed/persisted/PersistedQuerySupport.java

    }

    @Override
-    public PreparsedDocumentEntry getDocument(ExecutionInput executionInput, Function<ExecutionInput, PreparsedDocumentEntry> parseAndValidateFunction) {


Same pattern here. We delegate back from an async method to this method

This is also @PublicSpi and hence people will have derived from it and hence we dont want to break them

heoYH · 2021-11-15T07:06:17Z

@bbakerman thanks for your review

i understand your comment

and i fixed this PR

If you have time, please review again.

src/main/java/graphql/execution/preparsed/PreparsedDocumentProvider.java

src/main/java/graphql/execution/preparsed/persisted/PersistedQueryCache.java

bbakerman · 2021-11-15T21:21:32Z

src/test/groovy/graphql/execution/preparsed/persisted/InMemoryPersistedQueryCacheTest.groovy

-        def getDoc = inMemCache.getPersistedQueryDocument(hash, ei, onMiss)
-        def doc = getDoc.document
+        def getDoc = inMemCache.getPersistedQueryDocumentAsync(hash, ei, onMiss)
+        def doc = getDoc.get().document


Can we have one test that goes via the old deprecated path so that we have surety that while its alive it works

heoYH · 2021-11-16T01:48:01Z

@bbakerman thanks for review

add javaDoc,
rollback Async method Test,
add test that sync and async result is same

bbakerman

I am happy to merge this once the explicit imports is fixed.

Sorry to be a pain.

This is a great PR and will be very useful and I am keen to get it in - but I want to maintain code health as well

bbakerman · 2021-11-17T00:39:08Z

src/main/java/graphql/GraphQL.java

-import graphql.execution.instrumentation.InstrumentationState;
-import graphql.execution.instrumentation.SimpleInstrumentation;
+import graphql.execution.*;
+import graphql.execution.instrumentation.*;


Sorry we require explicit imports - if you are using IDEA set them to 999 before using * for this project

bbakerman · 2021-11-17T00:40:35Z

src/main/java/graphql/nextgen/GraphQL.java

-import graphql.Internal;
-import graphql.ParseAndValidate;
-import graphql.ParseAndValidateResult;
+import graphql.*;


explicit imports

heoYH · 2021-11-17T02:22:03Z

@bbakerman Thanks for the review

I'm glad it could be a merge

bbakerman

Thanks you and thanks for the follow up

bbakerman · 2021-11-17T06:55:29Z

Build failing

/home/runner/work/graphql-java/graphql-java/src/main/java/graphql/execution/preparsed/PreparsedDocumentProvider.java:27: error: type arguments not allowed here
> Task :javadoc
     * @deprecated - use {@link #getDocumentAsync(ExecutionInput executionInput, Function<ExecutionInput, PreparsedDocumentEntry> parseAndValidateFunction)}
                                ^
/home/runner/work/graphql-java/graphql-java/src/main/java/graphql/execution/preparsed/PreparsedDocumentProvider.java:30: warning - Tag @link:illegal character: "60" in "#getDocumentAsync(ExecutionInput executionInput, Function<ExecutionInput, PreparsedDocumentEntry> parseAndValidateFunction)"
/home/runner/work/graphql-java/graphql-java/src/main/java/graphql/execution/preparsed/PreparsedDocumentProvider.java:30: warning - Tag @link:illegal character: "62" in "#getDocumentAsync(ExecutionInput executionInput, Function<ExecutionInput, PreparsedDocumentEntry> parseAndValidateFunction)"
/home/runner/work/graphql-java/graphql-java/src/main/java/graphql/execution/preparsed/PreparsedDocumentProvider.java:30: warning - Tag @link: can't find getDocumentAsync(ExecutionInput executionInput, Function<ExecutionInput, PreparsedDocumentEntry> parseAndValidateFunction) in graphql.execution.preparsed.PreparsedDocumentProvider

heoYH · 2021-11-17T08:36:20Z

@bbakerman
fix @link method do not use generic parameter

bbakerman · 2021-12-02T23:50:14Z

Hmmm not sure why this is failing the tests

… call

heoYH · 2021-12-04T04:07:22Z

@bbakerman hi

Add handling of AbortExecutionException thrown inside an asynchronous call

I think, should add an exception handling layer
Because asynchronous errors and synchronous errors are mixed.

… call

bbakerman

Thanks so much for this PR

change preparsedDocumentProvider return value PreparsedDocumentEntry …

704f607

…to CompletableFuture<PreparsedDocumentEntry>