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

Add support for cache tag invalidation #1169

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"files.trimTrailingWhitespace": true,
"files.insertFinalNewline": true,
"editor.wordWrapColumn": 110,
"editor.formatOnSave": true,
"prettier.singleQuote": true,
"prettier.printWidth": 110,
"files.exclude": {
Expand Down
59 changes: 59 additions & 0 deletions packages/apollo-server-caching/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
{
"name": "apollo-server-caching",
"version": "2.0.0-beta.7",
"author": "opensource@apollographql.com",
"license": "MIT",
"repository": {
"type": "git",
"url": "https://github.com/apollographql/apollo-server/tree/master/packages/apollo-server-caching"
},
"homepage": "https://github.com/apollographql/apollo-server#readme",
"bugs": {
"url": "https://github.com/apollographql/apollo-server/issues"
},
"scripts": {
"clean": "rm -rf lib",
"compile": "tsc",
"prepare": "npm run clean && npm run compile",
"test": "jest --verbose"
},
"main": "dist/index.js",
"types": "dist/index.d.ts",
"engines": {
"node": ">=8"
},
"dependencies": {
"http-cache-semantics": "^4.0.0",
"memcached": "^2.2.2",
"redis": "^2.8.0"
},
"devDependencies": {
"@types/jest": "^23.0.0",
"@types/memcached": "^2.2.5",
"@types/redis": "^2.8.6",
"jest": "^23.1.0",
"jest-each": "^23.1.0",
Copy link
Contributor

@martijnwalraven martijnwalraven Jun 13, 2018

Choose a reason for hiding this comment

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

I didn't know about jest-each, but that's a neat solution! it seems support for this has (very) recently become part of core Jest though, so maybe we don't even need the dependency (see README).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch!

"ts-jest": "^22.4.6"
},
"jest": {
"testEnvironment": "node",
"transform": {
"^.+\\.(ts|js)$": "ts-jest"
},
"moduleFileExtensions": [
"ts",
"js",
"json"
],
"testRegex": "/__tests__/.*$",
"setupFiles": [
"<rootDir>/src/polyfills/fetch.ts",
"<rootDir>/src/polyfills/url.ts"
],
"globals": {
"ts-jest": {
"skipBabel": true
}
}
}
}
6 changes: 6 additions & 0 deletions packages/apollo-server-caching/src/__mocks__/common.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export const servers = {
Copy link
Contributor

@martijnwalraven martijnwalraven Jun 13, 2018

Choose a reason for hiding this comment

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

Having to deal with external dependencies means these are not really unit tests and we probably don't want to run them as part of CI for example. I think they are great as integration tests, so I'd like to keep them (especially if we add cache tags on top of the basic features), but I'm not sure how we should organize them to separate them from the unit tests and what the best practices around integration testing are (@jbaxleyiii may have some ideas there).

A script to start a Memcached and Redis server ready for these tests would be useful I think. And maybe we could run it as part of a npm run test:integration command.

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 a lot less familiar with Redis (though I suspect it should be similarly easy) but Memcached should be incredibly easy to mock. In fact, a quick search turned up memcached-mock and redis-mock.

If it's at all feasible to use those, rather than implementing a script to start one (which would need to struggle with cross-platform network/process implementation details, not to mention installation), that seems preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed! these integration tests probably don't need to be run with CI, its just for me to validate that the dependencies on external libraries work as expected. I'm running the databases with docker, will try to set it up as a npm script!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abernix that's awesome! I will use proxyquire to mock memcached and node-redis for testing (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: turns out I cant get proxyquire to work with typescript. Using jest.mock instead and it works great!

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better :)

memcachedHost: process.env.MEMCACHED_HOST || 'localhost:11211',
redisHost: process.env.REDIS_HOST || 'localhost',
};

export const delay = ms => new Promise(res => setTimeout(res, ms));
32 changes: 32 additions & 0 deletions packages/apollo-server-caching/src/__mocks__/date.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
const RealDate = global.Date;

export function mockDate() {
global.Date = new Proxy(RealDate, handler);
}

export function unmockDate() {
global.Date = RealDate;
}

let now = Date.now();

export function advanceTimeBy(ms: number) {
now += ms;
}

const handler = {
construct(target, args) {
if (args.length === 0) {
return new Date(now);
} else {
return new target(...args);
}
},
get(target, propKey, receiver) {
if (propKey === 'now') {
return () => now;
} else {
return target[propKey];
}
},
};
51 changes: 51 additions & 0 deletions packages/apollo-server-caching/src/__mocks__/fetch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
declare global {
namespace NodeJS {
interface Global {
fetch: typeof fetch;
}
}
}

type Headers = { [name: string]: string };

interface FetchMock extends jest.Mock<typeof fetch> {
mockResponseOnce(data?: any, headers?: Headers, status?: number);
mockJSONResponseOnce(data?: object, headers?: Headers);
}

const fetchMock = jest.fn<typeof fetch>() as FetchMock;

fetchMock.mockResponseOnce = (
data?: BodyInit,
headers?: Headers,
status: number = 200,
) => {
return fetchMock.mockImplementationOnce(async () => {
return new Response(data, {
status,
headers,
});
});
};

fetchMock.mockJSONResponseOnce = (
data = {},
headers?: Headers,
status?: number,
) => {
return fetchMock.mockResponseOnce(
JSON.stringify(data),
Object.assign({ 'Content-Type': 'application/json' }, headers),
status,
);
};

export default fetchMock;

export function mockFetch() {
global.fetch = fetchMock;
}

export function unmockFetch() {
global.fetch = fetch;
}
32 changes: 32 additions & 0 deletions packages/apollo-server-caching/src/__tests__/connectors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { servers, delay } from '../__mocks__/common';
import MemcachedKeyValueCache from '../connectors/memcached';
import RedisKeyValueCache from '../connectors/redis';
import each from 'jest-each';

// run test suite against each implementation of KeyValueCache
each([
[new MemcachedKeyValueCache(servers.memcachedHost)],
[new RedisKeyValueCache({ host: servers.redisHost })],
]).describe('%s', keyValueCache => {
beforeEach(() => {
keyValueCache.flush();
});

afterAll(() => {
keyValueCache.close();
});

it('can do a basic get and set', async () => {
await keyValueCache.set('hello', 'world');
expect(await keyValueCache.get('hello')).toBe('world');
expect(await keyValueCache.get('missing')).not.toBeDefined();
});

it('is able to expire keys based on ttl', async () => {
await keyValueCache.set('short', 's', { ttl: 1 });
await keyValueCache.set('long', 'l', { ttl: 5 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make sure we can read these values right after they have been set and then ensure that both are invalidated after advancing the time past 5 seconds

await delay(1500);
Copy link
Contributor

Choose a reason for hiding this comment

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

Having to depend on actual time passing (as opposed to mocking timers) is another reason this isn't really suitable as a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it acceptable to leave out tests for key expiry?

Copy link
Contributor

@n1ru4l n1ru4l Jun 13, 2018

Choose a reason for hiding this comment

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

@clarencenpy This may blast the complexity of the tests, but I can recommend lolex for doing time stuff. You can see an example here: https://github.com/n1ru4l/react-time-ago/blob/29006c7a2080626ec4206560fe4ecf69c3ca079b/src/index.test.js#L52

Copy link
Contributor Author

@clarencenpy clarencenpy Jun 13, 2018

Choose a reason for hiding this comment

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

For sure! I will test it out with the mocked databases and it will be great if that plays well together! Thanks 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using @martijnwalraven 's mockDate() (for calls to global date) and jest.useFakeTimers() (for calls to setTimeout). Seems to work great with the mock libs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Yeah, as long as we're using mocks for the databases we don't need actual delays.

expect(await keyValueCache.get('short')).not.toBeDefined();
expect(await keyValueCache.get('long')).toBe('l');
});
});
Loading