REST high-level client: add validate query API #31077

sohaibiftikhar · 2018-06-04T14:47:31Z

Relates to #27205

sohaibiftikhar · 2018-06-04T14:49:37Z

...c/main/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryResponse.java

+            Set<QueryExplanation> queryExplSet = new HashSet<>(getQueryExplanation());
+            // We only compare with the index and the shardId because for every failure this is unique.
+            // Also because it is hard to compare Throwable.
+            Set<Tuple<String, Integer>> shardFailureSet =


@nik9000 @javanna Since its hard to compare exceptions, in this case, I just check for the index and the shardId. Is this alright?

I'd prefer to keep the equals method complete and only use it in the test when there aren't exceptions. I dunno what we've done other places though.

I'd also like to do something like assertThat(exception.getMessage(), containsString("original message"));.

The assert part makes sense. However, I don't completely understand what needs to be done for the equals.

I mean to make equals just call equals on the Exceptions. It can certainly do Objects.equals for the null, but I'd prefer the equals make sure the objects are actually equal. And because we can't assert proper equality when round tripping the exceptions I'd hand write an test for the round tripping with exceptions and leave exceptions out of the AbstractStreamableTestCase infrastructure.

sohaibiftikhar · 2018-06-04T14:50:39Z

...t/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryResponseTests.java

+        // We cannot have XContent equivalence for ValidateResponseTests as it holds the BroadcastResponse which
+        // holds a List<DefaultShardOperationFailedException>. The DefaultShardOperationFailedException uses ElasticSearchException
+        // for serializing/deserializing a Throwable and the serialized and deserialized versions do not match.
+        return false;


@nik9000 @javanna I also added this for the same reason. Since exception messages change after deserialization with ElasticSearchException.

nik9000 · 2018-06-04T14:54:44Z

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

+    @Override
+    public int hashCode() {
+        int result = 1;
+        if (getIndex() != null) {


I'd use Objects.hash because the performance here isn't that important and it is easier to review for correctness.

Thanks! I didn't know this existed either :)

nik9000 · 2018-06-04T14:55:27Z

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

+
+    @Override
+    public boolean equals(Object o) {
+        if (o instanceof QueryExplanation) {


Can you make this look like our other equals methods?

nik9000 · 2018-06-04T14:57:27Z

...c/main/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryResponse.java

+            Set<QueryExplanation> queryExplSet = new HashSet<>(getQueryExplanation());
+            // We only compare with the index and the shardId because for every failure this is unique.
+            // Also because it is hard to compare Throwable.
+            Set<Tuple<String, Integer>> shardFailureSet =


I'd prefer to keep the equals method complete and only use it in the test when there aren't exceptions. I dunno what we've done other places though.

I'd also like to do something like assertThat(exception.getMessage(), containsString("original message"));.

nik9000 · 2018-06-04T14:58:24Z

test/framework/src/main/java/org/elasticsearch/test/AbstractStreamableXContentTestCase.java

+     * In case it does, such behaviour will be tested by comparing the XContent of the original instance
+     * and the parsed/deserialized instance.
+     */
+    protected boolean assertToXContentEquivalence() {


@nik9000 So what do you reckon? I shouldn't extend AbstractStreamableXContentTestCase?

I think you can extend that to get good randomized coverage for the case when you don't have any exceptions and then hand write a couple of test cases for the exception round tripping. I think @javanna talked about doing that some other place....

Have a look at ListTasksResponseTests, where we separate testing between two scenarios: without (no big problems, assertToXContentEquivalence true) and with (assertToXContentEquivalence false). I think that this change may not be needed if you do it that way.

nik9000 · 2018-06-04T14:59:17Z

@elasticmachine, whitelist this PR please.

nik9000 · 2018-06-04T15:02:57Z

@elasticmachine, add to whitelist

elasticmachine · 2018-06-04T18:28:45Z

Pinging @elastic/es-core-infra

sohaibiftikhar · 2018-06-04T20:35:30Z

I was not able to reproduce the errors on my machine. Will merge in master to see if that fixes it. There is a remote possibility that these are related though.

hub-cap · 2018-06-04T20:53:14Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

@@ -804,6 +805,17 @@ static Request putTemplate(PutIndexTemplateRequest putIndexTemplateRequest) thro
        return request;
    }

+    static Request validateQuery(ValidateQueryRequest validateQueryRequest) throws IOException {


Can you add a test to https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java

Yes, thanks for catching this.

hub-cap · 2018-06-04T20:57:00Z

docs/java-rest/high-level/indices/validate_query.asciidoc

+[[java-rest-high-indices-validate-query-request]]
+==== Validate Query Request
+
+A `ValidateRequest` requires one or more `indices` on which to the query is validated. If not index


ValidateQueryRequest

hub-cap · 2018-06-04T20:59:05Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

+        params.putParam("explain", Boolean.toString(validateQueryRequest.explain()));
+        params.putParam("all_shards", Boolean.toString(validateQueryRequest.allShards()));
+        params.putParam("rewrite", Boolean.toString(validateQueryRequest.rewrite()));
+        request.setEntity(createEntity(validateQueryRequest, REQUEST_BODY_CONTENT_TYPE));


it looks like we might be missing a few more options such as types and indicesOptions, as specified in https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryAction.java#L62-L67

types are being set in the URL I believe. I will add the indicesOptions.

sohaibiftikhar · 2018-06-06T21:21:53Z

@nik9000 Could you have a look again when you have time?

Relates to elastic#27205

-- Removed equals and hashCode from ValidateQueryResponse -- Extended AbstractBroadcastResponseTestCase instead of AbstractStreamableXContentTestCase -- Fixed asciidoc -- Added test for RequestConverters::validateQuery -- Added indicesOptions

javanna

left a couple of comments as well.

javanna · 2018-06-08T12:12:50Z

client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesClient.java

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-validate.html"> Validate Query API
+     * on elastic.co</a>
+     */
+    public void validateQueryAsync(ValidateQueryRequest validateQueryRequest, RequestOptions options,


can you please add params tag like we have now in all of the other methods?

javanna · 2018-06-08T12:15:00Z

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

+    }
+
+
+


nit: can you remove some of these empty lines please?

javanna · 2018-06-08T12:16:03Z

docs/java-rest/high-level/indices/validate_query.asciidoc

+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[validate-query-request-query]
+--------------------------------------------------
+<1> Build the desired query.
+


can you add an example of how to set the query to the request?

javanna · 2018-06-08T12:18:01Z

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

+            result &= other.getExplanation() == null;
+        } else {
+            result &= getExplanation().equals(other.getExplanation());
+        }


is there a reason not to use Objects.equals like we do in a lot of other equals methods?

You are right. I will switch this. It will at least remove the null checks.

javanna · 2018-06-08T12:18:51Z

...rc/main/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryRequest.java

+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.field("query");
+        query.toXContent(builder, params);
+        return builder;


can you make it a ToXContentObject please instead?

In this case, I should add builder.startObject in the toXContent method, right?

yea startObject and endObject, see #31155 also

javanna · 2018-06-08T12:21:43Z

...t/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryResponseTests.java

+            queryExplanations.add(queryExplanation);
+        }
+        Collections.shuffle(queryExplanations, random());
+        Collections.shuffle(shardFailures, random());


why do we need to shuffle these two?

We are creating the XContent thing very serially. First all successful ones and then all failures. I thought it would be more realistic if this was shuffled.

but they go to two different lists, so I think that what matters is that each item is randomized? I am not sure but shuffling buys us.

The query explanation contains the failures and the successful ones. Ok, we can do away with the shuffling of failures. But the query explanations should stay I think.

javanna · 2018-06-08T12:25:51Z

...t/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryResponseTests.java

+        );
+    }
+
+    protected static ValidateQueryResponse createRandomValidateQueryResponse() {


do these methods need to be protected?

Umm no I think. I will make it private.

-- added javadoc -- added documentation for setting query to request -- changed to Object.equals for QueryExplanation::equals -- extended ToXContentObject instead of ToXContentFragment for request

…o validate_query

javanna · 2018-06-13T10:56:05Z

docs/java-rest/high-level/supported-apis.asciidoc

 include::indices/get_templates.asciidoc[]
+>>>>>>> 0bfd18cc8b6ab7e58d1d34c50ae7a3b441799761


this may be why the docs don't build? :)

Yes. Thanks. Seems I forgot to build the docs after merging...

nik9000 · 2018-06-13T12:25:09Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

@@ -490,7 +491,7 @@ static Request update(UpdateRequest updateRequest) throws IOException {
            XContentType upsertContentType = updateRequest.upsertRequest().getContentType();
            if ((xContentType != null) && (xContentType != upsertContentType)) {
                throw new IllegalStateException("Update request cannot have different content types for doc [" + xContentType + "]" +
-                        " and upsert [" + upsertContentType + "] documents");


Could you revert these formatting changes that aren't part the change? It make git blame harder to work with and I kind of like the old formatting better anyway.

nik9000 · 2018-06-13T12:25:57Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java

@@ -712,8 +713,8 @@ public void testSyncedFlush() {
        Request request = RequestConverters.flushSynced(syncedFlushRequest);
        StringJoiner endpoint = new StringJoiner("/", "/", "");
        if (indices != null && indices.length > 0) {
-                endpoint.add(String.join(",", indices));
-            }
+            endpoint.add(String.join(",", indices));


This formatting change is fine though, because the old code was just wrong.

I'm aware that this is somewhat contradictory.

nik9000 · 2018-06-13T12:27:54Z

...level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

@@ -244,17 +248,17 @@ public void testDeleteIndexAsync() throws Exception {

            // tag::delete-index-execute-listener
            ActionListener<DeleteIndexResponse> listener =
-                    new ActionListener<DeleteIndexResponse>() {


I think this change is fine but I'd prefer it in a separate PR.

nik9000 · 2018-06-13T12:28:15Z

...level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

@@ -287,7 +291,7 @@ public void testCreateIndex() throws IOException {
            {
                // tag::create-index-request-mappings
                request.mapping("tweet", // <1>
-                        "{\n" +
+                    "{\n" +


We want to keep the "s aligned.

nik9000 · 2018-06-13T12:29:55Z

...level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

@@ -378,21 +382,21 @@ public void testCreateIndex() throws IOException {
            request = new CreateIndexRequest("twitter6");
            // tag::create-index-whole-source
            request.source("{\n" +
-                    "    \"settings\" : {\n" +


Can you revert the formatting change? I think the old way is better. If we want to do it we should do it in a separate PR anyway.

nik9000 · 2018-06-13T12:30:26Z

...level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

@@ -1844,8 +1848,8 @@ public void testIndexPutSettings() throws Exception {
        {
            // tag::put-settings-settings-source
            request.settings(
-                    "{\"index.number_of_replicas\": \"2\"}"
-                    , XContentType.JSON); // <1>
+                "{\"index.number_of_replicas\": \"2\"}"


Can you revert all of these formatting changes above?

I think I messed up something during the merge from master and my editor did all these formats. I will revert everything back.

nik9000 · 2018-06-13T12:31:42Z

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

+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        if (getIndex() != null) {
+            builder.field(INDEX_FIELD, getIndex());


I tend to use the members rather than the getters for these but either way is ok by me!

I used to prefer that too. Then I saw in some classes that the getters also had some logic which I wanted to use. So I started using these...

If the getters have logic that you want that is a great reason to use them!

nik9000 · 2018-06-13T12:32:28Z

...rc/main/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryRequest.java

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject()


I think this'd be better formatted like:

builder.startObject(); builder.field("query"); query.toXContent(builder, params); return builder.endObject();

nik9000

LGTM. @hub-cap, would you like another look?

nik9000 · 2018-06-13T17:20:33Z

LGTM. @hub-cap, would you like another look?

Woops, not @hub-cap, @javanna.

…o validate_query

javanna

LGTM thanks @sohaibiftikhar

Adds the validate query API to the high level rest client.

nik9000 · 2018-06-18T14:26:46Z

Merged and backported! Thanks @sohaibiftikhar!

* 6.x: Add get stored script and delete stored script to high level REST API Increasing skip version for failing test on 6.x Skip get_alias tests for 5.x (#31397) Fix defaults in GeoShapeFieldMapper output (#31302) Test: better error message on failure Mute DefaultShardsIT#testDefaultShards test Fix reference to XContentBuilder.string() (#31337) [DOCS] Adds monitoring breaking change (#31369) [DOCS] Adds security breaking change (#31375) [DOCS] Backports breaking change (#31373) RestAPI: Reject forcemerge requests with a body (#30792) Docs: Use the default distribution to test docs (#31251) Use system context for cluster state update tasks (#31241) [DOCS] Adds testing for security APIs (#31345) [DOCS] Removes ML item from release highlights [DOCS] Removes breaking change (#31376) REST high-level client: add validate query API (#31077) Move language analyzers from server to analysis-common module. (#31300) Expose lucene's RemoveDuplicatesTokenFilter (#31275) [Test] Fix :example-plugins:rest-handler on Windows Delete typos in SAML docs (#31199) Ensure we don't use a remote profile if cluster name matches (#31331) Test: Skip alias tests that failed all weekend [DOCS] Fix version in SQL JDBC Maven template [DOCS] Improve install and setup section for SQL JDBC Add ingest-attachment support for per document `indexed_chars` limit (#31352) SQL: Fix rest endpoint names in node stats (#31371) [DOCS] Fixes small issue in release notes Support for remote path in reindex api Closes #22913 [ML] Put ML filter API response should contain the filter (#31362) Remove trial status info from start trial doc (#31365) [DOCS] Added links in breaking changes pages [DOCS] Adds links to release notes and highlights Docs: Document changes in rest client QA: Fix tribe tests to use node selector REST Client: NodeSelector for node attributes (#31296) LLClient: Fix assertion on windows LLClient: Support host selection (#30523) Add QA project and fixture based test for discovery-ec2 plugin (#31107) [ML] Hold ML filter items in sorted set (#31338) [ML] Add description to ML filters (#31330)

* master: Add get stored script and delete stored script to high level REST API - post backport fix Add get stored script and delete stored script to high level REST API (#31355) Core: Combine Action and GenericAction (#31405) Fix reference to XContentBuilder.string() (#31337) Avoid sending duplicate remote failed shard requests (#31313) Fix defaults in GeoShapeFieldMapper output (#31302) RestAPI: Reject forcemerge requests with a body (#30792) Packaging: Remove windows bin files from the tar distribution (#30596) Docs: Use the default distribution to test docs (#31251) [DOCS] Adds testing for security APIs (#31345) Clarify that IP range data can be specified in CIDR notation. (#31374) Use system context for cluster state update tasks (#31241) Percentile/Ranks should return null instead of NaN when empty (#30460) REST high-level client: add validate query API (#31077) Move language analyzers from server to analysis-common module. (#31300) [Test] Fix :example-plugins:rest-handler on Windows Expose lucene's RemoveDuplicatesTokenFilter (#31275) Reload secure settings for plugins (#31383) Remove some cases in FieldTypeLookupTests that are no longer relevant. (#31381) Ensure we don't use a remote profile if cluster name matches (#31331) [TEST] Double write alias fault (#30942) [DOCS] Fix version in SQL JDBC Maven template [DOCS] Improve install and setup section for SQL JDBC SQL: Fix rest endpoint names in node stats (#31371) Support for remote path in reindex api - post backport fix Closes #22913 [ML] Put ML filter API response should contain the filter (#31362) Support for remote path in reindex api (#31290) Add byte array pooling to nio http transport (#31349) Remove trial status info from start trial doc (#31365) [DOCS] Adds links to release notes and highlights add is-write-index flag to aliases (#30942) Add rollover-creation-date setting to rolled over index (#31144) [ML] Hold ML filter items in sorted set (#31338) [Tests] Fix edge case in ScriptedMetricAggregatorTests (#31357)

camerondavison · 2019-03-15T15:04:27Z

Is there a reason that some methods in this api such as

elasticsearch/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryRequest.java

Line 114 in 7a6021c

public void explain(boolean explain) {

do not return this ValidateQueryRequest

javanna · 2019-03-15T15:59:19Z

hi @camerondavison the reason is merely that newly added methods do not return the request, while old methods do. We would prefer to have pure setters in our codebase at this point, but we don't see the value in such a big breaking change, hence we only apply the preferred standard when adding new methods for now.

sohaibiftikhar commented Jun 4, 2018

View reviewed changes

nik9000 requested changes Jun 4, 2018

View reviewed changes

javanna mentioned this pull request Jun 4, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

jtibshirani added >enhancement v7.0.0 :Core/Java High Level REST Client v6.4.0 labels Jun 4, 2018

hub-cap reviewed Jun 4, 2018

View reviewed changes

sohaibiftikhar force-pushed the validate_query branch from 07ada05 to 29a318c Compare June 5, 2018 11:20

sohaibiftikhar added 4 commits June 7, 2018 14:13

added validate query API

6a6b61f

Relates to elastic#27205

Fixes for review

aad5341

-- Removed equals and hashCode from ValidateQueryResponse -- Extended AbstractBroadcastResponseTestCase instead of AbstractStreamableXContentTestCase -- Fixed asciidoc -- Added test for RequestConverters::validateQuery -- Added indicesOptions

removed changes to AbstractStreamableXContentTestCase.java

7bcc283

switched to RequestOptions from Headers

53b0747

sohaibiftikhar force-pushed the validate_query branch from ac65eb3 to 53b0747 Compare June 7, 2018 12:49

javanna requested changes Jun 8, 2018

View reviewed changes

sohaibiftikhar added 2 commits June 13, 2018 01:49

Fixes for review

2d8f6b3

-- added javadoc -- added documentation for setting query to request -- changed to Object.equals for QueryExplanation::equals -- extended ToXContentObject instead of ToXContentFragment for request

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

ed05533

…o validate_query

javanna reviewed Jun 13, 2018

View reviewed changes

Fixed client documentation

8c9cb3c

nik9000 reviewed Jun 13, 2018

View reviewed changes

Fixed formatting

e4da255

nik9000 approved these changes Jun 13, 2018

View reviewed changes

sohaibiftikhar added 3 commits June 14, 2018 02:43

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

1677ed6

…o validate_query

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

00e7169

…o validate_query

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

c5095db

…o validate_query

javanna approved these changes Jun 14, 2018

View reviewed changes

nik9000 merged commit c4f8df3 into elastic:master Jun 18, 2018

sohaibiftikhar deleted the validate_query branch June 18, 2018 14:12

nik9000 pushed a commit that referenced this pull request Jun 18, 2018

REST high-level client: add validate query API (#31077)

43b46d7

Adds the validate query API to the high level rest client.

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

		include::indices/get_templates.asciidoc[]
		>>>>>>> 0bfd18cc8b6ab7e58d1d34c50ae7a3b441799761

REST high-level client: add validate query API #31077

REST high-level client: add validate query API #31077

Conversation

sohaibiftikhar commented Jun 4, 2018

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

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

Choose a reason for hiding this comment

nik9000 commented Jun 4, 2018

nik9000 commented Jun 4, 2018

elasticmachine commented Jun 4, 2018

sohaibiftikhar commented Jun 4, 2018

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

sohaibiftikhar Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

sohaibiftikhar commented Jun 6, 2018

javanna 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

sohaibiftikhar Jun 8, 2018 • edited Loading

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

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

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

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

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 commented Jun 13, 2018

javanna left a comment

Choose a reason for hiding this comment

nik9000 commented Jun 18, 2018

camerondavison commented Mar 15, 2019

javanna commented Mar 15, 2019

sohaibiftikhar Jun 4, 2018 •

edited

Loading

sohaibiftikhar Jun 8, 2018 •

edited

Loading