incomplete attributes in metadata structs #1054
Replies: 2 comments 5 replies
-
|
This is indeed an issue with no obvious good solution. I think the fact that the same protobuf definitions are used for different responses, containing different data is not great, but I guess that is something that every implementation of the Spotify API will have to deal with. In general I personally believe that forcing a check is better than silently using a default value for missing fields. When a programmer has to check if a value is present, they can decide to use a default value themselve or take another action. In contrast to that, a default value can be indistinguishable from a real value (e.g. default The issue of a program breaking when spotify changes data is not a big issue in my opinion. If not properly prepared, the program will stop to work the way it was intended in any case, but with explicit checks you will know about it or even be able to prevent it. In the other case it will just continue to go on with default values that may or may not work. So with Further explanation of Solution 3The idea of this proposal was to create an
This could look something like this (example for enum LazyArtist {
Minimal {
id: SpotifyId,
name: Option<String>,
},
Full(Box<Artist>),
}Performing a query to the endpoint dedicated to a given type, would always return the This does in my opinion provide a somewhat cleaner Api, but it of course has several disadvantages.
While Solution 3 was my proposal, I am really not sure which of the suggested solutions would be the best. All of them have advantages and disadvantages, which makes a decision very hard. |
Beta Was this translation helpful? Give feedback.
-
Fully agree with you on this!
Thanks for elaborating.
When you take the // used for: artists of an album (`Album::artists`), artists of a track (`Track::artists`), artists of an album of a track (`Track::album::artists`)
struct MinimalArtist {
id: SpotifyId,
name: String,
}
// used for: related artists (`Artist::related`)
struct RelatedArtist {
id: SpotifyId,
name: String,
activity_periods: ActivityPeriods,
portrait_group: Images,
}
// used for: artists (`Artist`)
struct Artist {
// same as the current definition
}So, implementing your solution here would mean: // either
enum LazyArtist {
Minimal {
id: SpotifyId,
name: String,
activity_periods: Option<ActivityPeriods>, // do we need Option here, otherwise it would be an empty `Vec`?
portrait_group: Option<Images>, // same question as above
},
Full(Box<Artist>),
}
// or
enum LazyArtist {
Minimal {
id: SpotifyId,
name: String,
},
Related {
id: SpotifyId,
name: String,
activity_periods: ActivityPeriods,
portrait_group: Images,
},
Full(Box<Artist>),
}
This is the main problem, I see with this proposal: A benefit I can imagine of having the enums in place is, it allows us to do things like the following: let mut track = Track::get(&session, &id).await.unwrap();
track.artist = LazyArtist::Full(Box::new(track.artist.load(&session).await.unwrap()));
// now we have a Track with a fully loaded Artist that we can pass around
// However, when using this, we still need to do:
let artist = match track.artist {
Artist::Full(artist) => artist,
_ => unreachable!(),
};
// which is obviously not that great, since it can lead to panics due to developer oversightsDo you have a better example, where this
Right. :) |
Beta Was this translation helpful? Give feedback.
-
|
Your interpretation of solution 3 is pretty much accurate, except the the
This would be at least a little bit better when using the hypothetical let mut track = Track::get(&session, &id).await.unwrap();
// The id is in both minimal and full, so it can be accessed with a convenicence function without matching (name as well I guess)
let artist_id = track.artist.get_id();
track.artist.load(&session).await.unwrap();
// now we have a Track with a fully loaded Artist that we can pass aroundThe second part would stay the same and still remain a problem. But while writing this, I realized that the load and match could be combined to make it a bit more friendly. let mut track = Track::get(&session, &id).await.unwrap();
// This loads the full artist variant inside the track and returns a Clone or Reference to the inner *full* artist
let full_artist = track.artist.load(&session).await.unwrap();
// The full_artist is the actual full artist, so no matches needed to access stuff that is not in minimal
println!("{:#?}", full_artist.cds);
// The track has also switched to the full artist variant, so this still works
let also_full_artist = match track.artist {
Artist::Full(artist) => artist,
_ => unreachable!(),
};
I guess the My own interpretation could in theory be used with Solution 1 to load all the |
Beta Was this translation helpful? Give feedback.
-
|
Thanks so much for this most excellent write-up and subsequent replies. Just to entertain my earlier thought (without saying I want to steer in that direction per se) could we not provide some macros to make life easy on us (so doing a |
Beta Was this translation helpful? Give feedback.
-
|
You're welcome. :)
In some instances, that should be quite easy. The
The problem here is that
|
Beta Was this translation helpful? Give feedback.
-
In this case what's wrong with default value |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, that would probably work in this case. |
Beta Was this translation helpful? Give feedback.
-
Recently, there have been several discussions about the current structs in the
librespot_metadatacrate. Since I have been involved in some of them myself, I thought I'd just write down the issue and some of my thoughts in a central place, as a starting point for future discussion.As far as I can see, two problems have been popping up with the current implementation:
Problem 1: missing attributes in API responses
Depending on the way a specific metadata struct has been instantiated, the degree to which attributes do actually have sensible values varies widely, and rarely all fields are covered.
One such example is
metadata::album::Albumstruct:A request to
/metadata/4/album/<id>returns quite some info; some attributes are still missing, but many are there:When requesting a track via
/metadata/4/track/<id>however, which contains information about its containing album as well, we receive less information:The issue here is now that both are internally parsed with the same
.protodefinitions (protocol::metadata::Album) and end up in the same higher-level struct from the metadata crate (metadata::album::Album). In the latter, most attributes are filled with default values, as you can see below (this is the album data from the requested track parsed intometadata::album::Album):This causes several problems:
Problem 2: Large struct sizes
As discovered here, e.g.
metadata::track::Trackis almost 1 KB on the stack, without all the possible data that a populatedTrackstruct would need additionally on the heap.Some data:
This is, of course, not generally a bad thing, it only becomes one, when most of these fields aren't actually filled with sensible data and still take up all the space.
Solution Idea 1:
Option<AllTheThings>As proposed by @roderickvd here (and apparently already discussed somewhere else before), all
structattributes that are optional in the protobuf definitions or appear to be sometimes available and sometimes not could be madeOption<T>s.This would solve several pain points with "Problem 1". However, the structs would still require as much space as before and some new problems might arise: Code that knows of the existence of optional fields now has to either
.unwrap()them ormatchthem. The former can lead to surprising breakage, when Spotify changes the data they sent and the latter would introduce quite some boilerplate in library and user code.Solution Idea 2:
structs for everyone and everythingAs the name size, this proposal would mean to add separate high-level (as in: contained in the
metadatacrate) structs for each and every situation that returns different attributes.Something similar does already exist in
ArtistWithRole, which is separate fromArtist. In this case though, the underlying protobuf definition is also different (protocol::metadata::ArtistWithRole).Where it is done exactly like that is the
rspotifycrate, e.g. for different album representations.While this might be the ideal solution from a library user's point of view, it potentially comes with a heavy burden for
librespotdevelopers.Although in contrast to the
.unwrap()in "solution 1", the metadata would not break immediately in most cases, since they are still provided with defaults from the protobufs.Solution Idea 3:
enumsI haven't yet thought about it more, but @dnlmlr also proposed something with regards to this issue. Since I'm not sure I understand it well enough to write down the details here myself, here's the link with his explanation: #1018 (comment)
I hope that this discussion is somehow useful to bundle the various talks into one single place. Looking forward to hearing your thoughts on the matter!
Beta Was this translation helpful? Give feedback.
All reactions