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

Allow infinite TTL (time-to-live) on KeyValueCache's set. #3623

Merged
merged 9 commits into from
Dec 27, 2019
17 changes: 13 additions & 4 deletions packages/apollo-server-cache-memcached/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { TestableKeyValueCache } from 'apollo-server-caching';
import {
TestableKeyValueCache,
KeyValueCacheSetOptions,
} from 'apollo-server-caching';
import Memcached from 'memcached';
import { promisify } from 'util';

export class MemcachedCache implements TestableKeyValueCache {
// FIXME: Replace any with proper promisified type
readonly client: any;
readonly defaultSetOptions = {
readonly defaultSetOptions: KeyValueCacheSetOptions = {
ttl: 300,
};

Expand All @@ -23,10 +26,16 @@ export class MemcachedCache implements TestableKeyValueCache {
async set(
key: string,
value: string,
options?: { ttl?: number },
options?: KeyValueCacheSetOptions,
): Promise<void> {
const { ttl } = Object.assign({}, this.defaultSetOptions, options);
await this.client.set(key, value, ttl);
if (typeof ttl === 'number') {
await this.client.set(key, value, ttl);
} else {
// In Memcached, zero indicates no specific expiration time. Of course,
// it may be purged from the cache for other reasons as deemed necessary.
await this.client.set(key, value, 0);
}
}

async get(key: string): Promise<string | undefined> {
Expand Down
17 changes: 13 additions & 4 deletions packages/apollo-server-cache-redis/src/RedisCache.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { TestableKeyValueCache } from 'apollo-server-caching';
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 = {
readonly defaultSetOptions: KeyValueCacheSetOptions = {
ttl: 300,
};

Expand All @@ -22,10 +25,16 @@ export class RedisCache implements TestableKeyValueCache<string> {
async set(
key: string,
value: string,
options?: { ttl?: number },
options?: KeyValueCacheSetOptions,
): Promise<void> {
const { ttl } = Object.assign({}, this.defaultSetOptions, options);
await this.client.set(key, value, 'EX', ttl);
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> {
Expand Down
14 changes: 10 additions & 4 deletions packages/apollo-server-cache-redis/src/RedisClusterCache.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { KeyValueCache } from 'apollo-server-caching';
import { KeyValueCache, KeyValueCacheSetOptions } from 'apollo-server-caching';
import Redis, {
ClusterOptions,
ClusterNode,
Expand All @@ -8,7 +8,7 @@ import DataLoader from 'dataloader';

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

Expand All @@ -27,10 +27,16 @@ export class RedisClusterCache implements KeyValueCache {
async set(
key: string,
data: string,
options?: { ttl?: number },
options?: KeyValueCacheSetOptions,
): Promise<void> {
const { ttl } = Object.assign({}, this.defaultSetOptions, options);
await this.client.set(key, data, 'EX', ttl);
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> {
Expand Down
8 changes: 5 additions & 3 deletions packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ const setKey = (key, value, type, ttl) => {
value,
ttl,
};
setTimeout(() => {
delete keyValue[key];
}, ttl * 1000);
if (ttl) {
setTimeout(() => {
delete keyValue[key];
}, ttl * 1000);
}
return Promise.resolve(true);
};

Expand Down
4 changes: 2 additions & 2 deletions packages/apollo-server-caching/src/KeyValueCache.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
/** Options for {@link KeyValueCache.set} */
interface KeyValueCacheSetOptions {
export interface KeyValueCacheSetOptions {
/**
* Specified in **seconds**, the time-to-live (TTL) value limits the lifespan
* of the data being stored in the cache.
*/
ttl?: number
ttl?: number | null
};

export interface KeyValueCache<V = string> {
Expand Down
4 changes: 2 additions & 2 deletions packages/apollo-server-caching/src/PrefixingKeyValueCache.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { KeyValueCache } from './KeyValueCache';
import { KeyValueCache, KeyValueCacheSetOptions } from './KeyValueCache';

// PrefixingKeyValueCache wraps another cache and adds a prefix to all keys used
// by all operations. This allows multiple features to share the same
Expand All @@ -16,7 +16,7 @@ export class PrefixingKeyValueCache<V = string> implements KeyValueCache<V> {
get(key: string) {
return this.wrapped.get(this.prefix + key);
}
set(key: string, value: V, options?: { ttl?: number }) {
set(key: string, value: V, options?: KeyValueCacheSetOptions) {
return this.wrapped.set(this.prefix + key, value, options);
}
delete(key: string) {
Expand Down
11 changes: 11 additions & 0 deletions packages/apollo-server-caching/src/__tests__/testsuite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@ export function testKeyValueCache_Expiration(
expect(await keyValueCache.get('short')).toBeUndefined();
expect(await keyValueCache.get('long')).toBeUndefined();
});

it('does not expire when ttl is null', async () => {
await keyValueCache.set('forever', 'yours', { ttl: null });
expect(await keyValueCache.get('forever')).toBe('yours');
advanceTimeBy(1500);
jest.advanceTimersByTime(1500);
expect(await keyValueCache.get('forever')).toBe('yours');
advanceTimeBy(4000);
jest.advanceTimersByTime(4000);
expect(await keyValueCache.get('forever')).toBe('yours');
});
});
}

Expand Down
6 changes: 5 additions & 1 deletion packages/apollo-server-caching/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export { KeyValueCache, TestableKeyValueCache } from './KeyValueCache';
export {
KeyValueCache,
TestableKeyValueCache,
KeyValueCacheSetOptions,
} from './KeyValueCache';
export { InMemoryLRUCache } from './InMemoryLRUCache';
export { PrefixingKeyValueCache } from './PrefixingKeyValueCache';
13 changes: 7 additions & 6 deletions packages/apollo-server-core/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,14 @@ export class ApolloServerBase {
}

if (requestOptions.persistedQueries !== false) {
const {
cache: apqCache = requestOptions.cache!,
...apqOtherOptions
} = requestOptions.persistedQueries || Object.create(null);

requestOptions.persistedQueries = {
cache: new PrefixingKeyValueCache(
(requestOptions.persistedQueries &&
requestOptions.persistedQueries.cache) ||
requestOptions.cache!,
APQ_CACHE_PREFIX,
),
cache: new PrefixingKeyValueCache(apqCache, APQ_CACHE_PREFIX),
...apqOtherOptions,
};
} else {
// the user does not want to use persisted queries, so we remove the field
Expand Down
8 changes: 8 additions & 0 deletions packages/apollo-server-core/src/graphqlOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ export type DataSources<TContext> = {

export interface PersistedQueryOptions {
cache: KeyValueCache;
/**
* Specified in **seconds**, this time-to-live (TTL) value limits the lifespan
* of how long the persisted query should be cached. To specify a desired
* lifespan of "infinite", set this to `null`, in which case the eviction will
* be determined by the cache's eviction policy, but the record will never
* simply expire.
*/
ttlSeconds?: number | null;
Copy link
Member Author

Choose a reason for hiding this comment

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

@trevor-scheer Can you try to convince me to change this parameter to be ttl, rather than ttlSeconds?

Considerations:

  • Against: It's come up a number of times before that parameters that define units of time could be more clear by specifying their precision as part of the name. (This is the only reason I actually put Seconds in the name.)
  • For: The existing interfaces which this translate to are still ttl, for better or for worse, but that's what they are.
  • For: It's not "seconds" when set to null, so ttlSeconds: null is a bit peculiar.
    • Along the same lines, this PR changes those existing interfaces to also support null.
  • For: While including the precision makes sense in many cases, TTLs are very often (e.g. Memcached, Redis, HTTP Cache-control, and so on). On the other hand, lru-cache sets its maxAge in milliseconds. To be honest, I find such flexibility unnecessary. Is it truly wise to cache things with sub-second precision?
  • For: Thanks to the TSDoc annotation provided directly above this, users with editors that support it are automatic given IntelliSense clarity that this in seconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, anyone should feel free to chime in on this naming puzzle.

}

export default GraphQLServerOptions;
Expand Down
15 changes: 12 additions & 3 deletions packages/apollo-server-core/src/requestPipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,9 +330,18 @@ export async function processGraphQLRequest<TContext>(
// an error) and not actually write, we'll write to the cache if it was
// determined earlier in the request pipeline that we should do so.
if (metrics.persistedQueryRegister && persistedQueryCache) {
Promise.resolve(persistedQueryCache.set(queryHash, query)).catch(
console.warn,
);
Promise.resolve(
persistedQueryCache.set(
queryHash,
query,
config.persistedQueries &&
typeof config.persistedQueries.ttlSeconds !== 'undefined'
? {
ttl: config.persistedQueries.ttlSeconds,
}
: Object.create(null),
),
).catch(console.warn);
}

let response: GraphQLResponse | null = await dispatcher.invokeHooksUntilNonNull(
Expand Down
73 changes: 67 additions & 6 deletions packages/apollo-server-integration-testsuite/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1226,16 +1226,20 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
Parameters<GraphQLRequestListener['didEncounterErrors']>
>;

beforeEach(async () => {
function createMockCache() {
const map = new Map<string, string>();
const cache = {
set: async (key, val) => {
return {
set: jest.fn(async (key, val) => {
await map.set(key, val);
},
get: async key => map.get(key),
delete: async key => map.delete(key),
}),
get: jest.fn(async key => map.get(key)),
delete: jest.fn(async key => map.delete(key)),
};
}

beforeEach(async () => {
didEncounterErrors = jest.fn();
const cache = createMockCache();
app = await createApp({
graphqlOptions: {
schema,
Expand All @@ -1253,6 +1257,63 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
});
});

it('when ttlSeconds is set, passes ttl to the apq cache set call', async () => {
const cache = createMockCache();
app = await createApp({
graphqlOptions: {
schema,
persistedQueries: {
cache: cache,
ttlSeconds: 900,
},
},
});

await request(app)
.post('/graphql')
.send({
extensions,
query,
});

expect(cache.set).toHaveBeenCalledWith(
expect.stringMatching(/^apq:/),
'{testString}',
expect.objectContaining({
ttl: 900,
}),
);
});

it('when ttlSeconds is unset, ttl is not passed to apq cache',
async () => {
const cache = createMockCache();
app = await createApp({
graphqlOptions: {
schema,
persistedQueries: {
cache: cache,
},
},
});

await request(app)
.post('/graphql')
.send({
extensions,
query,
});

expect(cache.set).toHaveBeenCalledWith(
expect.stringMatching(/^apq:/),
'{testString}',
expect.not.objectContaining({
ttl: 900,
}),
);
}
);

it('errors when version is not specified', async () => {
const result = await request(app)
.get('/graphql')
Expand Down