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

Fix ArrayIndexOutOfBoundsException when no ranges are specified in the query #23241

Merged

Conversation

@giorgosp
Copy link
Contributor

commented Feb 17, 2017

The issue is reproduced in 5.1, 5.2, 5.3, 5.x and master branch. My fix is on master.

This commit just checks the ranges array size at the spot where the exception is raised. I am not sure if this should be fixed at a higher level in the code.

Closes #22881

…s are specified in the query
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 17, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@clintongormley clintongormley requested a review from javanna Feb 20, 2017
@javanna

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Hi @giorgosp congrats on your first PR ;)

I was wondering, in the linked issue an NPE is mentioned, while in the title of this PR you mentioned an array index out of bounds exception. Are these two different bugs or the same one? Also, wouldn't it make sense to throw an error if no ranges are specified? @colings86 what do you think?

@colings86

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

I agree that we should throw an exception if no ranges are specified. We could actually check for that in the RangeAggregationBuilder so we fail early and on the coordinating node in this case.

@giorgosp

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2017

Hello guys, thanks for the feedback!

It is a NullPointerException in the 5.1 branch while in the others (5.2, 5.x, master) it is an ArrayIndexOutOfBoundsException and is thrown only if there is at least one doc indexed.

Indeed, returning an error in the response makes more sense.

Also, I am noticing that the same error will happen to all aggregations that use RangeAggregator. Maybe I should raise an IllegalStateException inside RangeAggregator's constructor instead of inside each one of **Builder.innerBuild() methods?

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

Hi @giorgosp we would prefer to fail early in the coordinating node, rather than later in each shard, hence it would be better to add validation to each builder that has this problem. Thanks!

giorgosp added 2 commits Feb 28, 2017
…no ranges are specified in the query"

This reverts commit ad57d8f.
…s in a range or date_range query
@giorgosp

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

Hi @javanna, please check the last commit (I will rebase all the commits when review is ok).

I am making the check inside the parse() method of the RangeAggregationBuilder. But there is an issue with this approach, I cannot create an integration test because it seems that integration tests do not rely on any "parse" methods.
Consequently, an integration test (see ad57d8f#diff-369471927f6eb5e2ebfdf58649078d26) will still raise the old exception.

I could manually call the builder's parse method from inside the integration test but this won't seem like an integration test anymore.

Thanks!

@javanna

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

@colings86 do you mind having a look please?

Copy link
Member

left a comment

Thanks for making the changes, I left a comment in the code, but also could we do this for the IpRangeAggregationBuilder and the GeoDistanceAggregationBuilder too?

It would also be good to add a test method which checks the exception is thrown when no ranges are given.

@@ -283,7 +288,7 @@ public DateRangeAggregationBuilder addUnboundedFrom(DateTime from) {
@Override
protected DateRangeAggregatorFactory innerBuild(SearchContext context, ValuesSourceConfig<Numeric> config,
AggregatorFactory<?> parent, Builder subFactoriesBuilder) throws IOException {
// We need to call processRanges here so they are parsed and we know whether `now` has been used before we make
// We need to call processRanges here so they are parsed and we know whether `now` has been used before we make
// the decision of whether to cache the request
Range[] ranges = processRanges(context, config);

This comment has been minimized.

Copy link
@colings86

colings86 Mar 14, 2017

Member

Could we put the check here so that it is run for both the transport and rest APIs? When put here we should make it an IllegalArgumentException and we should update the other Range Aggregation Builders too.

…s in the query

This fix is applied to range queries, date range queries, ip range queries and geo distance aggregation queries
@giorgosp

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

Hi @colings86, I have updated the code according to your comments. I am willing to make further changes if necessary. If it's fine, I will squash the commits and rebase the branch on master. Thanks!

@colings86

This comment has been minimized.

Copy link
Member

commented May 17, 2017

jenkins test this

Copy link
Member

left a comment

LGTM. Thanks for contributing this change. I'll squash and merge this change and backport to the 5.x and 5.4 branches

@colings86 colings86 merged commit 8a4d890 into elastic:master May 17, 2017
1 of 2 checks passed
1 of 2 checks passed
elasticsearch-ci Build finished.
Details
CLA Commit author has signed the CLA
Details
@giorgosp

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

Ok, thanks!

colings86 added a commit that referenced this pull request May 17, 2017
…e query (#23241)

* Fix ArrayIndexOutOfBoundsException in Range Aggregation when no ranges are specified in the query

* Revert "Fix ArrayIndexOutOfBoundsException in Range Aggregation when no ranges are specified in the query"

This reverts commit ad57d8f.

* Fix range aggregation out of bounds exception when there are no ranges in a range or date_range query

* Fix range aggregation out of bounds exception when there are no ranges in the query

This fix is applied to range queries, date range queries, ip range queries and geo distance aggregation queries
colings86 added a commit that referenced this pull request May 17, 2017
…e query (#23241)

* Fix ArrayIndexOutOfBoundsException in Range Aggregation when no ranges are specified in the query

* Revert "Fix ArrayIndexOutOfBoundsException in Range Aggregation when no ranges are specified in the query"

This reverts commit ad57d8f.

* Fix range aggregation out of bounds exception when there are no ranges in a range or date_range query

* Fix range aggregation out of bounds exception when there are no ranges in the query

This fix is applied to range queries, date range queries, ip range queries and geo distance aggregation queries
@clintongormley clintongormley added v5.4.1 and removed v5.4.2 labels May 22, 2017
@clintongormley clintongormley added v6.0.0-alpha2 and removed v6.0.0 labels Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.