Skip to content
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Fixed the SnapshotsInProgress error during index deletion ([#4570](https://github.com/opensearch-project/OpenSearch/pull/4570))
- [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))
Copy link
Member

@dreamer-89 dreamer-89 Oct 7, 2022

Choose a reason for hiding this comment

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

Thanks @xuezhou25 for the fix.

  1. Please change PR number 4669 -> 4708
  2. We can update the wording to make it more clear.


### Security
- CVE-2022-25857 org.yaml:snakeyaml DOS vulnerability ([#4341](https://github.com/opensearch-project/OpenSearch/pull/4341))
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
Contributor

Choose a reason for hiding this comment

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

Suggested change
builder.field(ERROR, ExceptionsHelper.summaryMessage(t));
builder.field(ERROR, ExceptionsHelper.summaryMessage(e));

t is null, which results in a more confusing Internal error exception even when a useful IllegalArgumentException is thrown. e should be passed to summaryMessage so a more useful message is thrown. This already resulted in a reproducible test failure which was fixed in #4702.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nknize I think the logic behind this code is to find the OpenSearchException down the cause chain (which is t). May be in this case we should do:

builder.field(ERROR, ExceptionsHelper.summaryMessage(t != null ? t : e));

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing. I will do this fix

Copy link
Contributor

@tlfeng tlfeng Nov 4, 2022

Choose a reason for hiding this comment

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

I agree with the suggested change from @reta . The original logic is to find the OpenSearchException down the cause chain, and show its error message.
Looks like if the code is changed to t != null ? t : e here in 2.x branch, then the corresponding change also need to be applied in main branch.

builder.field(ERROR, ExceptionsHelper.summaryMessage(e));

Copy link
Member

Choose a reason for hiding this comment

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

The ExceptionsHelper.summaryMessage(e) version is what exists on main currently. We should probably keep them consistent and then follow up with a change on main to do the ExceptionsHelper.summaryMessage(t != null ? t : e) version (and then backport that PR as well).

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrross Make sense, thank you! 😄

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