Skip to content

fix(viewtools): restore aggregation tree for VTL $estool.search (#36026)#36027

Open
fabrizzio-dotCMS wants to merge 5 commits into
mainfrom
issue-36026-vtl-aggregation-regression
Open

fix(viewtools): restore aggregation tree for VTL $estool.search (#36026)#36027
fabrizzio-dotCMS wants to merge 5 commits into
mainfrom
issue-36026-vtl-aggregation-regression

Conversation

@fabrizzio-dotCMS
Copy link
Copy Markdown
Member

@fabrizzio-dotCMS fabrizzio-dotCMS commented Jun 5, 2026

Fix for #36026 — VTL aggregation return-type regression

The Elasticsearch → OpenSearch migration made the search aggregation API vendor-neutral and, in doing so, changed the type exposed to Velocity as $results.aggregations ($estool.search(...)), silently breaking existing VTL templates (Velocity runs with runtime.references.strict=false, so broken accessors render empty instead of erroring).

  • Before: $results.aggregationsorg.elasticsearch.search.aggregations.Aggregations (object tree)
  • After (broken): $results.aggregationsMap<String, List<AggregationBucket>>, where AggregationBucket exposed only key() / docCount() and nested sub-aggregations were dropped.

Three regressions

  1. aggregations.<name>.bucketsnull (the map value was already the bucket list).
  2. bucket.getKeyAsNumber() / getDocCount() gone; the fluent key() / docCount() are not reachable as Velocity properties (no get/is prefix).
  3. Nested top_hits per bucket unrecoverable — ContentSearchResponse.from(...) mapped only first-level terms.

The fix — restore a vendor-neutral aggregation tree

A neutral tree that mirrors the legacy ES API shape, so existing templates keep working unchanged (honoring the migration's "no visible behavior change" goal).

  • New Aggregation typegetBuckets() (terms), getHits() (top_hits), Iterable over buckets. The container is a plain Map<String, Aggregation> (Velocity resolves $aggs.name via Map#get) — no separate container class.
  • AggregationBucket gains getKey() / getKeyAsString() / getKeyAsNumber() (lenient: null on non-numeric) / getDocCount() and nested getAggregations(); ES and OS factories now capture sub-aggregations.
  • SearchHit / SearchHits accessors switched to bean-style (getId(), getHits(), …) so $topHits.getHits().getHits()$hit.id resolve without alias methods.
  • ContentSearchResponse keeps the flat aggregations() Map intact (now @Value.Derived from the tree) → ContentTypeAPIImpl / WorkflowHelper and existing tests are unaffected; adds aggregationTree(). ContentSearchResults.getAggregations() (the VTL entry point) returns the tree.

The flat map is always derived from the tree, so the two views can never disagree.

Tests

AggregationDomainTest (unit) — Jackson round-trip with no duplicate id property, lenient getKeyAsNumber(), flat-map derivation excludes bucket-less aggs, nested sub-aggs reachable.

ContentSearchToolTest (integration, run against real OpenSearch):

Test Purpose
verbatimCustomerVtl_rendersAfterFix The customer template, verbatim, now renders buckets (key: / docCount:) and nested top_hits hits (hit id:).
neutralFluentVtl_emitsBuckets The fluent neutral form ($group.key() / docCount(), iterating the Aggregation).
aggregationTreePreservesTopHits API-level: aggregationTree() preserves first-level buckets and nested top_hits with its hits.
raw_flatAggregationsMapStillPopulated The flat Map used by Java callers stays populated.

✅ Unit 4/4 · ✅ Integration 4/4 against OpenSearch.

Closes #36026

🤖 Generated with Claude Code

…6026)

The ES → OS migration changed the type returned by $results.aggregations
from org.elasticsearch.search.aggregations.Aggregations to
Map<String, List<AggregationBucket>>. Existing VTL templates that walked
aggregations.<name>.buckets / bucket.getKeyAsNumber() / getDocCount() /
nested top_hits now silently produce no output.

ContentSearchToolTest renders the customer template through the dotCMS
Velocity engine and:
- verbatimCustomerVtl_reproducesRegression: locks the broken behaviour
  (data present, but .buckets is null so the bucket loop emits nothing).
- migratedVtl_emitsBuckets: documents the neutral-API migration path for
  the flat terms aggregation (key()/docCount(), no .buckets).
- raw_aggregationDataIsPresent: proves the buckets are in the response, so
  the regression is purely a VTL access/typing problem.

This is the entry point for the real fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 5, 2026

Claude finished @fabrizzio-dotCMS's task in 3m 5s —— View job


PR Review

  • Gathering context
  • Reading changed files
  • Analysis & review

The fix is structurally sound and the tests are well-written. A few issues worth addressing:


Issues

1. flatten() discriminator is fragile — getHits() == null

ContentSearchResponse.flatten() (line 89) uses aggregation.getHits() == null to decide whether to include an aggregation in the flat map. This works today because only top_hits sets getHits(). But if any future aggregation type also sets getHits() (or someone builds an Aggregation with both buckets and hits set), it will silently vanish from the flat map. The discriminant should be based on the type string, not an inferred null check.

Fix this →


2. Unknown OS aggregation types are silently dropped

Aggregation.fromSingleOS() (line 121–151) returns null for any type that isn't sterms, lterms, dterms, or top_hits. Those nulls are filtered out in fromOS() with no log. Aggregation types like date_histogram, range, avg, cardinality, etc. disappear without trace. A Logger.debug call with the skipped type name would save hours of debugging when a template stops returning results.


3. SearchHit.from(osHit) drops ES-style fields for OpenSearch hits

The ES factory calls .fields(esSearchHit.getFields()) (line 98), but the OS factory (line 110–128) never calls .fields(...). Any template using $hit.fields.someField on a top_hits result backed by OpenSearch will silently get an empty map. The OS Hit has osHit.fields() — it should be mapped.


4. @NotNull annotation inconsistency in SearchHits

SearchHits.iterator() imports org.jetbrains.annotations.NotNull (line 11), while Aggregation and AggregationBucket use javax.annotation.Nullable. The codebase standard (as seen in surrounding files) is javax.annotation. Pick one.


5. serialVersionUID on a non-Serializable class

ContentSearchResults (line 26) declares serialVersionUID = 1L but does not implement java.io.Serializable. The field is meaningless. Pre-existing, but now is a good time to clean it up.


6. Broken javadoc {@link Aggregations} in ContentSearchResponse

Line 24 of ContentSearchResponse.java references {@link Aggregations} — no such class exists. Should be Map<String, Aggregation> or just removed.


7. DoubleTermsBucket key may render in scientific notation

String.valueOf(osBucket.key()) for a double can produce "1.23E7" style strings. getKeyAsNumber() recovers the double correctly, but Velocity's $bucket.getKeyAsString() would render the scientific notation literally, which may not match what existing templates expect.


Reviewer request not yet implemented

@wezell requested converting SearchHit and SearchHits to records. The current Immutables approach works, but the reviewer's request is open. Not a blocker if agreed to defer, but needs a reply.

The ES→OS migration flattened the aggregations exposed to Velocity from an
ES Aggregations object tree to a Map<String,List<AggregationBucket>>, silently
breaking templates that walked aggregations.<name>.buckets, getKeyAsNumber()/
getDocCount(), nested getAggregations(), and the top_hits metric aggregation.

Restore a vendor-neutral aggregation tree that mirrors the legacy ES shape so
existing templates keep working unchanged:

- New Aggregation type (getBuckets for terms, getHits for top_hits, Iterable
  over buckets); the container is a plain Map<String,Aggregation> (Velocity
  resolves $aggs.name via Map#get) — no separate container class.
- AggregationBucket gains getKey/getKeyAsString/getKeyAsNumber/getDocCount and
  nested getAggregations(); ES and OS factories capture sub-aggregations.
- SearchHit/SearchHits accessors switched to bean-style (getId/getHits/...) so
  $topHits.getHits().getHits() and $hit.id resolve without alias methods.
- ContentSearchResponse keeps the flat aggregations() map intact (now derived
  from the tree) so ContentTypeAPIImpl/WorkflowHelper are unaffected; adds
  aggregationTree(). ContentSearchResults.getAggregations() returns the tree.

Verified: AggregationDomainTest (unit) and ContentSearchToolTest (integration,
against real OpenSearch) — the verbatim customer template renders buckets and
nested top_hits hits.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p body

Cosmetic readability only — pretty-print the aggregation JSON inside the
Velocity string literal and indent the #foreach body. Re-verified against real
OpenSearch (neutralFluentVtl_emitsBuckets passes).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cosmetic readability only — pretty-print the JSON query inside the verbatim
customer template's Velocity string literal (same query, unchanged semantics).
Re-verified against real OpenSearch (verbatimCustomerVtl_rendersAfterFix passes:
buckets + nested top_hits hits render).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fabrizzio-dotCMS fabrizzio-dotCMS changed the title test(viewtools): reproduce VTL aggregation return-type regression (#36026) fix(viewtools): restore aggregation tree for VTL $estool.search (#36026) Jun 5, 2026
@fabrizzio-dotCMS fabrizzio-dotCMS marked this pull request as ready for review June 5, 2026 22:17
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make SearchHit a record, not immutable :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a record too? Let me know if that is too much. work but it would be good to start flexing this muscle.

The flat ContentSearchResponse.aggregations() map dropped terms aggregations
whose bucket list was empty (the flatten() guard filtered on
!getBuckets().isEmpty()), breaking the contract that a declared terms
aggregation key is present even with no matching documents. This regressed
OSSearchAPIImplIntegrationTest.test_searchRaw_withTermsAgg_shouldReturnAggregationKey
in the OpenSearch Upgrade Suite.

Discriminate bucket vs metric aggregations by getHits()==null instead: terms
aggregations (hits null) are always included — even with empty buckets — while
top_hits (hits non-null) is skipped. Adds a unit regression guard.

Verified: AggregationDomainTest 5/5 (unit) and OSSearchAPIImplIntegrationTest
11/11 (integration, real OpenSearch upgrade suite).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Aggregation return-type change breaks existing VTL templates accessing $results.aggregations

2 participants