From 91125ac27a33301088e8c354e1880f5743569070 Mon Sep 17 00:00:00 2001
From: Joshua Kwan <joshua.m.kwan@gmail.com>
Date: Mon, 13 Jul 2020 17:36:09 -0400
Subject: [PATCH] Replace hmset/expire/hgetall with set/get

In the original implementation, there is a risk of an infinitely
long-lived cache key that arises if HMSET for the cache entry succeeds,
but EXPIRE fails or is not executed for any reason.

By serializing the cache `entry` to JSON and storing that single string
in Redis, we can take advantage of SET's ability to atomically accept an
EX-piration time along with the set.

The downside is we now have to do a JSON.stringify for every cache store
we do, and JSON.parse for every cache hit / retrieval, but that is
better than cache entries that never expire.

Tests pass against a local redis after modifying 1 test case which stores a
value then stores the same key again without cache. In the HMSET
implementation, this test passed because HMSET does not modify the
expiration date of existing keys. However, calls to SET always remove
old values and any expiration. Therefore, use a different key to store
the explicitly non-expiring cache entry.

Fixes #101.
---
 lib/ExpressRedisCache/add.js | 28 ++++++++++++++--------------
 lib/ExpressRedisCache/get.js |  6 +++++-
 test/add.js                  |  3 ++-
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/lib/ExpressRedisCache/add.js b/lib/ExpressRedisCache/add.js
index 4f5b225..7962e64 100644
--- a/lib/ExpressRedisCache/add.js
+++ b/lib/ExpressRedisCache/add.js
@@ -54,24 +54,24 @@ module.exports = (function () {
       var prefix = self.prefix.match(/:$/) ? self.prefix.replace(/:$/, '')
         : self.prefix;
 
-      /* Save as a Redis hash */
+      /** Serialize the entry to JSON so it can be stored as a regular key's value. */
       var redisKey = prefix + ':' + name;
-      self.client.hmset(redisKey, entry,
+      var args = [redisKey, JSON.stringify(entry)];
+
+      /** If @expire then tell Redis to expire **/
+      if (typeof entry.expire === 'number' && entry.expire > 0) {
+        args.push('EX', entry.expire);
+      }
+
+      self.client.set(args,
         domain.intercept(function (res) {
           var calculated_size = (size / 1024).toFixed(2);
-          /** If @expire then tell Redis to expire **/
-          if ( typeof entry.expire === 'number' && entry.expire > 0 ) {
-            self.client.expire(redisKey, +entry.expire,
-              domain.intercept(function () {
-                self.emit('message', require('util').format('SET %s ~%d Kb %d TTL (sec)', redisKey, calculated_size, +entry.expire));
-                callback(null, name, entry, res);
-              }));
-          }
-          else
-          {
-            self.emit('message', require('util').format('SET %s ~%d Kb', redisKey, calculated_size));
-            callback(null, name, entry, res);
+          var msg = require('util').format('SET %s ~%d Kb', redisKey, calculated_size);
+          if (typeof entry.expire === 'number' && entry.expire > 0) {
+            msg += require('util').format(' %d TTL (sec)', +entry.expire);
           }
+          self.emit('message', msg);
+          callback(null, name, entry, res);
         }));
     });
   }
diff --git a/lib/ExpressRedisCache/get.js b/lib/ExpressRedisCache/get.js
index dac1ffa..9cb084c 100644
--- a/lib/ExpressRedisCache/get.js
+++ b/lib/ExpressRedisCache/get.js
@@ -40,11 +40,15 @@ module.exports = (function () {
       var hasWildcard = redisKey.indexOf('*') >= 0;
 
       var fetchKey = function (key, cb) {
-        self.client.hgetall(key, domain.intercept(function (result) {
+        self.client.get(key, domain.intercept(function (result) {
           if ( result ) {
+            /** Deserialize the result from JSON. */
+            result = JSON.parse(result);
+
             var names = key.split(':');
             result.name = names[1];
             result.prefix = names[0];
+
             self.emit('message', require('util').format('GET %s ~%d Kb', key,
               (require('../sizeof')(result) / 1024).toFixed(2)));
           }
diff --git a/test/add.js b/test/add.js
index 9844e1d..6278502 100644
--- a/test/add.js
+++ b/test/add.js
@@ -14,6 +14,7 @@
   var host      =   process.env.EX_RE_CA_HOST || 'localhost';
   var port      =   process.env.EX_RE_CA_PORT || 6379;
 
+  var _noexpire =   "i don't expire";
   var _name     =   'test1';
   var _body     =   'test1 test1 test1';
   var _type     =   'text/plain';
@@ -44,7 +45,7 @@
     });
 
     it ( 'should allow for zero expiration', function(done) {
-      cache.add(_name, _body, { expire: 0 },
+      cache.add(_noexpire, _body, { expire: 0 },
         function($error, $name, $entry) {
           var resp;
           if($entry.expire !== 0) {