diff --git a/dotCMS/src/main/java/com/dotcms/content/index/domain/SearchHit.java b/dotCMS/src/main/java/com/dotcms/content/index/domain/SearchHit.java index b4278390a21c..2ea670fff2cb 100644 --- a/dotCMS/src/main/java/com/dotcms/content/index/domain/SearchHit.java +++ b/dotCMS/src/main/java/com/dotcms/content/index/domain/SearchHit.java @@ -1,7 +1,10 @@ package com.dotcms.content.index.domain; import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.Arrays; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; /** * Immutable domain representation of a single search result hit from any search engine. @@ -35,6 +38,9 @@ * @param getScore the search relevance score for this hit * @param getFields the document fields retrieved by the search query (additional fields beyond * the source document that were explicitly requested), empty if none were requested + * @param getSortValues the per-hit sort values the engine returns when the query sorts by a field + * (e.g. the computed distance for a {@code _geo_distance} sort), in the order the + * {@code sort} clause declared; empty for relevance-only (unsorted) queries * @author Fabrizio Araya * @see SearchHits * @see com.dotcms.content.index.ContentFactoryIndexOperations @@ -44,15 +50,17 @@ public record SearchHit( @JsonProperty("index") String getIndex, @JsonProperty("sourceAsMap") Map getSourceAsMap, @JsonProperty("score") float getScore, - @JsonProperty("fields") Map getFields) { + @JsonProperty("fields") Map getFields, + @JsonProperty("sortValues") List getSortValues) { /** - * Canonical constructor. Collection components default to an empty map when {@code null} so the - * accessors never return {@code null} (mirrors the previous Immutables collection defaults). + * Canonical constructor. Collection components default to an empty map/list when {@code null} so + * the accessors never return {@code null} (mirrors the previous Immutables collection defaults). */ public SearchHit { getSourceAsMap = getSourceAsMap == null ? Map.of() : getSourceAsMap; getFields = getFields == null ? Map.of() : getFields; + getSortValues = getSortValues == null ? List.of() : getSortValues; } /** @@ -71,12 +79,14 @@ public static Builder builder() { * @return a new SearchHit instance */ public static SearchHit from(org.elasticsearch.search.SearchHit esSearchHit) { + final Object[] esSortValues = esSearchHit.getSortValues(); return builder() .id(esSearchHit.getId()) .sourceAsMap(esSearchHit.getSourceAsMap()) .fields(esSearchHit.getFields()) .score(esSearchHit.getScore()) .index(esSearchHit.getIndex()) + .sortValues(esSortValues == null ? null : Arrays.asList(esSortValues)) .build(); } @@ -108,11 +118,22 @@ public static SearchHit from(org.opensearch.client.opensearch.core.search.Hit sourceMap = Map.of(); } + // OpenSearch returns per-hit sort values as a List tagged union; unwrap each to + // its raw scalar (Double/Long/Boolean/String, or null) so the neutral hit mirrors ES's Object[]. + final List osSortValues = osHit.sort(); + final List sortValues = (osSortValues == null || osSortValues.isEmpty()) + ? null + : osSortValues.stream() + .map(fieldValue -> (fieldValue == null || fieldValue.isNull()) + ? null : fieldValue._get()) + .collect(Collectors.toList()); + return builder() .id(osHit.id()) .index(osHit.index()) .sourceAsMap(sourceMap) .score(osHit.score() != null ? osHit.score().floatValue() : 0.0f) + .sortValues(sortValues) .build(); } @@ -128,6 +149,7 @@ public static final class Builder { private Map sourceAsMap = Map.of(); private float score; private Map fields = Map.of(); + private List sortValues = List.of(); public Builder id(final String id) { this.id = id; @@ -156,8 +178,13 @@ public Builder fields(final Map fields) { return this; } + public Builder sortValues(final List sortValues) { + this.sortValues = sortValues == null ? List.of() : sortValues; + return this; + } + public SearchHit build() { - return new SearchHit(id, index, sourceAsMap, score, fields); + return new SearchHit(id, index, sourceAsMap, score, fields, sortValues); } } } diff --git a/dotCMS/src/main/java/com/dotcms/rest/elasticsearch/ESContentResourcePortlet.java b/dotCMS/src/main/java/com/dotcms/rest/elasticsearch/ESContentResourcePortlet.java index c8c83d89ab0a..85b7f3c47d61 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/elasticsearch/ESContentResourcePortlet.java +++ b/dotCMS/src/main/java/com/dotcms/rest/elasticsearch/ESContentResourcePortlet.java @@ -356,11 +356,23 @@ private static JSONObject hitsToLegacyJson(final SearchHits hits) throws JSONExc .put("relation", total.relation() == Relation.EQUAL_TO ? "eq" : "gte")); final JSONArray arr = new JSONArray(); for (final SearchHit hit : hits.getHits()) { - arr.put(new JSONObject() + final JSONObject hitJson = new JSONObject() .put("_id", hit.getId()) .put("_index", hit.getIndex()) .put("_score", finiteOrNull(hit.getScore())) - .put("_source", new JSONObject(hit.getSourceAsMap()))); + .put("_source", new JSONObject(hit.getSourceAsMap())); + // Preserve the engine's native per-hit "sort" array (e.g. the _geo_distance value that + // field-sorted queries depend on). Only emitted when the query actually sorted by a + // field — relevance-only queries carry no sort values and get no "sort" key, matching + // the native ES/OS wire format. Non-finite entries are coerced to null like _score. + if (!hit.getSortValues().isEmpty()) { + final JSONArray sortArr = new JSONArray(); + for (final Object sortValue : hit.getSortValues()) { + sortArr.put(finiteOrNull(sortValue)); + } + hitJson.put("sort", sortArr); + } + arr.put(hitJson); } hitsObj.put("hits", arr); return hitsObj; @@ -379,6 +391,26 @@ private static Object finiteOrNull(final float value) { return Float.isFinite(value) ? Float.valueOf(value) : JSONObject.NULL; } + /** + * Object overload of {@link #finiteOrNull(float)} for per-hit {@code sort} values, which arrive as + * boxed scalars ({@link Double} for a {@code _geo_distance} sort, {@link Long}, {@link String}, …). + * Non-finite floating-point values are coerced to {@code null} for the same reason as {@code _score}; + * every other value (including {@code null}) passes through unchanged. + */ + private static Object finiteOrNull(final Object value) { + if (value == null) { + return JSONObject.NULL; + } + if (value instanceof Double) { + final double doubleValue = (Double) value; + return Double.isFinite(doubleValue) ? value : JSONObject.NULL; + } + if (value instanceof Float) { + return finiteOrNull(((Float) value).floatValue()); + } + return value; + } + /** Maps the neutral aggregation tree (keyed by aggregation name) to the ES-native {@code aggregations} JSON. */ private static JSONObject aggregationsToLegacyJson(final Map tree) throws JSONException { diff --git a/dotCMS/src/test/java/com/dotcms/content/index/domain/SearchHitTest.java b/dotCMS/src/test/java/com/dotcms/content/index/domain/SearchHitTest.java new file mode 100644 index 000000000000..a1e3ea7a589e --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/content/index/domain/SearchHitTest.java @@ -0,0 +1,77 @@ +package com.dotcms.content.index.domain; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.List; +import org.junit.Test; +import org.opensearch.client.opensearch._types.FieldValue; +import org.opensearch.client.opensearch.core.search.Hit; + +/** + * Unit tests for {@link SearchHit} conversion from vendor types, focused on the per-hit + * {@code sortValues} mapping restored for + * #36581. Exercises the OpenSearch + * {@code Hit.sort()} unwrap branch (a {@code List} tagged union), which the ES-only + * integration environment does not cover. + */ +public class SearchHitTest { + + /** + * Method to test: {@link SearchHit#from(Hit)} + * Given scenario: an OpenSearch hit whose {@code sort()} carries a double and a string FieldValue. + * Expected result: {@code getSortValues()} unwraps each FieldValue to its raw scalar, in order. + */ + @Test + public void from_openSearchHit_unwrapsSortValues() { + final Hit osHit = Hit.of(builder -> builder + .index("idx") + .id("1") + .sort(Arrays.asList(FieldValue.of(11.82d), FieldValue.of("abc")))); + + final SearchHit hit = SearchHit.from(osHit); + + final List sortValues = hit.getSortValues(); + assertEquals("both sort values must survive the conversion", 2, sortValues.size()); + assertEquals(11.82d, ((Number) sortValues.get(0)).doubleValue(), 0.0001d); + assertEquals("abc", sortValues.get(1)); + } + + /** + * Method to test: {@link SearchHit#from(Hit)} + * Given scenario: an OpenSearch hit with no {@code sort()} values (a relevance-only query). + * Expected result: {@code getSortValues()} is empty (never null), so serializers omit the + * {@code sort} key instead of emitting an empty array. + */ + @Test + public void from_openSearchHit_noSort_yieldsEmptySortValues() { + final Hit osHit = Hit.of(builder -> builder.index("idx").id("1")); + + final SearchHit hit = SearchHit.from(osHit); + + assertTrue("a hit without a sort clause must expose empty sort values", + hit.getSortValues().isEmpty()); + } + + /** + * Method to test: {@link SearchHit#from(Hit)} + * Given scenario: the {@code sort()} list contains a raw Java {@code null} element (defensive + * against a client that yields nulls). + * Expected result: the conversion does not NPE and maps the null element to a null entry. + */ + @Test + public void from_openSearchHit_nullSortElement_doesNotThrow() { + final Hit osHit = Hit.of(builder -> builder + .index("idx") + .id("1") + .sort(Arrays.asList(FieldValue.of(1.0d), null))); + + final SearchHit hit = SearchHit.from(osHit); + + final List sortValues = hit.getSortValues(); + assertEquals(2, sortValues.size()); + assertEquals(1.0d, ((Number) sortValues.get(0)).doubleValue(), 0.0001d); + assertTrue("a null FieldValue element must map to null, not throw", sortValues.get(1) == null); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/rendering/velocity/viewtools/ContentSearchToolTest.java b/dotcms-integration/src/test/java/com/dotcms/rendering/velocity/viewtools/ContentSearchToolTest.java index 254a289ea4cf..48d2c01f9f6a 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rendering/velocity/viewtools/ContentSearchToolTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rendering/velocity/viewtools/ContentSearchToolTest.java @@ -282,6 +282,25 @@ public class ContentSearchToolTest extends IntegrationTestBase { #end """; + /** + * A field-sorted {@code $estool.search(...)} whose per-hit {@code sort} values the template reads + * via {@code $hit.sortValues} (property → {@code getSortValues()}) and + * {@code $hit.getSortValues().get(0)}. Guards the Velocity-facing side of + * #36581: the neutral {@link SearchHit} + * now carries the sort values and exposes them under a bean-style {@code get}-accessor, so VTL + * templates that read the per-hit sort value (e.g. the {@code moddate} sort key, or a + * {@code _geo_distance} distance) resolve it instead of silently getting {@code null}. + */ + private static final String SEARCH_SORT_VTL = """ + #set($esQuery = '{"size":5,"query":{"bool":{"filter":[{"term":{"live":true}}]}},"sort":[{"moddate":"desc"}]}') + #set($results = $estool.search($esQuery)) + #foreach($hit in $results.hits.hits) + hit id: $!{hit.id} + sortValues: $!{hit.sortValues} + firstSort: $!{hit.getSortValues().get(0)} + #end + """; + /** * Locks the same non-aggregation accessor surface for {@code $estool.raw(...)} — * {@link ContentSearchResponse}. Because {@code raw()} returns a record, its top-level @@ -635,6 +654,25 @@ public void searchVtl_rendersHitFieldsAndTiming() throws Exception { output.contains("inode")); } + /** + * Velocity-facing regression for #36581: + * a field-sorted {@code $estool.search(...)} must expose the per-hit sort value in VTL. Asserts + * {@code $hit.sortValues} renders a non-empty array and {@code $hit.getSortValues().get(0)} renders + * the sort key (the {@code moddate} epoch here) — proving the neutral {@link SearchHit} both carries + * the value and exposes it under a bean {@code get}-accessor that Velocity resolves. + */ + @Test + public void searchVtl_rendersPerHitSortValues() throws Exception { + final String output = VelocityUtil.eval(SEARCH_SORT_VTL, velocityContext()); + Logger.info(this, "\n===== search sort VTL output =====\n" + output + "\n================================"); + + assertTrue("a field-sorted search() must expose a non-empty per-hit sort array via " + + "$hit.sortValues (getSortValues())", + Pattern.compile("sortValues:\\s*\\[[^\\]]+\\]").matcher(output).find()); + assertTrue("$hit.getSortValues().get(0) must render the moddate sort key as a number", + Pattern.compile("firstSort:\\s*\\d+").matcher(output).find()); + } + /** * Gap closer for the {@code raw()} sibling: exercises the hit & timing accessor surface of * {@code $estool.raw(...)} — a {@link ContentSearchResponse} record — through Velocity. Because 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 135461f88802..35047a02ce53 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 @@ -428,6 +428,117 @@ public void test_search_fieldSortedQuery_withTrackScores_preservesFiniteScore() !hits.getJSONObject(0).isNull("_score")); } + /** + * Method to test: {@link ESContentResourcePortlet#search(HttpServletRequest, HttpServletResponse, String, String, boolean, String, boolean)} + * Given scenario: a query that sorts by {@code _geo_distance} (a non-score field). Elasticsearch + * returns the computed distance for every hit under {@code hits.hits[i].sort} — that array + * is the value geo clients read to display each result's distance. + * Expected result: HTTP 200 and each hit carries a {@code sort} array whose first element is the + * finite geo distance, in ascending order. Regression guard for + * #36581: the phase-aware + * SearchAPI cutover (#36398) dropped the per-hit {@code sort} array from the rebuilt + * legacy ES-wire response, and #36480 (the {@code _score} NaN fix) did not restore it. + */ + @Test + public void test_search_geoDistanceSort_emitsPerHitSortValues() throws Exception { + final ContentType contentType = createGeoContentTypeWithCenters(); + final String geoField = contentType.variable().toLowerCase() + ".latlong"; + + // Origin matches "Center-0km"; the three centers sit at increasing distances. + final String jsonQuery = "{\n" + + " \"query\": { \"bool\": { \"must\": { \"term\": { \"contenttype\": { \"value\": \"" + + contentType.variable().toLowerCase() + "\" } } } } },\n" + + " \"sort\": [ { \"_geo_distance\": { \"" + geoField + "\": " + + "{ \"lat\": 42.4608, \"lon\": -83.1215 }, \"order\": \"asc\", \"unit\": \"km\", " + + "\"distance_type\": \"arc\", \"ignore_unmapped\": true } } ]\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"); + assertEquals("the three published centers must match", 3, hits.length()); + + double previousDistance = -1d; + for (int i = 0; i < hits.length(); i++) { + final JSONObject hit = hits.getJSONObject(i); + assertTrue("a _geo_distance-sorted hit must carry the per-hit 'sort' array (#36581)", + hit.has("sort") && !hit.isNull("sort")); + final JSONArray sort = hit.getJSONArray("sort"); + assertTrue("the 'sort' array must carry the computed distance", sort.length() >= 1); + final double distance = Double.parseDouble(String.valueOf(sort.get(0))); + assertTrue("the geo distance must be finite", Double.isFinite(distance)); + assertTrue("per-hit sort distances must be in ascending order", distance >= previousDistance); + previousDistance = distance; + } + // The closest center sits on the origin, so its distance rounds to ~0 km. + final double closest = Double.parseDouble( + String.valueOf(hits.getJSONObject(0).getJSONArray("sort").get(0))); + assertTrue("the nearest center's distance must be ~0 km", closest < 1d); + } + + /** + * Method to test: {@link ESContentResourcePortlet#search(HttpServletRequest, HttpServletResponse, String, String, boolean, String, boolean)} + * Given scenario: a relevance-only query (no {@code sort} clause). Elasticsearch does not emit a + * per-hit {@code sort} array for relevance-scored hits. + * Expected result: HTTP 200 and hits carry no {@code sort} key — the fix for #36581 must not + * add an empty/spurious {@code sort} to unsorted queries. + */ + @Test + public void test_search_relevanceOnlyQuery_omitsPerHitSort() throws Exception { + final ContentType contentType = createContentTypeWithPublishedContent(); + + final String jsonQuery = "{\n" + + " \"query\": { \"bool\": { \"must\": { \"term\": { \"contenttype\": { \"value\": \"" + + contentType.variable().toLowerCase() + "\" } } } } }\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); + for (int i = 0; i < hits.length(); i++) { + assertTrue("a relevance-only (unsorted) hit must not carry a 'sort' key", + !hits.getJSONObject(i).has("sort")); + } + } + + /** + * Creates a content type with a {@code latlong} text field (dynamically mapped to a + * {@code geo_point} by the {@code *latlong} template) and publishes three centers at increasing + * distances from the reference point (42.4608, -83.1215). + */ + private ContentType createGeoContentTypeWithCenters() throws Exception { + final List fields = new ArrayList<>(); + fields.add(new FieldDataGen().name("Title").velocityVarName("title").next()); + fields.add(new FieldDataGen().name("Latlong").velocityVarName("latlong").next()); + final ContentType contentType = new ContentTypeDataGen().fields(fields).nextPersisted(); + + final String[][] centers = { + {"Center-0km", "42.4608,-83.1215"}, + {"Center-12km", "42.55,-83.20"}, + {"Center-35km", "42.70,-83.40"} + }; + for (final String[] center : centers) { + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setProperty("title", center[0]) + .setProperty("latlong", center[1]) + .nextPersisted(); + ContentletDataGen.publish(contentlet); + APILocator.getContentletAPI().isInodeIndexed(contentlet.getInode(), true); + } + return contentType; + } + private ContentType createContentTypeWithPublishedContent() throws Exception { final long now = System.currentTimeMillis(); final ContentType contentType = new ContentTypeDataGen()