From 2e6c7f7b095a94392c9286cdce72cc9f37ccad92 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 27 Jan 2017 12:05:19 -0700 Subject: [PATCH 1/2] First pass at pagination --- graph/loaders/actions.js | 15 ++- graph/loaders/comments.js | 173 ++++++++++++++++++++----------- graph/loaders/util.js | 73 ++++++++++++- graph/mutators/action.js | 13 ++- graph/mutators/comment.js | 23 +++- graph/resolvers/action.js | 8 +- graph/resolvers/asset.js | 16 ++- graph/resolvers/comment.js | 24 +++-- graph/resolvers/date.js | 27 +++++ graph/resolvers/index.js | 2 + graph/resolvers/root_mutation.js | 16 +-- graph/resolvers/root_query.js | 38 ++++--- graph/resolvers/user.js | 21 ++-- graph/typeDefs.graphql | 83 ++++++++++++--- services/cache.js | 166 ++++++++++++++++++++++++++--- 15 files changed, 553 insertions(+), 145 deletions(-) create mode 100644 graph/resolvers/date.js diff --git a/graph/loaders/actions.js b/graph/loaders/actions.js index cc7d43a12..c41f17219 100644 --- a/graph/loaders/actions.js +++ b/graph/loaders/actions.js @@ -3,6 +3,7 @@ const DataLoader = require('dataloader'); const util = require('./util'); const ActionsService = require('../../services/actions'); +const ActionModel = require('../../models/action'); /** * Looks up actions based on the requested id's all bounded by the user. @@ -16,6 +17,17 @@ const genActionSummariessByItemID = ({user = {}}, item_ids) => { .then(util.arrayJoinBy(item_ids, 'item_id')); }; +/** + * Search for actions based on their action_type and item_type and ensures that + * the actions returned have unique item id's. + * @param {String} action_type the action to search by + * @param {String} item_type the item id to search by + * @return {Promise} resolves to distinct items actions + */ +const getItemIdsByActionTypeAndItemType = (_, action_type, item_type) => { + return ActionModel.distinct('item_id', {action_type, item_type}); +}; + /** * Creates a set of loaders based on a GraphQL context. * @param {Object} context the context of the GraphQL request @@ -23,6 +35,7 @@ const genActionSummariessByItemID = ({user = {}}, item_ids) => { */ module.exports = (context) => ({ Actions: { - getByItemID: new DataLoader((ids) => genActionSummariessByItemID(context, ids)), + getSummariesByItemID: new DataLoader((ids) => genActionSummariessByItemID(context, ids)), + getByTypes: ({action_type, item_type}) => getItemIdsByActionTypeAndItemType(context, action_type, item_type) } }); diff --git a/graph/loaders/comments.js b/graph/loaders/comments.js index 9a67bb621..7298cbbf4 100644 --- a/graph/loaders/comments.js +++ b/graph/loaders/comments.js @@ -1,85 +1,134 @@ -const DataLoader = require('dataloader'); - const util = require('./util'); -const ActionModel = require('../../models/action'); const CommentModel = require('../../models/comment'); -const CommentsService = require('../../services/comments'); /** - * Retrieves comments by an array of asset id's, results are returned in reverse - * chronological order. - * @param {Array} ids array of ids to lookup + * Returns the comment count for all comments that are public based on their + * asset ids. + * @param {Object} context graph context + * @param {Array} asset_ids the ids of assets for which there are + * comments that we want to get */ -const genCommentsByAssetID = (context, ids) => { - return CommentModel.find({ - asset_id: { - $in: ids +const getCountsByAssetID = (context, asset_ids) => { + return CommentModel.aggregate([ + { + $match: { + asset_id: { + $in: asset_ids + }, + status: { + $in: [null, 'ACCEPTED'] + }, + parent_id: null + } }, - parent_id: null, - status: { - $in: [null, 'ACCEPTED'] + { + $group: { + _id: '$asset_id', + count: { + $sum: 1 + } + } } - }) - .sort({created_at: -1}) - .then(util.arrayJoinBy(ids, 'asset_id')); + ]) + .then(util.singleJoinBy(asset_ids, '_id')) + .then((results) => results.map((result) => result ? result.count : 0)); }; /** - * Retrieves comments by an array of parent ids, results are returned in - * chronological order. - * @param {Array} ids array of ids to lookup + * Returns the comment count for all comments that are public based on their + * parent ids. + * @param {Object} context graph context + * @param {Array} parent_ids the ids of parents for which there are + * comments that we want to get */ -const genCommentsByParentID = (context, ids) => { - return CommentModel.find({ - parent_id: { - $in: ids +const getCountsByParentID = (context, parent_ids) => { + return CommentModel.aggregate([ + { + $match: { + parent_id: { + $in: parent_ids + }, + status: { + $in: [null, 'ACCEPTED'] + } + } }, - status: { - $in: [null, 'ACCEPTED'] + { + $group: { + _id: '$parent_id', + count: { + $sum: 1 + } + } } - }) - .sort({created_at: 1}) - .then(util.arrayJoinBy(ids, 'parent_id')); + ]) + .then(util.singleJoinBy(parent_ids, '_id')) + .then((results) => results.map((result) => result ? result.count : 0)); }; -const getCommentsByStatusAndAssetID = (context, {status = null, asset_id = null}) => { +/** + * Retrieves comments based on the passed in query that is filtered by the + * current used passed in via the context. + * @param {Object} context graph context + * @param {Object} query query terms to apply to the comments query + */ +const getCommentsByQuery = ({user}, {ids, statuses, asset_id, parent_id, limit, cursor, sort}) => { + let comments = CommentModel.find(); - // TODO: remove when we move the enum over to the uppercase. - if (status) { - status = status.toLowerCase(); + // Only administrators can search for comments with statuses that are not + // `null`, or `'ACCEPTED'`. + if (user != null && user.hasRoles('ADMIN') && statuses) { + comments = comments.where({ + status: { + $in: statuses + } + }); + } else { + comments = comments.where({ + status: { + $in: [null, 'ACCEPTED'] + } + }); } - return CommentsService.moderationQueue(status, asset_id); -}; - -const getCommentsByActionTypeAndAssetID = (context, {action_type, asset_id = null}) => { - return ActionModel.find({ - action_type, - item_type: 'COMMENTS' - }).then((actions) => { - let comments = CommentModel.find({ + if (ids) { + comments = comments.find({ id: { - $in: actions.map((action) => action.item_id) + $in: ids } - }).sort({created_at: 1}); + }); + } - if (asset_id) { - comments = comments.where({asset_id}); + if (asset_id) { + comments = comments.where({asset_id}); + } + + // We perform the undefined check because, null, is a valid state for the + // search to be with, which indicates that it is at depth 0. + if (parent_id !== undefined) { + comments = comments.where({parent_id}); + } + + if (cursor) { + if (sort === 'REVERSE_CHRONOLOGICAL') { + comments = comments.where({ + created_at: { + $lt: cursor + } + }); + } else { + comments = comments.where({ + created_at: { + $gt: cursor + } + }); } + } - return comments; - }); -}; - -const genCommentsByAuthorID = (context, authorIDs) => { - return CommentModel.find({ - author_id: { - $in: authorIDs - } - }) - .sort({created_at: -1}) - .then(util.arrayJoinBy(authorIDs, 'author_id')); + return comments + .sort({created_at: sort === 'REVERSE_CHRONOLOGICAL' ? -1 : 1}) + .limit(limit); }; /** @@ -89,10 +138,8 @@ const genCommentsByAuthorID = (context, authorIDs) => { */ module.exports = (context) => ({ Comments: { - getByParentID: new DataLoader((ids) => genCommentsByParentID(context, ids)), - getByAssetID: new DataLoader((ids) => genCommentsByAssetID(context, ids)), - getByStatusAndAssetID: (query) => getCommentsByStatusAndAssetID(context, query), - getByActionTypeAndAssetID: (query) => getCommentsByActionTypeAndAssetID(context, query), - getByAuthorID: new DataLoader((authorIDs) => genCommentsByAuthorID(context, authorIDs)) + getByQuery: (query) => getCommentsByQuery(context, query), + countByAssetID: new util.SharedCacheDataLoader('Comments.countByAssetID', 3600, (ids) => getCountsByAssetID(context, ids)), + countByParentID: new util.SharedCacheDataLoader('Comments.countByParentID', 3600, (ids) => getCountsByParentID(context, ids)) } }); diff --git a/graph/loaders/util.js b/graph/loaders/util.js index e947f2fa6..dbfcdedfd 100644 --- a/graph/loaders/util.js +++ b/graph/loaders/util.js @@ -1,4 +1,6 @@ const _ = require('lodash'); +const DataLoader = require('dataloader'); +const cache = require('../../services/cache'); /** * SingletonResolver is a cached loader for a single result. @@ -34,6 +36,7 @@ class SingletonResolver { */ const arrayJoinBy = (ids, key) => (items) => { const itemsByKey = _.groupBy(items, key); + return ids.map((id) => { if (id in itemsByKey) { return itemsByKey[id]; @@ -61,8 +64,76 @@ const singleJoinBy = (ids, key) => (items) => { }); }; +/** + * SharedCacheDataLoader provides a version of the DataLoader that wraps up a + * redis backed cache with the dataloader's request cache. + */ +class SharedCacheDataLoader extends DataLoader { + constructor(prefix, expiry, batchLoadFn, options) { + super(SharedCacheDataLoader.batchLoadFn(prefix, expiry, batchLoadFn), options); + + this._keyFunc = SharedCacheDataLoader.keyFunc(this._prefix); + this._prefix = prefix; + this._expiry = expiry; + } + + /** + * clear the key from the shared cache and the request cache + */ + clear(key) { + return cache + .invalidate(key, this._keyFunc) + .then(() => super.clear(key)); + } + + /** + * prime the shared cache and the request cache + */ + prime(key, value) { + return cache + .set(key, value, this._expiry, this._keyFunc) + .then(() => super.prime(key, value)); + } + + /** + * prime many values in the shared cache and the request cache + */ + primeMany(keys, values) { + return cache + .setMany(keys, values, this._expiry, this._keyFunc) + .then(() => keys.map((key, i) => super.prime(key, values[i]))); + } + + /** + * wraps up the prefix needed for the redis backed shared cache driver + */ + static keyFunc(prefix) { + return (key) => `cache.sbl[${prefix}][${key}]`; + } + + /** + * wraps the dataloader batchLoadFn with the shared cache's wrapper + */ + static batchLoadFn(prefix, expiry, batchLoadFn) { + return (ids) => cache.wrapMany(ids, expiry, (workKeys) => { + return batchLoadFn(workKeys); + }, SharedCacheDataLoader.keyFunc(prefix)); + } +} + +/** + * 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 + * key + */ +const objectCacheKeyFn = (...paths) => (obj) => { + return paths.map((path) => obj[path]).join(':'); +}; + module.exports = { singleJoinBy, arrayJoinBy, - SingletonResolver + objectCacheKeyFn, + SingletonResolver, + SharedCacheDataLoader }; diff --git a/graph/mutators/action.js b/graph/mutators/action.js index 3c4c32986..b5264fd49 100644 --- a/graph/mutators/action.js +++ b/graph/mutators/action.js @@ -18,10 +18,15 @@ const createAction = ({user = {}}, {item_id, item_type, action_type, metadata = user_id: user.id, action_type, metadata - }).then((result) => - item_type === 'USERS' && action_type === 'FLAG' ? - UsersService.setStatus(item_id, 'PENDING').then(() => result) - : result); + }).then((action) => { + if (item_type === 'USERS' && action_type === 'FLAG') { + return UsersService + .setStatus(item_id, 'PENDING') + .then(() => action); + } + + return action; + }); }; /** diff --git a/graph/mutators/comment.js b/graph/mutators/comment.js index cc62e48df..3c35b0e60 100644 --- a/graph/mutators/comment.js +++ b/graph/mutators/comment.js @@ -14,13 +14,29 @@ const Wordlist = require('../../services/wordlist'); * @param {String} [status=null] the status of the new comment * @return {Promise} resolves to the created comment */ -const createComment = ({user}, {body, asset_id, parent_id = null}, status = null) => { +const createComment = ({user, loaders: {Comments}}, {body, asset_id, parent_id = null}, status = null) => { return CommentsService.publicCreate({ body, asset_id, parent_id, status, author_id: user.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) { + if (parent_id != null) { + Comments.countByParentID.clear(parent_id); + } else { + Comments.countByAssetID.clear(asset_id); + } + } + + return comment; }); }; @@ -121,7 +137,7 @@ const createPublicComment = (context, commentInput) => { if (wordlist != null) { // TODO: this is kind of fragile, we should refactor this to resolve - // all these const's that we're using like 'comments', 'flag' to be + // all these const's that we're using like 'COMMENTS', 'FLAG' to be // defined in a checkable schema. return context.mutators.Action.createAction(null, { item_id: comment.id, @@ -131,7 +147,8 @@ const createPublicComment = (context, commentInput) => { field: 'body', details: 'Matched suspect word filters.' } - }).then(() => comment); + }) + .then(() => comment); } // Finally, we return the comment. diff --git a/graph/resolvers/action.js b/graph/resolvers/action.js index e6886653e..f8aa60245 100644 --- a/graph/resolvers/action.js +++ b/graph/resolvers/action.js @@ -1,10 +1,10 @@ const Action = { // This will load the user for the specific action. We'll limit this to the - // admin users only. - user({user_id}, _, {loaders, user}) { - if (user.hasRole('ADMIN')) { - return loaders.Users.getByID.load(user_id); + // admin users only or the current logged in user. + user({user_id}, _, {loaders: {Users}, user}) { + if (user && (user.hasRole('ADMIN') || user_id === user.id)) { + return Users.getByID.load(user_id); } } }; diff --git a/graph/resolvers/asset.js b/graph/resolvers/asset.js index dcf6e1a75..60b44fa62 100644 --- a/graph/resolvers/asset.js +++ b/graph/resolvers/asset.js @@ -1,9 +1,17 @@ const Asset = { - comments({id}, _, {loaders}) { - return loaders.Comments.getByAssetID.load(id); + comments({id}, {sort, limit}, {loaders: {Comments}}) { + return Comments.getByQuery({ + asset_id: id, + sort, + limit, + parent_id: null + }); }, - settings({settings = null}, _, {loaders}) { - return loaders.Settings.load() + commentCount({id}, _, {loaders: {Comments}}) { + return Comments.countByAssetID.load(id); + }, + settings({settings = null}, _, {loaders: {Settings}}) { + return Settings.load() .then((globalSettings) => { if (settings) { settings = Object.assign({}, globalSettings.toObject(), settings); diff --git a/graph/resolvers/comment.js b/graph/resolvers/comment.js index ce2283cda..b3d79225e 100644 --- a/graph/resolvers/comment.js +++ b/graph/resolvers/comment.js @@ -1,15 +1,23 @@ const Comment = { - user({author_id}, _, {loaders}) { - return loaders.Users.getByID.load(author_id); + user({author_id}, _, {loaders: {Users}}) { + return Users.getByID.load(author_id); }, - replies({id}, _, {loaders}) { - return loaders.Comments.getByParentID.load(id); + replies({id, asset_id}, {sort, limit}, {loaders: {Comments}}) { + return Comments.getByQuery({ + asset_id, + parent_id: id, + sort, + limit + }); }, - actions({id}, _, {loaders}) { - return loaders.Actions.getByItemID.load(id); + replyCount({id}, _, {loaders: {Comments}}) { + return Comments.countByParentID.load(id); }, - asset({asset_id}, _, {loaders}) { - return loaders.Assets.getByID.load(asset_id); + actions({id}, _, {loaders: {Actions}}) { + return Actions.getSummariesByItemID.load(id); + }, + asset({asset_id}, _, {loaders: {Assets}}) { + return Assets.getByID.load(asset_id); } }; diff --git a/graph/resolvers/date.js b/graph/resolvers/date.js new file mode 100644 index 000000000..2335b9713 --- /dev/null +++ b/graph/resolvers/date.js @@ -0,0 +1,27 @@ +const GraphQLScalarType = require('graphql').GraphQLScalarType; +const Kind = require('graphql/language').Kind; + +module.exports = new GraphQLScalarType({ + name: 'Date', + description: 'Date represented as an ISO8601 string', + serialize(value) { + return value.toISOString(); + }, + parseValue(value) { + return new Date(value); + }, + parseLiteral(ast) { + switch (ast.kind) { + case Kind.STRING: + + // This handles an empty string. + if (ast.value && ast.value.length === 0) { + return null; + } + + return new Date(ast.value); + default: + return null; + } + } +}); diff --git a/graph/resolvers/index.js b/graph/resolvers/index.js index 064b1e664..f25664f34 100644 --- a/graph/resolvers/index.js +++ b/graph/resolvers/index.js @@ -2,6 +2,7 @@ const Action = require('./action'); const ActionSummary = require('./action_summary'); const Asset = require('./asset'); const Comment = require('./comment'); +const Date = require('./date'); const RootMutation = require('./root_mutation'); const RootQuery = require('./root_query'); const Settings = require('./settings'); @@ -12,6 +13,7 @@ module.exports = { ActionSummary, Asset, Comment, + Date, RootMutation, RootQuery, Settings, diff --git a/graph/resolvers/root_mutation.js b/graph/resolvers/root_mutation.js index bf108671e..ddc8e4223 100644 --- a/graph/resolvers/root_mutation.js +++ b/graph/resolvers/root_mutation.js @@ -1,15 +1,15 @@ const RootMutation = { - createComment(_, {asset_id, parent_id, body}, {mutators}) { - return mutators.Comment.create({asset_id, parent_id, body}); + createComment(_, {asset_id, parent_id, body}, {mutators: {Comment}}) { + return Comment.create({asset_id, parent_id, body}); }, - createAction(_, {action}, {mutators}) { - return mutators.Action.create(action); + createAction(_, {action}, {mutators: {Action}}) { + return Action.create(action); }, - deleteAction(_, {id}, {mutators}) { - return mutators.Action.delete({id}); + deleteAction(_, {id}, {mutators: {Action}}) { + return Action.delete({id}); }, - updateUserSettings(_, {settings}, {mutators}) { - return mutators.User.updateSettings(settings); + updateUserSettings(_, {settings}, {mutators: {User}}) { + return User.updateSettings(settings); } }; diff --git a/graph/resolvers/root_query.js b/graph/resolvers/root_query.js index 5676b4f48..8aa14d5c6 100644 --- a/graph/resolvers/root_query.js +++ b/graph/resolvers/root_query.js @@ -1,39 +1,47 @@ const RootQuery = { - assets(_, args, {loaders, user}) { + assets(_, args, {loaders: {Assets}, user}) { if (user == null || !user.hasRoles('ADMIN')) { return null; } - return loaders.Assets.getAll.load(); + return Assets.getAll.load(); }, - asset(_, query, {loaders}) { + asset(_, query, {loaders: {Assets}}) { if (query.id) { // TODO: we may not always have a comment stream here, therefore, when we // load it, we may also need to create with the url. This may also have to // move the logic over to the mutators function as an upsert operation // possibly. - return loaders.Assets.getByID.load(query.id); + return Assets.getByID.load(query.id); } - return loaders.Assets.getByURL(query.url); + return Assets.getByURL(query.url); }, - settings(_, args, {loaders}) { - return loaders.Settings.load(); + settings(_, args, {loaders: {Settings}}) { + return Settings.load(); }, // This endpoint is used for loading moderation queues, so hide it in the // event that we aren't an admin. - comments(_, {query}, {loaders, user}) { - if (user == null || !user.hasRoles('ADMIN')) { - return null; + comments(_, {query: {action_type, statuses, asset_id, parent_id, limit, cursor, sort}}, {user, loaders: {Comments, Actions}}) { + let query = {statuses, asset_id, parent_id, limit, cursor, sort}; + + if (user != null && user.hasRoles('ADMIN') && action_type) { + return Actions.getByTypes({action_type, item_type: 'COMMENTS'}) + .then((actions) => { + + // Map the actions from the items referenced byt this query. The actions + // returned by this query are explicitly going to be distinct by their + // `item_id`'s. + let ids = actions.map((action) => action.item_id); + + // Perform the query using the available resolver. + return Comments.getByQuery({ids, statuses, asset_id, parent_id, limit, cursor, sort}); + }); } - if (query.action_type) { - return loaders.Comments.getByActionTypeAndAssetID(query); - } else { - return loaders.Comments.getByStatusAndAssetID(query); - } + return Comments.getByQuery(query); }, // This returns the current user, ensure that if we aren't logged in, we diff --git a/graph/resolvers/user.js b/graph/resolvers/user.js index 65882feee..8cc11bba3 100644 --- a/graph/resolvers/user.js +++ b/graph/resolvers/user.js @@ -1,16 +1,25 @@ const User = { - actions({id}, _, {loaders}) { - return loaders.Actions.getByID.load(id); + actions({id}, _, {loaders: {Actions}}) { + return Actions.getSummariesByItemID.load(id); }, - comments({id}, _, {loaders, user}) { + comments({id}, _, {loaders: {Comments}, user}) { // If the user is not an admin, only return comment list for the owner of // the comments. - if (!user.hasRoles('ADMIN') || user.id !== id) { - return null; + if (user && (user.hasRoles('ADMIN') || user.id === id)) { + return Comments.getByAuthorID.load(id); } - return loaders.Comments.getByAuthorID.load(id); + return null; + }, + roles({id, roles}, _, {user}) { + + // If the user is not an admin, only return the current user's roles. + if (user && (user.hasRoles('ADMIN') || user.id === id)) { + return roles; + } + + return null; } }; diff --git a/graph/typeDefs.graphql b/graph/typeDefs.graphql index 167f75f4f..744a53252 100644 --- a/graph/typeDefs.graphql +++ b/graph/typeDefs.graphql @@ -1,21 +1,50 @@ -interface ActionableItem { - id: ID! +# Establishes the ordering of the content by their created_at time stamp. +enum SORT_ORDER { + # newest to oldest order. + REVERSE_CHRONOLOGICAL + + # oldest to newer order. + CHRONOLOGICAL } +# Date represented as an ISO8601 string. +scalar Date + type UserSettings { # bio of the user. bio: String } -input CommentsInput { +input CommentsQuery { # current status of a comment. - status: COMMENT_STATUS + statuses: [COMMENT_STATUS] # asset that a comment is on. asset_id: ID - # action type to find comments that have an action with. + # the parent of the comment that we want to retrive. + parent_id: ID + + # comments returned will only be ones which have at least one action of this + # type. action_type: ACTION_TYPE + + # limit the number of results to be returned. + limit: Int = 10 + + # skip results from the last created_at timestamp. + cursor: Date + + # sort the results by created_at. + sort: SORT_ORDER = REVERSE_CHRONOLOGICAL +} + +enum USER_ROLES { + # an administrator of the site + ADMIN + + # a moderator of the site + MODERATOR } # Any person who can author comments, create actions, and view comments on a @@ -29,11 +58,14 @@ type User { # actions against a specific user. actions: [ActionSummary] + # the current roles of the user. + roles: [USER_ROLES] + # settings for a user. settings: UserSettings # returns all comments based on a query. - comments(query: CommentsInput): [Comment] + comments(query: CommentsQuery): [Comment] } type Comment { @@ -46,7 +78,10 @@ type Comment { user: User # the replies that were made to the comment. - replies(limit: Int = 3): [Comment] + replies(sort: SORT_ORDER = CHRONOLOGICAL, limit: Int = 3): [Comment] + + # the count of replies on a comment + replyCount: Int # the actions made against a comment. actions: [ActionSummary] @@ -58,7 +93,7 @@ type Comment { status: COMMENT_STATUS # the time when the comment was created - created_at: String! + created_at: Date! } enum ITEM_TYPE { @@ -78,11 +113,10 @@ type Action { item_id: ID! item_type: ITEM_TYPE! - item: ActionableItem user: User! - updated_at: String - created_at: String + updated_at: Date + created_at: Date } type ActionSummary { @@ -109,13 +143,31 @@ type Settings { } type Asset { + + # The current ID of the asset. id: ID! + + # The scraped title of the asset. title: String + + # The URL that the asset is locaed on. url: String - comments: [Comment] + + # The top level comments that are attached to the asset. + comments(sort: SORT_ORDER = REVERSE_CHRONOLOGICAL, limit: Int = 10): [Comment] + + # The count of top level comments on the asset. + commentCount: Int + + # The settings (rectified with the global settings) that should be applied to + # this asset. settings: Settings! - closedAt: String - created_at: String + + # The date that the asset was closed at. + closedAt: Date + + # The date that the asset was created. + created_at: Date } enum COMMENT_STATUS { @@ -125,6 +177,7 @@ enum COMMENT_STATUS { } type RootQuery { + # retrieves site wide settings and defaults. settings: Settings @@ -135,7 +188,7 @@ type RootQuery { asset(id: ID, url: String): Asset # retrieves comments based on the input query. - comments(query: CommentsInput): [Comment] + comments(query: CommentsQuery!): [Comment] # retrieves the current logged in user. me: User diff --git a/services/cache.js b/services/cache.js index 1d090e46d..21ae9911e 100644 --- a/services/cache.js +++ b/services/cache.js @@ -1,4 +1,5 @@ const redis = require('./redis'); +const debug = require('debug')('talk:cache'); const cache = module.exports = { client: redis.createClient() @@ -29,31 +30,103 @@ const keyfunc = (key) => { * resolved as the value to cache. * @return {Promise} Resolves to the value either retrieved from cache */ -cache.wrap = (key, expiry, work) => { +cache.wrap = (key, expiry, work, kf = keyfunc) => { return cache - .get(key) + .get(key, kf) .then((value) => { if (value !== null) { + debug('wrap: hit', kf(key)); return value; } + debug('wrap: miss', kf(key)); + return work() .then((value) => { - return cache - .set(key, value, expiry) - .then(() => value); + + process.nextTick(() => { + cache + .set(key, value, expiry, kf) + .then(() => { + debug('wrap: set complete'); + }) + .catch((err) => { + console.error(err); + }); + }); + + return value; }); }); }; +/** + * [wrapMany description] + * @param {Array} keys Either an array of objects represening + * this work + * @param {Integer} expiry Time in seconds for the cache entry to live for + * @param {Function} work A function that returns a promise that can be + * resolved as the value to cache. + * @param {Function} [kf=keyfunc] optional key function to use to turn the + * provided key into a string for the cache. + * @return {Promise} resovles to the values for the keys + */ +cache.wrapMany = (keys, expiry, work, kf = keyfunc) => { + return cache + .getMany(keys, kf) + .then((values) => { + + // find any of the null valued items by collecting the work + let workRefs = values + .map((value, index) => ({value, index, key: keys[index]})) + .filter(({value}) => value === null); + + let workKeys = workRefs.map(({key}) => key); + + debug(`wrapMany: hit ratio: ${keys.length - workKeys.length}/${keys.length}`); + + if (workKeys.length > 0) { + return work(workKeys) + .then((workedValues) => { + + // Set the items in the cache that we needed to retrive after the + // next process tick. + process.nextTick(() => { + cache + .setMany(workKeys, workedValues, expiry, kf) + .then(() => { + debug('wrapMany: setMany complete'); + }) + .catch((err) => { + console.error(err); + }); + }); + + return workedValues; + }) + .then((workedValues) => { + + // Walk over the worked keys to merge them with the existing values. + for (let i = 0; i < workRefs.length; i++) { + values[workRefs[i].index] = workedValues[i]; + } + + return values; + }); + } else { + return values; + } + }); +}; + /** * This returns a promise that returns a promise that resolves with the value * from the cache or null if it does not exist in the cache. * @param {Mixed} key Either an array of items composing a key or a string * @return {Promise} */ -cache.get = (key) => new Promise((resolve, reject) => { - cache.client.get(keyfunc(key), (err, reply) => { +cache.get = (key, kf = keyfunc) => new Promise((resolve, reject) => { + cache.client.get(kf(key), (err, reply) => { if (err) { return reject(err); } @@ -76,13 +149,80 @@ cache.get = (key) => new Promise((resolve, reject) => { }); }); +/** + * Returns many replies. + * @param {Array} keys Either an array of objects represening + * this work + * @param {Function} [kf=keyfunc] optional key function to use to turn the + * provided key into a string for the cache. + */ +cache.getMany = (keys, kf = keyfunc) => new Promise((resolve, reject) => { + cache.client.mget(keys.map(kf), (err, replies) => { + if (err) { + return reject(err); + } + + // Parse the replies. + for (let i = 0; i < replies.length; i++) { + let value = null; + + if (replies[i] != null) { + try { + + // Parse the stored cache value from JSON. + value = JSON.parse(replies[i]); + } catch (e) { + return reject(e); + } + } + + replies[i] = value; + } + + return resolve(replies); + }); +}); + +/** + * Sets many entries in the cache. + * @param {Array} keys array of keys + * @param {Array} values array of values to set + * @param {Function} [kf=keyfunc] optional key function to use to turn the + * provided key into a string for the cache. + */ +cache.setMany = (keys, values, expiry, kf = keyfunc) => { + let multi = cache.client.multi(); + + keys.forEach((key, index) => { + + // Serialize the value as JSON. + let reply = JSON.stringify(values[index]); + + // Queue up the set command. + multi.set(kf(key), reply, 'EX', expiry); + }); + + return new Promise((resolve, reject) => { + multi.exec((err) => { + if (err) { + return reject(err); + } + + resolve(); + }); + }); +}; + /** * This invalidates a cached entry in the cache. * @param {Mixed} key Either an array of items composing a key or a string * @return {Promise} */ -cache.invalidate = (key) => new Promise((resolve, reject) => { - cache.client.del(keyfunc(key), (err) => { +cache.invalidate = (key, kf = keyfunc) => new Promise((resolve, reject) => { + + debug(`invalidate: ${kf(key)}`); + + cache.client.del(kf(key), (err) => { if (err) { return reject(err); } @@ -94,17 +234,17 @@ cache.invalidate = (key) => new Promise((resolve, reject) => { /** * This sets a value on the key with the expiry and then resolves once it is * done. - * @param {Mixed} key Either an array of items composing a key or a string - * @param {Mixed} value Object to be serialized and set to the cache + * @param {Mixed} key Either an array of items composing a key or a string + * @param {Mixed} value Object to be serialized and set to the cache * @param {Integer} expiry Time in seconds for the cache entry to live for * @return {Promise} */ -cache.set = (key, value, expiry) => new Promise((resolve, reject) => { +cache.set = (key, value, expiry, kf = keyfunc) => new Promise((resolve, reject) => { // Serialize the value as JSON. let reply = JSON.stringify(value); - cache.client.set(keyfunc(key), reply, 'EX', expiry, (err) => { + cache.client.set(kf(key), reply, 'EX', expiry, (err) => { if (err) { return reject(err); } From 04cd7312a62c27cbfa5484551a57a5796dace45d Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 27 Jan 2017 15:52:05 -0700 Subject: [PATCH 2/2] Added support for new commentCount --- client/coral-embed-stream/src/Embed.js | 2 +- client/coral-framework/graphql/queries/streamQuery.graphql | 1 + client/coral-plugin-stream/Stream.js | 1 + graph/loaders/util.js | 2 +- 4 files changed, 4 insertions(+), 2 deletions(-) diff --git a/client/coral-embed-stream/src/Embed.js b/client/coral-embed-stream/src/Embed.js index fd505e299..2a7057cca 100644 --- a/client/coral-embed-stream/src/Embed.js +++ b/client/coral-embed-stream/src/Embed.js @@ -105,7 +105,7 @@ class Embed extends Component { loading ? :
- + Settings Configure Stream diff --git a/client/coral-framework/graphql/queries/streamQuery.graphql b/client/coral-framework/graphql/queries/streamQuery.graphql index 77f8ed356..583a2db13 100644 --- a/client/coral-framework/graphql/queries/streamQuery.graphql +++ b/client/coral-framework/graphql/queries/streamQuery.graphql @@ -17,6 +17,7 @@ query AssetQuery($asset_url: String!) { charCount requireEmailConfirmation } + commentCount comments { ...commentView replies { diff --git a/client/coral-plugin-stream/Stream.js b/client/coral-plugin-stream/Stream.js index 2ec84af24..633a98502 100644 --- a/client/coral-plugin-stream/Stream.js +++ b/client/coral-plugin-stream/Stream.js @@ -73,6 +73,7 @@ query AssetQuery($asset_id: ID!) { id title url + commentCount comments { ...commentView replies { diff --git a/graph/loaders/util.js b/graph/loaders/util.js index dbfcdedfd..4640d8245 100644 --- a/graph/loaders/util.js +++ b/graph/loaders/util.js @@ -72,9 +72,9 @@ class SharedCacheDataLoader extends DataLoader { constructor(prefix, expiry, batchLoadFn, options) { super(SharedCacheDataLoader.batchLoadFn(prefix, expiry, batchLoadFn), options); - this._keyFunc = SharedCacheDataLoader.keyFunc(this._prefix); this._prefix = prefix; this._expiry = expiry; + this._keyFunc = SharedCacheDataLoader.keyFunc(this._prefix); } /**