From 2c98dd5f9dca14d2dedc1c68433aa8934342d8b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bar=C4=B1=C5=9F=20Soner=20U=C5=9Fakl=C4=B1?= Date: Wed, 24 Apr 2019 14:38:46 -0400 Subject: [PATCH] Object cache refactor (#7558) * fix: cache refactor db.getObjectField no longer loads entire object db.getObjectsFields only clones data once more tests * feat: add back cache to redis db.getObjectField no longer loads entire object --- src/database/mongo/hash.js | 95 ++++++++++++++--------------- src/database/postgres/hash.js | 6 +- src/database/redis/hash.js | 111 +++++++++++++++++++++++----------- src/database/redis/main.js | 5 ++ test/database/hash.js | 41 +++++++++++++ 5 files changed, 170 insertions(+), 88 deletions(-) diff --git a/src/database/mongo/hash.js b/src/database/mongo/hash.js index 54a45edf60..b6be45b776 100644 --- a/src/database/mongo/hash.js +++ b/src/database/mongo/hash.js @@ -39,57 +39,29 @@ module.exports = function (db, module) { module.getObject = function (key, callback) { if (!key) { - return callback(); + return setImmediate(callback, null, null); } module.getObjects([key], function (err, data) { - if (err) { - return callback(err); - } - callback(null, data && data.length ? data[0] : null); + callback(err, data && data.length ? data[0] : null); }); }; module.getObjects = function (keys, callback) { - var cachedData = {}; - function getFromCache() { - process.nextTick(callback, null, keys.map(key => _.clone(cachedData[key]))); - } - - if (!Array.isArray(keys) || !keys.length) { - return callback(null, []); - } - - const unCachedKeys = cache.getUnCachedKeys(keys, cachedData); - - if (!unCachedKeys.length) { - return getFromCache(); - } - - var query = { _key: { $in: unCachedKeys } }; - if (unCachedKeys.length === 1) { - query._key = unCachedKeys[0]; - } - db.collection('objects').find(query, { projection: { _id: 0 } }).toArray(function (err, data) { - if (err) { - return callback(err); - } - data = data.map(helpers.deserializeData); - var map = helpers.toMap(data); - unCachedKeys.forEach(function (key) { - cachedData[key] = map[key] || null; - cache.set(key, cachedData[key]); - }); - - getFromCache(); - }); + module.getObjectsFields(keys, [], callback); }; module.getObjectField = function (key, field, callback) { if (!key) { - return callback(); + return setImmediate(callback, null, null); + } + const cachedData = {}; + cache.getUnCachedKeys([key], cachedData); + if (cachedData[key]) { + return setImmediate(callback, null, cachedData[key].hasOwnProperty(field) ? cachedData[key][field] : null); } - module.getObject(key, function (err, item) { + field = helpers.fieldToString(field); + db.collection('objects').findOne({ _key: key }, { projection: { _id: 0, [field]: 1 } }, function (err, item) { if (err || !item) { return callback(err, null); } @@ -99,7 +71,7 @@ module.exports = function (db, module) { module.getObjectFields = function (key, fields, callback) { if (!key) { - return callback(); + return setImmediate(callback, null, null); } module.getObjectsFields([key], fields, function (err, data) { callback(err, data ? data[0] : null); @@ -108,18 +80,16 @@ module.exports = function (db, module) { module.getObjectsFields = function (keys, fields, callback) { if (!Array.isArray(keys) || !keys.length) { - return callback(null, []); + return setImmediate(callback, null, []); } - module.getObjects(keys, function (err, items) { - if (err) { - return callback(err); - } - if (items === null) { - items = []; - } + const cachedData = {}; + function returnData() { + var mapped = keys.map(function (key) { + if (!fields.length) { + return _.clone(cachedData[key]); + } - const returnData = items.map((item) => { - item = item || {}; + const item = cachedData[key] || {}; const result = {}; fields.forEach((field) => { result[field] = item[field] !== undefined ? item[field] : null; @@ -127,7 +97,30 @@ module.exports = function (db, module) { return result; }); - callback(null, returnData); + callback(null, mapped); + } + + const unCachedKeys = cache.getUnCachedKeys(keys, cachedData); + if (!unCachedKeys.length) { + return process.nextTick(returnData); + } + + var query = { _key: { $in: unCachedKeys } }; + if (unCachedKeys.length === 1) { + query._key = unCachedKeys[0]; + } + db.collection('objects').find(query, { projection: { _id: 0 } }).toArray(function (err, data) { + if (err) { + return callback(err); + } + data = data.map(helpers.deserializeData); + var map = helpers.toMap(data); + unCachedKeys.forEach(function (key) { + cachedData[key] = map[key] || null; + cache.set(key, cachedData[key]); + }); + + returnData(); }); }; diff --git a/src/database/postgres/hash.js b/src/database/postgres/hash.js index aeb794185b..969104c84b 100644 --- a/src/database/postgres/hash.js +++ b/src/database/postgres/hash.js @@ -65,7 +65,7 @@ VALUES ($1::TEXT, jsonb_build_object($2::TEXT, $3::TEXT::JSONB)) module.getObject = function (key, callback) { if (!key) { - return callback(); + return callback(null, null); } db.query({ @@ -122,7 +122,7 @@ SELECT h."data" module.getObjectField = function (key, field, callback) { if (!key) { - return callback(); + return setImmediate(callback, null, null); } db.query({ @@ -151,7 +151,7 @@ SELECT h."data"->>$2::TEXT f module.getObjectFields = function (key, fields, callback) { if (!key) { - return callback(); + return setImmediate(callback, null, null); } db.query({ diff --git a/src/database/redis/hash.js b/src/database/redis/hash.js index 9df27690d4..355393f05c 100644 --- a/src/database/redis/hash.js +++ b/src/database/redis/hash.js @@ -3,6 +3,13 @@ module.exports = function (redisClient, module) { var helpers = module.helpers.redis; + const async = require('async'); + const _ = require('lodash'); + + const cache = require('../cache').create('redis'); + + module.objectCache = cache; + module.setObject = function (key, data, callback) { callback = callback || function () {}; if (!key || !data) { @@ -23,7 +30,11 @@ module.exports = function (redisClient, module) { return callback(); } redisClient.hmset(key, data, function (err) { - callback(err); + if (err) { + return callback(err); + } + cache.delObjectCache(key); + callback(); }); }; @@ -33,61 +44,90 @@ module.exports = function (redisClient, module) { return callback(); } redisClient.hset(key, field, value, function (err) { - callback(err); + if (err) { + return callback(err); + } + cache.delObjectCache(key); + callback(); }); }; module.getObject = function (key, callback) { - redisClient.hgetall(key, callback); + if (!key) { + return setImmediate(callback, null, null); + } + + module.getObjectsFields([key], [], function (err, data) { + callback(err, data && data.length ? data[0] : null); + }); }; module.getObjects = function (keys, callback) { - if (!Array.isArray(keys) || !keys.length) { - return setImmediate(callback, null, []); - } - if (keys.length > 1) { - helpers.execKeys(redisClient, 'batch', 'hgetall', keys, callback); - } else { - redisClient.hgetall(keys[0], (err, data) => callback(err, [data])); - } + module.getObjectsFields(keys, [], callback); }; module.getObjectField = function (key, field, callback) { - module.getObjectFields(key, [field], function (err, data) { - callback(err, data ? data[field] : null); - }); + if (!key) { + return setImmediate(callback, null, null); + } + const cachedData = {}; + cache.getUnCachedKeys([key], cachedData); + if (cachedData[key]) { + return setImmediate(callback, null, cachedData[key].hasOwnProperty(field) ? cachedData[key][field] : null); + } + redisClient.hget(key, field, callback); }; module.getObjectFields = function (key, fields, callback) { + if (!key) { + return setImmediate(callback, null, null); + } module.getObjectsFields([key], fields, function (err, results) { callback(err, results ? results[0] : null); }); }; module.getObjectsFields = function (keys, fields, callback) { - if (!Array.isArray(fields) || !fields.length) { - return callback(null, keys.map(function () { return {}; })); + if (!Array.isArray(keys) || !keys.length) { + return setImmediate(callback, null, []); } - var batch = redisClient.batch(); - - for (var x = 0; x < keys.length; x += 1) { - batch.hmget.apply(batch, [keys[x]].concat(fields)); + if (!Array.isArray(fields)) { + return callback(null, keys.map(function () { return {}; })); } - - batch.exec(function (err, results) { - if (err) { - return callback(err); - } - - results = results.map(function makeObject(array) { - var obj = {}; - for (var i = 0, ii = fields.length; i < ii; i += 1) { - obj[fields[i]] = array[i]; + const cachedData = {}; + const unCachedKeys = cache.getUnCachedKeys(keys, cachedData); + + async.waterfall([ + function (next) { + if (unCachedKeys.length > 1) { + helpers.execKeys(redisClient, 'batch', 'hgetall', unCachedKeys, next); + } else if (unCachedKeys.length === 1) { + redisClient.hgetall(unCachedKeys[0], (err, data) => next(err, [data])); + } else { + next(null, []); } - return obj; - }); - callback(null, results); - }); + }, + function (data, next) { + unCachedKeys.forEach(function (key, i) { + cachedData[key] = data[i] || null; + cache.set(key, cachedData[key]); + }); + + var mapped = keys.map(function (key) { + if (!fields.length) { + return _.clone(cachedData[key]); + } + + const item = cachedData[key] || {}; + const result = {}; + fields.forEach((field) => { + result[field] = item[field] !== undefined ? item[field] : null; + }); + return result; + }); + next(null, mapped); + }, + ], callback); }; module.getObjectKeys = function (key, callback) { @@ -116,12 +156,14 @@ module.exports = function (redisClient, module) { return setImmediate(callback); } redisClient.hdel(key, field, function (err) { + cache.delObjectCache(key); callback(err); }); }; module.deleteObjectFields = function (key, fields, callback) { helpers.execKeyValues(redisClient, 'batch', 'hdel', key, fields, function (err) { + cache.delObjectCache(key); callback(err); }); }; @@ -140,6 +182,7 @@ module.exports = function (redisClient, module) { if (err) { return callback(err); } + cache.delObjectCache(key); callback(null, Array.isArray(result) ? result.map(value => parseInt(value, 10)) : parseInt(result, 10)); } value = parseInt(value, 10); diff --git a/src/database/redis/main.js b/src/database/redis/main.js index 5fe8e27562..c81f3aa73e 100644 --- a/src/database/redis/main.js +++ b/src/database/redis/main.js @@ -16,6 +16,7 @@ module.exports = function (redisClient, module) { if (err) { return callback(err); } + module.objectCache.resetObjectCache(); callback(); }); }; @@ -35,6 +36,7 @@ module.exports = function (redisClient, module) { module.delete = function (key, callback) { callback = callback || function () {}; redisClient.del(key, function (err) { + module.objectCache.delObjectCache(key); callback(err); }); }; @@ -46,6 +48,7 @@ module.exports = function (redisClient, module) { batch.del(keys[i]); } batch.exec(function (err) { + module.objectCache.delObjectCache(keys); callback(err); }); }; @@ -72,6 +75,8 @@ module.exports = function (redisClient, module) { if (err && err.message !== 'ERR no such key') { return callback(err); } + module.objectCache.delObjectCache(oldKey); + module.objectCache.delObjectCache(newKey); callback(); }); }; diff --git a/test/database/hash.js b/test/database/hash.js index 392379e66b..f521b4238a 100644 --- a/test/database/hash.js +++ b/test/database/hash.js @@ -92,6 +92,20 @@ describe('Hash methods', function () { }); }); }); + + it('should work for field names with "." in them when they are cached', function (done) { + db.setObjectField('dotObject3', 'my.dot.field', 'foo2', function (err) { + assert.ifError(err); + db.getObject('dotObject3', function (err, data) { + assert.ifError(err); + db.getObjectField('dotObject3', 'my.dot.field', function (err, value) { + assert.ifError(err); + assert.equal(value, 'foo2'); + done(); + }); + }); + }); + }); }); describe('getObject()', function () { @@ -113,6 +127,15 @@ describe('Hash methods', function () { done(); }); }); + + it('should return null if key is falsy', function (done) { + db.getObject(null, function (err, data) { + assert.ifError(err); + assert.equal(arguments.length, 2); + assert.equal(data, null); + done(); + }); + }); }); describe('getObjects()', function () { @@ -163,6 +186,15 @@ describe('Hash methods', function () { done(); }); }); + + it('should return null if key is falsy', function (done) { + db.getObjectField(null, 'test', function (err, data) { + assert.ifError(err); + assert.equal(arguments.length, 2); + assert.equal(data, null); + done(); + }); + }); }); describe('getObjectFields()', function () { @@ -188,6 +220,15 @@ describe('Hash methods', function () { done(); }); }); + + it('should return null if key is falsy', function (done) { + db.getObjectFields(null, ['test', 'foo'], function (err, data) { + assert.ifError(err); + assert.equal(arguments.length, 2); + assert.equal(data, null); + done(); + }); + }); }); describe('getObjectsFields()', function () {