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

Add options to query Virtual Entities with more flexibility using async and/or QueryBuilder #5475

Closed
aelgasser opened this issue Apr 19, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@aelgasser
Copy link

Is your feature request related to a problem? Please describe.
I'm trying to use the work done here #4740 to query a virtual entity based on a QueryBuider query (in my case a group by with some conditions).

I created an EntitySchema with an expression that returns a QueryBuilder but soon realized that it's being included inside a subquery which I don't want because the where clause I pass through the expression is also passed to the nesting query (here and here) which doesn't work in my case as the properties filtered in the first query don't exist in the virtual entity and the result of the subquery, and thus Postgres rejects it.

export const schema = new EntitySchema({
  name: 'JobMetric',
  properties: {
    jobId: { type: 'string' },
    count: { type: 'number' },
  },
  expression: (em: EntityManager, where, options) => {
    const knex = em.getConnection().getKnex();

    const qb = em
      .createQueryBuilder(
        'Job',
        options.ctx,
        options.connectionType,
        false,
        options.logging
      )
      .groupBy('jobId')
      .select(['jobId', knex.raw('count(id) as count')])
      .where(where);

    return qb;
    // return await qb.execute();
  },
});

I wanted then to use the last line of the findFromVirtual function to return my data directly from the virtual entity by executing the query (see the commented qb.execute above), but the expression is not evaluated in an async context and thus I cannot await the result.

I tried to simply return the Promise object as a result but then the test checking if the result is an object is too naive and is catching the result as a Promise is also an object.

So I'm stuck with this query unless there is another way to get the result of this SQL in a virtual entity:

SELECT job_id, count(id) as count 
FROM job
WHERE job_owner = 'bob' -- the condition here is dynamic and can be more complex
GROUP BY job_id

Finally for instance here is the full query it is generating:

select * from (
  select "g0"."job_id", count(id) as count from "job" as "g0" 
  where "g0"."job_id" in ('5e3c760e91603606bc83ea15', '5e3c795fb95ecea263d16a27', '5e3c7bddb95ece6641d16a77') 
  and "g0"."score" = 10 
  group by "g0"."job_id"
) as "g0" 
where "g0"."job_id" in ('5e3c760e91603606bc83ea15    ', '5e3c795fb95ecea263d16a27', '5e3c7bddb95ece6641d16a77') 
and "g0"."score" = 10 

Which fails with the error: - column g0.score does not exist (the last line above)

Describe the solution you'd like

Either fix (1) this to await on the expression or fix (2) this to ignore Promises (and type https://github.com/mikro-orm/mikro-orm/blob/master/packages/knex/src/AbstractSqlDriver.ts#L223 as maybe a Promise) or add an option to not wrap the QueryBuilder in a subquery (3) here

Ideally, fix all to allow more flexibility, but at least (1).

Another solution would be to not pass the where clause to the sub-query here but that might break other use cases and is still sub-optimal as I don't need/want to have a sub-query.

Happy to discuss other ways that I may have overlooked.

Additional context
This is blocking me from producing some reporting features.

@aelgasser aelgasser added the enhancement New feature or request label Apr 19, 2024
@B4nan
Copy link
Member

B4nan commented Apr 19, 2024

I don't mind awaiting the expression, that sounds safe to do, same for ignoring promises in the second place.

PR welcome

@B4nan
Copy link
Member

B4nan commented Apr 20, 2024

So it's not gonna be that simple, we can't await the expression as that would mean executing the QB, both knex and the ORM query builder can be awaited directly. So 1. is not an option, but we can do 2. which will work out the same as you wanted.

@B4nan B4nan closed this as completed in ee98412 Apr 20, 2024
@aelgasser
Copy link
Author

@B4nan You were faster than me on this, I started a PR and through unit testing found as well that await doesn’t work, there are at least 2 other places that fail. Testing for promise did work with no regression on the unit tests. I was going to submit the PR later today.

Thank you!

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

2 participants