From 17ce2febf9104c03801eaab237b077c519480443 Mon Sep 17 00:00:00 2001 From: Keith Holliday Date: Tue, 7 Nov 2017 13:19:39 -0700 Subject: [PATCH] [WIP] Add initial fixes for concurrency (#9321) * Add initial fixes for concurrency * Added memory edit for notifications * Fixed tag delete * Fixed adding and moving task order * Updated delete task * Fixed lint * Fixed task adding * Switch to mongoose push and pull --- .eslintignore | 3 ++ .../integration/tasks/DELETE-tasks_id.test.js | 12 ++++- ...POST-tasks_move_taskId_to_position.test.js | 4 ++ .../controllers/api-v3/notifications.js | 9 +++- website/server/controllers/api-v3/tags.js | 15 ++++--- website/server/controllers/api-v3/tasks.js | 45 +++++++++++++++---- website/server/libs/cron.js | 1 - website/server/libs/taskManager.js | 16 ++++++- 8 files changed, 86 insertions(+), 19 deletions(-) diff --git a/.eslintignore b/.eslintignore index b107de65a1..7b618cca64 100644 --- a/.eslintignore +++ b/.eslintignore @@ -6,6 +6,9 @@ website/transpiled-babel/ website/common/transpiled-babel/ dist/ dist-client/ +apidoc_build/ +content_cache/ +node_modules/ # Not linted website/client-old/ diff --git a/test/api/v3/integration/tasks/DELETE-tasks_id.test.js b/test/api/v3/integration/tasks/DELETE-tasks_id.test.js index 6f9ef63b67..2f2c174973 100644 --- a/test/api/v3/integration/tasks/DELETE-tasks_id.test.js +++ b/test/api/v3/integration/tasks/DELETE-tasks_id.test.js @@ -141,6 +141,16 @@ describe('DELETE /tasks/:id', () => { }); }); - it('removes a task from user.tasksOrder'); // TODO + it('removes a task from user.tasksOrder', async () => { + let task = await user.post('/tasks/user', { + text: 'test habit', + type: 'habit', + }); + + await user.del(`/tasks/${task._id}`); + await user.sync(); + + expect(user.tasksOrder.habits.indexOf(task._id)).to.eql(-1); + }); }); }); diff --git a/test/api/v3/integration/tasks/POST-tasks_move_taskId_to_position.test.js b/test/api/v3/integration/tasks/POST-tasks_move_taskId_to_position.test.js index b11143abe5..2b17cd3919 100644 --- a/test/api/v3/integration/tasks/POST-tasks_move_taskId_to_position.test.js +++ b/test/api/v3/integration/tasks/POST-tasks_move_taskId_to_position.test.js @@ -40,9 +40,13 @@ describe('POST /tasks/:taskId/move/to/:position', () => { let taskToMove = tasks[1]; expect(taskToMove.text).to.equal('habit 2'); + let newOrder = await user.post(`/tasks/${tasks[1]._id}/move/to/3`); + await user.sync(); + expect(newOrder[3]).to.equal(taskToMove._id); expect(newOrder.length).to.equal(5); + expect(user.tasksOrder.habits).to.eql(newOrder); }); it('can move task to new position using alias', async () => { diff --git a/website/server/controllers/api-v3/notifications.js b/website/server/controllers/api-v3/notifications.js index d739cb1748..4d4a8fc809 100644 --- a/website/server/controllers/api-v3/notifications.js +++ b/website/server/controllers/api-v3/notifications.js @@ -37,7 +37,10 @@ api.readNotification = { } user.notifications.splice(index, 1); - await user.save(); + + await user.update({ + $pull: { notifications: req.params.notificationId }, + }).exec(); res.respond(200, user.notifications); }, @@ -77,7 +80,9 @@ api.readNotifications = { user.notifications.splice(index, 1); } - await user.save(); + await user.update({ + $pullAll: { notifications }, + }).exec(); res.respond(200, user.notifications); }, diff --git a/website/server/controllers/api-v3/tags.js b/website/server/controllers/api-v3/tags.js index 0f28368f5c..63844c38e6 100644 --- a/website/server/controllers/api-v3/tags.js +++ b/website/server/controllers/api-v3/tags.js @@ -5,7 +5,7 @@ import { NotFound, } from '../../libs/errors'; import _ from 'lodash'; -import { removeFromArray } from '../../libs/collectionManipulators'; +import find from 'lodash/find'; /** * @apiDefine TagNotFound @@ -216,19 +216,24 @@ api.deleteTag = { let validationErrors = req.validationErrors(); if (validationErrors) throw validationErrors; - let tag = removeFromArray(user.tags, { id: req.params.tagId }); - if (!tag) throw new NotFound(res.t('tagNotFound')); + let tagFound = find(user.tags, (tag) => { + return tag.id === req.params.tagId; + }); + if (!tagFound) throw new NotFound(res.t('tagNotFound')); + + await user.update({ + $pull: { tags: { id: tagFound.id } }, + }).exec(); // Remove from all the tasks TODO test await Tasks.Task.update({ userId: user._id, }, { $pull: { - tags: tag.id, + tags: tagFound.id, }, }, {multi: true}).exec(); - await user.save(); res.respond(200, {}); }, }; diff --git a/website/server/controllers/api-v3/tasks.js b/website/server/controllers/api-v3/tasks.js index 9c483b3591..60d3b67774 100644 --- a/website/server/controllers/api-v3/tasks.js +++ b/website/server/controllers/api-v3/tasks.js @@ -581,12 +581,17 @@ api.scoreTask = { // TODO move to common code? if (task.type === 'todo') { if (!wasCompleted && task.completed) { - removeFromArray(user.tasksOrder.todos, task._id); - } else if (wasCompleted && !task.completed) { - let hasTask = removeFromArray(user.tasksOrder.todos, task._id); - if (!hasTask) { - user.tasksOrder.todos.push(task._id); - } // If for some reason it hadn't been removed previously don't do anything + // @TODO: mongoose's push and pull should be atomic and help with + // our concurrency issues. If not, we need to use this update $pull and $push + // await user.update({ + // $pull: { 'tasksOrder.todos': task._id }, + // }).exec(); + user.tasksOrder.todos.pull(task._id); + } else if (wasCompleted && !task.completed && user.tasksOrder.todos.indexOf(task._id) === -1) { + // await user.update({ + // $push: { 'tasksOrder.todos': task._id }, + // }).exec(); + user.tasksOrder.todos.push(task._id); } } @@ -685,11 +690,28 @@ api.moveTask = { if (!task) throw new NotFound(res.t('taskNotFound')); if (task.type === 'todo' && task.completed) throw new BadRequest(res.t('cantMoveCompletedTodo')); - let order = user.tasksOrder[`${task.type}s`]; + // In memory updates + let order = user.tasksOrder[`${task.type}s`]; moveTask(order, task._id, to); - await user.save(); + // Server updates + // @TODO: maybe bulk op? + let pullQuery = { $pull: {} }; + pullQuery.$pull[`tasksOrder.${task.type}s`] = task.id; + await user.update(pullQuery).exec(); + + // Handle push to bottom + let position = to; + if (to === -1) position = [`tasksOrder.${task.type}s`].length - 1; + + let updateQuery = { $push: {} }; + updateQuery.$push[`tasksOrder.${task.type}s`] = { + $each: [task._id], + $position: position, + }; + await user.update(updateQuery).exec(); + res.respond(200, order); }, }; @@ -1245,7 +1267,12 @@ api.deleteTask = { if (task.type !== 'todo' || !task.completed) { removeFromArray((challenge || user).tasksOrder[`${task.type}s`], taskId); - await Bluebird.all([(challenge || user).save(), task.remove()]); + + let pullQuery = {$pull: {}}; + pullQuery.$pull[`tasksOrder.${task.type}s`] = task._id; + let taskOrderUpdate = (challenge || user).update(pullQuery).exec(); + + await Bluebird.all([taskOrderUpdate, task.remove()]); } else { await task.remove(); } diff --git a/website/server/libs/cron.js b/website/server/libs/cron.js index 5686948b3b..398b88fa98 100644 --- a/website/server/libs/cron.js +++ b/website/server/libs/cron.js @@ -447,7 +447,6 @@ export function cron (options = {}) { // First remove a possible previous cron notification // we don't want to flood the users with many cron notifications at once - let oldCronNotif = user.notifications.toObject().find((notif, index) => { if (notif.type === 'CRON') { user.notifications.splice(index, 1); diff --git a/website/server/libs/taskManager.js b/website/server/libs/taskManager.js index fcf580e7e5..154e07e61c 100644 --- a/website/server/libs/taskManager.js +++ b/website/server/libs/taskManager.js @@ -77,6 +77,8 @@ export async function createTasks (req, res, options = {}) { let toSave = Array.isArray(req.body) ? req.body : [req.body]; + let taskOrderToAdd = {}; + toSave = toSave.map(taskData => { // Validate that task.type is valid if (!taskData || Tasks.tasksTypes.indexOf(taskData.type) === -1) throw new BadRequest(res.t('invalidTaskType')); @@ -103,11 +105,23 @@ export async function createTasks (req, res, options = {}) { if (validationErrors) throw validationErrors; // Otherwise update the user/challenge/group - owner.tasksOrder[`${taskType}s`].unshift(newTask._id); + if (!taskOrderToAdd[`${taskType}s`]) taskOrderToAdd[`${taskType}s`] = []; + taskOrderToAdd[`${taskType}s`].unshift(newTask._id); return newTask; }); + // Push all task ids + let taskOrderUpdateQuery = {$push: {}}; + for (let taskType in taskOrderToAdd) { + taskOrderUpdateQuery.$push[`tasksOrder.${taskType}`] = { + $each: taskOrderToAdd[taskType], + $position: 0, + }; + } + + await owner.update(taskOrderUpdateQuery).exec(); + // tasks with aliases need to be validated asyncronously await _validateTaskAlias(toSave, res);