Fix playlist metadata fields parsing

[eladyn]

On the new-api branch, running the playlist_tracks example failed for me on all playlists I tried. (e.g., spotify:playlist:44PATzOl8NaSihxJQs02h6)

T22:31:02Z ERROR librespot_core::session] could not dispatch command: Service unavailable { error handling Mercury response: MercuryResponse { uri: "hm://pusher/v1/connections/some_random_long_string_dont_know_if_this_is_sensitive", status_code: 200, payload: [] } }
[core/src/spotify_id.rs:151] &src = [ 0, 0, 0, 95, 44, 47, 124, 183, 128, 198, 126, 114, 116, 12, 168, 188, 186, 208, 26, 252, 170, 101, 87, 65 ]
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: InvalidArgument, error: InvalidId }', examples/playlist_tracks.rs:36:58
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Logging several values revealed that some fields (revision, item_id) were parsed into SpotifyIds, which failed, since SpotifyId expects exactly 16 bytes, while those were of different lengths. Additionally, there were some optional fields (diff, sync_result), which weren't present in this case, and, as such, a default was used. This default included a [], which would then also be parsed as a SpotifyId and failed.

Apart from that, the timestamps included in the response were all in milliseconds and exceeded the accepted range in OffsetDateTime::from_unix_timestamp().

See playlist_proto.txt (or the binary version playlist_bin.txt) for the data for all this occurred with.

---

While the example works now, I'm not sure about some things:

• There are certainly other optional fields than the two that I met during my testing (diff, sync_result). Should those all be wrapped in Options or should the SpotifyId just be changed to not fail on the default case ([])?
• Is there a better way to map optional fields to Option<T>s than the .as_ref().map(TryInto::try_into).transpose()??
• Is the TryFrom<i64> for Date implementation used anywhere else, where the value is not in milliseconds?
• Should there be a wrapper type / type alias for those Vec<u8> fields?

[roderickvd]

Really glad you’re helping out on this. It’s certainly true that the major changes to metadata haven’t been properly tested. Indeed I had not gotten around to testing the playlists and honestly I never use the examples, though I should.

Regarding your questions:

There are a lot of optional fields throughout. Currently these are all set to default values. We had a discussion on it earlier to make it Options. Principally I would favor that but — and this is a big but — how might we do so without a lot and a lot of boilerplate both in the library and on the caller side of things? Mind you, if we do it here then we should do it everywhere in metadata.

I’m not sure on the date representation across the board. It doesn’t seem that Spotify is consistent on it. We need to test it more.

I indeed would like a wrapper type for those IDs.

Keep it coming 👍

[eladyn]

Thank you for the quick answer!

I’m not sure on the date representation across the board. It doesn’t seem that Spotify is consistent on it. We need to test it more.

I tried to get data from Spotify for all structs, where the conversion with TryFrom<i64> for Date is used, and those that I was able to look at do indeed use milliseconds as their timestamps. (PlaylistItemAttributes, Album and Episode)
To avoid future confusion, I went ahead and removed this conversion with try_into() and replaced it with more explicit calls. (Date::from_timestamp_ms())

There are a lot of optional fields throughout. Currently these are all set to default values. We had a discussion on it earlier to make it Options. Principally I would favor that but — and this is a big but — how might we do so without a lot and a lot of boilerplate both in the library and on the caller side of things? Mind you, if we do it here then we should do it everywhere in metadata.

I would personally prefer the Option variant as well, since otherwise a caller might falsely assume that something is initialized with proper data while, in practice, it might not always be. Although I certainly understand the implications of such a change. Maybe it would be better suited for a follow-up PR? In this case, what should I do about the two current changes in that direction (they are necessary to make the example work).

[roderickvd]

Sorry I didn’t get back to you sooner. I have very little time these days. Yet I should encourage contributions like this one or course, exactly the juice we need!

The implications of making everything an option would be quite severe I think. Have you toyed with this idea, how would the code look? Could we do something to help apps lose the boilerplate?

[roderickvd]

Will merge this piece of work -- if you have any more thoughts on the question I last asked then let me know on Gitter or open a new PR. Thanks for this!

[eladyn]

Thank you!

Sorry for not replying to your previous message. I don't have much time at the moment, but be assured that I haven't lost interest in contributing to this project!