From 95ff02da334a24d3bf65083fa4bf4b0e25a9d998 Mon Sep 17 00:00:00 2001 From: Adrien Guyon <48474636+adriguy@users.noreply.github.com> Date: Thu, 2 Sep 2021 16:20:27 +0200 Subject: [PATCH] perf(bulk-actions): improve performance when calling getIdsFromRequest (#797) --- package.json | 2 +- src/services/has-many-getter.js | 2 +- src/services/query-options.js | 16 ++++++- src/services/resources-getter.js | 7 +-- test/databases.test.js | 81 ++++++++++++++++++++++++++------ yarn.lock | 8 ++-- 6 files changed, 91 insertions(+), 25 deletions(-) diff --git a/package.json b/package.json index e8b3b154..dd8d54f5 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ "@babel/runtime": "7.10.1", "bluebird": "2.9.25", "core-js": "3.6.5", - "forest-express": "9.2.0", + "forest-express": "9.2.2", "http-errors": "1.6.1", "lodash": "4.17.21", "moment": "2.19.4", diff --git a/src/services/has-many-getter.js b/src/services/has-many-getter.js index 3998f164..d163e927 100644 --- a/src/services/has-many-getter.js +++ b/src/services/has-many-getter.js @@ -37,7 +37,7 @@ class HasManyGetter extends ResourcesGetter { as: associationName, scope: false, required: !!buildOptions.forCount, // Why? - ...pick(options, ['where', 'include']), + ...pick(options, ['attributes', 'where', 'include']), }], }); diff --git a/src/services/query-options.js b/src/services/query-options.js index c15325b1..2094da45 100644 --- a/src/services/query-options.js +++ b/src/services/query-options.js @@ -31,6 +31,15 @@ class QueryOptions { options.limit = this._limit; } + if (this._restrictFieldsOnRootModel && this._requestedFields.size) { + // Restricting loaded fields on the root model is opt-in with sequelize to avoid + // side-effects as this was not supported historically and it would probably break + // smart fields. + // @see https://github.com/ForestAdmin/forest-express-sequelize/blob/7d7ad0/src/services/resources-getter.js#L142 + + options.attributes = [...this._requestedFields].filter((s) => !s.includes('.')); + } + return SequelizeCompatibility.postProcess(this._model, options); } @@ -93,6 +102,7 @@ class QueryOptions { this._options = options; // Used to compute relations that will go in the final 'include' + this._restrictFieldsOnRootModel = false; this._requestedFields = new Set(); this._neededFields = new Set(); this._scopes = []; // @see sequelizeScopes getter @@ -115,11 +125,15 @@ class QueryOptions { * Add the required includes from a list of field names. * @param {string[]} fields Fields of HasOne and BelongTo relations are * accepted (ie. 'book.name'). + * @param {string[]} fields the output of the extractRequestedFields() util function + * @param {boolean} applyOnRootModel restrict fetched fields also on the root */ - async requireFields(fields) { + async requireFields(fields, applyOnRootModel = false) { if (fields) { fields.forEach((field) => this._requestedFields.add(field)); } + + this._restrictFieldsOnRootModel = Boolean(applyOnRootModel); } /** diff --git a/src/services/resources-getter.js b/src/services/resources-getter.js index 2668d1f5..021010d0 100644 --- a/src/services/resources-getter.js +++ b/src/services/resources-getter.js @@ -52,14 +52,15 @@ class ResourcesGetter { async _buildQueryOptions(buildOptions = {}) { const { forCount, tableAlias } = buildOptions; const { - fields, filters, search, searchExtended, segment, segmentQuery, timezone, + fields, filters, restrictFieldsOnRootModel, + search, searchExtended, segment, segmentQuery, timezone, } = this._params; - const requestedFields = extractRequestedFields(fields, this._model, Schemas.schemas); const scopeFilters = await scopeManager.getScopeForUser(this._user, this._model.name, true); + const requestedFields = extractRequestedFields(fields, this._model, Schemas.schemas); const queryOptions = new QueryOptions(this._model, { tableAlias }); - await queryOptions.requireFields(requestedFields); + await queryOptions.requireFields(requestedFields, restrictFieldsOnRootModel); await queryOptions.search(search, searchExtended); await queryOptions.filterByConditionTree(filters, timezone); await queryOptions.filterByConditionTree(scopeFilters, timezone); diff --git a/test/databases.test.js b/test/databases.test.js index b4544671..07bf349f 100644 --- a/test/databases.test.js +++ b/test/databases.test.js @@ -573,7 +573,7 @@ const user = { renderingId: 1 }; time_range: null, filters: null, }, options, user).perform(); - expect(stat.value).toHaveLength(0); + expect(stat.value).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -1230,7 +1230,7 @@ const user = { renderingId: 1 }; }; try { const result = await new ResourcesGetter(models.user, null, params, user).perform(); - expect(result[0]).toHaveLength(0); + expect(result[0]).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -1526,7 +1526,7 @@ const user = { renderingId: 1 }; }; try { const result = await new ResourcesGetter(models.bike, null, params, user).perform(); - expect(result[0]).toHaveLength(0); + expect(result[0]).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -1602,7 +1602,7 @@ const user = { renderingId: 1 }; const result = await new ResourcesGetter( models.georegion, null, params, user, ).perform(); - expect(result[0]).toHaveLength(0); + expect(result[0]).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -1687,7 +1687,7 @@ const user = { renderingId: 1 }; }; try { const result = await new ResourcesGetter(models.bird, null, params, user).perform(); - expect(result[0]).toHaveLength(0); + expect(result[0]).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -2512,7 +2512,7 @@ const user = { renderingId: 1 }; try { const result = await new ResourcesGetter(models.user, null, params, user).perform(); - expect(result[0]).toHaveLength(0); + expect(result[0]).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -2699,7 +2699,7 @@ const user = { renderingId: 1 }; try { const result = await new ResourcesGetter(models.user, null, params, user).perform(); - expect(result[0]).toHaveLength(0); + expect(result[0]).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -2784,7 +2784,7 @@ const user = { renderingId: 1 }; try { const result = await new ResourcesGetter(models.user, null, params, user).perform(); - expect(result[0]).toHaveLength(0); + expect(result[0]).toBeEmpty(); } finally { spy.mockRestore(); connectionManager.closeConnection(); @@ -2852,7 +2852,7 @@ const user = { renderingId: 1 }; }); describe('request on the resources getter with a smart field', () => { - it('should only retrieve requested fields when only DB fields are used', async () => { + it('should only retrieve requested fields on relations when only DB fields are used', async () => { expect.assertions(5); const { models } = initializeSequelize(); const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null); @@ -2864,7 +2864,7 @@ const user = { renderingId: 1 }; try { const result = await new ResourcesGetter(models.address, null, params).perform(); - expect(result[0]).not.toHaveLength(0); + expect(result[0]).not.toBeEmpty(); expect(result[0][0]).toHaveProperty('user'); expect(result[0][0].user.dataValues).toHaveProperty('firstName'); expect(result[0][0].user.dataValues).toHaveProperty('id'); @@ -2875,7 +2875,7 @@ const user = { renderingId: 1 }; } }); - it('should retrieve all fields when a smart field is requested', async () => { + it('should retrieve all fields on relations when a smart field is requested', async () => { expect.assertions(5); const { models } = initializeSequelize(); const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null); @@ -2887,7 +2887,7 @@ const user = { renderingId: 1 }; try { const result = await new ResourcesGetter(models.address, null, params).perform(); - expect(result[0]).not.toHaveLength(0); + expect(result[0]).not.toBeEmpty(); expect(result[0][0]).toHaveProperty('user'); expect(result[0][0].user.dataValues).toHaveProperty('firstName'); expect(result[0][0].user.dataValues).toHaveProperty('id'); @@ -2898,6 +2898,57 @@ const user = { renderingId: 1 }; } }); }); + + describe('request on the resources getter with restrictFieldsOnRootModel flag', () => { + it('should only retrieve requested fields with the flag', async () => { + expect.assertions(6); + const { models } = initializeSequelize(); + const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null); + const params = { + ...baseParams, + fields: { address: 'id,line,zipCode' }, + page: { number: '1' }, + restrictFieldsOnRootModel: true, + }; + + try { + const result = await new ResourcesGetter(models.address, null, params).perform(); + expect(result[0]).not.toBeEmpty(); + expect(result[0][0].dataValues).toHaveProperty('id'); + expect(result[0][0].dataValues).toHaveProperty('line'); + expect(result[0][0].dataValues).toHaveProperty('zipCode'); + expect(result[0][0].dataValues).not.toHaveProperty('city'); + expect(result[0][0].dataValues).not.toHaveProperty('country'); + } finally { + spy.mockRestore(); + connectionManager.closeConnection(); + } + }); + + it('should ignore requested fields without the flag', async () => { + expect.assertions(6); + const { models } = initializeSequelize(); + const spy = jest.spyOn(scopeManager, 'getScopeForUser').mockReturnValue(null); + const params = { + ...baseParams, + fields: { address: 'id,line,zipCode' }, + page: { number: '1' }, + }; + + try { + const result = await new ResourcesGetter(models.address, null, params).perform(); + expect(result[0]).not.toBeEmpty(); + expect(result[0][0].dataValues).toHaveProperty('id'); + expect(result[0][0].dataValues).toHaveProperty('line'); + expect(result[0][0].dataValues).toHaveProperty('zipCode'); + expect(result[0][0].dataValues).toHaveProperty('city'); + expect(result[0][0].dataValues).toHaveProperty('country'); + } finally { + spy.mockRestore(); + connectionManager.closeConnection(); + } + }); + }); }); describe('hasmany > has-many-getter', () => { @@ -2977,7 +3028,7 @@ const user = { renderingId: 1 }; params, user, ).perform(); - expect(result[0]).not.toHaveLength(0); + expect(result[0]).not.toBeEmpty(); const firstEntry = result[0][0]; expect(Object.keys(firstEntry.user.dataValues)).toStrictEqual(['id']); @@ -3144,7 +3195,7 @@ const user = { renderingId: 1 }; params, user, ).perform(); - expect(result[0]).not.toHaveLength(0); + expect(result[0]).not.toBeEmpty(); expect(result[0][0].user.dataValues).toHaveProperty('id'); expect(result[0][0].user.dataValues).toHaveProperty('firstName'); expect(result[0][0].user.dataValues).toHaveProperty('lastName'); @@ -3173,7 +3224,7 @@ const user = { renderingId: 1 }; params, user, ).perform(); - expect(result[0]).not.toHaveLength(0); + expect(result[0]).not.toBeEmpty(); expect(result[0][0]).toHaveProperty('user'); expect(result[0][0].user.dataValues).toHaveProperty('firstName'); expect(result[0][0].user.dataValues).toHaveProperty('id'); diff --git a/yarn.lock b/yarn.lock index 0a7f692c..d0721d25 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5067,10 +5067,10 @@ for-in@^1.0.2: resolved "https://registry.yarnpkg.com/for-in/-/for-in-1.0.2.tgz#81068d295a8142ec0ac726c6e2200c30fb6d5e80" integrity sha1-gQaNKVqBQuwKxybG4iAMMPttXoA= -forest-express@9.2.0: - version "9.2.0" - resolved "https://registry.yarnpkg.com/forest-express/-/forest-express-9.2.0.tgz#c7e22b66d93536984a4197c09067b97dc20f5e8f" - integrity sha512-e9LCoplxXCsLuznLShuMT+oyxBA/+6R+Ib1sazpg4U8/Xe37AgBt2XKefsc1cyUh5EATJKQCNXm95518dij6Ew== +forest-express@9.2.2: + version "9.2.2" + resolved "https://registry.yarnpkg.com/forest-express/-/forest-express-9.2.2.tgz#0e5e7310568a6e20dfe74f6cf3768d06997464e0" + integrity sha512-8Sk0ckIWVDGzPpPik36Gd/jTcCn63eeRR5VLaE1QWzzkKzfPo4WeSyvggI/3HyWVhzmy1Nlizwbof/ki9bRIfQ== dependencies: "@babel/runtime" "7.10.1" base32-encode "1.1.1"