Fix double referenced types in paginated endpoints #337
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,34 @@ use futures::{future::Future, stream::Stream}; | |
| /// Alias for `futures::stream::Stream<Item = T>`, since async mode is enabled. | ||
| pub type Paginator<'a, T> = Pin<Box<dyn Stream<Item = T> + 'a>>; | ||
|
|
||
| pub type RequestFuture<'a, T> = Pin<Box<dyn 'a + Future<Output = ClientResult<Page<T>>>>>; | ||
|
There was a problem hiding this comment. I have to say, It's a little headache to understand this function signature 😂 There was a problem hiding this comment. I see what you mean, coming up with it wasn't easy either. 😀 In essence it's mostly the signature of a |
||
|
|
||
| /// This is used to handle paginated requests automatically. | ||
| pub fn paginate_with_ctx<'a, Ctx: 'a, T, Request>( | ||
| ctx: Ctx, | ||
| req: Request, | ||
| page_size: u32, | ||
| ) -> Paginator<'a, ClientResult<T>> | ||
| where | ||
| T: 'a + Unpin, | ||
| Request: 'a + for<'ctx> Fn(&'ctx Ctx, u32, u32) -> RequestFuture<'ctx, T>, | ||
|
There was a problem hiding this comment. I am not sure if I understand the design correctly, so I try to retell the story. What the signature means is to allow So that the returned value of the All the thing we want to do is to specify that the lifetime of There was a problem hiding this comment. That sounds correct. I'll add my version, so you can double check. :) The problem we need to solve is: When we move anything into the closure ( The way we solve it: We pass the reference from outside and await the future nearly immediately afterwards. As such, the lifetime bounds are upheld and the result we get no longer references Due to the limitations in HRTBs, we can't formulate a bound at this time that works with a raw There was a problem hiding this comment. I am a little confused about this:
fn user_playlists<'a>(
&'a self,
user_id: UserId<'a>,
) -> Paginator<'_, ClientResult<SimplifiedPlaylist>> {
paginate_with_ctx(
(self, user_id),
move |(slf, user_id), limit, offset| {
slf.user_playlists_manual(user_id.as_ref(), Some(limit), Some(offset))
},
self.get_config().pagination_chunks,
)
}We pass the
According to the function signature: By the way, I realize Rust is hard, I still have a lot to learn. 😂 There was a problem hiding this comment.
What I meant by that is: pub fn paginate_with_ctx<'a, Ctx: 'a, T, Request>(...) -> Paginator<'a, ClientResult<T>>
where
T: 'a + Unpin,
Request: 'a + for<'ctx> Fn(&'ctx Ctx, u32, u32) -> RequestFuture<'ctx, T>,
{
let mut offset = 0;
Box::pin(stream! {
loop {
// we create a reference to ctx here
let page = req(&ctx, page_size, offset).await?;
// due to `await`, the reference is not used again after this line
offset += page.items.len() as u32;
for item in page.items {
// doesn't reference ctx, so yielding the items is fine
yield Ok(item);
}
...
}
})
}
The function itself takes a reference and so does the There was a problem hiding this comment. I get your point, it makes sense: pub fn paginate_with_ctx<'a, Ctx: 'a, T, Request>(...) -> Paginator<'a, ClientResult<T>>
where
T: 'a + Unpin,
Request: 'a + for<'ctx> Fn(&'ctx Ctx, u32, u32) -> RequestFuture<'ctx, T>,
{
let mut offset = 0;
Box::pin(stream! {
loop {
// we create a reference to ctx here.
// the `&ctx` is a shortlived reference, it's just like we put `ctx` into a bounded scope.
// ---- scope begins here ----
let page = req(&ctx, page_size, offset).await?;
// due to `await`, the reference is not used again after this line
// ------ scope ends here -----
offset += page.items.len() as u32;
for item in page.items {
// doesn't reference ctx, so yielding the items is fine
yield Ok(item);
}
...
}
})
}It's clear now :) |
||
| { | ||
| use async_stream::stream; | ||
| let mut offset = 0; | ||
| Box::pin(stream! { | ||
| loop { | ||
| let page = req(&ctx, page_size, offset).await?; | ||
| offset += page.items.len() as u32; | ||
| for item in page.items { | ||
| yield Ok(item); | ||
| } | ||
| if page.next.is_none() { | ||
| break; | ||
| } | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| pub fn paginate<'a, T, Fut, Request>(req: Request, page_size: u32) -> Paginator<'a, ClientResult<T>> | ||
| where | ||
| T: 'a + Unpin, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little confused that what's
ctxfor? What's purpose ofctxvariable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the sync case: not much. This is only to keep compatibility with the async version, which needs those. I'll try to explain why on the other review comments.