diff --git a/docs/backend/OPENSEARCH_MIGRATION.md b/docs/backend/OPENSEARCH_MIGRATION.md index 92a1c231b2e5..e19f76223190 100644 --- a/docs/backend/OPENSEARCH_MIGRATION.md +++ b/docs/backend/OPENSEARCH_MIGRATION.md @@ -593,6 +593,51 @@ hand carries genuine semantic intent — and routing by tag is the natural expre - Index timestamp is part of the identity — never hardcode index names - Working index is always a superset — never write to live without also writing to working +### Non-finite numbers (`NaN` / `Infinity`) in manual JSON serialization — #36478 + +**Rule: any `float`/`double` you serialize that originates from an underlying search/index/DB/compute +API can be non-finite. Coerce it to `null` before it reaches the serializer. Never assume a number is +finite just because it is a "score", "distance", or "average".** + +**Where non-finite values come from.** Elasticsearch/OpenSearch set a hit `_score` to **`NaN`** +whenever the hit is *not relevance-scored* — any query that **sorts by a field** without +`track_scores: true`, plus `filter` / `constant_score` / aggregation-only (`size: 0`) contexts. The +same hazard applies to suggester option `score`, aggregation metric values (avg/sum/stats on empty or +degenerate buckets), pgvector distances, and any Java-computed ratio/division (`0.0/0.0 → NaN`, +`x/0.0 → Infinity`). + +**Both serializers are traps, in different ways:** + +| Serializer | Behavior on a non-finite number | Symptom | +|------------|----------------------------------|---------| +| dotCMS `com.dotmarketing.util.json.JSONObject` / `JSONArray` (strict) | `testValidity()` throws `JSONException("JSON does not allow non-finite numbers.")` — **twice over**: eagerly inside `.put(key, value)` *and* again at serialization inside `numberToString` (reached from `.toString()`) | **HTTP 500** | +| Jackson `ObjectMapper` | Does **not** throw by default; writes the bare tokens `NaN` / `Infinity` / `-Infinity`, which are **not valid JSON** | Strict client parsers (`JSON.parse`, most SDKs) **reject the response**; silently non-standard payload. `QUOTE_NON_NUMERIC_NUMBERS` only turns them into `"NaN"` strings — still not a number/null a consumer expects | + +**Why the ES cutover exposed this.** The pre-#36398 endpoints returned Elasticsearch's *native* +serializer output (`SearchResponse.toString()`), and ES XContent serializes non-finite as `null`. +#36398 rebuilt the same wire shape through the **strict** dotCMS `JSONObject`, which rejects what ES +tolerated — turning a silently-null field into a 500. Matching ES's native `null` behavior is +therefore the correct fix, not an arbitrary choice. + +**The pattern (see `ESContentResourcePortlet.toLegacyEsJson` / `hitsToLegacyJson`):** + +- Guard every **explicit** numeric `.put(...)` — the strict writer validates *eagerly*, so the value + must already be finite-or-`null` when inserted: + ```java + .put("_score", finiteOrNull(hit.getScore())) // NaN/Infinity -> JSONObject.NULL + ``` +- For values that enter a tree **unvalidated** — via `new JSONObject(Map)` / bean-wrapping (e.g. the + suggester block, `_source` numerics) — a per-field guard is not enough: they skip the eager check + but still throw at `.toString()`. Either sanitize the source map, or run one recursive pass over the + built tree that coerces every non-finite `Float`/`Double` to `JSONObject.NULL` before serializing. + Only `float`/`double` can be non-finite; integral and `BigDecimal` values are safe. + +**Regression coverage.** `ESContentResourcePortletNaNScoreTest` (fast unit test) drives the adapter +with `NaN`/`Infinity` scores and asserts `_score: null` instead of a throw. Note the integration +tests in `ESContentResourcePortletTest` use relevance-scored `bool`/`term` queries, so they do **not** +reproduce the trigger — an end-to-end guard needs a *field-sorted* (or `constant_score`/`size:0`) +query asserting HTTP 200. + ### Index name divergence between providers **Fresh install** (Phases 1–3 from day zero): ES and OS indices are created in the same bootstrap diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/elasticsearch/ESContentResourcePortletTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/elasticsearch/ESContentResourcePortletTest.java index 47d649245915..135461f88802 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rest/elasticsearch/ESContentResourcePortletTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rest/elasticsearch/ESContentResourcePortletTest.java @@ -41,9 +41,15 @@ import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.Optional; +import javax.servlet.ReadListener; +import javax.servlet.ServletInputStream; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.ws.rs.core.MediaType; @@ -338,6 +344,163 @@ public void test_search_esresponse_preservesLegacyEsWireShape() throws Exception typesAggFound); } + /** + * Method to test: {@link ESContentResourcePortlet#search(HttpServletRequest, HttpServletResponse, String, String, boolean, String, boolean)} + * Given scenario: a query that sorts by a field and does not set {@code track_scores}. + * Elasticsearch/OpenSearch then return a non-finite ({@code NaN}) {@code _score} for + * every hit (the hits are not relevance-scored). + * Expected result: HTTP 200 and each hit's {@code _score} serialized as {@code null} (the + * Elasticsearch-native wire format), not HTTP 500 + * {@code "JSON does not allow non-finite numbers."}. Regression guard for + * #36478. + */ + @Test + public void test_search_fieldSortedQuery_nonFiniteScore_returnsOkWithNullScore() throws Exception { + final ContentType contentType = createContentTypeWithPublishedContent(); + + final String jsonQuery = "{\n" + + " \"query\": { \"bool\": { \"must\": { \"term\": { \"contenttype\": \"" + + contentType.variable() + "\" } } } },\n" + + " \"sort\": [ { \"moddate\": \"desc\" } ]\n" + + "}"; + + final Response response = new ESContentResourcePortlet() + .search(createHttpRequest(false), new MockHttpResponse(), jsonQuery, "0", true, null, false); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + assertHitScoresAreNull(response.getEntity().toString()); + } + + /** + * Method to test: {@link ESContentResourcePortlet#searchRaw(HttpServletRequest)} + * Given scenario: the same field-sorted (non-relevance-scored) query posted to + * {@code /api/es/raw}, whose body is read from the request input stream. + * Expected result: HTTP 200 and {@code _score: null} per hit — {@code /api/es/raw} shares the + * {@code toLegacyEsJson} adapter with {@code /api/es/search}, so it is subject to the + * same #36478 regression and must be guarded too. + */ + @Test + public void test_searchRaw_fieldSortedQuery_nonFiniteScore_returnsOkWithNullScore() throws Exception { + final ContentType contentType = createContentTypeWithPublishedContent(); + + final String jsonQuery = "{\n" + + " \"query\": { \"bool\": { \"must\": { \"term\": { \"contenttype\": \"" + + contentType.variable() + "\" } } } },\n" + + " \"sort\": [ { \"moddate\": \"desc\" } ]\n" + + "}"; + + final Response response = new ESContentResourcePortlet() + .searchRaw(createHttpRequestWithBody(jsonQuery)); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + // searchRaw returns the legacy ES-wire object directly (no "esresponse" wrapper array). + assertHitScoresAreNull(response.getEntity().toString(), false); + } + + /** + * Method to test: {@link ESContentResourcePortlet#search(HttpServletRequest, HttpServletResponse, String, String, boolean, String, boolean)} + * Given scenario: a field-sorted query that does set {@code track_scores: true}, forcing + * Elasticsearch to compute a finite relevance score. + * Expected result: HTTP 200 and a finite (non-null) {@code _score} — the non-finite guard must + * not alter legitimate finite scores. + */ + @Test + public void test_search_fieldSortedQuery_withTrackScores_preservesFiniteScore() throws Exception { + final ContentType contentType = createContentTypeWithPublishedContent(); + + final String jsonQuery = "{\n" + + " \"track_scores\": true,\n" + + " \"query\": { \"bool\": { \"must\": { \"term\": { \"contenttype\": \"" + + contentType.variable() + "\" } } } },\n" + + " \"sort\": [ { \"moddate\": \"desc\" } ]\n" + + "}"; + + final Response response = new ESContentResourcePortlet() + .search(createHttpRequest(false), new MockHttpResponse(), jsonQuery, "0", true, null, false); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + + final JSONObject esresponse = new JSONObject(response.getEntity().toString()) + .getJSONArray("esresponse").getJSONObject(0); + final JSONArray hits = esresponse.getJSONObject("hits").getJSONArray("hits"); + assertTrue("query must return at least one hit", hits.length() > 0); + assertTrue("a track_scores query must keep a finite (non-null) _score", + !hits.getJSONObject(0).isNull("_score")); + } + + private ContentType createContentTypeWithPublishedContent() throws Exception { + final long now = System.currentTimeMillis(); + final ContentType contentType = new ContentTypeDataGen() + .field(new FieldDataGen().name("Title").velocityVarName("title").next()) + .nextPersisted(); + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setProperty("title", "nan-score-" + now) + .nextPersisted(); + ContentletDataGen.publish(contentlet); + APILocator.getContentletAPI().isInodeIndexed(contentlet.getInode(), true); + return contentType; + } + + /** Asserts every hit's {@code _score} is {@code null}, reading the {@code /api/es/search} "esresponse" wrapper. */ + private void assertHitScoresAreNull(final String entity) throws JSONException { + assertHitScoresAreNull(entity, true); + } + + private void assertHitScoresAreNull(final String entity, final boolean wrappedInEsResponse) + throws JSONException { + final JSONObject esresponse = wrappedInEsResponse + ? new JSONObject(entity).getJSONArray("esresponse").getJSONObject(0) + : new JSONObject(entity); + final JSONArray hits = esresponse.getJSONObject("hits").getJSONArray("hits"); + assertTrue("field-sorted query must return at least one hit", hits.length() > 0); + for (int i = 0; i < hits.length(); i++) { + assertTrue("_score of a non-relevance-scored (field-sorted) hit must serialize as null", + hits.getJSONObject(i).isNull("_score")); + } + } + + private HttpServletRequest createHttpRequestWithBody(final String body) throws Exception { + final HttpServletRequest request = createHttpRequest(false); + when(request.getInputStream()) + .thenReturn(new MockServletInputStream( + new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8)))); + return request; + } + + /** Minimal {@link ServletInputStream} over a byte array, to feed a request body to {@code searchRaw}. */ + private static class MockServletInputStream extends ServletInputStream { + + private final InputStream sourceStream; + + MockServletInputStream(final InputStream sourceStream) { + this.sourceStream = sourceStream; + } + + @Override + public int read() throws IOException { + return sourceStream.read(); + } + + @Override + public boolean isFinished() { + try { + return sourceStream.available() == 0; + } catch (final IOException e) { + return true; + } + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public void setReadListener(final ReadListener readListener) { + // no-op: synchronous read is sufficient for the test + } + } + private HttpServletRequest createHttpRequest(final boolean anonymous) throws Exception{ final MockHeaderRequest request = new MockHeaderRequest(new MockSessionRequest( new MockAttributeRequest(new MockHttpRequestIntegrationTest("localhost", "/").request())