[-] Smart Field Search - Ease the search on smart fields and deprecate old ways to do it

[VincentMolinie]

Pull Request checklist:

• Write an explicit title for the Pull Request
• Write changes made in the CHANGELOG.md
• Create automatic tests
• Test manually the implemented changes
• Review my own code (indentation, syntax, style, simplicity, readability)
• Wonder if you can improve the existing code

Before

We had to do this kind of thing

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      var searchCondition = {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };

      query.where[Op.and][0][Op.or].push(searchCondition);

      return query;
    }
  }]
});

After

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      return {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };
    }
  }]
});

[SteveBunlon]

Is this planed to be exported to the mongoose liana ?
Also I don't see any related clickup / trello card.
Has this been discussed with the product team ?
Is Marc aware he'll need to update the documentation ?
I let you investigate the test failures.

[SteveBunlon]

[IMPROVEMENT] I don't like that, the where value should initialized as the other, no need to create a function.

[VincentMolinie]

It's async. So I cannot initialized it like the other 😢

[SteveBunlon]

[SCARED] You are returning a promise of promise (wait for the count value and return it directly instead of returning a promise of the value) and you don't wait for the 'getWhere()' promise value here again. The where operator expects an object.

[VincentMolinie]

A promise of a promise is a promise 🤔

[arnaudbesnier]

Pull Request checklist:

• Write an explicit title for the Pull Request
• Write changes made in the CHANGELOG.md
• Create automatic tests
• Test manually the implemented changes
• Review my own code (indentation, syntax, style, simplicity, readability)
• Wonder if you can improve the existing code

Before

We had to do this kind of thing

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      var searchCondition = {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };

      query.where[Op.and][0][Op.or].push(searchCondition);

      return query;
    }
  }]
});

After

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      return {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };
    }
  }]
});

So you can't use include to join if necessary?

[VincentMolinie]

Pull Request checklist:

• Write an explicit title for the Pull Request
• Write changes made in the CHANGELOG.md
• Create automatic tests
• Test manually the implemented changes
• Review my own code (indentation, syntax, style, simplicity, readability)
• Wonder if you can improve the existing code

Before

We had to do this kind of thing

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      var searchCondition = {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };

      query.where[Op.and][0][Op.or].push(searchCondition);

      return query;
    }
  }]
});

After

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      return {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };
    }
  }]
});

So you can't use include to join if necessary?

You are absolutely right. I thought about it, but most of the time includes does not work as expected (For example try with a hasMany...).

I think we need to rework a little bit the smart field to add a requirement property for example
That field could look like requirement: ['users.*', 'companies.{id,name}']...

Tell me what you think about it @arnaudbesnier

[arnaudbesnier]

Pull Request checklist:

• Write an explicit title for the Pull Request
• Write changes made in the CHANGELOG.md
• Create automatic tests
• Test manually the implemented changes
• Review my own code (indentation, syntax, style, simplicity, readability)
• Wonder if you can improve the existing code

Before

We had to do this kind of thing

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      var searchCondition = {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };

      query.where[Op.and][0][Op.or].push(searchCondition);

      return query;
    }
  }]
});

After

collection('customers', {
  fields: [{
    field: 'fullname',
    type: 'String',
    get: (customer) => {
      return customer.firstname + ' ' + customer.lastname;
    },
    search: function (query, search) {
      let s = models.sequelize;
      let split = search.split(' ');

      return {
        [Op.and]: [
          { firstname: { [Op.like]: `%${split[0]}%` } },
          { lastname: { [Op.like]: `%${split[1]}%` } },
        ]
      };
    }
  }]
});

So you can't use include to join if necessary?

You are absolutely right. I thought about it, but most of the time includes does not work as expected (For example try with a hasMany...).

I think we need to rework a little bit the smart field to add a requirement property for example
That field could look like requirement: ['users.*', 'companies.{id,name}']...

Tell me what you think about it @arnaudbesnier

I am not ok to remove flexibility here.

The day developers will update to the new (major?) version containing this code, they will potentially lose their Smart Field search feature (if they used include). There is no alternatives with the current code.

I am not fan of the requirement attribute idea as presented, because:

• it is defined in the field but only impacts the search behaviour.
• from my point of view, all this search configuration/implementation should be located in the search function.

[VincentMolinie]

Actually after thinking about it the include is useless as of right now the call is async so you can do your search on the other model if necessary and then do a id in [myListOfId] for example @arnaudbesnier

[arnaudbesnier]

Ok, I think this something could also have with the current implementation.

What is important is what is removed.
With the new implementation, you won't have the ability to do it in a single SQL query anymore.

[VincentMolinie]

Ok, I think this something could also have with the current implementation.

What is important is what is removed.
With the new implementation, you won't have the ability to do it in a single SQL query anymore.

yeah but like I said it won't work most of the time, because if you try to do it on many relationships it won't work. And belongsTo might work but not like you might expect...