Skip to content

Commit

Permalink
fix double referenced types in paginated endpoints
Browse files Browse the repository at this point in the history
This removes the potential double reference to Id types (&'a Id<'_>)
which was previously required due to limitations in the async variant of
the `paginate` function.
Due to limitations in HRTBs (see
https://kevincox.ca/2022/04/16/rust-generic-closure-lifetimes/) it is
not possible, to write something like `Req: for<'a> Fn(&'a) ->
('a + Future<...>)`. The sync version remains almost unchanged.
  • Loading branch information
eladyn committed Jul 9, 2022
1 parent 0b63f89 commit b2bfaff
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 31 deletions.
63 changes: 39 additions & 24 deletions src/clients/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::{
auth_urls,
clients::{
convert_result,
pagination::{paginate, Paginator},
pagination::{paginate, paginate_with_ctx, Paginator},
},
http::{BaseHttpClient, Form, Headers, HttpClient, Query},
join_ids,
Expand Down Expand Up @@ -314,13 +314,20 @@ where
/// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-artists-albums)
fn artist_albums<'a>(
&'a self,
artist_id: &'a ArtistId<'_>,
artist_id: ArtistId<'a>,
album_type: Option<AlbumType>,
market: Option<Market>,
) -> Paginator<'_, ClientResult<SimplifiedAlbum>> {
paginate(
move |limit, offset| {
self.artist_albums_manual(artist_id, album_type, market, Some(limit), Some(offset))
paginate_with_ctx(
(self, artist_id),
move |(slf, artist_id), limit, offset| {
slf.artist_albums_manual(
artist_id.as_ref(),
album_type,
market,
Some(limit),
Some(offset),
)
},
self.get_config().pagination_chunks,
)
Expand All @@ -329,7 +336,7 @@ where
/// The manually paginated version of [`Self::artist_albums`].
async fn artist_albums_manual(
&self,
artist_id: &ArtistId<'_>,
artist_id: ArtistId<'_>,
album_type: Option<AlbumType>,
market: Option<Market>,
limit: Option<u32>,
Expand Down Expand Up @@ -467,18 +474,21 @@ where
/// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-an-albums-tracks)
fn album_track<'a>(
&'a self,
album_id: &'a AlbumId<'_>,
album_id: AlbumId<'a>,
) -> Paginator<'_, ClientResult<SimplifiedTrack>> {
paginate(
move |limit, offset| self.album_track_manual(album_id, Some(limit), Some(offset)),
paginate_with_ctx(
(self, album_id),
move |(slf, album_id), limit, offset| {
slf.album_track_manual(album_id.as_ref(), Some(limit), Some(offset))
},
self.get_config().pagination_chunks,
)
}

/// The manually paginated version of [`Self::album_track`].
async fn album_track_manual(
&self,
album_id: &AlbumId<'_>,
album_id: AlbumId<'_>,
limit: Option<u32>,
offset: Option<u32>,
) -> ClientResult<Page<SimplifiedTrack>> {
Expand Down Expand Up @@ -631,12 +641,13 @@ where
/// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-a-shows-episodes)
fn get_shows_episodes<'a>(
&'a self,
id: &'a ShowId<'_>,
id: ShowId<'a>,
market: Option<Market>,
) -> Paginator<'_, ClientResult<SimplifiedEpisode>> {
paginate(
move |limit, offset| {
self.get_shows_episodes_manual(id, market, Some(limit), Some(offset))
paginate_with_ctx(
(self, id),
move |(slf, id), limit, offset| {
slf.get_shows_episodes_manual(id.as_ref(), market, Some(limit), Some(offset))
},
self.get_config().pagination_chunks,
)
Expand All @@ -645,7 +656,7 @@ where
/// The manually paginated version of [`Self::get_shows_episodes`].
async fn get_shows_episodes_manual(
&self,
id: &ShowId<'_>,
id: ShowId<'_>,
market: Option<Market>,
limit: Option<u32>,
offset: Option<u32>,
Expand Down Expand Up @@ -996,15 +1007,16 @@ where
/// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-playlists-tracks)
fn playlist_items<'a>(
&'a self,
playlist_id: &'a PlaylistId<'_>,
playlist_id: PlaylistId<'a>,
fields: Option<&'a str>,
market: Option<Market>,
) -> Paginator<'_, ClientResult<PlaylistItem>> {
paginate(
move |limit, offset| {
self.playlist_items_manual(
paginate_with_ctx(
(self, playlist_id, fields),
move |(slf, playlist_id, fields), limit, offset| {
slf.playlist_items_manual(
playlist_id.as_ref(),
fields,
*fields,
market,
Some(limit),
Some(offset),
Expand Down Expand Up @@ -1050,18 +1062,21 @@ where
/// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-list-users-playlists)
fn user_playlists<'a>(
&'a self,
user_id: &'a UserId<'_>,
user_id: UserId<'a>,
) -> Paginator<'_, ClientResult<SimplifiedPlaylist>> {
paginate(
move |limit, offset| self.user_playlists_manual(user_id, Some(limit), Some(offset)),
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,
)
}

/// The manually paginated version of [`Self::user_playlists`].
async fn user_playlists_manual(
&self,
user_id: &UserId<'_>,
user_id: UserId<'_>,
limit: Option<u32>,
offset: Option<u32>,
) -> ClientResult<Page<SimplifiedPlaylist>> {
Expand Down
11 changes: 11 additions & 0 deletions src/clients/pagination/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ use crate::{model::Page, ClientError, ClientResult};
/// Alias for `Iterator<Item = T>`, since sync mode is enabled.
pub type Paginator<'a, T> = Box<dyn Iterator<Item = T> + 'a>;

pub fn paginate_with_ctx<'a, Ctx: 'a, T: 'a, Request: 'a>(
ctx: Ctx,
req: Request,
page_size: u32,
) -> Paginator<'a, ClientResult<T>>
where
Request: Fn(&Ctx, u32, u32) -> ClientResult<Page<T>>,
{
paginate(move |limit, offset| req(&ctx, limit, offset), page_size)
}

/// This is used to handle paginated requests automatically.
pub fn paginate<'a, T: 'a, Request: 'a>(
req: Request,
Expand Down
8 changes: 6 additions & 2 deletions src/clients/pagination/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
//! * A `Paginator` struct which wraps the iterable of items
//! * A `paginate` function, which returns a `Paginator` based on a request that
//! may be repeated in order to return a continuous sequence of `Page`s
//! * A `paginate_with_ctx` function that does the same as the `paginate`
//! function, but accepts a generic context that works around lifetime issues
//! in the async version due to restrictions in HRTBs
//! (https://kevincox.ca/2022/04/16/rust-generic-closure-lifetimes/)
//!
//! Note that `Paginator` should actually be a trait so that a dynamic
//! allocation can be avoided when returning it with `-> impl Iterator<T>`, as
Expand All @@ -25,6 +29,6 @@ mod iter;
mod stream;

#[cfg(feature = "__sync")]
pub use iter::{paginate, Paginator};
pub use iter::{paginate, paginate_with_ctx, Paginator};
#[cfg(feature = "__async")]
pub use stream::{paginate, Paginator};
pub use stream::{paginate, paginate_with_ctx, Paginator};
27 changes: 27 additions & 0 deletions src/clients/pagination/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>>>>;

/// This is used to handle paginated requests automatically.
pub fn paginate_with_ctx<'a, Ctx: 'a, T: 'a, Request: 'a>(
ctx: Ctx,
req: Request,
page_size: u32,
) -> Paginator<'a, ClientResult<T>>
where
T: Unpin,
Request: for<'ctx> Fn(&'ctx Ctx, u32, u32) -> RequestFuture<'ctx, T>,
{
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: 'a, Fut, Request: 'a>(
req: Request,
page_size: u32,
Expand Down
6 changes: 3 additions & 3 deletions tests/test_with_credential.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ async fn test_album_tracks() {
let birdy_uri = AlbumId::from_uri("spotify:album:6akEvsycLGftJxYudPjmqK").unwrap();
creds_client()
.await
.album_track_manual(&birdy_uri, Some(2), None)
.album_track_manual(birdy_uri, Some(2), None)
.await
.unwrap();
}
Expand All @@ -71,7 +71,7 @@ async fn test_artists_albums() {
creds_client()
.await
.artist_albums_manual(
&birdy_uri,
birdy_uri,
Some(AlbumType::Album),
Some(Market::Country(Country::UnitedStates)),
Some(10),
Expand Down Expand Up @@ -208,7 +208,7 @@ mod test_pagination {
let album = AlbumId::from_uri(ALBUM).unwrap();

let names = client
.album_track(&album)
.album_track(album)
.map(|track| track.unwrap().name)
.collect::<Vec<_>>()
.await;
Expand Down
4 changes: 2 additions & 2 deletions tests/test_with_oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist {
.await
.unwrap();
assert_eq!(playlist.id, fetched_playlist.id);
let user_playlists = fetch_all(client.user_playlists(&user.id)).await;
let user_playlists = fetch_all(client.user_playlists(user.id)).await;
let current_user_playlists = fetch_all(client.current_user_playlists()).await;
assert_eq!(user_playlists.len(), current_user_playlists.len());

Expand All @@ -642,7 +642,7 @@ async fn check_playlist_create(client: &AuthCodeSpotify) -> FullPlaylist {

#[maybe_async]
async fn check_num_tracks(client: &AuthCodeSpotify, playlist_id: PlaylistId<'_>, num: i32) {
let fetched_tracks = fetch_all(client.playlist_items(&playlist_id.as_ref(), None, None)).await;
let fetched_tracks = fetch_all(client.playlist_items(playlist_id, None, None)).await;
assert_eq!(fetched_tracks.len() as i32, num);
}

Expand Down

0 comments on commit b2bfaff

Please sign in to comment.