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: Fix check if endpoint is /graphql or not #15599

Merged
merged 8 commits into from
Sep 16, 2022

Conversation

joselcvarela
Copy link
Member

@joselcvarela joselcvarela commented Sep 15, 2022

Description

By using CACHE_STATUS_HEADER we were able to check that Graphql requests always reported MISS which means that Graphql requests were not being cached.
The source of the issue is that Express reports on req.path the route we defined for the controller.
For example, we have a router for entire application where we define router.post('/graphl', graphqlRouter).
Then we define graphqlRouter.post('/', handler). In this case, inside handler our req.path will be / because /graphql was defined on parent.

In order to solve the issue we take req.originalUrl which contains the entire path of the request.

Also, it seems that System Graphql was not being cached, so I added those too, but let me know if that's intended and I can remove it.

After fixing the initial issue I found that every request was responding with the Grapqhl Introspection.
This was cause by retrieving the same key every time because req.query for Grapqhl requests does/should not exist.
So fixed the key to be based on req.body

Type of Change

Requirements Checklist

  • New / updated tests are included
  • All tests are passing locally
  • Performed a self-review of the submitted code

If adding a new feature:

  • Documentation was added/updated. PR:

@joselcvarela joselcvarela changed the title Fix check if endpoint is graphql or not Cache: Fix check if endpoint is /graphql or not Sep 15, 2022
@joselcvarela joselcvarela marked this pull request as draft September 15, 2022 18:18
Copy link
Member

@licitdev licitdev left a comment

Choose a reason for hiding this comment

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

PR affects all POST requests. Changes in progress...

@joselcvarela
Copy link
Member Author

In fact this is not working as expected and will break eveything.
All Graphql request are responding with the introspection response 😐
Will take a look now

@rijkvanzanten
Copy link
Member

In fact this is not working as expected and will break eveything.
All Graphql request are responding with the introspection response 😐
Will take a look now

Make sure you clear your own cache during testing occasionally

Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

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

Also gotta make sure we don't cache if the body includes mutations

@joselcvarela
Copy link
Member Author

joselcvarela commented Sep 15, 2022

Also gotta make sure we don't cache if the body includes mutations

This seems to already be handled here:
https://github.com/directus/directus/blob/fix-graphql-cache-requests/api/src/middleware/graphql.ts#L61
and here:
https://github.com/directus/directus/blob/fix-graphql-cache-requests/api/src/middleware/respond.ts#L30
👍

Edit:
Tested doing mutations and no new entries were added to Redis

@joselcvarela joselcvarela marked this pull request as ready for review September 15, 2022 18:42
@licitdev licitdev added this to the Next Release milestone Sep 16, 2022
@rijkvanzanten rijkvanzanten merged commit 54cd2d4 into main Sep 16, 2022
@rijkvanzanten rijkvanzanten deleted the fix-graphql-cache-requests branch September 16, 2022 13:12
qborisb pushed a commit to qborisb/directus that referenced this pull request Sep 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants