Skip to content

change preparsedDocumentProvider return value to CompletableFuture<PreparsedDocumentEntry> #2612

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.

Sign up for GitHub

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

Jump to bottom
Merged
bbakerman merged 10 commits into from
Dec 6, 2021
Merged

change preparsedDocumentProvider return value to CompletableFuture<PreparsedDocumentEntry> #2612

bbakerman merged 10 commits into from
Dec 6, 2021

Conversation

@heoYH
Copy link
Contributor

@heoYH heoYH commented Nov 15, 2021

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

@heo-d
change preparsedDocumentProvider return value PreparsedDocumentEntry …
704f607 
…to CompletableFuture<PreparsedDocumentEntry>
bbakerman
bbakerman reviewed Nov 15, 2021
View reviewed changes
src/main/java/graphql/execution/preparsed/persisted/PersistedQueryCache.java Outdated
* @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;
Copy link
Member

@bbakerman bbakerman Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
bbakerman requested changes Nov 15, 2021
View reviewed changes
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
bbakerman reviewed Nov 15, 2021
View reviewed changes
src/main/java/graphql/execution/preparsed/persisted/PersistedQuerySupport.java
}

@Override
public PreparsedDocumentEntry getDocument(ExecutionInput executionInput, Function<ExecutionInput, PreparsedDocumentEntry> parseAndValidateFunction) {
Copy link
Member

@bbakerman bbakerman Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

heoYH commented Nov 15, 2021

@bbakerman thanks for your review

i understand your comment

and i fixed this PR

If you have time, please review again.

@heoYH heoYH requested a review from bbakerman November 15, 2021 07:06
bbakerman
bbakerman reviewed Nov 15, 2021
View reviewed changes
src/test/groovy/graphql/execution/preparsed/persisted/InMemoryPersistedQueryCacheTest.groovy Outdated
def getDoc = inMemCache.getPersistedQueryDocument(hash, ei, onMiss)
def doc = getDoc.document
def getDoc = inMemCache.getPersistedQueryDocumentAsync(hash, ei, onMiss)
def doc = getDoc.get().document
Copy link
Member

@bbakerman bbakerman Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@heoYH
Copy link
Contributor Author

heoYH commented Nov 16, 2021

@bbakerman thanks for review

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

@heoYH heoYH requested a review from bbakerman November 16, 2021 01:48
bbakerman
bbakerman requested changes Nov 17, 2021
View reviewed changes
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/main/java/graphql/GraphQL.java Outdated
import graphql.execution.instrumentation.InstrumentationState;
import graphql.execution.instrumentation.SimpleInstrumentation;
import graphql.execution.*;
import graphql.execution.instrumentation.*;
Copy link
Member

@bbakerman bbakerman Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/main/java/graphql/nextgen/GraphQL.java Outdated
import graphql.Internal;
import graphql.ParseAndValidate;
import graphql.ParseAndValidateResult;
import graphql.*;
Copy link
Member

@bbakerman bbakerman Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit imports

@heoYH
Copy link
Contributor Author

heoYH commented Nov 17, 2021

@bbakerman Thanks for the review

I'm glad it could be a merge

@heoYH heoYH requested a review from bbakerman November 17, 2021 02:22
bbakerman
bbakerman approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you and thanks for the follow up

@bbakerman bbakerman added this to the 18.0 milestone Nov 17, 2021
@bbakerman
Copy link
Member

bbakerman commented Nov 17, 2021

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
Copy link
Contributor Author

heoYH commented Nov 17, 2021 •
edited
Loading

@bbakerman
fix @link method do not use generic parameter

@bbakerman
Copy link
Member

bbakerman commented Dec 2, 2021

Hmmm not sure why this is failing the tests

@heoYH
Copy link
Contributor Author

heoYH commented Dec 4, 2021

@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.

bbakerman
bbakerman approved these changes Dec 6, 2021
View reviewed changes
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this PR

@bbakerman bbakerman merged commit 89a3373 into graphql-java:master Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

18.0

Development

Successfully merging this pull request may close these issues.

3 participants

@heoYH @bbakerman @heo-d