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

First draft for write pagination #298

Closed
wants to merge 1 commit into from

Conversation

nick-lehmann
Copy link

@nick-lehmann nick-lehmann commented Feb 17, 2022

Description

Add a method to paginate write requests to the Spotify API. Fixes #296.

This is currently a draft. I'm still quite new to Rust and struggle with async Rust code. At the moment, the changes do not work and I'm looking for some feedback by someone more experienced than me.

Motivation and Context

Spotify imposes a limit on items not only for reading endpoints but also for endpoints that create data. Currently, the api client will simply fail in case the given items exceed the limit. This PR adds pagination for methods like playlist_add_items.

Dependencies

None

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

Adapted integration tests to check that data is successfully created even if the items exceeds the limits of the Spotify API.

Is this change properly documented?

Not yet. Will do that as soon as the feature is complete.

@nick-lehmann
Copy link
Author

@marioortizmanero @ramsayleung it would be very kind of you if you would have a quick look at the approach I have taken. At the moment, the code does not work because I pass a closure to my write_paginate function. The closure gets the current chunk of items, transform them into the payload and executes the endpoint_post method.

However, the self reference to the oauth client (here) does not live long enough to do this. I was unable to fix this issue. May you please give me a hint where I got things wrong and how I could improve on my design? I hope I did formulate my problem adequatly 😅

@marioortizmanero
Copy link
Collaborator

It would probably be easier if you started with the synchronous version, and then moved on to the asynchronous one. The self borrow issue can be solved by using generics for the Future instead:

pub async fn write_paginate<'a, Item, Writer: 'a, Fut>(
    writer: Writer,
    items: Vec<Item>,
    chunk_size: u32,
) -> String
where
    Writer: Fn(&[Item], u32) -> Fut,
    Fut: futures::Future<Output = Result<String, ClientError>>,
    Item: Display + Debug,

The problem then becomes that you're creating params inside the function, and it's borrowed by the future you're returning. params would drop at the end of the closure, but you haven't awaited the future. Option 1 would be to create params separately. Option 2 is to await the future in the closure, but that would require async closures, which are not stable yet. Option 3 would be to make write_paginate a macro instead. But I think it's a design issue, maybe it can be done with a different approach, also knowing these other things:

  • Don't unwrap, propagate the error
  • The returned values should be combined. Returning the last one only doesn't make sense for e.g. current_user_saved_albums_contains.
  • Maybe different naming? Not sure what write_paginate refers to.

@github-actions
Copy link

Message to comment on stale PRs. If none provided, will not mark PRs stale

@github-actions github-actions bot added the Stale label Jun 27, 2023
@github-actions github-actions bot closed this Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically do paginated post requests
2 participants