Add Deep Filtering Feature #2452

kamalbennani · 2018-12-08T22:23:14Z

⚠️ This is a "Work In Progress" PR ⚠️
⚠️ The API has been changed, the description is outdated ⚠️

My PR is a:

💥 Breaking change
🐛 Bug fix #issueNumber
💅 Enhancement
🚀 New feature

Main update on the:

Admin
Documentation
Framework
Plugin

Proposal

I think that generated service can be simplified a lot.
The problem with the actual implementation is that:

The logic is duplicated in a lot of different places (API Services + Content Manager + User Permissions etc ..)
Which make it hard, very hard to maintain for both the developers and Strapi collaborators (Due to updates/Migrations)

We could use a simple technique where we implement a sort of a Query Service that handles fetching for us.

Here is an example of the code:

Mongoose Proposal

const { has, isEmpty, get } = require('lodash');
const { buildQueryJoins, buildQueryFilter } = require('./query-utils');

class Query {
  constructor(model) {
    this.model = model;
  }

  buildQuery(filter) {
    const filterStage = buildQueryFilter(this.model, filter.where);
    const query = this.model.aggregate(filterStage);

    return query;
  }

  find(filter) {
    this.query = this.buildQuery(filter);
    if (has(filter, 'start')) this.query.skip(filter.start);
    if (has(filter, 'limit')) this.query.limit(filter.limit);
    if (!isEmpty(filter.sort)) this.query.sort(filter.sort);

    return this;
  }

  count(filter) {
    this.query = this.buildQuery(filter);
    this.query = this.query
      .count("count")
      .then(result => get(result, `0.count`, 0));
    return this;
  }

  populate(populate) {
    // @TODO add a warning message if the query is undefined
    const queryJoins = buildQueryJoins(this.model, { whitelistedPopulate: populate });
    this.query = this.query.append(queryJoins);
    return this;
  }

  execute() {
    return this.query;
  }
}

Post Service

module.exports = {

  /**
   * Promise to fetch all courses.
   *
   * @return {Promise}
   */

  fetchAll: (params, populate) => {
    // Conver JSON filter to MongoDB filter
    const filter = new Builder(Post, params).convert();

    return new Query(Post) // Generic Query which will be implemented by Mongoose and Bookshelf
      .find(filter) // Built-in Deep Relation Filter
      .populate(populate) // List of relations to populate
      .execute();
  },

  veryComplexeFilter: (params, populate) => {
    // Conver JSON filter to MongoDB filter
    const filter = new Builder(Post)
         .eq('active', true) // All active posts
         .contains("author.company", "strapi") // The author's company contains strapi
         .between("likes", [10, 20]) // Where the likes are between 10 and 20
         .limit(10)
         .sort(['likes', 'desc'])
         .convert(); // It converts the JSON filter to MongoDB filter by populating only the relation needed to fulfill this query

    return new Query(Post) // Generic Query which will be implemented by Mongoose and Bookshelf
      .find(filter) // Built-in Deep Relation Filter
      .populate(populate) // List of relations to populate
      .execute();
  }
}

@lauriejim @Aurelsicoko What do you think guys?

WIP (I'll add a complete description later on with concrete examples)

TODOs

Hook	REST	GraphQL
Bookshelf	One-to-Many Deep Filter Many-to-One Deep Filter Many-to-Many Deep Filter Morph Deep Filter	One-to-Many Deep Filter Many-to-One Deep Filter Many-to-Many Deep Filter Morph Deep Filter
Mongoose	One-to-Many Deep Filter Many-to-One Deep Filter Many-to-Many Deep Filter Morph Deep Filter	One-to-Many Deep Filter Many-to-One Deep Filter Many-to-Many Deep Filter Morph Deep Filter

Scenarios

kamalbennani · 2018-12-08T23:30:06Z

packages/strapi-generate-api/templates/bookshelf/service.template

@@ -29,22 +29,19 @@ module.exports = {
      .map(ast => ast.alias);

    return <%= globalID %>.query(function(qb) {
-      _.forEach(filters.where, (where, key) => {


@Aurelsicoko @lauriejim We need to find a way to let the users modify the logic if they want, but the question is: "Do they really need to modify the Core Query System?"

I mean we can, for instance, put this utility functions inside of a folder that they can update but I don't see why should we duplicate the code in the different places (API Services / Content Manager and so on)

You're right! We could centralise everything in a single place but the users need to keep control if they want to edit something. It should not be hidden somewhere to avoid updates...

@Aurelsicoko I think doing something like what @soupette did with the docs plugin and centralize everything but allow for model level overrides

kamalbennani · 2018-12-08T23:43:10Z

packages/strapi-hook-bookshelf/lib/relations.js

+                  symbol: value.symbol || '='
+                };
+              }
+              qb.where(fieldKey, value.symbol, value.value);


Question: Should we ignore the field if it doesn't exist in the model's attributes (because the actual behaviour now is that you get a 500 Internal Error message)

@lauriejim ?

@kamalbennani @lauriejim realistically I think it should give a 4xx error. The error is with the user and their query. Ignoring it could make it look like there is nothing wrong. It should still toss an error, but in this case it isn't the server's fault 😜

kamalbennani · 2018-12-08T23:45:16Z

packages/strapi-plugin-graphql/services/Query.js

        );

-        return controller(ctx, next);
+        return controller(Object.assign({}, ctx, queryOpts, { send: ctx.send }), next, { populate: [] });


Don't populate any relation in the GraphQL context (Which will make things go faster :yay:)

I've already did an update like that in this PR #2380, we should try to synchronise our work.

kamalbennani · 2018-12-08T23:46:04Z

packages/strapi-plugin-graphql/services/Resolvers.js

                default:
                  // Where.
-                  queryOpts.query = strapi.utils.models.convertParams(name, {


No need to convertParams here because it's already done in the content manager

You're right but I'm not 100% sure... it should work without converting the parameters but we're not doing randomly, I think there is a reason but I can't remember why.

kamalbennani · 2018-12-09T12:22:02Z

packages/strapi-plugin-upload/config/queries/bookshelf.js

      }
    }).fetchAll({
-      withRelated: populate || _.keys(_.groupBy(_.reject(this.associations, { autoPopulate: false }), 'alias'))


Does anyone remember why here we reject the non-autoPopulated association but in the mongoose version we don't (https://github.com/strapi/strapi/pull/2452/files#diff-5a892334926e352d367245b7876dd6bdL10)

We're rejecting the relations which don't have the autoPopulate set to true (see https://github.com/strapi/strapi/blob/master/packages/strapi-generate-api/templates/mongoose/service.template#L24).

I meant that we are not doing it in packages/strapi-plugin-upload/config/queries/mongoose.js

kamalbennani · 2018-12-09T12:25:00Z

packages/strapi-plugin-users-permissions/models/User.settings.json

-  }
-}
+  },
+  "collectionName": "users-permissions_user"


To be removed!

kamalbennani · 2018-12-09T12:26:12Z

packages/strapi-plugin-graphql/services/Resolvers.js

-              queryOpts.query.id.symbol = 'IN';
+              // Construct the "where" query to only retrieve entries which are
+              // related to this entry.
+              _.set(queryOpts, ['query', association.via], obj[ref.primaryKey]);


I think that there is no need to separate the manyToMany case from the rest of the logic, Could please confirm this @lauriejim @Aurelsicoko

This is not only the manyToMany case but it's also the oneToMany or manyToOne process.

kamalbennani · 2018-12-09T13:15:11Z

packages/strapi-hook-bookshelf/lib/relations.js

+            const fieldKey = (association && association.nature === 'manyToMany')
+              ? `${association.tableCollectionName}.${attributes[key].attribute}_${attributes[key].column}`
+              : `${strapiModel.collectionName}.${key}`;
+            if (_.isArray(value.value) && value.symbol !== 'IN') {


There is probably no need for this check! because we've removed the check in the GraphQL manyToMany relation

kamalbennani · 2018-12-10T08:49:51Z

What's the best way to implement the unit tests, Yesterday I tried to run the existing test but it took at least 30/40 mins without success

lauriejim · 2018-12-10T11:15:21Z

Jest test are broken on master we fixed it on cypress branch. A pull request is open.
Will be merge soon as possible.

abdonrd · 2018-12-12T20:56:22Z

Cypress PR (#2348) merged! 🎉

jacargentina · 2018-12-18T20:02:08Z

@lauriejim how can I help to get this merged, I NEED it 🙏

lukefanning · 2018-12-19T16:19:59Z

Sorry guys quick question I'm new to strapi, does this PR only refer to being able to do a deep filter using GET params or does this also apply to GraphQL queries? I seem to be unable to deep filter with GraphQL

jacargentina · 2018-12-21T17:14:49Z

@ssg-luke both of them, GET and GraphQL (right @kamalbennani ?)

kamalbennani · 2018-12-21T17:25:21Z

@ssg-luke both of them, GET and GraphQL (right @kamalbennani ?)

Yes :D

kamalbennani · 2019-01-02T23:30:02Z

packages/strapi-generate-api/templates/mongoose/service.template

-    return <%= globalID %>
-      .count()
-      .where(filters.where);
+      // Convert `params` object to filter compatible with Mongo.


remove indentation

jacargentina · 2019-01-04T19:54:13Z

@kamalbennani @lauriejim any way i can help with this? It is becoming very hard to do simple queries without it. 🙏

kamalbennani · 2019-01-04T20:53:25Z

@jacargentina We're doing our best to be able to merge this as soon as possible, we need to make sure that it breaks nothing and make it easy to integrate with existing Strapi projects effortlessly

kamalbennani · 2019-02-20T22:38:52Z

@kret13 it seems to be working just fine on a newly generated project :/

Could you provide us some steps to reproduce the same behaviour

kret13 · 2019-02-21T08:45:30Z

I haven't mentioned that it doesn't work when you add limit. Try with limit field to make count be number lesser than totalCount.
So steps are:

make a query on someConnection with filtering on any relation field that is not it's id
add limit to filter and try to make results of count and totalCount be different. For example, if there is totalCount of batches with name:"something" is equal 2, limit that to 1 so that it should display count: 1 and totalCount: 2

kamalbennani · 2019-02-21T08:53:17Z

@kret13 I'll try to reproduce it again, thanks mate 💪
Could you push the tests more and tell us if everything works as excepted for you (except the aggregation of course :p)

…ethod 'where' in the query class

…e the order of the pipelines matters

kamalbennani · 2019-02-21T23:05:52Z

@kret13 it's fixed now, could you try again

kret13 · 2019-02-22T13:36:15Z

I tried this again and it seems that it's good now.

lauriejim · 2019-02-25T15:24:33Z

I got a log when I filter with relation attribute

[2019-02-25T15:15:43.575Z] warn Your filter: {
  "author.name": "admin",
  "limit": "1"
} contains a field "author.name" that doesn't appear neither on your model definition nor in the basic filter operators,
            This field will be ignored for now.
[2019-02-25T15:15:43.582Z] debug GET /articles?author.name=admin&limit=1

Look like an error but it's not.

I have to check with actual master branch but when I install plugins, plugins permission disappear.
It's really strange.

lauriejim · 2019-02-26T10:47:30Z

The issue I report not happen on master investigation have to be done to fix that.

jacargentina · 2019-03-06T20:17:17Z

@lauriejim 3 months "birthday" soon. Any chance to get this merged on master ?

derrickmehaffy · 2019-03-07T01:05:07Z

@jacargentina it is being tested, last I saw last week there was issues that needed to be worked out that are being looked into.

Maybe Jim can give a timeline but due to the size of this PR it cannot be merged without very detailed testing.

alexandrebodin · 2019-03-11T10:16:21Z

Hi guys,

Thank you for your work @kamalbennani We are taking over the PR to help release it asap for the community ;)

We are opening a new PR with the same commits merged into a freature branch on the main repo.
Here is the new PR #2961

Go there if you have new comments or questions 👍

kamalbennani force-pushed the feature/deep-filtering-mongoose-bookshelf branch from a716001 to c5ccdee Compare December 8, 2018 22:54

kamalbennani commented Dec 8, 2018

View reviewed changes

lauriejim changed the title ~~Add Deep Filtering Feature [Rework]~~ [WIP] Add Deep Filtering Feature [Rework] Dec 9, 2018

kamalbennani force-pushed the feature/deep-filtering-mongoose-bookshelf branch from c5ccdee to edbd3ca Compare December 9, 2018 12:18

kamalbennani commented Dec 9, 2018

View reviewed changes

kamalbennani force-pushed the feature/deep-filtering-mongoose-bookshelf branch from edbd3ca to 07d621b Compare December 9, 2018 15:51

derrickmehaffy mentioned this pull request Dec 15, 2018

Cannot filter on nested relation when using Graphql #2475

Closed

kamalbennani force-pushed the feature/deep-filtering-mongoose-bookshelf branch 2 times, most recently from b711340 to 03614fb Compare December 24, 2018 12:08

mnlbox mentioned this pull request Dec 26, 2018

View relation fields and filter and sort based on realtion field #2543

Closed

2 tasks

kamalbennani force-pushed the feature/deep-filtering-mongoose-bookshelf branch from 8a7834e to 5434289 Compare January 2, 2019 23:28

kamalbennani commented Jan 2, 2019

View reviewed changes

kamalbennani force-pushed the feature/deep-filtering-mongoose-bookshelf branch 3 times, most recently from e2cc569 to a971616 Compare January 3, 2019 21:09

Update the version of strapi-utils inside of CTM

10511bf

kamalbennani added 2 commits February 22, 2019 00:02

Add the possibility to define post processors functions & add a new m…

2a8d7b2

…ethod 'where' in the query class

Use where instead of passing it directly to the count function becaus…

d7cf25f

…e the order of the pipelines matters

kamalbennani mentioned this pull request Feb 23, 2019

Aggregation not send correct values #2151

Closed

Merge branch 'master' into feature/deep-filtering-mongoose-bookshelf

ca51ddb

lauriejim added this to the 3.0.0-alpha.25 milestone Feb 25, 2019

lauriejim added the flag: 💥 Breaking change This PR contains breaking changes and should not be merged label Feb 25, 2019

lauriejim changed the title ~~[Ready For Review / QA] Add Deep Filtering Feature [commit-by-commit]~~ Add Deep Filtering Feature Feb 25, 2019

oritwoen mentioned this pull request Feb 27, 2019

Allow filter on a relation attribute #1948

Merged

8 tasks

Fix loose permissions with Mongo

74c355b

lauriejim mentioned this pull request Feb 28, 2019

GraphQL mutation error column "_id" on SQL #2530

Closed

Merge branch 'master' into feature/deep-filtering-mongoose-bookshelf

3ddcfd2

lauriejim removed this from the 3.0.0-alpha.25 milestone Mar 6, 2019

derrickmehaffy mentioned this pull request Mar 7, 2019

Fix for SQL Mutation Updates #2943

Merged

13 tasks

lauriejim mentioned this pull request Mar 8, 2019

Aggregation not work correctly #2157

Closed

alexandrebodin changed the base branch from master to feature/deep-filtering-mongoose-bookshelf March 11, 2019 10:09

alexandrebodin added this to the 3.0.0-alpha.25 milestone Mar 11, 2019

alexandrebodin merged commit fce66b5 into strapi:feature/deep-filtering-mongoose-bookshelf Mar 11, 2019

alexandrebodin mentioned this pull request Mar 11, 2019

Deep filtering #2961

Merged

24 tasks

strapi locked and limited conversation to collaborators Mar 11, 2019

Add Deep Filtering Feature #2452

Add Deep Filtering Feature #2452

Conversation

kamalbennani commented Dec 8, 2018 • edited

Proposal

Mongoose Proposal

TODOs

Scenarios

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalbennani Dec 9, 2018 • edited

Choose a reason for hiding this comment

kamalbennani commented Dec 10, 2018

lauriejim commented Dec 10, 2018

abdonrd commented Dec 12, 2018

jacargentina commented Dec 18, 2018

lukefanning commented Dec 19, 2018 • edited

jacargentina commented Dec 21, 2018

kamalbennani commented Dec 21, 2018

Choose a reason for hiding this comment

jacargentina commented Jan 4, 2019

kamalbennani commented Jan 4, 2019 • edited

kamalbennani commented Feb 20, 2019

kret13 commented Feb 21, 2019

kamalbennani commented Feb 21, 2019

kamalbennani commented Feb 21, 2019

kret13 commented Feb 22, 2019

lauriejim commented Feb 25, 2019

lauriejim commented Feb 26, 2019

jacargentina commented Mar 6, 2019

derrickmehaffy commented Mar 7, 2019

alexandrebodin commented Mar 11, 2019 • edited

kamalbennani commented Dec 8, 2018 •

edited

kamalbennani Dec 9, 2018 •

edited

lukefanning commented Dec 19, 2018 •

edited

kamalbennani commented Jan 4, 2019 •

edited

alexandrebodin commented Mar 11, 2019 •

edited