PR audit findings (#1155)

* Re-enable AVA worker threads via nodeArguments tsx loader

Register the tsx loader through AVA's nodeArguments config instead of
NODE_OPTIONS, which lets the test suites run with worker threads enabled.
Removes the --no-worker-threads workaround added during the TypeScript
migration (#1067) and the duplicated NODE_OPTIONS exports in package.json
and bin/system-tests.sh.

* Use nvm in pre-commit hook only when it is available

Load and use nvm to select the project's Node version when nvm is installed.
When it is not (e.g. Node provided by Nix or a system package manager), skip
nvm entirely and use whatever node is on PATH rather than failing with
'nvm: command not found'. Version compatibility is left to the developer, with
CI enforcing the pinned Node version.

* Scope collection searches via _index body filter instead of request path

Listing many collection indices in the OpenSearch request path can overflow
the server's URL line-length limit. PR #1047 worked around this by falling
back to searching all indices once more than 10 collections were requested,
which loses scoping and degrades performance.

Instead, always restrict to the requested collections via a terms filter on
the _index metadata field in the query body, which has no comparable length
limit. The request path keeps the default restriction (*, -.*, -collections)
so searches never reach into system or collections indices.

Fixes #770.

* Tighten STAC types: bbox and collection summaries

- StacItem.bbox is now BBox only, matching the STAC document model. The
  request-time string form remains on APIParameters where it belongs; the
  document type no longer forces consumers to narrow away a string. getBBox's
  return type is tightened to match (its sns.ts consumer already indexed it as
  an array).
- Collection summaries now accept a Range Object or JSON Schema in addition to
  value arrays, per the STAC spec, instead of only string[]/number[].
- Fixed a missing space in the StacItem.geometry union.

* Add PR #1155 link to CHANGELOG entries for this branch

* Fix ingestStatus mis-reporting for empty-message ingest errors

Post-ingest SNS notifications derived ingestStatus from the truthiness of the
error message string (payload.error ? 'failed' : 'successful'). Since the
converted ingest lambda passes result.error?.message, an ingest failure whose
Error has an empty message produced a falsy value and was published as
'successful' — silently hiding the failure from SNS subscribers.

Derive the status from the presence of an error (error !== undefined) instead.
Add integration-style unit tests that drive publishRecordToSns through a mocked
SNS client and assert the published MessageAttributes, covering success,
error-with-message, and the empty-message failure case.

* Restore missing-href scenario in asset-proxy test

The TS test conversion (#1095) added href: '' to the 'handles assets without
href' test to satisfy the Assets type, which turned it into a with-empty-href
test that no longer matched its name. Provide an asset genuinely missing href
(via a cast) so it exercises the intended missing-href branch of
getProxiedAssets.

* Preserve declared temporal sub-intervals during auto-extent calculation

The automatic temporal extent feature (#999) replaced a collection's entire
extent.temporal.interval array with a single computed interval whenever the
overall extent needed backfilling, discarding any declared sub-intervals
(interval[1..n]). Only compute the overall extent (interval[0]) from items and
preserve declared sub-intervals. Adds a regression test covering a collection
with multiple intervals.

* Resolve mapped collection indices without re-hashing

When COLLECTION_TO_INDEX_MAPPINGS is configured, a mapped collection resolves to
a specific index name (e.g. a shared or remote index) that must be used as-is.
The scoped-search path was passing every resolved name through
collectionUniqueIndexID(), re-hashing the mapped name into a nonexistent index
so those collections returned no results. Collections with no mapping entry still
fall back to their hashed index name.

Extract the resolution into a pure, exported resolveCollectionIndices(collections,
mapping) so the mapped/unmapped/mixed cases are unit-testable without the
module-level mapping cache, and add tests covering each.

* Prefer request-path index scoping, body filter only as fallback

Scoping collections via an _index body filter against a broad path (_all) makes
OpenSearch enumerate and fan out a can_match request to every shard in the
cluster before skipping the non-matching ones; path-based scoping instead prunes
to the target shards at the coordinating node before any fan-out. Always using
the body filter therefore regressed the shard-fan-out optimization noted in
issue #770 for the common case of a few collections.

Scope in the request path when the resolved index list fits within the URL
line-length budget, and fall back to the _index body filter only when the list
would overflow it. Correctness and the large-collection-list fix are preserved;
typical queries regain path-based shard pruning. Addresses review feedback on
PR #1155.

Author: jkeifer

.husky/pre-commit

@@ -1,11 +1,14 @@
 #!/usr/bin/env sh
 
-# Load nvm
-export NVM_DIR="$HOME/.nvm"
+# Load nvm if available
+export NVM_DIR="${NVM_DIR:-$HOME/.nvm}"
 [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"
 
-# Use project's Node version
-if [ -f .nvmrc ]; then
+# Select the project's Node version via nvm when it's available. When nvm is not
+# installed (e.g. Node provided by Nix or a system package manager), just use
+# whatever node is on PATH — it's up to the developer to run a compatible Node,
+# and CI enforces the pinned version regardless.
+if [ -f .nvmrc ] && command -v nvm >/dev/null 2>&1; then
   nvm use
 fi
 

CHANGELOG.md

@@ -8,6 +8,30 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 
 ### Fixed
+- Scoped searches over collections configured with `COLLECTION_TO_INDEX_MAPPINGS`
+  now target the mapped index name directly. Previously the mapped name was
+  re-hashed as if it were a collection id, producing a nonexistent index and
+  returning no results for those collections. Collections without a mapping entry
+  continue to resolve to their hashed index name.
+  ([1155](https://github.com/stac-utils/stac-server/pull/1155))
+- Automatic temporal extent calculation no longer discards a collection's declared
+  temporal sub-intervals. Only the overall extent (`interval[0]`) is computed from
+  items; any finer-grained sub-intervals (`interval[1..n]`) declared on the
+  collection are now preserved instead of being overwritten with a single computed
+  interval. ([1155](https://github.com/stac-utils/stac-server/pull/1155))
+- Post-ingest SNS notifications now report `ingestStatus` as `failed` for any
+  ingest error, including errors whose message is empty. Previously the status
+  was derived from the truthiness of the error message, so an empty-message
+  error was mis-reported as `successful`. ([1155](https://github.com/stac-utils/stac-server/pull/1155))
+- Searches with too many collections no longer fail with an OpenSearch
+  request-URL length error, and no longer fall back to searching all indices.
+  Collections are scoped in the request path as before when the index list is
+  short; only when it would overflow the URL length limit does the search fall
+  back to an `_index` filter in the query body, preserving index scoping either
+  way. ([770](https://github.com/stac-utils/stac-server/issues/770), [1155](https://github.com/stac-utils/stac-server/pull/1155))
+- Re-enabled AVA worker threads for the test suites by registering the `tsx`
+  loader via AVA's `nodeArguments` config instead of `NODE_OPTIONS`, removing the
+  `--no-worker-threads` workaround introduced during the TypeScript migration. ([1155](https://github.com/stac-utils/stac-server/pull/1155))
 
 - The `datetime_frequency` aggregation now honors the `datetime_frequency_interval`
   parameter (`day`/`week`/`month`/`quarter`/`year`) instead of always
@@ -50,6 +74,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Changed
 
 - Upgrade `got` from 13 to 15 (HTTP client used by the ingest lambda for fetching remote STAC records). No API or behavior changes; remote-record fetches keep lenient Content-Length handling. `got` 15 requires Node.js >= 22, now declared in `engines` and used by all CI workflows.
+- Corrected STAC types: `StacItem.bbox` is now `BBox` (matching the STAC document
+  model, with the request-string form kept only on `APIParameters`), and
+  collection `summaries` now accept range objects and JSON Schema values in
+  addition to value arrays, per the STAC spec. ([1155](https://github.com/stac-utils/stac-server/pull/1155))
 - Typing the top level lambda layer ([1087](https://github.com/stac-utils/stac-server/pull/1087))
 - Typing the api layer in `api.ts`, pushing some minor functions to a new utility files `api-utils.ts` ([1081](https://github.com/stac-utils/stac-server/pull/1081))
 - Converting the database layer to typescript as part of migration ([1077](https://github.com/stac-utils/stac-server/pull/1077))

bin/system-tests.sh

@@ -10,14 +10,12 @@ export ENABLE_TRANSACTIONS_EXTENSION=true
 export REQUEST_LOGGING_ENABLED=false
 # export ENABLE_RESPONSE_COMPRESSION=false
 
-# Force ALL Node processes (including AVA workers) to use tsx
-export NODE_OPTIONS="--import=tsx"
-
 echo "Running tests"
 set +e
 
 # add --match to restrict by test name
-npx ava "tests/system/${TEST_PATTERN}" --serial --verbose --no-worker-threads
+# tsx is registered via the ava `nodeArguments` config in package.json
+npx ava "tests/system/${TEST_PATTERN}" --serial --verbose
 TEST_RESULT="$?"
 set -e
 

package.json

@@ -19,7 +19,7 @@
     "test": "npm run test:unit && npm run test:system",
     "test:system": "./bin/system-tests.sh",
     "test:system:coverage": "c8 npm run test:system",
-    "test:unit": "NODE_OPTIONS='--import=tsx' ava tests/unit/*.[tj]s --no-worker-threads",
+    "test:unit": "ava tests/unit/*.[tj]s",
     "test:unit:coverage": "c8 npm run test:unit",
     "typecheck": "tsc && tsc -p tsconfig.test.json",
     "audit-prod": "npx better-npm-audit audit --production",
@@ -37,6 +37,9 @@
     "timeout": "1m",
     "extensions": [
       "ts"
+    ],
+    "nodeArguments": [
+      "--import=tsx"
     ]
   },
   "publishConfig": {

src/lib/api.ts

@@ -1065,16 +1065,23 @@ const populateTemporalExtentIfMissing = async (
   backend: Backend,
   collection: StacCollection
 ): Promise<void> => {
-  const [start, end] = collection.extent?.temporal?.interval?.[0] ?? [null, null]
+  const intervals = collection.extent?.temporal?.interval
+  const [start, end] = intervals?.[0] ?? [null, null]
 
-  // Nothing to do when both bounds are already declared.
+  // Nothing to do when both bounds of the overall extent are already declared.
   if (start != null && end != null) return
 
   const temporalExtent = await backend.getTemporalExtentFromItems(collection.id)
   if (temporalExtent && collection.extent?.temporal) {
     const [computedStart, computedEnd] = temporalExtent[0]
-    // Preserve any declared bound; only fill the missing one(s) from items.
-    collection.extent.temporal.interval = [[start ?? computedStart, end ?? computedEnd]]
+    // Only the overall extent (interval[0]) is computed from items; any declared
+    // sub-intervals (interval[1..n]) are preserved, as is any bound already set
+    // on the overall extent.
+    const rest = intervals?.slice(1) ?? []
+    collection.extent.temporal.interval = [
+      [start ?? computedStart, end ?? computedEnd],
+      ...rest
+    ]
   }
 }
 

src/lib/database.ts

@@ -78,7 +78,6 @@ export const DEFAULT_FIELDS = [
   'collection',
   'properties.datetime'
 ]
-const MAX_COLLECTIONS_IN_QUERY_PATH = 10
 
 let collectionToIndexMapping: Record<string, string> | null = null
 let unrestrictedIndices: string[] | null = null
@@ -481,6 +480,22 @@ export function collectionUniqueIndexID(collectionId: string): string {
   return `${collectionId.toLowerCase()}-${collectionHash(collectionId)}`
 }
 
+/**
+ * Resolve a list of collection ids to the OpenSearch index names to search.
+ *
+ * A collection present in the `COLLECTION_TO_INDEX_MAPPINGS` mapping resolves to
+ * its configured index name and is used as-is (it may be a shared or remote index
+ * that must not be altered). Any other collection — including every collection
+ * when the mapping is empty — resolves to the hashed index name derived from its
+ * id. This matches how mapping values are used in populateUnrestrictedIndices.
+ */
+export function resolveCollectionIndices(
+  collections: string[],
+  mapping: Record<string, string>
+): string[] {
+  return collections.map((c) => mapping[c] ?? collectionUniqueIndexID(c))
+}
+
 function buildItemSearchQuery(parameters: QueryParameters): OpenSearchFilterQuery {
   const { intersects, collections, ids } = parameters
   const filterQueries: OpenSearchFilterQuery[] = []
@@ -523,6 +538,15 @@ function buildItemSearchQuery(parameters: QueryParameters): OpenSearchFilterQuer
   }
 }
 
+// Normalise a bool `filter`/`must_not` clause, which may be a single query
+// object or an array, into an array.
+function toFilterArray(
+  v: OpenSearchFilterQuery | OpenSearchFilterQuery[] | undefined
+): OpenSearchFilterQuery[] {
+  if (!v) return []
+  return Array.isArray(v) ? v : [v]
+}
+
 function buildOpenSearchQuery(parameters: QueryParameters): OpenSearchBody {
   const { query, filter, intersects, collections, ids } = parameters
 
@@ -547,25 +571,17 @@ function buildOpenSearchQuery(parameters: QueryParameters): OpenSearchBody {
     itemSearchQuery = buildItemSearchQuery(parameters)
   }
 
-  // Normalise filter/must_not which can be single item or array
-  const toArray = (
-    v: OpenSearchFilterQuery | OpenSearchFilterQuery[] | undefined
-  ): OpenSearchFilterQuery[] => {
-    if (!v) return []
-    return Array.isArray(v) ? v : [v]
-  }
-
   const combinedFilter: OpenSearchFilterQuery[] = [
-    ...toArray(cql2Query.bool?.filter),
-    ...toArray(stacqlQuery.bool?.filter),
-    ...toArray(itemSearchQuery.bool?.filter)
+    ...toFilterArray(cql2Query.bool?.filter),
+    ...toFilterArray(stacqlQuery.bool?.filter),
+    ...toFilterArray(itemSearchQuery.bool?.filter)
   ]
   const combinedShould: OpenSearchFilterQuery[] = [
     ...(cql2Query.bool?.should || []),
   ]
   const combinedMustNot: OpenSearchFilterQuery[] = [
-    ...toArray(cql2Query.bool?.must_not),
-    ...toArray(stacqlQuery.bool?.must_not),
+    ...toFilterArray(cql2Query.bool?.must_not),
+    ...toFilterArray(stacqlQuery.bool?.must_not),
   ]
 
   if (!isEmpty(combinedFilter)) {
@@ -586,6 +602,45 @@ function buildOpenSearchQuery(parameters: QueryParameters): OpenSearchBody {
   return { query: { bool: osBool } }
 }
 
+/**
+ * Restrict a query to a set of indices by adding a `terms` filter on the
+ * `_index` metadata field.
+ *
+ * This is the fallback for when the index list is too long to list in the request
+ * path: OpenSearch puts the path-based index list in the request URL, which is
+ * subject to the server's `http.max_initial_line_length` limit (4KB by default),
+ * so a search spanning many collections can exceed that and fail. The body has no
+ * comparable limit, so this keeps the search scoped without the cliff. The search
+ * must then be sent to a broad path (the default restriction) for the `_index`
+ * filter to narrow against. See issue #770.
+ */
+function restrictQueryToIndices(
+  body: OpenSearchBody, indices: string[]
+): OpenSearchBody {
+  const indexFilter: OpenSearchFilterQuery = { terms: { _index: indices } }
+
+  // match_all means there are no other constraints; wrap it so we can filter.
+  if (body.query.match_all) {
+    return { ...body, query: { bool: { filter: [indexFilter] } } }
+  }
+
+  const bool = body.query.bool ?? {}
+  const filter = [...toFilterArray(bool.filter), indexFilter]
+
+  return { ...body, query: { ...body.query, bool: { ...bool, filter } } }
+}
+
+// The comma-joined index list goes in the request path, which shares OpenSearch's
+// `http.max_initial_line_length` budget (4KB by default) with the method, the
+// rest of the path, the query string, and URL-encoding expansion. Cap the joined
+// index list well below that so the rest of the request line always fits; longer
+// lists fall back to an `_index` body filter instead.
+const MAX_INDEX_PATH_LENGTH = 2048
+
+function indexPathWithinLimit(indices: string[]): boolean {
+  return indices.join(',').length <= MAX_INDEX_PATH_LENGTH
+}
+
 function buildIdQuery(id: string): OpenSearchBody {
   return {
     query: {
@@ -869,10 +924,6 @@ async function populateCollectionToIndexMapping() {
   }
 }
 
-async function indexForCollection(collectionId: string): Promise<string> {
-  return (collectionToIndexMapping ?? {})[collectionId] || collectionId
-}
-
 async function populateUnrestrictedIndices() {
   if (!unrestrictedIndices) {
     if (process.env['COLLECTION_TO_INDEX_MAPPINGS']) {
@@ -914,32 +965,37 @@ export async function constructSearchParams(
     body.search_after = buildSearchAfter(parameters)
   }
 
-  let indices
+  // Default index restriction used when no collections are requested:
+  // `*, -.*, -collections` (all local item indices, excluding system indices and
+  // the collections index).
+  if (!unrestrictedIndices) {
+    await populateUnrestrictedIndices()
+  }
+  let index = unrestrictedIndices!
+
   if (Array.isArray(collections) && collections.length) {
-    if (collections.length > MAX_COLLECTIONS_IN_QUERY_PATH) {
-      indices = ['_all']
-    } else if (process.env['COLLECTION_TO_INDEX_MAPPINGS']) {
-      if (!collectionToIndexMapping) await populateCollectionToIndexMapping()
-      indices = await Promise.all(collections.map(async (x) => await indexForCollection(x)))
-    } else {
-      indices = collections
+    // Resolve the explicitly-requested collections to their indices.
+    if (process.env['COLLECTION_TO_INDEX_MAPPINGS'] && !collectionToIndexMapping) {
+      await populateCollectionToIndexMapping()
     }
-  } else {
-    if (!unrestrictedIndices) {
-      await populateUnrestrictedIndices()
+    const indices = resolveCollectionIndices(collections, collectionToIndexMapping ?? {})
+
+    // Prefer scoping in the request path: OpenSearch prunes to those shards at
+    // the coordinating node before fan-out, which is cheaper than searching all
+    // indices and filtering per-shard. But the path is part of the request URL,
+    // so a long index list can overflow the server's line-length limit; in that
+    // case fall back to an `_index` terms filter in the request body (which has
+    // no comparable limit), leaving the path at the default restriction so it
+    // never reaches into system indices.
+    if (indexPathWithinLimit(indices)) {
+      index = indices
+    } else {
+      body = restrictQueryToIndices(body, indices)
     }
-    indices = unrestrictedIndices!
   }
-  // hash indices
-  indices = indices.map((index) => {
-    if (DEFAULT_INDICES.includes(index) || index === '_all') {
-      return index
-    }
-    return collectionUniqueIndexID(index)
-  })
 
   const searchParams: SearchParameters = {
-    index: indices,
+    index,
     body,
     size: limit,
     track_total_hits: true

src/lib/sns.ts

@@ -6,6 +6,8 @@ import { StacRecord } from './types.js'
 
 interface SNSPayload {
   record: StacRecord
+  // The ingest error message, or `undefined` when ingestion succeeded. An empty
+  // string is a failure with no message, not a success.
   error: string | undefined
 }
 /**
@@ -36,7 +38,9 @@ const attrsFromPayload = function (
     },
     ingestStatus: {
       DataType: 'String',
-      StringValue: payload.error ? 'failed' : 'successful'
+      // Presence of an error, not its message text, determines failure: an
+      // error with an empty message still means the ingest failed.
+      StringValue: payload.error !== undefined ? 'failed' : 'successful'
     },
     collection: {
       DataType: 'String',

src/lib/stac-utils.ts

@@ -87,7 +87,7 @@ export function getStartAndEndDates(
   return { startDate, endDate }
 }
 
-export function getBBox(record: StacRecord): string | BBox | undefined {
+export function getBBox(record: StacRecord): BBox | undefined {
   if (isCollection(record)) {
     return record.extent?.spatial?.bbox?.length > 0
       ? record.extent.spatial.bbox[0]

src/lib/types.ts

@@ -11,8 +11,8 @@ export interface StacItem {
   stac_version: string
   stac_extensions?: string[]
   id: string
-  geometry: Geometry| null
-  bbox?: BBox | string // only required if geometry is not null
+  geometry: Geometry | null
+  bbox?: BBox // only required if geometry is not null
   properties: ItemProperties
   links: Link[]
   assets: Assets
@@ -37,7 +37,7 @@ export interface StacCollection {
   extent: Extent
   queryables?: Queryables
   aggregations?: Aggregation[]
-  summaries?: {[key: string]: string[] | number[]}
+  summaries?: {[key: string]: Summary}
   links: Link[]
   assets?: Assets
   item_assets?: {[key: string]: Asset}
@@ -48,6 +48,25 @@ export interface FeatureCollection {
   features: StacItem[]
 }
 
+/**
+ * A Range Object summary. Per the STAC spec a range may be open-ended, so a
+ * bound may be omitted; at least one of `minimum`/`maximum` is present.
+ */
+export type SummaryRange =
+  | { minimum: string | number, maximum?: string | number }
+  | { maximum: string | number, minimum?: string | number }
+
+/**
+ * A collection summary. Per the STAC spec a summary may be a set of distinct
+ * values, a Range Object with `minimum`/`maximum` bounds, or a JSON Schema
+ * describing the values.
+ * https://github.com/radiantearth/stac-spec/blob/master/collection-spec/collection-spec.md#summaries
+ */
+export type Summary =
+  | Array<string | number | boolean>
+  | SummaryRange
+  | { [key: string]: unknown } // JSON Schema
+
 export interface StacCatalog {
   stac_version: string
   type: string