Add option to only return simple exception messages #10117
Conversation
jaymode
added
>enhancement
v2.0.0-beta1
review
v1.5.0
:Core/Infra/REST API
|
@s1monw can you review this? |
| while (t != null) { | ||
| if (counter++ > 10) { | ||
| break; |
There was a problem hiding this comment.
Won't this cause no exception to be added at all? Shouldn't we be returning something in this case still?
| @@ -72,6 +72,10 @@ be cached for. Defaults to `1728000` (20 days) | |||
| header should be returned. Note: This header is only returned, when the setting is | |||
| set to `true`. Defaults to `false` | |||
|
|
|||
| |`http.emit_stack_trace` |Enables or disables the output of stack traces in response | |||
| output. Note: When set to `false` this setting overrides the `error_trace` request parameter | |||
| and no stack trace will be output; only a simple message will be displayed. Defaults to `true` | |||
There was a problem hiding this comment.
wait this defaults to true? I don't think we should do this it should be default to false!
There was a problem hiding this comment.
If we do that then we need to make sure the tests will always override this to true.
There was a problem hiding this comment.
oh I think I misread it by default we print the stacktraces right? ok then true is just fine :)
|
@rjernst pushed a commit addressing the feedback |
| } | ||
|
|
||
| if (!added) { |
There was a problem hiding this comment.
Perhaps this function would be easier if it were structured like detailedMessage(t)? Then this function can just return when it finds an ES exc, and exhausting the loop can have a single return statement at the end. I think the null case should be extracted out since it is the same whether we do simple or detailed exceptions? The caller locations in convert will look more inline then, like:
if (t == null) {
buidler.field("error", "Unknown");
} else if (channel.emitStackTrace()) {
builder.field("error", detailedMessage(t));
} else {
builder.field("error", simpleMessage(t));
}
|
@rjernst pushed some commits based on the additional feedback. I also changed the setting name; I think the original name was confusing and didn't accurately represent all of the behavior being changed. |
| return builder; | ||
| } | ||
|
|
||
| private static void buildErrorTrace(RestChannel channel, Throwable t, XContentBuilder builder) throws IOException { |
There was a problem hiding this comment.
maybe move the conditional logic back out to the caller? the null check isn't needed, and the function name implies it always adds this trace, when in fact it depends on the request.
|
Looks good, just a couple more comments, no need for further review. |
Adds a setting to disable detailed error messages and full exception stack traces in HTTP responses. When set to false, the error_trace request parameter will result in a HTTP 400 response. When the error_trace parameter is not present, the message of the first ElasticsearchException will be output and no nested exception messages will be output.
jaymode
force-pushed
the
simple_error_message_only
branch
from
7d0f8d9
to
7fff43c
Compare
|
@jaymode Note that this commit breaks transport plugins. |
|
I think the default should always be |
RestChannel has changed with this PR: elastic/elasticsearch#10117 Closes #47. (cherry picked from commit 2747a45)
RestChannel has changed with this PR: elastic/elasticsearch#10117 Closes #47. (cherry picked from commit 2747a45) (cherry picked from commit bc8abd7)
RestChannel has changed with this PR: elastic/elasticsearch#10117 Closes #47.
RestChannel has changed with this PR: elastic/elasticsearch#10117 Closes #38. (cherry picked from commit 5d8ec9b)
RestChannel has changed with this PR: elastic/elasticsearch#10117 Closes #38. (cherry picked from commit 5d8ec9b) (cherry picked from commit 6873b43)
RestChannel has changed with this PR: elastic/elasticsearch#10117 Closes #38.
`RestChannel` now has a parameter `detailedErrorsEnabled` which wasn't being passed in, causing a `NoSuchMethodError` if you try to run it with elasticsearch 1.5 (and a compile error if you tried to compile it). See also elastic/elasticsearch#10117 Closes #30.
`RestChannel` now has a parameter `detailedErrorsEnabled` which wasn't being passed in, causing a `NoSuchMethodError` if you try to run it with elasticsearch 1.5 (and a compile error if you tried to compile it). See also elastic/elasticsearch#10117 Closes #30. (cherry picked from commit 91445ef)
`RestChannel` now has a parameter `detailedErrorsEnabled` which wasn't being passed in, causing a `NoSuchMethodError` if you try to run it with elasticsearch 1.5 (and a compile error if you tried to compile it). See also elastic/elasticsearch#10117 Closes #30. (cherry picked from commit 91445ef) (cherry picked from commit 849a457)
clintongormley
changed the title
Adds a setting to disable detailed error messages and full exception stack traces
in http responses. When set to false, the error_trace request parameter will be
ignored and only the message of the first ElasticsearchException will be output;
no nested exception messages will be output.