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

feat: add support for getRawMany() and row count as it is #975

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

okaprinarjaya
Copy link

@okaprinarjaya okaprinarjaya commented Sep 11, 2024

Hi,

In this PR, I try to solve two following issues:

  1. Need to include in query sum of amounts of relations #265 --> Primary issue that I need to solve.
  2. GROUP BY removed in count query when using getManyAndCount() typeorm/typeorm#5127 , Bug - .groupBy is empty on count query, resulting in wrong count when .groupBy is used typeorm/typeorm#7454

I totally open for any feedback, and I do really need @ppetzold guidance to improve my changes in this PR.

Thank You

@okaprinarjaya okaprinarjaya changed the title Feature: Add support for getRawMany(), and flexible row count Feature: Add support for getRawMany(), and row count as it is Sep 11, 2024
Copy link
Owner

@ppetzold ppetzold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid at first sight. Could you add new config params description to README?

@ppetzold ppetzold changed the title Feature: Add support for getRawMany(), and row count as it is feat: add support for getRawMany() and row count as it is Sep 12, 2024
@okaprinarjaya
Copy link
Author

okaprinarjaya commented Sep 12, 2024

Looks solid at first sight. Could you add new config params description to README?

Hi,
Thank you for your reply,
Ok I will help to update the documentation (README).

And I have some concerns

  1. about usage of queryBuilderCloned . I need your opinion, about that. Is it better to use queryBuilderCloned or move lines: https://github.com/ppetzold/nestjs-paginate/blob/master/src/paginate.ts#L222-L230 into below / under line: 389, then we focus on process the counting first, save the counting result, and then execute data rows fetching. - Fixet at: b6d4478
  2. about custom getCount() method that mimick typeorm's getCount() method. I need your opinion about this. Do we really need to create a custom getCount() method that "override" typeorm's getCount() method? because at line: 393 using typeorm's getManyAndCount() method. If you're Ok / align with my custom getCount() idea, then we can adjusts to remove the usage of typeorm's getManyAndCount() method. - Yes! we need it to easily switch to typeorm's original getCount() or to the as it is count.
  3. I'm not confident with naming of two new config attribute: 1. getDataAsRaw , 2. rowCountAsItIs . Do you have better naming idea?

@Helveg
Copy link
Collaborator

Helveg commented Sep 28, 2024

Hi! I'd like to weigh in here on point 3:

  • 3.1: returnRaw, fetchRaw
  • 3.2: preserveRowCount, exactRowCount

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.

3 participants