[matthewberry]

Problem

/api/v1/search builds its next and previous links from CLEANSearchQueryParams.model_dump(), so the parameters use the model's field names rather than the query parameter names. Three differ, and the endpoint ignores the model names:

Sent by client	Written in the link
ec_number	clean_ec_number
protein	protein_name
uniprot	uniprot_id

So following next silently drops those filters. From production today:

GET /api/v1/search?ec_number=1.1.1.1&limit=2&offset=4   (total 36682)
next: …/api/v1/search?clean_ec_number=1.1.1.1&format=json&offset=6&limit=2

That next link searches the whole table instead of the 36,682 matches, and it also runs an unfiltered COUNT. I confirmed the dropped filters locally with TestClient and a fake DB rather than following the link on production.

Fix

A new page_links(request, offset, limit, total) helper builds both links as the request's own URL with offset and limit replaced (request.url.include_query_params), so every parameter keeps the name the caller sent, including repeated ones like organism. Unknown parameters are carried through too, deliberately: a next link is the same request, next page.

/typeahead now uses the same helper. Its hand-written mapping from model fields to query names was correct, but it's the pattern that caused this bug, and a filter added to both endpoints later would only have carried through /search links.

Also fixed in the same code, on both endpoints: past the end of the results, previous pointed at another empty page (offset=100000&limit=10 with 36,682 matches gave previous = offset=99990). It now goes to the last page, or to offset=0 when there are no results.

Tests

tests/test_search_pagination_links.py:

• for /search and /typeahead, sends one of every filter (including a repeated organism on /search) and checks that next and previous contain exactly those parameters plus the new offset/limit. Following each link must return 200 and run the same COUNT and data queries, SQL and bound arguments, as the original request, with only LIMIT/OFFSET normalized. The DB mocks are reset before each follow, so a link that never reaches the DB fails.
• with no limit, next carries the default page size (5000)
• past the end, previous goes to the last page on both endpoints, and to offset=0 when there are no results

Mutation checks: with main's link code, 5 of the 6 tests fail (the typeahead round trip passes, since its old mapping was correct); without the clamp, the three past-the-end tests fail; a link that drops ordering fails. Full suite: 31 passed. ruff check app tests reports the same 4 errors as main.

Not changed

The links still start with http://, because the app sees the request behind the TLS-terminating proxy. Production answers http:// with a 301 to https://, so following a link works for GET clients. Emitting https:// directly would need uvicorn's --proxy-headers/--forwarded-allow-ips in the Dockerfile, which is a separate change.

[matthewberry]

I found no correctness bugs in the new link-building code. The main issue is that the round-trip test can pass without exercising the followed link (inline below). I haven't run the suite, and none of these were reproduced.

One comment is outside the diff: /typeahead (app/routers/search.py ~L566) still maps model fields to query names by hand, which is the pattern that caused this bug. A filter added to both endpoints later would carry through /search links automatically but be dropped from /typeahead links. Sharing one request-based helper between the two would close that gap.

[matthewberry]

This can pass without testing the followed link. If the link is rejected before it reaches the DB (a 422 from a renamed parameter, or a 400 from parse_query_params), fetchval isn't called again, so call_args still holds the original request's query and the comparison passes. Suggest calling db.fetchval.reset_mock() before each follow and asserting follow(...).status_code == 200.

[matthewberry]

Confirmed and fixed in a491ab7. I reproduced it: following a link that returns 422 left call_args on the original request, and the comparison passed. follow() now resets both DB mocks first and asserts a 200, so a rejected link fails the test, and so does one that never reaches the DB, since call_args is then None. I checked that follow() raises on a limit=-1 link.

[matthewberry]

This compares only the COUNT query (fetchval). The data query (db.fetch) that returns the page is never compared, so a change to it alone wouldn't fail this test.

[matthewberry]

Confirmed and fixed in a491ab7. This also hid a gap: ordering only changes the data query's ORDER BY, so the round trip never checked it. The comparison now covers the full SQL and bound arguments of both the COUNT (fetchval) and the data query (fetch), with only LIMIT/OFFSET normalized. I checked that dropping ordering from the request changes the data query but not the COUNT.

[matthewberry]

This now copies every parameter the caller sent, not just /search filters. With ?organsim=Homo%20sapiens&_=1729600000&token=abc, all three are carried into next/previous: the misspelled filter stays silently ignored on every page, and anything token-like ends up in the JSON body and in logs. Consider limiting this to the endpoint's known query names (aliases included).

[matthewberry]

Declining this one. Copying every parameter is intentional: a next link means "the same request, next page", and dropping parameters would quietly rewrite what the caller asked for.

On the two risks:

• Tokens in logs: uvicorn's access log already records the full query string of every request; the current production pod has those lines for /search. A token in the URL is logged on the first request whether or not it's echoed. The links go back only to the client that sent the request, and nothing in the app logs them.
• Misspelled filters: organsim= is already ignored on page 1. Carrying it through keeps every page consistent with the first, and leaves the typo visible in the link rather than silently removed.

[matthewberry]

Starlette already does this: str(request.url.include_query_params(offset=next_offset, limit=current_limit)) keeps the multi-valued params and replaces offset and limit. That would remove the base_url split, the list concatenation and the urlencode in both blocks.

[matthewberry]

Done in a491ab7. I checked Starlette 0.46.2's source first: include_query_params re-parses the query with keep_blank_values=True, and MultiDict.update replaces only the given keys, so repeated parameters like organism survive. Both endpoints now use one page_links(request, offset, limit, total) helper built on it.

[matthewberry]

This predates the PR, but it's in this block: when offset is already past total_count, previous is offset - limit, which is another empty page. For example, offset=100000&limit=10 with 36,682 rows sends a client back through about 6,300 empty pages. Clamping to max(0, min(current_offset - current_limit, total_count - current_limit)) would fix it.

[matthewberry]

Confirmed and fixed in a491ab7, using your clamp. Reproduced first: offset=100000&limit=10 with 36,682 matches returned no rows and previous = offset=99990. /typeahead had the same bug in its own copy (max(0, offset - limit)); both now go through the shared helper. Tests cover past the end on both endpoints (→ last page) and past the end of an empty result (→ offset=0), and fail with the old expression.

[matthewberry]

Thanks. Replies are inline; everything except the parameter filtering is in a491ab7.

/typeahead hand mapping: agreed, done. Its mapping was correct today, but it's now replaced by the same page_links helper /search uses, so a filter added to both endpoints later carries through both sets of links. One side effect: /typeahead links now also repeat every parameter the caller sent, consistent with the /search decision in the thread on line 342. test_typeahead_next_and_previous_keep_every_filter sends every context filter and checks the followed links run the same queries.

I mutation-checked the tests: with main's link code 5 of the 6 link tests fail (the typeahead round trip passes, since its old mapping was correct); removing the clamp fails all three past-the-end tests. Full suite: 31 passed.