From 2a6e1e0b841bdc627227eb7af60a268d25d6179c Mon Sep 17 00:00:00 2001 From: Alexandre Bodin Date: Tue, 6 Oct 2020 09:20:43 +0200 Subject: [PATCH] Add tests to data-loaders Signed-off-by: Alexandre Bodin --- .../lib/buildQuery.js | 12 +- .../services/__tests__/data-loaders.test.js | 68 +++++++ .../services/data-loaders.js | 171 ++---------------- .../services/resolvers-builder.js | 3 - .../services/type-definitions.js | 128 +++++++------ .../strapi-plugin-graphql/services/utils.js | 2 +- 6 files changed, 165 insertions(+), 219 deletions(-) create mode 100644 packages/strapi-plugin-graphql/services/__tests__/data-loaders.test.js diff --git a/packages/strapi-connector-mongoose/lib/buildQuery.js b/packages/strapi-connector-mongoose/lib/buildQuery.js index a7175f5e90..af42e85257 100644 --- a/packages/strapi-connector-mongoose/lib/buildQuery.js +++ b/packages/strapi-connector-mongoose/lib/buildQuery.js @@ -1,7 +1,7 @@ 'use strict'; const _ = require('lodash'); -const { isEmpty, set, omit, assoc } = require('lodash/fp'); +const { isNil, isEmpty, set, omit, assoc } = require('lodash/fp'); const semver = require('semver'); const { hasDeepFilters, @@ -708,10 +708,12 @@ const findModelPath = ({ rootModel, path }) => { * @param {Object} indexMap - index map of the form { [id]: index } */ const orderByIndexMap = indexMap => entities => { - return entities.reduce((acc, entry) => { - acc[indexMap[entry._id]] = entry; - return acc; - }, []); + return entities + .reduce((acc, entry) => { + acc[indexMap[entry._id]] = entry; + return acc; + }, []) + .filter(entity => !isNil(entity)); }; module.exports = buildQuery; diff --git a/packages/strapi-plugin-graphql/services/__tests__/data-loaders.test.js b/packages/strapi-plugin-graphql/services/__tests__/data-loaders.test.js new file mode 100644 index 0000000000..04e45a330a --- /dev/null +++ b/packages/strapi-plugin-graphql/services/__tests__/data-loaders.test.js @@ -0,0 +1,68 @@ +'use strict'; + +const dataLoaders = require('../data-loaders'); + +describe('dataloader', () => { + describe('serializeKey', () => { + test('Serializes objects to json', () => { + expect(dataLoaders.serializeKey(1928)).toBe(1928); + expect(dataLoaders.serializeKey('test')).toBe('test'); + expect(dataLoaders.serializeKey([1, 2, 3])).toBe('[1,2,3]'); + expect(dataLoaders.serializeKey({ foo: 'bar' })).toBe('{"foo":"bar"}'); + expect(dataLoaders.serializeKey({ foo: 'bar', nested: { bar: 'foo' } })).toBe( + '{"foo":"bar","nested":{"bar":"foo"}}' + ); + }); + }); + + describe('makeQuery', () => { + test('makeQuery single calls findOne', async () => { + const uid = 'uid'; + const find = jest.fn(() => [{ id: 1 }]); + const findOne = jest.fn(() => ({ id: 1 })); + const filters = { _limit: 5 }; + + global.strapi = { + query() { + return { find, findOne }; + }, + }; + + await dataLoaders.makeQuery(uid, { single: true, filters }); + + expect(findOne).toHaveBeenCalledWith(filters, []); + }); + + test('makeQuery calls find', async () => { + const uid = 'uid'; + const find = jest.fn(() => [{ id: 1 }]); + const filters = { _limit: 5, _sort: 'field' }; + + global.strapi = { + query() { + return { find }; + }, + }; + + await dataLoaders.makeQuery(uid, { filters }); + + expect(find).toHaveBeenCalledWith(filters, []); + }); + + test('makeQuery disables populate to optimize fetching a bit', async () => { + const uid = 'uid'; + const find = jest.fn(() => [{ id: 1 }]); + const filters = { _limit: 5 }; + + global.strapi = { + query() { + return { find }; + }, + }; + + await dataLoaders.makeQuery(uid, { filters }); + + expect(find).toHaveBeenCalledWith(filters, []); + }); + }); +}); diff --git a/packages/strapi-plugin-graphql/services/data-loaders.js b/packages/strapi-plugin-graphql/services/data-loaders.js index 06fead1967..461ed55475 100644 --- a/packages/strapi-plugin-graphql/services/data-loaders.js +++ b/packages/strapi-plugin-graphql/services/data-loaders.js @@ -12,7 +12,7 @@ const DataLoader = require('dataloader'); module.exports = { loaders: {}, - initializeLoader: function() { + initializeLoader() { this.resetLoaders(); // Create loaders for each relational field (exclude core models). @@ -35,170 +35,37 @@ module.exports = { this.createLoader('strapi::user'); }, - resetLoaders: function() { + resetLoaders() { this.loaders = {}; }, - createLoader: function(modelUID) { + createLoader(modelUID) { if (this.loaders[modelUID]) { return this.loaders[modelUID]; } - this.loaders[modelUID] = new DataLoader( - keys => { - // Extract queries from keys and merge similar queries. - const { queries, map } = this.extractQueries(modelUID, _.cloneDeep(keys)); + const loadFn = queries => this.batchQuery(modelUID, queries); + const loadOptions = { + cacheKeyFn: key => this.serializeKey(key), + }; - // Run queries in parallel. - return Promise.all(queries.map(query => this.makeQuery(modelUID, query))).then(results => { - // Use to match initial queries order. - return this.mapData(modelUID, keys, map, results); - }); - }, - { - cacheKeyFn: key => { - return _.isObjectLike(key) ? JSON.stringify(_.cloneDeep(key)) : key; - }, - } - ); + this.loaders[modelUID] = new DataLoader(loadFn, loadOptions); }, - mapData: function(modelUID, originalMap, map, results) { - // Use map to re-dispatch data correctly based on initial keys. - return originalMap.map((query, index) => { - // Find the index of where we should extract the results. - const indexResults = map.findIndex(queryMap => queryMap.indexOf(index) !== -1); - const data = results[indexResults]; - - // Retrieving referring model. - const ref = strapi.getModel(modelUID); - - if (query.single) { - // Return object instead of array for one-to-many relationship. - return data.find( - entry => - entry[ref.primaryKey].toString() === (query.params[ref.primaryKey] || '').toString() - ); - } - - // Generate constant for skip parameters. - // Note: we shouldn't support both way of doing this kind of things in the future. - const skip = query.options._start || 0; - const limit = _.get(query, 'options._limit', 100); // Take into account the limit if its equal 0 - - // Extracting ids from original request to map with query results. - const ids = this.extractIds(query, ref); - - const ast = ref.associations.find(ast => ast.alias === ids.alias); - const astModel = ast - ? strapi.getModel(ast.model || ast.collection, ast.plugin) - : strapi.getModel(modelUID); - - if (!_.isArray(ids)) { - return data - .filter(entry => entry !== undefined) - .filter(entry => { - const aliasEntry = entry[ids.alias]; - - if (_.isArray(aliasEntry)) { - return _.find( - aliasEntry, - value => value[astModel.primaryKey].toString() === ids.value - ); - } - - const entryValue = aliasEntry[astModel.primaryKey].toString(); - return entryValue === ids.value; - }) - .slice(skip, skip + limit); - } - - return data - .filter(entry => entry !== undefined) - .filter(entry => ids.map(id => id.toString()).includes(entry[ref.primaryKey].toString())) - .slice(skip, skip + limit); - }); + serializeKey(key) { + return _.isObjectLike(key) ? JSON.stringify(_.cloneDeep(key)) : key; }, - extractIds: (query, ref) => { - if (_.get(query.options, `query.${ref.primaryKey}`)) { - return _.get(query.options, `query.${ref.primaryKey}`); + async batchQuery(modelUID, queries) { + // Extract queries from keys and merge similar queries. + return Promise.all(queries.map(query => this.makeQuery(modelUID, query))); + }, + + async makeQuery(modelUID, query = {}) { + if (query.single === true) { + return strapi.query(modelUID).findOne(query.filters, []); } - const alias = _.first(Object.keys(query.options.query)); - const value = query.options.query[alias].toString(); - return { - alias, - value, - }; - }, - - makeQuery: async function(modelUID, query = {}) { - if (_.isEmpty(query.ids)) { - return []; - } - - const ref = strapi.getModel(modelUID); - const ast = ref.associations.find(ast => ast.alias === query.alias); - - const ids = _.chain(query.ids) - .filter(id => !_.isEmpty(id) || _.isInteger(id)) // Only keep valid ids - .map(id => id.toString()) // convert ids to string - .uniq() // Remove redundant ids - .value(); - - const params = { - ...query.options, - [`${query.alias}_in`]: ids, - _start: 0, // Don't apply start or skip - _limit: -1, // Don't apply a limit - }; - - // Run query and remove duplicated ID. - return strapi.entityService.find( - { params, populate: ast ? [query.alias] : [] }, - { model: modelUID } - ); - }, - - extractQueries: function(modelUID, keys) { - const queries = []; - const map = []; - - keys.forEach((current, index) => { - // Extract query options. - // Note: the `single` means that we've only one entry to fetch. - const { single = false, params = {}, association } = current; - const { query = {}, ...options } = current.options; - - // Retrieving referring model. - const { primaryKey } = strapi.getModel(modelUID); - - // Generate array of IDs to fetch. - const ids = []; - - // Only one entry to fetch. - if (single) { - ids.push(params[primaryKey]); - } else if (_.isArray(query[primaryKey])) { - ids.push(...query[primaryKey]); - } else { - ids.push(query[association.via]); - } - - queries.push({ - ids, - options, - alias: _.first(Object.keys(query)) || primaryKey, - }); - - map[queries.length - 1 > 0 ? queries.length - 1 : 0] = []; - map[queries.length - 1].push(index); - }); - - return { - queries, - map, - }; + return strapi.query(modelUID).find(query.filters, []); }, }; diff --git a/packages/strapi-plugin-graphql/services/resolvers-builder.js b/packages/strapi-plugin-graphql/services/resolvers-builder.js index 31e96b1546..e76ae184a1 100644 --- a/packages/strapi-plugin-graphql/services/resolvers-builder.js +++ b/packages/strapi-plugin-graphql/services/resolvers-builder.js @@ -148,8 +148,6 @@ const buildQueryContext = ({ options, graphqlContext }) => { const ctx = cloneKoaContext(context); - // Note: we've to used the Object.defineProperties to reset the prototype. It seems that the cloning the context - // cause a lost of the Object prototype. const opts = amountLimiting(_options); ctx.query = { @@ -165,7 +163,6 @@ const buildQueryContext = ({ options, graphqlContext }) => { /** * Checks if a resolverPath (resolver or resovlerOf) might be resolved */ - const getPolicies = config => { const { resolver, policies = [], resolverOf } = config; diff --git a/packages/strapi-plugin-graphql/services/type-definitions.js b/packages/strapi-plugin-graphql/services/type-definitions.js index 0154b69a0a..04c57be5a2 100644 --- a/packages/strapi-plugin-graphql/services/type-definitions.js +++ b/packages/strapi-plugin-graphql/services/type-definitions.js @@ -177,88 +177,100 @@ const buildAssocResolvers = model => { const target = association.model || association.collection; const targetModel = strapi.getModel(target, association.plugin); - switch (association.nature) { + const { nature, alias } = association; + + switch (nature) { case 'oneToManyMorph': case 'manyMorphToOne': case 'manyMorphToMany': - case 'manyToManyMorph': - case 'manyWay': { - resolver[association.alias] = async obj => { - if (obj[association.alias]) { - return assignOptions(obj[association.alias], obj); + case 'manyToManyMorph': { + resolver[alias] = async obj => { + if (obj[alias]) { + return assignOptions(obj[alias], obj); } const params = { ...initQueryOptions(targetModel, obj), id: obj[primaryKey], }; - const populate = [association.alias]; - const entry = await strapi.entityService.findOne( - { params, populate }, - { model: model.uid } - ); + const entry = await strapi.query(model.uid).findOne(params, [alias]); - return assignOptions(entry[association.alias], obj); + return assignOptions(entry[alias], obj); }; break; } default: { - resolver[association.alias] = async (obj, options) => { - // Construct parameters object to retrieve the correct related entries. + resolver[alias] = async (obj, options) => { + const loader = strapi.plugins.graphql.services['data-loaders'].loaders[targetModel.uid]; + + const localId = obj[model.primaryKey]; + const targetPK = targetModel.primaryKey; + const foreignId = _.get(obj[alias], targetModel.primaryKey, obj[alias]); + const params = { - model: targetModel.uid, + ...initQueryOptions(targetModel, obj), + ...convertToParams(_.omit(amountLimiting(options), 'where')), + ...convertToQuery(options.where), }; - let queryOpts = initQueryOptions(targetModel, obj); + if (['oneToOne', 'oneWay', 'manyToOne'].includes(nature)) { + if (!_.has(obj, alias) || _.isNil(foreignId)) { + return null; + } - if (association.type === 'model') { - params[targetModel.primaryKey] = _.get( - obj, - `${association.alias}.${targetModel.primaryKey}`, - obj[association.alias] - ); - } else { - const queryParams = amountLimiting(options); - queryOpts = { - ...queryOpts, - ...convertToParams(_.omit(queryParams, 'where')), // Convert filters (sort, limit and start/skip, publicationState) - ...convertToQuery(queryParams.where), + // check this is en entity and not a mongo ID + if (_.has(obj[alias], targetPK)) { + return assignOptions(obj[alias], obj); + } + + const query = { + single: true, + filters: { + ...params, + [targetPK]: foreignId, + }, }; - if ( - ((association.nature === 'manyToMany' && association.dominant) || - association.nature === 'manyWay') && - _.has(obj, association.alias) // if populated - ) { - _.set( - queryOpts, - ['query', targetModel.primaryKey], - obj[association.alias] - ? obj[association.alias].map(val => val[targetModel.primaryKey] || val).sort() - : [] - ); - } else { - _.set(queryOpts, ['query', association.via], obj[targetModel.primaryKey]); - } + return loader.load(query).then(r => assignOptions(r, obj)); } - const results = association.model - ? await strapi.plugins.graphql.services['data-loaders'].loaders[targetModel.uid].load( - { - params, - options: queryOpts, - single: true, - } - ) - : await strapi.plugins.graphql.services['data-loaders'].loaders[targetModel.uid].load( - { - options: queryOpts, - association, - } - ); + if (['oneToMany', 'manyToMany'].includes(nature)) { + const { via } = association; - return assignOptions(results, obj); + const filters = { + ...params, + [`${via}.id`]: localId, + }; + + return loader.load({ filters }).then(r => assignOptions(r, obj)); + } + + if (nature === 'manyWay') { + let targetIds = []; + + // find the related ids to query them and apply the filters + if (Array.isArray(obj[alias])) { + targetIds = obj[alias].map(value => value[targetPK] || value); + } else { + const entry = await strapi + .query(model.uid) + .findOne({ [primaryKey]: obj[primaryKey] }, [alias]); + + if (_.isEmpty(entry[alias])) { + return []; + } + + targetIds = entry[alias].map(el => el[targetPK]); + } + + const filters = { + ...params, + [`${targetPK}_in`]: targetIds.map(_.toString), + }; + + return loader.load({ filters }).then(r => assignOptions(r, obj)); + } }; break; } diff --git a/packages/strapi-plugin-graphql/services/utils.js b/packages/strapi-plugin-graphql/services/utils.js index dd5cedb5c8..06be9b8edf 100644 --- a/packages/strapi-plugin-graphql/services/utils.js +++ b/packages/strapi-plugin-graphql/services/utils.js @@ -79,7 +79,7 @@ const amountLimiting = (params = {}) => { if (!amountLimit) return params; - if (!params.limit || params.limit === -1 || params.limit > amountLimit) { + if (_.isNil(params.limit) || params.limit === -1 || params.limit > amountLimit) { params.limit = amountLimit; } else if (params.limit < 0) { params.limit = 0;