How to deal with the innards of the Plex API?

[Mossop]

I've been thinking a little on two different but related issues that I see with this crate as it stands. I would be interested if you have thoughts @andrey-yantsen or if this is even a problem you care about before I consider tinkering with any potential solutions.

The first problem is that the Plex REST API is sort of painful. We have fields that in some API calls are returned as numbers but returned as strings in others (aside: I'm pretty sure this is because the API used to be XML only which has no notion of numbers as such and whatever they've done to support JSON isn't entirely correct or complete). We have objects that seem like they are the same type when returned from different API calls but one call may return a bunch of additional fields compared to another. So far this is resolved by liberal usage of Option for fields and guessing at a fields expected type and converting accordingly.

The second problem is that right now there isn't a much of a public API that we expose to abstract away from the innards of Plex, other than a couple of things we just give direct access to the deserialized structs. That's fine, particularly right now it gives a lot of flexibility for playing around. But when combined with the above issue what it leaves us with is an exposed API that can be awkward to use (liberal need to check or unwrap Option) and needing to break API almost every time a new API call is implemented (the patch I'm working on right now is forced to convert yet more fields to Option).

One thing that could be done to reduce the usage of Option would be to deserialize to different structs for different API calls, even different structs for different media types. In this way only the fields known to be possible for a given "thing" are present. That could be a vast explosion in the number of structs though I think there are tricks we could do with macros and such to re-use groups of fields across them.

Though a totally reasonable thing to say to this is that this is all as it should be, this is a low-level crate with the intent of doing minimal wrapping to make Plex Rust accessible and safe. Other crates can be built on top of this to make a nice high-level abstraction as folks desire. Better to expose all the guts so they have everything they could need.

Have you had any thoughts along these lines?

[andrey-yantsen]

I haven't been thinking about this, to be honest. Initially, the project was born under the idea of "having at least something", so all the architectural decisions weren't very mature and well-thought for 3rd-party usage.

You're raising great points. I think for now, it's ok to stick with returning the raw objects, as well as adding/removing Option when needed. Still, it seems like it's time to start adding more comments to the fields — i.e. we need to understand when a field can be None or in which cases the API returns a string instead of an integer. After this, we might want to start adding an abstraction layer for the public, user-friendly APIs.

[Mossop]

That makes sense. I may try to do some work to add some of that documentation.

[Mossop]

Raising this again since as you can see from #567 I hit a few more field errors. They're both fields that I don't care about but because my Plex server presented them in a different format to that which we expect deserialization fails for the entire struct and so made certain items in my library unusable. I think it would be better for this crate to fail softly in the case of most fields failing to deserialize. What do you think to making it so fields that are wrapped in an Option deserialize to None if they don't deserialize correctly for the expected type? We could make it strict in tests but permissive otherwise. Another idea would be to use some new enum that either has the expected field value or an additional state with the raw serde_json::Value for inspection by consumers, but I'm not sure there is a lot to be gained there. I think we should also always allow deserializing numbers from strings etc.

I'd be happy to work on implementing this if you think it makes sense.

[andrey-yantsen]

Thanks, Dave! #569 looks promising. I like how one can do field.result()? and get the error promotion without too much trouble :)

I have some concerns about the naming, though. MetadataField implies that those fields are supposed to be used within Metadata, while we have a lot of other places where this magic could be helpful.

Also, I'm not sure about the FromValue trait. It looks like an unnecessary complication at first glance. I'm talking about the slightly unexpected int-to-string and string-to-int casting. And that in its current state, we'll have to implement it for all the possible structs (while they already will have Deserialize implemented, which should be enough for the end goal we're trying to achieve here). I'll look better at the PR during the weekends, though.

[Mossop]

On the naming, that was just something random we can call it whatever we like. I was anticipating only using this for primitive types (integer, float, string, bool) while structs would remain as they are. The FromValue trait is just one way we can take care of the cases where we want to deserialize numbers from strings etc. but there are others if you prefer. Anyway no rush, let me know what you think of it in general then I can do more on it later.

[andrey-yantsen]

Ok, let's start from the beginning.

Was the issue you encountered due to Plex suddenly changing the return type, or was it because the library currently "enforces" adding all the possible fields to structs? If it's the latter, I'm inclined to ditch this approach and keep it only for the enums. This way, the new fields will be added (in theory) only after proper investigation.

I have controversial feelings towards having semi-deserialized structs. If the library doesn't know what exactly hides inside, why the end user will?

[Mossop]

Was the issue you encountered due to Plex suddenly changing the return type, or was it because the library currently "enforces" adding all the possible fields to structs?

In the case of skip_count the issue appears to be that most API calls return it as a number but the metadata object embedded in the response to creating a transcode serializes it as a string, so this is a case where what looks like the same data is returned from different API endpoints but is serialised differently. This seems to be common.

The skip_parent case I've lost track of unfortunately. The field was added by you in the first place as a plain bool so I expect it was definitely being returned as such in some case and I know I had to fix it as it was breaking me but it was a few weeks ago and I completely failed to take notes on why. But I would guess that it is a similar situation.

In both cases my project doesn't need either of those fields so just not including them in the struct would have also solved my problems, but maybe another project would so it seems valuable to attempt to deserialize them. My chief concern is that with the breadth of media formats I don't know how we could ever be sure that we've got all the field definitions "right" and at the moment getting one wrong breaks deserialization of the entire struct for all dependents regardless of whether they care for that field or not.

So I think we should be more permissive about primitive fields (numeric, boolean and string) and other than cases that we can be pretty sure about (we can be pretty sure that rating_key exists on Metadata f.e.) do our best to deserialize (always attempt to convert numbers, bools and strings into what we think the field is) and when that fails at least let the rest of the struct deserialize. It would serve my purposes to just deserialize broken fields as None but #569 is an attempt to do a bit better and retain information about the error which could allow dependents to understand and maybe act on the problem better. It also has the benefit that anything else would involve a lot more attributes on all the fields, but up to you whether you think the added complexity is worth it.

I have controversial feelings towards having semi-deserialized structs. If the library doesn't know what exactly hides inside, why the end user will?

I would certainly prefer to have perfectly typed structs. Honestly I'd like to get rid of most of the Option's too but I think we already don't know enough about what the data types should look like to do that and there would be so many slightly different struct definitions that it would be infeasible. Certainly if we know we've got a field's type wrong we should correct it. I'd just also prefer not to be broken when I happen across some new minor difference and then have to spend time figuring out a patch and getting it landed. You're as responsive as I could hope for thankfully but it's still all time and particularly when it is in a field I have no interest in in the first place it's annoying to have to take that time.

[andrey-yantsen]

Thank you for such a detailed response, Dave!

I agree that #569 is good enough for what you're trying to achieve. So let's start with this approach.