Skip to content
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

Fixed misunderstanding message 'No OpenSearchException found' when detailed_error disabled #4669

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Segment Replication] Adding check to make sure checkpoint is not processed when a shard's shard routing is primary ([#4630](https://github.com/opensearch-project/OpenSearch/pull/4630))
- [Bug]: Fixed invalid location of JDK dependency for arm64 architecture([#4613](https://github.com/opensearch-project/OpenSearch/pull/4613))
- [Bug]: Alias filter lost after rollover ([#4499](https://github.com/opensearch-project/OpenSearch/pull/4499))
- Fixed misunderstanding message "No OpenSearchException found" when detailed_error disabled ([#4669](https://github.com/opensearch-project/OpenSearch/pull/4669))
- Attempt to fix Github workflow for Gradle Check job ([#4679](https://github.com/opensearch-project/OpenSearch/pull/4679))
- Fix flaky DecommissionControllerTests.testTimesOut ([4683](https://github.com/opensearch-project/OpenSearch/pull/4683))
- Fix new race condition in DecommissionControllerTests ([4688](https://github.com/opensearch-project/OpenSearch/pull/4688))
Expand Down
15 changes: 15 additions & 0 deletions server/src/main/java/org/opensearch/ExceptionsHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,21 @@ public static RestStatus status(Throwable t) {
return RestStatus.INTERNAL_SERVER_ERROR;
}

public static String summaryMessage(Throwable t) {
if (t != null) {
if (t instanceof OpenSearchException) {
return t.getClass().getSimpleName() + "[" + t.getMessage() + "]";
} else if (t instanceof IllegalArgumentException) {
return "Invalid argument";
} else if (t instanceof JsonParseException) {
return "Failed to parse JSON";
} else if (t instanceof OpenSearchRejectedExecutionException) {
return "Too many requests";
}
}
return "Internal failure";
}

public static Throwable unwrapCause(Throwable t) {
int counter = 0;
Throwable result = t;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -594,16 +594,14 @@ public static void generateFailureXContent(XContentBuilder builder, Params param

// Render the exception with a simple message
if (detailed == false) {
String message = "No OpenSearchException found";
Throwable t = e;
for (int counter = 0; counter < 10 && t != null; counter++) {
if (t instanceof OpenSearchException) {
message = t.getClass().getSimpleName() + "[" + t.getMessage() + "]";
break;
}
t = t.getCause();
}
builder.field(ERROR, message);
builder.field(ERROR, ExceptionsHelper.summaryMessage(t));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be passing in e since t is null... this is causing other confusing Internal failure exceptions even when the original exception was an InternalArgumentException. see this build failure

Fixing in #4702

return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,12 @@ public void testStatus() {
assertThat(ExceptionsHelper.status(new OpenSearchRejectedExecutionException("rejected")), equalTo(RestStatus.TOO_MANY_REQUESTS));
}

public void testSummaryMessage() {
assertThat(ExceptionsHelper.summaryMessage(new IllegalArgumentException("illegal")), equalTo("Invalid argument"));
assertThat(ExceptionsHelper.summaryMessage(new JsonParseException(null, "illegal")), equalTo("Failed to parse JSON"));
assertThat(ExceptionsHelper.summaryMessage(new OpenSearchRejectedExecutionException("rejected")), equalTo("Too many requests"));
}

public void testGroupBy() {
ShardOperationFailedException[] failures = new ShardOperationFailedException[] {
createShardFailureParsingException("error", "node0", "index", 0, null),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -814,12 +814,7 @@ public void testFailureToAndFromXContentWithNoDetails() throws IOException {
}
assertNotNull(parsedFailure);

String reason;
if (failure instanceof OpenSearchException) {
reason = failure.getClass().getSimpleName() + "[" + failure.getMessage() + "]";
} else {
reason = "No OpenSearchException found";
}
String reason = ExceptionsHelper.summaryMessage(failure);
assertEquals(OpenSearchException.buildMessage("exception", reason, null), parsedFailure.getMessage());
assertEquals(0, parsedFailure.getHeaders().size());
assertEquals(0, parsedFailure.getMetadata().size());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public void testNonOpenSearchExceptionIsNotShownAsSimpleMessage() throws Excepti
assertThat(text, not(containsString("UnknownException[an error occurred reading data]")));
assertThat(text, not(containsString("FileNotFoundException[/foo/bar]")));
assertThat(text, not(containsString("error_trace")));
assertThat(text, containsString("\"error\":\"No OpenSearchException found\""));
assertThat(text, containsString("\"error\":\"Internal failure\""));
}

public void testErrorTrace() throws Exception {
Expand Down