alex/ephyra
Watch
1
0
Fork
You've already forked ephyra
 0
Code Issues 2 Pull requests Projects Releases Packages 1 Wiki Activity Actions

/Items ignores the by-name filters a client actually sends, so the web client's genre page lists every film #120

New issue
Closed
opened 2026-08-14 00:15:06 +02:00 by Claude · 1 comment
Claude commented 2026-08-14 00:15:06 +02:00
Collaborator
Copy link

#91 fixed the six id-valued filters. The by-name ones beside them are still read off the query and dropped, and one of them has a visible symptom in the pinned web bundle.

The genre page is wrong in a browser. jellyfin-web's genre page sends Genres=, never GenreIds= — so #91's fix does not reach it. Measured on the live pair:

/Items?IncludeItemTypes=Movie&Recursive=true&Genres=Science%20Fiction
  upstream: 2  | Blade Runner, Alien
  ours:     6  | Blade Runner, Alien, Twin Beacons, Night Signal, Red Harbour, Blue Field

A viewer clicking Science Fiction is shown the whole library. This was found by driving the browser during #91's review; no scenario covers Genres, which is why six milestones of replays never reported it.

Also confirmed unbound, same shape, same cause — a field missing from Params in crates/jellyfin-http/src/routes/items.rs, which is #[serde(default)] with no deny_unknown_fields, so the parameter is accepted and dropped in silence:

/Items?Recursive=true&nameLessThan=B     upstream: 10   ours: 22

nameStartsWithOrGreater is the same pair and was not measured.

And one the other way round: /Items filters on seriesId, which upstream's GetItems does not accept at all. The parameter is not in the controller's signature, so upstream ignores it and we narrow on it:

/Items?Recursive=true&seriesId=<Test Pattern>   upstream: 22 (ignores it)   ours: 7

Binding something upstream does not is the same defect as failing to bind something it does: the two servers answer differently for a parameter a client sent.

Where to start

crates/jellyfin-http/src/routes/items.rs — the Params struct and the mapping into ItemQuery. Upstream's binding is Jellyfin.Api/Controllers/ItemsController.cs and the application is Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs.

Do the whole parameter list rather than these three. Compare Params against upstream's controller signature field by field and settle every difference in both directions — bound-but-shouldn't-be as well as unbound. That is the only way this class of bug stops being found one instance at a time, and #91 plus this issue is already twice.

This interacts with the parentless-listing gate #91 added: ItemQuery::narrows() can only see what the route binds, so an unbound filter is also invisible to that decision.

How to know it is done

The three measurements above agree, a harness scenario carries Genres and the by-name filters so a replay holds it, and the browser's genre page lists what upstream's lists.

House rules

Tests run under cargo nextest; upstream's source is the checkout in upstream/
at the pin (v12.0-rc5, 6d501ba4); generated files are regenerated rather
than edited; comments say what the code does, not how it got there. The
reference pair is cargo xtask reference up, a run is
cargo run -p jellyfin-harness -- replay --upstream URL --ours URL --scenario FILE --out DIR,
and crates/jellyfin-harness/README.md says what a diff means. A server started
off current main binds UDP 7359 for discovery — set [discovery] enabled = false
in a scratch config if another one is already running.

#91 fixed the six **id-valued** filters. The by-name ones beside them are still read off the query and dropped, and one of them has a visible symptom in the pinned web bundle. **The genre page is wrong in a browser.** jellyfin-web's genre page sends `Genres=`, never `GenreIds=` — so #91's fix does not reach it. Measured on the live pair: ``` /Items?IncludeItemTypes=Movie&Recursive=true&Genres=Science%20Fiction upstream: 2 | Blade Runner, Alien ours: 6 | Blade Runner, Alien, Twin Beacons, Night Signal, Red Harbour, Blue Field ``` A viewer clicking *Science Fiction* is shown the whole library. This was found by driving the browser during #91's review; no scenario covers `Genres`, which is why six milestones of replays never reported it. Also confirmed unbound, same shape, same cause — a field missing from `Params` in `crates/jellyfin-http/src/routes/items.rs`, which is `#[serde(default)]` with no `deny_unknown_fields`, so the parameter is accepted and dropped in silence: ``` /Items?Recursive=true&nameLessThan=B upstream: 10 ours: 22 ``` `nameStartsWithOrGreater` is the same pair and was not measured. **And one the other way round: `/Items` filters on `seriesId`, which upstream's `GetItems` does not accept at all.** The parameter is not in the controller's signature, so upstream ignores it and we narrow on it: ``` /Items?Recursive=true&seriesId=<Test Pattern> upstream: 22 (ignores it) ours: 7 ``` Binding something upstream does not is the same defect as failing to bind something it does: the two servers answer differently for a parameter a client sent. ## Where to start `crates/jellyfin-http/src/routes/items.rs` — the `Params` struct and the mapping into `ItemQuery`. Upstream's binding is `Jellyfin.Api/Controllers/ItemsController.cs` and the application is `Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs`. **Do the whole parameter list rather than these three.** Compare `Params` against upstream's controller signature field by field and settle every difference in both directions — bound-but-shouldn't-be as well as unbound. That is the only way this class of bug stops being found one instance at a time, and #91 plus this issue is already twice. This interacts with the parentless-listing gate #91 added: `ItemQuery::narrows()` can only see what the route binds, so an unbound filter is also invisible to that decision. ## How to know it is done The three measurements above agree, a harness scenario carries `Genres` and the by-name filters so a replay holds it, and the browser's genre page lists what upstream's lists. ### House rules Tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin** (`v12.0-rc5`, `6d501ba4`); generated files are regenerated rather than edited; comments say what the code does, not how it got there. The reference pair is `cargo xtask reference up`, a run is `cargo run -p jellyfin-harness -- replay --upstream URL --ours URL --scenario FILE --out DIR`, and `crates/jellyfin-harness/README.md` says what a diff means. A server started off current `main` binds UDP 7359 for discovery — set `[discovery] enabled = false` in a scratch config if another one is already running.
Claude commented 2026-08-14 12:25:26 +02:00
Author
Collaborator
Copy link

Done, on main.

request                                             upstream   before   after
/Items?IncludeItemTypes=Movie&Recursive=true
      &Genres=Science Fiction                              2        4        2
/Items?Recursive=true&nameLessThan=B                       9       25        9
/Items?Recursive=true&nameStartsWithOrGreater=S            4       25        4
/Items?Recursive=true&Studios=Brandywine Productions       2        4        2
/Items?Recursive=true&Tags=nothing                         0       25        0
/Items?Recursive=true&seriesId=<Test Pattern>       25 (ignored)     7       25

The genre page was the point: the pinned web client sends Genres=, never GenreIds=, so #91's fix never reached it and a viewer clicking Science Fiction was shown the whole library.

Rather than binding the three named here, the whole parameter list was held against upstream's controller signature in both directions. Genres, Studios, Artists, Tags, OfficialRatings, NameLessThan and NameStartsWithOrGreater are bound; seriesId is gone, because GetItems never took it and we were narrowing on something upstream ignores. NARROWING_UNREAD shrank by seven and stays the honest register of the rest — the four names outside it are justified against InternalItemsQuery.HasFilters, which tests MinParentalRating and never MaxParentalRating.

official_ratings reproduces WhereItemOrDescendantMatches — direct, AncestorIds, then LinkedChildren — and the name range matches ApplyNameFilters, which compares on SortName.ToLower().

The batch this closed in

Seven issues went out together because they looked like one defect class. They turned out to be three roots:

  • #120 and #124 — /Items' Params had been grown one client at a time instead of held against upstream's controller signature, and /UserItems/Resume shared it.
  • #121 and #122 — an ordering key read off the wrong column: sortBy=Name maps to CleanName upstream, not SortName, and a by-name search orders by relevance with no sort-name tiebreak, so ties fall out in id order.
  • #133 — its own, and it forced a real fix in the shared sort table: upstream's PremiereDate coalesces with ProductionYear where ours read the bare column.

A reviewer re-derived the parameter audit mechanically — extracting every [FromQuery] name from GetItems and doing set arithmetic against what we bind — and got nothing bound that upstream does not take and nothing in the register that is not an upstream parameter. This class is closed rather than narrowed, which matters because it had been found three times.

Verified on a restored pair: browsing 34 → 0, items 33 requests at 0, clients 56 → 3, portable 115 → 6. JELLYFIN_RS_REQUIRE_FIXTURES=1 cargo nextest run --workspace: 2228 passed, 0 skipped. Clippy clean. No ignore-list entry was added or touched.

Two things the review caught that the batch had not

Five of the ten new scenario entries could not fail. The default snapshot has no genres, studios or tags, so five items.json entries compared two empty pages — reverting the whole fix would still have replayed clean. They moved into editing.json, after the save that gives one film a genre, a tag, a certificate and a studio and then takes them away, so each selects one row out of nineteen. That is the fifth time this session the same failure has turned up in a different layer, after M5's vacuous scenarios, M8's bundle.json, #111's all-refusals visibility.json and #137's silently-skipping tests.

Reading the reference can write to it. Studios=, Artists= and Albums= resolve through upstream's GetStudio/GetArtist, which call CreateItemByName — so a read files a BaseItems row. That is how a stray Brandywine Productions got into the shared fixture mid-session and made portable.json disagree by 149. Genres, Tags and OfficialRatings narrow through ItemValues and are safe. Both halves are now written into crates/jellyfin-harness/README.md.

Done, on `main`. ``` request upstream before after /Items?IncludeItemTypes=Movie&Recursive=true &Genres=Science Fiction 2 4 2 /Items?Recursive=true&nameLessThan=B 9 25 9 /Items?Recursive=true&nameStartsWithOrGreater=S 4 25 4 /Items?Recursive=true&Studios=Brandywine Productions 2 4 2 /Items?Recursive=true&Tags=nothing 0 25 0 /Items?Recursive=true&seriesId=<Test Pattern> 25 (ignored) 7 25 ``` The genre page was the point: the pinned web client sends `Genres=`, never `GenreIds=`, so #91's fix never reached it and a viewer clicking *Science Fiction* was shown the whole library. Rather than binding the three named here, the whole parameter list was held against upstream's controller signature in both directions. `Genres`, `Studios`, `Artists`, `Tags`, `OfficialRatings`, `NameLessThan` and `NameStartsWithOrGreater` are bound; `seriesId` is **gone**, because `GetItems` never took it and we were narrowing on something upstream ignores. `NARROWING_UNREAD` shrank by seven and stays the honest register of the rest — the four names outside it are justified against `InternalItemsQuery.HasFilters`, which tests `MinParentalRating` and never `MaxParentalRating`. `official_ratings` reproduces `WhereItemOrDescendantMatches` — direct, `AncestorIds`, then `LinkedChildren` — and the name range matches `ApplyNameFilters`, which compares on `SortName.ToLower()`. --- ## The batch this closed in Seven issues went out together because they looked like one defect class. They turned out to be **three roots**: - **#120 and #124** — `/Items`' `Params` had been grown one client at a time instead of held against upstream's controller signature, and `/UserItems/Resume` shared it. - **#121 and #122** — an ordering key read off the wrong column: `sortBy=Name` maps to `CleanName` upstream, not `SortName`, and a by-name search orders by relevance with no sort-name tiebreak, so ties fall out in id order. - **#133** — its own, and it forced a real fix in the shared sort table: upstream's `PremiereDate` coalesces with `ProductionYear` where ours read the bare column. A reviewer re-derived the parameter audit mechanically — extracting every `[FromQuery]` name from `GetItems` and doing set arithmetic against what we bind — and got nothing bound that upstream does not take and nothing in the register that is not an upstream parameter. **This class is closed rather than narrowed**, which matters because it had been found three times. Verified on a restored pair: `browsing` **34 → 0**, `items` 33 requests at 0, `clients` **56 → 3**, `portable` 115 → 6. `JELLYFIN_RS_REQUIRE_FIXTURES=1 cargo nextest run --workspace`: **2228 passed, 0 skipped**. Clippy clean. No ignore-list entry was added or touched. ### Two things the review caught that the batch had not **Five of the ten new scenario entries could not fail.** The `default` snapshot has no genres, studios or tags, so five `items.json` entries compared two empty pages — reverting the whole fix would still have replayed clean. They moved into `editing.json`, after the save that gives one film a genre, a tag, a certificate and a studio and then takes them away, so each selects one row out of nineteen. That is the fifth time this session the same failure has turned up in a different layer, after M5's vacuous scenarios, M8's `bundle.json`, #111's all-refusals `visibility.json` and #137's silently-skipping tests. **Reading the reference can write to it.** `Studios=`, `Artists=` and `Albums=` resolve through upstream's `GetStudio`/`GetArtist`, which call `CreateItemByName` — so a *read* files a `BaseItems` row. That is how a stray `Brandywine Productions` got into the shared fixture mid-session and made `portable.json` disagree by 149. `Genres`, `Tags` and `OfficialRatings` narrow through `ItemValues` and are safe. Both halves are now written into `crates/jellyfin-harness/README.md`.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
No results found.
Labels
Clear labels   
correctness
Diverges from upstream

  
encoding
Transcoding, ffmpeg, and the arg strings

  
http
Route behaviour

  
jobs
Scheduled tasks

  
operations
Installing a server and keeping one running

  
p1
Do this first

  
p2
After the p1s

  
p3
Real, and not urgent

  
performance
Speed or resource use

  
providers
Metadata from somewhere other than the file

  
scanner
The library scan

  
security
Who is allowed to see what

  
testing
Test coverage and harness

No labels
correctness
encoding
http
jobs
operations
p1
p2
p3
performance
providers
scanner
security
testing
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
alex/ephyra#120
No description provided.