garhow
Omitted fields in responses leads to deserialization errors
I originally mentioned this in https://github.com/garhow/steam-rs/pull/53#issuecomment-3043761964 but to have it reported in a more visible place, responses from Steam can have fields completely omitted from the json rather than have a placeholder such as null or an empty string, which will often break serialization due to strictly expecting the field to exist in the response. This happens, for example, when a query returns an empty list or an option toggles whether the response contains one field or another (i.e. the "return short description" option in PublishedFileService's query files API).
Admittedly this is largely a documentation issue on Steam's part (like seriously, I can't find documentation anywhere about their web API responses) but it's still annoying to run into. Of course, due to this lack of documentation, I don't have a complete solution other than "do your best to catch undocumented cases!" but I think clear direction on what your preferred plan of action when library consumers do run into this issue (make the field optional, set a default value, or something else) would be helpful for both you as the maintainer as well as anyone who might be able to contribute.
We feel that a good solution to this is a issue is a bit hard to come by, I personally feel that if we run into a field which may or may not be present in the response we should wrap it in a Option<> because thats explicitly telling the consumer that the field they are wanting to use could be None and they would have to add their own handling for such a case, this in my opinion is a better approach then using #[serde(default)] except if Steam's own documentation (which is hard to comeby) specifies a default value for a field though I am not against its use for certain assumptions that aren't present in Steam's docs (which I would leave up to the maintainer's personal decision to make theses assumptions in good faith) . For the maintainer I will admit that it is tedious as making the assumption that a field is present only for it to not be present is annoying (and time consuming).
The only way for us to catch theses are in the tests (which is not a bullet proof solution). Please feel free to shoot me down though as we are open to solutions/discussion. Many Thanks
I agree with you that making these fields Options is a better solution, in #53 I used serde(default) since it was more convenient at the time, but with this in mind I'll be closing/editing #53 so that it only introduces the string (de)serialization for SteamID and make a separate set of changes for missing fields specifically.
One question, if the omitted field is an empty list (vec) would it be preferable to set that as an Option or to use the default value of an empty vec? This was actually the main reason i used #[serde(default)] looking back on it and imo it's more expected to at least retrieve an empty list from a response rather than an optional list that contains at least one value.
Yeah, I would see a empy Vec or VecDeque would be a good use for #[serde(default)] as both a Option and a empty Vec are equivalent to the same thing in my eyes & could be seen as better consumer friendly (unwraps a Option<Vec<String>> to find it empty)