Introduce DocumentParsingException#92646
Conversation
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @romseygeek, I've created a changelog YAML for you. |
…nto exceptions/mapper-parsing
|
Hi @romseygeek, I've updated the changelog YAML for you. |
javanna
left a comment
There was a problem hiding this comment.
Left a couple of comments, nothing major. One more thing I could use is a test where we check the full toXContent representation of the query, if we don't have it yet (I may have missed it)
| - match: { items.0.index.error.type: mapper_parsing_exception } | ||
| - match: { items.0.index.error.reason: "Can't find dynamic template for dynamic template name [bar_foo] of field [foo_location]"} | ||
| - match: { items.0.index.error.type: document_parsing_exception } | ||
| - match: { items.0.index.error.reason: "[-1:-1] failed to parse: Can't find dynamic template for dynamic template name [bar_foo] of field [foo_location]"} |
There was a problem hiding this comment.
That -1:-1 isn't great? Shall we omit printing out the location when it's not set? Or should it be set to the location where the field appears first ?
There was a problem hiding this comment.
I've changed this so that it doesn't output the location when it's UNKNOWN
| } | ||
|
|
||
| return new MapperParsingException("failed to parse", e); | ||
| return new DocumentParsingException(XContentLocation.UNKNOWN, "failed to parse: " + e.getMessage(), e); |
There was a problem hiding this comment.
We can't really provide a more useful location in these cases?
There was a problem hiding this comment.
In some cases there's no information, but I've reworked things a bit so that we can catch jackson errors that do include location data and include that in the error message.
|
|
||
| private static MapperParsingException wrapInMapperParsingException(SourceToParse source, Exception e) { | ||
| private static DocumentParsingException wrapInDocumentParsingException(SourceToParse source, Exception e) { | ||
| // if its already a mapper parsing exception, no need to wrap it... |
There was a problem hiding this comment.
either remove the comment or update it?
| documentParserContext.addIgnoredField(name()); | ||
| } else { | ||
| throw new MapperParsingException("Error executing script on field [" + name() + "]", e); | ||
| throw new DocumentParsingException(XContentLocation.UNKNOWN, "Error executing script on field [" + name() + "]", e); |
There was a problem hiding this comment.
this makes me wonder if we should use this exception when we don't have a location. Is this a document parsing error?
There was a problem hiding this comment.
I think this still counts as an error in document parsing? It's still definitely not a mapper parsing exception...
…nto exceptions/mapper-parsing
|
I've updated this in response to your feedback @javanna. CI will not pass for a while as I need to chase down a bunch of error message changes in yaml tests, but I think it's ready for review otherwise. |
|
@romseygeek this was already approved, feel free to merge when ready. |
Document parsing methods currently throw MapperParsingException. This
isn't very helpful, as it doesn't contain any information about where the parse
error happened - it is designed for parsing mappings, which are realised into
java maps before being examined. This commit introduces a new exception
specifically for document parsing that extends XContentException, so that
it reports the current position of the parser as part of its error message.
Fixes #85083