-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Return positions of parse errors found in JSON #10837
Conversation
…d only primitives and changed all of the parsers that throw QueryParsingExceptions which now mandates the passing of the error location info.
… all parsers that throw this exception type
|
||
package org.elasticsearch.common.xcontent; | ||
|
||
public class XContentLocation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have some javadocs here and please make the class an all members final. I wonder if we should not even have getters here and make it a very simple struct
left some minor comments otherwise LGTM |
LGTM |
if we are changing all the places that call |
lets do that in a sep PR? |
sure (my thought was if we already change all this files now) |
If there's no objections I'll revise this PR with @kimchy 's suggestion |
ok sure go ahead @markharwood |
…r. This change simplified the parser classes that throw exceptions but ElasticsearchExceptionTests and BytesRestResonse tests needed tweaking to work around the lack of QueryParseContext available in unit tests. I had to introduce a subclass of QueryParsingException which doesn’t require QueryParseContext to work but can still report on line/column number errors.
The change to pass QueryParseContext to QueryParsingException made all parser code cleaner. |
* This constructor is provided for use in unit tests where a | ||
* {@link QueryParseContext} may not be available | ||
*/ | ||
protected QueryParsingException(Index index, int line, int col, String msg, Throwable cause) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it need to be protected? or just package private?
I had a quick look and I really like it, having the |
…kage as base class but in src/test
Thanks, @javanna - I moved the test exception out into same package in test framework and changed visibilities as per your comments |
LGTM |
LGTM too |
Pushed to master 528f648 |
Extend SearchParseException and QueryParsingException to report position information in query JSON where errors were found. All query DSL parser classes that throw these exception types now pass the underlying position information (line and column number) at the point the error was found.
Closes #3303