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

Add get/update item metadata methods #42

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

Erotemic
Copy link
Contributor

Related to #40 I spent way to much time figuring out how to use the API to update item metadata. Relevant discussion is on #jellyfin-dev:matrix.org on 2024-Jan-02. (If there is a way to link to a matrix conversation, let me know).

There is a jellyfin architecture quirk that makes this implementation less than ideal. It turns out that when you call the UpdateItem endpoint, any data you don't provide is overwritten with null, so we are stuck with one of two bad options:

  1. Force the user to provide all item metadata fields, even if they don't want to update them.
  2. Perform a query before we post the update to construct a well-formed request behind the scenes.

For ease of use, I opted for the second option, but raised another issue: there isn't a an easy API call to get metadata for a single item. There is a get_item, but it doesn't return all of the fields required by the update method. The get_items call does do this, but you have to call it with a list of 1 and then unpack it, which is kinda awkward. I made this problem worse by adding a 3rd method get_item_metadata as a stopgap, but I don't want to merge it and add to API bloat.

Fortunately, this problem can likely be solved by updating the get_item call to pass "Fields": info() in its params dictionary, but I wanted to discuss it first before I did it. Similarly, I'd also like to discuss the name update_item_metadata, which I chose just to let me move forward and modify my library, but it's name doesn't agree with the rest of the API.

My current thoughts are to take the following actions:

Remove get_item_metadata and change get_item to either:

    def get_item(self, item_id):
        return self.users("/Items/%s" % item_id, params={
            'Fields': info()
        })

Or let the user pass in Fields if necessary:

    def get_item(self, item_id, params=None):
        return self.users("/Items/%s" % item_id, params=params)

Then rename update_item_metadata to update_item and keep the same signature.

But there are other directions I could go, so I'd like to discuss before I move further. In the meantime this stop-gap API works well enough for me.

@iwalton3
Copy link
Member

Fortunately, this problem can likely be solved by updating the get_item call to pass "Fields": info() in its params dictionary, but I wanted to discuss it first before I did it.

I think this is reasonable if it doesn't break any existing usages of that function.

@Erotemic
Copy link
Contributor Author

I made the change. The only way it would break someone is if they wrote code that fails if extra information is returned. The new returned object should always be a superset of what is currently returned on the master branch.

@iwalton3 iwalton3 merged commit 787a1ae into jellyfin:master Feb 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants