Repository navigation
fix(api): reject misplaced pipeline aggregations and bad buckets_paths like ES - #1824
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 |
…s like ES Motivation: `catch-lenient.txt` (xerj-org#1583) records ES-YAML cases where Elasticsearch returns an error and XERJ does not. 25 of them are one class: pipeline aggregations that are misplaced or point at aggregations that do not exist. ES 8.13 refuses those requests with a 400; XERJ answered 200 with a `null` (or a silently skipped) pipeline value, so a typo in a `buckets_path`, or a `derivative` under a `terms`, looked like "no data". Root cause: Pipeline aggregations become sentinels during execution and are resolved afterwards; a missing path target degrades to `{"value": null}` (`resolve_pipeline_agg_with_gap`, `resolve_bucket_script`) and a parent-only pipeline whose parent is not a bucket list is simply never applied (`apply_bucket_pipeline_ops`). Nothing validated placement or paths, and the executor has no error channel for it. Fix: `find_invalid_pipeline_agg` (es_compat.rs), a request-time walk of the aggregation tree next to `find_invalid_agg_order_path` (xerj-org#1283) and chained after it at the same four call sites (`_search` via `build_search_request`, `_msearch`, `_search/template`, `_msearch/template`). So the answer does not depend on which executor would run. ES 8.13's rules and messages: - derivative / cumulative_sum / serial_diff / moving_fn / cumulative_cardinality / moving_percentiles need a histogram, date_histogram or auto_date_histogram parent; - bucket_script / bucket_selector / bucket_sort must be declared inside another aggregation, and that parent must be multi-bucket (single-bucket parents named by ES's internal class: InternalFilter, ...); - bucket_sort must have a sort, size or from; moving_fn's window > 0; - every buckets_path (and bucket_sort sort key) must name aggregations that exist from the level the pipeline is declared at: "No aggregation found for path [p]" / "No aggregation [x] found for path [p]", wrapped "Validation Failed: 1: ...;" as ES does. Path checking reports a path only when no reading of it can name an existing aggregation: value keys (`p.99`, `p[99.9]`, `s.avg`), dotted aggregation names (`v.v`), `_count`/`_key`/`_bucket_count`, references to sibling pipelines and anything past a metric aggregation are left to the executor as before. Evidence: - Full ES-YAML suite against a node built from this commit: 0 failed, and the runner flagged exactly 25 catch-lenient entries as stale (now met); those 25 lines are deleted, and with the ratchet enforcing them the suite is `1402 passed · 0 failed · 10 skipped`, catch-lenient 151 -> 126. - Unit tests: valid shapes (pipelines on pipelines, value/bracket keys, dotted names, `_count`, bucket_sort with only `from`) are not refused; each rule produces ES's message.
|
Rebased onto current The conflicts were mechanical (both PRs added a request-time validator at the same insertion point and chained it at the same four call sites), so I re-applied this change on top of Re-verified on the new head: ( |
5dc67ee to
23a89f1
Compare
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.
|
Xerj Merger made the fix: reviewed the full diff: pipeline-aggregation placement and |
AI-assisted change, run and reviewed by @amartya1523.
Burns down 25 lines of the
catch-lenient.txtratchet from #1583 (151 → 126). All 25 are one class: pipeline aggregations that are misplaced or point at aggregations that do not exist.What this changes, and why
Elasticsearch 8.13 refuses these requests with a 400. XERJ answered 200 with a
null(or silently skipped) pipeline value, so a typo in abuckets_path, or aderivativeunder aterms, looked like "no data".Root cause. Pipeline aggregations become sentinels during execution and are resolved afterwards:
{"value": null}(resolve_pipeline_agg_with_gap,resolve_bucket_script);apply_bucket_pipeline_ops).Nothing validated placement or paths, and the executor has no error channel for it.
Fix.
find_invalid_pipeline_agg(es_compat.rs) is a request-time walk of the aggregation tree, placed next tofind_invalid_agg_order_path(#1283). It is chained after that check at the same four call sites (_searchviabuild_search_request,_msearch,_search/template,_msearch/template), so the answer does not depend on which executor would run. It applies ES 8.13's rules and messages:{type} aggregation [{name}] must have a histogram, date_histogram or auto_date_histogram as parent but doesn't have a parent{type} aggregation [{name}] must have a histogram, date_histogram or auto_date_histogram as parent{type} aggregation [{name}] must be declared inside of another aggregationExpected a multi bucket aggregation but got [InternalFilter] for aggregation [{name}][{name}] is configured to perform nothing. Please set either of [sort, size, from] to use bucket_sortwindow≤ 0[window] must be a positive, non-zero integer.buckets_path(or bucket_sort sort key) naming nothingValidation Failed: 1: No aggregation found for path [p];/No aggregation [x] found for path [p];Path checking is one-sided on purpose. A path is reported only when no reading of it can name an existing aggregation. These are left to the executor as before:
p.99,p[99.9],s.avg);v.v);_count,_keyand_bucket_count;A false positive would turn a working query into a 400, which is worse than the gap.
Evidence
Full ES-YAML suite against a node built from this commit. The runner flagged exactly 25 catch-lenient entries as stale (
NOTE stale catch-lenient entry, delete it: …) and 0 failed. Those 25 lines are deleted. With the ratchet now enforcing them:(151 before.) The 25 cases:
bad pathin avg_bucket / max_bucket / min_bucket / sum_bucket / percentiles_bucket / cumulative_sum / derivative / moving_fn;invalid parent aggregationin bucket_script / bucket_selector / bucket_sort / cumulative_sum / derivative / moving_fn / serial_diff;top level failsin bucket_script / bucket_selector / bucket_sort;invalid pathin bucket_script / bucket_selector;invalid path failsandno action failsin bucket_sort;Not under date_histoin derivative / moving_fn;Bad windowin moving_fn.Unit tests (
es_compat::pipeline_agg_validation_tests):valid_pipelines_passpins the shapes that must keep working:p.99andp[99.9]value keys,s.avg, and the dotted agg namev.v.value;_count;buckets_paths;bucket_sortwith[{b: {order: desc}}, "_key"]+size, orfromalone.Checks
cargo fmt --all -- --check(exit 0)cargo build --release -p xerj-server -p es-yaml-runnercargo test --release -p xerj-api --lib pipeline_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(release/fat-LTO test binaries do not fit this machine's free disk; CI runs them) and clippy.Out of scope, left in the list:
bucket_script/bucket_selector·bad script: these need a Painless-style runtimeNullPointerException.pipeline.yml·Max pipeline on terms agg: the expected message names the ES result class (LongTerms), which depends on the field mapping.pipeline.ymlmulti-value-metric cases: ES raises them at reduce time, so on an empty index ES answers 200; a static check would be stricter than ES.Engine unit test
bucket_script_missing_buckets_path_target_stays_an_explicit_null(aggs.rs) is untouched: this PR does not changexerj-engine, so the executor keeps its lenient behaviour, and the API now refuses such requests before they reach it.Provenance
search_phase_execution_exception(viaXerjError::invalid_query, like [bug]:terms.orderon a non-existent aggregation is accepted (HTTP 200, order silently dropped) instead of a 400 #1283's check), while ES usesaction_request_validation_exception/illegal_argument_exception. The YAML cases match the reason text only, and I did not change the type mapping.