apollo-server-plugin-response-cache: shouldReadFromCache and shouldWriteToCache are declared as returning ValueOrPromise but never await

[u007]

my sample code uses strapi:

module.exports = {
  "endpoint": "/graphql",
  "shadowCRUD": true,
  "playgroundAlways": false,
  "depthLimit": 7,
  "amountLimit": 200,
  "shareEnabled": false,
  federation: false,
  apolloServer: {
    tracing: 'production' !== strapi.config.environment ? true : false,
    // persistedQueries: { ttl: 10 * MAX_AGE }, // we set this to be a factor of 10, somewhat arbitrary
    // cacheControl: { defaultMaxAge: MAX_AGE },
    plugins: [
      apolloServerPluginResponseCache({
        shouldReadFromCache,
        shouldWriteToCache,
        extraCacheKeyData,
        sessionId,
      }),
      injectCacheControl()
    ]
  },
  redisCacheClient: cache
}

// does not work all the time
async function shouldReadFromCache(requestContext) {
  if ('IntrospectionQuery' == requestContext.operationName) return false;// graphql ping ignore
  // return true;
  if (cacheQueryNames.includes(requestContext.operationName)) {
    const expired = await cacheExpired(requestContext);
    console.debug("shouldReadFromCache:", requestContext.operationName, 'expired?', expired);
    return !expired;
  }
  console.debug("readCache nocache:", requestContext.operationName);
  return false;
}

[centogram]

I am using this plugin with strapi as well shouldReadFromCache appears to be ignored entrely. Cache is always returned even when I return false.

[glasser]

Good catch. These hooks are declared as returning ValueOrPromise<boolean> but as you said, neither of them actually are awaited when called. That means that when you write an async hook, it always returns a Promise, which the calling code always considers to be truthy.

This has been broken since this initial implementation. I don't think anyone could be relying on this behavior, since it's equivalent to not passing the hooks at all (other than the fact that having the hooks gives you the ability to execute code at a certain place in the lifecycle). Both functions are called inside async functions (though admittedly in one case there's no other await in the function) so adding awaits should be straightforward.

Would one of you like to send in a PR? Ideally you can test that it works too. There is a test in apollo-server-integration-test-suite of these options, so you could make a little test that validates that the async versions work too. CC me on it and I'll review!

[macrozone]

damn, just ran into this problem.

I hope @centogram 's Pull request will be merged and released soon.

edit: sorry, did not want to sound impatient and harsh. Keep up the nice work!

patch-package saved me in the meantime ;)

[glasser]

Fixed by @centogram and @u007 in #4890! I intend to release AS 2.20 within the next week (unless a potential family issue comes up — but definitely within the next few "@glasser business days" :) )

[u007]

@glasser thank you, stay safe :)

[glasser]

Released in v2.20.0.

[u007]

cool thank you :)