Fix NPE when `values` is omitted on percentile_ranks agg by polyfractal · Pull Request #26046 · elastic/elasticsearch

polyfractal · 2017-08-03T17:47:25Z

An array of values is required because there is no default (or reasonable way to set a default). But validation for values only happens if it is actually set on the builder. If the values param is omitted entirely then values is passed in as null, and later NPEs when attempting to iterate over the ranks.

I wasn't sure the best place to do this validation, since regular aggs don't have a validate() or equivalent. And I couldn't find a way to make params required in the ObjectParser. So checking right before the factory is built seemed like the best/only place?

@cbuescher would you mind reviewing this?

jpountz · 2017-08-07T06:36:19Z

I'm ok with this change, but I think that a better fix might be to make values a required argument of PercentileRanksAggregationBuilder and remove the setter? This way it won't be possible to create illegal instances of PercentileRanksAggregationBuilder?

cbuescher

@polyfractal I think checking values is a good change, however I'd also like to try removing the setter and try to validate the values as part of the constructor if it is mandatory. I think using ConstructingObjectParser should work in this case. I haven't tried but I can give it a go if you are stuck somewhere.

polyfractal · 2017-08-07T14:06:35Z

Ah right, I didn't think about ConstructingObjectParser. ++ will make the changes!

polyfractal · 2017-08-09T18:04:30Z

Alright, so @cbuescher, @nik9000 and I chatted about this earlier today. The way aggregations are built makes it tricky to use ConstructingObjectParser. To construct the object, we need both an externally provided value (aggregationName) and a value from the request (values).

We kicked around a few ideas:

Seeing if the NamedObject parsers would potentially work
Closing over the name when calling parse
Merging as-is and enhancing ConstructingObjectParser in a follow up PR to support this workflow a bit better
Using the parser context as a way to pass in the aggregation name

We agreed to see if the context approach would work, and if not, fall back to option 3 (merge as-is and follow up with another PR).

The most recent commit adjusts to use ConstructingObjectParser. The change was a bit more invasive than I wanted; the helpers that add common numeric fields (missing, etc) only supported ObjectParser. So I added support for Constructing, as well as generifying the methods so that the context wasn't hardcoded to Void.

The rest of the changes are just fixing up the tests.

Sooo... thoughts? Keep this, or revert to the simple exception and trying to enhance ConstructingObjectParser?

cbuescher

Hi @polyfractal, thats a great change. I left a couple of comments how to maybe simplify things a little more and some nits but I think this is almost good to go.

cbuescher · 2017-08-10T12:26:06Z

core/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java

+1, I think I did this in some WIP branch as well the other day. Didn't get back to it though.

Since this is implemented also in ObjectParser, I would make this an abstract method in AbstractObjectParser. I think this way ValuesSourceParserHelper.declareFields() could work AbstractObjectParser without all the current duclication. Not 100% sure though but I'd give it a try.

cbuescher · 2017-08-10T12:45:47Z

core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java

I think this whole block can go if all the existing declare* methods in this class take AbstractObjectParser instead of ObjectParser as an argument. That works when getName() (see above) is mave abstract in AbstractObjectParser.

cbuescher · 2017-08-10T12:49:41Z

...elasticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java

nit: space between after (List)

cbuescher · 2017-08-10T12:50:48Z

...elasticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java

nit: maybe static import for ConstructingObjectParser.constructorArg, that makes it more readable IMHO.

cbuescher · 2017-08-10T12:53:17Z

...elasticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java

Okay, I see now how you are piggybacking the name in the Context slot. maybe worth a comment here to remember the "why" in a few weeks from now.

cbuescher · 2017-08-10T12:54:04Z

core/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksIT.java

nit: spaces between the values

cbuescher · 2017-08-10T12:55:05Z

...elasticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java

Should this include a check for an empy array? Not sure, just a though. In case the answer is yes, maybe also add a test for that.

polyfractal · 2017-08-10T15:22:41Z

Review comments addressed, definitely cleaner with the change to AbstractObjectParser :)

Also added a few more tests to the AggregatorTestCase tests, since they are easy and fast to execute.

cbuescher

Thanks, LGTM. I left two small nits about comments but nothing that needs another review round once CI is green.

cbuescher · 2017-08-10T15:27:46Z

...elasticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java

cbuescher · 2017-08-10T15:29:27Z

...elasticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java

Thanks. I don't think its hacky ;-) Its okay as long as we have the context argument, when we try to remove it one day it might be useful to know why we use it here.

cbuescher

Ah, I forgot one thing, since this changes the aggregation constructor and the AggregationBuilders helper signature without keeping the old version, I think this should be marked as breaking. I don't know which versions this was supposed to go to but in any case I think some migration docs would be needed.

An array of values is required because there is no default (or reasonable way to set a default). But validation for values only happens if it is actually set. If the values param is omitted entirely than the agg builder will NPE.

- Moves the parser to ConstructingObjectParser - Uses the context to pass in the aggregation name - Adds getName() to ConstructingObjectParser - Adds more helpers to ValuesSourceParserHelper to support Constructing variety

An array of values is required because there is no default (or reasonable way to set a default). But validation for values only happens if it is actually set. If the values param is omitted entirely than the agg builder will NPE.

* master: (458 commits) Prevent cluster internal `ClusterState.Custom` impls to leak to a client (elastic#26232) Add packaging test for systemd runtime directive [TEST] Reenable RareClusterStateIt#testDeleteCreateInOneBulk Serialize and expose timeout of acknowledged requests in REST layer (elastic#26189) (refactor) some opportunities to use diamond operator (elastic#25585) [DOCS] Clarified readme for testing a single page Settings: Add keystore.seed auto generated secure setting (elastic#26149) Update version information (elastic#25226) "result" : created -> "result" : "created" (elastic#25446) Set RuntimeDirectory (elastic#23526) Drop upgrade from full cluster restart tests (elastic#26224) Further improve docs for requests_per_second Docs disambiguate reindex's requests_per_second (elastic#26185) [DOCS] Cleanup link for ec2 discovery (elastic#26222) Fix document field equals and hash code test Use holder pattern for lazy deprecation loggers Settings: Add keystore creation to add commands (elastic#26126) Docs: Cleanup docs for ec2 discovery (elastic#26065) Fix NPE when `values` is omitted on percentile_ranks agg (elastic#26046) Several internal improvements to internal test cluster infra (elastic#26214) ...

$@polyfractal$ polyfractal added :Analytics/Aggregations Aggregations >bug labels Aug 3, 2017

$@polyfractal$ polyfractal requested a review from cbuescher August 3, 2017 17:47

cbuescher self-assigned this Aug 7, 2017

cbuescher requested changes Aug 7, 2017

View reviewed changes

$@polyfractal$ polyfractal force-pushed the fixPercentileRanksNPE branch 2 times, most recently from e6784d4 to b33e6f1 Compare August 9, 2017 17:55

cbuescher requested changes Aug 10, 2017

View reviewed changes

cbuescher approved these changes Aug 10, 2017

View reviewed changes

cbuescher requested changes Aug 10, 2017

View reviewed changes

polyfractal added 6 commits August 15, 2017 11:03

$@polyfractal$

Use ConstructingObjectParser

36d99d9

- Moves the parser to ConstructingObjectParser - Uses the context to pass in the aggregation name - Adds getName() to ConstructingObjectParser - Adds more helpers to ValuesSourceParserHelper to support Constructing variety

$@polyfractal$

Fix line lengths

a5626f3

$@polyfractal$

Review cleanup and some more tests

d84394a

$@polyfractal$

Comment tweaks

1223973

$@polyfractal$

Update breaking changes docs

d5b5eb8

$@polyfractal$ polyfractal force-pushed the fixPercentileRanksNPE branch from f600ea5 to d5b5eb8 Compare August 15, 2017 15:08

$@polyfractal$ polyfractal added >breaking-java v6.0.0 v6.1.0 v7.0.0 labels Aug 15, 2017

$@polyfractal$ polyfractal merged commit d26becc into elastic:master Aug 15, 2017

colings86 added the 6.0.0-beta2 label Aug 24, 2017

colings86 added v6.0.0-beta2 and removed v6.0.0 6.0.0-beta2 labels Aug 24, 2017

lcawl removed the v6.1.0 label Dec 12, 2017

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

Conversation

polyfractal commented Aug 3, 2017

Uh oh!

jpountz commented Aug 7, 2017

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

polyfractal commented Aug 7, 2017

Uh oh!

polyfractal commented Aug 9, 2017

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal commented Aug 10, 2017

Uh oh!

cbuescher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

$@polyfractal$ polyfractal commented Aug 3, 2017

cbuescher left a comment •

edited

Loading

cbuescher left a comment •

edited

Loading