fix(search): guard non-finite _score in /api/es/search response (#36478) - #36480
Conversation
After the phase-aware SearchAPI cutover (#36398), /api/es/search and /api/es/raw rebuild the legacy Elasticsearch-wire response via dotCMS's JSONObject. Elasticsearch returns a non-finite _score (NaN) for hits that are not relevance-scored — field-sorted queries (unless track_scores=true) and filter/constant_score/aggregation-only contexts. JSONObject.put rejects non-finite numbers ("JSON does not allow non-finite numbers"), producing an HTTP 500. The previous implementation returned ES-native JSON, which serializes NaN as null, so the error did not occur before the cutover. Coerce non-finite hit scores to null in ESContentResourcePortlet (matching ES's native wire format), and add a fast unit regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @dsilvam's task in 12m 35s —— View job 🔍 dotCMS Backend Review[🟡 Medium]
import static org.junit.Assert.fail;💡 Remove the unused import. [🟡 Medium]
@Test
public void test_infinite_score_serializes_as_null() throws Exception {
final JSONObject result = hitsToLegacyJson(hitsWithScore(Float.POSITIVE_INFINITY));
...
}💡 Add a fourth Sub-agent results:
No Critical or High severity findings. The fix itself is correct and appropriately scoped. Next steps
Note: I'm unable to submit a formal GitHub PR review (approve/request changes) per my configured capabilities. See the FAQ for more information. |
|
Tick the box to add this pull request to the merge queue (same as
|
…sion (dotCMS#36478) (dotCMS#36489) ### Proposed Changes Follow-up to dotCMS#36478. The base fix (non-finite `_score` → HTTP 500 `"JSON does not allow non-finite numbers."`) landed on `main` via **dotCMS#36480** with **unit** coverage only. This PR adds the missing **end-to-end integration coverage** for the actual production trigger, and documents the failure class so it does not recur. **No production code change** — this is test + docs on top of the merged fix. * **Integration tests** — `ESContentResourcePortletTest` (already registered in `MainSuite2a`), covering both affected endpoints with the reliable NaN trigger (a query that **sorts by a field** without `track_scores`): * `/api/es/search` (`search()`) → field-sorted query → **HTTP 200** with each hit's `_score` = `null`. * `/api/es/raw` (`searchRaw()`, body read from the request input stream) → same query → **HTTP 200** with `_score` = `null`. `/api/es/raw` shares the `toLegacyEsJson` adapter, so it is subject to the same regression. * Control: `track_scores: true` keeps a **finite** `_score` (the guard must not alter legitimate scores). * **Docs** — `docs/backend/OPENSEARCH_MIGRATION.md` → *Known Gotchas* → new section *"Non-finite numbers (`NaN`/`Infinity`) in manual JSON serialization"*. ### Why this matters The existing integration tests only exercise relevance-scored `bool`/`term` queries, so the field-sorted path that produces `NaN` scores in production was never covered end-to-end. The unit test proves the mapping logic; these ITs prove the full REST → phase-aware `SearchAPI` → neutral response → legacy-wire adapter path returns valid JSON. ### Documented gotcha (prevention) Any `float`/`double` serialized from an underlying search/index/DB/compute API can be non-finite (`_score` is `NaN` on field-sorted / filter / `constant_score` / aggregation-only queries; also suggester scores, aggregation metrics, computed ratios). The two serializers fail differently: | Serializer | Behavior on non-finite | Symptom | |------------|------------------------|---------| | dotCMS strict `JSONObject`/`JSONArray` | `testValidity()` throws — eagerly in `put(...)` and again at serialization in `numberToString` | **HTTP 500** | | Jackson `ObjectMapper` | Writes bare `NaN`/`Infinity` tokens (not valid JSON) | Strict client parsers reject the response | Rule: coerce non-finite `float`/`double` to `null` (ES-native wire format) before it reaches the serializer. ### Checklist - [x] Tests — 3 new integration cases in `ESContentResourcePortletTest` (registered in `MainSuite2a`); module `test-compile` passes. - [x] Translations — N/A (no user-facing strings). - [x] Security Implications Contemplated — none; test + docs only, no schema/index/API-contract change. ### Notes - The base fix is already on `main` (dotCMS#36480); this PR does not re-touch `ESContentResourcePortlet`. - A codebase audit for other manual-serialization sinks found no additional reachable failure point (suggester score is same-sink but finite in practice; the AI vector-search `distance` path was verified as a false positive — model embeddings are never zero-magnitude, so pgvector cosine distance stays finite). This PR relates to: dotCMS#36478 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#36581) (dotCMS#36582) ## Proposed Changes Fixes dotCMS#36581. `/api/es/search` (and `/api/es/raw`) silently dropped the per-hit `sort` array from the response. When a query sorts by a field — e.g. a `_geo_distance` sort — Elasticsearch/OpenSearch return the computed sort value on each hit under `hits.hits[i].sort`; for a geo sort that value *is* the distance clients display. Since the phase-aware SearchAPI cutover (dotCMS#36398, first released `v26.07.04-01`) the legacy ES-wire response is rebuilt manually and the `sort` element was lost. dotCMS#36480 (the non-finite `_score` fix) only touched `_score`; it did not restore `sort`, and the `track_scores: true` workaround does not bring it back. ### Root cause (two layers) 1. **Model** — `com.dotcms.content.index.domain.SearchHit` had no component for per-hit sort values, so `SearchHit.from(esSearchHit)` / `from(osHit)` discarded `getSortValues()` / `sort()` on ingestion. 2. **Serialization** — `ESContentResourcePortlet.hitsToLegacyJson()` emitted only `_id/_index/_score/_source`. ### Fix (symmetric ES/OS) - `SearchHit`: add a `sortValues` component, populated from `esSearchHit.getSortValues()` (ES `Object[]`) and `osHit.sort()` (OpenSearch `List<FieldValue>`, unwrapped via `_get()`). Defaults to an empty list, so JSON (de)serialization of cached hits stays backward-compatible. - `hitsToLegacyJson`: emit a `sort` array per hit **only when sort values are present** — relevance-only queries get no `sort` key, matching the native engine wire format. Non-finite entries are coerced to `null` (new `finiteOrNull(Object)` overload), consistent with the `_score` handling. Both `/api/es/search` (GET + POST) and `/api/es/raw` share `toLegacyEsJson`, so all are covered. ## Testing Integration tests added to `ESContentResourcePortletTest` (registered in `MainSuite2a`): - `test_search_geoDistanceSort_emitsPerHitSortValues` — a `_geo_distance`-sorted query returns each hit with a `sort` array carrying the finite distance, in ascending order (nearest ~0 km). - `test_search_relevanceOnlyQuery_omitsPerHitSort` — a relevance-only query returns hits with **no** `sort` key. Verified end-to-end on a local build in migration Phase 0: the customer's exact `_geo_distance` query now returns `sort: [0.0003, 11.82, 35.03, 71.57]` km per hit — identical to the native Elasticsearch response — with `_score` still `null`. | | before fix | native ES | with fix | |---|---|---|---| | per-hit `sort` | ❌ absent | ✅ `[0.0003, 11.82, 35.03, 71.57]` | ✅ identical | 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Proposed Changes
Fixes #36478. Canonical fix on
main; will be cherry-picked torelease-26.07.06-01after merge (per dotCMS backport convention)._scorevalues (NaN/Infinity) tonullinESContentResourcePortlet.hitsToLegacyJson, restoring the pre-regression Elasticsearch-native wire format.ESContentResourcePortletNaNScoreTest) coveringNaN,Infinity, and finite scores.Root cause
Regression introduced by #36398 (route
/api/es/raw&/api/es/searchthrough the phase-aware SearchAPI), first released in v26.07.04-01.The endpoints now rebuild the legacy ES-wire response via dotCMS's
com.dotmarketing.util.json.JSONObjectinstead of returning ES's native JSON. The per-hit score was written as a rawfloat:Elasticsearch returns a non-finite
_score(NaN) for hits that are not relevance-scored — most commonly any query that sorts by a field (unlesstrack_scores=true), plus filter/constant_score/aggregation-only contexts.JSONObject.put(...)runstestValidity(), which throwsJSONException("JSON does not allow non-finite numbers.")on non-finite values → HTTP 500. The previous implementation returnedSearchResponse.toString()(ES XContent), which serializesNaNasnull, so the error did not occur before the cutover.Impact was systemic and not content-related (reproducible on Production/Staging/UAT), affecting custom
/api/es/searchcalls and built-in Site Search.Fix
No response-shape change for finite scores; a score that previously 500'd now serializes as
"_score": null.Checklist
ESContentResourcePortletNaNScoreTest(3 tests, all passing):NaN→null,Infinity→null, finite score preserved.Additional Info
Interim workaround for affected customers (no deploy): for client-controlled queries, add
"track_scores": trueto sorted queries so ES returns a finite score. Does not cover built-in Site Search — that is resolved by this patch.Suggester
scorefields are unaffected: they carry only finite ES suggester scores and are not the reproduced path.🤖 Generated with Claude Code
This PR fixes: #36478