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

Progress time received from API can be negative #363

Closed
bamode opened this issue Oct 15, 2022 · 5 comments
Closed

Progress time received from API can be negative #363

bamode opened this issue Oct 15, 2022 · 5 comments
Labels
bug Something isn't working help wanted Extra attention is needed Stale

Comments

@bamode
Copy link

bamode commented Oct 15, 2022

Describe the bug
This issue effects specifically the structs CurrentlyPlayingContext and CurrentPlaybackContext when deserializing the Spotify API response that corresponds to the progress field of these structs. It appears to be parsing it as a u32 to turn into a std::time::Duration and this part of the API response is occasionally a negative number. This results in upstream bugs in the spotifyd and spotify-tui projects.

Expected behavior
The API deserialization should be able to replace an unexpected negative time value with some default.

@bamode bamode added bug Something isn't working help wanted Extra attention is needed labels Oct 15, 2022
@ramsayleung
Copy link
Owner

Thanks for your report, The Spotify's documentation doesn't point out the progress could be a negative number.

If progress could be a negative number, then parsing it as an u32 to turn into a std::time::Duration is inappropriate. std::time::Duration doesn't support negative value.

I would like to replace std::time::Duration to chrono::Duration, which supports negative value. It would be a break change.

@ramsayleung
Copy link
Owner

If we consider replacing std::time::Duration to chrono::Duration, we need to replace all fields using std::time::Duration to chrono::Duration. Otherwise, it will become inconsistent that some fields are using std::time::Duration to represent time, other fields are using chrono::Duration to represent time.

@marioortizmanero
Copy link
Collaborator

Do we know in which cases we receive a negative value? Another option is to have a custom serializer that sets these cases to zero, though I do prefer to represent the API as closely as possible with chrono::Duration.

@ramsayleung
Copy link
Owner

As @bamode points out, the progress field in CurrentlyPlayingContext could be negative. I think it doesn't make sense if we set these cases to zero.

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Jun 24, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed Stale
Projects
None yet
Development

No branches or pull requests

3 participants