refactor(roms): give the ROM filters a single home, and match them against the facets mirror

[sdornan]

Description

Three commits, in order: collapse the duplicated filter definitions into a registry, carry the filter vocabulary as one model, then repoint every filter at the narrow roms_facets mirror. Plus two merges of master, and four follow-up commits from the review passes (both covered further down).

1. The registry

The ROM filter vocabulary was written out in five places: the /api/roms query parameters, filter_roms's 63-line signature, the kwargs.get block in get_roms_scalar, the criteria translation in get_smart_collection_criteria, and the is_unscoped test that decides what may be cached. Ten near-identical _filter_by_* methods differed only in the column they matched on.

Two of those copies are latent bugs rather than just duplication:

• needs_metadata_join was a hand-kept list of the filters that read roms_metadata. A filter added to filters_to_apply but forgotten there still runs, but under group_by_meta_id it cross-joins and the dedup window ranks the whole library instead of the matches.
• is_unscoped gates the gallery's sidecar caches, which are keyed on user/order/grouping but not on the filters. A filter missing from that list would let one user's narrowed gallery be stored under, and served from, the shared "all" key.

ROM_FILTER_SPECS is now one declarative row per filter — name, how values are matched, which column — and the join and dispatch are derived from it.

2. The model

RomFilterParams is the vocabulary in one place. The route takes it as a dependency, filter_roms reads its fields, smart collections rebuild it from stored criteria (legacy selected_* keys and the platform_id scalar included). has_filters() reads the model's own fields, so a filter added later counts towards the cache gate whether or not anyone remembers it, and a test asserts that for all 27 narrowing fields.

statuses deliberately stays outside the registry: it matches against RomUser, is applied after the grouping window, and carries the default "hide hidden roms" behaviour.

3. The facets mirror

Every filter matched against either roms or the roms_metadata view over it. Both carry the raw provider-metadata blobs inline, so evaluating a facet predicate reads the whole wide table, and no index can serve a JSON containment test.

roms_facets already holds exactly these values in a narrow, trigger-maintained mirror (0100, extended by 0103 and 0112) — the dropdown queries and the stats coverage breakdown already read it for this reason. The filters now read it too.

On a seeded 40k-game library (139 MB of roms against 8 MB of roms_facets, stock 128 MB buffer pool), measured through filter_roms:

query	before	after
total count	374 ms	70 ms
rom id index	615 ms	74 ms
page (limit 50)	1.5 ms	1.3 ms

Both sidecars run on every gallery request, since with_total and with_rom_id_index default on. The page query was already fast — it stops early on an indexed sort, so it never evaluated the predicate over the whole library. That is also why this went unnoticed: the visible list is quick, while the count and the virtual-scroll index behind it are not.

The mirror covers the provider id columns too (METADATA_SOURCE_FACET_COLUMNS, already used by stats), so metadata_providers moves with the rest rather than being the one filter left reading roms.

Matching is unchanged, verified three ways on the seeded library:

• every mirrored column is byte-equal to its source across all 40,000 rows (<=>), and no rom is missing its facets row
• each filter selects the same ids as before, under any / all / none
• the trigger tracks an UPDATE to a rom's metadata, so the mirror does not go stale

A test asserts every registry column belongs to RomFacets, so a filter cannot quietly be repointed back at the wide table.

Why as_query_dependency and not Annotated[RomFilterParams, Query()]

FastAPI expands a Pydantic query model into individual parameters only when the model is a route's sole query parameter — _get_flat_fields_from_params bails out at len(fields) == 1. /api/roms has a dozen others, so the native form leaves a single required parameter named filters. That is not only a documentation problem: the generated TypeScript client is built from that schema, and a request carrying the actual fields is rejected with 422 filters: Field required.

as_query_dependency builds a dependency whose signature carries the model's fields, each with its own constraints; a dependency's parameters are flattened individually, so they survive. Because the failure is silent, an endpoint test asserts /api/roms exposes genres/matched/platform_ids and no filters parameter — verified to fail if the route reverts to the native form.

The API contract

/api/roms still takes the same 58 query parameters, with identical names, types, descriptions, defaults and constraints. Verified against a schema captured before any change, and re-verified after both merges and all four review commits:

• same parameter set, zero content differences on any parameter
• no other path or component schema changed, except GET /api/roms/download, which deliberately gains minimum: 1 on collection_id and smart_collection_id (see review finding 2 below)
• the generated frontend client covers response models only, not query parameters, so src/__generated__ is byte-identical (273 files, no diff) and npm run generate is a no-op

The only other difference is the order of parameters within the OpenAPI array: FastAPI lists a dependency's parameters after the route's own. OpenAPI treats parameters as unordered and clients key by name — which the identical generated client demonstrates.

The *_logic parameters stay plain str rather than becoming an enum: today an unrecognised value falls through to "any" behaviour, and rejecting it would be a real contract change for third-party clients. Worth doing separately.

A note on the model's field style

Fields are written name: Annotated[T, Field(description=...)] = default rather than name: T = Field(default, description=...). Without the pydantic.mypy plugin — which this repo does not enable — mypy reads Field(None, ...) as a value rather than a default and reports every RomFilterParams(...) call as missing 40-odd required arguments. The Annotated form gives mypy a real default and needs no config change. Enabling the plugin repo-wide is the alternative (it needs pydantic added to Trunk's mypy environment via packages:) and is worth considering on its own merits, but it is a repo-wide change that does not belong here.

Verification

• Generated SQL is byte-identical to the pre-refactor code across 18 cases covering every filter, all three logic operators, grouping and per-user scoping (commits 1-2; commit 3 changes the table a predicate reads, and is covered by the equivalence checks above).
• Full backend suite after both merges: 4562 passed. The 28 failures and 2 errors are identical, test for test, to what master alone produces in the same environment (OAuth, scan sockets, invite-link URLs, a refresh-token case and a root-permissions filesystem case).
• isort / black / ruff clean on every file this PR touches; mypy reports no new errors against a baseline of the same five modules.

The benchmark above is synthetic: generated uniform rows on MariaDB 10.11 in a container, reporting the best of five runs. Real libraries carry several provider blobs per row rather than the one seeded here, so the ratio should if anything be conservative — but treat the shape of the win as the claim, not the absolute milliseconds. Happy to add the seed/benchmark scripts under backend/tools/ if reviewers want to reproduce it against a real library.

Merges with master

Two merge commits, both in areas that overlap this PR, so they are worth a reviewer's attention:

• fix(roms): invalidate a user's gallery sidecars on rom_user writes #4463 (sidecar invalidation) hoisted the per-call sidecar cache key into one sidecar_cache_key and added a separate build_unscoped_filter_values_cache_key. This PR's per-site key building is dropped in favour of theirs; the hoisted key now reads filters.group_by_meta_id. Their new test_rom_sidecar_cache.py passes against this PR's cache gate.
• fix(roms): sort grouped galleries by the best sibling's key #4465 (grouped galleries sort by the group aggregate) added sort_key / order_by / order_dir to filter_roms and a new grouped-sort ordering path. Those parameters are kept alongside filters, and the grouped-sort block's search_term now comes off the model. Their new sort tests were updated to construct RomFilterParams, since this PR changes filter_roms's calling convention.

Still open (not in this PR)

1. Make the facet columns indexable now that they live in one narrow table: GIN indexes on PostgreSQL, or a normalised roms_facet_values (rom_id, kind, value) child table for MariaDB/MySQL, which have no JSON index. roms_facets carries only idx_roms_facets_platform_id today, so the remaining ~70 ms is still a scan.
2. The frontend still spells the filter vocabulary out in galleryFilter.ts, useGalleryFilterUrl and smartCollectionCriteria.ts. Deriving one FILTER_KEYS from the generated types would make the compiler catch a backend-added filter the URL serializer missed — which this PR makes cheaper to cause, since adding a filter is now one registry row.
3. Validate order_by — done upstream by fix(roms): sort grouped galleries by the best sibling's key #4465, which landed while this PR was open: _mapped_sort_column now requires a ColumnProperty, so a relationship name falls back to the name sort instead of raising. All that is left of the original idea is advertising the valid sort keys in the OpenAPI schema, which is a documentation nicety rather than a fix.
4. Validating filter_criteria at the write boundary, and get_random_rom / download_roms hand-repeating constraints that live on RomFilterParams — both discussed under review finding 1 below.

Review passes (four follow-up commits)

The repo's pr-ready gauntlet was run over the whole range. The security audit came back clean: no new egress hosts, no dependency, lockfile, CI or workflow changes at all, no binaries or encoded blobs, and every protected_route / Scope. / hidden_platform_ids / hidden_rom_ids occurrence in the diff is a context line, not an added or removed one. hidden_* are keyword-only Python parameters bound from get_permissions(request); they are not fields of RomFilterParams, and the model ignores extras, so no client-supplied key can reach them.

Two findings are worth a reviewer's eye:

1. Unusable stored criteria now make a smart collection match nothing. filter_criteria is stored as unvalidated JSON, so a row can hold {"collection_id": 0}, {"hltb_main_story_min": -1} or {"platform_ids": [3, "not-an-id"]}. Validating them raised, and because refresh_smart_collections_for_roms loops every smart collection with no per-item guard, one bad row aborted the refresh of every collection after it on any ROM write and 500'd anyone opening a public smart collection with it. Dropping the offending entry instead is not safe either: dropping a filter drops the constraint with it, so platform_ids: [3, "not-an-id"] would widen from one platform to the whole library. from_stored_criteria now answers None, and its one caller turns that into a query matching nothing. The refresh loop still cannot be stopped by one bad row, and a collection never shows more than its criteria claim. Note the consequence: refresh_smart_collection writes that empty result into rom_count / rom_ids / path_covers_*, so a corrupt row visibly empties its collection, with a log.warning naming the field. Validating filter_criteria on write (endpoints/collections.py) is the deeper fix and would give the user a signal at the point of error; it is not in this PR because it does not retire the read path (rows written by earlier versions are already stored) and because the stored blob's vocabulary is deliberately wider than the model's (get_smart_collection_members reads order_by/order_dir straight off the dict), so it would have to be validate-only, never normalize-and-store.

2. /api/roms/download gains ge=1 on two parameters, which is a small deliberate contract change. It declared collection_id and smart_collection_id unbounded while get_roms_scalar validates them through RomFilterParams, which constrains both, so ?collection_id=0 raised ValidationError inside the handler and returned 500. Both now carry the same ge=1 that /api/roms and /api/roms/random already declare, so the same request returns 422 from FastAPI instead. This is the only OpenAPI difference in the PR: minimum: 1 on those two parameters of GET /api/roms/download, verified by diffing the schema. The generated frontend client covers response models only, not query parameters, so src/__generated__ is still byte-identical and npm run generate remains a no-op. Worth noting for a follow-up: ge=1 is now typed out in three places for the same field (the model, get_random_rom, download_roms). as_query_dependency already builds exactly the annotation needed, so exposing a single-field version of it would remove all four restatements.

The remaining passes inlined a now-single-use validator, replaced a hand-built log message with pydantic's own error, folded a redundant test into an existing parametrize list, moved the new membership test to test_smart_collections.py (which owns that behaviour and already had the fixture), extracted a narrowing helper for eight repeated assertions, and trimmed every comment block over two lines.

Also fixed along the way: as_query_dependency silently mis-built a route for a field carrying an alias or a default_factory (one mutable default built at decoration time and shared across requests). Neither shape exists on RomFilterParams; both now raise at startup, and the helper has the tests it shipped without. And two comments claimed metadata_providers matches id columns on Rom when it matches them on roms_facets (METADATA_SOURCE_FACET_COLUMNS), which invites a reader to skip its join and cross-join the mirror; corrected, and pinned by a test asserting every registered filter joins the mirror exactly once.

Re-verified after all four: 790 backend tests green across the affected handler, router and endpoint suites; frontend typecheck clean, 1236 tests green, build clean; isort / black / ruff clean and mypy reporting no new errors against the same-file baseline. One local-tooling note: trunk itself cannot run in this environment (its plugin bundle download returns HTTP 403 through the sandbox proxy), so the tools it wraps were run directly at the versions in .trunk/trunk.yaml and with the config files in .trunk/configs/ — running them on their own defaults is what let an isort ordering slip past into a red trunk_check earlier in this PR. CI's trunk_check is the authority.

Checklist

• I've tested the changes locally
• I've updated relevant comments
• I've assigned reviewers for this PR
• I've added unit tests that cover the changes

AI assistance disclosure

Per CONTRIBUTING.md: this PR was written with AI assistance (Claude Code), to a large extent. The analysis, design, implementation, the benchmark harness and the verification harnesses (OpenAPI schema diff, generated-TypeScript diff, SQL compilation diff, column equivalence, mypy baseline diff) were produced in an agent session and reviewed against the checks reported above. Please read the diff as machine-authored work: those checks constrain it tightly, but they are not a substitute for human review. The two merge resolutions above especially, since they touch code that landed in parallel.

Note on the branch name

The branch is named claude/graphql-api-migration-vu20z2 because the session began by evaluating whether RomM should move to GraphQL. The conclusion was no — the API is largely binary/transfer-shaped, types are already generated from OpenAPI, and the third-party client ecosystem (Playnite, Argosy, grout, the iOS and desktop clients) makes a second API surface expensive — and this filter work is what came out of that discussion instead. The name no longer describes the contents.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MLreYqGLgHJLei6NThVXCz

[greptile-apps]

Greptile Summary

This PR centralizes ROM filtering in a shared RomFilterParams model and declarative registry, updates gallery and smart-collection callers to use it, and moves multi-value predicates onto the narrow trigger-maintained roms_facets mirror.

• Derives filter dispatch, facet joins, scope detection, and cache gating from shared definitions.
• Preserves individual /api/roms query parameters through a generated FastAPI dependency.
• Adds regression coverage for query-schema exposure, stored criteria, cache classification, grouped joins, and facet-backed filtering.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete correctness, security, or repository-rule failures identified.

The refactored callers consistently use the shared filter model, known legacy criteria remain normalized, request parsing and constraints are covered, and the facet mirror is backfilled and maintained by database triggers.

Important Files Changed

Filename	Overview
backend/handler/database/rom_filters.py	Introduces the shared filter model, registry, cache-scope predicates, and legacy smart-collection normalization.
backend/handler/database/roms_handler.py	Refactors filter application around the shared model and directs registered predicates to RomFacets.
backend/endpoints/roms/init.py	Replaces duplicated route parameters and cache-gating checks with the shared filter dependency and model helpers.
backend/handler/database/collections_handler.py	Rebuilds smart-collection filters through RomFilterParams.from_stored_criteria.
backend/utils/router.py	Adds a reusable dependency-signature builder that exposes Pydantic fields as individual FastAPI query parameters.

_{Reviews (1): Last reviewed commit: "perf(roms): match the gallery filters ag..." | Re-trigger Greptile}

[Copilot]

🟢 Approval recommended

No blocking issues remain; the outstanding documentation update is only a minor nit.

Pull request overview

Refactors ROM filtering around a shared model and registry, using the narrow roms_facets mirror while preserving the API contract.

Changes:

• Centralizes filter definitions and smart-collection criteria.
• Routes filtering and cache checks through shared models.
• Adds facet-join, grouping, API, and regression coverage.

File summaries

File	Reviewed change
backend/utils/router.py	Flattens model fields into query parameters.
backend/tests/handler/database/test_roms_verified_filter.py	Updates verified-filter coverage.
backend/tests/handler/database/test_roms_missing_filter.py	Updates missing-filter coverage.
backend/tests/handler/database/test_roms_group_by_index.py	Updates grouped-query coverage.
backend/tests/handler/database/test_roms_facet_filter_join.py	Tests facet joins during grouping.
backend/tests/handler/database/test_rom_filter_params.py	Tests filter modeling and registry behavior.
backend/tests/handler/database/test_collections_handler.py	Tests stored criteria conversion.
backend/tests/endpoints/roms/test_rom.py	Verifies individual OpenAPI query parameters.
backend/handler/database/roms_handler.py	Applies registry filters against roms_facets; one documentation nit remains.
backend/handler/database/rom_filters.py	Defines shared filter parameters and registry logic.
backend/handler/database/collections_handler.py	Uses shared filters for smart collections.
backend/endpoints/roms/__init__.py	Integrates modeled filters and cache gating.

Review details

Suppressed comments (1)

backend/handler/database/roms_handler.py:1153

• These mappings now read provider IDs from RomFacets, but the docstring above still says they are matched against columns on Rom. Please update the documentation so it preserves the facet-mirror invariant for future changes.

            METADATA_SOURCE_FACET_COLUMNS[value]
            for value in values
            if value in METADATA_SOURCE_FACET_COLUMNS

• Files reviewed: 12/12 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.