Expand the scope of seek-based pagination on search #7983
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7983 +/- ##
==========================================
- Coverage 87.67% 87.55% -0.12%
==========================================
Files 271 271
Lines 26750 26574 -176
==========================================
- Hits 23452 23268 -184
- Misses 3298 3306 +8 ☔ View full report in Codecov by Sentry. |
Turbo87
added
C-enhancement ✨
eth3lbert
force-pushed
the
search-seek
branch
from
be2d3da to
f3921c6
Compare
src/controllers/krate/search.rs
Outdated
|
|
||
| query = query.order(crates::created_at.desc()); | ||
| seek = Some(Seek::New); | ||
| query = query.order((crates::created_at.desc(), crates::id.asc())); |
There was a problem hiding this comment.
If larger ids signify newer items with the same created_at timestamp, it might be more intuitive to switch the sorting order from ascending crates::id.asc() to descending crates::id.desc().
There was a problem hiding this comment.
If larger
idssignify newer items with the samecreated_attimestamp
yep, that's how it would be. I'm not sure if it's worth breaking the consistency. I haven't checked, but I would be surprised if we actually had crates in the database with the exact same created_at timestamp.
There was a problem hiding this comment.
If we can guarantee that timestamps will never be identical, then the auxiliary ordering column becomes unnecessary and can be removed. However, if the possibility of duplicate timestamps exists, then it's better to keep it.
Since we all use descending order for sorting, I think it would be more intuitive to change all the auxiliary column id.asc() to id.desc(). What do you think?
There was a problem hiding this comment.
I think it would be more intuitive to change all the auxiliary column
id.asc()toid.desc()
yeah, that should also work :)
There was a problem hiding this comment.
It is implemented in the new commit.
src/tests/routes/crates/list.rs
Outdated
| // Test for bug with showing null results first when sorting | ||
| // by descending downloads | ||
| let json = anon.search("sort=recent-downloads"); | ||
| assert_eq!(json.meta.total, 4); | ||
| assert_eq!(json.crates[0].name, "foo_sort"); | ||
| assert_eq!(json.crates[1].name, "baz_sort"); | ||
| assert_eq!(json.crates[2].name, "bar_sort"); | ||
| assert_eq!(json.crates[3].name, "other_sort"); | ||
| for json in search_both(&anon, "sort=recent-downloads") { | ||
| assert_eq!(json.meta.total, 4); | ||
| assert_eq!(json.crates[0].name, "foo_sort"); | ||
| assert_eq!(json.crates[1].name, "baz_sort"); | ||
| assert_eq!(json.crates[2].name, "bar_sort"); | ||
| assert_eq!(json.crates[3].name, "other_sort"); | ||
| } | ||
| let (resp, calls) = page_with_seek(&anon, "sort=recent-downloads"); | ||
| assert_eq!(resp[0].crates[0].name, "foo_sort"); | ||
| assert_eq!(resp[1].crates[0].name, "baz_sort"); | ||
| assert_eq!(resp[2].crates[0].name, "bar_sort"); | ||
| assert_eq!(resp[3].crates[0].name, "other_sort"); | ||
| assert_eq!(resp[3].meta.total, 4); | ||
| assert_eq!(calls, 5); |
There was a problem hiding this comment.
This code snippet can be safely removed as it's already covered by above test case.
|
☔ The latest upstream changes (presumably #8050) made this pull request unmergeable. Please resolve the merge conflicts. |
eth3lbert
force-pushed
the
search-seek
branch
from
868daa6 to
f001731
Compare
|
Rebased and resolved all merge conflicts. |
eth3lbert
force-pushed
the
search-seek
branch
from
f001731 to
64d05d5
Compare
…pagination on search
Since we all use descending order for sorting, it would be more intuitive to change all the auxiliary column from ascending to descending.
|
Rebased once more. AFAICT all of the existing functionality should still work the same as before, so I think we can try this out as an experimental feature. If it works, great, if it doesn't, we can always revert :) |
This PR add seek-based pagination support for all sorting.
This PR is best reviewed commit-by-commit.
Optimization opportunities remain, but not within this PR:
per_page, we could check for additional records by querying withper_page + 1. If the actual count exceedsper_page, it indicates there's a next page; otherwise, the next page is None. This eliminates unnecessary requests when no further pages exist.