Skip to content
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

Inconsistent exception types thrown while parsing XContent #27147

Closed
olcbean opened this issue Oct 27, 2017 · 6 comments
Closed

Inconsistent exception types thrown while parsing XContent #27147

olcbean opened this issue Oct 27, 2017 · 6 comments

Comments

@olcbean
Copy link
Contributor

@olcbean olcbean commented Oct 27, 2017

In most cases, if the expected START_OBJECT is not detected, an IllegalArgumentException is thrown :

public static void ensureExpectedToken(Token expected, Token actual, Supplier<XContentLocation> location) {
if (actual != expected) {
String message = "Failed to parse object: expecting token of type [%s] but found [%s]";
throw new ParsingException(location.get(), String.format(Locale.ROOT, message, expected, actual));
}
}

However ObjectParser throws IllegalStateException :

if (token != XContentParser.Token.START_OBJECT) {
throw new IllegalStateException("[" + name + "] Expected START_OBJECT but was: " + token);
}

When parsing mget two different exception types may be thrown :
ParsingException

if ((token = parser.nextToken()) != XContentParser.Token.START_OBJECT) {
final String message = String.format(
Locale.ROOT,
"unexpected token [%s], expected [%s]",
token,
XContentParser.Token.START_OBJECT);
throw new ParsingException(parser.getTokenLocation(), message);
}

IllegalArgumentException

if (token != XContentParser.Token.START_OBJECT) {
throw new IllegalArgumentException("docs array element should include an object");
}

while RepositoryData consistently throws ElasticsearchParseException :

throw new ElasticsearchParseException("start object expected");


I think that it would be much cleaner if only one exception type is used (at least from now on).

For example what exception type should be thrown when a stricter validation of query field names is introduced (#26013). A different one per request (based on the exception thrown till now in the request)? Or an exception of type X?

Or maybe some of the exception types should be changed? For example can IllegalStateException be replaced by IllegalArgumentException?

@javanna

This comment has been minimized.

Copy link
Member

@javanna javanna commented Oct 31, 2017

I think that ParsingException is the way to go , as it also includes the position where the problem was found in the original input. IllegalArgumentException sounds ok but it doesn't have the position info which is very useful. IllegalStateException seems wrong to me in this context.

@jasontedor jasontedor removed the discuss label Nov 3, 2017
@olcbean

This comment has been minimized.

Copy link
Contributor Author

@olcbean olcbean commented Nov 4, 2017

Thanks @javanna! So we should stick to the ParsingException in general.
Actually one more question : shouldn't the ParsingException be actually an IllegalArgumentException?

@javanna

This comment has been minimized.

Copy link
Member

@javanna javanna commented Nov 6, 2017

Yes let's stick to ParsingException

shouldn't the ParsingException be actually an IllegalArgumentException

it could be, I guess it is convenient to subclass ElasticsearchException so that a few common methods around serialization (transport and REST) can be inherited. I do not see an advantage in changing this at this point.

@liketic

This comment has been minimized.

Copy link
Contributor

@liketic liketic commented Nov 7, 2017

Hi @olcbean @javanna I found this issue is marked as adoptme. Can I take this? 👍

@olcbean

This comment has been minimized.

Copy link
Contributor Author

@olcbean olcbean commented Nov 7, 2017

Hey @liketic, I have already pushed a PR for ObjectParser. Sorry about that.

@olcbean

This comment has been minimized.

Copy link
Contributor Author

@olcbean olcbean commented Jan 26, 2018

Closed by #27302

@olcbean olcbean closed this Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.