Skip to content

Conversation

@fpnuseis
Copy link
Contributor

@fpnuseis fpnuseis commented Dec 4, 2025

This PR continues the work from the abandoned PR #975:
https://github.com/ppetzold/nestjs-paginate/pull/975

The original PR introduced raw-fetch support but also proposed a new count-related config option.
Since the current version of the library already provides buildCountQuery, which fully satisfies the count customization needs, the additional count config was not included.

Copilot AI review requested due to automatic review settings December 4, 2025 16:37
Copilot finished reviewing on behalf of fpnuseis December 4, 2025 16:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new fetchRaw configuration option to enable retrieving raw SQL results from TypeORM queries, which is particularly useful for aggregated or computed fields.

Key changes:

  • Added fetchRaw boolean option to PaginateConfig interface
  • Updated pagination logic to use getRawMany() instead of getMany() when fetchRaw is enabled
  • Added comprehensive test with SQL aggregates using a new SaleEntity for validation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/paginate.ts Adds fetchRaw config option and implements conditional logic to use getRawMany() across different pagination scenarios (standard pagination, cursor pagination, and non-paginated queries)
src/paginate.spec.ts Adds test infrastructure (repository, entity imports) and a comprehensive test case demonstrating fetchRaw with SQL aggregates and custom count query
src/tests/sale.entity.ts Introduces a new test entity for sales data with decimal columns to support aggregate function testing
README.md Documents the new fetchRaw configuration option with clear description, use cases, and important notes about SQL aliases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@fpnuseis fpnuseis marked this pull request as draft December 4, 2025 16:44
Ensures consistent line endings across the project
by replacing CRLF with LF.

This improves collaboration and prevents potential
cross-platform compatibility issues.
@fpnuseis
Copy link
Contributor Author

fpnuseis commented Dec 4, 2025

@ppetzold
I need help.
buildCountQuery currently takes a cloned QueryBuilder, adjusts it, and returns another QueryBuilder.
Later, the library calls .getCount() on that returned builder.

The problem is: TypeORM’s getCount() cannot correctly count grouped results.
Even if the user sets their own select, groupBy, or distinct, getCount() internally rebuilds the count query and ends up counting base table rows instead of grouped rows.

Because of this limitation, I’m considering redesigning the buildCountQuery API so that:

Instead of returning a QueryBuilder,

buildCountQuery returns a number (or a Promise),
allowing the user to execute their own count query freely.

For example:

buildCountQuery: async (qb) => {
    const sql = qb.orderBy().limit().offset().take().skip().getQuery();

    const result = await qb
        .createQueryBuilder()
        .select('COUNT(*)', 'total_rows')
        .from(`(${sql})`, 'query_count')
        .setParameters(qb.getParameters())
        .getRawOne();

    return Number(result.total_rows);
}

This makes it possible for users to:

  • Run raw subquery counts
  • Count grouped results correctly
  • Avoid TypeORM’s restrictive getCount() behavior
  • Fully customize the count logic for raw/aggregate queries

What do you think about this redesign?
I’m also open to different ideas if you have alternative suggestions.

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.

1 participant