Handle NPE due to a weird configuration by ebuildy · Pull Request #48007 · elastic/elasticsearch

ebuildy · 2019-10-14T14:45:03Z

Prevent java.lang.NullPointerException. To reproduce: set max_query_terms to 0 and you get:

"Caused by: java.lang.NullPointerException",
"at org.elasticsearch.common.lucene.search.XMoreLikeThis.createQueue(XMoreLikeThis.java:766) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.common.lucene.search.XMoreLikeThis.createQueue(XMoreLikeThis.java:716) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.common.lucene.search.XMoreLikeThis.like(XMoreLikeThis.java:630) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.common.lucene.search.MoreLikeThisQuery.createQuery(MoreLikeThisQuery.java:172) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.common.lucene.search.MoreLikeThisQuery.rewrite(MoreLikeThisQuery.java:156) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.apache.lucene.search.IndexSearcher.rewrite(IndexSearcher.java:667) ~[lucene-core-8.1.0.jar:8.1.0 dbe5ed0b2f17677ca6c904ebae919363f2d36a0a - ishan - 2019-05-09 19:34:03]",
"at org.elasticsearch.search.internal.ContextIndexSearcher.rewrite(ContextIndexSearcher.java:110) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.search.DefaultSearchContext.preProcess(DefaultSearchContext.java:263) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.search.query.QueryPhase.preProcess(QueryPhase.java:91) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.search.SearchService.createContext(SearchService.java:603) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.search.SearchService.createAndPutContext(SearchService.java:550) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.search.SearchService.executeQueryPhase(SearchService.java:353) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.search.SearchService.lambda$executeQueryPhase$1(SearchService.java:340) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.action.ActionListener.lambda$map$2(ActionListener.java:145) ~[elasticsearch-7.3.1.jar:7.3.1]",
"at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:62) ~[elasticsearch-7.3.1.jar:7.3.1]",
"... 8 more"] }

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?
If submitting code, have you built your formula locally prior to submission with gradle check?
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
If submitting code, have you checked that your submission is for an OS and architecture that we support?
If you are submitting this code for a class then read our policy for that.

Prevent ``java.lang.NullPointerException``. To reproduce: set ``max_query_terms`` to 0 and you get: ``` "Caused by: java.lang.NullPointerException", "at org.elasticsearch.common.lucene.search.XMoreLikeThis.createQueue(XMoreLikeThis.java:766) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.common.lucene.search.XMoreLikeThis.createQueue(XMoreLikeThis.java:716) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.common.lucene.search.XMoreLikeThis.like(XMoreLikeThis.java:630) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.common.lucene.search.MoreLikeThisQuery.createQuery(MoreLikeThisQuery.java:172) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.common.lucene.search.MoreLikeThisQuery.rewrite(MoreLikeThisQuery.java:156) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.apache.lucene.search.IndexSearcher.rewrite(IndexSearcher.java:667) ~[lucene-core-8.1.0.jar:8.1.0 dbe5ed0b2f17677ca6c904ebae919363f2d36a0a - ishan - 2019-05-09 19:34:03]", "at org.elasticsearch.search.internal.ContextIndexSearcher.rewrite(ContextIndexSearcher.java:110) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.search.DefaultSearchContext.preProcess(DefaultSearchContext.java:263) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.search.query.QueryPhase.preProcess(QueryPhase.java:91) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.search.SearchService.createContext(SearchService.java:603) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.search.SearchService.createAndPutContext(SearchService.java:550) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.search.SearchService.executeQueryPhase(SearchService.java:353) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.search.SearchService.lambda$executeQueryPhase$1(SearchService.java:340) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.action.ActionListener.lambda$map$2(ActionListener.java:145) ~[elasticsearch-7.3.1.jar:7.3.1]", "at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:62) ~[elasticsearch-7.3.1.jar:7.3.1]", "... 8 more"] } ```

williamrandolph · 2019-10-14T14:51:59Z

@elasticmachine test this please

elasticmachine · 2019-10-14T14:54:51Z

Pinging @elastic/es-search (:Search/Search)

cbuescher

Hi @ebuildy,

thanks for the PR, I'd like to understand how you ran into this particular NPE and would rather catch it earlier on by checking the input parameters and add vaidation checks / throw IAE in the builders and query itself before we even get to constructing a query. Does this make sense or am I missing that would make max_query_terms of 0 (or even negative) a setting we should to support?

cbuescher · 2019-10-15T13:22:51Z

server/src/main/java/org/elasticsearch/common/lucene/search/XMoreLikeThis.java

+
+        if (limit == 0) {
+            return queue;
+        }


Rather than checking for a particular value this late, I'd rather try improving the input validation for the max_query_terms parameter, both in the MoreLikeThisQueryBuilder setter and also maybe in MoreLikeThisQuery. I think we should reject negative or zero values here, since at least to my understanding it indicates a wrong usage of the API. Or are there cases you know where this parameter setting would make sense?

I agree there is no sense to set max_query_terms = 0, maybe the API should validate this parameter instead.

Are you interested in changing this PR to add validation to the MoreLikeThisQueryBuilder and MoreLikeThisQuery setters for the max query terms? I think we should reject all values <= 0 there already with an IllegalArgumentException and a fitting message. Adding these checkes should also be tests in the corresponding unit tests. If you don't want to do this just let me know, I'll open an issue so we can fix it later.

Sure! thanks you

cbuescher · 2019-12-05T18:25:28Z

Hi @ebuildy, just checking in to see if you are still interested in working on this PR. If you are busy and cannot continue working on it we can also open an issue and somebody else can pick it up.

ebuildy · 2019-12-06T15:22:19Z

hello @cbuescher I am quite busy by this end of the year, if someone can work on it this would be perfect, sorry for my late.

cbuescher · 2019-12-06T16:50:08Z

@ebuildy no problem, thanks for getting back. I opened #49927 and will close this PR then, but feel free to pick it up once you have time if it is still open.

williamrandolph added the :Search/Search Search-related issues that do not fall into other categories label Oct 14, 2019

williamrandolph added the >bug label Oct 14, 2019

cbuescher requested changes Oct 15, 2019

View reviewed changes

cbuescher self-assigned this Oct 15, 2019

cbuescher mentioned this pull request Dec 6, 2019

Prevent illegal max_query_terms values in MLT query #49927

Closed

cbuescher closed this Dec 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle NPE due to a weird configuration#48007

Handle NPE due to a weird configuration#48007
ebuildy wants to merge 1 commit intoelastic:masterfrom
ebuildy:patch-1

ebuildy commented Oct 14, 2019

Uh oh!

williamrandolph commented Oct 14, 2019

Uh oh!

elasticmachine commented Oct 14, 2019

Uh oh!

cbuescher left a comment

Uh oh!

cbuescher Oct 15, 2019

Uh oh!

ebuildy Oct 15, 2019

Uh oh!

cbuescher Oct 16, 2019

Uh oh!

ebuildy Oct 17, 2019

Uh oh!

cbuescher commented Dec 5, 2019

Uh oh!

ebuildy commented Dec 6, 2019

Uh oh!

cbuescher commented Dec 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ebuildy commented Oct 14, 2019

Uh oh!

williamrandolph commented Oct 14, 2019

Uh oh!

elasticmachine commented Oct 14, 2019

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

cbuescher Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

ebuildy Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

cbuescher Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ebuildy Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Dec 5, 2019

Uh oh!

ebuildy commented Dec 6, 2019

Uh oh!

cbuescher commented Dec 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants