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 cursor pagination desc order #2037

Merged
merged 11 commits into from Jan 14, 2024
Merged

Add cursor pagination desc order #2037

merged 11 commits into from Jan 14, 2024

Conversation

ziimakc
Copy link
Contributor

@ziimakc ziimakc commented Dec 29, 2023

New Features

Add cursor pagination desc order support

Usage:

cursor_by().asc() or cursor_by().desc()


Moved query builder logic inside all because it depends on sort order asc/desc

@ziimakc
Copy link
Contributor Author

ziimakc commented Dec 30, 2023

It seems more complicated then I expected, for example for the following cases

first=2,after=5,asc 6,7 is id>5 ASC
last=2,after=5,asc 9,10 is id>5 DESC + reverse result
last=2,before=5,asc 3,4 is id<5 DESC + reverse result
first=2,before=5,asc 1,2 is id<5 ASC 

first=2,after=5,desc 4,3 is id<5 DESC
last=2,after=5,desc 2,1 is id<5 ASC + reverse result
last=2,before=5,desc 7,6 is id>5 ASC + reverse result
first=2,before=5,desc 10,9 is id>5 DESC

first=2,after=5,before=9,asc 6,7 is id>5 and id<9 ASC
last=2,after=5,before=9,asc 7,8 is id>5 and id<9 DESC + reverse result
last=2,after=5,before=1,desc 3,2 is id<5 and id>1 ASC + reverse result
first=2,after=5,before=1,desc 4,3 is id<5 and id>1 DESC

We can say rules is that:

  • reverse sort order in sql query when last is used
  • if order reversed, reverse results
  • for after cursor use > when asc is used
  • for before cursor use > when desc is used

Will test it out and see how it works

@ziimakc
Copy link
Contributor Author

ziimakc commented Dec 30, 2023

Will add mock query tests a bit later Done.

@ziimakc ziimakc changed the title WIP: Add cursor pagination desc order Add cursor pagination desc order Dec 31, 2023
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Happy new year. Thank you! This is sure a lot of effort, and it seems to be working. I am not too concerned as long as our test coverage increases! (which leads to some comments below)

),])]
);

Ok(())
}

#[smol_potat::test]
async fn composite_keys_5() -> Result<(), DbErr> {
async fn composite_keys_5_desc() -> Result<(), DbErr> {
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid changing existing test cases, since it shrinks our test coverage. Can you extract the common parts and make it a new test case?

Copy link
Contributor Author

@ziimakc ziimakc Jan 12, 2024

Choose a reason for hiding this comment

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

I didn't change any existing tests, but duplicated all existing ones with desc order, it shows a bit incorrectly by github, I assume because names composite_keys_5 and composite_keys_5_desc and the rest of the test body except after and first replaced by before, last and desc which produces same query, but reversed results

Copy link
Member

@tyt2y3 tyt2y3 Jan 14, 2024

Choose a reason for hiding this comment

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

Oh thanks! Got it now

].join(" "))
);

assert_eq!(
DbBackend::Postgres.build(&
Entity::find()
.cursor_by((Column::Col1, Column::Col2, Column::Col3, Column::Col4, Column::Col5, Column::Col6))
.after(("val_1", "val_2", "val_3", "val_4", "val_5", "val_6"))
.before(("val_1", "val_2", "val_3", "val_4", "val_5", "val_6")).desc().apply_limit().apply_order_by().apply_filters()
Copy link
Member

@tyt2y3 tyt2y3 Jan 12, 2024

Choose a reason for hiding this comment

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

Likewise, it's better to test both before and after separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as my previous comment, it's a copy of cursor_by_many named as cursor_by_many_desc, no tests were removed or changed except additions like .apply_limit().apply_order_by().apply_filters() because filters applied when we call all()

@tyt2y3 tyt2y3 merged commit c5adb9d into SeaQL:master Jan 14, 2024
30 of 31 checks passed
Copy link

🎉 Released In 0.12.11 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

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.

None yet

2 participants