Add tests to data-loaders

Signed-off-by: Alexandre Bodin <bodin.alex@gmail.com>

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) => {
+  return entities
-    acc[indexMap[entry._id]] = entry;
+    .reduce((acc, entry) => {
-    return acc;
+      acc[indexMap[entry._id]] = entry;
-  }, []);
+      return acc;
+    }, [])
+    .filter(entity => !isNil(entity));
 };
 
 module.exports = buildQuery;

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, []);
+    });
+  });
+});

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(
+    const loadFn = queries => this.batchQuery(modelUID, queries);
-      keys => {
+    const loadOptions = {
-        // Extract queries from keys and merge similar queries.
+      cacheKeyFn: key => this.serializeKey(key),
-        const { queries, map } = this.extractQueries(modelUID, _.cloneDeep(keys));
+    };
 
-        // Run queries in parallel.
+    this.loaders[modelUID] = new DataLoader(loadFn, loadOptions);
-        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;
-        },
-      }
-    );
   },
 
-  mapData: function(modelUID, originalMap, map, results) {
+  serializeKey(key) {
-    // Use map to re-dispatch data correctly based on initial keys.
+    return _.isObjectLike(key) ? JSON.stringify(_.cloneDeep(key)) : key;
-    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);
-    });
   },
 
-  extractIds: (query, ref) => {
+  async batchQuery(modelUID, queries) {
-    if (_.get(query.options, `query.${ref.primaryKey}`)) {
+    // Extract queries from keys and merge similar queries.
-      return _.get(query.options, `query.${ref.primaryKey}`);
+    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));
+    return strapi.query(modelUID).find(query.filters, []);
-    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,
-    };
   },
 };

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;
 

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 'manyToManyMorph': {
-        case 'manyWay': {
+          resolver[alias] = async obj => {
-          resolver[association.alias] = async obj => {
+            if (obj[alias]) {
-            if (obj[association.alias]) {
+              return assignOptions(obj[alias], obj);
-              return assignOptions(obj[association.alias], obj);
             }
 
             const params = {
               ...initQueryOptions(targetModel, obj),
               id: obj[primaryKey],
             };
-            const populate = [association.alias];
 
-            const entry = await strapi.entityService.findOne(
+            const entry = await strapi.query(model.uid).findOne(params, [alias]);
-              { params, populate },
-              { model: model.uid }
-            );
 
-            return assignOptions(entry[association.alias], obj);
+            return assignOptions(entry[alias], obj);
           };
           break;
         }
         default: {
-          resolver[association.alias] = async (obj, options) => {
+          resolver[alias] = async (obj, options) => {
-            // Construct parameters object to retrieve the correct related entries.
+            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') {
+              // check this is en entity and not a mongo ID
-              params[targetModel.primaryKey] = _.get(
+              if (_.has(obj[alias], targetPK)) {
-                obj,
+                return assignOptions(obj[alias], obj);
-                `${association.alias}.${targetModel.primaryKey}`,
+              }
-                obj[association.alias]
+
-              );
+              const query = {
-            } else {
+                single: true,
-              const queryParams = amountLimiting(options);
+                filters: {
-              queryOpts = {
+                  ...params,
-                ...queryOpts,
+                  [targetPK]: foreignId,
-                ...convertToParams(_.omit(queryParams, 'where')), // Convert filters (sort, limit and start/skip, publicationState)
+                },
-                ...convertToQuery(queryParams.where),
               };
 
-              if (
+              return loader.load(query).then(r => assignOptions(r, obj));
-                ((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]);
-              }
             }
 
-            const results = association.model
+            if (['oneToMany', 'manyToMany'].includes(nature)) {
-              ? await strapi.plugins.graphql.services['data-loaders'].loaders[targetModel.uid].load(
+              const { via } = association;
-                  {
-                    params,
-                    options: queryOpts,
-                    single: true,
-                  }
-                )
-              : await strapi.plugins.graphql.services['data-loaders'].loaders[targetModel.uid].load(
-                  {
-                    options: queryOpts,
-                    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;
         }

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;