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

Cache queries #2017

Merged
merged 32 commits into from
Jan 17, 2022
Merged

Cache queries #2017

merged 32 commits into from
Jan 17, 2022

Conversation

k0ka
Copy link
Collaborator

@k0ka k0ka commented Dec 30, 2021

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Query caching has been added: the result of GraphQL\Language\Parser::parse() will be serialized and stored in the Laravel cache. It can be disabled in the config.

I tested it on my production queries and it shows about 5-10% performance boost. I use Relay with a lot of fragments.

I use sha256 hash of query as a key. So we may develop it further towards implementing Apollo Automatic persisted queries. While Relay currently uses md5 hashes for their Persisted queries, I guess they will change it soon to a better hashing algorithm.

Breaking changes

GraphQL::executeQuery() is deprecated and should be removed in the next version. parseAndExecuteQuery() or executeParsedQuery() should be used instead. This PR has a simple executeQuery() stub that executes one of above methods depending on the query type.

@spawnia spawnia added the enhancement A feature or improvement label Jan 1, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
UPGRADE.md Outdated Show resolved Hide resolved
Query caching is enabled in non-local environments by default. You can define cache store and cache duration,
see `config/lighthouse.php`.

The GraphQL schema doesn't affect query parsing, so you have to flush query cache only on the package update.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The GraphQL schema doesn't affect query parsing, so you have to flush query cache only on the package update.
Make sure to flush the query cache when you deploy an upgraded version of the dependency `webonyx/graphql-php`:
some-one-liner that clears the cache?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add another command like lighthouse:query:cache:clear but it only clears the cache store. So I guess it's easier to just use the laravel default command.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That Laravel command clears all caches?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it fully clears the default store. If the user specified non-default store for query cache they should add an argument to the command
php artisan cache:clear --store=query_cache_store

src/GraphQL.php Show resolved Hide resolved
src/lighthouse.php Outdated Show resolved Hide resolved
src/lighthouse.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator

spawnia commented Jan 1, 2022

I need to think about the deprecation of executeQuery() and the naming of the new methods.

Perhaps we should extract a method parse(string $query): DocumentNode?

k0ka and others added 2 commits January 2, 2022 12:19
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
@k0ka
Copy link
Collaborator Author

k0ka commented Jan 2, 2022

Perhaps we should extract a method parse(string $query): DocumentNode?

This method might also throw a SyntaxError which must be caught and wrapped in the ExecutionResult. So it's better to leave parseAndExecuteQuery() and it is not too big and complex to split it.

@spawnia
Copy link
Collaborator

spawnia commented Jan 2, 2022

I could imagine the memoized parse() method being useful by itself.

@k0ka k0ka mentioned this pull request Jan 3, 2022
@k0ka
Copy link
Collaborator Author

k0ka commented Jan 8, 2022

I extracted the GraphQL::parse() method

Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Like the extracted method, just a few more nits regarding the docs.

Considering Automated persisted queries, would you foresee further changes to the proposed API?

src/GraphQL.php Outdated Show resolved Hide resolved
src/GraphQL.php Show resolved Hide resolved
src/GraphQL.php Outdated Show resolved Hide resolved
src/lighthouse.php Outdated Show resolved Hide resolved
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
@k0ka
Copy link
Collaborator Author

k0ka commented Jan 8, 2022

Considering Automated persisted queries, would you foresee further changes to the proposed API?

I don't thinks this API will have any changes. We will have to add another method like getParsedQueryByHash(string $sha256hash): DocumentNode. It should get the corresponding cache value and return apollo compatible error if this value is not present in the cache.

@spawnia
Copy link
Collaborator

spawnia commented Jan 11, 2022

The API of the GraphQL class is one thing that might have to change, but I am also concerned about the configuration format. We have some questionable structure in the config file (schema.register...) already. I would like to implement both features at once to make sure they fit together nicely, The config for APQ would probably overlap with query_cache quite a bit.

I can look into this myself, but am also quite happy if you push it further.

@k0ka
Copy link
Collaborator Author

k0ka commented Jan 15, 2022

I added the APQ support. webonyx/graphql-php already supports it so it was quite easy.

As for config schema: I guess the better cache format will be like this:

'cache' => [
    'schema' => [
          'version' => env('LIGHTHOUSE_CACHE_VERSION', 1),

          // other parameters that are currently in the cache.*
    ],

    'query' => [
          // parameters from query_cache
    ],

    'validate' => [
         // future parameters for https://github.com/nuwave/lighthouse/issues/2018
    ],
]

But it will break backward compatibility. So I guess it's easier to leave as is and refactor it in the next major version.

@k0ka k0ka requested a review from spawnia January 15, 2022 12:58
Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Awesome job!

@spawnia
Copy link
Collaborator

spawnia commented Jan 17, 2022

@k0ka invited you as a collaborator. If you would like to join our Slack, I can add you to our collaborator chat as well.

@spawnia spawnia merged commit fa22f75 into nuwave:master Jan 17, 2022
@k0ka k0ka deleted the cache_queries branch January 22, 2022 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants