-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Adding Caching Adapter, allows caching of _Role, _User and _SCHMEA (fixes #168) #1664
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
var CacheController = require('../src/Controllers/CacheController.js').default; | ||
|
||
describe('CacheController', function() { | ||
var FakeCacheAdapter; | ||
var FakeAppID = 'foo'; | ||
var KEY = 'hello'; | ||
|
||
beforeEach(() => { | ||
FakeCacheAdapter = { | ||
get: () => Promise.resolve(null), | ||
put: jasmine.createSpy('put'), | ||
del: jasmine.createSpy('del'), | ||
clear: jasmine.createSpy('clear') | ||
} | ||
|
||
spyOn(FakeCacheAdapter, 'get').and.callThrough(); | ||
}); | ||
|
||
|
||
it('should expose role and user caches', (done) => { | ||
var cache = new CacheController(FakeCacheAdapter, FakeAppID); | ||
|
||
expect(cache.role).not.toEqual(null); | ||
expect(cache.role.get).not.toEqual(null); | ||
expect(cache.user).not.toEqual(null); | ||
expect(cache.user.get).not.toEqual(null); | ||
|
||
done(); | ||
}); | ||
|
||
|
||
['role', 'user'].forEach((cacheName) => { | ||
it('should prefix ' + cacheName + ' cache', () => { | ||
var cache = new CacheController(FakeCacheAdapter, FakeAppID)[cacheName]; | ||
|
||
cache.put(KEY, 'world'); | ||
var firstPut = FakeCacheAdapter.put.calls.first(); | ||
expect(firstPut.args[0]).toEqual([FakeAppID, cacheName, KEY].join(':')); | ||
|
||
cache.get(KEY); | ||
var firstGet = FakeCacheAdapter.get.calls.first(); | ||
expect(firstGet.args[0]).toEqual([FakeAppID, cacheName, KEY].join(':')); | ||
|
||
cache.del(KEY); | ||
var firstDel = FakeCacheAdapter.del.calls.first(); | ||
expect(firstDel.args[0]).toEqual([FakeAppID, cacheName, KEY].join(':')); | ||
}); | ||
}); | ||
|
||
it('should clear the entire cache', () => { | ||
var cache = new CacheController(FakeCacheAdapter, FakeAppID); | ||
|
||
cache.clear(); | ||
expect(FakeCacheAdapter.clear.calls.count()).toEqual(1); | ||
|
||
cache.user.clear(); | ||
expect(FakeCacheAdapter.clear.calls.count()).toEqual(2); | ||
|
||
cache.role.clear(); | ||
expect(FakeCacheAdapter.clear.calls.count()).toEqual(3); | ||
}); | ||
|
||
it('should handle cache rejections', (done) => { | ||
|
||
FakeCacheAdapter.get = () => Promise.reject(); | ||
|
||
var cache = new CacheController(FakeCacheAdapter, FakeAppID); | ||
|
||
cache.get('foo').then(done, () => { | ||
fail('Promise should not be rejected.'); | ||
}); | ||
}); | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
const InMemoryCache = require('../src/Adapters/Cache/InMemoryCache').default; | ||
|
||
|
||
describe('InMemoryCache', function() { | ||
var BASE_TTL = { | ||
ttl: 10 | ||
}; | ||
var NO_EXPIRE_TTL = { | ||
ttl: NaN | ||
}; | ||
var KEY = 'hello'; | ||
var KEY_2 = KEY + '_2'; | ||
|
||
var VALUE = 'world'; | ||
|
||
|
||
function wait(sleep) { | ||
return new Promise(function(resolve, reject) { | ||
setTimeout(resolve, sleep); | ||
}) | ||
} | ||
|
||
it('should destroy a expire items in the cache', (done) => { | ||
var cache = new InMemoryCache(BASE_TTL); | ||
|
||
cache.put(KEY, VALUE); | ||
|
||
var value = cache.get(KEY); | ||
expect(value).toEqual(VALUE); | ||
|
||
wait(BASE_TTL.ttl * 5).then(() => { | ||
value = cache.get(KEY) | ||
expect(value).toEqual(null); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('should delete items', (done) => { | ||
var cache = new InMemoryCache(NO_EXPIRE_TTL); | ||
cache.put(KEY, VALUE); | ||
cache.put(KEY_2, VALUE); | ||
expect(cache.get(KEY)).toEqual(VALUE); | ||
expect(cache.get(KEY_2)).toEqual(VALUE); | ||
|
||
cache.del(KEY); | ||
expect(cache.get(KEY)).toEqual(null); | ||
expect(cache.get(KEY_2)).toEqual(VALUE); | ||
|
||
cache.del(KEY_2); | ||
expect(cache.get(KEY)).toEqual(null); | ||
expect(cache.get(KEY_2)).toEqual(null); | ||
done(); | ||
}); | ||
|
||
it('should clear all items', (done) => { | ||
var cache = new InMemoryCache(NO_EXPIRE_TTL); | ||
cache.put(KEY, VALUE); | ||
cache.put(KEY_2, VALUE); | ||
|
||
expect(cache.get(KEY)).toEqual(VALUE); | ||
expect(cache.get(KEY_2)).toEqual(VALUE); | ||
cache.clear(); | ||
|
||
expect(cache.get(KEY)).toEqual(null); | ||
expect(cache.get(KEY_2)).toEqual(null); | ||
done(); | ||
}); | ||
|
||
it('should deafult TTL to 5 seconds', () => { | ||
var cache = new InMemoryCache({}); | ||
expect(cache.ttl).toEqual(5 * 1000); | ||
}); | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
var InMemoryCacheAdapter = require('../src/Adapters/Cache/InMemoryCacheAdapter').default; | ||
|
||
describe('InMemoryCacheAdapter', function() { | ||
var KEY = 'hello'; | ||
var VALUE = 'world'; | ||
|
||
function wait(sleep) { | ||
return new Promise(function(resolve, reject) { | ||
setTimeout(resolve, sleep); | ||
}) | ||
} | ||
|
||
it('should expose promisifyed methods', (done) => { | ||
var cache = new InMemoryCacheAdapter({ | ||
ttl: NaN | ||
}); | ||
|
||
var noop = () => {}; | ||
|
||
// Verify all methods return promises. | ||
Promise.all([ | ||
cache.put(KEY, VALUE), | ||
cache.del(KEY), | ||
cache.get(KEY), | ||
cache.clear() | ||
]).then(() => { | ||
done(); | ||
}); | ||
}); | ||
|
||
it('should get/set/clear', (done) => { | ||
var cache = new InMemoryCacheAdapter({ | ||
ttl: NaN | ||
}); | ||
|
||
cache.put(KEY, VALUE) | ||
.then(() => cache.get(KEY)) | ||
.then((value) => expect(value).toEqual(VALUE)) | ||
.then(() => cache.clear()) | ||
.then(() => cache.get(KEY)) | ||
.then((value) => expect(value).toEqual(null)) | ||
.then(done); | ||
}); | ||
|
||
it('should expire after ttl', (done) => { | ||
var cache = new InMemoryCacheAdapter({ | ||
ttl: 10 | ||
}); | ||
|
||
cache.put(KEY, VALUE) | ||
.then(() => cache.get(KEY)) | ||
.then((value) => expect(value).toEqual(VALUE)) | ||
.then(wait.bind(null, 50)) | ||
.then(() => cache.get(KEY)) | ||
.then((value) => expect(value).toEqual(null)) | ||
.then(done); | ||
}) | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
export class CacheAdapter { | ||
/** | ||
* Get a value in the cache | ||
* @param key Cache key to get | ||
* @return Promise that will eventually resolve to the value in the cache. | ||
*/ | ||
get(key) {} | ||
|
||
/** | ||
* Set a value in the cache | ||
* @param key Cache key to set | ||
* @param value Value to set the key | ||
* @param ttl Optional TTL | ||
*/ | ||
put(key, value, ttl) {} | ||
|
||
/** | ||
* Remove a value from the cache. | ||
* @param key Cache key to remove | ||
*/ | ||
del(key) {} | ||
|
||
/** | ||
* Empty a cache | ||
*/ | ||
clear() {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
const DEFAULT_CACHE_TTL = 5 * 1000; | ||
|
||
|
||
export class InMemoryCache { | ||
constructor({ | ||
ttl = DEFAULT_CACHE_TTL | ||
}) { | ||
this.ttl = ttl; | ||
this.cache = Object.create(null); | ||
} | ||
|
||
get(key) { | ||
let record = this.cache[key]; | ||
if (record == null) { | ||
return null; | ||
} | ||
|
||
// Has Record and isnt expired | ||
if (isNaN(record.expire) || record.expire >= Date.now()) { | ||
return record.value; | ||
} | ||
|
||
// Record has expired | ||
delete this.cache[key]; | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
} | ||
|
||
put(key, value, ttl = this.ttl) { | ||
if (ttl < 0 || isNaN(ttl)) { | ||
ttl = NaN; | ||
} | ||
|
||
var record = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we handle value == null as a delete? |
||
value: value, | ||
expire: ttl + Date.now() | ||
} | ||
|
||
if (!isNaN(record.expire)) { | ||
record.timeout = setTimeout(() => { | ||
this.del(key); | ||
}, ttl); | ||
} | ||
|
||
this.cache[key] = record; | ||
} | ||
|
||
del(key) { | ||
var record = this.cache[key]; | ||
if (record == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also instead of loose equality id go with !record There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I avoid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Record will never be 0 as it's ever wrapped into an object There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a general rule I never use
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually it isn't more specific as undefined == null and null == undefined. but ok. Don't wanna enter the details There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the rest of the code, we check with ! for those exact cases. Less misleading. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the method retuns a Promise.resolve but in that case... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I will remove the rest of the |
||
return; | ||
} | ||
|
||
if (record.timeout) { | ||
clearTimeout(record.timeout); | ||
} | ||
|
||
delete this.cache[key]; | ||
} | ||
|
||
clear() { | ||
this.cache = Object.create(null); | ||
} | ||
|
||
} | ||
|
||
export default InMemoryCache; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import {InMemoryCache} from './InMemoryCache'; | ||
|
||
export class InMemoryCacheAdapter { | ||
|
||
constructor(ctx) { | ||
this.cache = new InMemoryCache(ctx) | ||
} | ||
|
||
get(key) { | ||
return new Promise((resolve, reject) => { | ||
let record = this.cache.get(key); | ||
if (record == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we use null for cache miss, let's use strict equality There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more just a guard to stop trying to |
||
return resolve(null); | ||
} | ||
|
||
return resolve(JSON.parse(record)); | ||
}) | ||
} | ||
|
||
put(key, value, ttl) { | ||
this.cache.put(key, JSON.stringify(value), ttl); | ||
return Promise.resolve(); | ||
} | ||
|
||
del(key) { | ||
this.cache.del(key); | ||
return Promise.resolve(); | ||
} | ||
|
||
clear() { | ||
this.cache.clear(); | ||
return Promise.resolve(); | ||
} | ||
} | ||
|
||
export default InMemoryCacheAdapter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should return a promise to match other functions / or other functions should return something else than promises for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InMemoryCache
isn't actually aCacheAdapter
,there was a cache already in the application which needed to be synchronous (
AppCache
), I moved that implementation here and added a TTL option to it, so we don't have two cache classes.the
InMemoryCacheAdapter
wraps this in promises.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case remove all the return promises. That's odd tot have an interface that has inconsistent return values for related operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point are the promises.
There needs to be two caches in a parse-server one for the applications
AppCache
and one for actually cached dataCacheController/Adapter
(_Roles/_Session/etc..).The
AppCache
needs to be synchronous and cant be used remotely (Caches actual JS Objects not JSON), that is whereInMemoryCache
comes from, this is so that we don't have two different Cache APIs in parse-serverThe promise based architecture is so that users can replace
CacheAdapter
with remote caching systems, redis/memcached.The default
CacheAdatpter
(InMemoryCacheAdapter
) is something that just wraps the synchronousInMemoryCache
with promises.