Repository navigation
fix(api): validate composite aggregation definitions like ES - #1854
Merged
Merged
Conversation
Motivation: Six `aggregations/composite.yml` cases sit in `catch-lenient.txt` (xerj-org#1583): Elasticsearch refuses the request with a 400, XERJ answered 200. `run_composite` returned an empty bucket list for missing or empty `sources`, let a later source silently overwrite an earlier one of the same name, ignored a `missing_order` that can never apply, accepted a page `size` far above `search.max_buckets`, and ran under any parent. Root cause: Composite definitions were never validated; `run_composite` and the columnar `exec_composite` treat every malformed shape as "nothing to return" or pick one interpretation. Fix: `find_invalid_composite_agg` (es_compat.rs), a request-time walk chained after `find_invalid_agg_order_path` (xerj-org#1283) at its four call sites, with ES 8.13's messages: - `sources` absent -> `Required [sources]`; empty -> `Composite [sources] cannot be null or empty`; - duplicate source names -> `Composite source names must be unique, found duplicates: [..]`; - `missing_order` other than `default` without `missing_bucket: true` -> `missingOrder can only be set if missingBucket is true`; - `size` above `search.max_buckets` (`aggs::max_buckets()`, the node-wide value) -> ES's too-many-buckets message; - a parent other than nested / reverse_nested / filter (through any chain of them) -> `[composite] aggregation cannot be used with a parent aggregation of type: [TermsAggregatorFactory]`. Evidence: - Full ES-YAML suite against a node built from this commit: 0 failed, and exactly these 6 catch-lenient entries reported stale; with them deleted and enforced: `1402 passed · 0 failed · 10 skipped`, catch-lenient 151 -> 145. - Unit tests pin the valid shapes composite.yml relies on (nested / filter / reverse_nested parents, `missing_bucket: true` with an order, sub-aggregations) and each refusal message.
|
Thanks for this contribution — we'd like to merge it. Before we can, we need a signed Contributor License Agreement from: @amartya1523 It is a one-time thing per contributor, not per pull request. Read CLA.md, then open a small pull request adding your GitHub username to Once it is merged, comment |
Owner
|
@cla-bot check |
|
Re-checking the CLA status for every commit author on this pull request… |
Owner
|
Xerj Merger made the fix: composite aggregation definitions are now validated the way Elasticsearch does it. Missing or duplicate sources, an invalid composite body, a bad array size, and missing_order without missing_bucket are refused with ES-shaped 400s on search, msearch, search templates and msearch templates. That removes 6 lines from catch-lenient.txt. Squash-merged on head b7e0642 after Build + Test passed. Thanks @amartya1523! |
5 of 6 tasks
amartya1523
added a commit
to amartya1523/xerj
that referenced
this pull request
Oct 20, 2026
Motivation: 20 `catch-lenient.txt` lines (xerj-org#1583) are aggregation requests that Elasticsearch 8.13 refuses with a 400 at parse time and XERJ answered 200, ignoring the bad value or returning an empty or defaulted result: a negative cardinality `precision_threshold` or unknown `execution_hint`, `filters` with no filters, a non-object `meta`, an `ip_prefix` `prefix_length` out of range, a non-positive `median_absolute_deviation` `compression`, unknown tdigest execution hints, `percentile_ranks` without usable `values`, a `terms` with neither `field` nor `script`, `random_sampler` misuse, and `time_series` under `global` or next to an aggregation incompatible with time-series execution. Root cause: None of these parameters or placements were validated; the executors treat each malformed shape as "use a default" or "nothing to return". Fix: `find_invalid_agg_params` (es_compat.rs), a request-time walk chained after `find_invalid_agg_order_path` (xerj-org#1283) at its four call sites (`_search`, `_msearch`, `_search/template`, `_msearch/template`), with ES 8.13's messages. Only checks that need no mapping are made: e.g. `avg` on a keyword field (which needs the field type) is left alone. Evidence: - Full ES-YAML suite against a node built from this commit: 0 failed, and exactly 20 catch-lenient entries reported stale; with them deleted and enforced: `1404 passed · 0 failed · 10 skipped` on current main (after xerj-org#1854 and xerj-org#1870), catch-lenient 128 -> 108. - Unit tests pin the valid shapes the suite relies on (the five cardinality hints, `default`/`high_accuracy`, random_sampler 0.1 and 1.0, time_series with date_histogram, terms with only a script, an incompatible type in a request without time_series) and each message.
xerj-org
pushed a commit
that referenced
this pull request
Oct 20, 2026
#1861) Motivation: 20 `catch-lenient.txt` lines (#1583) are aggregation requests that Elasticsearch 8.13 refuses with a 400 at parse time and XERJ answered 200, ignoring the bad value or returning an empty or defaulted result: a negative cardinality `precision_threshold` or unknown `execution_hint`, `filters` with no filters, a non-object `meta`, an `ip_prefix` `prefix_length` out of range, a non-positive `median_absolute_deviation` `compression`, unknown tdigest execution hints, `percentile_ranks` without usable `values`, a `terms` with neither `field` nor `script`, `random_sampler` misuse, and `time_series` under `global` or next to an aggregation incompatible with time-series execution. Root cause: None of these parameters or placements were validated; the executors treat each malformed shape as "use a default" or "nothing to return". Fix: `find_invalid_agg_params` (es_compat.rs), a request-time walk chained after `find_invalid_agg_order_path` (#1283) at its four call sites (`_search`, `_msearch`, `_search/template`, `_msearch/template`), with ES 8.13's messages. Only checks that need no mapping are made: e.g. `avg` on a keyword field (which needs the field type) is left alone. Evidence: - Full ES-YAML suite against a node built from this commit: 0 failed, and exactly 20 catch-lenient entries reported stale; with them deleted and enforced: `1404 passed · 0 failed · 10 skipped` on current main (after #1854 and #1870), catch-lenient 128 -> 108. - Unit tests pin the valid shapes the suite relies on (the five cardinality hints, `default`/`high_accuracy`, random_sampler 0.1 and 1.0, time_series with date_histogram, terms with only a script, an incompatible type in a request without time_series) and each message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI-assisted change, run and reviewed by @amartya1523.
Burns down 6 lines of the
catch-lenient.txtratchet from #1583 (151 → 145): everyaggregations/composite.ymlcase except the two date-format ones. Independent of #1824: same pattern, different aggregation, and it touches the same call sites, so whichever merges second needs a trivial rebase.What this changes, and why
Elasticsearch 8.13 refuses a malformed
compositeaggregation at parse time with a 400. XERJ answered 200 instead:sources→run_compositereturned an empty bucket list;missing_orderwithoutmissing_bucket: true→ ignored, though it can never apply;size: 1000000000→ accepted (ES caps it atsearch.max_buckets);terms→ ran (ES allows only no parent, ornested/reverse_nested/filterchains).Fix.
find_invalid_composite_agg(es_compat.rs) is a request-time walk chained afterfind_invalid_agg_order_path(#1283) at its four call sites (_search,_msearch,_search/template,_msearch/template). It uses ES 8.13's messages:sourcesabsentRequired [sources]sourcesempty / not a listComposite [sources] cannot be null or emptyComposite source names must be unique, found duplicates: [keyword]missing_order≠defaultwithoutmissing_bucket: truemissingOrder can only be set if missingBucket is truesize>search.max_buckets(aggs::max_buckets(), the node-wide value)Trying to create too many buckets. Must be less than or equal to: [65536] but was [1000000000]. …[composite] aggregation cannot be used with a parent aggregation of type: [TermsAggregatorFactory]Evidence
Full ES-YAML suite against a node built from this commit. The runner reported exactly these 6 entries as stale, with 0 failed:
With those deleted and enforced:
The passing
composite.ymlcases that nest composite undernested, under anested > filter > nestedchain, and underfilter, as well as themissing_bucket: true+missing_ordercase, all still pass.Unit tests (
es_compat::composite_agg_validation_tests):Checks
cargo fmt --all -- --check(exit 0)cargo build --release -p xerj-server -p es-yaml-runnercargo test --release -p xerj-api --lib composite_agg_validation(2 passed)[limits] disk_flood_stage_percent = 0because this machine's disk is above the 95% flood stage.Not run: the full
cargo test -p xerj-api(disk) and clippy.Left in the list:
Composite aggregation with invalid format/with lossy format. ES raises those while rendering bucket keys (a format whose output cannot be parsed back), so they need bucket data, not a request-time check.Provenance
[XxxAggregatorFactory]name for parents other thanterms(onlyterms→TermsAggregatorFactoryis exercised by the YAML). The name is derived by CamelCasing the aggregation type. ES's real class could differ for some types (e.g.date_histogram), which would make the message text inexact but still a 400.