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

No proper pagination support with query builder #2823

Closed
andrew-sol opened this issue Feb 25, 2022 · 10 comments
Closed

No proper pagination support with query builder #2823

andrew-sol opened this issue Feb 25, 2022 · 10 comments

Comments

@andrew-sol
Copy link

andrew-sol commented Feb 25, 2022

I'm currently migrating from TypeORM and I've found that MikroORM lacks pagination support for QueryBuilder. Well, it works, until I need to add any leftJoinAndSelect().

There are a few issues actually (when we add leftJoinAndSelect()):

  1. getCount() returns wrong count
  2. limit() also limits joined relations
  3. orderBy() causes SQL error (when used with or without groupBy())

Using findAndCount() or QueryFlag.PAGINATE is not an option for me.

Let's imagine such a simple example:

@Entity()
export class User {
  @PrimaryKey()
  id: number;

  @OneToMany(() => Comment, (comment) => comment.user)
  comments = new Collection<Comment>(this);
}

And imagine that we have 2 users, each of them has 10 comments.

Query 1:

const query = getEntityManager()
  .getRepository(User)
  .qb('user')
  .select('*')
  .leftJoinAndSelect('user.comments', 'comments')
  .where({ id: 1 }); // let's select just one user here

const count = await query.getCount(); // returns 10, should be 1

Query 2:

const query = getEntityManager()
  .getRepository(User)
  .qb('user')
  .select('*')
  .leftJoinAndSelect('user.comments', 'comments')
  .limit(2);

const data = await query.getResultList();
// 1) data contains only 1 user (should be all 2)
// 2) data[0].comments contains only 2 comments

Query 3. Let's try adding orderBy():

const query = getEntityManager()
  .getRepository(User)
  .qb('user')
  .select('*')
  .leftJoinAndSelect('user.comments', 'comments')
  .limit(2)
  .orderBy({ 'user.id': 'DESC' });

const count = await query.getCount();

// SQL Error:
// select count("user"."id") as "count" from "users" as "user" left join "comments" as "comments" on "user"."id" = "comments"."user_id" where "user"."id" = 1 order by "user"."id" desc - 
// column "user.id" must appear in the GROUP BY clause or be used in an aggregate function

Query 4. Let's add groupBy():

const query = getEntityManager()
  .getRepository(User)
  .qb('user')
  .select('*')
  .leftJoinAndSelect('user.comments', 'comments')
  .limit(2)
  .groupBy('user.id')
  .orderBy({ 'user.id': 'DESC' });

const count = await query.getCount();

// SQL Error:
// select "user".*, "comments"."id" as "comments__id", "comments"."created_at" as "comments__created_at", "comments"."updated_at" as "comments__updated_at", "comments"."user_id" as "comments__user_id", "comments"."text" as "comments__text", "comments"."nft_id" as "comments__nft_id", "comments"."parent_id" as "comments__parent_id" from "users" as "user" left join "comments" as "comments" on "user"."id" = "comments"."user_id" where "user"."id" = 1 group by "user"."id" order by "user"."id" desc limit 2 - 
//  column "comments.id" must appear in the GROUP BY clause or be used in an aggregate function

That's it. TypeORM handles pagination nicely with their skip() and take() that we should use instead of limit() and offset() if we have at least one leftJoinAndSelect() on the query builder. They wrap the count query into SELECT COUNT(DISTINCT("user"."id")) as "cnt" FROM (...main query here...) and also do an additional query without limits to avoid limiting joined relations (though I don't know all the details).

Is there some way around these issues right now?

Versions

Dependency Version
node 16.13.2
typescript 4.5.5
mikro-orm 5.0.4
your-driver PostgreSQL
@B4nan
Copy link
Member

B4nan commented Feb 25, 2022

They wrap the count query into SELECT COUNT(DISTINCT("user"."id")) as "cnt" FROM (...main query here...) and also do an additional query without limits to avoid limiting joined relations (though I don't know all the details).

Sounds exactly like what QueryFlag.PAGINATE does, why do you say its not an option for you?

edit: The plan was to actually make this flag implicit if you join to-many relation in QB.

@andrew-sol
Copy link
Author

QueryFlag.PAGINATE is not an option because it's usable only inside find query. And query builder gives more control over the select query. Sometimes I need to add subselects for "virtual" columns and sometimes I have some complex filters with subqueries. Also, there is just one paragraph about pagination in the docs right now so I might not know something.

@B4nan
Copy link
Member

B4nan commented Feb 25, 2022

QueryFlag.PAGINATE is not an option because it's usable only inside find query.

You can use it with QB too via qb.setFlag(). The em.find() method basically just calls QB, most of the features are implemented in the driver/QB (e.g. autojoining).

@andrew-sol
Copy link
Author

andrew-sol commented Feb 25, 2022

Seems like adding setFlag(QueryFlag.PAGINATE) prevents limiting joined relations in getResultList() but getCount() still counts everything like before. And it's still not possible to do orderBy().

@B4nan
Copy link
Member

B4nan commented Feb 25, 2022

Then please provide details for that. I am not going to add skip/take, I don't see a reason for different API doing the same thing, instead I do want to enable this paginate flag based on what is joined.

Looking at qb.getCount(), that indeed resets limit/offset (and it is not connected to the paginate flag anyhow). Why would you want to use count query together with pagination? You can just call qb.execute() if you don't like that. The reason for this is that you can clone a QB (that already has a limit/offset) and call getCount() on it, which should ignore the limit/offset.

@andrew-sol
Copy link
Author

Minimal pagination interface looks like this:

{
  limit: number,
  offset: number,
  total: number,
  data: any[],
}

We need total here to be able to count the total pages available based on limit.
The problem with getCount() is that it counts joined relations when it should not.

I just discovered that if I call it like this:

query.getCount('DISTINCT("user"."id")')

then it works the way I need.

I suppose it could do this distinct wrapping automatically.

@B4nan
Copy link
Member

B4nan commented Feb 25, 2022

We need total here to be able to count the total pages available based on limit.

You will need a separate call to get the total number of results, I hope we don't need to argue about that. I know there are some driver specific ways to do it in a single query (in mysql at least), tried it some time ago and it is much worse performance wise than a separate query.

I just discovered that if I call it like this:

There is a second boolean argument to enable disctinct query, in both qb.count() and qb.getCount() (or you can use QueryFlag.DISTINCT). Enabling it by default is technically a breaking change, so no plans to do it any time soon.

I am still waiting for details regarding what is not working with paginate + order by.

@andrew-sol
Copy link
Author

Right, I didn't notice that second arg at first. So query.getCount('user.id', true) is much better.

About orderBy(). The query:

const query = getEntityManager()
  .getRepository(User)
  .qb('user')
  .select('*')
  .leftJoinAndSelect('user.comments', 'comments')
  .orderBy({ 'user.id': 'DESC' })
  .setFlag(QueryFlag.PAGINATE);

const count = await query.getCount('user.id', true);

This query fails if I call getCount() on it:

select count(distinct("user"."id")) as "count" from "users" as "user" left join "comments" as "comments" on "user"."id" = "comments"."user_id" order by "user"."id" desc - 
column "user.id" must appear in the GROUP BY clause or be used in an aggregate function

It's not critical because I can move orderBy() into the .orderBy(...).getResultList() call.
Though getCount() could also strip "ORDER BY" as well as it does it with limit/offset, I suppose.

@B4nan
Copy link
Member

B4nan commented Feb 25, 2022

Though getCount() could also strip "ORDER BY" as well as it does it with limit/offset, I suppose.

Yeah, that sounds reasonable.

So to wrap up, the paginate flag works fine (except this one thing), and we can close this issue with enabling it by default when we detect to-many join?

@andrew-sol
Copy link
Author

Right. Thanks for your help.

@B4nan B4nan closed this as completed in db9963f Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants