Add multi-column pagination support #522
Conversation
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
Nice! I'm curious how you figured out what the appropriate trait bounds are.
Is it worth adding a test? I'm not sure if it's worth generating a whole fake table with fake data...but on the other hand, I found the boolean filter condition easy to get wrong. It's handy to know it works even with data like (1, 1), (1, 2), (1, 3), (2, 1), (2, 2), (2, 3) no matter where the marker is in that sequence.
| dropshot::PaginationOrder::Ascending => { | ||
| if let Some((v1, v2)) = marker { | ||
| query = query.filter(c1.eq(v1.clone()).and(c2.gt(v2))); | ||
| query = query.or_filter(c1.gt(v1)); |
There was a problem hiding this comment.
Two functions provided by Diesel actually made these bounds a little easier for me to write, but admittedly different from your original implementation:
https://docs.diesel.rs/master/diesel/prelude/trait.QueryDsl.html#method.or_filter
https://docs.diesel.rs/master/diesel/prelude/trait.QueryDsl.html#method.then_order_by
These are basically "optional versions" of filter and order, respectively, that can reduce the size of arguments passed.
They should be semantically equivalent to the "single-call, larger argument" version, but the trait bounds get a little less hairy.
I mentioned this on our internal chat channel, but:
Sure, added tests for both the single and multi-column cases with a bespoke table for testing. |
| .execute_async(pool.pool()) | ||
| .await | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
nice. this is a good model for future tests
|
Closing this PR, as it was merged in with #512 |
3 participants
See #512 for context / desired usage.
This extends the
paginatedfunction to a two-column variant, allowing sorting through multiple columns simultaneously.