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

Pagination plugin ignores transactions #1625

Merged
merged 3 commits into from Mar 3, 2018

Conversation

@jordansexton
Copy link
Contributor

@jordansexton jordansexton commented Aug 3, 2017

Pagination plugin ignores transactions

Introduction

This fixes #1623 and fixes #1645 by passing the the current transaction (if any) to both the pager and the counter methods. This fix may conflict with changes made in #1561 (superseding #1272 and #1261). Perhaps @vellotis wants to weigh in?

jordansexton added 2 commits Aug 3, 2017
This fixes bookshelf#1623 by passing the options used (including the current transaction, if any) to both the pager and the counter methods.
Passing only the `transacting` property means that `withRelated`, `required`, and other options will only apply to the pager fetch.
@@ -174,7 +175,7 @@ module.exports = function paginationPlugin (bookshelf) {
});
qb.countDistinct.apply(qb, [`${tableName}.${idAttribute}`]);

}).fetchAll().then(result => {
}).fetchAll({transacting}).then(result => {

Choose a reason for hiding this comment

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

would it make more sense to pass in fetchOptions directly here?
I'm using the bookshelf-paranoia plugin and it passes in withDeleted option, which pulls in records if they're softdeleted or not. Since it doesn't get passed to the pagination plugin, it only counts rows that are not soft deleted.

Loading

Copy link
Contributor Author

@jordansexton jordansexton Aug 11, 2017

Choose a reason for hiding this comment

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

I think passing all options might be a problem, as this includes passing things like require. I'd argue that the require must apply to the fetchAll of the rows you want, not the count. I don't know if this matters in practice, or why the count is using a fetchAll rather than a fetch anyway. It would also be passing withRelated, which will fail in the worst case or be wasteful in the best case. Maybe just blacklisting some options to exclude could work, though it presents some forward compatibility issues.

Loading

@Zakini
Copy link

@Zakini Zakini commented Nov 29, 2017

Any plans to merge this?

Loading

@ricardograca
Copy link
Member

@ricardograca ricardograca commented Nov 29, 2017

@mrhwick might have a different opinion, but I think this PR needs to have at least one test added for this before it's merged.

Loading

@mrhwick
Copy link
Member

@mrhwick mrhwick commented Nov 29, 2017

A regression test would be useful. I'll take a look at #1623 and see if I can derive a reproduction case from there to write a regression test for this change.

Loading

@ricardograca
Copy link
Member

@ricardograca ricardograca commented Jan 17, 2018

@jordansexton Can you add tests to verify the correct behavior of this bug fix?

Loading

@ricardograca ricardograca moved this from To Do to In Progress in Version 0.13.0 Mar 3, 2018
Copy link
Member

@ricardograca ricardograca left a comment

Looks good and fixes two issues at once. I'll add a test for these changes after this is merged.

Loading

@ricardograca ricardograca merged commit 449f073 into bookshelf:master Mar 3, 2018
1 check passed
Loading
Version 0.13.0 automation moved this from In Progress to Done Mar 3, 2018
ricardograca referenced this issue Mar 3, 2018
Add test for fetchPage inside a transaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

5 participants