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 API #5

Closed
tyt2y3 opened this issue May 21, 2021 · 27 comments · Fixed by #10 or #20
Closed

Pagination API #5

tyt2y3 opened this issue May 21, 2021 · 27 comments · Fixed by #10 or #20
Assignees

Comments

@tyt2y3
Copy link
Member

tyt2y3 commented May 21, 2021

A new struct Paginator that wraps a Select, that would:

  1. lazily COUNT the total number of records and number of pages
  2. perform paging by LIMIT and OFFSET

A helper method to convert a select into a paginator: (something like)

fn paginate(page_size: i32) -> Paginator<T>

The Paginator should impl the Stream trait.

Ref:
https://docs.rs/futures-core/0.3.15/futures_core/stream/trait.Stream.html
https://tokio.rs/tokio/tutorial/streams
https://docs.rs/async-std/1.9.0/async_std/stream/trait.Stream.html

@tyt2y3 tyt2y3 changed the title Pagination API design Pagination API May 21, 2021
@tyt2y3 tyt2y3 mentioned this issue May 21, 2021
36 tasks
@billy1624 billy1624 self-assigned this May 26, 2021
@billy1624
Copy link
Member

billy1624 commented May 26, 2021

So, both Select and SelectTwo can be converted into Paginator?

let's say...

  • Select => SelectPaginator
  • SelectTwo => SelectTwoPaginator

@tyt2y3
Copy link
Member Author

tyt2y3 commented May 26, 2021

Yes, any Select* can be converted into Paginator.
But no, there should only be one Paginator, which is generic over any Select.
Similar idea to the built in traits Iterator and IntoIterator.

@billy1624
Copy link
Member

Getting Stream from db connection

pub trait Connection {
    // new
    async fn fetch(&self, stmt: Statement) -> Pin<Box<dyn Stream<Item = Result<QueryResult, QueryErr>>>>;
}

Will be impl by SelectModel and SelectTwoModel to generalize the conversion from QueryResult to Item

i.e. Item returned by one() and all()

  • FromQueryResult
  • (FromQueryResult, FromQueryResult)
  • serde_json::Value
  • etc...
pub trait QueryResultConverter {
    type Item;

    fn convert_query_result(res: &QueryResult) -> Result<Self::Item, QueryErr>;
}

Paginator that accept any QueryResultConverter and stream corresponding Item

pub struct Paginator<C>
where
    C: QueryResultConverter,
{
    pub(crate) query: SelectStatement,
    pub(crate) convertor: PhantomData<C>,
}

impl<C> Paginator<C>
where
    C: QueryResultConverter,
{
    pub async fn fetch(self) -> Pin<Box<dyn Stream<Item = Result<C::Item, QueryErr>>>> {
        todo!();
        let item: C::Item = C::convert_query_result(&row);
    }
}

Still thinking who to keep the pagination state

  • page_num
  • page_size

@billy1624
Copy link
Member

billy1624 commented May 27, 2021

The Paginator...

use crate::{Database, QueryErr, SelectorTrait};
use futures::Stream;
use sea_query::SelectStatement;
use std::{
    marker::PhantomData,
    pin::Pin,
    task::{Context, Poll},
};

#[derive(Clone, Debug)]
pub struct Paginator<'d, S>
where
    S: SelectorTrait,
{
    pub(crate) query: SelectStatement,
    pub(crate) page: usize,
    pub(crate) page_size: usize,
    pub(crate) buffer: Vec<S::Item>,
    pub(crate) db: &'d Database,
}

impl<'d, S> Paginator<'d, S>
where
    S: SelectorTrait,
{
    pub fn new(query: SelectStatement, page_size: usize, db: &'d Database) -> Self {
        Self {
            query,
            page: 0,
            page_size,
            buffer: Vec::with_capacity(page_size),
            db,
        }
    }
}

impl<S> Stream for Paginator<'_, S>
where
    S: SelectorTrait,
{
    type Item = S::Item;

    fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
        todo!();
    }
}

@billy1624
Copy link
Member

Go see see the POC on pagination branch

@tyt2y3
Copy link
Member Author

tyt2y3 commented May 27, 2021

Need to print some timestamps to assess whether the time sequence is right

@billy1624
Copy link
Member

What do u mean? loll

@billy1624
Copy link
Member

@billy1624
Copy link
Member

@tyt2y3 check pagination branch again

@tyt2y3
Copy link
Member Author

tyt2y3 commented May 28, 2021

Can we avoid the turbo fish?

fruit::Entity::find().paginate::<fruit::Model>

@billy1624
Copy link
Member

Can we avoid the turbo fish?

fruit::Entity::find().paginate::<fruit::Model>

Why this is called turbo fish? loll

@tyt2y3
Copy link
Member Author

tyt2y3 commented May 29, 2021

@billy1624
Copy link
Member

See latest commit

@billy1624
Copy link
Member

Just figure out how to Pin the Stream, so user don't have to Pin it manually

But the method signature be like...

// paginator.rs

pub type PinBoxStream<'db, Item> = Pin<Box<dyn Stream<Item = Item> + 'db>>;

pub fn into_stream(mut self) -> PinBoxStream<'db, Result<Vec<S::Item>, QueryErr>> { ... }
pub fn into_stream_future(mut self) -> PinBoxStream<'db, impl Future<Output = Result<Vec<S::Item>, QueryErr>>> { ... }

@billy1624
Copy link
Member

Trying to get rid of stream! macro

Ref: https://gist.github.com/TimLuq/83a35453405f4c6e0f63fb2a0caa9f6e

@tyt2y3
Copy link
Member Author

tyt2y3 commented May 31, 2021

Please open a PR when it is ready.

@tyt2y3
Copy link
Member Author

tyt2y3 commented Jun 6, 2021

Can you add some test cases to Paginator?

@tyt2y3
Copy link
Member Author

tyt2y3 commented Jun 6, 2021

Some how it stuck in infinite loop when running the example

@billy1624
Copy link
Member

Some how it stuck in infinite loop when running the example

I got the same issue. Fixed on latest commit, error caused by code refactoring.

@billy1624
Copy link
Member

Can you add some test cases to Paginator?

How should we test the Paginator?

Perform assert_eq between each elements from...

  • Entity::find().all()
  • Stream.try_next()

Is this a reasonable test case?

@tyt2y3
Copy link
Member Author

tyt2y3 commented Jun 7, 2021

Oh it was my bad. But this is the exact problem we'd like to catch in test cases.
Since paging works across multiple database calls, it could only be fully tested with a mock connection.

@billy1624
Copy link
Member

So unit test for paginator will be implemented once mock connection is available?

@tyt2y3
Copy link
Member Author

tyt2y3 commented Jun 7, 2021

Seems so.

@tyt2y3 tyt2y3 closed this as completed Jun 7, 2021
@billy1624
Copy link
Member

Will mark it down on todo list

@tyt2y3 tyt2y3 reopened this Jun 13, 2021
@tyt2y3
Copy link
Member Author

tyt2y3 commented Jun 13, 2021

There is now MockDatabase for testing

let db = MockDatabase::new()

@billy1624 billy1624 linked a pull request Jun 14, 2021 that will close this issue
@billy1624
Copy link
Member

See the draft PR for the first paginator unit test. See if that make sense.

@billy1624
Copy link
Member

billy1624 commented Jun 14, 2021

  1. Should we add some helper to avoid this?

    // TODO: auto impl
    impl From<fruit::Model> for MockRow {
    fn from(model: fruit::Model) -> Self {
    let map = maplit::btreemap! {
    "id" => Into::<Value>::into(model.id),
    "name" => Into::<Value>::into(model.name),
    "cake_id" => Into::<Value>::into(model.cake_id),
    };
    map.into()
    }
    }

  2. And should we introduce trait IntoMockRow to avoid the manual into.

    let db = MockDatabase::new()
    .append_query_results(vec![
    // TODO: take any IntoMockRow
    page1
    .clone()
    .into_iter()
    .map(|model| Into::<MockRow>::into(model))
    .collect(),
    page2
    .clone()
    .into_iter()
    .map(|model| Into::<MockRow>::into(model))
    .collect(),
    ])
    .into_database();

  3. Also, I cannot get the transaction log from MockDatabase once it turns into Database.

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 a pull request may close this issue.

2 participants