[1/9] Media-api enhancement: searchAfter cursor navigation endpoint - #4849
Open
paperboyo wants to merge 5 commits into
Open
[1/9] Media-api enhancement: searchAfter cursor navigation endpoint#4849paperboyo wants to merge 5 commits into
paperboyo wants to merge 5 commits into
Conversation
New POST /images/search-after endpoint for cursor-based (search_after)
pagination.
--- What's here ---
sorts.scala
reverseSorts (flip asc/desc on a sort seq), jsonToSort (deserialise flat and
nested-object ES sort clause entries from JSON), orderOf/sortModeOf helpers.
dateAddedToCollectionAscending added alongside the existing Descending — see
"N-1 note" below.
ElasticSearchModel.scala
SearchAfterParams, SearchAfterRawResults case classes. SearchParamsBody with
fromJson (parses POST body into SearchParams: query, date range, filters,
hasRightsAcquired, syndicationStatus, orderBy, countAll, page size/offset).
payType not parsed (disabled in Kahuna).
hasRightsAcquired also added to SearchParams + SearchParams.apply(request) so
it is available as a GET query param too.
ElasticSearch.scala
searchAfter() method: buildFilterOpt reuse, null-zone strip/remap on
seekToEnd cursors, cursor-length validation, PIT branch (bypasses prepareSearch
migration dedup filter — correctness requirement), plain branch. _source
projection schema-derived at startup (reflection on Image fields minus
{embedding, originalMetadata, fileMetadata} plus fieldAliasConfigs paths;
cuts payload ~2.1 MB → ~370 KB/page). resolveSearchAfterHit strips drop-fields
from a copy of _source before validate[Image] while keeping the full source for
alias extraction (avoids JsError when field aliases touch fileMetadata leaves).
QueryBuilder.scala
dateAddedToCollection pathHierarchy filter widened to also match
"-dateAddedToCollection" (ascending token).
MediaApi.scala
hitToImageEntity lifted to private method. searchAfterImages action:
auth.async(parse.json), parses body via SearchParamsBody.fromJson, calls
searchAfter, serialises response via typed SearchAfterResponse case class +
OWrites.
conf/routes
POST /images/search-after inserted before GET /images/:id.
--- Two decisions for team consideration ---
1. POST for a read endpoint — cursor + sort clause + filters are too large for
a query string; POST with application/json is the pragmatic choice. Play's
CSRF filter does not check application/json by default.
2. auth.async(parse.json) — first use of this body-parser combinator pattern
in media-api. Worth aligning if the team has a preferred alternative.
--- N-1 note: dateAddedToCollectionAscending ---
The "-dateAddedToCollection" token previously fell through to a fieldSort on an
unmapped field with no unmappedType, causing an ES error. dateAddedToCollectionAscending
(ASC, unmappedType "date") fixes this for direct API callers. Note: Kahuna's
getOrder() in media-api.js transforms unrecognised orderBy tokens to -uploadTime
before the request reaches this code, so this change has no effect on the Kahuna
UI — it benefits direct API calls only. The filter half (QueryBuilder widening)
was needed regardless; the sort half makes the server-side contract coherent
rather than leaving a half-state (filter fires, sort silently ignored).
16 new integration tests: forward/reverse cursor pagination, null-zone round-trip, seekToEnd+null-zone crash guard, cursor-mismatch → 422, dateAddedToCollection filter both orders (cursor path), dateAddedToCollection sort both orders (Kahuna search() path), fieldAliases projection, silent fieldAlias (isPotentiallyGraphic via fileMetadata.xmp.pur:adultContentWarning). graphic-image-1 fixture added to ElasticSearchTestBase.
paperboyo
added a commit
that referenced
this pull request
Aug 1, 2026
…so that they track with main
Contributor
There was a problem hiding this comment.
Pull request overview
Adds cursor-based Elasticsearch pagination to media-api for the Kupua frontend.
Changes:
- Adds authenticated
POST /images/search-after. - Implements cursor, reverse, PIT, null-zone, filtering, and source projection support.
- Adds integration coverage and collection-sort improvements.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
media-api/conf/routes |
Registers the search-after endpoint. |
media-api/app/controllers/MediaApi.scala |
Parses requests and builds responses. |
media-api/app/lib/elasticsearch/ElasticSearch.scala |
Implements cursor-based searching. |
media-api/app/lib/elasticsearch/ElasticSearchModel.scala |
Adds request and response models. |
media-api/app/lib/elasticsearch/QueryBuilder.scala |
Adds rights and collection filtering. |
media-api/app/lib/elasticsearch/sorts.scala |
Adds sort parsing and reversal. |
media-api/test/lib/elasticsearch/ElasticSearchTest.scala |
Adds pagination integration tests. |
media-api/test/lib/elasticsearch/ElasticSearchTestBase.scala |
Adds graphic-image test data. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+671
to
+672
| val projectionIncludes: Seq[String] = | ||
| imageSourceFields.filterNot(searchAfterDropFields) ++ config.fieldAliasConfigs.map(_.elasticsearchPath) |
Contributor
Author
There was a problem hiding this comment.
Kupua drops full-fat fileMetadata on the floor for perf. We pass on bits from it via fieldAliases (including “silent” ones, like xmp.pur:adultContentWarning to satiate Potentially Graphic without resorting to painful script. But whether we should allow passing it through when requested… I don’t know, so leaving open if only for info.
🤖 narrative:
Correct, and it's a deliberate trade-off rather than an oversight — but you're right that returning empty data is the wrong way to express it. The lean _source projection (the Image schema minus embedding/originalMetadata/fileMetadata, plus the fieldAliasConfigs leaf paths) is what takes a page from ~1.7 MB to ~370 KB, and the alias passthrough is the intended escape hatch for the few fileMetadata leaves the client actually needs. Nothing requests include=fileMetadata on this endpoint today.
Two options: reject include=fileMetadata outright, or honour it by making the drop-set conditional on include. If we do the latter it must stay dropped by default and only be fetched when explicitly asked for, so the payload win is never silently lost. Leaving open for a view on which.
Comment on lines
+131
to
+132
| offset = (body \ "offset").asOpt[Int].getOrElse(0), | ||
| length = (body \ "length").asOpt[Int].getOrElse(10), |
Contributor
Author
There was a problem hiding this comment.
Pretty sure it’s of no consequence, but left unresolved in case this non-consequence better be backed by some code (that’s not yet added).
🤖 narrative:
Valid, and I'd like a steer rather than deciding unilaterally. Cursor pagination is the alternative to offset pagination, not a companion to it — the client never sends offset, and there's no .from() anywhere in searchAfter. So my instinct is to reject a non-zero offset with a 422 rather than implement it, which makes the contract honest without adding a mode that shouldn't be used. The alternatives are applying .from() only when no cursor is present, or documenting that it's ignored. Leaving open for a view.
First batch of fixes from the Copilot review on PR #4849. Five mechanical corrections on the cursor path only — no behaviour change for any existing caller. Kahuna's createSort/parseSortBy and imageSearch are untouched. Validation errors were 500s, not 422s. searchAfter threw InvalidUriParams synchronously, before any Future existed, so the controller's .recover never saw it. The body moves to a private searchAfterQuery; the public searchAfter converts the throw to a failed Future. That also lets the odd `return Future.failed(...)` cursor-length guard become a plain throw, so all four validation sites now behave identically. The cursor-length case was the one that already worked, which is why the existing "cursor-mismatch → 422" test passed and masked the rest. jsonToSort trusted its input. An empty sort entry threw NoSuchElementException; a missing or non-string `order` threw JsResultException; extra fields were silently discarded. Now shape-checked, with every malformed shape surfacing as InvalidUriParams. orderOf treated anything other than "desc" as ascending, so a typo such as `decs` produced valid-looking but wrongly-ordered results. Now strictly asc/desc. An empty sort clause was accepted. A cursor endpoint without a deterministic sort returns an unusable continuation cursor, so it is now rejected. The check lives in the ES layer rather than the controller because media-api has no controller tests — all test files are lib-level — and the controller's existing .recover already maps InvalidUriParams to 422. The syndication review-queue runtime mapping was missing. search() attaches syndicationReviewQueueFixMapping when querying the review queue with syndication.review.useRuntimeFieldsFix enabled; searchAfter built the same filter but never declared the field. Elasticsearch does not error on an unmapped field in a term query — it matches nothing — so the must_not clause silently evaporated and images with an active deny-syndication lease could reappear in the review queue, differing from Kahuna's answer for the same query with no error and no log line. Root cause is leases.leases being a plain (non-nested) object array: ES flattens access and endDate into unpaired lists, and the Painless runtime field walks leases one at a time to restore the pairing. Tests were written red-first, which turned the reviewer's claims into evidence: the pre-fix run produced exactly the predicted NoSuchElementException and JsResultException, the silently-accepted "decs", and a stack trace of jsonToSort throwing straight out of searchAfter past the controller's recover. New SortsTest.scala covers jsonToSort and orderOf directly (9 cases, no Docker). ElasticSearchTest gains four cases and a third ES instance with the runtime-fields flag enabled. TZ=UTC sbt "media-api/test" → 213/213 green; no existing test needed changing. The syndication test is knowingly weak: it passes with and without the fix. The runtime field only diverges from the flat clause on multi-lease documents, and every fixture has a single lease. Making it non-vacuous would need a multi-lease fixture in the shared ElasticSearchTestBase.images, which would shift hardcoded counts across several existing syndication tests — disproportionate for a six-line parity fix whose equivalence to search() is plain by inspection. Still outstanding from the same review: the PIT _shard_doc tiebreaker, the nested-aware null-zone exists query, and three API-contract questions for the team.
Second batch from the Copilot review on PR #4849. One real bug fixed, one claim investigated and refuted. Null-zone exists was not nested-aware. When the primary sort field lives inside a nested type, the null-zone filter built `must_not exists(field)` as a root-level query. A root-level exists cannot match a parent document for a field inside a nested mapping, so the must_not excluded nothing and images that do have the field leaked into the null zone — returned a second time across a full scroll. This is reachable from a real client sort: usagesDateAdded sorts on usages.dateAdded with nested path "usages", and usages is a NestedField in Mappings.usagesMapping. The primary FieldSort already carries its nested path, so the fix reads it and wraps the exists in a nestedQuery, keeping the flat form for non-nested fields. A failing-first test showed eight usage-bearing fixtures leaking before the fix and none after. The PIT _shard_doc tiebreaker is dropped deliberately; no change. The reviewer's premise holds under measurement — Elasticsearch 8.18 does append an implicit _shard_doc to every hit's sort array under a point-in-time snapshot, giving three values against a two-field client clause. The predicted consequence does not: ES does not reject a shorter search_after, it accepts it and compares the client's prefix, so a two-page PIT walk already worked. Preserving the tiebreaker was implemented, went green, and was reverted. Cursors outlive the PIT: clients persist them and retry without a PIT once one expires, and a _shard_doc value in a non-PIT search_after is rejected by ES with a 400. It would also reject every client-synthesised cursor — seek anchors and null-zone estimates are built client-side and cannot contain a shard ordinal the client has no way to know. The truncation is therefore correct, conditional on the caller's sort clause ending in a unique tiebreaker; a deliberately non-unique sort was measured losing eight of twenty-seven documents. That precondition is now stated in a comment, and the server cannot detect field uniqueness on the caller's behalf. Three integration tests added. The two PIT tests pin both halves of the cursor contract: one asserts ES returns n+1 sort values while the cursor we hand back is n, so a future ES change surfaces immediately; the other walks the whole corpus a page at a time under a PIT and asserts nothing is lost or repeated. TZ=UTC sbt "media-api/test" → 216/216 green.
paperboyo
added a commit
that referenced
this pull request
Aug 17, 2026
First batch of fixes from the Copilot review on PR #4849. Five mechanical corrections on the cursor path only — no behaviour change for any existing caller. Kahuna's createSort/parseSortBy and imageSearch are untouched. Validation errors were 500s, not 422s. searchAfter threw InvalidUriParams synchronously, before any Future existed, so the controller's .recover never saw it. The body moves to a private searchAfterQuery; the public searchAfter converts the throw to a failed Future. That also lets the odd `return Future.failed(...)` cursor-length guard become a plain throw, so all four validation sites now behave identically. The cursor-length case was the one that already worked, which is why the existing "cursor-mismatch → 422" test passed and masked the rest. jsonToSort trusted its input. An empty sort entry threw NoSuchElementException; a missing or non-string `order` threw JsResultException; extra fields were silently discarded. Now shape-checked, with every malformed shape surfacing as InvalidUriParams. orderOf treated anything other than "desc" as ascending, so a typo such as `decs` produced valid-looking but wrongly-ordered results. Now strictly asc/desc. An empty sort clause was accepted. A cursor endpoint without a deterministic sort returns an unusable continuation cursor, so it is now rejected. The check lives in the ES layer rather than the controller because media-api has no controller tests — all test files are lib-level — and the controller's existing .recover already maps InvalidUriParams to 422. The syndication review-queue runtime mapping was missing. search() attaches syndicationReviewQueueFixMapping when querying the review queue with syndication.review.useRuntimeFieldsFix enabled; searchAfter built the same filter but never declared the field. Elasticsearch does not error on an unmapped field in a term query — it matches nothing — so the must_not clause silently evaporated and images with an active deny-syndication lease could reappear in the review queue, differing from Kahuna's answer for the same query with no error and no log line. Root cause is leases.leases being a plain (non-nested) object array: ES flattens access and endDate into unpaired lists, and the Painless runtime field walks leases one at a time to restore the pairing. Tests were written red-first, which turned the reviewer's claims into evidence: the pre-fix run produced exactly the predicted NoSuchElementException and JsResultException, the silently-accepted "decs", and a stack trace of jsonToSort throwing straight out of searchAfter past the controller's recover. New SortsTest.scala covers jsonToSort and orderOf directly (9 cases, no Docker). ElasticSearchTest gains four cases and a third ES instance with the runtime-fields flag enabled. TZ=UTC sbt "media-api/test" → 213/213 green; no existing test needed changing. The syndication test is knowingly weak: it passes with and without the fix. The runtime field only diverges from the flat clause on multi-lease documents, and every fixture has a single lease. Making it non-vacuous would need a multi-lease fixture in the shared ElasticSearchTestBase.images, which would shift hardcoded counts across several existing syndication tests — disproportionate for a six-line parity fix whose equivalence to search() is plain by inspection. Still outstanding from the same review: the PIT _shard_doc tiebreaker, the nested-aware null-zone exists query, and three API-contract questions for the team.
paperboyo
added a commit
that referenced
this pull request
Aug 17, 2026
New phase-3-d3-searchafter-post-pr-review.md records the triage of the ten Copilot review comments on PR #4849: per-comment verdicts with file cites, the three-batch split, the open decisions, and the procedure for porting fixes to the PR branch and replying to each comment. Notable verdicts, all verified against source rather than taken on the reviewer's word: comment 6 (usageStatus/syndicationStatus returning 500) is byte-identical to the existing GET path and therefore not a regression; comment 3 (nested null-zone exists) is real and kupua's own direct-ES adapter already handles it correctly; comment 1 (PIT _shard_doc) is unverifiable today because the PIT branch has no caller and every test passes pitId=None, so it will be settled by experiment rather than argument. Changelog gains the Batch A entry. Index updated.
paperboyo
added a commit
that referenced
this pull request
Aug 17, 2026
Second batch from the Copilot review on PR #4849. One real bug fixed, one claim investigated and refuted. Null-zone exists was not nested-aware. When the primary sort field lives inside a nested type, the null-zone filter built `must_not exists(field)` as a root-level query. A root-level exists cannot match a parent document for a field inside a nested mapping, so the must_not excluded nothing and images that do have the field leaked into the null zone — returned a second time across a full scroll. This is reachable from a real client sort: usagesDateAdded sorts on usages.dateAdded with nested path "usages", and usages is a NestedField in Mappings.usagesMapping. The primary FieldSort already carries its nested path, so the fix reads it and wraps the exists in a nestedQuery, keeping the flat form for non-nested fields. A failing-first test showed eight usage-bearing fixtures leaking before the fix and none after. The PIT _shard_doc tiebreaker is dropped deliberately; no change. The reviewer's premise holds under measurement — Elasticsearch 8.18 does append an implicit _shard_doc to every hit's sort array under a point-in-time snapshot, giving three values against a two-field client clause. The predicted consequence does not: ES does not reject a shorter search_after, it accepts it and compares the client's prefix, so a two-page PIT walk already worked. Preserving the tiebreaker was implemented, went green, and was reverted. Cursors outlive the PIT: clients persist them and retry without a PIT once one expires, and a _shard_doc value in a non-PIT search_after is rejected by ES with a 400. It would also reject every client-synthesised cursor — seek anchors and null-zone estimates are built client-side and cannot contain a shard ordinal the client has no way to know. The truncation is therefore correct, conditional on the caller's sort clause ending in a unique tiebreaker; a deliberately non-unique sort was measured losing eight of twenty-seven documents. That precondition is now stated in a comment, and the server cannot detect field uniqueness on the caller's behalf. Three integration tests added. The two PIT tests pin both halves of the cursor contract: one asserts ES returns n+1 sort values while the cursor we hand back is n, so a future ES change surfaces immediately; the other walks the whole corpus a page at a time under a PIT and asserts nothing is lost or repeated. TZ=UTC sbt "media-api/test" → 216/216 green.
paperboyo
added a commit
that referenced
this pull request
Aug 17, 2026
…and add wire-contract tests Three unrelated problems, all found while auditing kupua's own tests after the media-api review work on PR #4849 finished. None were caused by that work — its four commits contain no TypeScript at all — but the audit is what surfaced them. The unit suite was green for the wrong reason, and only on some machines. vite.config.ts declares no `environment` under `test:`, so vitest runs in its default node environment and nothing supplies DOM globals. Two test files call sessionStorage directly, so whether they pass depends entirely on the Node version: Node 25 exposes sessionStorage as a stable global, while Node 22 — the minimum this project's `engines` field allows — does not. Same commit, 1045/1045 green on v25.8.1 and 13 failures on v22.12.0. Fixed with `// @vitest-environment jsdom` docblocks on history-snapshot and image-offset-cache. jsdom was already installed and declared in devDependencies, just never wired up. selection-store needed nothing; it already stubs storage via vi.stubGlobal and documents that it runs without a DOM. Both files also gain a deterministic environment guard asserting `typeof window` is "object". This was verified load-bearing by temporarily removing a docblock: the guard failed while the other twelve tests carried on passing on Node 25. That is the point — a misconfiguration that previously failed only for developers on older Node now fails identically everywhere, including for whoever introduces it. `npm run build` was broken and no test could have caught it. tsc failed on CqlSearchInput: @guardian/cql's TextSuggestionOption.label is `string | undefined`, meaning the key must be present but may hold undefined, whereas this project's TypeaheadSuggestion.label is optional and may be absent entirely. Adapted at the boundary in buildDynamicFieldFallback with a LabelledSuggestion type, rather than making label required across the codebase and rippling into every producer. Worth remembering that vitest strips types via esbuild, so a green unit suite says nothing about whether the project compiles. Wire-contract tests added for the 422s the media-api work introduced. POST /images/search-after now rejects a sort clause that is empty, names more than one field per entry, or uses a direction other than asc/desc — and nothing on this side pinned any of it. The direct-ES path tolerates all three, so a future change to buildSortClause could have kept every existing test green while breaking only --use-media-api. The sort-builders tests now derive their token list from SORTABLE_FIELDS, so a newly sortable field is covered automatically, and assert all three invariants for both buildSortClause and reverseSortClause. The adapter test asserts the request body always carries a non-empty sort, including when orderBy is absent. Also archives the post-PR-review doc now that all ten review comments are actioned, and clears the session worklog. Unit 1131/1131, build passes, e2e 244/244.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Co-authored by Claude. Description written by me, with Claude, hopefully useful, narrative below.
What does this change?
This is the first of nine planned additions to media-api to support experimental frontend which currently talks to ES directly from me laptop. The intention is for those to not have any effect on production app. Changes go beyond additive only where it seems absurd to not make small amendments and only after verifying no ill effects for real app. These are always noted and up for debate (as is all the rest of it, of course).
How should a reviewer test this change?
Mercilessly. This includes tests, which together with existing tests, pass cleanly. To verify on working prototype, run local Grid with
--use-TESTand ALSO run Kupua with--use-media-api(it passes prototype’s searchAfter requests via this modified, local, media-api).How can success be measured?
It’s still up for debate. We may never measure it, but it might be silly not to try to. Unless, it would be silly to try. Also: “Try not. Do, or do not. There is no try”.
Who should look at this?
@twrichards (and anyone and everyone else who feels like)
Tested? Documented?
Here is 🤖 narrative (and gazillions of “docs” here):
Why
Kupua (the in-development Grid frontend prototype) needs cursor-based pagination
(search-after) to let users scroll through millions of images without the 100,000-hit ES
offset wall. Currently kupua hits ES directly. This PR adds the server-side half so that
traffic can eventually be routed through media-api instead.
This is the first of ~9 planned media-api extensions to support kupua (PIT snapshots,
multi-get, and others). The conventions established here — POST+JSON for cursor endpoints,
shared
hitToImageEntity/SearchParamsBodybuilding blocks — will apply to the rest, soit's worth settling any disagreement now rather than per-PR later.
This is a net-new route with no existing callers — it carries zero traffic today and has
zero blast radius on current behaviour until kupua is switched to use it.
What
New route:
POST /images/search-aftersorts.scala—reverseSorts,jsonToSort(flat + nested-object sort clause deserialisation),orderOf/sortModeOfhelpers.ElasticSearchModel.scala—SearchAfterParams,SearchAfterRawResults,SearchParamsBody(parses the POST body: query, date range, label/uploader/category/collections/has/is filters,
hasRightsAcquired,syndicationStatus,orderBy,countAll, page size/offset).payTypeisalways
None— not sent by kupua (disabled in its UI; cost filtering is a plain free/non-freeboolean there).
ElasticSearch.scala—searchAfter(): reusesbuildFilterOpt, applies null-zonestrip/remap on seek-to-end cursors, validates cursor length, fans into a PIT branch (bypasses
prepareSearchmigration dedup filter) or a plain branch._sourceprojection isschema-derived at startup (reflection on
Imagefields minus{embedding, originalMetadata, fileMetadata}plusfieldAliasConfigspaths) — cuts payload from ~1.7 MB to ~370 KB per page.resolveSearchAfterHitstrips the drop-set from a copy of_sourcebeforevalidate[Image](avoids
JsErrorwhen field aliases touchfileMetadataleaves) while keeping the full sourcefor alias extraction.
QueryBuilder.scala— two additions:dateAddedToCollectionfilter widens to also match"-dateAddedToCollection"(the ascending token kupua sends; see the Kahuna note below). Also anew
hasRightsAcquiredFilter(syndicationRights.rights.acquiredterm query) — closes a paritygap where kupua's direct-ES path already applied this filter but it was silently dropped when
routed via media-api.
MediaApi.scala—hitToImageEntitylifted to private method.searchAfterImagesactionenriches each hit via the lifted
hitToImageEntity(→imageResponse.create), with a typedSearchAfterResponsecase class +OWrites. (A lean one-pass writer,createForBrowse, wasprototyped and measured but reverted — it is not in this PR. See the Performance note below.)
conf/routes—POST /images/search-afterbeforeGET /images/:id.[EDIT: amended post-Copilot review]
ElasticSearchTest.scala/ElasticSearchTestBase.scala— 23 integration tests, and a newSortsTest.scala— 9 unit tests (no Docker) for the sort-clause deserialiser:forward/reverse cursor pagination, null-zone round-trip, seekToEnd+null-zone,
cursor-mismatch→ 422, dateAddedToCollection filter both orders (cursor path),
dateAddedToCollection sort both orders (Kahuna
search()path), fieldAliases projection,isPotentiallyGraphic via fieldAlias.
One small, intentional improvement to the media-api sort contract
This PR makes the
-dateAddedToCollection(ascending) sort token work when callingGET /imagesdirectly. It previously didn't: the token fell through to afieldSortonan unmapped field with no
unmappedType, so ES errored / no-op'd. Two coupled pieces:QueryBuilder.scala— thedateAddedToCollectionpathHierarchy filter now also fires forthe negated token (kupua needs this; its ascending collection sort is meaningless without
the collection filter).
sorts.scala+ElasticSearch.scala— addeddateAddedToCollectionAscending(ASC,unmappedType("date")) and the matchingsearch()case, so the sort actually appliesinstead of erroring.
Note on Kahuna: Kahuna's
getOrder()function inmedia-api.jstransforms anyunrecognised
orderBytoken to-uploadTimebefore the request reaches media-api. So-dateAddedToCollectionis stripped by the JS layer and never arrives at this code pathfrom the Kahuna UI (even via manual URL editing). This change benefits direct API
callers (curl, REST clients, future integrations). We left the server-side contract
more correct; the Kahuna JS is a separate concern.
Performance note
Each hit is enriched via the lifted
hitToImageEntity→imageResponse.create— the samepath Kahuna's
GET /imagesuses. On a fast production ES link the dominant per-page cost isthis Argo envelope build (~55 ms/page, measured):
imageResponse.createruns a ~12-stepJsObject.transformchain and presigns S3 URLs per hit. A lean one-pass writer(
createForBrowse) was prototyped and measured (~42% envelope reduction) but reverted —it is not in this PR. It is the main remaining server-side optimisation and is worth building
before this endpoint carries production browse traffic.
The ~3× slowness versus direct-ES that you may see in local dev is a separate, dev-only
artefact: the elastic4s client → ES leg is uncompressed (~5× the bytes over the same SSH
tunnel), which does not apply on the production same-VPC link. Independent confirmation: #4784
(gzip compression on the ES REST client, approved, not yet merged) measured the same shape —
a 2.4–3.2× win over the SSH tunnel, but no regression (±14 ms, within noise) on TEST's intra-VPC
link. Once #4784 merges, this local-dev artefact goes away for every endpoint, not just this one.
Two decisions for team consideration
1.
POSTfor a read endpoint. The cursor + sort clause + filter set is too large for a querystring.
POSTwithapplication/jsonbody is the pragmatic choice. Play's CSRF filter does notcheck
application/jsonby default, so no CSRF config changes were needed. Worth agreeing thisas the convention for future cursor/filter-heavy endpoints.
2.
auth.async(parse.json). The action usesauth.asyncwith theparse.jsonbody parsercombinator — first use of this pattern in media-api. If the team prefers a different shape for
authenticated JSON endpoints, this is the place to align.
Neither requires a code change here — just noting for review.