From bf424573a4dca275d443239eeb6feaa007b45c47 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Mon, 28 May 2018 13:38:59 +0200 Subject: [PATCH] Members: user .lean() to improve performances (#10399) * perf(members): use lean where possible * fix unit tests * fix unit tests and update calls to old function * simplify code and add tests --- package-lock.json | 36 +++++++++++++------ package.json | 2 +- test/api/v3/unit/libs/webhooks.test.js | 23 ++++++------ test/api/v3/unit/models/user.test.js | 37 +++++++++++++++++++- website/server/controllers/api-v3/members.js | 12 +++---- website/server/controllers/api-v3/user.js | 5 ++- website/server/libs/webhook.js | 5 ++- website/server/models/user/methods.js | 20 ++++++++--- 8 files changed, 101 insertions(+), 39 deletions(-) diff --git a/package-lock.json b/package-lock.json index d9c5787303..caa5728379 100644 --- a/package-lock.json +++ b/package-lock.json @@ -151,7 +151,7 @@ }, "@sinonjs/formatio": { "version": "2.0.0", - "resolved": "https://registry.npmjs.org/@sinonjs/formatio/-/formatio-2.0.0.tgz", + "resolved": "http://registry.npmjs.org/@sinonjs/formatio/-/formatio-2.0.0.tgz", "integrity": "sha512-ls6CAMA6/5gG+O/IdsBcblvnd8qcO/l1TYoNeAzp3wcISOxlPXQEus0mLcdwazEkWjaBdaJ3TaxmNgCLWwvWzg==", "dev": true, "requires": { @@ -3765,7 +3765,7 @@ }, "compression": { "version": "1.7.2", - "resolved": "https://registry.npmjs.org/compression/-/compression-1.7.2.tgz", + "resolved": "http://registry.npmjs.org/compression/-/compression-1.7.2.tgz", "integrity": "sha1-qv+81qr4VLROuygDU9WtFlH1mmk=", "requires": { "accepts": "1.3.5", @@ -7544,6 +7544,7 @@ "version": "1.1.3", "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-1.1.3.tgz", "integrity": "sha512-WIr7iDkdmdbxu/Gh6eKEZJL6KPE74/5MEsf2whTOFNxbIoIixogroLdKYqB6FDav4Wavh/lZdzzd3b2KxIXC5Q==", + "optional": true, "requires": { "nan": "2.6.2", "node-pre-gyp": "0.6.39" @@ -7580,6 +7581,7 @@ "version": "1.1.4", "resolved": "https://registry.npmjs.org/are-we-there-yet/-/are-we-there-yet-1.1.4.tgz", "integrity": "sha1-u13KOCu5TwXhUZQ3PRb9O6HKEQ0=", + "optional": true, "requires": { "delegates": "1.0.0", "readable-stream": "2.2.9" @@ -7813,6 +7815,7 @@ "version": "1.0.5", "resolved": "https://registry.npmjs.org/fstream-ignore/-/fstream-ignore-1.0.5.tgz", "integrity": "sha1-nDHa40dnAY/h0kmyTa2mfQktoQU=", + "optional": true, "requires": { "fstream": "1.0.11", "inherits": "2.0.3", @@ -7823,6 +7826,7 @@ "version": "2.7.4", "resolved": "https://registry.npmjs.org/gauge/-/gauge-2.7.4.tgz", "integrity": "sha1-LANAXHU4w51+s3sxcCLjJfsBi/c=", + "optional": true, "requires": { "aproba": "1.1.1", "console-control-strings": "1.1.0", @@ -7911,6 +7915,7 @@ "version": "1.1.1", "resolved": "https://registry.npmjs.org/http-signature/-/http-signature-1.1.1.tgz", "integrity": "sha1-33LiZwZs0Kxn+3at+OE0qPvPkb8=", + "optional": true, "requires": { "assert-plus": "0.2.0", "jsprim": "1.4.0", @@ -8008,6 +8013,7 @@ "version": "1.4.0", "resolved": "https://registry.npmjs.org/jsprim/-/jsprim-1.4.0.tgz", "integrity": "sha1-o7h+QCmNjDgFUtjMdiigu5WiKRg=", + "optional": true, "requires": { "assert-plus": "1.0.0", "extsprintf": "1.0.2", @@ -8067,6 +8073,7 @@ "version": "0.6.39", "resolved": "https://registry.npmjs.org/node-pre-gyp/-/node-pre-gyp-0.6.39.tgz", "integrity": "sha512-OsJV74qxnvz/AMGgcfZoDaeDXKD3oY3QVIbBmwszTFkRisTSXbMQyn4UWzUMOtA5SVhrBZOTp0wcoSBgfMfMmQ==", + "optional": true, "requires": { "detect-libc": "1.0.2", "hawk": "3.1.3", @@ -8095,6 +8102,7 @@ "version": "4.1.0", "resolved": "https://registry.npmjs.org/npmlog/-/npmlog-4.1.0.tgz", "integrity": "sha512-ocolIkZYZt8UveuiDS0yAkkIjid1o7lPG8cYm05yNYzBn8ykQtaiPMEGp8fY9tKdDgm8okpdKzkvu1y9hUYugA==", + "optional": true, "requires": { "are-we-there-yet": "1.1.4", "console-control-strings": "1.1.0", @@ -8215,6 +8223,7 @@ "version": "2.81.0", "resolved": "https://registry.npmjs.org/request/-/request-2.81.0.tgz", "integrity": "sha1-xpKJRqDgbF+Nb4qTM0af/aRimKA=", + "optional": true, "requires": { "aws-sign2": "0.6.0", "aws4": "1.6.0", @@ -8356,6 +8365,7 @@ "version": "3.4.0", "resolved": "https://registry.npmjs.org/tar-pack/-/tar-pack-3.4.0.tgz", "integrity": "sha1-I74tf2cagzk3bL2wuP4/3r8xeYQ=", + "optional": true, "requires": { "debug": "2.6.8", "fstream": "1.0.11", @@ -8405,12 +8415,14 @@ "uuid": { "version": "3.0.1", "resolved": "https://registry.npmjs.org/uuid/-/uuid-3.0.1.tgz", - "integrity": "sha1-ZUS7ot/ajBzxfmKaOjBeK7H+5sE=" + "integrity": "sha1-ZUS7ot/ajBzxfmKaOjBeK7H+5sE=", + "optional": true }, "verror": { "version": "1.3.6", "resolved": "https://registry.npmjs.org/verror/-/verror-1.3.6.tgz", "integrity": "sha1-z/XfEpRtKX0rqu+qJoniW+AcAFw=", + "optional": true, "requires": { "extsprintf": "1.0.2" } @@ -8419,6 +8431,7 @@ "version": "1.1.2", "resolved": "https://registry.npmjs.org/wide-align/-/wide-align-1.1.2.tgz", "integrity": "sha512-ijDLlyQ7s6x1JgCLur53osjm/UXUYD9+0PbYKrBsYisYXzCxN+HC3mYDNy/dWdmf3AwqwU3CXwDCvsNgGK1S0w==", + "optional": true, "requires": { "string-width": "1.0.2" } @@ -11123,9 +11136,9 @@ "dev": true }, "kareem": { - "version": "2.0.7", - "resolved": "https://registry.npmjs.org/kareem/-/kareem-2.0.7.tgz", - "integrity": "sha512-p8+lEpsNs4N0fvNOC1/zzDO0wDrD3Pb1G+OwfIG+gKVK3MyY5jeaGYh+9Qx6jb4fEG2b3E6U98vaE9MH7Gilsw==" + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/kareem/-/kareem-2.1.0.tgz", + "integrity": "sha512-ycoMY1tVkcH1/NaxGn2erZaUC3CodmX7Fl6DUVXjN73+uecWYTaaldRkxNY3HeSKQnQTWnoxRKnZfVHcB8tIWg==" }, "karma": { "version": "2.0.2", @@ -13399,13 +13412,13 @@ } }, "mongoose": { - "version": "5.1.1", - "resolved": "https://registry.npmjs.org/mongoose/-/mongoose-5.1.1.tgz", - "integrity": "sha512-pmVX//61yX8hV/Db790Cym8CGYd29OYQROYx6ve6ab4LbUW0+GehiBVDlnE9Tvogaz8nndHlVUb+iXwOBw/MNw==", + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/mongoose/-/mongoose-5.1.2.tgz", + "integrity": "sha512-k9hssPMgBnUYG5e9NoUbx/2ERDyelDY0Vf6BwjtmoETUhVT7pQUe1o+oelLLuHF3ZVY2qgienK8pnrI5pdvlxA==", "requires": { "async": "2.1.4", "bson": "1.0.6", - "kareem": "2.0.7", + "kareem": "2.1.0", "lodash.get": "4.4.2", "mongodb": "3.0.8", "mongoose-legacy-pluralize": "1.0.2", @@ -13613,7 +13626,8 @@ "nan": { "version": "2.6.2", "resolved": "https://registry.npmjs.org/nan/-/nan-2.6.2.tgz", - "integrity": "sha1-5P805slf37WuzAjeZZb0NgWn20U=" + "integrity": "sha1-5P805slf37WuzAjeZZb0NgWn20U=", + "optional": true }, "nanomatch": { "version": "1.2.9", diff --git a/package.json b/package.json index 529f9b0f7d..f4790f9386 100644 --- a/package.json +++ b/package.json @@ -61,7 +61,7 @@ "method-override": "^2.3.5", "moment": "^2.22.1", "moment-recur": "^1.0.7", - "mongoose": "^5.1.1", + "mongoose": "^5.1.2", "morgan": "^1.7.0", "nconf": "^0.10.0", "node-gcm": "^0.14.4", diff --git a/test/api/v3/unit/libs/webhooks.test.js b/test/api/v3/unit/libs/webhooks.test.js index 9e010c45b6..a426da76a6 100644 --- a/test/api/v3/unit/libs/webhooks.test.js +++ b/test/api/v3/unit/libs/webhooks.test.js @@ -7,6 +7,9 @@ import { questActivityWebhook, userActivityWebhook, } from '../../../../../website/server/libs/webhook'; +import { + model as User, +} from '../../../../../website/server/models/user'; import { generateUser, } from '../../../../helpers/api-unit.helper.js'; @@ -306,17 +309,6 @@ describe('webhooks', () => { return this; }, }, - addComputedStatsToJSONObj () { - let mockStats = Object.assign({ - maxHealth: 50, - maxMP: 103, - toNextLevel: 40, - }, this.stats); - - delete mockStats.toJSON; - - return mockStats; - }, }, task: { text: 'text', @@ -324,6 +316,15 @@ describe('webhooks', () => { direction: 'up', delta: 176, }; + + let mockStats = Object.assign({ + maxHealth: 50, + maxMP: 103, + toNextLevel: 40, + }, data.user.stats); + delete mockStats.toJSON; + + sandbox.stub(User, 'addComputedStatsToJSONObj').returns(mockStats); }); it('sends task and stats data', () => { diff --git a/test/api/v3/unit/models/user.test.js b/test/api/v3/unit/models/user.test.js index 00ea3f5607..2a3e7e6ad6 100644 --- a/test/api/v3/unit/models/user.test.js +++ b/test/api/v3/unit/models/user.test.js @@ -42,13 +42,48 @@ describe('User Model', () => { expect(userToJSON.stats.maxHealth).to.not.exist; expect(userToJSON.stats.toNextLevel).to.not.exist; - user.addComputedStatsToJSONObj(userToJSON.stats); + User.addComputedStatsToJSONObj(userToJSON.stats, userToJSON); expect(userToJSON.stats.maxMP).to.exist; expect(userToJSON.stats.maxHealth).to.equal(common.maxHealth); expect(userToJSON.stats.toNextLevel).to.equal(common.tnl(user.stats.lvl)); }); + it('can transform user object without mongoose helpers', async () => { + let user = new User(); + await user.save(); + let userToJSON = await User.findById(user._id).lean().exec(); + + expect(userToJSON.stats.maxMP).to.not.exist; + expect(userToJSON.stats.maxHealth).to.not.exist; + expect(userToJSON.stats.toNextLevel).to.not.exist; + expect(userToJSON.id).to.not.exist; + + User.transformJSONUser(userToJSON); + + expect(userToJSON.id).to.equal(userToJSON._id); + expect(userToJSON.stats.maxMP).to.not.exist; + expect(userToJSON.stats.maxHealth).to.not.exist; + expect(userToJSON.stats.toNextLevel).to.not.exist; + }); + + it('can transform user object without mongoose helpers (including computed stats)', async () => { + let user = new User(); + await user.save(); + let userToJSON = await User.findById(user._id).lean().exec(); + + expect(userToJSON.stats.maxMP).to.not.exist; + expect(userToJSON.stats.maxHealth).to.not.exist; + expect(userToJSON.stats.toNextLevel).to.not.exist; + + User.transformJSONUser(userToJSON, true); + + expect(userToJSON.id).to.equal(userToJSON._id); + expect(userToJSON.stats.maxMP).to.exist; + expect(userToJSON.stats.maxHealth).to.equal(common.maxHealth); + expect(userToJSON.stats.toNextLevel).to.equal(common.tnl(user.stats.lvl)); + }); + context('notifications', () => { it('can add notifications without data', () => { let user = new User(); diff --git a/website/server/controllers/api-v3/members.js b/website/server/controllers/api-v3/members.js index a09cda938a..421e2cf4bb 100644 --- a/website/server/controllers/api-v3/members.js +++ b/website/server/controllers/api-v3/members.js @@ -55,7 +55,7 @@ api.getMember = { // manually call toJSON with minimize: true so empty paths aren't returned let memberToJSON = member.toJSON({minimize: true}); - member.addComputedStatsToJSONObj(memberToJSON.stats); + User.addComputedStatsToJSONObj(memberToJSON.stats, member); res.respond(200, memberToJSON); }, @@ -282,16 +282,12 @@ function _getMembersForItem (type) { .sort({_id: 1}) .limit(limit) .select(fields) + .lean() .exec(); // manually call toJSON with minimize: true so empty paths aren't returned - let membersToJSON = members.map(member => { - let memberToJSON = member.toJSON({minimize: true}); - if (addComputedStats) member.addComputedStatsToJSONObj(memberToJSON.stats); - - return memberToJSON; - }); - res.respond(200, membersToJSON); + members.forEach(member => User.transformJSONUser(member, addComputedStats)); + res.respond(200, members); }; } diff --git a/website/server/controllers/api-v3/user.js b/website/server/controllers/api-v3/user.js index bf8dd9f75e..3bb1919292 100644 --- a/website/server/controllers/api-v3/user.js +++ b/website/server/controllers/api-v3/user.js @@ -8,6 +8,9 @@ import { basicFields as basicGroupFields, model as Group, } from '../../models/group'; +import { + model as User, +} from '../../models/user'; import * as Tasks from '../../models/task'; import _ from 'lodash'; import * as passwordUtils from '../../libs/password'; @@ -90,7 +93,7 @@ api.getUser = { let {daysMissed} = user.daysUserHasMissed(new Date(), req); userToJSON.needsCron = false; if (daysMissed > 0) userToJSON.needsCron = true; - user.addComputedStatsToJSONObj(userToJSON.stats); + User.addComputedStatsToJSONObj(userToJSON.stats, userToJSON); } return res.respond(200, userToJSON); diff --git a/website/server/libs/webhook.js b/website/server/libs/webhook.js index 691525a95a..7956953bc4 100644 --- a/website/server/libs/webhook.js +++ b/website/server/libs/webhook.js @@ -2,6 +2,9 @@ import got from 'got'; import { isURL } from 'validator'; import logger from './logger'; import nconf from 'nconf'; +import { + model as User, +} from '../models/user'; const IS_PRODUCTION = nconf.get('IS_PROD'); @@ -72,7 +75,7 @@ export let taskScoredWebhook = new WebhookSender({ transformData (data) { let { user, task, direction, delta } = data; - let extendedStats = user.addComputedStatsToJSONObj(user.stats.toJSON()); + let extendedStats = User.addComputedStatsToJSONObj(user.stats.toJSON(), user); let userData = { // _id: user._id, added automatically when the webhook is sent diff --git a/website/server/models/user/methods.js b/website/server/models/user/methods.js index c9e77427ab..80e0f385c2 100644 --- a/website/server/models/user/methods.js +++ b/website/server/models/user/methods.js @@ -171,17 +171,27 @@ schema.statics.pushNotification = async function pushNotification (query, type, await this.update(query, {$push: {notifications: newNotification.toObject()}}, {multi: true}).exec(); }; +// Static method to add/remove properties to a JSON User object, +// For example for when the user is returned using `.lean()` and thus doesn't +// have access to any mongoose helper +schema.statics.transformJSONUser = function transformJSONUser (jsonUser, addComputedStats = false) { + // Add id property + jsonUser.id = jsonUser._id; + + if (addComputedStats) this.addComputedStatsToJSONObj(jsonUser.stats, jsonUser); +}; + // Add stats.toNextLevel, stats.maxMP and stats.maxHealth // to a JSONified User stats object -schema.methods.addComputedStatsToJSONObj = function addComputedStatsToUserJSONObj (statsObject) { +schema.statics.addComputedStatsToJSONObj = function addComputedStatsToUserJSONObj (userStatsJSON, user) { // NOTE: if an item is manually added to this.stats then // common/fns/predictableRandom must be tweaked so the new item is not considered. // Otherwise the client will have it while the server won't and the results will be different. - statsObject.toNextLevel = common.tnl(this.stats.lvl); - statsObject.maxHealth = common.maxHealth; - statsObject.maxMP = common.statsComputed(this).maxMP; + userStatsJSON.toNextLevel = common.tnl(user.stats.lvl); + userStatsJSON.maxHealth = common.maxHealth; + userStatsJSON.maxMP = common.statsComputed(user).maxMP; - return statsObject; + return userStatsJSON; }; /**