An IncludeItemTypes value that does not match is dropped rather than narrowing, and matching is case-sensitive #181
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found on a person's page in the throwaway instance in
TESTCONTAINER/, whichdraws an Audiobooks section holding two music tracks. The library has no
audiobooks.
What it looks like
The pinned web client asks the person page's audiobook section for
—
Audiobook, with a smallb, which is how jellyfin-web spells it. The enummember is
AudioBook. We do not match it, and rather than narrowing to nothingwe drop the filter and answer with everything the rest of the query allows.
What answers
Same request, one person, only the type name changed:
IncludeItemTypesAudiobookAudioand aMusicAlbumAudioBookmovieAudioand aMusicAlbumNotAThingAudioand aMusicAlbumAudioand aMusicAlbumSo the matching is case-sensitive, and an unmatched value is silently the same
as asking for no narrowing at all.
Upstream
ASP.NET binds a
BaseItemKindthroughEnumConverter, which parsescase-insensitively —
Audiobook,audiobookandAudioBookare one value —and a value that is no member of the enum fails model binding, which on a
[ApiController]is a400, not a wider answer.Where to start
Wherever
IncludeItemTypesis parsed into item types. Two changes: fold casewhen matching, and decide what an unmatched value is — upstream's answer is a
400, and quietly widening the query is the one answer it cannot be. The samequestion applies to
ExcludeItemTypes, where dropping an unmatched valueexcludes nothing.
How to know it is done
IncludeItemTypes=AudiobookandIncludeItemTypes=audiobookanswer exactlywhat
AudioBookanswers, an unmatched value is refused rather than widening,and the person page's Audiobooks section is empty for a library with no
audiobooks.
bundle.jsonis the scenario written by the client rather than byus, so this is the kind of request it should be carrying.
House rules
Tests run under
cargo nextest; upstream's source is the checkout inupstream/at the pin. Generated artifacts are regenerated, never edited.Fixed in
1995867, with a review's comment notes indb0bad8.Half of what this issue claimed was wrong, and measuring it is what showed
that. Against the reference instance at the pin:
So the case-insensitivity is real and is fixed. The refusal is not:
upstream's
CommaDelimitedCollectionModelBindercatches the conversion failureper element and keeps the ones that converted, so a value naming no variant is
answered as if it were not there — which is what this server already did. The
400this issue asked for would have been the deviation.ExcludeItemTypesbinds through the same binder and is now case-insensitivewith it.
One thing a review turned up, not filed because no client sends it: upstream's
binder also accepts the underlying integer, so
IncludeItemTypes=13names avariant there and names none here.