fix(viewtools): keep aggregations in $json.generate() reflection JSON (#36435) - #36512
Conversation
…#36435) Templates that round-trip a raw search response through $json.generate($rawResults.response) and then navigate the resulting JSONObject (e.g. $results.aggregations.<name>.buckets) silently got no aggregations after the ES→OpenSearch migration: the customer's sitemap #foreach iterated zero times with no error or log line. Root cause: JSONTool.generate(Object) is new com.dotmarketing.util.json. JSONObject(bean) — a reflection serializer that only reads getX()/isX() accessors AND honours Jackson's @JsonIgnore. ContentSearchResponse carried @JsonIgnore on getAggregations() (to keep the neutral Jackson wire single-sourced from aggregationTree), so the vendored JSONObject also skipped it and the aggregations vanished from the generated JSON. Fix: move the Jackson suppression from a method-level @JsonIgnore on getAggregations() to a class-level @JsonIgnoreProperties("aggregations"). Jackson honours the class-level annotation (wire shape unchanged — the tree is still emitted only as aggregationTree), but the vendored JSONObject does NOT read it, so getAggregations() is again visible to the $json.generate() reflection path. Verified: the neutral Jackson JSON gains no duplicate 'aggregations' key. Tests: - AggregationDomainTest (unit): drives the exact new JSONObject(response) reflection path and asserts aggregations survive down to bucket key/docCount; the Jackson-shape test now also asserts no duplicate key. - ContentSearchToolTest (integration): drives the customer's $json.generate($rawResults.response)-then-navigate idiom through the real Velocity engine — the path #36026/#36027 did not cover. Note: the legacy .get("asMap") hop only worked because the pre-migration object was an ES Aggregations (getAsMap()); the neutral tree is flatter, so the correct navigation is aggregations.<name>.buckets directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @fabrizzio-dotCMS's task in 5m 43s —— View job Rollback-Safety Analysis Complete
Verdict: Unsafe to rollback — Category H-8 (VTL Viewtool Contract Change), Risk: 🟠 HIGH
Full details posted as a separate PR comment. Label |
…#36435) Audit of the ContentSearchResponse object graph found two more Velocity back-compat aliases with the exact same latent regression as getAggregations(): getTookInMillis() and getSuggest() also carried a method-level @JsonIgnore, so they were silently dropped from $json.generate($rawResults.response) (the vendored JSONObject bean constructor honours @JsonIgnore). Pre-migration the raw ES SearchResponse exposed both via getters, so templates round-tripping timing/suggestions through $json.generate() regressed the same way aggregations did — just lower blast radius (no ticket yet). Extend the class-level @JsonIgnoreProperties to {"aggregations", "tookInMillis", "suggest"} and drop the method-level @JsonIgnore from getTookInMillis()/getSuggest() (and the now-redundant @JsonIgnore on the suggest record component). Jackson still suppresses all three on the neutral wire (single-sourced as aggregationTree / tookMillis / wire-omitted), but the vendored JSONObject — which does not read the class-level annotation — exposes them to the $json.generate() path again. Rest of the graph audited clean: SearchHits, SearchHit, Aggregation, AggregationBucket, TotalHits and ContentSearchResults carry no @JsonIgnore getters, so no other attribute is affected. Tests: AggregationDomainTest now asserts tookInMillis and suggest also survive the new JSONObject(response) reflection hop, and the Jackson neutral-shape test asserts none of the three leak a duplicate wire key (tookMillis stays, tookInMillis/suggest/aggregations absent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pull Request Unsafe to Rollback!!!
|
|
Thanks — the H-8 flag is understood and addressed as a documentation/ops concern, not a code change, since (as the bot notes) restoring the Actions taken:
No safer code alternative exists without abandoning the fix itself, so proceeding with the documented rollback caveat. |
|
Tick the box to add this pull request to the merge queue (same as
|
Summary
The problem: When a VTL template does
$json.generate($rawResults.response)to turn search results into JSON and then reads the aggregations, the JSON came out without the aggregations. The#foreachprinted nothing — no error, no log. A customer hit this because theirsitemap.vtlrendered only the base URL (Freshdesk 38225).The cause: During the ES→OpenSearch migration,
getAggregations()was annotated@JsonIgnore(so Jackson wouldn't duplicate the tree in the API JSON). But the serializer behind$json.generate()also honours that annotation → it skipped the aggregations → they vanished from the generated JSON.The fix: Move that suppression from a method-level
@JsonIgnoreto a class-level@JsonIgnoreProperties. Jackson still honours it → the API JSON is unchanged. The$json.generate()serializer does not honour it → the aggregations show up again. Extended totookInMillisandsuggest, which shared the same latent bug.The breaking change: The legacy
.get("asMap")hop no longer works (it was an Elasticsearch artifact). The correct navigation is now direct:aggregations.<name>.buckets.Verification: unit tests 14/14 ✅, end-to-end integration 10/10 ✅ (real Velocity engine + Elasticsearch).
Problem
Templates that round-trip a raw search response through
$json.generate($rawResults.response)and then navigate the resultingJSONObject(e.g.$results.aggregations.<name>.buckets) silently got no aggregations after the ES→OpenSearch migration. A customer'ssitemap.vtlrendered only the base URL — every aggregation-driven loop (folders, pages, urlmaps) iterated zero times, with no exception and no log line (Freshdesk 38225).This is a distinct path from #36026/#36027, which restored direct Velocity object navigation (
$rawResults.aggregations...). This bug shows up only when the template serializes the response through$json.generate()first.Root cause
JSONTool.generate(Object)is literallynew com.dotmarketing.util.json.JSONObject(bean)— a reflection/bean serializer that:getX()/isX()accessors, and@JsonIgnore(JSONObject.isIgnorable()).ContentSearchResponse.getAggregations()carried a method-level@JsonIgnore(added so the neutral Jackson wire is single-sourced fromaggregationTreeand not double-emitted). Because the vendoredJSONObjectalso honours@JsonIgnore, it skipped the getter too — so the aggregations vanished from the generated JSON and the customer's#foreachiterated nothing.An audit of the object graph found two more Velocity back-compat aliases with the exact same latent regression:
getTookInMillis()andgetSuggest()also carried a method-level@JsonIgnore. Pre-migration the raw ESSearchResponseexposed both via getters, so$json.generate()templates reading timing/suggestions regressed the same way (lower blast radius — no ticket yet).Fix
Move the Jackson suppression from method-level
@JsonIgnoreon the getters to a class-level@JsonIgnoreProperties({"aggregations", "tookInMillis", "suggest"}):@JsonIgnore?@JsonIgnoreProperties?aggregationTree, timing=tookMillis, suggest omitted)JSONObject($json.generate())getX()aliases are visible again → data restoredNet:
$json.generate()templates work again, and the neutral Jackson JSON gains no duplicateaggregations/tookInMillis/suggestkey.Rest of the graph audited clean:
SearchHits,SearchHit,Aggregation,AggregationBucket,TotalHitsandContentSearchResultscarry no@JsonIgnoregetters, so no other attribute is affected.Test plan
Automated
AggregationDomainTest(unit, no search engine): drives the exactnew JSONObject(response)reflection path and assertsaggregations(down to each bucket'skey/docCount),tookInMillisandsuggestall survive; the Jackson-shape test asserts none of the three leak a duplicate wire key (tookMillis/aggregationTreestay;tookInMillis/suggest/aggregationsabsent). 14/14 green. Verified failing on the pre-fix source (1 failure) and passing with the fix.ContentSearchToolTest(integration): drives the customer's$json.generate($rawResults.response)-then-navigate idiom through the real dotCMS Velocity engine — the path Aggregation return-type change breaks existing VTL templates accessing $results.aggregations #36026/fix(viewtools): restore aggregation tree for VTL $estool.search (#36026) #36027 did not cover.Manual (QA)
TC1 — aggregations (the reported bug):
RAW JSONcontains an"aggregations"block, and the loop prints onekey: … docCount: …line per bucket.RAW JSONhas noaggregationsblock and the loop prints nothing.TC2 — tookInMillis & suggest (the two siblings fixed in the same PR):
RAW JSONcontains both a"tookInMillis"number and a"suggest"key (an object,{}when the query has no suggester);tookInMillis:renders a number andsuggest key present:renders (not blank).RAW JSONhas neithertookInMillisnorsuggest; both output lines render blank.suggestpopulated) add a suggester to the query, e.g."suggest":{"my-suggestion":{"text":"lorem","term":{"field":"title"}}}, and confirm thesuggestblock carries the suggester's entries.QA Note
.get("asMap")hop (e.g.$results.aggregations.get("asMap").folders.buckets) only ever worked because the pre-migration object was an ESAggregations(which hadgetAsMap()). The neutral tree is flatter, so the correct navigation is nowaggregations.<name>.bucketsdirectly — drop theasMaphop. Worth a docs/KB note for customers using thejson.generate()-then-navigate idiom./api/es/rawand/api/es/searchneutral JSON shape is unchanged (verified by the Jackson no-duplicate assertions).Closes #36435.
🤖 Generated with Claude Code
Rollback safety (H-8 — reviewed)
The rollback-safety bot flags this as a VTL viewtool contract change (H-8, HIGH): the PR restores the
$json.generate($rawResults.response).aggregations(and.tookInMillis/.suggest) contract, so rolling back to N-1 would silently re-break any template that relies on it. This is inherent to the forward-fix and is expected:#foreach, no error/log)./api/es/raw&/api/es/searchJSON is unchanged (pinned by the Jackson no-duplicate assertions).Release-notes action for ops: rolling back this release silently reintroduces the
$json.generate(...).aggregationsregression (Freshdesk 38225) for any template adopting the restored idiom this cycle. Weigh that in any rollback decision.