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

feat: Allow Redis client to be injected #5034

Merged
merged 4 commits into from
Apr 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions packages/apollo-server-cache-redis/src/BaseRedisCache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import {
TestableKeyValueCache,
KeyValueCacheSetOptions,
} from 'apollo-server-caching';
import DataLoader from 'dataloader';

export interface RedisClient {
set: (key: string, value: string, option?: string, optionValue?: number) => Promise<any>
mget: (...key: Array<string>) => Promise<Array<string | null>>
flushdb: () => Promise<any>
del: (key: string) => Promise<number>
quit: () => Promise<any>
}

export class BaseRedisCache implements TestableKeyValueCache<string> {
readonly client: RedisClient;
readonly defaultSetOptions: KeyValueCacheSetOptions = {
ttl: 300,
};

private loader: DataLoader<string, string | null>;

constructor(client: RedisClient) {
this.client = client;

this.loader = new DataLoader(keys => client.mget(...keys), {
cache: false,
});
}

async set(
key: string,
value: string,
options?: KeyValueCacheSetOptions,
): Promise<void> {
const { ttl } = Object.assign({}, this.defaultSetOptions, options);
if (typeof ttl === 'number') {
await this.client.set(key, value, 'EX', ttl);
} else {
// We'll leave out the EXpiration when no value is specified. Of course,
// it may be purged from the cache for other reasons as deemed necessary.
await this.client.set(key, value);
}
}

async get(key: string): Promise<string | undefined> {
const reply = await this.loader.load(key);
if (reply !== null) {
return reply;
}
return;
}

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

Choose a reason for hiding this comment

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

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

}

// Drops all data from Redis. This should only be used by test suites ---
// production code should never drop all data from an end user Redis cache!
async flush(): Promise<void> {
await this.client.flushdb();
}

async close(): Promise<void> {
await this.client.quit();
return;
}
}
60 changes: 3 additions & 57 deletions packages/apollo-server-cache-redis/src/RedisCache.ts
Original file line number Diff line number Diff line change
@@ -1,62 +1,8 @@
import {
TestableKeyValueCache,
KeyValueCacheSetOptions,
} from 'apollo-server-caching';
import Redis, { RedisOptions } from 'ioredis';
import DataLoader from 'dataloader';

export class RedisCache implements TestableKeyValueCache<string> {
readonly client: any;
readonly defaultSetOptions: KeyValueCacheSetOptions = {
ttl: 300,
};

private loader: DataLoader<string, string | null>;
import { BaseRedisCache } from './BaseRedisCache';

export class RedisCache extends BaseRedisCache {
constructor(options?: RedisOptions) {
const client = new Redis(options);
this.client = client;

this.loader = new DataLoader(keys => client.mget(...keys), {
cache: false,
});
}

async set(
key: string,
value: string,
options?: KeyValueCacheSetOptions,
): Promise<void> {
const { ttl } = Object.assign({}, this.defaultSetOptions, options);
if (typeof ttl === 'number') {
await this.client.set(key, value, 'EX', ttl);
} else {
// We'll leave out the EXpiration when no value is specified. Of course,
// it may be purged from the cache for other reasons as deemed necessary.
await this.client.set(key, value);
}
}

async get(key: string): Promise<string | undefined> {
const reply = await this.loader.load(key);
if (reply !== null) {
return reply;
}
return;
}

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

// Drops all data from Redis. This should only be used by test suites ---
// production code should never drop all data from an end user Redis cache!
async flush(): Promise<void> {
await this.client.flushdb();
}

async close(): Promise<void> {
await this.client.quit();
return;
super(new Redis(options));
}
}
65 changes: 15 additions & 50 deletions packages/apollo-server-cache-redis/src/RedisClusterCache.ts
Original file line number Diff line number Diff line change
@@ -1,64 +1,29 @@
import { KeyValueCache, KeyValueCacheSetOptions } from 'apollo-server-caching';
import Redis, {
ClusterOptions,
ClusterNode,
Redis as RedisInstance,
} from 'ioredis';
import DataLoader from 'dataloader';
import { BaseRedisCache } from './BaseRedisCache';

export class RedisClusterCache implements KeyValueCache {
readonly client: any;
readonly defaultSetOptions: KeyValueCacheSetOptions = {
ttl: 300,
};

private loader: DataLoader<string, string | null>;
export class RedisClusterCache extends BaseRedisCache {
private readonly clusterClient: Redis.Cluster;

constructor(nodes: ClusterNode[], options?: ClusterOptions) {
const client = this.client = new Redis.Cluster(nodes, options);

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

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

{ cache: false },
);
}

async set(
key: string,
data: string,
options?: KeyValueCacheSetOptions,
): Promise<void> {
const { ttl } = Object.assign({}, this.defaultSetOptions, options);
if (typeof ttl === 'number') {
await this.client.set(key, data, 'EX', ttl);
} else {
// We'll leave out the EXpiration when no value is specified. Of course,
// it may be purged from the cache for other reasons as deemed necessary.
await this.client.set(key, data);
}
}

async get(key: string): Promise<string | undefined> {
const reply = await this.loader.load(key);
// reply is null if key is not found
if (reply !== null) {
return reply;
}
return;
}

async delete(key: string): Promise<boolean> {
return await this.client.del(key);
const clusterClient = new Redis.Cluster(nodes, options)
super({
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)))
Copy link
Member

Choose a reason for hiding this comment

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

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.

},
quit: clusterClient.quit.bind(clusterClient),
set: clusterClient.set.bind(clusterClient),
});
this.clusterClient = clusterClient;
}

async flush(): Promise<void> {
const masters = this.client.nodes('master') || [];
const masters = this.clusterClient.nodes('master') || [];
await Promise.all(masters.map((node: RedisInstance) => node.flushdb()));
}

async close(): Promise<void> {
await this.client.quit();
return;
}
}
3 changes: 2 additions & 1 deletion packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ class Redis {
private timeouts = new Set<NodeJS.Timer>();

async del(key: string) {
const keysDeleted = this.keyValue.hasOwnProperty(key) ? 1 : 0;
delete this.keyValue[key];
return true;
return keysDeleted;
}

async get(key: string) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
jest.mock('ioredis');

import { BaseRedisCache, RedisClient } from '../index';
import {
testKeyValueCache_Basics,
testKeyValueCache_Expiration,
} from '../../../apollo-server-caching/src/__tests__/testsuite';


describe('BaseRedisCacheTest', () => {
const store: { [key: string]: string } = {};
const testRedisClient: RedisClient = {
set: jest.fn((key: string, value: string, option?: string, ttl?: number) => {
store[key] = value;
option === 'EX' && ttl && setTimeout(() => delete store[key], ttl * 1000);
return Promise.resolve();
}),
mget: jest.fn((...keys) => Promise.resolve(keys.map((key: string) => store[key]))),
flushdb: jest.fn(() => Promise.resolve()),
del: jest.fn((key: string) => {
const keysDeleted = store.hasOwnProperty(key) ? 1 : 0;
delete store[key];
return Promise.resolve(keysDeleted);
}),
quit: jest.fn(() => Promise.resolve()),
}

const cache = new BaseRedisCache(testRedisClient);
testKeyValueCache_Basics(cache);
testKeyValueCache_Expiration(cache);
});
1 change: 1 addition & 0 deletions packages/apollo-server-cache-redis/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export { RedisCache } from './RedisCache';
export { RedisClusterCache } from './RedisClusterCache';
export { RedisClient, BaseRedisCache } from './BaseRedisCache';