rethrow script compilation exceptions into ingest configuration exceptions#19318
rethrow script compilation exceptions into ingest configuration exceptions#19318talevy merged 2 commits intoelastic:masterfrom
Conversation
There was a problem hiding this comment.
Why lose the original exception?
There was a problem hiding this comment.
Please don't lose the original exception. It's already difficult enough to debug script exceptions without them being swallowed.
There was a problem hiding this comment.
Ok, thought I could re-use some headers configuration stuff and enough information would make its way through the current exception's message
def1617 to
5c6bbef
Compare
|
LGTM |
There was a problem hiding this comment.
If I understand this code path correctly, this converts every exception thrown during template compilation into an ElasticsearchParseException and I'm not comfortable with that. For example, it means bugs in compilation get converted into a parse exception that sounds like the user can correct? What's the reasoning here?
There was a problem hiding this comment.
we've been using ElasticsearchParseException as the exception container for all our pipeline build-time configuration exceptions.
it means bugs in compilation get converted into a parse exception that sounds like the user can correct
why would the user not be able to correct these exceptions?
There was a problem hiding this comment.
why would the user not be able to correct these exceptions?
Because I'm specifically referring to exceptions being thrown because of bugs in the compilation explicitly not due to the user's template.
There was a problem hiding this comment.
I see, I guess I am not familiar with such exceptions. If all such exceptions extend ElasticsearchException then I suppose there is no real need to wrap the exception, but instead just add the appropriate ingest headers. So, I can check for whether an ElasticsearchException is thrown and simply add the headers and pass it along. If other exceptions are thrown, I would need to wrap it in some sort of ElasticsearchException so that I can relay the proper headers to the user. what do you think?
There was a problem hiding this comment.
We can't safely say that all such exceptions will extend ElasticsearchException (e.g., a bad NullPointerException), but I like your idea of wrapping the ones that do not extend (as long as it's not wrapping it in an exception that sounds like the user can do something about it).
There was a problem hiding this comment.
@jasontedor Would using ExceptionsHelper#convertToElastic(...) helper method in ConfigurationUtils#newConfigurationException(...) or similar here be sufficient?
I think the excepting being thrown if the mustache template couldn't be compiled is a MustacheException, which gets wrapped by GeneralScriptException in ScriptService#compileInternal(...), which is what also should be fixed. Instead the lang-mustache should catch MustacheException exceptions with ScriptException.
There was a problem hiding this comment.
Would using
ExceptionsHelper#convertToElastic(...)helper method inConfigurationUtils#newConfigurationException(...)or similar here be sufficient?
+1
There was a problem hiding this comment.
I'll update to use this helper. didn't know about it
5c6bbef to
adf3890
Compare
Currently, mustache compile exceptions in pipelines are reported as regular ES exceptions, e.g.
{ "error": { "root_cause": [ { "type": "general_script_exception", "reason": "Failed to compile inline script [{{#join}}{{/join}}] using lang [mustache]" } ], "type": "general_script_exception", "reason": "Failed to compile inline script [{{#join}}{{/join}}] using lang [mustache]", "caused_by": { "type": "mustache_exception", "reason": "Mustache function [join] must contain one and only one identifier" } }, "status": 500 }This makes it difficult to gather which property in the pipeline configuration is causing this exception. This PR wraps these exceptions in ElasticsearchParseException so that we can relay the information