From 7bf26e35297032bbe157df3ff8baf2a0e4e69e8b Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 20 Jan 2020 15:24:59 -0500 Subject: [PATCH] Add "did you mean" to unknown queries (#51177) This replaces the message we return for unknown queries with the standard one that we use for unknown fields from `ObjectParser`. This is nice because it includes "did you mean". One day we might convert parsing queries to using object parser, but that looks complex. This change is much smaller and seems useful. --- .../50_multi_search_template.yml | 2 +- .../test/indices.validate_query/10_basic.yml | 16 ++++++++++++- .../xcontent/SuggestingErrorOnUnknown.java | 23 ++++++++++++------- .../index/query/AbstractQueryBuilder.java | 9 ++++---- .../query/AbstractQueryBuilderTests.java | 9 ++++++-- .../index/query/BoolQueryBuilderTests.java | 2 +- .../template/SimpleIndexTemplateIT.java | 2 +- .../rest-api-spec/test/ml/datafeeds_crud.yml | 2 +- .../watcher/execute_watch/20_transform.yml | 2 +- 9 files changed, 47 insertions(+), 20 deletions(-) diff --git a/modules/lang-mustache/src/test/resources/rest-api-spec/test/lang_mustache/50_multi_search_template.yml b/modules/lang-mustache/src/test/resources/rest-api-spec/test/lang_mustache/50_multi_search_template.yml index a5072d529b9b5..f4dfda0a72ec5 100644 --- a/modules/lang-mustache/src/test/resources/rest-api-spec/test/lang_mustache/50_multi_search_template.yml +++ b/modules/lang-mustache/src/test/resources/rest-api-spec/test/lang_mustache/50_multi_search_template.yml @@ -100,7 +100,7 @@ setup: - match: { responses.1.error.root_cause.0.reason: "/Unexpected.end.of.input/" } - match: { responses.2.hits.total: 1 } - match: { responses.3.error.root_cause.0.type: parsing_exception } - - match: { responses.3.error.root_cause.0.reason: "/no.\\[query\\].registered.for.\\[unknown\\]/" } + - match: { responses.3.error.root_cause.0.reason: "/unknown.query.\\[unknown\\]/" } --- "Multi-search template with invalid request": diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.validate_query/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.validate_query/10_basic.yml index 29b1e664d61c0..88187aa201a28 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.validate_query/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.validate_query/10_basic.yml @@ -12,6 +12,10 @@ setup: --- "Validate query api": + - skip: + version: ' - 7.99.99' + reason: message changed in 8.0.0 + - do: indices.validate_query: q: query string @@ -43,7 +47,17 @@ setup: invalid_query: {} - is_false: valid - - match: {error: 'org.elasticsearch.common.ParsingException: no [query] registered for [invalid_query]'} + - match: {error: '/.+unknown\squery\s\[invalid_query\].+/' } + + - do: + indices.validate_query: + explain: true + body: + query: + boool: {} + + - is_false: valid + - match: {error: '/.+unknown\squery\s\[boool\]\sdid\syou\smean\s\[bool\]\?.+/' } - do: indices.validate_query: diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/SuggestingErrorOnUnknown.java b/server/src/main/java/org/elasticsearch/common/xcontent/SuggestingErrorOnUnknown.java index c24f74ef814a7..231faa61b38c6 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/SuggestingErrorOnUnknown.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/SuggestingErrorOnUnknown.java @@ -32,7 +32,19 @@ public class SuggestingErrorOnUnknown implements ErrorOnUnknown { @Override public String errorMessage(String parserName, String unknownField, Iterable candidates) { - String message = String.format(Locale.ROOT, "[%s] unknown field [%s]", parserName, unknownField); + return String.format(Locale.ROOT, "[%s] unknown field [%s]%s", parserName, unknownField, suggest(unknownField, candidates)); + } + + @Override + public int priority() { + return 0; + } + + /** + * Builds suggestions for an unknown field, returning an empty string if there + * aren't any suggestions or " did you mean " and then the list of suggestions. + */ + public static String suggest(String unknownField, Iterable candidates) { // TODO it'd be nice to combine this with BaseRestHandler's implementation. LevenshteinDistance ld = new LevenshteinDistance(); final List> scored = new ArrayList<>(); @@ -43,7 +55,7 @@ public String errorMessage(String parserName, String unknownField, Iterable { // sort by distance in reverse order, then parameter name for equal distances @@ -54,7 +66,7 @@ public String errorMessage(String parserName, String unknownField, Iterable keys = scored.stream().map(Tuple::v2).collect(toList()); - StringBuilder builder = new StringBuilder(message).append(" did you mean "); + StringBuilder builder = new StringBuilder(" did you mean "); if (keys.size() == 1) { builder.append("[").append(keys.get(0)).append("]"); } else { @@ -63,9 +75,4 @@ public String errorMessage(String parserName, String unknownField, Iterable parseInnerQueryBuilder(parser)); - assertEquals("no [query] registered for [foo]", exception.getMessage()); + assertEquals("unknown query [boool] did you mean [bool]?", exception.getMessage()); + } + source = "{ \"match_\" : {} }"; + try (XContentParser parser = createParser(JsonXContent.jsonXContent, source)) { + ParsingException exception = expectThrows(ParsingException.class, () -> parseInnerQueryBuilder(parser)); + assertEquals("unknown query [match_] did you mean any of [match, match_all, match_none]?", exception.getMessage()); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java index 1a1609fe7c50e..97aa6f0d40589 100644 --- a/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java @@ -284,7 +284,7 @@ public void testUnknownQueryName() throws IOException { String query = "{\"bool\" : {\"must\" : { \"unknown_query\" : { } } } }"; ParsingException ex = expectThrows(ParsingException.class, () -> parseQuery(query)); - assertEquals("no [query] registered for [unknown_query]", ex.getMessage()); + assertEquals("unknown query [unknown_query]", ex.getMessage()); } public void testRewrite() throws IOException { diff --git a/server/src/test/java/org/elasticsearch/indices/template/SimpleIndexTemplateIT.java b/server/src/test/java/org/elasticsearch/indices/template/SimpleIndexTemplateIT.java index ce708d47d1722..e1ce312b7f674 100644 --- a/server/src/test/java/org/elasticsearch/indices/template/SimpleIndexTemplateIT.java +++ b/server/src/test/java/org/elasticsearch/indices/template/SimpleIndexTemplateIT.java @@ -533,7 +533,7 @@ public void testAliasInvalidFilterValidJson() throws Exception { () -> createIndex("test")); assertThat(e.getMessage(), equalTo("failed to parse filter for alias [invalid_alias]")); assertThat(e.getCause(), instanceOf(ParsingException.class)); - assertThat(e.getCause().getMessage(), equalTo("no [query] registered for [invalid]")); + assertThat(e.getCause().getMessage(), equalTo("unknown query [invalid]")); } public void testAliasInvalidFilterInvalidJson() throws Exception { diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/datafeeds_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/datafeeds_crud.yml index 8712342951c46..da4dfe7ec23d4 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/datafeeds_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/datafeeds_crud.yml @@ -160,7 +160,7 @@ setup: --- "Test put datafeed with invalid query": - do: - catch: /parsing_exception/ + catch: /failed\sto\sparse\sfield\s\[query\]/ ml.put_datafeed: datafeed_id: test-datafeed-1 body: > diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/watcher/execute_watch/20_transform.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/watcher/execute_watch/20_transform.yml index fa0793378756e..c1d5f3482a972 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/watcher/execute_watch/20_transform.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/watcher/execute_watch/20_transform.yml @@ -188,5 +188,5 @@ setup: - match: { watch_record.state: "executed" } - match: { watch_record.status.execution_state: "executed" } - match: { watch_record.result.transform.status: "failure" } - - match: { watch_record.result.transform.reason: "no [query] registered for [does_not_exist]" } + - match: { watch_record.result.transform.reason: "unknown query [does_not_exist]" } - is_true: watch_record.result.transform.error