Fix query when filtering and sorting on a relational field (#9045)

* fix query when filtering and sorting on a relational field

* add tests + refacto

* remove useless code

packages/strapi-connector-bookshelf/lib/buildQuery.js

@@ -1,7 +1,7 @@
 'use strict';
 
 const _ = require('lodash');
-const { each, prop, reject, isEmpty } = require('lodash/fp');
+const { each, prop, isEmpty } = require('lodash/fp');
 const { singular } = require('pluralize');
 const { toQueries, runPopulateQueries } = require('./utils/populate-queries');
 
@@ -14,15 +14,21 @@ const BOOLEAN_OPERATORS = ['or'];
  * @param {Object} options.filters - Filters params (start, limit, sort, where)
  */
 const buildQuery = ({ model, filters }) => qb => {
+  const joinsTree = buildJoinsAndFilter(qb, model, filters);
+
   if (_.has(filters, 'where') && Array.isArray(filters.where) && filters.where.length > 0) {
     qb.distinct();
   }
 
-  const joinsTree = buildJoinsAndFilter(qb, model, filters);
-
   if (_.has(filters, 'sort')) {
-    const clauses = filters.sort.map(buildSortClauseFromTree(joinsTree));
-    qb.orderBy(reject(isEmpty, clauses));
+    const clauses = filters.sort.map(buildSortClauseFromTree(joinsTree)).filter(c => !isEmpty(c));
+    const orderBy = clauses.map(({ order, alias }) => ({ order, column: alias }));
+    const orderColumns = clauses.map(({ alias, column }) => ({ [alias]: column }));
+    const columns = [`${joinsTree.alias}.*`, ...orderColumns];
+
+    qb.distinct()
+      .column(columns)
+      .orderBy(orderBy);
   }
 
   if (_.has(filters, 'start')) {
@@ -47,13 +53,21 @@ const buildQuery = ({ model, filters }) => qb => {
  */
 const buildSortClauseFromTree = tree => ({ field, order }) => {
   if (!field.includes('.')) {
-    return { column: field, order };
+    return {
+      column: `${tree.alias}.${field}`,
+      order,
+      alias: `_strapi_tmp_${tree.alias}_${field}`,
+    };
   }
 
   const [relation, attribute] = field.split('.');
   for (const { alias, assoc } of Object.values(tree.joins)) {
     if (relation === assoc.alias) {
-      return { column: `${alias}.${attribute}`, order };
+      return {
+        column: `${alias}.${attribute}`,
+        order,
+        alias: `_strapi_tmp_${alias}_${attribute}`,
+      };
     }
   }
 

packages/strapi-connector-bookshelf/lib/mount-models.js

@@ -610,6 +610,10 @@ module.exports = async ({ models, target }, ctx, { selfFinalize = false } = {})
         const formatValue = createFormatter(definition.client);
         function formatEntry(entry) {
           Object.keys(entry.attributes).forEach(key => {
+            if (key.startsWith('_strapi_tmp_')) {
+              delete entry.attributes[key];
+              return;
+            }
             const attr = definition.attributes[key] || {};
             entry.attributes[key] = formatValue(attr, entry.attributes[key]);
           });

packages/strapi-plugin-content-manager/tests/api/basic-count-relations.test.e2e.js

@@ -1,171 +0,0 @@
-'use strict';
-
-const { createAuthRequest } = require('../../../../test/helpers/request');
-const { createStrapiInstance } = require('../../../../test/helpers/strapi');
-const { createTestBuilder } = require('../../../../test/helpers/builder');
-
-let strapi;
-let rq;
-const builder = createTestBuilder();
-
-let data = {
-  stamps: [],
-  collectors: [],
-};
-
-const stamp = {
-  name: 'stamp',
-  kind: 'collectionType',
-  attributes: {
-    name: {
-      type: 'string',
-    },
-  },
-};
-
-const collector = {
-  name: 'collector',
-  kind: 'collectionType',
-  attributes: {
-    name: {
-      type: 'string',
-    },
-    age: {
-      type: 'integer',
-    },
-    stamps: {
-      nature: 'manyWay',
-      target: 'application::stamp.stamp',
-      unique: false,
-    },
-    stamps_m2m: {
-      nature: 'manyToMany',
-      targetAttribute: 'collectors',
-      target: 'application::stamp.stamp',
-      unique: false,
-      dominant: true,
-    },
-    stamps_one_many: {
-      nature: 'oneToMany',
-      targetAttribute: 'collector',
-      target: 'application::stamp.stamp',
-      unique: false,
-    },
-  },
-};
-
-const stampFixtures = [
-  {
-    name: '1946',
-  },
-  {
-    name: '1947',
-  },
-  {
-    name: '1948',
-  },
-];
-
-const collectorFixtures = ({ stamp }) => [
-  {
-    name: 'Bernard',
-    age: 25,
-    stamps: [stamp[0].id, stamp[1].id],
-    stamps_m2m: [stamp[0].id],
-    stamps_one_many: [],
-  },
-  {
-    name: 'Isabelle',
-    age: 55,
-    stamps: [stamp[0].id],
-    stamps_m2m: [],
-    stamps_one_many: [stamp[1].id, stamp[2].id],
-  },
-  {
-    name: 'Emma',
-    age: 23,
-    stamps: [],
-    stamps_m2m: [stamp[0].id, stamp[1].id],
-    stamps_one_many: [stamp[0].id],
-  },
-];
-
-const getCollectorByName = (collectors, name) => collectors.find(c => c.name === name);
-const getStampByName = (stamps, name) => stamps.find(s => s.name === name);
-
-describe('CM API - Count relations', () => {
-  beforeAll(async () => {
-    await builder
-      .addContentTypes([stamp, collector])
-      .addFixtures(stamp.name, stampFixtures)
-      .addFixtures(collector.name, collectorFixtures)
-      .build();
-
-    strapi = await createStrapiInstance();
-    rq = await createAuthRequest({ strapi });
-
-    data.collectors = builder.sanitizedFixturesFor(collector.name, strapi);
-    data.stamps = builder.sanitizedFixturesFor(stamp.name, strapi);
-  }, 60000);
-
-  afterAll(async () => {
-    await strapi.destroy();
-    await builder.cleanup();
-  }, 60000);
-
-  test('many-way', async () => {
-    const res = await rq({
-      method: 'GET',
-      url: '/content-manager/collection-types/application::collector.collector',
-    });
-
-    expect(res.statusCode).toBe(200);
-    expect(Array.isArray(res.body.results)).toBe(true);
-    expect(res.body.results).toHaveLength(3);
-    expect(getCollectorByName(res.body.results, 'Bernard').stamps.count).toBe(2);
-    expect(getCollectorByName(res.body.results, 'Isabelle').stamps.count).toBe(1);
-    expect(getCollectorByName(res.body.results, 'Emma').stamps.count).toBe(0);
-  });
-
-  test('many-to-many (collector -> stamps)', async () => {
-    const res = await rq({
-      method: 'GET',
-      url: '/content-manager/collection-types/application::collector.collector',
-    });
-
-    expect(res.statusCode).toBe(200);
-    expect(Array.isArray(res.body.results)).toBe(true);
-    expect(res.body.results).toHaveLength(3);
-    expect(getCollectorByName(res.body.results, 'Bernard').stamps_m2m.count).toBe(1);
-    expect(getCollectorByName(res.body.results, 'Isabelle').stamps_m2m.count).toBe(0);
-    expect(getCollectorByName(res.body.results, 'Emma').stamps_m2m.count).toBe(2);
-  });
-
-  test('many-to-many (stamp -> collectors)', async () => {
-    const res = await rq({
-      method: 'GET',
-      url: '/content-manager/collection-types/application::stamp.stamp',
-    });
-
-    expect(res.statusCode).toBe(200);
-    expect(Array.isArray(res.body.results)).toBe(true);
-    expect(res.body.results).toHaveLength(3);
-    expect(getStampByName(res.body.results, '1946').collectors.count).toBe(2);
-    expect(getStampByName(res.body.results, '1947').collectors.count).toBe(1);
-    expect(getStampByName(res.body.results, '1948').collectors.count).toBe(0);
-  });
-
-  test('one-to-many', async () => {
-    const res = await rq({
-      method: 'GET',
-      url: '/content-manager/collection-types/application::collector.collector',
-    });
-
-    expect(res.statusCode).toBe(200);
-    expect(Array.isArray(res.body.results)).toBe(true);
-    expect(res.body.results).toHaveLength(3);
-    expect(getCollectorByName(res.body.results, 'Bernard').stamps_one_many.count).toBe(0);
-    expect(getCollectorByName(res.body.results, 'Isabelle').stamps_one_many.count).toBe(2);
-    expect(getCollectorByName(res.body.results, 'Emma').stamps_one_many.count).toBe(1);
-  });
-});

packages/strapi-plugin-content-manager/tests/api/basic-relations.test.e2e.js

@@ -0,0 +1,404 @@
+'use strict';
+
+const { createAuthRequest } = require('../../../../test/helpers/request');
+const { createStrapiInstance } = require('../../../../test/helpers/strapi');
+const { createTestBuilder } = require('../../../../test/helpers/builder');
+
+let strapi;
+let rq;
+const builder = createTestBuilder();
+
+let data = {
+  stamps: [],
+  collectors: [],
+};
+
+const stamp = {
+  name: 'stamp',
+  kind: 'collectionType',
+  attributes: {
+    name: {
+      type: 'string',
+    },
+  },
+};
+
+const collector = {
+  name: 'collector',
+  kind: 'collectionType',
+  attributes: {
+    name: {
+      type: 'string',
+    },
+    age: {
+      type: 'integer',
+    },
+    stamps: {
+      nature: 'manyWay',
+      target: 'application::stamp.stamp',
+      unique: false,
+    },
+    stamps_one_way: {
+      nature: 'oneWay',
+      target: 'application::stamp.stamp',
+      unique: false,
+    },
+    stamps_m2m: {
+      nature: 'manyToMany',
+      targetAttribute: 'collectors',
+      target: 'application::stamp.stamp',
+      unique: false,
+      dominant: true,
+    },
+    stamps_one_many: {
+      nature: 'oneToMany',
+      targetAttribute: 'collector',
+      target: 'application::stamp.stamp',
+      unique: false,
+    },
+    stamps_one_one: {
+      nature: 'oneToOne',
+      targetAttribute: 'collector_one_one',
+      target: 'application::stamp.stamp',
+      unique: false,
+    },
+  },
+};
+
+const stampFixtures = [
+  {
+    name: '1946',
+  },
+  {
+    name: '1947',
+  },
+  {
+    name: '1948',
+  },
+];
+
+
+
+
+const collectorFixtures = ({ stamp }) =>  [
+  {
+    name: 'Bernard',
+    age: 25,
+    stamps: [stamp[0].id, stamp[1].id],
+    stamps_m2m: [stamp[0].id],
+    stamps_one_many: [],
+    stamps_one_way: stamp[0].id,
+    stamps_one_one: stamp[0].id,
+  },
+  {
+    name: 'Isabelle',
+    age: 55,
+    stamps: [stamp[0].id],
+    stamps_m2m: [],
+    stamps_one_many: [stamp[1].id, stamp[2].id],
+    stamps_one_way: stamp[1].id,
+    stamps_one_one: stamp[1].id,
+  },
+  {
+    name: 'Emma',
+    age: 23,
+    stamps: [],
+    stamps_m2m: [stamp[0].id, stamp[1].id],
+    stamps_one_many: [stamp[0].id],
+    stamps_one_way: stamp[2].id,
+    stamps_one_one: stamp[2].id,
+  },
+];
+
+
+const getCollectorByName = (collectors, name) => collectors.find(c => c.name === name);
+const getStampByName = (stamps, name) => stamps.find(s => s.name === name);
+
+describe('CM API', () => {
+  beforeAll(async () => {
+    await builder
+      .addContentTypes([stamp, collector])
+      .addFixtures(stamp.name, stampFixtures)
+      .addFixtures(collector.name, collectorFixtures)
+      .build();
+
+
+    strapi = await createStrapiInstance();
+    rq = await createAuthRequest({ strapi });
+
+    data.collectors = builder.sanitizedFixturesFor(collector.name, strapi);
+    data.stamps = builder.sanitizedFixturesFor(stamp.name, strapi);
+  }, 60000);
+
+  afterAll(async () => {
+    await strapi.destroy();
+    await builder.cleanup();
+  }, 60000);
+
+  describe('Count relations', () => {
+    test('many-way', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(getCollectorByName(res.body.results, 'Bernard').stamps.count).toBe(2);
+      expect(getCollectorByName(res.body.results, 'Isabelle').stamps.count).toBe(1);
+      expect(getCollectorByName(res.body.results, 'Emma').stamps.count).toBe(0);
+    });
+
+    test('many-to-many (collector -> stamps)', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(getCollectorByName(res.body.results, 'Bernard').stamps_m2m.count).toBe(1);
+      expect(getCollectorByName(res.body.results, 'Isabelle').stamps_m2m.count).toBe(0);
+      expect(getCollectorByName(res.body.results, 'Emma').stamps_m2m.count).toBe(2);
+    });
+
+    test('many-to-many (stamp -> collectors)', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::stamp.stamp',
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(getStampByName(res.body.results, '1946').collectors.count).toBe(2);
+      expect(getStampByName(res.body.results, '1947').collectors.count).toBe(1);
+      expect(getStampByName(res.body.results, '1948').collectors.count).toBe(0);
+    });
+
+    test('one-to-many', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(getCollectorByName(res.body.results, 'Bernard').stamps_one_many.count).toBe(0);
+      expect(getCollectorByName(res.body.results, 'Isabelle').stamps_one_many.count).toBe(2);
+      expect(getCollectorByName(res.body.results, 'Emma').stamps_one_many.count).toBe(1);
+    });
+  });
+
+  describe('Filter relations', () => {
+    test('many-way', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _where: { 'stamps.name': '1946' },
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(2);
+      expect(res.body.results[0].name).toBe('Bernard');
+      expect(res.body.results[1].name).toBe('Isabelle');
+    });
+
+    test('many-to-many (collector -> stamps)', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _where: { 'stamps_m2m.name': '1946' },
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(2);
+      expect(getCollectorByName(res.body.results, 'Bernard')).toBeDefined();
+      expect(getCollectorByName(res.body.results, 'Emma')).toBeDefined();
+    });
+
+    test('many-to-many (stamp -> collectors)', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::stamp.stamp',
+        qs: {
+          _where: { 'collectors.name': 'Emma' },
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(2);
+      expect(getStampByName(res.body.results, '1946')).toBeDefined();
+      expect(getStampByName(res.body.results, '1947')).toBeDefined();
+    });
+
+    test('one-to-many', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _where: { 'stamps_one_many.name': '1947' },
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(1);
+      expect(res.body.results[0].name).toBe('Isabelle');
+    });
+
+    test('many-to-one', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::stamp.stamp',
+        qs: {
+          _where: { 'collector.name': 'Isabelle' },
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(2);
+      expect(getStampByName(res.body.results, '1947')).toBeDefined();
+      expect(getStampByName(res.body.results, '1948')).toBeDefined();
+    });
+
+    test('one-way', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _where: { 'stamps_one_way.name': '1947' },
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(1);
+      expect(getCollectorByName(res.body.results, 'Isabelle')).toBeDefined();
+    });
+
+    test('one-one', async () => {
+      const res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _where: { 'stamps_one_one.name': '1947' },
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(1);
+      expect(getCollectorByName(res.body.results, 'Isabelle')).toBeDefined();
+    });
+  });
+
+  describe('Sort relations', () => {
+    test('many-to-one', async () => {
+      let res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::stamp.stamp',
+        qs: {
+          _sort: 'collector.name:ASC',
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(res.body.results[0].collector.name).toBe('Emma');
+      expect(res.body.results[1].collector.name).toBe('Isabelle');
+      expect(res.body.results[2].collector.name).toBe('Isabelle');
+
+      res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::stamp.stamp',
+        qs: {
+          _sort: 'collector.name:DESC',
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(res.body.results[0].collector.name).toBe('Isabelle');
+      expect(res.body.results[1].collector.name).toBe('Isabelle');
+      expect(res.body.results[2].collector.name).toBe('Emma');
+    });
+
+    test('one-way', async () => {
+      let res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _sort: 'stamps_one_way.name:ASC',
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(res.body.results[0].stamps_one_way.name).toBe('1946');
+      expect(res.body.results[1].stamps_one_way.name).toBe('1947');
+      expect(res.body.results[2].stamps_one_way.name).toBe('1948');
+
+      res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _sort: 'stamps_one_way.name:DESC',
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(res.body.results[0].stamps_one_way.name).toBe('1948');
+      expect(res.body.results[1].stamps_one_way.name).toBe('1947');
+      expect(res.body.results[2].stamps_one_way.name).toBe('1946');
+    });
+
+    test('one-one', async () => {
+      let res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _sort: 'stamps_one_one.name:ASC',
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(res.body.results[0].stamps_one_one.name).toBe('1946');
+      expect(res.body.results[1].stamps_one_one.name).toBe('1947');
+      expect(res.body.results[2].stamps_one_one.name).toBe('1948');
+
+      res = await rq({
+        method: 'GET',
+        url: '/content-manager/collection-types/application::collector.collector',
+        qs: {
+          _sort: 'stamps_one_one.name:DESC',
+        },
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(Array.isArray(res.body.results)).toBe(true);
+      expect(res.body.results).toHaveLength(3);
+      expect(res.body.results[0].stamps_one_one.name).toBe('1948');
+      expect(res.body.results[1].stamps_one_one.name).toBe('1947');
+      expect(res.body.results[2].stamps_one_one.name).toBe('1946');
+    });
+  });
+});

packages/strapi-utils/lib/build-query.js

@@ -146,9 +146,17 @@ const normalizeSortClauses = (clauses, { model }) => {
   }));
 
   normalizedClauses.forEach(({ field }) => {
-    if (field.includes('.')) {
+    const fieldDepth = field.split('.').length - 1;
+    if (fieldDepth === 1) {
       // Check if the relational field exists
       getAssociationFromFieldKey({ model, field });
+    } else if (fieldDepth > 1) {
+      const err = new Error(
+        `Sorting on ${field} is not possible: you cannot sort at a depth greater than 1`
+      );
+
+      err.status = 400;
+      throw err;
     }
   });