feat: Allow Redis client to be injected #5034

adamnoakes · 2021-03-15T14:19:22Z

Allow Redis client to be injected by using the BaseRedisCache class which contains the existing implementation logic without the client initialisation.
Removes duplication between RedisCache and RedisClusterCache
Fixes a bug where the result of delete is returned as a number, rather than a boolean. (Our TypeScript types have always declared that this function returns a boolean as KeyValueCache.delete should, but the actual implementation returned a number. This was not noticed because of the use of an any type.)

In my opinion, BaseRedisCache should really be RedisCache and the existing RedisCache + RedisCacheCluster should either be renamed to identify that they are using the ioredis client, or removed and left for the consumer of the library to implement/ inject the client using this method. But this would obviously be a breaking change so maybe one to think about for the future?

I've created an interface for the RedisClient rather than rely on the ioredis type as i do not believe apollo should be tied to the ioredis interface.

This PR solves the same issue as #4871 in a slightly different way.
It will solve these issues:

I welcome any feedback/ requested changes.

apollo-cla · 2021-03-15T14:19:24Z

@adamnoakes: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

adamnoakes · 2021-03-15T14:37:12Z

@glasser

glasser · 2021-03-15T18:23:10Z

Thanks, this is on my list for my next PR review day.

adamnoakes · 2021-03-15T19:30:32Z

Np, no rush 👍

# Conflicts: # packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts

glasser

Generally looking good but the PR needs to be updated to avoid mget with Redis.Cluster.

glasser · 2021-03-25T21:34:05Z

packages/apollo-server-cache-redis/src/BaseRedisCache.ts

+  }
+
+  async delete(key: string): Promise<boolean> {
+    return await this.client.del(key) > 0;


Good catch. I've expanded your description of this in the PR description.

glasser · 2021-03-25T21:34:26Z

packages/apollo-server-cache-redis/src/RedisClusterCache.ts


-export class RedisClusterCache implements KeyValueCache {
+export class RedisClusterCache extends BaseRedisCache {
  readonly client: any;


Huh, this is basically redefining the superclass field's type to be any. But what the del/delete issues shows is that we should avoid this. I've pushed a commit which should fix this. Note that I'm using the ioredis type directly instead of defining an interface like you did in BaseRedisCache, which seems fine because it's not part of our package's API.

glasser · 2021-03-25T21:50:22Z

packages/apollo-server-cache-redis/src/RedisClusterCache.ts

-
-    this.loader = new DataLoader(
-      (keys = []) =>
-        Promise.all(keys.map(key => client.get(key).catch(() => null))),


So RedisClusterCache didn't use mget and this appears to have been a conscious choice when RedisClusterCache was added. Various issues such as redis/ioredis#811 suggest this is still a concern in newer versions of ioredis.

So we need to support the non-mget path (perhaps subclasses can provide a protected useMget() method?)

I've committed a potential solution, that fixes it in the injected client rather than adding logic to work around it in BaseRedisClient class, let me know what you think.

On second thought after merging this means you can't actually create your own Redis.Cluster object, so I'd rather move the logic back into the base class.

To me it would make sense if you wanted to create your own cluster cache to use the base class, the logic in the current RedisClusterCache seems mainly to work around ioredis, if you were using node-redis with the redis-clustr wrapper for example that already has logic to split multi-key commands. Having the base class allows apollo to just focus on core redis logic and let the consumer of the library handle anything implementation specific.

@adamnoakes Makes sense. I was trying to write the docs for #5088 to show examples and I thought it would be nice to be "all in" on the new class rather than showing the older pass-through implementations, but you can't really do that with Redis.Cluster. Thus the API change in #5088.

But I see what you mean about other implementations that do support mget and cluster.

What if I kept the structure from #5088 but changed the names to be client and getOnlyClient or noMgetClient or something?

Hmm yeah i see, could do it via a flag? like useMGet that defaults to true. Would be cleaner imo than getOnlyClient/ noMgetClient as the client will likely still have mget (if they are just passing cluster ioredis client in as is for example).

On the topic of going all in on the new class. Would be nice to rename it to just RedisCache when you do the next major/ breaking change release and remove the existing RedisCache/ RedisClusterCache classes as i think they are redundant with this approach.

The flag seems like a nice idea, but I do like the type checking we get by providing two different arguments. I am going to go with noMgetClient though to make it clear that it's about what the client supports, not whether you're in a cluster. See 21f148e

Tracking the major version bump suggestion at #5099

Sounds good, thank you for your help on getting this merged.

On the flag type checking, I was curious so had a play and looks like you could have a flag and type checking using overloads.

tsplayground

interface GetClient { get: () => void; } interface MGetClient { mget: () => void; } function test(client: MGetClient, options?: { useMGet: true }): void; function test(client: GetClient, options: { useMGet: false }): void; function test(client: GetClient | MGetClient, options: { useMGet: boolean } = { useMGet: true }): void { return undefined; } test({ mget: () => {}}); // ok test({ get: () => {}}, { useMGet: false }); // ok test({ mget: () => {}}, { useMGet: true }); // ok test({ get: () => {}}); // type error test({ get: () => {}}, { useMGet: true }); // type error test({ mget: () => {}}, { useMGet: false }); // type error

nod yeah, that definitely works, but I think I'm going to go with this way (which provides type checking even if you're being more dynamic, whereas yours only provides full type checking if you're using literal true or false).

glasser · 2021-04-06T20:49:12Z

packages/apollo-server-cache-redis/src/RedisClusterCache.ts

+      del: clusterClient.del.bind(clusterClient),
+      flushdb: clusterClient.flushdb.bind(clusterClient),
+      mget(...keys: Array<string>): Promise<Array<string | null>> {
+        return Promise.all(keys.map(key => clusterClient.get(key).catch(() => null)))


I'm raising a pretty big eyebrow at that "ignore all errors" catch, but I see that it's copied from the previous version, so not gonna worry about it.

glasser · 2021-04-06T20:51:53Z

Thanks! I'll add the changelog entry on main.

Add docs and changelog. As part of doing this, realize that the implementation in #5034 makes it annoying to inject a cluster client, so switch the new API to decide about get vs mget based on which kind of client is passed to the base class.

glasser · 2021-04-09T00:05:04Z

I've released a prerelease with this fix ([email protected], part of the Apollo Server 2.23.0 release). Try out the alpha and see if it works for you! Please provide any feedback in #5094.

glasser · 2021-04-14T23:24:55Z

This has been released in [email protected].

Allow Redis client to be injected

9c1a1b7

adamnoakes and others added 2 commits March 23, 2021 12:26

Merge branch 'main' into an/injectable_redis_client

ed4d466

# Conflicts: # packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts

Make nodes call typesafer

06922f2

glasser requested changes Mar 25, 2021

View reviewed changes

override mget for cluster

5b3812d

adamnoakes requested a review from glasser April 4, 2021 16:36

glasser approved these changes Apr 6, 2021

View reviewed changes

glasser merged commit 1a7f470 into apollographql:main Apr 6, 2021

glasser mentioned this pull request Apr 6, 2021

apollo-server-cache-redis: follow-up to #5034 #5088

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow Redis client to be injected #5034

feat: Allow Redis client to be injected #5034

adamnoakes commented Mar 15, 2021 •

edited by glasser

Loading

apollo-cla commented Mar 15, 2021

adamnoakes commented Mar 15, 2021

glasser commented Mar 15, 2021

adamnoakes commented Mar 15, 2021

glasser left a comment

glasser Mar 25, 2021

glasser Mar 25, 2021

glasser Mar 25, 2021

adamnoakes Mar 26, 2021

glasser Apr 6, 2021

adamnoakes Apr 7, 2021

glasser Apr 7, 2021

adamnoakes Apr 9, 2021

glasser Apr 9, 2021

adamnoakes Apr 10, 2021

glasser Apr 14, 2021

glasser Apr 6, 2021

glasser commented Apr 6, 2021

glasser commented Apr 9, 2021

glasser commented Apr 14, 2021

feat: Allow Redis client to be injected #5034

feat: Allow Redis client to be injected #5034

Conversation

adamnoakes commented Mar 15, 2021 • edited by glasser Loading

apollo-cla commented Mar 15, 2021

adamnoakes commented Mar 15, 2021

glasser commented Mar 15, 2021

adamnoakes commented Mar 15, 2021

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Apr 6, 2021

glasser commented Apr 9, 2021

glasser commented Apr 14, 2021

adamnoakes commented Mar 15, 2021 •

edited by glasser

Loading