graphql-yoga icon indicating copy to clipboard operation
graphql-yoga copied to clipboard

Yoga/Envelop Response cache caches even when the result contains non-cacheable type

Open wodCZ opened this issue 2 years ago • 5 comments

Describe the bug

When using either yoga or envelop response cache plugin with:

  • global ttl set to 0
  • ttlPerType for an object type First set to 3600_000

Having a query that queries for both first and second, the whole result is cached, even though second didn't have an explicit ttlPerType defined - so should have the default ttl of 0, thus the response shouldn't be cached (or only partially, for the first result).

Your Example Website or App

https://codesandbox.io/p/sandbox/charming-yalow-q689gx-q689gx

Steps to Reproduce the Bug or Issue

Run the default query from the codesandbox.

Expected behavior

The query should not be cached, i.e. should return

    "responseCache": {
      "hit": false,
      "didCache": false
    }

instead, the result is cached.

Notice that if we explicitly add Second: 0 to ttlPerType, the query is not cached, as expected.

Screenshots or Videos

No response

Platform

  • OS: macOS
  • NodeJS: 21.2.0, but any supported, really
  • @graphql-yoga/plugin-response-cache: 3.1.0

Additional context

No response

wodCZ avatar Nov 16 '23 16:11 wodCZ

Hi, thank you for taking time to fill the issue.

This is actually the expected behaviour. The strategy of this plugin is to maximise caching, which means that it will cache if at least on entity is cachable in the schema.

If an entity or a field should never be cached, you can disable it per type or field with an explicit maxAge of 0.

The global ttl configuration is used when maxAge was found in the query, as a fallback. Which allow to globally cache without setting the maxAge everywhere.

I'm closing this issue since the behaviour describe is the expected one. If you have more questions, please feel free to comment or open a new issue.

EmrysMyrddin avatar Nov 20 '23 12:11 EmrysMyrddin

Hi @EmrysMyrddin, thank you for the response!

Sorry for marking the issue as a bug, I understand it could feel uncomfortable given that it is working as intended.

First, I'd like to suggest a documentation update, so this behaviour is obvious from the example and cannot be confused again. Looking at https://the-guild.dev/graphql/yoga-server/docs/features/response-caching#time-to-live-ttl


Time to Live (TTL)

It is possible to give cached operations a time to live. Either globally, based on schema coordinates or object types.

If a query operation result contains multiple objects of the same or different types, the lowest TTL is picked.

Using plugin configuration

useResponseCache({
  session: () => null,
  // by default cache all operations for 2 seconds
  ttl: 2_000,
  ttlPerType: {
    // only cache query operations containing User for 500ms
    User: 500
  },
  ttlPerSchemaCoordinate: {
    // cache operations selecting Query.lazy for 10 seconds
    'Query.lazy': 10_000
  }
})

I suggest adding the following


...

Note that the strategy of this plugin is to maximise caching, which means that it will cache if at least on entity is cachable in the schema.

The global ttl configuration is used when maxAge was not found in the query, as a fallback. Which allow to globally cache without setting the maxAge everywhere.

For example, given the following configuration:

useResponseCache({
  session: () => null,
  // when the result does not include any cachable entity, do not cache the result
  ttl: 0,
  ttlPerType: {
    // when the result contains user, allow the whole result to be cached for 500ms
    User: 500
  },
  ttlPerSchemaCoordinate: {
    // cache result that include Query.lazy for 10 seconds
    'Query.lazy': 10_000
  }
})

If an entity or a field should never be cached, you can disable it per type or field with an explicit maxAge of 0.


I think this would avoid my confusion and eventually save time to other users migrating from a different driver.

I'm personally migrating from mercurius & their cache, which works a bit differently - it does not cache the whole result, just the nodes (i.e. would return cached first and re-execute resolver for second in my example).

Now, I like the yoga ecosystem, but I need to find somewhat gradual migration plan. We have a running ecommerce system with tens of thousands api requests daily and cache is crucial for us. Unfortunately, the expected behaviour means that I'd have to:

  1. locate all existing types and explicitly list them with maxAge of 0 (~around 100 of them)
  2. implement a mechanism that performs a check that every type has maxAge defined — since we can not afford that any result gets leaked by accident with the maximise caching strategy.
  3. raise maxAge for ~10 types we cache currently

I hope you understand my issue now. So my question would be, is there a way to flip the behaviour to only cache when all types have explicit maxAge? Is there maybe a different caching plugin that works at object type level, instead of response level? Any other approach you could recommend, please?

Thank you!

wodCZ avatar Nov 20 '23 18:11 wodCZ

Yes, the doc should be better on that point, given that this is also the behavior of the response cache of Apollo.

Let me reopen this as a feature request and documentation issue.

What we can propose as an alternative is to let you pass a function in ttlForType and ttlForSchemaCoordinate instead of an object. This would let you customise more freely the behaviour of the TTL.

This way, you could probably do something like this:

const _ttlPerType = {
  User: 500
}
function ttlPerType(typeName: string) {
  return _ttlPerType[typeName] ?? 0;
}

Do you think this would suite your needs ?

EmrysMyrddin avatar Nov 21 '23 20:11 EmrysMyrddin

Thanks for getting back to me and re-classifying the issue.

Yes, I think the function would help me achieve my goal. Probably this could be achieved with the existing code by using proxy object and providing a get handler?

const _ttlPerType = {
  User: 500
};
const ttlPerType = new Proxy({}, {
    get(target, prop, receiver) {
       return _ttlPerType[prop] ?? 0;
    }
});

I haven't tested yet.

wodCZ avatar Nov 24 '23 16:11 wodCZ

Yes, to be honest that was my idea 😄 But after some discussion, we agreed that it would be more clear and flexible to provide a clean API for this :-)

But that should do the trick if you want a work around waiting this to be implemented.

EmrysMyrddin avatar Dec 05 '23 09:12 EmrysMyrddin