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 global Sequelize hook to make count behavior consistent with find #4320

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

svenaas
Copy link
Contributor

@svenaas svenaas commented Dec 5, 2023

In ORM queries incorporating association joins, Sequelize's count sometimes reports a larger number than the number of rows that Sequelize returns by the same query. This causes inconsistencies between the actual number of returned results and the number of total results reported, a problem which specifically comes up with some of our reports.

This PR adds a global beforeCount hook that changes Sequelize's default behavior to make count consistent. It also introduces a test which fails in the absence of this hook.

In addition to the test case, the misbehavior can be seen in the admin interface at /reports/users:

Without the hook a current local development instance will show 14 rows of user data:
Screenshot 2023-12-05 at 2 30 05 PM

but an inconsistent "total results: 17":
image

With the hook the total is consistent:
Screenshot 2023-12-05 at 2 30 10 PM

Background

In working on #4313 it came to my attention that some of our PaginatedQueryPages were displaying inconsistent information. Specifically, the "total results: n" in PaginationBanner differed from the number of rows shown.

@sknep identified this number as coming from the Sequelize's count via the paginate function in api/utils/index.js, and found other people reporting related issues.

This led me to sequelize/sequelize#9481 and from there to sequelize/sequelize#10557 where various workarounds are discussed. I experimented with simply adding distinct: true, but that caused other things to fail. What did work for me was James M. Greene's beforeCount hook, which I opted to place in api/models/index.js.

Changes proposed in this pull request:

  • Adds global Sequelize hook
  • Adds unit test for this hook in test/api/unit/models/sequelize-hook.test.js — is there a better place for this to live?

security considerations

None. This change is a bug fix that makes a reported count consistent with the size of the result set it refers to.

@svenaas svenaas marked this pull request as ready for review December 5, 2023 20:11
@svenaas svenaas requested a review from a team December 5, 2023 20:11
sknep
sknep previously approved these changes Dec 5, 2023
drewbo
drewbo previously approved these changes Dec 5, 2023
@svenaas svenaas force-pushed the sequelize-hook-for-consistency branch from c05fe29 to 673c39f Compare December 5, 2023 21:17
@svenaas
Copy link
Contributor Author

svenaas commented Dec 5, 2023

Sorry to dismiss your review, @drewbo — I'd forgotten to link directly to the comment where the hook workaround came from.

@svenaas svenaas requested a review from drewbo December 5, 2023 21:18
@svenaas svenaas merged commit 43f7f66 into main Dec 5, 2023
3 checks passed
@svenaas svenaas deleted the sequelize-hook-for-consistency branch December 5, 2023 21:54
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