Skip to content
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

Add section on operation caching to reference documentation #233

Closed

Conversation

jord1e
Copy link
Contributor

@jord1e jord1e commented Dec 22, 2021

Closes #234

Hello,

This PR adds support for PreparsedDocumentProvider.

GraphQL Java documentation is available here: https://www.graphql-java.com/documentation/execution#query-caching

This also adds support for Apollo Persisted Queries via PersistedQuerySupport and ApolloPersistedQuerySupport- an implementation example or integration test for this may be wanted in the future

There are a couple of pain points that need to be resolved before merging:

  1. Formatting in IntelliJ messes everything up > how do I fix this (maybe include this in the README or a CONTRIBUTING.md)
  2. Copyright headers are not yet added
  3. Please check my English

@jord1e jord1e marked this pull request as draft December 22, 2021 13:35
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 22, 2021
@rstoyanchev
Copy link
Contributor

Thanks for the suggestion.

It is already possible to get access to GraphQL.Builder. For example:

@Bean
public GraphQlSourceBuilderCustomizer reactiveQuerydslRegistrar() {
	return sourceBuilder -> {
		PersistedQueryCache cache = new InMemoryPersistedQueryCache(new HashMap<>());
		ApolloPersistedQuerySupport provider = new ApolloPersistedQuerySupport(cache);
		sourceBuilder.configureGraphQl(builder -> builder.preparsedDocumentProvider(provider));
	};
}

I'm wondering why we should add this as an explicit option? It is a little more convenient, but no other benefit I can see and we don't need to mirror all available options on GraphQL.Builder if they are just pass-through's. Maybe a section in the docs on the topic along with an example could be sufficient.

@jord1e
Copy link
Contributor Author

jord1e commented Jan 14, 2022

I have reflected on this a little bit.

DGS Framework has the option of specify a bean, which injects the PreparsedDocumentProvider into the GraphQL object at runtime:

https://github.com/Netflix/dgs-framework/blob/3d1d61f52e7308ef21c0b5a195f9a01511421b18/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt#L98-L102

Specifying a PreparsedDocumentProvider that caches frequently occurring queries should really be done for every application running in production. When profiling my application some time ago parsing and validating took ~18% of execution time on small queries:

Profiling results

After introducing the PreparsedDocumentProvider with caching this was reduced to a Caffeine cache lookup call, which is practically free. This result is amplified when the queries get bigger, and nesting increases (fragments are big offenders).

Specifying a PreparsedDocumentProvider should therefore be encouraged by the framework, and not stowed away in GraphQlSourceBuilderCustomizer. If there is sufficient support, I am prepared to open a PR for the WebMVC and WebFlux spring boot starters in order to facilitate injection via a @bean annotation by next week (maybe in time for 1.0.0-M5?).

@rstoyanchev
Copy link
Contributor

I appreciate the further insight.

The aim for methods on GraphQlSource.Builder is to expose hooks into the build() algorithm so everything is possible, but not every underlying option is mirrored. In that sense, the key is whether we need to be involved in some part of the config to ensure everything works or enable something not possible otherwise, like this recent change 2ecac0b to enable multiple WiringFactory instances by independent parties.

In terms of the importance, I'm keen on adding to the documentation in the section on GraphQlSource where we elaborate on a range of things you can configure, providing mentions mentions for things that people look for and it helps them to get oriented.

@jord1e
Copy link
Contributor Author

jord1e commented Jan 18, 2022

So adding automatic registration of a PreparsedDocumentProvider bean via the starters is out of the question?

I can edit the pull request to add documentation in that case.

@rstoyanchev
Copy link
Contributor

Correct, no plans at the moment to add detection of PreparsedDocumentProvider beans. Feel free to edit the PR along the lines of turning it into a documentation change. Thanks!

@rstoyanchev rstoyanchev added the type: documentation A documentation task label Jan 19, 2022
# Conflicts:
#	spring-graphql/src/main/java/org/springframework/graphql/execution/DefaultGraphQlSourceBuilder.java
@jord1e jord1e force-pushed the preparsed-document-provider branch from 1748866 to 65545a7 Compare January 30, 2022 11:51
@jord1e jord1e force-pushed the preparsed-document-provider branch from 65545a7 to 9f9d093 Compare January 30, 2022 12:06
@jord1e jord1e marked this pull request as ready for review January 30, 2022 12:07
@jord1e
Copy link
Contributor Author

jord1e commented Jan 30, 2022

@rstoyanchev Ready for review, sorry for the delay

@rstoyanchev rstoyanchev self-assigned this Feb 8, 2022
@rstoyanchev rstoyanchev removed the status: waiting-for-triage An issue we've not yet triaged label Feb 8, 2022
@rstoyanchev rstoyanchev added this to the 1.0.0-M6 milestone Feb 8, 2022
@rstoyanchev rstoyanchev changed the title Add PreparsedDocumentProvider support Add section on operation caching to reference documentation Feb 17, 2022
rstoyanchev pushed a commit that referenced this pull request Feb 17, 2022
rstoyanchev added a commit that referenced this pull request Feb 17, 2022
Important to mention the `GraphQlSourceBuilderCustomizer` which is
otherwise mentioned neither in the Boot starter nor the reference here.
It is the path to a number of customizations that are discussed in
`GraphQlSource` subsections.

See gh-233
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 17, 2022

I changed this slightly to refer to the GraphQL Java documentation for most of the details, and for the rest focus on Spring GraphQL specific config.

One more note that the processing of the PR was more time complicated than necessary because the main branch was merged into the PR branch. This is a problem because the commits need to be squashed. So generally, either use rebase or otherwise do not worry if it gets behind. We'll take care to rebase it before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for PreparsedDocumentProvider
3 participants