aliatech / loopback-mongo-aggregate-mixin

Loopback mixin to query MongoDB aggregation pipeline and build the instances from results
MIT License
8 stars 4 forks source link

$unwind stage is not built for hasMany relation fields #10

Open DavidKrpt opened 4 years ago

DavidKrpt commented 4 years ago

there's a test implying that

it('Should aggregate where hasMany relation properties with $unwind', callback)

but after logging the resulting pipeline, I saw that it had no $unwind stage.

The only place in the code where the $unwind stage is built is in the buildLookup function in an if block:

  function buildLookup (aggregate, where, parentRelation = null) {
    _.each(where, (value, key) => {
      const keys = key.split('.');
      const headKey = keys.shift();
      const relation = (parentRelation ? parentRelation.modelTo : Model).relations[headKey];
      if (!relation) return;
      const lookupOpts = buildLookupOptsFromRelation(relation, parentRelation);
      aggregate.lookup(lookupOpts);
      if (_.includes(['hasOne', 'belongsTo'], relation.type)) {
        let unwindPath = relation.name;
        const parentRelationName = _.get(parentRelation, 'name', '');
        if (parentRelationName.length) {
          unwindPath = `${parentRelationName}.${unwindPath}`;
        }
        aggregate.unwind({
          path: `$${ unwindPath}`,
          preserveNullAndEmptyArrays: true,
        });
      }
      /* istanbul ignore else */
      if (keys.length) {
        buildLookup(aggregate, {[keys.join('.')]: value}, relation);
      }
    });
  }

the $unwind stage is built only when the following condition is true.

if (_.includes(['hasOne', 'belongsTo'], relation.type))

this seemed a bit odd. I tried 2 things:

  1. inverting the condition so that $unwind is built only for hasMany relations.
  2. removing the if block altogether.

In both cases all the tests passed, and the results of my requests stayed the same: hasMany relational field filters work only if the relation is 1 level deep and the field by which I'm filtering is not a foreign key. I'm still investigating and I would appreciate any suggestion !

By the way, what does this comment mean / istanbul ignore else / ? :)

Thanks in advance and sorry for jumping in with multiple issues.

Akeri commented 4 years ago

Hi David, thank you for using this module and taking your time reporting issues. I would like to be able to review it calmly and fix them. Meanwhile feel free to send PR if you find a solution. Thank you!

By the way, /* istanbul ignore else */ means that the else block for that "if" should be ignored for the coverage percetange. Actually there is no "else" but Istambull was detecting it as a coverage lack.