Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto nesting sub-queries on $and clauses on joined tables #2677

Closed
alexbadm opened this issue Jan 27, 2022 · 9 comments
Closed

Auto nesting sub-queries on $and clauses on joined tables #2677

alexbadm opened this issue Jan 27, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@alexbadm
Copy link

Is your feature request related to a problem? Please describe.

I'm always frustrated when I need an ordinary query for an entity with two linked entities (m:n relation), but need to switch to query builder and construct nested queries.

Usual cases when usual em.find is not suitable:

  • Find user with two friends (on a user's profile page I want to see mutual friends for me and the user)
  • Find a transport company which works in two countries (when I want to find a supplier which can deliver from one country to another, I need a company which works in both countries)
  • Any filter which allows multiple selection (tags, labels, etc. with m:n relation, and has to find entities having all selected items)

Describe the solution you'd like

I would like to make such queries with usual em.find syntax, e.g.:

const companies = await em.find(Company, {
  $and: [
    { country: { name: 'Canada' } },
    { country: { name: 'Argentina' } },
  ],
});

Describe alternatives you've considered

The current available alternative in mikro-orm is to construct a nested query with QueryBuilder, which is verbose and poorly readable. Also this disallows us to construct more complex queries, each time we need to focus on constructing SQL instead of being focusing on the application logic.

const subquery = em.createQueryBuilder(Company, 'c1')
  .select('id')
  .leftJoin('c1.country', 'c2')
  .where({
    'c2.name': 'Canada',
  });

const companies = await em.createQueryBuilder(Company, 'c1')
  .select()
  .leftJoin('c1.country', 'c2')
  .where({
    id: { $in: subquery.getKnexQuery() },
    'c2.name': 'Argentina',
  })
  .groupBy('c1.id')
  .execute();

I suggest to extend the where parser to detect $and clauses on connected relations and automatically make sub-queries.

@alexbadm alexbadm added the enhancement New feature or request label Jan 27, 2022
@alexbadm
Copy link
Author

I've started working on it, here is my progress:
https://github.com/CareerLunch/mikro-orm/commit/b9ad9b1242ef5e26aa10a1baeb2ad3f1a36e04dd

Please, take a look, maybe you know better place for this change in code.

Also added some functions to Utils, but most likely it won't be used anywhere else, so maybe it's better to place it as local functions? Just wanted to have them tested.

@B4nan
Copy link
Member

B4nan commented Jan 27, 2022

Instead of subqueries, I would rather see multiple joins as discussed on slack. And I believe the place is wrong (maybe not if we want subqueries, but I dont think we want them), this needs to happen in CriteriaNode and its child classes (that is where the mapping happens), definitly not in driver - that layer has nothing to do with this.

Instead of trying to fix the query in driver, you need to "not break it" earlier. CriteriaNode is what handles auto joining and mapping of joins between each other.

@B4nan
Copy link
Member

B4nan commented Jan 27, 2022

One hint for testing, this needs to be resolved also for QB usage - QB itself is handling this via CriteriaNode here:

https://github.com/mikro-orm/mikro-orm/blob/master/packages/knex/src/query/QueryBuilder.ts#L200-L207

In other words, this needs to do the same too:

const companies = await em.createQueryBuilder(Company).where({
  $and: [
    { country: { name: 'Canada' } },
    { country: { name: 'Argentina' } },
  ],
}).getResult();

@B4nan
Copy link
Member

B4nan commented Jan 27, 2022

One more hint: qb.getAliasForJoinPath() is what we use to map those conditions to existing joins. If this method return undefined, it will create new join branch. If we temporarily make this method always return undefined, this test will be passing:

  test('GH issue 2677', async () => {
    const qb1 = orm.em.createQueryBuilder(Book2);
    qb1.select('*').where({
      $and: [
        { tags: { name: 'tag1' } },
        { tags: { name: 'tag2' } },
      ],
    });
    expect(qb1.getQuery()).toEqual('select `e0`.*, `e0`.price * 1.19 as `price_taxed` ' +
      'from `book2` as `e0` ' +
      'left join `book2_tags` as `e2` on `e0`.`uuid_pk` = `e2`.`book2_uuid_pk` ' +
      'left join `book_tag2` as `e1` on `e2`.`book_tag2_id` = `e1`.`id` ' +
      'left join `book2_tags` as `e4` on `e0`.`uuid_pk` = `e4`.`book2_uuid_pk` ' +
      'left join `book_tag2` as `e3` on `e4`.`book_tag2_id` = `e3`.`id` ' +
      'where `e1`.`name` = ? and `e3`.`name` = ?');
  });

@B4nan B4nan mentioned this issue Jan 27, 2022
48 tasks
@B4nan
Copy link
Member

B4nan commented Jan 28, 2022

I've been playing with this and got it working for the top level $and, trying out more complex cases now. Do I get it right that you would expect this behaviour:

const qb = orm.em.createQueryBuilder(Book2);
qb.select('*').where({
  $and: [
    {
      author: {
        $and: [
          { favouriteBook: { title: 'a1' } },
          { favouriteBook: { title: 'a2' } },
        ],
      },
    },
    {
      author: {
        $and: [
          { favouriteBook: { title: 'a3' } },
          { favouriteBook: { title: 'a4' } },
        ],
      },
    },
  ],
});
expect(qb.getQuery()).toEqual('select `e0`.*, `e0`.price * 1.19 as `price_taxed` ' +
  'from `book2` as `e0` ' +
  'left join `author2` as `e1` on `e0`.`author_id` = `e1`.`id` ' +
  'left join `book2` as `e2` on `e1`.`favourite_book_uuid_pk` = `e2`.`uuid_pk` ' +
  'left join `book2` as `e3` on `e1`.`favourite_book_uuid_pk` = `e3`.`uuid_pk` ' +
  'left join `author2` as `e4` on `e0`.`author_id` = `e4`.`id` ' +
  'left join `book2` as `e5` on `e4`.`favourite_book_uuid_pk` = `e5`.`uuid_pk` ' +
  'left join `book2` as `e6` on `e4`.`favourite_book_uuid_pk` = `e6`.`uuid_pk` ' +
  'where `e2`.`title` = ? and `e3`.`title` = ? and `e5`.`title` = ? and `e6`.`title` = ?');

edit: I guess this make sense only for M:N relations, right? here its m:1 relations

@alexbadm
Copy link
Author

Hi, that's great!
Yes, it only makes sense for m:n and 1:m and not to m:1, so favouriteBook's clauses should merge.

@alexbadm
Copy link
Author

I've also been playing with this for a while. I updated the code of ObjectCriteriaNode.shouldAutoJoin:

  private shouldAutoJoin(nestedAlias: string | undefined): boolean {
    if (!this.prop || !this.parent) {
      return false;
    }

    const embeddable = this.prop.reference === ReferenceType.EMBEDDED;
    const knownKey = [ReferenceType.SCALAR, ReferenceType.MANY_TO_ONE, ReferenceType.EMBEDDED].includes(this.prop.reference) || (this.prop.reference === ReferenceType.ONE_TO_ONE && this.prop.owner);
    const operatorKeys = knownKey && Object.keys(this.payload).every(key => Utils.isOperator(key, false));
    const primaryKeys = knownKey && Object.keys(this.payload).every(key => {
      const meta = this.metadata.find(this.entityName)!;
      if (!meta.primaryKeys.includes(key)) {
        return false;
      }
      if (!Utils.isPlainObject(this.payload[key].payload) || ![ReferenceType.ONE_TO_ONE, ReferenceType.MANY_TO_ONE].includes(meta.properties[key].reference)) {
        return true;
      }
      return Object.keys(this.payload[key].payload).every(k => meta.properties[key].targetMeta!.primaryKeys.includes(k));
    });

    // here are the added lines
    const relationToMany = [ReferenceType.MANY_TO_MANY, ReferenceType.ONE_TO_MANY].includes(this.prop.reference);
    const isArrayItem = !this.parent.key && Utils.isGroupOperator(this.parent.parent?.key ?? '');

    // return !primaryKeys && !nestedAlias && !operatorKeys && !embeddable;
    return !primaryKeys && (!nestedAlias || relationToMany && isArrayItem) && !operatorKeys && !embeddable;
  }

And this makes the trick but adds extra joins:

// searching authors that have both cheap and expensive books
const qb = orm.em.createQueryBuilder(Author2).where({
  $and: [
    { books2: { price: { $lt: 100 } } },
    { books2: { price: { $gt: 1000 } } },
  ],
});
console.log(qb.getQuery());
select `e0`.* from `author2` as `e0`
    left join `book2` as `e1` on `e0`.`id` = `e1`.`author_id`
    left join `book2` as `e2` on `e0`.`id` = `e2`.`author_id`
    left join `book2` as `e3` on `e0`.`id` = `e3`.`author_id`
    left join `book2` as `e4` on `e0`.`id` = `e4`.`author_id`
where `e1`.`price` < ? and `e2`.`price` > ?

There is something I missed. I'm going to proceed my tests on Monday.

@B4nan
Copy link
Member

B4nan commented Jan 29, 2022

I think I already found the right way to do it, without any side effects. All that's needed is to adjust the getPath method of CriteriaNode to respect indexes in $and clauses (and save such indexes when we create the node structure via factory).

So I don't really need you to work on this more, but I would very much welcome some additional tests.

Yes, it only makes sense for m:n and 1:m and not to m:1, so favouriteBook's clauses should merge.

Both Book.author and Author.favouriteBook are m:1, so in that example there should be no branching, right? But if they would be both m:n/1:m, it would work the way I proposed.

I also don't think we need to care about $or branching (which your code would do).

@sixty4bit
Copy link

The left joins are not created on nested entities. I'm trying to do something along the lines of:

const qb = orm.em.createQueryBuilder(Author2).where({
  $and: [
    { books2: { genre: { name: { $in: ['Fantasy', 'Murder/Mystery'] } } } },
    { books2: { tags: { name: { $in: ['tag1', 'tag2'] } } } },
  ]
});
console.log(qb.getQuery());

I need a left join on both tags and genre. Any help is appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants