cache: Add in ExpirationTime to local caches. #46
Conversation
This field allows entries to expire from the cache in the case that the detectors are improved or there was a bug in the classification. The parameter is configurable and users desiring the old behavior can set their expiration time to be a large value.
|
@davisjam We should have a discussion on what we want the cache invalidation tests to look like. One idea is to create a special directory with the invalidation tests that we clear after every cache invalidation test. |
src/cache/client/npm/test/test.js
Outdated
| }, | ||
| cache: { | ||
| type: vulnRegexDetector.cacheTypes.persistent, | ||
| expirationTime: -1 |
There was a problem hiding this comment.
Suggest you declare a cacheConfig variable at the top of the function and use it here and above.
src/cache/client/npm/test/test.js
Outdated
| hostname: 'no such host', | ||
| port: 1 | ||
| }, | ||
| cache: { |
There was a problem hiding this comment.
Refactor into a cacheConfig as in persistent case.
| @@ -47,7 +47,8 @@ const defaultServerConfig = { | |||
|
|
|||
| const defaultCacheConfig = { | |||
| type: CACHE_TYPES.persistent, | |||
| persistentDir: path.join(os.tmpdir(), 'vuln-regex-detector-client-persistentCache') | |||
| persistentDir: path.join(os.tmpdir(), 'vuln-regex-detector-client-persistentCache'), | |||
| expirationTime: 60 * 60 * 24 * 7 // 7 days | |||
There was a problem hiding this comment.
Change comment to // 7 days in seconds
| @@ -341,8 +347,19 @@ function updateCache (config, pattern, response) { | |||
| if (!useCache(config)) { | |||
| return; | |||
| } | |||
| /* Only cache VULNERABLE|SAFE responses. */ | |||
There was a problem hiding this comment.
Add a newline before and after this block.
| @@ -351,15 +368,20 @@ function checkCache (config, pattern) { | |||
| return RESPONSE_UNKNOWN; | |||
| } | |||
|
|
|||
| return kvGet(config, pattern); | |||
| let valueRetrieved = kvGet(config, pattern); | |||
| return; | ||
| } | ||
| /* This entry will expire config.expirationTime seconds from now. */ | ||
| let expirationTimeInMilliseconds = 1000 * config.cache.expirationTime; |
| } | ||
| /* Check if the cache entry has expired. */ | ||
| let lastValidDate = new Date(valueRetrieved.validUntil); | ||
| if (Date.now() > lastValidDate) { |
There was a problem hiding this comment.
I prefer to write comparisons as smaller < larger.
| /* Check if the cache entry has expired. */ | ||
| let lastValidDate = new Date(valueRetrieved.validUntil); | ||
| if (Date.now() > lastValidDate) { | ||
| /* The entry in the cache has expired. */ |
There was a problem hiding this comment.
For consistency, The cache entry has expired.
| @@ -418,7 +440,7 @@ function kvPersistentFname (config, key) { | |||
| * Using a hash might give us false reports on collisions, but this is | |||
| * exceedingly unlikely in typical use cases (a few hundred regexes tops). */ | |||
| const hash = crypto.createHash('md5').update(key).digest('hex'); | |||
| const fname = path.join(config.cache.persistentDir, `${hash}.json`); | |||
| const fname = path.join(config.cache.persistentDir, `${hash}-v2.json`); | |||
There was a problem hiding this comment.
Do you think we should the number "2" a global CACHE_VERSION variable? Then a comment on that variable can indicate history better.
Cache invalidation -> cache expiration time? Agreed that you shouldn't use the default persistentDir because of the risk of collisions with cached user queries that have larger expiration times. How about |
|
@ewmson Can you update the README with the new config option? |
1.) Adding const 2.) Refactoring out a CACHE_VERSION 3.) Adding documentation to README 4.) Adding a special dir for the cache invalidation tests that is purged after each test.
…all from an already imported npm module.
cache: Add in ExpirationTime to local caches.
Problem:
After a regex has been rescanned #45 the local client will still have the old result cached.
This means a user who had previously received the result will never hit the server again and see the updated case.
Solution:
Add an expiration time to the cached values that will require the server to be queried again once the time in the cache has exceeded this time. This should be configurable based on the needs of the client.
Fixes: #44