Refactor pushStatusHandler to use Parse instead of direct access (#4173)

* Refactors pushStatusHandler to use HTTP interface so we can bind CloudCode hooks

* Handle correctly nested dot atomic operations

* Better handling of restricted class names, add support for afterSave _PushStatus

* Adds simple testing for afterSave(PushStatus)

* Reverts jobStatusHandler

* Addresses fixes

* adds delays to all methods

spec/CloudCode.spec.js

@@ -1627,4 +1627,19 @@ describe('afterFind hooks', () => {
       })
       .then(() => done());
   });
+
+  it('should validate triggers correctly', () => {
+    expect(() => {
+      Parse.Cloud.beforeSave('_Session', () => {});
+    }).toThrow('Triggers are not supported for _Session class.');
+    expect(() => {
+      Parse.Cloud.afterSave('_Session', () => {});
+    }).toThrow('Triggers are not supported for _Session class.');
+    expect(() => {
+      Parse.Cloud.beforeSave('_PushStatus', () => {});
+    }).toThrow('Only afterSave is allowed on _PushStatus');
+    expect(() => {
+      Parse.Cloud.afterSave('_PushStatus', () => {});
+    }).not.toThrow();
+  });
 });

spec/Parse.Push.spec.js

@@ -127,6 +127,7 @@ describe('Parse.Push', () => {
           alert: 'Hello world!'
         }
       }, {useMasterKey: true}))
+      .then(() => delayPromise(500))
       .then(() => {
         request.get({
           url: 'http://localhost:8378/1/classes/_PushStatus',
@@ -155,6 +156,7 @@ describe('Parse.Push', () => {
           alert: 'Hello world!'
         }
       }, {useMasterKey: true}))
+      .then(() => delayPromise(500)) // put a delay as we keep writing
       .then(() => {
         request.get({
           url: 'http://localhost:8378/1/classes/_PushStatus',

spec/PushController.spec.js

@@ -388,6 +388,11 @@ describe('PushController', () => {
   });
 
   it('properly creates _PushStatus', (done) => {
+    const pushStatusAfterSave = {
+      handler: function() {}
+    };
+    const spy = spyOn(pushStatusAfterSave, 'handler').and.callThrough();
+    Parse.Cloud.afterSave('_PushStatus', pushStatusAfterSave.handler);
     var installations = [];
     while(installations.length != 10) {
       const installation = new Parse.Object("_Installation");
@@ -466,8 +471,36 @@ describe('PushController', () => {
         return query.find();
       }).catch((error) => {
         expect(error.code).toBe(119);
-        done();
+      })
-      });
+      .then(() => {
+        function getPushStatus(callIndex) {
+          return spy.calls.all()[callIndex].args[0].object;
+        }
+        expect(spy).toHaveBeenCalled();
+        expect(spy.calls.count()).toBe(4);
+        const allCalls = spy.calls.all();
+        allCalls.forEach((call) => {
+          expect(call.args.length).toBe(2);
+          const object = call.args[0].object;
+          expect(object instanceof Parse.Object).toBe(true);
+        });
+        expect(getPushStatus(0).get('status')).toBe('pending');
+        expect(getPushStatus(1).get('status')).toBe('running');
+        expect(getPushStatus(1).get('numSent')).toBe(0);
+        expect(getPushStatus(2).get('status')).toBe('running');
+        expect(getPushStatus(2).get('numSent')).toBe(10);
+        expect(getPushStatus(2).get('numFailed')).toBe(5);
+        // Those are updated from a nested . operation, this would
+        // not render correctly before
+        expect(getPushStatus(2).get('failedPerType')).toEqual({
+          android: 5
+        });
+        expect(getPushStatus(2).get('sentPerType')).toEqual({
+          ios: 10
+        });
+        expect(getPushStatus(3).get('status')).toBe('succeeded');
+      })
+      .then(done).catch(done.fail);
   });
 
   it('should properly report failures in _PushStatus', (done) => {
@@ -710,7 +743,7 @@ describe('PushController', () => {
     }).then(() => {
       return Parse.Object.saveAll(installations).then(() => {
         return pushController.sendPush(payload, {}, config, auth);
-      });
+      }).then(() => new Promise(resolve => setTimeout(resolve, 300)));
     }).then(() => {
       const query = new Parse.Query('_PushStatus');
       return query.find({useMasterKey: true}).then((results) => {
@@ -776,20 +809,15 @@ describe('PushController', () => {
       var config = new Config(Parse.applicationId);
       return Parse.Object.saveAll(installations).then(() => {
         return pushController.sendPush(payload, {}, config, auth);
-      }).then(() => new Promise(resolve => setTimeout(resolve, 100)));
+      }).then(() => new Promise(resolve => setTimeout(resolve, 300)));
     }).then(() => {
       const query = new Parse.Query('_PushStatus');
       return query.find({useMasterKey: true}).then((results) => {
         expect(results.length).toBe(1);
         const pushStatus = results[0];
         expect(pushStatus.get('status')).toBe('scheduled');
-        done();
       });
-    }).catch((err) => {
+    }).then(done).catch(done.err);
-      console.error(err);
-      fail('should not fail');
-      done();
-    });
   });
 
   it('should not enqueue push when device token is not set', (done) => {

spec/PushWorker.spec.js

@@ -165,7 +165,7 @@ describe('PushWorker', () => {
       const spy = spyOn(config.database, "update").and.callFake(() => {
         return Promise.resolve();
       });
-      handler.trackSent([
+      const toAwait = handler.trackSent([
         {
           transmitted: false,
           device: {
@@ -239,13 +239,13 @@ describe('PushWorker', () => {
       expect(lastCall.args[2]).toEqual({
         deviceToken: { '__op': "Delete" }
       });
-      done();
+      toAwait.then(done).catch(done);
     });
 
     it('tracks push status per UTC offsets', (done) => {
       const config = new Config('test');
-      const handler = pushStatusHandler(config, 'ABCDEF1234');
+      const handler = pushStatusHandler(config);
-      const spy = spyOn(config.database, "update").and.callThrough();
+      const spy = spyOn(Parse, "_request").and.callThrough();
       const UTCOffset = 1;
       handler.setInitial().then(() => {
         return handler.trackSent([
@@ -266,14 +266,9 @@ describe('PushWorker', () => {
         ], UTCOffset)
       }).then(() => {
         expect(spy).toHaveBeenCalled();
-        expect(spy.calls.count()).toBe(1);
         const lastCall = spy.calls.mostRecent();
-        expect(lastCall.args[0]).toBe('_PushStatus');
+        expect(lastCall.args[0]).toBe('PUT');
-        const updatePayload = lastCall.args[2];
+        expect(lastCall.args[1]).toBe(`classes/_PushStatus/${handler.objectId}`);
-        expect(updatePayload.updatedAt instanceof Date).toBeTruthy();
-        // remove the updatedAt as not testable
-        delete updatePayload.updatedAt;
-
         expect(lastCall.args[2]).toEqual({
           numSent: { __op: 'Increment', amount: 1 },
           numFailed: { __op: 'Increment', amount: 1 },
@@ -284,7 +279,7 @@ describe('PushWorker', () => {
           count: { __op: 'Increment', amount: -2 },
         });
         const query = new Parse.Query('_PushStatus');
-        return query.get('ABCDEF1234', { useMasterKey: true });
+        return query.get(handler.objectId, { useMasterKey: true });
       }).then((pushStatus) => {
         const sentPerUTCOffset = pushStatus.get('sentPerUTCOffset');
         expect(sentPerUTCOffset['1']).toBe(1);
@@ -315,7 +310,7 @@ describe('PushWorker', () => {
         ], UTCOffset)
       }).then(() => {
         const query = new Parse.Query('_PushStatus');
-        return query.get('ABCDEF1234', { useMasterKey: true });
+        return query.get(handler.objectId, { useMasterKey: true });
       }).then((pushStatus) => {
         const sentPerUTCOffset = pushStatus.get('sentPerUTCOffset');
         expect(sentPerUTCOffset['1']).toBe(3);
@@ -330,7 +325,7 @@ describe('PushWorker', () => {
       spyOn(config.database, "create").and.callFake(() => {
         return Promise.resolve();
       });
-      const spy = spyOn(config.database, "update").and.callFake(() => {
+      const spy = spyOn(Parse, "_request").and.callFake(() => {
         return Promise.resolve();
       });
       const UTCOffset = -6;
@@ -353,14 +348,8 @@ describe('PushWorker', () => {
         },
       ], UTCOffset).then(() => {
         expect(spy).toHaveBeenCalled();
-        expect(spy.calls.count()).toBe(1);
         const lastCall = spy.calls.mostRecent();
-        expect(lastCall.args[0]).toBe('_PushStatus');
+        expect(lastCall.args[1]).toBe(`classes/_PushStatus/${handler.objectId}`);
-        const updatePayload = lastCall.args[2];
-        expect(updatePayload.updatedAt instanceof Date).toBeTruthy();
-        // remove the updatedAt as not testable
-        delete updatePayload.updatedAt;
-
         expect(lastCall.args[2]).toEqual({
           numSent: { __op: 'Increment', amount: 1 },
           numFailed: { __op: 'Increment', amount: 1 },

src/Controllers/DatabaseController.js

@@ -308,6 +308,19 @@ DatabaseController.prototype.update = function(className, query, update, {
     });
 };
 
+function expandResultOnKeyPath(object, key, value) {
+  if (key.indexOf('.') < 0) {
+    object[key] = value[key];
+    return object;
+  }
+  const path = key.split('.');
+  const firstKey = path[0];
+  const nextPath = path.slice(1).join('.');
+  object[firstKey] = expandResultOnKeyPath(object[firstKey] || {}, nextPath, value[firstKey]);
+  delete object[key];
+  return object;
+}
+
 function sanitizeDatabaseResult(originalObject, result) {
   const response = {};
   if (!result) {
@@ -319,7 +332,8 @@ function sanitizeDatabaseResult(originalObject, result) {
     if (keyUpdate && typeof keyUpdate === 'object' && keyUpdate.__op
       && ['Add', 'AddUnique', 'Remove', 'Increment'].indexOf(keyUpdate.__op) > -1) {
       // only valid ops that produce an actionable result
-      response[key] = result[key];
+      // the op may have happend on a keypath
+      expandResultOnKeyPath(response, key, result);
     }
   });
   return Promise.resolve(response);

src/Push/PushWorker.js

@@ -52,6 +52,7 @@ export class PushWorker {
     const auth = master(config);
     const where = utils.applyDeviceTokenExists(query.where);
     delete query.where;
+    pushStatus = pushStatusHandler(config, pushStatus.objectId);
     return rest.find(config, auth, '_Installation', where, query).then(({results}) => {
       if (results.length == 0) {
         return;
@@ -63,7 +64,6 @@ export class PushWorker {
   }
 
   sendToAdapter(body: any, installations: any[], pushStatus: any, config: Config, UTCOffset: ?any): Promise<*> {
-    pushStatus = pushStatusHandler(config, pushStatus.objectId);
     // Check if we have locales in the push body
     const locales = utils.getLocalesFromPush(body);
     if (locales.length > 0) {

src/StatusHandler.js

@@ -1,5 +1,6 @@
 import { md5Hash, newObjectId } from './cryptoUtils';
 import { logger }               from './logger';
+import Parse                    from 'parse/node';
 
 const PUSH_STATUS_COLLECTION = '_PushStatus';
 const JOB_STATUS_COLLECTION = '_JobStatus';
@@ -50,6 +51,47 @@ function statusHandler(className, database) {
   })
 }
 
+function restStatusHandler(className) {
+  let lastPromise = Promise.resolve();
+
+  function create(object) {
+    lastPromise = lastPromise.then(() => {
+      return Parse._request(
+        'POST',
+        `classes/${className}`,
+        object,
+        { useMasterKey: true }
+      ).then((result) => {
+        // merge the objects
+        const response = Object.assign({}, object, result);
+        return Promise.resolve(response);
+      });
+    });
+    return lastPromise;
+  }
+
+  function update(where, object) {
+    // TODO: when we have updateWhere, use that for proper interfacing
+    lastPromise = lastPromise.then(() => {
+      return Parse._request(
+        'PUT',
+        `classes/${className}/${where.objectId}`,
+        object,
+        { useMasterKey: true }).then((result) => {
+        // merge the objects
+        const response = Object.assign({}, object, result);
+        return Promise.resolve(response);
+      });
+    });
+    return lastPromise;
+  }
+
+  return Object.freeze({
+    create,
+    update
+  })
+}
+
 export function jobStatusHandler(config) {
   let jobStatus;
   const objectId = newObjectId(config.objectIdSize);
@@ -103,11 +145,12 @@ export function jobStatusHandler(config) {
   });
 }
 
-export function pushStatusHandler(config, objectId = newObjectId(config.objectIdSize)) {
+export function pushStatusHandler(config, existingObjectId) {
 
   let pushStatus;
   const database = config.database;
-  const handler = statusHandler(PUSH_STATUS_COLLECTION, database);
+  const handler = restStatusHandler(PUSH_STATUS_COLLECTION);
+  let objectId = existingObjectId;
   const setInitial = function(body = {}, where, options = {source: 'rest'}) {
     const now = new Date();
     let pushTime = now.toISOString();
@@ -133,8 +176,6 @@ export function pushStatusHandler(config, objectId = newObjectId(config.objectId
       pushHash = 'd41d8cd98f00b204e9800998ecf8427e';
     }
     const object = {
-      objectId,
-      createdAt: now,
       pushTime,
       query: JSON.stringify(where),
       payload: payloadString,
@@ -148,7 +189,8 @@ export function pushStatusHandler(config, objectId = newObjectId(config.objectId
       ACL: {}
     }
 
-    return handler.create(object).then(() => {
+    return handler.create(object).then((result) => {
+      objectId = result.objectId;
       pushStatus = {
         objectId
       };
@@ -159,12 +201,11 @@ export function pushStatusHandler(config, objectId = newObjectId(config.objectId
   const setRunning = function(count) {
     logger.verbose(`_PushStatus ${objectId}: sending push to %d installations`, count);
     return handler.update({status:"pending", objectId: objectId},
-      {status: "running", updatedAt: new Date(), count });
+      {status: "running", count });
   }
 
   const trackSent = function(results, UTCOffset, cleanupInstallations = process.env.PARSE_SERVER_CLEANUP_INVALID_INSTALLATIONS) {
     const update = {
-      updatedAt: new Date(),
       numSent: 0,
       numFailed: 0
     };
@@ -236,26 +277,33 @@ export function pushStatusHandler(config, objectId = newObjectId(config.objectId
   const complete = function() {
     return handler.update({ objectId }, {
       status: 'succeeded',
-      count: {__op: 'Delete'},
+      count: {__op: 'Delete'}
-      updatedAt: new Date()
     });
   }
 
   const fail = function(err) {
+    if (typeof err === 'string') {
+      err = { message: err };
+    }
     const update = {
-      errorMessage: JSON.stringify(err),
+      errorMessage: err,
-      status: 'failed',
+      status: 'failed'
-      updatedAt: new Date()
     }
     return handler.update({ objectId }, update);
   }
 
-  return Object.freeze({
+  const rval = {
-    objectId,
     setInitial,
     setRunning,
     trackSent,
     complete,
     fail
-  })
+  };
+
+  // define objectId to be dynamic
+  Object.defineProperty(rval, "objectId", {
+    get: () => objectId
+  });
+
+  return Object.freeze(rval);
 }

src/cloud-code/Parse.Cloud.js

@@ -1,19 +1,11 @@
 import { Parse }     from 'parse/node';
 import * as triggers from '../triggers';
 
-function validateClassNameForTriggers(className) {
-  const restrictedClassNames = [ '_Session' ];
-  if (restrictedClassNames.indexOf(className) != -1) {
-    throw `Triggers are not supported for ${className} class.`;
-  }
-  return className;
-}
-
 function getClassName(parseClass) {
   if (parseClass && parseClass.className) {
-    return validateClassNameForTriggers(parseClass.className);
+    return parseClass.className;
   }
-  return validateClassNameForTriggers(parseClass);
+  return parseClass;
 }
 
 var ParseCloud = {};

src/triggers.js

@@ -30,6 +30,20 @@ const baseStore = function() {
   });
 };
 
+function validateClassNameForTriggers(className, type) {
+  const restrictedClassNames = [ '_Session' ];
+  if (restrictedClassNames.indexOf(className) != -1) {
+    throw `Triggers are not supported for ${className} class.`;
+  }
+  if (type == Types.beforeSave && className === '_PushStatus') {
+    // _PushStatus uses undocumented nested key increment ops
+    // allowing beforeSave would mess up the objects big time
+    // TODO: Allow proper documented way of using nested increment ops
+    throw 'Only afterSave is allowed on _PushStatus';
+  }
+  return className;
+}
+
 const _triggerStore = {};
 
 export function addFunction(functionName, handler, validationHandler, applicationId) {
@@ -46,6 +60,7 @@ export function addJob(jobName, handler, applicationId) {
 }
 
 export function addTrigger(type, className, handler, applicationId) {
+  validateClassNameForTriggers(className, type);
   applicationId = applicationId || Parse.applicationId;
   _triggerStore[applicationId] =  _triggerStore[applicationId] || baseStore();
   _triggerStore[applicationId].Triggers[type][className] = handler;