Merge pull request #405 from coralproject/asset-comment-total-count

Added support for total comment count

graph/loaders/comments.js

@@ -1,4 +1,8 @@
-const util = require('./util');
+const {
+  SharedCounterDataLoader,
+  singleJoinBy,
+  arrayJoinBy
+} = require('./util');
 const DataLoader = require('dataloader');
 
 const CommentModel = require('../../models/comment');
@@ -11,6 +15,38 @@ const CommentModel = require('../../models/comment');
  *                                    comments that we want to get
  */
 const getCountsByAssetID = (context, asset_ids) => {
+  return CommentModel.aggregate([
+    {
+      $match: {
+        asset_id: {
+          $in: asset_ids
+        },
+        status: {
+          $in: ['NONE', 'ACCEPTED']
+        }
+      }
+    },
+    {
+      $group: {
+        _id: '$asset_id',
+        count: {
+          $sum: 1
+        }
+      }
+    }
+  ])
+  .then(singleJoinBy(asset_ids, '_id'))
+  .then((results) => results.map((result) => result ? result.count : 0));
+};
+
+/**
+ * Returns the comment count for all comments that are public based on their
+ * asset ids.
+ * @param {Object}        context     graph context
+ * @param {Array<String>} asset_ids   the ids of assets for which there are
+ *                                    comments that we want to get
+ */
+const getParentCountsByAssetID = (context, asset_ids) => {
   return CommentModel.aggregate([
     {
       $match: {
@@ -32,7 +68,7 @@ const getCountsByAssetID = (context, asset_ids) => {
       }
     }
   ])
-  .then(util.singleJoinBy(asset_ids, '_id'))
+  .then(singleJoinBy(asset_ids, '_id'))
   .then((results) => results.map((result) => result ? result.count : 0));
 };
 
@@ -64,7 +100,7 @@ const getCountsByParentID = (context, parent_ids) => {
       }
     }
   ])
-  .then(util.singleJoinBy(parent_ids, '_id'))
+  .then(singleJoinBy(parent_ids, '_id'))
   .then((results) => results.map((result) => result ? result.count : 0));
 };
 
@@ -216,7 +252,7 @@ const genRecentReplies = (context, ids) => {
 
   ])
   .then((replies) => replies.map((reply) => reply.replies))
-  .then(util.arrayJoinBy(ids, 'parent_id'));
+  .then(arrayJoinBy(ids, 'parent_id'));
 };
 
 /**
@@ -267,7 +303,7 @@ const genRecentComments = (_, ids) => {
 
   ])
   .then((replies) => replies.map((reply) => reply.comments))
-  .then(util.arrayJoinBy(ids, 'asset_id'));
+  .then(arrayJoinBy(ids, 'asset_id'));
 };
 
 /**
@@ -294,7 +330,7 @@ const genComments = ({user}, ids) => {
       }
     });
   }
-  return comments.then(util.singleJoinBy(ids, 'id'));
+  return comments.then(singleJoinBy(ids, 'id'));
 };
 
 /**
@@ -307,8 +343,9 @@ module.exports = (context) => ({
     get: new DataLoader((ids) => genComments(context, ids)),
     getByQuery: (query) => getCommentsByQuery(context, query),
     getCountByQuery: (query) => getCommentCountByQuery(context, query),
-    countByAssetID: new util.SharedCacheDataLoader('Comments.countByAssetID', 3600, (ids) => getCountsByAssetID(context, ids)),
-    countByParentID: new util.SharedCacheDataLoader('Comments.countByParentID', 3600, (ids) => getCountsByParentID(context, ids)),
+    countByAssetID: new SharedCounterDataLoader('Comments.totalCommentCount', 3600, (ids) => getCountsByAssetID(context, ids)),
+    parentCountByAssetID: new SharedCounterDataLoader('Comments.countByAssetID', 3600, (ids) => getParentCountsByAssetID(context, ids)),
+    countByParentID: new SharedCounterDataLoader('Comments.countByParentID', 3600, (ids) => getCountsByParentID(context, ids)),
     genRecentReplies: new DataLoader((ids) => genRecentReplies(context, ids)),
     genRecentComments: new DataLoader((ids) => genRecentComments(context, ids))
   }

graph/loaders/util.js

@@ -121,6 +121,29 @@ class SharedCacheDataLoader extends DataLoader {
   }
 }
 
+/**
+ * SharedCounterDataLoader is identical to SharedCacheDataLoader with the
+ * exception in that it is designed to work with numerical cached data.
+ */
+class SharedCounterDataLoader extends SharedCacheDataLoader {
+
+  /**
+   * Increments the key in the cache if it already exists in the cache, if not
+   * it does nothing.
+   */
+  incr(key) {
+    return cache.incr(key, this._expiry, this._keyFunc);
+  }
+
+  /**
+   * Decrements the key in the cache if it already exists in the cache, if not
+   * it does nothing.
+   */
+  decr(key) {
+    return cache.decr(key, this._expiry, this._keyFunc);
+  }
+}
+
 /**
  * Maps an object's paths to a string that can be used as a cache key.
  * @param  {Array} paths paths on the object to be used to generate the cache
@@ -145,5 +168,6 @@ module.exports = {
   objectCacheKeyFn,
   arrayCacheKeyFn,
   SingletonResolver,
-  SharedCacheDataLoader
+  SharedCacheDataLoader,
+  SharedCounterDataLoader
 };

graph/mutators/comment.js

@@ -33,16 +33,17 @@ const createComment = ({user, loaders: {Comments}}, {body, asset_id, parent_id =
   })
   .then((comment) => {
 
-    // TODO: explore using an `INCR` operation to update the counts here
-
     // If the loaders are present, clear the caches for these values because we
-    // just added a new comment, hence the counts should be updated.
-    if (Comments && Comments.countByAssetID && Comments.countByParentID) {
+    // just added a new comment, hence the counts should be updated. We should
+    // perform these increments in the event that we do have a new comment that
+    // is approved or without a comment.
+    if (status === 'NONE' || status === 'APPROVED') {
       if (parent_id != null) {
-        Comments.countByParentID.clear(parent_id);
+        Comments.countByParentID.incr(parent_id);
       } else {
-        Comments.countByAssetID.clear(asset_id);
+        Comments.parentCountByAssetID.incr(asset_id);
       }
+      Comments.countByAssetID.incr(asset_id);
     }
 
     return comment;
@@ -182,15 +183,18 @@ const setCommentStatus = ({loaders: {Comments}}, {id, status}) => {
     .then((comment) => {
 
       // If the loaders are present, clear the caches for these values because we
-      // just added a new comment, hence the counts should be updated.
-      if (Comments && Comments.countByAssetID && Comments.countByParentID) {
-        if (comment.parent_id != null) {
-          Comments.countByParentID.clear(comment.parent_id);
-        } else {
-          Comments.countByAssetID.clear(comment.asset_id);
-        }
+      // just added a new comment, hence the counts should be updated. It would
+      // be nice if we could decrement the counters here, but that would result
+      // in us having to know the initial state of the comment, which would
+      // require another database query.
+      if (comment.parent_id != null) {
+        Comments.countByParentID.clear(comment.parent_id);
+      } else {
+        Comments.parentCountByAssetID.clear(comment.asset_id);
       }
 
+      Comments.countByAssetID.clear(comment.asset_id);
+
       return comment;
     });
 };

graph/resolvers/asset.js

@@ -10,7 +10,18 @@ const Asset = {
       parent_id: null
     });
   },
-  commentCount({id}, _, {loaders: {Comments}}) {
+  commentCount({id, commentCount}, _, {loaders: {Comments}}) {
+    if (commentCount != null) {
+      return commentCount;
+    }
+
+    return Comments.parentCountByAssetID.load(id);
+  },
+  totalCommentCount({id, totalCommentCount}, _, {loaders: {Comments}}) {
+    if (totalCommentCount != null) {
+      return totalCommentCount;
+    }
+
     return Comments.countByAssetID.load(id);
   },
   settings({settings = null}, _, {loaders: {Settings}}) {

graph/typeDefs.graphql

@@ -422,6 +422,9 @@ type Asset {
   # The count of top level comments on the asset.
   commentCount: Int
 
+  # The total count of all comments made on the asset.
+  totalCommentCount: Int
+
   # The settings (rectified with the global settings) that should be applied to
   # this asset.
   settings: Settings!

package.json

@@ -83,7 +83,7 @@
     "passport-facebook": "^2.1.1",
     "passport-local": "^1.0.0",
     "react-apollo": "^0.10.0",
-    "redis": "^2.6.3",
+    "redis": "^2.6.5",
     "uuid": "^2.0.3"
   },
   "devDependencies": {

services/cache.js

@@ -1,5 +1,6 @@
 const redis = require('./redis');
 const debug = require('debug')('talk:cache');
+const crypto = require('crypto');
 
 const cache = module.exports = {
   client: redis.createClient()
@@ -60,6 +61,157 @@ cache.wrap = (key, expiry, work, kf = keyfunc) => {
     });
 };
 
+// This is designed to increment a key and add an expiry iff the key already
+// exists.
+const INCR_SCRIPT = `
+if redis.call('GET', KEYS[1]) ~= false then
+  redis.call('INCR', KEYS[1])
+  redis.call('EXPIRE', KEYS[1], ARGV[1])
+end
+`;
+
+// Stores the SHA1 hash of INCR_SCRIPT, used for executing via EVALSHA.
+let INCR_SCRIPT_HASH;
+
+// This is designed to decrement a key and add an expiry iff the key already
+// exists.
+const DECR_SCRIPT = `
+if redis.call('GET', KEYS[1]) ~= false then
+  redis.call('DECR', KEYS[1])
+  redis.call('EXPIRE', KEYS[1], ARGV[1])
+end
+`;
+
+// Stores the SHA1 hash of DECR_SCRIPT, used for executing via EVALSHA.
+let DECR_SCRIPT_HASH;
+
+// Load the script into redis and track the script hash that we will use to exec
+// increments on.
+const loadScript = (name, script) => new Promise((resolve, reject) => {
+
+  let shasum = crypto.createHash('sha1');
+  shasum.update(script);
+
+  let hash = shasum.digest('hex');
+
+  cache.client
+    .script('EXISTS', hash, (err, [exists]) => {
+      if (err) {
+        return reject(err);
+      }
+
+      if (exists) {
+        debug(`already loaded ${name} as SHA[${hash}], not loading again`);
+
+        return resolve(hash);
+      }
+
+      debug(`${name} not loaded as SHA[${hash}], loading`);
+
+      cache.client
+        .script('load', script, (err, hash) => {
+          if (err) {
+            return reject(err);
+          }
+
+          debug(`loaded ${name} as SHA[${hash}]`);
+
+          resolve(hash);
+        });
+    });
+});
+
+// Load the INCR_SCRIPT and DECR_SCRIPT into Redis.
+Promise.all([
+  loadScript('INCR_SCRIPT', INCR_SCRIPT),
+  loadScript('DECR_SCRIPT', DECR_SCRIPT)
+])
+.then(([incrScriptHash, decrScriptHash]) => {
+  INCR_SCRIPT_HASH = incrScriptHash;
+  DECR_SCRIPT_HASH = decrScriptHash;
+})
+.catch((err) => {
+  throw err;
+});
+
+/**
+ * This will increment a key in redis and update the expiry iff it already
+ * exists, otherwise it will do nothing.
+ */
+cache.incr = (key, expiry, kf = keyfunc) => new Promise((resolve, reject) => {
+  cache.client
+    .evalsha(INCR_SCRIPT_HASH, 1, kf(key), expiry, (err) => {
+      if (err) {
+        return reject(err);
+      }
+
+      return resolve();
+    });
+});
+
+/**
+ * This will decrement a key in redis and update the expiry iff it already
+ * exists, otherwise it will do nothing.
+ */
+cache.decr = (key, expiry, kf = keyfunc) => new Promise((resolve, reject) => {
+  cache.client
+    .evalsha(DECR_SCRIPT_HASH, 1, kf(key), expiry, (err) => {
+      if (err) {
+        return reject(err);
+      }
+
+      return resolve();
+    });
+});
+
+/**
+ * This will increment many keys in redis and update the expiry iff it already
+ * exists, otherwise it will do nothing.
+ */
+cache.incrMany = (keys, expiry, kf = keyfunc) => {
+  let multi = cache.client.multi();
+
+  keys.forEach((key) => {
+
+    // Queue up the evalsha command.
+    multi.evalsha(INCR_SCRIPT_HASH, 1, kf(key), expiry);
+  });
+
+  return new Promise((resolve, reject) => {
+    multi.exec((err) => {
+      if (err) {
+        return reject(err);
+      }
+
+      resolve();
+    });
+  });
+};
+
+/**
+ * This will decrement many keys in redis and update the expiry iff it already
+ * exists, otherwise it will do nothing.
+ */
+cache.decrMany = (keys, expiry, kf = keyfunc) => {
+  let multi = cache.client.multi();
+
+  keys.forEach((key) => {
+
+    // Queue up the evalsha command.
+    multi.evalsha(DECR_SCRIPT_HASH, 1, kf(key), expiry);
+  });
+
+  return new Promise((resolve, reject) => {
+    multi.exec((err) => {
+      if (err) {
+        return reject(err);
+      }
+
+      resolve();
+    });
+  });
+};
+
 /**
  * [wrapMany description]
  * @param  {Array<String>} keys         Either an array of objects represening