Rest Datasource Never hits the Cache

[no1melman]

So according to the lines below, once a fetch is made it is stored permanently into this memoization object, therefore the HttpCache is never hit again....

This is bad, because we're not using all the cache logic in the HttpCache and respecting cache headers from requests etc... in fact this piece of code makes that completely and utterly redundant.

apollo-server/packages/apollo-datasource-rest/src/RESTDataSource.ts

Lines 268 to 278 in bbae424

	if (request.method === 'GET') {
	let promise = this.memoizedResults.get(cacheKey);
	if (promise) return promise;
	promise = performRequest();
	this.memoizedResults.set(cacheKey, promise);
	return promise;
	} else {
	this.memoizedResults.delete(cacheKey);
	return performRequest();
	}

One assumes that we should just remove it - I'm happy to make the change - just want to confirm my findings are right

[no1melman]

I've read this blog

It points out that the memoization is for request deduping. Which is fine, but one would have thought that once the promise had resolved (and therefore in the cache) that you would remove the memoize and just hit the cache. That would stop multiple calls being made to the backend if one had already been made...

something like

if (promise && !promise.IsComplete) return promise;

Obviously there is no IsComplete property, so that would need looking into - maybe this just screams of being pushed down into the lower abstractions in the HTTPCache.

To me this doesn't change the problem, memoization is prevent cache hits, but I get why we are memoizing it, but I would still consider this not working correctly.

[soerenuhrbach]

TL;DR Make sure your data source is constructed for each graphql request and not a singleton

---

I struggled with memoization too and thought it behaves incorrectly but it doesn't.

My project is based on graphql-modules thats why I registered my data source as a provider (which is a singleton by default).

const PlayerModule = new GraphQLModule({
  providers: [PlayerAPI],
  typeDefs,
  resolvers,
  context: context => context
});

In #1511 (comment) I found the cause of the misbehaviour:
The data sources are designed to be created for each request but here the data source is constructed once which causes that the memoizedResults-map stores the responses permanently.

So I configured my data source that it is recreated for every request.

@Injectable({
  scope: ProviderScope.Request
})
export class PlayerAPI extends RESTDataSource {
...

After that change the memoization behaves correctly.

---

I hope I could help.
This issue can be closed since the problem is caused by incorrect use.