Skip to content

Fix: always paginate GET /users; /search 500 on empty acronym filter#216

Merged
alexskr merged 3 commits intodevelopfrom
fix/slow-users-endpoint
May 1, 2026
Merged

Fix: always paginate GET /users; /search 500 on empty acronym filter#216
alexskr merged 3 commits intodevelopfrom
fix/slow-users-endpoint

Conversation

@mdorf
Copy link
Copy Markdown
Member

@mdorf mdorf commented Apr 28, 2026

Summary

  • Force pagination on GET /users by removing the if page? guard in helpers/users_helper.rb. The endpoint now always returns a paged response ({page, pageCount, totalCount, prevPage, nextPage, links, collection}), never a flat array.
  • Add a deterministic default sort (username asc) so clients walking nextPage links get stable page boundaries.
  • Bump ontologies_linked_data to develop tip (cdbf8f6), which includes Fix /users perf with security: idempotent admin?, drop inverse attrs from auth load ontologies_linked_data#286 — idempotent User#admin? and inverse-attribute exclusion from the auth-middleware load (the dominant per-user cost in the timeout).
  • Also fixes /search and /property_search returning 500 when the acronym filter resolves to empty (e.g. ontology_types filter rejects all candidates, or the requesting user has no access to any of the requested ontologies). Pre-existing regression introduced by commit 388d1bd0 ("converted acronyms filter from Boolean to Term syntax"). Bundled here because both are production-critical and shipping in the same release window.

Why /users is slow

Production users were unable to create ontologies, with Faraday::TimeoutError coming back from bioportal_web_ui controllers that called LinkedData::Client::Models::User.all. Response time of /users?include=all grew linearly with user count: every serialized user re-invoked Thread.current[:remote_user]&.admin?, which re-loaded :role via Goo on each call (N+1 over a single user), compounded by the absence of pagination. With ~5,300 users in production, this exceeded Faraday's 60s read timeout on every authenticated request.

Why /search and /property_search are broken

Commit 388d1bd0 replaced get_quoted_field_query_param with get_terms_field_query_param at the start of filter_query construction. The new term-syntax function correctly early-returns "" for an empty acronyms list (an empty _query_:"{!terms f=…}" would itself be malformed), but the rest of filter_query construction implicitly relied on the prior function's vacuous submissionAcronym:"" placeholder always producing a non-empty starting clause.

When acronyms filtered to empty, filter_query was "" and the subsequent << \" AND <clause>\" appends produced a stray-AND fq that Solr rejected with 400 (surfaced as 500 by the API). Same bug at two call sites: helpers/search_helper.rb:160 and helpers/properties_search_helper.rb:48. A third call site (search_helper.rb:181, valueset_root_ids) is unaffected — already guarded by unless valueset_root_ids.empty?.

Fix: substitute Solr's match-all literal *:* when filter_query is empty after the acronyms step. AND'ing further clauses onto *:* narrows correctly. Side benefit: this is also more correct than the pre-388d1bd0 behavior, which silently returned 0 hits when acronyms was empty due to the vacuously-restrictive submissionAcronym:"" placeholder.

Cross-repo rollout

The full /users fix spans four repos and must roll out in a specific order:

  1. Fix /users perf with security: idempotent admin?, drop inverse attrs from auth load ontologies_linked_data#286 (merged)
  2. This PR — ontologies_api
  3. ncbo/ontologies_api_ruby_client (companion PR — auto-flatten paged responses, slim User attrs)
  4. ncbo/bioportal_web_ui — bump Gemfile.lock to the new client release (no controller code changes needed)

Breaking change

GET /users now always returns a paged Hash, never a flat array. Direct HTTP consumers (anything bypassing the ontologies_api_ruby_client gem) need to read response['collection'] instead of treating the response as an array. The forthcoming client release transparently walks pages and returns a flat array to existing in-process callers, so consumers using the gem are unaffected.

Local benchmarks (21,339 users on dev triplestore, after #286 + this PR)

Scenario Before After
Raw /users?include=all (no params, paginates to 50) timeout 2.0s
Raw pagesize=5000 slim include 155.7s 2.1s
Raw pagesize=5000&include=all (6.7 MB) ~9 min 13.8s
Models::User.all walk via client (5 pages × 5000) timeout 44.8s

Per-page cost dropped from ~30ms × N (linear) to roughly flat at ~2s for slim include.

Test plan

  • bundle exec rake test passes locally
  • test_all_users asserts paged response shape (page, totalCount, collection)
  • test_all_users_pagination asserts ?pagesize=2 returns page 1 of 2 with nextPage=2
  • test_all_users_include_all_is_paged asserts ?include=all&pagesize=2 paginates and each item carries the requested attributes (e.g. created)
  • test_search_with_empty_acronym_filter_returns_ok asserts /search?q=anything&ontology_types=NONEXISTENT_TYPE returns 200 with empty collection (regression for the *:* fix)
  • test_property_search_with_empty_acronym_filter_returns_ok asserts the equivalent for /property_search
  • Smoke test against staging: curl '/users?include=all' returns paged JSON; curl '/users?pagesize=50&include=all' returns first page in a few seconds; curl '/search?q=Conceptual%20Entity&ontologies=STY&require_exact_match=true' returns 200 (was 500)
  • Verify bioportal_web_ui (with the matching client gem) renders /admin/users, the New Ontology form's user dropdown, and /users admin page correctly after deploy

mdorf added 3 commits April 28, 2026 14:58
Drops the `if page?` guard so /users unconditionally returns a Page
object. Adds a deterministic default sort (username asc) so paged
responses are stable for clients walking nextPage links.

Addresses production timeouts on /users?include=all. Pairs with
ncbo/ontologies_linked_data#286 (idempotent User#admin?, drop inverse
attrs from auth load) and the corresponding ontologies_api_ruby_client
auto-paginate change. Tests updated for the paged response shape and
cover ?pagesize and ?include=all combinations.
Commit 388d1bd ("converted acronyms filter from Boolean to Term syntax")
replaced get_quoted_field_query_param at the start of filter_query
construction in BOTH search_helper.rb (line 160) and
properties_search_helper.rb (line 48). The new term-syntax function
correctly early-returns "" for an empty acronyms list (an empty
terms-query would itself be malformed), but the rest of filter_query
construction in both helpers implicitly relied on the prior function's
vacuous `submissionAcronym:""` placeholder always producing a non-empty
starting clause.

When acronyms filtered to empty (e.g. ontology_types restriction matches
nothing, or the requesting user has no access to any of the requested
ontologies), filter_query was "" and the subsequent `<< " AND <clause>"`
appends produced a stray-AND fq that Solr rejected with 400 (surfaced
as 500 by the API), breaking both /search and /property_search for any
call whose acronyms resolved empty.

Fix: when filter_query is empty after the acronyms step, use Solr's
match-all literal `*:*`. AND'ing further clauses onto `*:*` narrows
correctly, producing well-formed queries semantically equivalent to the
constraints that follow. Also more correct than the pre-388d1bd
behavior, which always silently added `submissionAcronym:""` (matching
zero docs) when acronyms was empty — search-without-acronyms now
actually returns matches.

The third in-tree call site (search_helper.rb:181 — valueset_root_ids)
is unaffected: it's already guarded by `unless valueset_root_ids.empty?`
on the line above, so the function input is guaranteed non-empty.

Adds regression tests for both endpoints exercising the empty-acronyms
path via ontology_types=NONEXISTENT.
@mdorf mdorf changed the title Fix: always paginate GET /users; default order_by username Fix /users timeouts and /search 500 on empty acronym filter Apr 30, 2026
@mdorf mdorf changed the title Fix /users timeouts and /search 500 on empty acronym filter Fix: always paginate GET /users; /search 500 on empty acronym filter Apr 30, 2026
@alexskr alexskr merged commit 8828e8c into develop May 1, 2026
10 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