Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -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;
Expand Down Expand Up @@ -114,28 +112,31 @@ public NamedXContentRegistry(List<Entry> entries) {
}

/**
* 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 XContentParseException} if the
* {@code categoryClass} isn't registered because this is almost always a bug. Throws an {@link XContentParseException} if the
* {@code categoryClass} is registered but the {@code name} isn't.
*
* @throws XContentParseException if the categoryClass or name is not registered
*/
public <T, C> T parseNamedObject(Class<T> categoryClass, String name, XContentParser parser, C context) throws IOException {
Map<String, Entry> 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 XContentParseException("named objects are not supported for this parser");
}
throw new ElasticsearchException("Unknown namedObject category [" + categoryClass.getName() + "]");
throw new XContentParseException("unknown named object category [" + categoryClass.getName() + "]");
}
Entry entry = parsers.get(name);
if (entry == null) {
throw new UnknownNamedObjectException(parser.getTokenLocation(), categoryClass, name);
throw new XContentParseException(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 XContentParseException(parser.getTokenLocation(),
"unable to parse " + categoryClass.getSimpleName() + " with name [" + name + "]: parser didn't match");
}
return categoryClass.cast(entry.parser.parse(parser, context));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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<XContentLocation> 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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.elasticsearch.common.xcontent.UnknownNamedObjectException;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentLocation;
import org.elasticsearch.common.xcontent.XContentParseException;
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
Expand Down Expand Up @@ -316,11 +317,11 @@ public static QueryBuilder parseInnerQueryBuilder(XContentParser parser) throws
QueryBuilder result;
try {
result = parser.namedObject(QueryBuilder.class, queryName, null);
} catch (UnknownNamedObjectException e) {
} catch (XContentParseException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to use this XContentParseException for anything other that unknown named objects we shouldn't catch it here. We need to be sure this is an unknown named object. Either we should rename the exception or make a subclass of it for unknown named objects, I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's probably a good idea, for now there are no other uses of this exception, but if that changes in the future I will make a subclass of it for this.

// 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
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;
Expand Down Expand Up @@ -1024,22 +1025,20 @@ public void testNamedObject() throws IOException {
{
p.nextToken();
assertEquals("test", p.namedObject(Object.class, "str", null));
UnknownNamedObjectException e = expectThrows(UnknownNamedObjectException.class,
XContentParseException e = expectThrows(XContentParseException.class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be NamedObejectNotFoundException instead, right? It is more specific.

() -> 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(XContentParseException.class, () -> p.namedObject(String.class, "doesn't matter", null));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too

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(XContentParseException.class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here!

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
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 {
Expand Down Expand Up @@ -187,11 +188,9 @@ public void testParseTypedKeysObject() throws IOException {
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,
XContentParseException e = expectThrows(XContentParseException.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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParseException;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.common.xcontent.json.JsonXContent;
Expand Down Expand Up @@ -220,8 +221,8 @@ public void testUnknownFieldsExpection() throws IOException {
"}\n";
{
XContentParser parser = createParser(rescoreElement);
Exception e = expectThrows(ParsingException.class, () -> RescorerBuilder.parseFromXContent(parser));
assertEquals("Unknown RescorerBuilder [bad_rescorer_name]", e.getMessage());
Exception e = expectThrows(XContentParseException.class, () -> RescorerBuilder.parseFromXContent(parser));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these should be NamedObjectNotFoundException I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I changed all the occurences, thanks

assertEquals("[3:27] unable to parse RescorerBuilder with name [bad_rescorer_name]: parser not found", e.getMessage());
}

rescoreElement = "{\n" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentParseException;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.common.xcontent.json.JsonXContent;
Expand Down Expand Up @@ -180,8 +181,8 @@ public void testUnknownSuggestionTypeThrows() throws IOException {
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());
XContentParseException e = expectThrows(XContentParseException.class, () -> Suggestion.fromXContent(parser));
assertEquals("[1:31] unable to parse Suggestion with name [unknownType]: parser not found", e.getMessage());
}
}

Expand Down