Fixed concurrency issues with push devices (#10598)

* Fixed concurrency issues with push devices

* Fixed push notificaiton response and model adding

test/api/v3/integration/user/POST-user_push_device.test.js

@@ -50,11 +50,24 @@ describe('POST /user/push-devices', () => {
   });
 
   it('adds a push device to the user', async () => {
-    let response = await user.post('/user/push-devices', {type, regId});
+    const response = await user.post('/user/push-devices', {type, regId});
     await user.sync();
 
     expect(response.message).to.equal(t('pushDeviceAdded'));
+    expect(response.data[0].type).to.equal(type);
+    expect(response.data[0].regId).to.equal(regId);
     expect(user.pushDevices[0].type).to.equal(type);
     expect(user.pushDevices[0].regId).to.equal(regId);
   });
+
+  it('removes a push device to the user', async () => {
+    await user.post('/user/push-devices', {type, regId});
+
+    const response = await user.del(`/user/push-devices/${regId}`);
+    await user.sync();
+
+    expect(response.message).to.equal(t('pushDeviceRemoved'));
+    expect(response.data[0]).to.not.exist;
+    expect(user.pushDevices[0]).to.not.exist;
+  });
 });

website/server/controllers/api-v3/pushNotifications.js

@@ -3,6 +3,7 @@ import {
   NotAuthorized,
   NotFound,
 } from '../../libs/errors';
+import { model as PushDevice } from '../../models/pushDevice';
 
 let api = {};
 
@@ -25,17 +26,17 @@ api.addPushDevice = {
     userFieldsToExclude: ['inbox'],
   })],
   async handler (req, res) {
-    let user = res.locals.user;
+    const user = res.locals.user;
 
     req.checkBody('regId', res.t('regIdRequired')).notEmpty();
     req.checkBody('type', res.t('typeRequired')).notEmpty().isIn(['ios', 'android']);
 
-    let validationErrors = req.validationErrors();
+    const validationErrors = req.validationErrors();
     if (validationErrors) throw validationErrors;
 
-    let pushDevices = user.pushDevices;
+    const pushDevices = user.pushDevices;
 
-    let item = {
+    const item = {
       regId: req.body.regId,
       type: req.body.type,
     };
@@ -44,9 +45,14 @@ api.addPushDevice = {
       throw new NotAuthorized(res.t('pushDeviceAlreadyAdded'));
     }
 
-    pushDevices.push(item);
+    // Concurrency safe update
+    const pushDevice = (new PushDevice(item)).toJSON(); // Create a mongo doc
+    await user.update({
+      $push: { pushDevices: pushDevice },
+    }).exec();
 
-    await user.save();
+    // Update the response
+    user.pushDevices.push(pushDevice);
 
     res.respond(200, user.pushDevices, res.t('pushDeviceAdded'));
   },
@@ -70,16 +76,18 @@ api.removePushDevice = {
     userFieldsToExclude: ['inbox'],
   })],
   async handler (req, res) {
-    let user = res.locals.user;
+    const user = res.locals.user;
 
     req.checkParams('regId', res.t('regIdRequired')).notEmpty();
-    let validationErrors = req.validationErrors();
+
+    const validationErrors = req.validationErrors();
     if (validationErrors) throw validationErrors;
-    let regId = req.params.regId;
 
-    let pushDevices = user.pushDevices;
+    const regId = req.params.regId;
 
-    let indexOfPushDevice = pushDevices.findIndex((element) => {
+    const pushDevices = user.pushDevices;
+
+    const indexOfPushDevice = pushDevices.findIndex((element) => {
       return element.regId === regId;
     });
 
@@ -87,8 +95,12 @@ api.removePushDevice = {
       throw new NotFound(res.t('pushDeviceNotFound'));
     }
 
+    // Concurrency safe update
+    const pullQuery = { $pull: { pushDevices: { $elemMatch: { regId } } } };
+    await user.update(pullQuery).exec();
+
+    // Update the response
     pushDevices.splice(indexOfPushDevice, 1);
-    await user.save();
 
     res.respond(200, user.pushDevices, res.t('pushDeviceRemoved'));
   },