Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject multiple methods in `percentiles` aggregation #26163

Merged
merged 2 commits into from Aug 15, 2017

Conversation

Projects
None yet
4 participants
@cbuescher
Copy link
Member

commented Aug 11, 2017

Currently the percentiles aggregation allows specifying both possible methods
in the query DSL, but only the later one is used. This changes it to rejecting
such requests with an error. Setting the method multiple times via the java API
still works (and the last one wins).

Closes #26095

...org/elasticsearch/search/aggregations/metrics/percentiles/PercentilesAggregationBuilder.java Outdated
* overwrite {@link #method()} to check that it is not called twice when parsing xContent and throw
* an error while the Java API should allow this
*/
private static class InternalBuilder extends PercentilesAggregationBuilder {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Aug 11, 2017

Author Member

In a first wip of ths PR I was doing the checks if the setter was called multiple times in the original builder, but that felt akward since it would also affect the java api and would mean the PercentilesAggregationBuilder would somehow need that call counter somewhere. Attempts to use ConstructingObjectParser to check that methods was specified only once also were not promosing, so I went with this internal temporary object for the parser output for now as it felt least invasive.

setIfNotNull(returnedAgg::field, internal.field());
setIfNotNull(returnedAgg::script, internal.script());
setIfNotNull(returnedAgg::method, internal.method());
setIfNotNull(returnedAgg::percentiles, internal.percentiles());

This comment has been minimized.

Copy link
@polyfractal

polyfractal Aug 14, 2017

Member

Should we also check for empty arrays too? Or maybe down in the percentiles() method?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Aug 14, 2017

Author Member

Sure, I will add something along those lines.

@polyfractal

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

LGTM. The requirement to keep java builder behavior unchanged seems to complicate life a bit, but the solution seems reasonable to me :)

cbuescher added some commits Aug 10, 2017

Reject multiple methods in `percentiles` aggregation
Currently the `percentiles` aggregation allows specifying both possible methods
in the query DSL, but only the later one is used. This changes it to rejecting
such requests with an error. Setting the method multiple times via the java API
still works (and the last one wins).

Closes #26095

@cbuescher cbuescher force-pushed the cbuescher:fix-26095 branch to 71df102 Aug 15, 2017

@cbuescher cbuescher merged commit 34610b8 into elastic:master Aug 15, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

cbuescher added a commit that referenced this pull request Aug 15, 2017

Reject multiple methods in `percentiles` aggregation (#26163)
Currently the `percentiles` aggregation allows specifying both possible methods
in the query DSL, but only the later one is used. This changes it to rejecting
such requests with an error. Setting the method multiple times via the java API
still works (and the last one wins).

Closes #26095

cbuescher added a commit that referenced this pull request Aug 15, 2017

Reject multiple methods in `percentiles` aggregation (#26163)
Currently the `percentiles` aggregation allows specifying both possible methods
in the query DSL, but only the later one is used. This changes it to rejecting
such requests with an error. Setting the method multiple times via the java API
still works (and the last one wins).

Closes #26095

@lcawl lcawl removed the v6.1.0 label Dec 12, 2017

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.