From 5299664ae345a6d5244343964b4c222e54bb5b44 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 17 Jan 2020 12:00:03 -0500 Subject: [PATCH] "did you mean" for ObjectParser with top named (#51018) (#51165) When you declare an ObjectParser with top level named objects like we do with `significant_terms` we didn't support "did you mean". This fixes that. Relates #50938 --- .../NamedObjectNotFoundException.java | 13 ++++++++---- .../xcontent/NamedXContentRegistry.java | 9 ++++---- .../common/xcontent/ObjectParser.java | 8 +++++-- .../xcontent/XContentParseException.java | 9 +------- .../common/xcontent/ObjectParserTests.java | 5 ++++- .../test/search.aggregation/30_sig_terms.yml | 15 +++++++++++++ .../common/xcontent/BaseXContentTestCase.java | 9 +++++--- .../xcontent/XContentParserUtilsTests.java | 2 +- .../gateway/MetaDataStateFormatTests.java | 21 ++++++++++++++++++- .../rescore/QueryRescorerBuilderTests.java | 2 +- .../search/suggest/SuggestionTests.java | 2 +- 11 files changed, 68 insertions(+), 27 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java index ecc322b60d8..eea323ad7f3 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java @@ -24,12 +24,17 @@ package org.elasticsearch.common.xcontent; * parse for a particular name */ public class NamedObjectNotFoundException extends XContentParseException { + private final Iterable candidates; - public NamedObjectNotFoundException(String message) { - this(null, message); + public NamedObjectNotFoundException(XContentLocation location, String message, Iterable candidates) { + super(location, message); + this.candidates = candidates; } - public NamedObjectNotFoundException(XContentLocation location, String message) { - super(location, message); + /** + * The possible matches. + */ + public Iterable getCandidates() { + return candidates; } } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java index 9135bf648a1..9efe568955f 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java @@ -123,19 +123,18 @@ public class NamedXContentRegistry { if (parsers == null) { if (registry.isEmpty()) { // The "empty" registry will never work so we throw a better exception as a hint. - throw new NamedObjectNotFoundException("named objects are not supported for this parser"); + throw new XContentParseException("named objects are not supported for this parser"); } - throw new NamedObjectNotFoundException("unknown named object category [" + categoryClass.getName() + "]"); + throw new XContentParseException("unknown named object category [" + categoryClass.getName() + "]"); } Entry entry = parsers.get(name); if (entry == null) { - throw new NamedObjectNotFoundException(parser.getTokenLocation(), "unable to parse " + categoryClass.getSimpleName() + - " with name [" + name + "]: parser not found"); + throw new NamedObjectNotFoundException(parser.getTokenLocation(), "unknown field [" + name + "]", parsers.keySet()); } if (false == entry.name.match(name, parser.getDeprecationHandler())) { /* Note that this shouldn't happen because we already looked up the entry using the names but we need to call `match` anyway * because it is responsible for logging deprecation warnings. */ - throw new NamedObjectNotFoundException(parser.getTokenLocation(), + throw new XContentParseException(parser.getTokenLocation(), "unable to parse " + categoryClass.getSimpleName() + " with name [" + name + "]: parser didn't match"); } return categoryClass.cast(entry.parser.parse(parser, context)); diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index 478b4c13694..30810637044 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -27,8 +27,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.EnumSet; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Consumer; @@ -140,8 +142,10 @@ public final class ObjectParser extends AbstractObjectParser candidates = new HashSet<>(objectParser.fieldParserMap.keySet()); + e.getCandidates().forEach(candidates::add); + String message = ErrorOnUnknown.IMPLEMENTATION.errorMessage(objectParser.name, field, candidates); + throw new XContentParseException(location, message, e); } consumer.accept(value, o); }; diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java index cf9d4ed6713..8a7a10d6ebb 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java @@ -59,13 +59,6 @@ public class XContentParseException extends IllegalArgumentException { @Override public String getMessage() { - return location.map(l -> "[" + l.toString() + "] ").orElse("") + getBareMessage(); - } - - /** - * Get the exception message without location information. - */ - public String getBareMessage() { - return super.getMessage(); + return location.map(l -> "[" + l.toString() + "] ").orElse("") + super.getMessage(); } } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java index 2c926debf84..8d9503ce2a7 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java @@ -38,6 +38,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; @@ -804,7 +805,9 @@ public class ObjectParserTests extends ESTestCase { { XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"not_supported_field\" : \"foo\"}"); XContentParseException ex = expectThrows(XContentParseException.class, () -> TopLevelNamedXConent.PARSER.parse(parser, null)); - assertEquals("[1:2] [test] unable to parse Object with name [not_supported_field]: parser not found", ex.getMessage()); + assertEquals("[1:2] [test] unknown field [not_supported_field]", ex.getMessage()); + NamedObjectNotFoundException cause = (NamedObjectNotFoundException) ex.getCause(); + assertThat(cause.getCandidates(), containsInAnyOrder("str", "int", "float", "bool")); } } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/30_sig_terms.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/30_sig_terms.yml index 99d0f29a9b2..5204d65d333 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/30_sig_terms.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/30_sig_terms.yml @@ -137,3 +137,18 @@ search: rest_total_hits_as_int: true body: { "size" : 0, "aggs" : { "ip_terms" : { "significant_terms" : { "field" : "ip", "exclude" : "127.*" } } } } + +--- +'Misspelled fields get "did you mean"': + - skip: + version: " - 7.99.99" + reason: Implemented in 8.0 (to be backported to 7.7) + - do: + catch: /\[significant_terms\] unknown field \[jlp\] did you mean \[jlh\]\?/ + search: + body: + aggs: + foo: + significant_terms: + field: foo + jlp: {} diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java b/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java index 4c2fcfbb589..860adf2c233 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java @@ -22,6 +22,7 @@ package org.elasticsearch.common.xcontent; import com.fasterxml.jackson.core.JsonGenerationException; import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.core.JsonParseException; + import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.Constants; import org.elasticsearch.cluster.metadata.IndexMetaData; @@ -78,6 +79,7 @@ import java.util.concurrent.TimeUnit; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.equalTo; @@ -1169,16 +1171,17 @@ public abstract class BaseXContentTestCase extends ESTestCase { { NamedObjectNotFoundException e = expectThrows(NamedObjectNotFoundException.class, () -> p.namedObject(Object.class, "unknown", null)); - assertThat(e.getMessage(), endsWith("unable to parse Object with name [unknown]: parser not found")); + assertThat(e.getMessage(), endsWith("unknown field [unknown]")); + assertThat(e.getCandidates(), containsInAnyOrder("test1", "test2", "deprecated", "str")); } { - Exception e = expectThrows(NamedObjectNotFoundException.class, () -> p.namedObject(String.class, "doesn't matter", null)); + Exception e = expectThrows(XContentParseException.class, () -> p.namedObject(String.class, "doesn't matter", null)); assertEquals("unknown named object category [java.lang.String]", e.getMessage()); } } try (XContentParser emptyRegistryParser = xcontentType().xContent() .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, new byte[] {})) { - Exception e = expectThrows(NamedObjectNotFoundException.class, + Exception e = expectThrows(XContentParseException.class, () -> emptyRegistryParser.namedObject(String.class, "doesn't matter", null)); assertEquals("named objects are not supported for this parser", e.getMessage()); } diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/XContentParserUtilsTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/XContentParserUtilsTests.java index 5b65e6af789..db3dabdcafe 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentParserUtilsTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentParserUtilsTests.java @@ -190,7 +190,7 @@ public class XContentParserUtilsTests extends ESTestCase { ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation); NamedObjectNotFoundException e = expectThrows(NamedObjectNotFoundException.class, () -> parseTypedKeysObject(parser, delimiter, Boolean.class, a -> {})); - assertThat(e.getMessage(), endsWith("unable to parse Boolean with name [type]: parser not found")); + assertThat(e.getMessage(), endsWith("unknown field [type]")); } final long longValue = randomLong(); diff --git a/server/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTests.java index c7dab0dc4d4..c2f04a8bd03 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTests.java @@ -30,9 +30,12 @@ import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.Version; +import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.cluster.metadata.IndexGraveyard; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.cluster.metadata.RepositoriesMetaData; +import org.elasticsearch.common.ParseField; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -277,7 +280,7 @@ public class MetaDataStateFormatTests extends ESTestCase { List dirList = Arrays.asList(dirs); Collections.shuffle(dirList, random()); MetaData loadedMetaData = format.loadLatestState(logger, hasMissingCustoms ? - NamedXContentRegistry.EMPTY : xContentRegistry(), dirList.toArray(new Path[0])); + xContentRegistryWithMissingCustoms() : xContentRegistry(), dirList.toArray(new Path[0])); MetaData latestMetaData = meta.get(numStates-1); assertThat(loadedMetaData.clusterUUID(), not(equalTo("_na_"))); assertThat(loadedMetaData.clusterUUID(), equalTo(latestMetaData.clusterUUID())); @@ -706,4 +709,20 @@ public class MetaDataStateFormatTests extends ESTestCase { } return realId + 1; } + + /** + * The {@link NamedXContentRegistry} to use for most {@link XContentParser} in this test. + */ + protected final NamedXContentRegistry xContentRegistry() { + return new NamedXContentRegistry(ClusterModule.getNamedXWriteables()); + } + + /** + * The {@link NamedXContentRegistry} to use for {@link XContentParser}s that should be + * missing all of the normal cluster state parsers. + */ + protected NamedXContentRegistry xContentRegistryWithMissingCustoms() { + return new NamedXContentRegistry(Arrays.asList( + new NamedXContentRegistry.Entry(MetaData.Custom.class, new ParseField("garbage"), RepositoriesMetaData::fromXContent))); + } } diff --git a/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java b/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java index 82616b4c22a..1f0394216e0 100644 --- a/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java @@ -223,7 +223,7 @@ public class QueryRescorerBuilderTests extends ESTestCase { "}\n"; try (XContentParser parser = createParser(rescoreElement)) { Exception e = expectThrows(NamedObjectNotFoundException.class, () -> RescorerBuilder.parseFromXContent(parser)); - assertEquals("[3:27] unable to parse RescorerBuilder with name [bad_rescorer_name]: parser not found", e.getMessage()); + assertEquals("[3:27] unknown field [bad_rescorer_name]", e.getMessage()); } rescoreElement = "{\n" + " \"bad_fieldName\" : 20\n" + diff --git a/server/src/test/java/org/elasticsearch/search/suggest/SuggestionTests.java b/server/src/test/java/org/elasticsearch/search/suggest/SuggestionTests.java index 36e49933ed5..e53aab10b9d 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/SuggestionTests.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/SuggestionTests.java @@ -187,7 +187,7 @@ public class SuggestionTests extends ESTestCase { ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.nextToken(), parser::getTokenLocation); ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.nextToken(), parser::getTokenLocation); NamedObjectNotFoundException e = expectThrows(NamedObjectNotFoundException.class, () -> Suggestion.fromXContent(parser)); - assertEquals("[1:31] unable to parse Suggestion with name [unknownType]: parser not found", e.getMessage()); + assertEquals("[1:31] unknown field [unknownType]", e.getMessage()); } }