diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c760745..e563e9f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,23 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed +- Pagination no longer breaks for items without a `datetime` (e.g. when only + `start_datetime`/`end_datetime` are set). The default sort now assigns missing + datetimes a concrete value instead of the Long sentinel that could not be + reused as a `search_after` value, and the `next` token is base64url-encoded so + sort values containing commas or nulls survive the round-trip (a plain + comma-join corrupted them). + ([608](https://github.com/stac-utils/stac-server/issues/608), + [1082](https://github.com/stac-utils/stac-server/issues/1082)) +- Pagination no longer skips items when sorting by a `sortby` field that some + items lack. A custom sort replaced the default sort's unique tiebreaker, so + items sharing a sort value could not be disambiguated across pages; custom + sorts now append the `id` tiebreaker the default sort already used. + ([608](https://github.com/stac-utils/stac-server/issues/608), + [1082](https://github.com/stac-utils/stac-server/issues/1082)) +- GET `/search` and items pagination no longer drop the response body fields on + the second page: an empty `fields` object is no longer serialized into the + `next` link as `fields={}` (which was re-parsed as a field named `{}`). - Link hrefs no longer start with `undefined://` when neither `STAC_API_URL` nor a `stac-endpoint`/`X-Forwarded-*` header is set (e.g. behind API Gateway, which does not set `X-Forwarded-Proto`/`-Host` by default). The endpoint now falls back to the diff --git a/src/lib/api.ts b/src/lib/api.ts index f0b532f8..88f10f51 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -347,8 +347,17 @@ const buildPaginationLinks = function ( method: httpMethod, type: 'application/geo+json' } + // Drop empty values, including empty objects/arrays — a bare `fields: {}` + // would otherwise serialize into the GET link as `fields={}` and be + // re-parsed as a field named "{}", stripping the response _source (#1082). + const isPresent = (value: unknown): boolean => { + if (value === undefined || value === null || value === '') return false + if (typeof value === 'object') return Object.keys(value as object).length > 0 + return true + } const nextParams = pickBy( - assign(parameters, { bbox, intersects, limit, next: lastItemSort, collections, filter }) + assign(parameters, { bbox, intersects, limit, next: lastItemSort, collections, filter }), + isPresent ) if (httpMethod === 'GET') { const nextQueryParameters = dictToURI(nextParams) diff --git a/src/lib/database.ts b/src/lib/database.ts index 54123afc..2469b9ea 100644 --- a/src/lib/database.ts +++ b/src/lib/database.ts @@ -601,7 +601,10 @@ function buildIdQuery(id: string): OpenSearchBody { } const DEFAULT_SORTING: SortParameters = [ - { 'properties.datetime': { order: 'desc' } }, + // `missing: 0` (epoch) gives items without a `datetime` a concrete sort value + // instead of OpenSearch's Long sentinel, which loses precision in JS and can't + // be reused as a `search_after` value — breaking pagination (#608 / #1082). + { 'properties.datetime': { order: 'desc', missing: 0 } }, { id: { order: 'desc' } }, { collection: { order: 'desc' } } ] @@ -609,24 +612,46 @@ const DEFAULT_SORTING: SortParameters = [ function buildSort(parameters: QueryParameters): SortParameters { const { sortby } = parameters if (sortby && sortby.length) { - return sortby.map((sortRule) => { + const rules: SortParameters = sortby.map((sortRule) => { const { field, direction } = sortRule - return { - [field]: { - order: direction - } - } + return { [field]: { order: direction } } }) + // `search_after` pagination needs the sort to end in a unique key, or items + // that share the user's sort value get skipped or duplicated across pages + // (worst with fields that are absent on some items — they all collapse to + // the same sentinel value). Append the `id` tiebreaker the default sort + // already guarantees, unless the caller is already sorting by it. (#608 / + // #1082) + if (!rules.some((rule) => 'id' in rule)) { + rules.push({ id: { order: 'desc' } }) + } + return rules } return DEFAULT_SORTING } -function buildSearchAfter(parameters: QueryParameters): string[] | undefined { +function buildSearchAfter( + parameters: QueryParameters +): Array | undefined { const { next } = parameters - if (next) { - return next.split(',') + if (!next) return undefined + + // Current format: base64url-encoded JSON of the OpenSearch sort values. Unlike + // a plain comma-join/split, this round-trips values that contain commas (e.g. + // an item id or collection id) and preserves nulls instead of coercing them to + // empty strings. (It does not rescue the Long sentinel OpenSearch emits for a + // missing sort field — that value is already lossy once parsed into a JS + // number, before it is ever encoded here. That is handled at the sort level + // instead: `missing: 0` on the default `datetime` sort, and a unique `id` + // tiebreaker on custom sorts. See buildSort / DEFAULT_SORTING and #608 / #1082.) + try { + const decoded: unknown = JSON.parse(Buffer.from(next, 'base64url').toString('utf8')) + if (Array.isArray(decoded)) return decoded + } catch { + // Not a base64url-JSON token; fall back to the legacy comma-joined format + // so pagination links issued by older versions keep working. } - return undefined + return next.split(',') } /** @@ -957,9 +982,12 @@ async function search( // @ts-ignore -- OpenSearch response body is of unknown shape const results = hits.map((r) => (r._source)) const lastItem = hits.at(-1) - let lastItemSort = null + let lastItemSort: string | null = null if (lastItem && lastItem.sort) { - lastItemSort = lastItem.sort.join(',') + // base64url-encoded JSON so sort values survive the `next` pagination token + // even when they contain commas or nulls (which a comma-join corrupts). See + // buildSearchAfter. + lastItemSort = Buffer.from(JSON.stringify(lastItem.sort)).toString('base64url') } const response = { diff --git a/src/lib/types.ts b/src/lib/types.ts index 8e0f2ad8..59052cbb 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -226,13 +226,13 @@ export interface DateQuery { export interface OpenSearchBody { query: OpenSearchFilterQuery sort?: SortParameters - search_after?: string[] | undefined + search_after?: Array | undefined size?: number aggs?: Record } export interface SortRule { - [field: string]: { order: string } + [field: string]: { order: string, missing?: string | number } } export type SortParameters = SortRule[] diff --git a/tests/system/test-api-pagination.ts b/tests/system/test-api-pagination.ts new file mode 100644 index 00000000..614a0a19 --- /dev/null +++ b/tests/system/test-api-pagination.ts @@ -0,0 +1,167 @@ +import anyTest, { type TestFn } from 'ava' +import { deleteAllIndices, refreshIndices } from '../helpers/database.js' +import { ingestItem } from '../helpers/ingest.js' +import { randomId, loadFixture } from '../helpers/utils.js' +import { setup } from '../helpers/system-tests.js' +import type { StandUpResult } from '../helpers/system-tests.js' +import type { Link } from '../../src/lib/types.js' + +type TestContext = StandUpResult & { collectionId: string } +const test = anyTest as TestFn + +/* eslint-disable @typescript-eslint/no-explicit-any, no-await-in-loop */ + +const ingest = async (t: any, item: unknown) => ingestItem({ + ingestQueueUrl: t.context.ingestQueueUrl, + ingestTopicArn: t.context.ingestTopicArn, + item +}) + +// follow a server-issued absolute link href against the local test client +const followNext = async (t: any, links: Link[]) => { + const next = links.find((l) => l.rel === 'next') + if (!next) return undefined + const path = next.href.replace(/^https?:\/\/[^/]+\//, '') + // POST searches return POST-style pagination links (the token is in the body, + // not the href), so follow them with the link's method. + if (next.method === 'POST') { + return t.context.api.client.post(path, { + resolveBodyOnly: false, + throwHttpErrors: false, + json: next.body + }) + } + return t.context.api.client.get(path, { resolveBodyOnly: false, throwHttpErrors: false }) +} + +test.before(async (t) => { + await deleteAllIndices() + t.context = (await setup()) as TestContext + t.context.collectionId = randomId('collection') + await ingest(t, await loadFixture('landsat-8-l1-collection.json', { id: t.context.collectionId })) + await refreshIndices() +}) + +test.after.always(async (t) => { if (t.context.api) await t.context.api.close() }) + +// Regression for #608 / #1082: items with a null `datetime` (start/end set +// instead) broke pagination — OpenSearch emits a Long sentinel sort value that +// the old comma-joined `next` token corrupted, 400ing the follow-up page. +test.serial('paginates through items with null datetime', async (t) => { + const { collectionId } = t.context + const ids: string[] = [] + for (let i = 0; i < 3; i += 1) { + const id = `nodt-${i}` + ids.push(id) + await ingest(t, await loadFixture('stac/LC80100102015050LGN00.json', { + id, + collection: collectionId, + properties: { + datetime: null, + start_datetime: `2015-01-0${i + 1}T00:00:00Z`, + end_datetime: `2015-01-0${i + 1}T01:00:00Z` + } + })) + } + await refreshIndices() + + const seen = new Set() + let resp = await t.context.api.client.get( + `collections/${collectionId}/items?limit=1`, + { resolveBodyOnly: false, throwHttpErrors: false } + ) + for (let page = 0; page < 5; page += 1) { + t.is(resp.statusCode, 200, `page ${page} status`) + for (const f of resp.body.features) seen.add(f.id) + const next = await followNext(t, resp.body.links) + if (!next || resp.body.features.length === 0) break + resp = next + } + + for (const id of ids) t.true(seen.has(id), `paginated to ${id}`) +}) + +// Regression for the custom-`sortby` variant of #608 / #1082: a custom sort +// replaces the default sort's unique `id`/`collection` tiebreakers. When some +// items lack the sort field they all collapse to the same missing-value sort +// key, and with no tiebreaker `search_after` can't disambiguate them — the +// follow-up page skips those items entirely. buildSort must append the `id` +// tiebreaker to custom sorts, as the default sort already guarantees. +test.serial('paginates a custom sortby when some items lack the field', async (t) => { + const { collectionId } = t.context + const ids: string[] = [] + for (let i = 0; i < 4; i += 1) { + const id = `csort-${i}` + ids.push(id) + // Half the items have `eo:cloud_cover`; half omit it (fixture properties are + // shallow-replaced by the override, so omitting the key removes the field). + const properties: Record = { + datetime: `2017-01-0${i + 1}T00:00:00Z` + } + if (i % 2 === 0) properties['eo:cloud_cover'] = i * 10 + await ingest(t, await loadFixture('stac/LC80100102015050LGN00.json', { + id, + collection: collectionId, + properties + })) + } + await refreshIndices() + + const seen = new Set() + let resp = await t.context.api.client.post('search', { + resolveBodyOnly: false, + throwHttpErrors: false, + json: { + collections: [collectionId], + limit: 1, + sortby: [{ field: 'properties.eo:cloud_cover', direction: 'desc' }] + } + }) + for (let page = 0; page < 8; page += 1) { + t.is(resp.statusCode, 200, `page ${page} status`) + for (const f of resp.body.features) seen.add(f.id) + const next = await followNext(t, resp.body.links) + if (!next || resp.body.features.length === 0) break + resp = next + } + + for (const id of ids) t.true(seen.has(id), `paginated to ${id}`) +}) + +// Regression for #823: a `next` link must be returned even when the `sortby` +// field is excluded via the fields extension (pagination uses OpenSearch's sort +// metadata, not the item body). +test.serial('returns next link when the sortby field is excluded', async (t) => { + const { collectionId } = t.context + for (let i = 0; i < 3; i += 1) { + await ingest(t, await loadFixture('stac/LC80100102015050LGN00.json', { + id: `srt-${i}`, + collection: collectionId, + properties: { + datetime: `2016-01-0${i + 1}T00:00:00Z`, + updated: `2016-02-0${i + 1}T00:00:00Z` + } + })) + } + await refreshIndices() + + const resp = await t.context.api.client.post('search', { + resolveBodyOnly: false, + json: { + collections: [collectionId], + limit: 1, + sortby: [{ field: 'properties.updated', direction: 'desc' }], + fields: { exclude: ['properties.updated'], include: [] } + } + }) + t.is(resp.statusCode, 200) + t.is(resp.body.features.length, 1) + const next = resp.body.links.find((l: Link) => l.rel === 'next') + t.truthy(next, 'next link present despite excluded sortby field') + + // Following the (POST) next link must return the *next* item, not repeat page 1. + const page2 = await followNext(t, resp.body.links) + t.is(page2?.statusCode, 200, 'second page fetches successfully') + t.is(page2?.body.features.length, 1) + t.not(page2?.body.features[0].id, resp.body.features[0].id, 'page 2 is a different item') +}) diff --git a/tests/system/test-api-search-get.ts b/tests/system/test-api-search-get.ts index 67ceb72d..2e00e0a2 100644 --- a/tests/system/test-api-search-get.ts +++ b/tests/system/test-api-search-get.ts @@ -76,12 +76,15 @@ test('/search preserve bbox in next links', async (t) => { const nextUrl = new URL(nextLink.href) t.deepEqual(nextUrl.searchParams.get('bbox'), bbox) - t.deepEqual(nextUrl.searchParams.get('next'), - [ - new Date(response.features[1].properties.datetime).getTime(), - response.features[1].id, - response.features[1].collection - ].join(',')) + // the `next` token is base64url-encoded JSON of the OpenSearch sort values + const decodedNext = JSON.parse( + Buffer.from(nextUrl.searchParams.get('next') ?? '', 'base64url').toString('utf8') + ) + t.deepEqual(decodedNext, [ + new Date(response.features[1].properties.datetime).getTime(), + response.features[1].id, + response.features[1].collection + ]) const nextResponse = await got.get(nextUrl).json() as SearchBody t.is(nextResponse.features.length, 0) @@ -114,12 +117,14 @@ test('/search preserve bbox and datetime in next links', async (t) => { const nextLink = response.links.find((x: Link) => x.rel === 'next') const nextUrl = new URL(nextLink.href) - t.deepEqual(nextUrl.searchParams.get('next'), - [ - new Date(response.features[0].properties.datetime).getTime(), - response.features[0].id, - response.features[0].collection - ].join(',')) + const decodedNext = JSON.parse( + Buffer.from(nextUrl.searchParams.get('next') ?? '', 'base64url').toString('utf8') + ) + t.deepEqual(decodedNext, [ + new Date(response.features[0].properties.datetime).getTime(), + response.features[0].id, + response.features[0].collection + ]) t.deepEqual(nextUrl.searchParams.get('bbox'), bbox) t.deepEqual(nextUrl.searchParams.get('datetime'), datetime) })