Fix failure for validate API on a terms query #29483

pcsanwald · 2018-04-11T18:58:09Z

Fixes #29033, currently WIP as I need to fix a few integration tests. As @colings86 described on the issue, we're missing a step for terms queries, we need to rewrite the query on the coordinating node.

Steps to reproduce (and confirm fix)

Run a local server: ./gradlew server
create index curl -XPUT 'localhost:9200/twitter' -H 'Content-Type: application/json' -d'{}'
add mapping curl -XPUT 'localhost:9200/twitter/_mapping/_doc' -H 'Content-Type: application/json' -d'{ "properties": { "user": { "type": "integer" }, "followers": { "type": "integer" } } }'
run a validate request on a terms query: curl -XPOST 'localhost:9200/twitter/_validate/query?explain=true' -H 'Content-Type: application/json' -d'{ "query": { "terms": { "user": { "index": "twitter", "type": "_doc", "id": "2", "path": "followers" } } } }'

…rtSearchAction

jasontedor

I left some comments.

jasontedor · 2018-04-11T19:04:12Z

...java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java

-import java.util.List;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;


This will fail precommit.

jasontedor · 2018-04-11T19:04:27Z

...java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java

 import org.elasticsearch.search.internal.AliasFilter;
 import org.elasticsearch.search.internal.SearchContext;
 import org.elasticsearch.search.internal.ShardSearchLocalRequest;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.threadpool.ThreadPool;
+import org.elasticsearch.transport.RemoteClusterAware;


I see a lot of unnecessary imports here.

pcsanwald · 2018-04-11T21:01:52Z

...java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java

+        ActionListener<org.elasticsearch.index.query.QueryBuilder> rewriteListener = ActionListener.wrap(rewrittenQuery -> {
+            request.query(rewrittenQuery);
+            super.doExecute(task, request, listener);
+        }, ex -> {


I added this section (with advice from @imotov) to address a deficiency in the code uncovered by one of our integration tests, org.elasticsearch.validate.SimpleValidateQueryIT.testExplainValidateQueryTwoNodes, which tests an error case. Because originally I was propagating the listener's onError method, an XContent exception was being thrown by the server (bad!), rather than wrapped in a response that wraps the error according to our conventions (good! yay, integration tests!).

My "fix" raises a few questions about what the right response looks like, given this context.

pcsanwald · 2018-04-11T21:03:19Z

Added some integration test fixes, and my own comments about those fixes; I could use some advice on the best way to handle this error in the right way at the point in execution where it's occurring (on the coordinating node, so there are no shards involved yet in the query).

elasticmachine · 2018-04-12T08:21:05Z

Pinging @elastic/es-search-aggs

pcsanwald · 2018-04-19T13:56:10Z

@jpountz happy to wait

colings86

@pcsanwald I left some comments

colings86 · 2018-04-23T10:17:53Z

server/src/main/java/org/elasticsearch/index/query/Rewriteable.java

@@ -111,7 +113,7 @@
                }
            }
            rewriteResponse.onResponse(builder);
-        } catch (IOException ex) {
+        } catch (IOException|XContentParseException|ParsingException ex) {


Instead of XContentParseException I think we should catch its super class IllegalArgumentException here since I know that we often throw IllegalArgumentException when validating requests as well

Reminder about this comment in case you missed it before?

colings86 · 2018-04-23T10:22:02Z

...java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java

+            }
+            List<QueryExplanation> explanations = new ArrayList<>();
+            // TODO[PCS]: given that we can have multiple indices, what's the best way to include
+            // these in a query explanation?


I assume this TODO still needs to be resolved? I don't have a good solution here other than having something like "__coordinating_node_rewrite__" in place of the index name

I think this TODO can be removed now?

colings86 · 2018-04-23T10:23:35Z

...java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java

+                    false,
+                    explanations,
+                    // TODO[PCS]: is it correct to have 0 for total shards if the request is invalidated before
+                    // it makes it to the shards? I think the answer is probably no?


I assume this TODO still needs to be resolved? Again I don't have a great solution here but given that it never made it onto any shard it is true that there were 0 total shards involved in the request, of which 0 succeeded and 0 failed so maybe this is ok?

colings86

@pcsanwald I think this is close, I left a couple of comments but they should be quick fixes. I also left a comment on the version checks and how they will need to be dealt with through back-porting to 6.x. I'm happy to walk through this process in more detail with you if you would like.

colings86 · 2018-04-26T08:35:53Z

...java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java

+            }
+            List<QueryExplanation> explanations = new ArrayList<>();
+            // TODO[PCS]: given that we can have multiple indices, what's the best way to include
+            // these in a query explanation?


I think this TODO can be removed now?

colings86 · 2018-04-26T08:39:24Z

...er/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java

+            out.writeOptionalString(index);
+        } else {
+            out.writeString(index);
+        }


I think that we should back port this fix to 6.4. These version checks are fine to leave here for now but when you back port you will need to change them to Version.V_6_4_0 in the back ported commit on the 6.x branch an then after that add a new commit to the master branch to change these checks to Version.V_6_4_0 on master too. This process avoids getting CI failures before you've had a chance to back port the commit to 6.x.

colings86 · 2018-04-26T08:40:16Z

server/src/main/java/org/elasticsearch/index/query/Rewriteable.java

@@ -111,7 +113,7 @@
                }
            }
            rewriteResponse.onResponse(builder);
-        } catch (IOException ex) {
+        } catch (IOException|XContentParseException|ParsingException ex) {


Reminder about this comment in case you missed it before?

pcsanwald · 2018-04-30T14:12:27Z

@elasticmachine please test this

pcsanwald · 2018-04-30T20:50:54Z

@elasticmachine test this please

colings86

LGTM

* WIP commit to try calling rewrite on coordinating node during TransportSearchAction * Use re-written query instead of using the original query * fix incorrect/unused imports and wildcarding * add error handling for cases where an exception is thrown * correct exception handling such that integration tests pass successfully * fix additional case covered by IndicesOptionsIntegrationIT. * add integration test case that verifies queries are now valid * add optional value for index * address review comments: catch superclass of XContentParseException fixes elastic#29483

* master: (68 commits) [DOCS] Removes X-Pack Elasticsearch release notes (#30272) Correct an example in the top-level suggester documentation. (#30224) [DOCS] Removes broken link [DOCS] Adds file realm configuration details (#30221) [DOCS] Adds PKI realm configuration details (#30225) Fix a reference to match_phrase_prefix in the match query docs. (#30282) Fix failure for validate API on a terms query (#29483) [DOCS] Fix 6.4-specific link in changelog (#30314) Remove RepositoriesMetaData variadic constructor (#29569) Test: increase authentication logging for debugging [DOCS] Removes redundant SAML realm settings (#30196) REST Client: Add Request object flavored methods (#29623) [DOCS] Adds changelog to Elasticsearch Reference (#30271) [DOCS] Fixes section error SQL: Teach the CLI to ignore empty commands (#30265) [DOCS] Adds Active Directory realm configuration details (#30223) [DOCS] Removes redundant file realm settings (#30192) [DOCS] Fixes users command name (#30275) Build: Move gradle wrapper jar to a dot dir (#30146) Build: Log a warning if disabling reindex-from-old (#30304)

* Fix failure for validate API on a terms query (#29483) * WIP commit to try calling rewrite on coordinating node during TransportSearchAction * Use re-written query instead of using the original query * fix incorrect/unused imports and wildcarding * add error handling for cases where an exception is thrown * correct exception handling such that integration tests pass successfully * fix additional case covered by IndicesOptionsIntegrationIT. * add integration test case that verifies queries are now valid * add optional value for index * address review comments: catch superclass of XContentParseException fixes #29483 * Backport terms-query-validate-bug changes to 6.x

Paul Sanwald added 3 commits April 10, 2018 14:49

WIP commit to try calling rewrite on coordinating node during Transpo…

b22eeb8

…rtSearchAction

Use re-written query instead of using the original query

8127f2d

Merge branch 'master' into terms-query-validate-bug

c291ea0

pcsanwald added the WIP label Apr 11, 2018

pcsanwald changed the title ~~Fix incorrect failure when validating terms query~~ Fix incorrect failure when using validate API on a terms query Apr 11, 2018

pcsanwald changed the title ~~Fix incorrect failure when using validate API on a terms query~~ Fix failure for validate API on a terms query Apr 11, 2018

jasontedor reviewed Apr 11, 2018

View reviewed changes

Paul Sanwald added 2 commits April 11, 2018 12:17

fix incorrect/unused imports and wildcarding

f94a48a

add error handling for cases where an exception is thrown

3e4b77c

pcsanwald commented Apr 11, 2018

View reviewed changes

colings86 added the :Search/Search Search-related issues that do not fall into other categories label Apr 12, 2018

colings86 added the >bug label Apr 12, 2018

Paul Sanwald added 4 commits April 16, 2018 13:59

Merge branch 'master' into terms-query-validate-bug

392431e

correct exception handling such that integration tests pass successfully

f218da2

fix additional case covered by IndicesOptionsIntegrationIT.

4925ba8

Merge branch 'master' into terms-query-validate-bug

454ab2b

Paul Sanwald added 2 commits April 19, 2018 11:39

add integration test case that verifies queries are now valid

3849ae6

Merge branch 'master' into terms-query-validate-bug

ddc9f42

colings86 reviewed Apr 23, 2018

View reviewed changes

Paul Sanwald added 3 commits April 25, 2018 08:56

Merge branch 'master' into terms-query-validate-bug

e0003d0

Merge branch 'master' into terms-query-validate-bug

39330c9

add optional value for index

226da40

colings86 reviewed Apr 26, 2018

View reviewed changes

address review comments: catch superclass of XContentParseException

6d5eeff

elastic deleted a comment from jpountz Apr 27, 2018

Merge branch 'master' into terms-query-validate-bug

f5dd57d

pcsanwald removed the WIP label May 1, 2018

colings86 added v7.0.0 v6.4.0 labels May 1, 2018

colings86 approved these changes May 1, 2018

View reviewed changes

pcsanwald merged commit 00b21f8 into elastic:master May 1, 2018

pcsanwald mentioned this pull request May 1, 2018

6.x Backport: Terms query validate bug #30319

Merged

pcsanwald mentioned this pull request May 4, 2018

add version compatibility from 6.4.0 after backport, see #30319 #30390

Merged

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

javanna mentioned this pull request May 1, 2019

Query validation doesn't work consistently on inner queries #38801

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failure for validate API on a terms query #29483

Fix failure for validate API on a terms query #29483

pcsanwald commented Apr 11, 2018

jasontedor left a comment

jasontedor Apr 11, 2018

jasontedor Apr 11, 2018

pcsanwald Apr 11, 2018 •

edited

Loading

pcsanwald commented Apr 11, 2018

elasticmachine commented Apr 12, 2018

pcsanwald commented Apr 19, 2018

colings86 left a comment

colings86 Apr 23, 2018

colings86 Apr 26, 2018

colings86 Apr 23, 2018

colings86 Apr 26, 2018

colings86 Apr 23, 2018

colings86 left a comment

colings86 Apr 26, 2018

colings86 Apr 26, 2018

colings86 Apr 26, 2018

pcsanwald commented Apr 30, 2018

pcsanwald commented Apr 30, 2018

colings86 left a comment

Fix failure for validate API on a terms query #29483

Fix failure for validate API on a terms query #29483

Conversation

pcsanwald commented Apr 11, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcsanwald Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

pcsanwald commented Apr 11, 2018

elasticmachine commented Apr 12, 2018

pcsanwald commented Apr 19, 2018

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcsanwald commented Apr 30, 2018

pcsanwald commented Apr 30, 2018

colings86 left a comment

Choose a reason for hiding this comment

pcsanwald Apr 11, 2018 •

edited

Loading