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

Count unique occurrences of a column when using distinct to calculate pagination details #1950

Merged

Conversation

Projects
None yet
2 participants
@davidgf
Copy link
Contributor

commented Feb 19, 2019

Introduction

The pagination plugin is returning the wrong rowCount when the query contains a SELECT DISTINCT statement.

Motivation

In one of the projects we're working on, we need to paginate different occurrences of a column within a table. When we reached the last page, we've noticed that we were still getting pagination details. Not only so, but if we requested a page with an offset greater that the total, we still got pagination details along with an empty set of results. This was due to calculating the rowCount incorrectly.

Fixes #1948.

Proposed solution

When counting the total number of rows, the pagination plugin is excluding all columns statements from the grouping columns, even when they are part of a distinct statement. Therefore, the resulting count query selects all the records in the table, rather than only unique occurrences of a column in that table, and returning a wrong rowCount. The proposed solution simply keeps columns statements when distinct is true.

@ricardograca
Copy link
Member

left a comment

Looks good to me. Thanks for the fix.

@ricardograca ricardograca merged commit b686fb7 into bookshelf:master Feb 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidgf davidgf deleted the davidgf:fix-pagination-for-distinct-selects branch Feb 19, 2019

@davidgf

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Thank you for accepting it 😄 Do you know when the fix could be released in npm?

@ricardograca

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Possibly in the next version (1.0.0), if not earlier.

@ricardograca

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

This was released in version 0.15.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.