-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Output a consistent format when generating error json #90529
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
Conversation
Now, error fields will always have 'type' and 'reason' fields, and the information in those fields is the same regardless of whether the output is detailed or not
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Hi @thecoop, I've created a changelog YAML for you. |
Note this does have some BwC concerns, as this changes the output format when not detailed, but from #89387 we consider the current behaviour a bug |
237ef12
to
cec444a
Compare
server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java
Outdated
Show resolved
Hide resolved
Hi @thecoop, I've updated the changelog YAML for you. |
Hi @thecoop, I've updated the changelog YAML for you. |
Hi @thecoop, I've updated the changelog YAML for you. Note that since this PR is labelled |
071c718
to
2a7fda6
Compare
2a7fda6
to
750eb7d
Compare
This PR is marked as a "breaking" change. Is it really a breaking change? |
…d errors is changing in v9 (elastic#116330)" This reverts commit bd091d3.
Hi @thecoop, I've updated the changelog YAML for you. Note that since this PR is labelled |
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.
One minor nit, but it looks good to me!
server/src/test/java/org/elasticsearch/rest/RestResponseTests.java
Outdated
Show resolved
Hide resolved
@elasticmachine rerun elasticsearch-ci/part-1 |
@elasticmachine update branch |
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.
LGTM, just one optional thought / questions
@@ -146,12 +147,12 @@ public RestResponse(RestChannel channel, RestStatus status, Exception e) throws | |||
params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params); | |||
} | |||
|
|||
if (channel.detailedErrorsEnabled() == false) { | |||
if (channel.request().getRestApiVersion() == RestApiVersion.V_8 && channel.detailedErrorsEnabled() == false) { | |||
deprecationLogger.warn( |
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.
Should this change to critical now?
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.
I don't think so, there's (probably) nothing directly for users to do; we consider the existing behaviour a bug, as it's surprising the format changes with non-detailed errors.
Now, error fields will always have 'type' and 'reason' fields, and the information in those fields is the same regardless of whether the output is detailed or not
Now, error fields will always have 'type' and 'reason' fields, and the information in those fields is the same regardless of whether the output is detailed or not
Friendly reminder that this PR seems to be a breaking change for 9.0, but is missing from the 9.0 release note. We may want to add an entry to the breaking change section of 9.0 release note. |
See #125485 |
@thecoop is this PR relevant to the serverless changelog? [FYI this question is based on 9.0 breaking changes] |
No, this option is not relevant for serverless |
Now, error fields will always have 'type' and 'reason' fields, and the information in those fields is the same regardless of whether the output is detailed or not. This also reverses the test changes in #116330, due to the deprecation warning not (generally) being there anymore.
This fixes #89387