fix(viewtools): keep aggregations in $json.generate() reflection JSON (#36435)

dotCMS/src/main/java/com/dotcms/content/index/domain/ContentSearchResponse.java

@@ -39,12 +39,25 @@
  *                        sub-aggregations and {@code top_hits} preserved); Velocity resolves
  *                        {@code $results.aggregations.<name>} through this map
  */
+// The Velocity-only aliases {@code aggregations}, {@code tookInMillis} and {@code suggest} are
+// suppressed for Jackson at the CLASS level (not with method-level {@code @JsonIgnore} on the
+// getters) on purpose. Each is redundant on the neutral Jackson wire (the tree is serialized as
+// {@code aggregationTree}, timing as {@code tookMillis}, and suggestions are wire-omitted), but a
+// method-level {@code @JsonIgnore} would ALSO hide them from the reflection-based
+// {@code com.dotmarketing.util.json.JSONObject} bean constructor that {@code JSONTool.generate(Object)}
+// uses — it honours {@code @JsonIgnore}. That reflection path is exactly what
+// {@code $json.generate($response)} templates walk, so a method-level ignore silently drops the data
+// there (issue #36435 — originally only {@code aggregations} was reported, {@code tookInMillis} and
+// {@code suggest} shared the same latent regression). A class-level {@code @JsonIgnoreProperties} is
+// read by Jackson but NOT by the vendored JSONObject, so it keeps the wire shape unchanged while
+// leaving the {@code getX()} aliases visible to Velocity's json tool.
+@com.fasterxml.jackson.annotation.JsonIgnoreProperties({"aggregations", "tookInMillis", "suggest"})
 public record ContentSearchResponse(
         SearchHits hits,
         @Nullable String scrollId,
         long tookMillis,
         Map<String, Aggregation> aggregationTree,
-        @com.fasterxml.jackson.annotation.JsonIgnore Map<String, Object> suggest) {
+        Map<String, Object> suggest) {
 
     /**
      * Canonical constructor. {@code aggregationTree} and {@code suggest} default to an empty map when
@@ -72,8 +85,10 @@ public Map<String, List<AggregationBucket>> aggregations() {
     // that access WITHOUT changing the JSON wire shape:
     //  - getHits()/getScrollId() return the same values as the record components, so Jackson merges
     //    them into the existing "hits"/"scrollId" fields (no new key, no duplicate).
-    //  - getTookInMillis()/getAggregations() carry @JsonIgnore so they remain Velocity-only and
-    //    never add a field to the neutral JSON.
+    //  - getTookInMillis()/getAggregations()/getSuggest() are Velocity-only and must NOT add a field
+    //    to the neutral JSON; that suppression lives in the class-level @JsonIgnoreProperties above
+    //    (Jackson-only) rather than a method-level @JsonIgnore, so they stay visible to the
+    //    reflection-based $json.generate() path (issue #36435).
 
     /** Velocity/back-compat alias for {@link #hits()}; serializes as the same {@code hits} field. */
     public SearchHits getHits() {
@@ -87,31 +102,39 @@ public String getScrollId() {
 
     /**
      * Velocity/back-compat alias mirroring Elasticsearch {@code SearchResponse.getTookInMillis()}.
-     * {@code @JsonIgnore} keeps the neutral JSON unchanged (timing is serialized as {@code tookMillis}).
+     * Deliberately NOT annotated {@code @JsonIgnore}: it must stay visible to the reflection-based
+     * {@code $json.generate($response)} path (issue #36435). The neutral Jackson JSON is kept
+     * unchanged (timing is serialized only as {@code tookMillis}) by the class-level
+     * {@code @JsonIgnoreProperties("tookInMillis")}.
      */
-    @com.fasterxml.jackson.annotation.JsonIgnore
     public long getTookInMillis() {
         return tookMillis;
     }
 
     /**
      * Velocity/back-compat alias exposing the aggregation tree as {@code $r.aggregations}, matching
      * {@code ContentSearchResults#getAggregations()} so {@code $dotcontent.raw(...)} and
-     * {@code $dotcontent.search(...)} templates walk aggregations the same way. {@code @JsonIgnore}
-     * keeps the neutral JSON unchanged (the tree is serialized as {@code aggregationTree}).
+     * {@code $dotcontent.search(...)} templates walk aggregations the same way.
+     *
+     * <p>Deliberately NOT annotated {@code @JsonIgnore}: this accessor must stay visible to the
+     * reflection-based {@code com.dotmarketing.util.json.JSONObject} bean constructor behind
+     * {@code JSONTool.generate(Object)}, so {@code $json.generate($response).aggregations...} templates
+     * keep working (issue #36435). The neutral Jackson JSON is kept unchanged (the tree is serialized
+     * only as {@code aggregationTree}) by the class-level {@code @JsonIgnoreProperties("aggregations")}
+     * instead — Jackson honours it, the vendored JSONObject does not.</p>
      */
-    @com.fasterxml.jackson.annotation.JsonIgnore
     public Map<String, Aggregation> getAggregations() {
         return aggregationTree;
     }
 
     /**
      * Velocity/back-compat alias for {@link #suggest()}, exposing search suggestions as
-     * {@code $r.suggest}. Kept {@code @JsonIgnore} (like the {@code suggest} component itself) so the
-     * neutral JSON shape of {@code /api/es/raw} is unchanged; the ES-wire {@code suggest} block is
-     * emitted only by the {@code /api/es/search} legacy adapter.
+     * {@code $r.suggest}. Deliberately NOT annotated {@code @JsonIgnore}: it must stay visible to the
+     * reflection-based {@code $json.generate($response)} path (issue #36435). The neutral JSON shape of
+     * {@code /api/es/raw} is unchanged (suggestions are wire-omitted) via the class-level
+     * {@code @JsonIgnoreProperties("suggest")}; the ES-wire {@code suggest} block is emitted only by the
+     * {@code /api/es/search} legacy adapter.
      */
-    @com.fasterxml.jackson.annotation.JsonIgnore
     public Map<String, Object> getSuggest() {
         return suggest;
     }

dotCMS/src/test/java/com/dotcms/content/index/domain/AggregationDomainTest.java

@@ -10,6 +10,8 @@
 import static org.mockito.Mockito.when;
 
 import com.dotcms.rest.api.v1.DotObjectMapperProvider;
+import com.dotmarketing.util.json.JSONArray;
+import com.dotmarketing.util.json.JSONObject;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.datatype.guava.GuavaModule;
@@ -178,6 +180,7 @@ public void contentSearchResponse_jacksonSerializesNeutralShape() throws Excepti
         final ContentSearchResponse response = ContentSearchResponse.builder()
                 .hits(hits).tookMillis(42L)
                 .aggregationTree(Map.of("content_types", terms))
+                .suggest(Map.of("titleSuggest", List.of()))
                 .build();
 
         final JsonNode root = mapper.readTree(mapper.writeValueAsString(response));
@@ -186,6 +189,18 @@ public void contentSearchResponse_jacksonSerializesNeutralShape() throws Excepti
         assertTrue("aggregationTree must be present", root.has("aggregationTree"));
         assertTrue("declared aggregation must survive serialization",
                 root.path("aggregationTree").has("content_types"));
+        // The Velocity-only aliases getAggregations()/getTookInMillis()/getSuggest() must NOT leak into
+        // the neutral Jackson JSON — the tree is single-sourced as aggregationTree, timing as tookMillis,
+        // and suggestions are wire-omitted. The class-level @JsonIgnoreProperties (not method-level
+        // @JsonIgnore) is what suppresses them here, so they stay visible to the reflection-based
+        // $json.generate() path exercised in
+        // jsonGenerate_ofResponse_preservesAggregationsForVelocityNavigation (issue #36435).
+        assertFalse("getAggregations() must not double-emit an 'aggregations' key in the neutral JSON",
+                root.has("aggregations"));
+        assertFalse("getTookInMillis() must not add a 'tookInMillis' key (timing stays as tookMillis)",
+                root.has("tookInMillis"));
+        assertFalse("getSuggest() must not add a 'suggest' key to the neutral JSON",
+                root.has("suggest"));
 
         // hits must be a nested object, not a bare array (SearchHits implements Iterable).
         final JsonNode hitsNode = root.get("hits");
@@ -373,6 +388,74 @@ public void aggregation_metadata_defaultsToEmptyWhenUnset() {
         assertTrue("metadata defaults to empty when unset", agg.getMetadata().isEmpty());
     }
 
+    // =========================================================================
+    // $json.generate($rawResults.response) reflection navigation — issue #36435
+    // =========================================================================
+    //
+    // Some templates do NOT navigate the Velocity object directly (the path #36026/#36027
+    // restored); they first round-trip the raw response through JSONTool.generate(Object), i.e.
+    // `#set($results = $json.generate($rawResults.response))`, and then walk the resulting
+    // JSONObject: `$results.aggregations.<name>.buckets`. JSONTool.generate(Object) is literally
+    // `new com.dotmarketing.util.json.JSONObject(bean)` — a reflection/bean constructor that only
+    // picks up public zero-arg `getX()`/`isX()` accessors and (crucially) SKIPS any accessor
+    // annotated with Jackson's @JsonIgnore. This test exercises that exact reflection path against
+    // the neutral ContentSearchResponse so the regression is pinned as a fast unit test.
+
+    /**
+     * Reproduction / regression for <a href="https://github.com/dotCMS/core/issues/36435">#36435</a>.
+     *
+     * <p>A customer's {@code sitemap.vtl} does {@code #set($results = $json.generate($rawResults.response))}
+     * and then {@code #foreach($folder in $results.aggregations.<name>.buckets)}. Before the ES→OS
+     * migration the raw response was an ES {@code SearchResponse} bean and its {@code getAggregations()}
+     * (and {@code getTookInMillis()}, {@code getSuggest()}) reflected into the JSON, so those survived
+     * the {@code $json.generate()} hop. After the migration the raw response is
+     * {@link ContentSearchResponse}; those Velocity aliases carried a method-level {@code @JsonIgnore}
+     * (to keep the neutral Jackson wire single-sourced), and the dotCMS-vendored {@link JSONObject} bean
+     * constructor honours {@code @JsonIgnore} too — so they vanished from the generated JSON and the
+     * {@code #foreach} silently iterated zero times.</p>
+     *
+     * <p>Contract this pins: running the response through the same reflection constructor
+     * {@code JSONTool.generate(Object)} uses must expose the Velocity-alias family — the aggregations
+     * navigable as {@code aggregations.<name>.buckets} down to each bucket's {@code key}/{@code docCount},
+     * plus {@code tookInMillis} and {@code suggest}. All three moved from a method-level
+     * {@code @JsonIgnore} to a class-level {@code @JsonIgnoreProperties} that only Jackson honours.</p>
+     */
+    @Test
+    public void jsonGenerate_ofResponse_preservesAggregationsForVelocityNavigation() {
+        final AggregationBucket about = AggregationBucket.builder().key("/about-us/").docCount(12).build();
+        final AggregationBucket products = AggregationBucket.builder().key("/products/").docCount(7).build();
+        final Aggregation folders = Aggregation.builder()
+                .name("folders").type("sterms").buckets(List.of(about, products)).build();
+
+        final ContentSearchResponse response = ContentSearchResponse.builder()
+                .hits(SearchHits.empty()).tookMillis(5L)
+                .aggregationTree(Map.of("folders", folders))
+                .suggest(Map.of("titleSuggest", List.of()))
+                .build();
+
+        // Exactly what JSONTool.generate(Object o) does: new JSONObject(o).
+        final JSONObject json = new JSONObject(response);
+
+        assertTrue("aggregations must survive the $json.generate() reflection hop (issue #36435)",
+                json.has("aggregations"));
+
+        final JSONObject aggs = json.getJSONObject("aggregations");
+        assertTrue("the declared 'folders' aggregation must be present after JSON generation",
+                aggs.has("folders"));
+
+        final JSONArray buckets = aggs.getJSONObject("folders").getJSONArray("buckets");
+        assertEquals("both folder buckets must survive", 2, buckets.length());
+        assertEquals("/about-us/", buckets.getJSONObject(0).getString("key"));
+        assertEquals(12L, buckets.getJSONObject(0).getLong("docCount"));
+        assertEquals("/products/", buckets.getJSONObject(1).getString("key"));
+
+        // The sibling Velocity aliases that shared the same latent regression (issue #36435) must also
+        // survive the reflection hop: getTookInMillis() and getSuggest().
+        assertEquals("tookInMillis must survive the $json.generate() reflection hop", 5L,
+                json.getLong("tookInMillis"));
+        assertTrue("suggest must survive the $json.generate() reflection hop", json.has("suggest"));
+    }
+
     /** An empty (but non-null) Elasticsearch aggregation set whose buckets carry no sub-aggs. */
     private static Aggregations emptyEsAggregations() {
         final Aggregations aggs = mock(Aggregations.class);

dotcms-integration/src/test/java/com/dotcms/rendering/velocity/viewtools/ContentSearchToolTest.java

@@ -326,6 +326,50 @@ public class ContentSearchToolTest extends IntegrationTestBase {
             #end
             """;
 
+    /**
+     * The customer's <b>other</b> idiom (issue #36435): instead of navigating the Velocity object
+     * directly (the path {@link #verbatimCustomerVtl_rendersAfterFix()} covers), the template first
+     * round-trips the raw response through {@code $json.generate($rawResults.response)} and then walks
+     * the resulting {@code JSONObject}: {@code $results.aggregations.<name>.buckets}.
+     *
+     * <p>{@code $rawResults.response} is the {@link ContentSearchResponse} exposed by
+     * {@link ContentSearchResults#getResponse()}. {@code $json.generate(Object)} is literally
+     * {@code new com.dotmarketing.util.json.JSONObject(response)} — a reflection/bean serializer that
+     * only sees {@code getX()} accessors and honours {@code @JsonIgnore}. Before the fix,
+     * {@code getAggregations()} carried {@code @JsonIgnore}, so the aggregations vanished from the
+     * generated JSON and this loop iterated zero times (silent empty sitemap). The fix moves that
+     * suppression to a class-level {@code @JsonIgnoreProperties} that only Jackson honours, so the
+     * reflection serializer keeps {@code aggregations} — this loop must now emit buckets.</p>
+     *
+     * <p>Note: the legacy {@code .get("asMap")} hop the customer used only ever existed because the
+     * pre-migration object was an ES {@code Aggregations} (which had {@code getAsMap()}); the neutral
+     * tree is flatter, so the correct navigation is {@code aggregations.<name>.buckets} directly.</p>
+     */
+    private static final String JSON_GENERATE_VTL = """
+            #set($esQuery = '{
+                "aggs": {
+                    "content_types": {
+                        "terms": { "field": "contentType", "size": 5 }
+                    }
+                },
+                "size": 0,
+                "query": {
+                    "bool": {
+                        "filter": [ { "term": { "live": true } } ]
+                    }
+                }
+            }')
+
+            ## The #36435 idiom: JSON-ify the raw response FIRST, then navigate the JSONObject.
+            #set($rawResults = $estool.search($esQuery))
+            #set($results = $json.generate($rawResults.response))
+            aggregations: $!{results.aggregations}
+            #foreach($group in $results.aggregations.content_types.buckets)
+              json key: $!{group.key}
+              json docCount: $!{group.docCount}
+            #end
+            """;
+
     @BeforeClass
     public static void prepare() throws Exception {
         IntegrationTestInitService.getInstance().init();
@@ -378,11 +422,17 @@ private ESContentTool liveContentTool() {
         return tool;
     }
 
-    /** Builds a Velocity context with the live {@code $estool} and a mock {@code $response} bound. */
+    /**
+     * Builds a Velocity context with the live {@code $estool}, a mock {@code $response} and the
+     * {@code $json} tool bound. {@link JSONTool} needs no real init ({@code init(Object)} is a no-op
+     * and {@code generate(Object)} is stateless), so a plain instance mirrors what the Velocity
+     * toolbox binds as {@code $json}.
+     */
     private Context velocityContext() {
         final Context ctx = new VelocityContext();
         ctx.put("estool", liveContentTool());
         ctx.put("response", mock(HttpServletResponse.class));
+        ctx.put("json", new JSONTool());
         return ctx;
     }
 
@@ -407,6 +457,25 @@ public void verbatimCustomerVtl_rendersAfterFix() throws Exception {
                 output.contains("hit id:"));
     }
 
+    /**
+     * End-to-end regression for <a href="https://github.com/dotCMS/core/issues/36435">#36435</a>:
+     * drives the customer's {@code $json.generate($rawResults.response)}-then-navigate idiom through
+     * the real dotCMS Velocity engine and asserts the aggregation buckets survive the JSON round-trip
+     * (the {@code json key:} / {@code json docCount:} loop must execute). This is the path #36026/#36027
+     * did NOT cover — those navigate the Velocity object directly; this one serializes first.
+     */
+    @Test
+    public void jsonGenerateThenNavigateVtl_emitsBuckets() throws Exception {
+        final String output = VelocityUtil.eval(JSON_GENERATE_VTL, velocityContext());
+        Logger.info(this, "\n===== $json.generate VTL output =====\n" + output + "\n================================");
+
+        assertTrue("FIX (#36435): $json.generate($rawResults.response) must keep the aggregations, so "
+                        + "the bucket loop executes and emits 'json key:' lines",
+                output.contains("json key:"));
+        assertTrue("FIX (#36435): bucket doc counts must render with real numbers after the JSON round-trip",
+                Pattern.compile("json docCount:\\s*\\d+").matcher(output).find());
+    }
+
     /**
      * The fluent neutral form also works: iterating the {@link Aggregation} directly and calling
      * {@code key()} / {@code docCount()}.