From eebda6974dea3445d5a4043a843d206046861470 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Tue, 27 Mar 2018 16:51:31 -0600 Subject: [PATCH] Decouple NamedXContentRegistry from ElasticsearchException (#29253) * Decouple NamedXContentRegistry from ElasticsearchException This commit decouples `NamedXContentRegistry` from using either `ElasticsearchException`, `ParsingException`, or `UnknownNamedObjectException`. This will allow us to move NamedXContentRegistry to its own lib as part of the xcontent extraction work. Relates to #28504 --- .../cluster/metadata/MetaData.java | 4 +- .../NamedObjectNotFoundException.java | 35 +++++++++++++ .../xcontent/NamedXContentRegistry.java | 19 +++---- .../xcontent/XContentParseException.java | 52 +++++++++++++++++++ .../index/query/AbstractQueryBuilder.java | 6 +-- .../common/xcontent/BaseXContentTestCase.java | 15 +++--- .../xcontent/XContentParserUtilsTests.java | 7 ++- .../rescore/QueryRescorerBuilderTests.java | 10 ++-- .../search/suggest/SuggestionTests.java | 6 +-- 9 files changed, 118 insertions(+), 36 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java create mode 100644 server/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 9fff294daea..a569bb9a36e 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -43,7 +43,7 @@ import org.elasticsearch.common.regex.Regex; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.xcontent.UnknownNamedObjectException; +import org.elasticsearch.common.xcontent.NamedObjectNotFoundException; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -1173,7 +1173,7 @@ public class MetaData implements Iterable, Diffable, To try { Custom custom = parser.namedObject(Custom.class, currentFieldName, null); builder.putCustom(custom.getWriteableName(), custom); - } catch (UnknownNamedObjectException ex) { + } catch (NamedObjectNotFoundException ex) { logger.warn("Skipping unknown custom object with type {}", currentFieldName); parser.skipChildren(); } diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java b/server/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java new file mode 100644 index 00000000000..ecc322b60d8 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/common/xcontent/NamedObjectNotFoundException.java @@ -0,0 +1,35 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.xcontent; + +/** + * Thrown when {@link NamedXContentRegistry} cannot locate a named object to + * parse for a particular name + */ +public class NamedObjectNotFoundException extends XContentParseException { + + public NamedObjectNotFoundException(String message) { + this(null, message); + } + + public NamedObjectNotFoundException(XContentLocation location, String message) { + super(location, message); + } +} diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java b/server/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java index c19a667776f..9135bf648a1 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java @@ -19,10 +19,8 @@ package org.elasticsearch.common.xcontent; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.CheckedFunction; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.ParsingException; import java.io.IOException; import java.util.ArrayList; @@ -114,28 +112,31 @@ public class NamedXContentRegistry { } /** - * Parse a named object, throwing an exception if the parser isn't found. Throws an {@link ElasticsearchException} if the - * {@code categoryClass} isn't registered because this is almost always a bug. Throws a {@link UnknownNamedObjectException} if the + * Parse a named object, throwing an exception if the parser isn't found. Throws an {@link NamedObjectNotFoundException} if the + * {@code categoryClass} isn't registered because this is almost always a bug. Throws an {@link NamedObjectNotFoundException} if the * {@code categoryClass} is registered but the {@code name} isn't. + * + * @throws NamedObjectNotFoundException if the categoryClass or name is not registered */ public T parseNamedObject(Class categoryClass, String name, XContentParser parser, C context) throws IOException { Map parsers = registry.get(categoryClass); if (parsers == null) { if (registry.isEmpty()) { // The "empty" registry will never work so we throw a better exception as a hint. - throw new ElasticsearchException("namedObject is not supported for this parser"); + throw new NamedObjectNotFoundException("named objects are not supported for this parser"); } - throw new ElasticsearchException("Unknown namedObject category [" + categoryClass.getName() + "]"); + throw new NamedObjectNotFoundException("unknown named object category [" + categoryClass.getName() + "]"); } Entry entry = parsers.get(name); if (entry == null) { - throw new UnknownNamedObjectException(parser.getTokenLocation(), categoryClass, name); + throw new NamedObjectNotFoundException(parser.getTokenLocation(), "unable to parse " + categoryClass.getSimpleName() + + " with name [" + name + "]: parser not found"); } 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 ParsingException(parser.getTokenLocation(), - "Unknown " + categoryClass.getSimpleName() + " [" + name + "]: Parser didn't match"); + throw new NamedObjectNotFoundException(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/server/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java new file mode 100644 index 00000000000..cd2e3dbb59b --- /dev/null +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentParseException.java @@ -0,0 +1,52 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.xcontent; + +import java.util.Optional; + +/** + * Thrown when one of the XContent parsers cannot parse something. + */ +public class XContentParseException extends IllegalArgumentException { + + private final Optional location; + + public XContentParseException(String message) { + this(null, message); + } + + public XContentParseException(XContentLocation location, String message) { + super(message); + this.location = Optional.ofNullable(location); + } + + public int getLineNumber() { + return location.map(l -> l.lineNumber).orElse(-1); + } + + public int getColumnNumber() { + return location.map(l -> l.columnNumber).orElse(-1); + } + + @Override + public String getMessage() { + return location.map(l -> "[" + l.toString() + "] ").orElse("") + super.getMessage(); + } +} diff --git a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java index d272bb29fbf..942c72f2293 100644 --- a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -31,7 +31,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.lucene.BytesRefs; import org.elasticsearch.common.xcontent.AbstractObjectParser; -import org.elasticsearch.common.xcontent.UnknownNamedObjectException; +import org.elasticsearch.common.xcontent.NamedObjectNotFoundException; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentLocation; import org.elasticsearch.common.xcontent.XContentParser; @@ -316,11 +316,11 @@ public abstract class AbstractQueryBuilder> QueryBuilder result; try { result = parser.namedObject(QueryBuilder.class, queryName, null); - } catch (UnknownNamedObjectException e) { + } 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 [" + e.getName() + "]"); + "no [query] registered for [" + queryName + "]"); } //end_object of the specific query (e.g. match, multi_match etc.) element if (parser.currentToken() != XContentParser.Token.END_OBJECT) { 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 4efd5f480f8..b46485952d7 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java @@ -67,6 +67,7 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; @@ -1007,22 +1008,20 @@ public abstract class BaseXContentTestCase extends ESTestCase { { p.nextToken(); assertEquals("test", p.namedObject(Object.class, "str", null)); - UnknownNamedObjectException e = expectThrows(UnknownNamedObjectException.class, + NamedObjectNotFoundException e = expectThrows(NamedObjectNotFoundException.class, () -> p.namedObject(Object.class, "unknown", null)); - assertEquals("Unknown Object [unknown]", e.getMessage()); - assertEquals("java.lang.Object", e.getCategoryClass()); - assertEquals("unknown", e.getName()); + assertThat(e.getMessage(), endsWith("unable to parse Object with name [unknown]: parser not found")); } { - Exception e = expectThrows(ElasticsearchException.class, () -> p.namedObject(String.class, "doesn't matter", null)); - assertEquals("Unknown namedObject category [java.lang.String]", e.getMessage()); + Exception e = expectThrows(NamedObjectNotFoundException.class, () -> p.namedObject(String.class, "doesn't matter", null)); + assertEquals("unknown named object category [java.lang.String]", e.getMessage()); } { XContentParser emptyRegistryParser = xcontentType().xContent() .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, new byte[] {}); - Exception e = expectThrows(ElasticsearchException.class, + Exception e = expectThrows(NamedObjectNotFoundException.class, () -> emptyRegistryParser.namedObject(String.class, "doesn't matter", null)); - assertEquals("namedObject is not supported for this parser", e.getMessage()); + 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 e31a1ce7202..5b65e6af789 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentParserUtilsTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentParserUtilsTests.java @@ -40,6 +40,7 @@ import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpect import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureFieldName; import static org.elasticsearch.common.xcontent.XContentParserUtils.parseTypedKeysObject; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.instanceOf; public class XContentParserUtilsTests extends ESTestCase { @@ -187,11 +188,9 @@ public class XContentParserUtilsTests extends ESTestCase { ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation); ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.nextToken(), parser::getTokenLocation); ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation); - UnknownNamedObjectException e = expectThrows(UnknownNamedObjectException.class, + NamedObjectNotFoundException e = expectThrows(NamedObjectNotFoundException.class, () -> parseTypedKeysObject(parser, delimiter, Boolean.class, a -> {})); - assertEquals("Unknown Boolean [type]", e.getMessage()); - assertEquals("type", e.getName()); - assertEquals("java.lang.Boolean", e.getCategoryClass()); + assertThat(e.getMessage(), endsWith("unable to parse Boolean with name [type]: parser not found")); } final long longValue = randomLong(); 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 2f35a832c30..9a9797734b6 100644 --- a/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java @@ -19,15 +19,14 @@ package org.elasticsearch.search.rescore; -import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.NamedObjectNotFoundException; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -40,9 +39,7 @@ import org.elasticsearch.index.mapper.ContentPath; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.TextFieldMapper; -import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.MatchAllQueryBuilder; -import org.elasticsearch.index.query.MatchQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.QueryShardContext; @@ -58,7 +55,6 @@ import java.io.IOException; import static java.util.Collections.emptyList; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; -import static org.hamcrest.Matchers.containsString; public class QueryRescorerBuilderTests extends ESTestCase { @@ -220,8 +216,8 @@ public class QueryRescorerBuilderTests extends ESTestCase { "}\n"; { XContentParser parser = createParser(rescoreElement); - Exception e = expectThrows(ParsingException.class, () -> RescorerBuilder.parseFromXContent(parser)); - assertEquals("Unknown RescorerBuilder [bad_rescorer_name]", e.getMessage()); + 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()); } rescoreElement = "{\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 7a57d2c3e67..c8384a948a6 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/SuggestionTests.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/SuggestionTests.java @@ -19,10 +19,10 @@ package org.elasticsearch.search.suggest; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.text.Text; import org.elasticsearch.common.xcontent.DeprecationHandler; +import org.elasticsearch.common.xcontent.NamedObjectNotFoundException; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContent; @@ -180,8 +180,8 @@ public class SuggestionTests extends ESTestCase { ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation); ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.nextToken(), parser::getTokenLocation); ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.nextToken(), parser::getTokenLocation); - ParsingException e = expectThrows(ParsingException.class, () -> Suggestion.fromXContent(parser)); - assertEquals("Unknown Suggestion [unknownType]", e.getMessage()); + NamedObjectNotFoundException e = expectThrows(NamedObjectNotFoundException.class, () -> Suggestion.fromXContent(parser)); + assertEquals("[1:31] unable to parse Suggestion with name [unknownType]: parser not found", e.getMessage()); } }