Move ObjectParser into the x-content lib #29373
Conversation
This moves `ObjectParser`, `AbstractObjectParser`, and `ConstructingObjectParser` into the libs/x-content dependency. This decoupling allows them to be used for parsing for projects that don't want to depend on the entire Elasticsearch jar. Relates to elastic#28504
dakrone
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
Strange, this never failed on local CI...
| * Return the detail message, including the message from the nested exception | ||
| * if there is one. | ||
| */ | ||
| public String getDetailedMessage() { |
There was a problem hiding this comment.
This seems to only be used for tests. Maybe it should be a helper method in the test framework instead of part of the public api? I would be afraid of something accidentally using this in ES code.
There was a problem hiding this comment.
I'm not sure, we have this also in ElasticsearchException (which is where I originally saw this). How about if instead I use ExceptionsHelper.detailedMessage since all of the callers are in server?
There was a problem hiding this comment.
The javadocs on that method show it is deprecated and should not be used. Why is this necessary? Why are tests converted in this PR to using it?
There was a problem hiding this comment.
I didn't convert the tests to use it, a fair number were already using getDetailedMessage provided by ElasticsearchException, because the previous ParsingException used to be a subclass of ElasticsearchException.
As for the necessity, it seems to make it easier for tests to check that somewhere in the chain of causes a specific exception message exists, without having to know exactly which cause it will be.
There was a problem hiding this comment.
Ok, my confusion stemmed from the fact the first result when searching down the diff was ScriptProcessorFactoryTests.java, which this PR changes from testing against getMessage(), to using getDetailedMessage(). I see now that is the only case. Can it be switched back?
Changing the tests to use ExceptionsHelper.detailedMessage seems ok given they all already use it.
There was a problem hiding this comment.
Okay, I've removed this method (getDetailedMessage) and reverted the ScriptProcessorFactoryTests back to the getMessage()
|
|
||
| public final class ObjectParserHelper<Value, Context> { | ||
|
|
||
| public void declareRawObject(final AbstractObjectParser<Value, Context> parser, |
There was a problem hiding this comment.
Was this moved out because CheckedFunction is not yet in core, or for another reason? Can we at least have javadocs on the class/method?
There was a problem hiding this comment.
It was moved out because BytesReference cannot go in core (since it relies on Lucene). I'll add Javadocs for this class and method
* Move ObjectParser into the x-content lib This moves `ObjectParser`, `AbstractObjectParser`, and `ConstructingObjectParser` into the libs/x-content dependency. This decoupling allows them to be used for parsing for projects that don't want to depend on the entire Elasticsearch jar. Relates to #28504
Just like `ElasticsearchException`, the inner most `XContentParseException` tends to contain the root cause of the exception and show be show to the user in the `root_cause` field. The effectively undoes most of the changes that elastic#29373 made to the `root_cause` for parsing exceptions. The `type` field still changes from `parse_exception` to `x_content_parse_exception`, but this seems like a fairly safe change. `ElasticsearchWrapperException` *looks* tempting to implement this but the behavior isn't quite right. `ElasticsearchWrapperExceptions` are entirely unwrapped until the cause no longer `implements ElasticsearchWrapperException` but `XContentParseException` should be unwrapped until its cause is no longer an `XContentParseException` but no further. In other words, `ElasticsearchWrapperException` are unwrapped one step too far. Closes elastic#30261
Just like `ElasticsearchException`, the inner most `XContentParseException` tends to contain the root cause of the exception and show be show to the user in the `root_cause` field. The effectively undoes most of the changes that #29373 made to the `root_cause` for parsing exceptions. The `type` field still changes from `parse_exception` to `x_content_parse_exception`, but this seems like a fairly safe change. `ElasticsearchWrapperException` *looks* tempting to implement this but the behavior isn't quite right. `ElasticsearchWrapperExceptions` are entirely unwrapped until the cause no longer `implements ElasticsearchWrapperException` but `XContentParseException` should be unwrapped until its cause is no longer an `XContentParseException` but no further. In other words, `ElasticsearchWrapperException` are unwrapped one step too far. Closes #30261
Just like `ElasticsearchException`, the inner most `XContentParseException` tends to contain the root cause of the exception and show be show to the user in the `root_cause` field. The effectively undoes most of the changes that #29373 made to the `root_cause` for parsing exceptions. The `type` field still changes from `parse_exception` to `x_content_parse_exception`, but this seems like a fairly safe change. `ElasticsearchWrapperException` *looks* tempting to implement this but the behavior isn't quite right. `ElasticsearchWrapperExceptions` are entirely unwrapped until the cause no longer `implements ElasticsearchWrapperException` but `XContentParseException` should be unwrapped until its cause is no longer an `XContentParseException` but no further. In other words, `ElasticsearchWrapperException` are unwrapped one step too far. Closes #30261
Just like `ElasticsearchException`, the inner most `XContentParseException` tends to contain the root cause of the exception and show be show to the user in the `root_cause` field. The effectively undoes most of the changes that #29373 made to the `root_cause` for parsing exceptions. The `type` field still changes from `parse_exception` to `x_content_parse_exception`, but this seems like a fairly safe change. `ElasticsearchWrapperException` *looks* tempting to implement this but the behavior isn't quite right. `ElasticsearchWrapperExceptions` are entirely unwrapped until the cause no longer `implements ElasticsearchWrapperException` but `XContentParseException` should be unwrapped until its cause is no longer an `XContentParseException` but no further. In other words, `ElasticsearchWrapperException` are unwrapped one step too far. Closes #30261
This moves
ObjectParser,AbstractObjectParser, andConstructingObjectParserinto the libs/x-content dependency. This decouplingallows them to be used for parsing for projects that don't want to depend on the
entire Elasticsearch jar.
Relates to #28504