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

Recent Distinct Count change broke existing query. #3182

Closed
jamesmeneghello opened this issue Jun 4, 2022 · 5 comments
Closed

Recent Distinct Count change broke existing query. #3182

jamesmeneghello opened this issue Jun 4, 2022 · 5 comments
Labels
bug Something isn't working

Comments

@jamesmeneghello
Copy link
Contributor

Describe the bug

The change in eebe34d broke some existing queries for us. Downgrading @mikro-orm/knex to 5.1.4 works as expected.

const initiatedTradesQuery = em
  .createQueryBuilder(Trade)
  .where({
      ...
  })
  .count('id', true);

Stack trace

stack: "error: select count(distinct(*)) as \"count\" from \"trade\" as \"t0\" where ...' - syntax error at or near \"*\"\n    at Parser.parseErrorMessage (/home/james/src/glx-lazarus/.yarn/cache/pg-protocol-npm-1.5.0-390f8d9ed8-b839d12caf.zip/node_modules/pg-protocol/src/parser.ts:369:69)\n    at Parser.handlePacket (/home/james/src/glx-lazarus/.yarn/cache/pg-protocol-npm-1.5.0-390f8d9ed8-b839d12caf.zip/node_modules/pg-protocol/src/parser.ts:188:21)\n    at Parser.parse (/home/james/src/glx-lazarus/.yarn/cache/pg-protocol-npm-1.5.0-390f8d9ed8-b839d12caf.zip/node_modules/pg-protocol/src/parser.ts:103:30)\n    at Socket.<anonymous> (/home/james/src/glx-lazarus/.yarn/cache/pg-protocol-npm-1.5.0-390f8d9ed8-b839d12caf.zip/node_modules/pg-protocol/src/index.ts:7:48)

Versions

Dependency Version
node 16.5
typescript 4.4.4
mikro-orm 5.1.5
your-driver postgresql
@B4nan
Copy link
Member

B4nan commented Jun 4, 2022

Reproduction? We have tests like this: https://github.com/mikro-orm/mikro-orm/blob/master/tests/QueryBuilder.test.ts#L881-L886

(works the same without .setFlag(QueryFlag.DISTINCT), that's probably just for coverage)

@jamesmeneghello
Copy link
Contributor Author

Reproduced it, but I'm not entirely sure if this is actually a bug or just a weird interaction. tl;dr: the combination of qb.count('id', true) and later calling it with getCount() causes it. Correctly using getCount('id', true) later works fine.

import { Entity, ManyToOne, MikroORM, PrimaryKey, Property } from '@mikro-orm/core';
import type { SqlEntityManager } from '@mikro-orm/knex';

@Entity()
export class Trade {

  @PrimaryKey()
  id!: number;

    @Property()
    tradeStart: Date = new Date();

}

describe('GH issue 3182', () => {

  let orm: MikroORM;

  beforeAll(async () => {
    orm = await MikroORM.init({
      entities: [Trade],
      type: 'postgresql',
      dbName: 'mikro_orm_test_3182',
    });
    await orm.getSchemaGenerator().refreshDatabase();
  });

  afterAll(async () => {
    await orm.close(true);
  });

  test(`GH issue 3182`, async () => {
    const em = orm.em.fork() as SqlEntityManager;

    const qb = em.createQueryBuilder(Trade)
    .count('id', true);

    await qb.getCount();
  });

});

error: select count(distinct(*)) as "count" from "trade" as "t0" - syntax error at or near "*"

@B4nan
Copy link
Member

B4nan commented Jun 5, 2022

Yeah you dont need both, qb.getCount() creates a clone of the QB and removes limit/offset/order by automatically. But it should indeed work, we could skip the cloning in case we see the QB type is already count type.

Btw the same reproduces if you await such QB too, as awaiting count QB will call qb.getCount() without params.

@B4nan B4nan added bug Something isn't working and removed needs clarification labels Jun 5, 2022
@B4nan B4nan closed this as completed in a97324a Jun 5, 2022
@jamesmeneghello
Copy link
Contributor Author

Thanks for the quick fix!

@tufailra97
Copy link

In my case, I wanted to join and group the results by id as well as selecting fields conditionally, so the solution proposed above and in here didn't work. So I ended up with this:

const qb = this.em
      .createQueryBuilder(Role, 'role')
      .where({ account, isActive: true });

    if (filters?.name?.length) {
      qb.andWhere({ name: { $ilike: `%${filters.name}%` } });
    }

    const total = await qb.getCount();

    qb.select([
      'id',
      'name',
      'description',
      'isActive',
      'isAdmin',
      'permissions',
      'createdAt',
      'updatedAt'
    ])
      .orderBy([
        { name: filters?.nameOrder },
        { createdAt: filters?.createdAtOrder }
      ])
      .limit(filters.pageSize)
      .offset(getOffset(filters.pageIndex, filters.pageSize));

    if (filters?.includeUserCount) {
      qb.addSelect(['count(users) as "userCount"']);
      qb.leftJoin('role.users', 'users').groupBy('role.id');
    }

    const roles = await qb.execute();

where I first get the count, then the actual results.

Generated queries:

select count(*) as "count" from "roles" as "role" where "role"."account_id" = 'ffb2d02d-5a70-40b1-a62f-90d6c515b4ab' and "role"."is_active" = true
-- 
select "role"."id", "role"."name", "role"."description", "role"."is_active", "role"."is_admin", "role"."permissions", "role"."created_at", "role"."updated_at", count(users) as "userCount" from "roles" as "role" left join "user_profiles" as "users" on "role"."id" = "users"."role_id" where "role"."account_id" = 'ffb2d02d-5a70-40b1-a62f-90d6c515b4ab' and "role"."is_active" = true group by "role"."id" order by "role"."name" asc, "role"."created_at" desc limit 20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants