Repository navigation
fix(api): reject invalid aggregation parameters and placements like ES - #1861
Conversation
|
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 |
|
@cla-bot check |
|
Re-checking the CLA status for every commit author on this pull request… |
|
Hi @amartya1523, #1854 (composite aggregation validation) and #1870 just landed on main, and this branch now conflicts with main in |
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.
|
Rebased onto current
Re-verified on the new head: ( |
87b93c2 to
601f4b8
Compare
|
Xerj Merger made the fix: reviewed the full diff. |
Resolve conflicts with xerj-org#1861 (find_invalid_agg_params): keep both request-time validators, chain find_invalid_pipeline_agg after find_invalid_agg_params at all four call sites, and drop this PR's 25 catch-lenient lines from main's list.
…#2913) * fix(api): reject avg/sum/min/max/stats on a non-numeric field like ES Motivation: `{"aggs": {"a": {"avg": {"field": "string_field"}}}}` on a keyword field answered 200 with `"value": null` (`sum` answered `-0.0`), so a metric pointed at the wrong field looked like an empty index. ES 8.13 fails every shard with `Field [string_field] of type [keyword] is not supported for aggregation [avg]` (ValuesSourceRegistry#getAggregator) and answers 400. Four catch-lenient lines are this case (`{avg,max,min,sum}_metric.yml · Aggregating wrong datatype test`). Root cause: `find_invalid_agg_params` (#1861) deliberately made only mapping-free checks; nothing on the search path compared a metric's `field` with the mapped type. Fix: `find_unsupported_metric_agg_field`, called in `search_impl` after the field-alias rewrite, walks the aggregation tree (sub-aggregations included) and resolves each `avg`/`sum`/`min`/`max`/`stats`/ `extended_stats` field through the participating indices' mappings, following object `properties` and multi-field `fields` (`name.raw`). It refuses only types ES never accepts for these aggregations (keyword, constant_keyword, wildcard, ip, geo_point, version), and only when every target index maps the field that way: an unmapped field, or one that is numeric in at least one target, still answers 200 as ES does (a partial-shard success there). The error uses the existing `search_shard_validation_error` envelope (search_phase_execution_exception wrapping illegal_argument_exception). Evidence: - ES-YAML suite against a node built from this change (with its two sibling scroll fixes): `1453 passed · 0 failed · 10 skipped`, no stale catch-lenient entries; the four lines above are deleted. * test(api): pin the metric aggregation field-type check Runs through the ES router: avg on a keyword, max on an ip, sum on a keyword multi-field, stats through a field alias and min under a terms bucket each answer 400 with ES's reason; avg on a numeric field, on an unmapped field, across a keyword/long pair of indices, and value_count on a keyword still answer 200. On main the first test fails with the 200 it used to return: {"a":{"avg":{"field":"s"}}} -> {..."aggregations":{"a":{"value":null}}} left: 200 right: 400
AI-assisted change, run and reviewed by @amartya1523.
Burns down 20 lines of the
catch-lenient.txtratchet from #1583 (151 → 131). These are aggregation requests Elasticsearch 8.13 refuses at parse time with a 400, where XERJ answered 200 and ignored the bad value, or returned an empty/defaulted result.It is independent of #1824 (pipeline aggregations), #1854 (composite) and #1859 (search limits): different checks, same pattern. All four edit
catch-lenient.txt, and #1824 / #1854 also chain at the same four call sites, so whichever merges later needs a trivial rebase.What this changes, and why
find_invalid_agg_params(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:metanot an objectExpected [START_OBJECT] under [meta], but got a [VALUE_NULL] in [the_filter]precision_threshold< 0[precisionThreshold] must be greater than or equal to 0. Found [-1] in [distinct_int]execution_hintInvalid execution mode for cardinality aggregation. Got [bogus]expected one of [...]filterswith no filters[filters] cannot be empty.prefix_lengthoutside[0, 32]/[0, 128][prefix_length] must be in range [0, 32] while value is [44]compression≤ 0[compression] must be greater than 0. Found [0.0] in [mad]execution_hintInvalid execution_hint [no_such_hint], valid values are [default, high_accuracy]compression< 0[compression] must be greater than or equal to 0. Found [-1.0]valuesmissing / null / empty / non-numeric[values] must not be null…)fieldnorscriptRequired one of fields [field, script], but none were specified.probabilityoutside (0, 0.5] ∪ {1.0}[probability] must be between 0.0 and 0.5 or exactly 1.0, was [1.5]/… greater than 0.0 …… must have sub-aggregations/… cannot have a parent aggregation/… does not support sampling [time_series] aggregation [ts]globalTime series aggregations cannot be used inside global aggregation.[by_time] aggregation is incompatible with time series execution modeDeliberately not checked here: anything that needs the field's mapped type (
avgon a keyword field, regex include/exclude on an ip field, percentiles on a string field). Those stay in the list for a mapping-aware follow-up.Evidence
Full ES-YAML suite against a node built from this commit. The runner reported exactly 20 catch-lenient entries stale, with 0 failed:
Invalid Precision test,invalid execution hint throwsBad params,null metaInvalid IPv4 prefix,Invalid IPv6 prefixbad arguments,with execution hintinvalid paramsinvalid params,with execution hintExecution hintTest random_sampler aggregation with poor settingsNo field or scriptComposite …,Global …,Sampler …,auto_date_histogram …,rare_terms …,variable_width_histogram …With them deleted and enforced:
Unit tests (
es_compat::agg_param_validation_tests):valid_aggregations_passpins the shapes the suite relies on:high_accuracy/default;metaobjects;Checks
cargo fmt --all -- --check(exit 0)cargo build --release -p xerj-server -p es-yaml-runnercargo test --release -p xerj-api --lib agg_param_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.Provenance
percentile_ranksvaluesmessages. The YAML only checks for a 400 (bad_request), and I followed ES's[values] must not be nullphrasing without checking it against a live ES.