From a5ce9fc175b835e855932a81e609d44d45663301 Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Mon, 18 Sep 2017 15:01:07 -0400 Subject: [PATCH] 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 | 15 ++++++ spec/Parse.Push.spec.js | 2 + spec/PushController.spec.js | 48 +++++++++++++---- spec/PushWorker.spec.js | 31 ++++------- src/Controllers/DatabaseController.js | 16 +++++- src/Push/PushWorker.js | 2 +- src/StatusHandler.js | 78 +++++++++++++++++++++------ src/cloud-code/Parse.Cloud.js | 12 +---- src/triggers.js | 15 ++++++ 9 files changed, 161 insertions(+), 58 deletions(-) diff --git a/spec/CloudCode.spec.js b/spec/CloudCode.spec.js index a1250bbe..5c3ca5ae 100644 --- a/spec/CloudCode.spec.js +++ b/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(); + }); }); diff --git a/spec/Parse.Push.spec.js b/spec/Parse.Push.spec.js index d495356e..a5771aef 100644 --- a/spec/Parse.Push.spec.js +++ b/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', diff --git a/spec/PushController.spec.js b/spec/PushController.spec.js index 70eca6d1..aa607389 100644 --- a/spec/PushController.spec.js +++ b/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) => { - console.error(err); - fail('should not fail'); - done(); - }); + }).then(done).catch(done.err); }); it('should not enqueue push when device token is not set', (done) => { diff --git a/spec/PushWorker.spec.js b/spec/PushWorker.spec.js index 100869be..8848ddca 100644 --- a/spec/PushWorker.spec.js +++ b/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 spy = spyOn(config.database, "update").and.callThrough(); + const handler = pushStatusHandler(config); + 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'); - const updatePayload = lastCall.args[2]; - expect(updatePayload.updatedAt instanceof Date).toBeTruthy(); - // remove the updatedAt as not testable - delete updatePayload.updatedAt; - + expect(lastCall.args[0]).toBe('PUT'); + expect(lastCall.args[1]).toBe(`classes/_PushStatus/${handler.objectId}`); 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'); - const updatePayload = lastCall.args[2]; - expect(updatePayload.updatedAt instanceof Date).toBeTruthy(); - // remove the updatedAt as not testable - delete updatePayload.updatedAt; - + expect(lastCall.args[1]).toBe(`classes/_PushStatus/${handler.objectId}`); expect(lastCall.args[2]).toEqual({ numSent: { __op: 'Increment', amount: 1 }, numFailed: { __op: 'Increment', amount: 1 }, diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 1867fff4..c1cdfdad 100644 --- a/src/Controllers/DatabaseController.js +++ b/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); diff --git a/src/Push/PushWorker.js b/src/Push/PushWorker.js index deedaaed..e3abf472 100644 --- a/src/Push/PushWorker.js +++ b/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) { diff --git a/src/StatusHandler.js b/src/StatusHandler.js index eb4cbbfb..b29e580d 100644 --- a/src/StatusHandler.js +++ b/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'}, - updatedAt: new Date() + count: {__op: 'Delete'} }); } const fail = function(err) { + if (typeof err === 'string') { + err = { message: err }; + } const update = { - errorMessage: JSON.stringify(err), - status: 'failed', - updatedAt: new Date() + errorMessage: err, + status: 'failed' } return handler.update({ objectId }, update); } - return Object.freeze({ - objectId, + const rval = { setInitial, setRunning, trackSent, complete, fail - }) + }; + + // define objectId to be dynamic + Object.defineProperty(rval, "objectId", { + get: () => objectId + }); + + return Object.freeze(rval); } diff --git a/src/cloud-code/Parse.Cloud.js b/src/cloud-code/Parse.Cloud.js index 8cfb54ef..0bffba29 100644 --- a/src/cloud-code/Parse.Cloud.js +++ b/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 = {}; diff --git a/src/triggers.js b/src/triggers.js index c5df1e25..f15b7e33 100644 --- a/src/triggers.js +++ b/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;