"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
This commit is contained in:
Nik Everett 2020-01-17 12:00:03 -05:00 committed by GitHub
parent 4fc865e579
commit 5299664ae3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 68 additions and 27 deletions

View File

@ -24,12 +24,17 @@ package org.elasticsearch.common.xcontent;
* parse for a particular name
*/
public class NamedObjectNotFoundException extends XContentParseException {
private final Iterable<String> candidates;
public NamedObjectNotFoundException(String message) {
this(null, message);
public NamedObjectNotFoundException(XContentLocation location, String message, Iterable<String> candidates) {
super(location, message);
this.candidates = candidates;
}
public NamedObjectNotFoundException(XContentLocation location, String message) {
super(location, message);
/**
* The possible matches.
*/
public Iterable<String> getCandidates() {
return candidates;
}
}

View File

@ -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));

View File

@ -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<Value, Context> extends AbstractObjectParser<Val
try {
o = parser.namedObject(categoryClass, field, context);
} catch (NamedObjectNotFoundException e) {
// TODO It'd be lovely if we could the options here but we don't have the right stuff plumbed through. We'll get to it!
throw new XContentParseException(location, "[" + objectParser.name + "] " + e.getBareMessage(), e);
Set<String> 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);
};

View File

@ -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();
}
}

View File

@ -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"));
}
}

View File

@ -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: {}

View File

@ -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());
}

View File

@ -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();

View File

@ -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<Path> 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)));
}
}

View File

@ -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" +

View File

@ -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());
}
}