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

fix(knex): Fix total calculation #3525

Open
wants to merge 1 commit into
base: dove
Choose a base branch
from

Conversation

CITIZENDOT
Copy link

Summary

  • Tell us about the problem your pull request is solving.
    • When the query contains DISTINCT or GROUP BY, incorrect total is being returned. This PR fixes it by calculating the count using a subquery. More details below:
    • When DISTINCT clause is used, clearSelect() removes the DISTINCT clause in the countBuilder query, hence returning non-distinct count. For eg:
      • SELECT DISTINCT `users.*` FROM `users` WHERE ....
      • is converted to
      • SELECT `*` FROM `users` WHERE ...
      • losing the distinct property of the query.
    • When GROUP BY clause is used, GROUP BY is applied on COUNT(`${name}.${id}`), which is useless, because rows returned by COUNT will not contain any column other than total to GROUP BY. Hence non-distinct rows are returned again.
    • Hence the solution is, to wrap the main query into a subquery, and count the rows returned by it, which is what this PR does.
  • Are there any open issues that are related to this?
    • No. But this is a bug, I can create an issue if necessary.
  • Is this PR dependent on PRs in other repos?
    • No

@CITIZENDOT CITIZENDOT marked this pull request as draft August 15, 2024 11:55
@CITIZENDOT CITIZENDOT marked this pull request as ready for review August 15, 2024 12:07
@daffl daffl changed the title Fix total calculation fix(knex): Fix total calculation Sep 2, 2024
@daffl
Copy link
Member

daffl commented Sep 2, 2024

I think this makes sense, however, the tests are failing with a TypeScript error at https://github.com/feathersjs/feathers/actions/runs/10403476191/job/29583715944?pr=3525#step:8:698

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

Successfully merging this pull request may close these issues.

2 participants