Skip to content

Conversation

lostb1t
Copy link
Contributor

@lostb1t lostb1t commented Sep 30, 2025

So this is an edge case. But i have a plugin that uses an external search engine. And ids might not
match, breaking swiftfin search for me

I know this is an edge case but i don’t think removing the guard is in anyway harmful.

And if there's a mismatch i would say it would be intentional like my case

@JPKribs JPKribs added the external Problem or feature is dependent on an external implementation label Sep 30, 2025
@LePips
Copy link
Member

LePips commented Sep 30, 2025

I understand the use case but it does raise a flag that we would ever get a different id here. Is there anything about the BaseItemDTo.locationType on these items where we can also allow returning the item?

@lostb1t
Copy link
Contributor Author

lostb1t commented Oct 1, 2025

Not really, they can be both local and remote.

The plugin matches on providerids. But thats not bullet proof.

looking at the code i don't really understand what issue the guard tries to solve. It already request the item by ID. At this point if an id does not match you can safely assume its intentional

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a sanity check, as they never really hurt.

I do take issue with this operation not being invariant over the item id. Identification is used throughout the app to ensure proper item presentation and updating - regardless of the use case. I can never assume something like this is "intentional", the plugin is arguably breaking basic guarantees clients can make about server data. While there may be no harm in this function's immediate context, I can see issues elsewhere throughout the app I can't prevent.

I'm fine to relax this check, but I did add a comment for when I inevitably forget why I did so.

@lostb1t
Copy link
Contributor Author

lostb1t commented Oct 1, 2025

yeah sounds fair.

thanks!

@LePips LePips merged commit b4bb248 into jellyfin:main Oct 1, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Problem or feature is dependent on an external implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants