Skip to content

Add "did you mean" to unknown queries #51177

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

Merged
merged 6 commits into from
Jan 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,19 @@
public class SuggestingErrorOnUnknown implements ErrorOnUnknown {
@Override
public String errorMessage(String parserName, String unknownField, Iterable<String> 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<String> candidates) {
// TODO it'd be nice to combine this with BaseRestHandler's implementation.
LevenshteinDistance ld = new LevenshteinDistance();
final List<Tuple<Float, String>> scored = new ArrayList<>();
Expand All @@ -43,7 +55,7 @@ public String errorMessage(String parserName, String unknownField, Iterable<Stri
}
}
if (scored.isEmpty()) {
return message;
return "";
}
CollectionUtil.timSort(scored, (a, b) -> {
// sort by distance in reverse order, then parameter name for equal distances
Expand All @@ -54,7 +66,7 @@ public String errorMessage(String parserName, String unknownField, Iterable<Stri
return a.v2().compareTo(b.v2());
});
List<String> 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 {
Expand All @@ -63,9 +75,4 @@ public String errorMessage(String parserName, String unknownField, Iterable<Stri
builder.append("?");
return builder.toString();
}

@Override
public int priority() {
return 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.common.lucene.BytesRefs;
import org.elasticsearch.common.xcontent.AbstractObjectParser;
import org.elasticsearch.common.xcontent.NamedObjectNotFoundException;
import org.elasticsearch.common.xcontent.SuggestingErrorOnUnknown;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentLocation;
import org.elasticsearch.common.xcontent.XContentParser;
Expand All @@ -41,6 +42,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;

Expand Down Expand Up @@ -313,10 +315,9 @@ public static QueryBuilder parseInnerQueryBuilder(XContentParser parser) throws
try {
result = parser.namedObject(QueryBuilder.class, queryName, null);
} catch (NamedObjectNotFoundException e) {
// Preserve the error message from 5.0 until we have a compellingly better message so we don't break BWC.
// This intentionally doesn't include the causing exception because that'd change the "root_cause" of any unknown query errors
throw new ParsingException(new XContentLocation(e.getLineNumber(), e.getColumnNumber()),
"no [query] registered for [" + queryName + "]");
String message = String.format(Locale.ROOT, "unknown query [%s]%s", queryName,
SuggestingErrorOnUnknown.suggest(queryName, e.getCandidates()));
throw new ParsingException(new XContentLocation(e.getLineNumber(), e.getColumnNumber()), message, e);
}
//end_object of the specific query (e.g. match, multi_match etc.) element
if (parser.currentToken() != XContentParser.Token.END_OBJECT) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,15 @@ public void testParseInnerQueryBuilderExceptions() throws IOException {
assertEquals("[foo] query malformed, no start_object after query name", exception.getMessage());
}

source = "{ \"foo\" : {} }";
source = "{ \"boool\" : {} }";
try (XContentParser parser = createParser(JsonXContent.jsonXContent, source)) {
ParsingException exception = expectThrows(ParsingException.class, () -> 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());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: >
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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