diff --git a/CHANGELOG.md b/CHANGELOG.md index 6956af0501a5f..861b9fae5f417 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix bug in Assertion framework(Yaml Rest test): numeric comparison fails when comparing Integer vs Long (or Float vs Double) ([#19376](https://github.com/opensearch-project/OpenSearch/pull/19376)) - Fix Netty deprecation warnings in transport-netty4 module ([#20233](https://github.com/opensearch-project/OpenSearch/pull/20233)) - Fix snapshot restore when an index sort is present ([#20284](https://github.com/opensearch-project/OpenSearch/pull/20284)) +- Fix SearchPhaseExecutionException to properly initCause ([#20320](https://github.com/opensearch-project/OpenSearch/pull/20320)) ### Dependencies - Bump `com.google.auth:google-auth-library-oauth2-http` from 1.38.0 to 1.41.0 ([#20183](https://github.com/opensearch-project/OpenSearch/pull/20183)) diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhaseExecutionException.java b/server/src/main/java/org/opensearch/action/search/SearchPhaseExecutionException.java index 9ede6b4e3eb1b..013b1c139a47f 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhaseExecutionException.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhaseExecutionException.java @@ -60,11 +60,36 @@ public SearchPhaseExecutionException(String phaseName, String msg, ShardSearchFa } public SearchPhaseExecutionException(String phaseName, String msg, Throwable cause, ShardSearchFailure[] shardFailures) { - super(msg, deduplicateCause(cause, shardFailures)); + super(msg, getEffectiveCause(cause, shardFailures)); this.phaseName = phaseName; this.shardFailures = shardFailures; } + /** + * Determines the effective cause for this exception. If an explicit cause is provided and not + * duplicated in shard failures, use it. Otherwise, use the first shard failure's cause. + */ + private static Throwable getEffectiveCause(Throwable cause, ShardSearchFailure[] shardFailures) { + if (shardFailures == null) { + throw new IllegalArgumentException("shardSearchFailures must not be null"); + } + // If explicit cause provided and not duplicated in shard failures, use it + if (cause != null) { + for (ShardSearchFailure failure : shardFailures) { + if (failure.getCause() == cause) { + // Cause is duplicated, but still return it to properly wire the chain + return cause; + } + } + return cause; + } + // No explicit cause - use first shard failure's cause if available + if (shardFailures.length > 0 && shardFailures[0].getCause() != null) { + return shardFailures[0].getCause(); + } + return null; + } + public SearchPhaseExecutionException(StreamInput in) throws IOException { super(in); phaseName = in.readOptionalString(); @@ -78,22 +103,6 @@ public void writeTo(StreamOutput out) throws IOException { out.writeArray(shardFailures); } - private static Throwable deduplicateCause(Throwable cause, ShardSearchFailure[] shardFailures) { - if (shardFailures == null) { - throw new IllegalArgumentException("shardSearchFailures must not be null"); - } - // if the cause of this exception is also the cause of one of the shard failures we don't add it - // to prevent duplication in stack traces rendered to the REST layer - if (cause != null) { - for (ShardSearchFailure failure : shardFailures) { - if (failure.getCause() == cause) { - return null; - } - } - } - return cause; - } - @Override public RestStatus status() { if (shardFailures.length == 0) { @@ -116,18 +125,6 @@ public ShardSearchFailure[] shardFailures() { return shardFailures; } - @Override - public Throwable getCause() { - Throwable cause = super.getCause(); - if (cause == null) { - // fall back to guessed root cause - for (OpenSearchException rootCause : guessRootCauses()) { - return rootCause; - } - } - return cause; - } - private static String buildMessage(String phaseName, String msg, ShardSearchFailure[] shardFailures) { StringBuilder sb = new StringBuilder(); sb.append("Failed to execute phase [").append(phaseName).append("], ").append(msg); diff --git a/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java b/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java index 826f98d56372e..e03a9b500ba60 100644 --- a/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java +++ b/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java @@ -45,6 +45,7 @@ import org.opensearch.common.UUIDs; import org.opensearch.common.collect.Tuple; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.common.ParsingException; import org.opensearch.core.common.Strings; @@ -251,9 +252,33 @@ public void testDeduplicate() throws IOException { builder.startObject(); ex.toXContent(builder, ToXContent.EMPTY_PARAMS); builder.endObject(); - String expected = "{\"type\":\"search_phase_execution_exception\",\"reason\":\"all shards failed\",\"phase\":\"search\"," - + "\"grouped\":true,\"failed_shards\":[{\"shard\":1,\"index\":\"foo\",\"node\":\"node_1\",\"reason\":" - + "{\"type\":\"parsing_exception\",\"reason\":\"foobar\",\"line\":1,\"col\":2}}]}"; + String expected = XContentHelper.stripWhitespace(""" + { + "type": "search_phase_execution_exception", + "reason": "all shards failed", + "phase": "search", + "grouped": true, + "failed_shards": [ + { + "shard": 1, + "index": "foo", + "node": "node_1", + "reason": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + } + ], + "caused_by": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + } + """); assertEquals(expected, builder.toString()); } { @@ -278,11 +303,44 @@ public void testDeduplicate() throws IOException { builder.startObject(); ex.toXContent(builder, ToXContent.EMPTY_PARAMS); builder.endObject(); - String expected = "{\"type\":\"search_phase_execution_exception\",\"reason\":\"all shards failed\"," - + "\"phase\":\"search\",\"grouped\":true,\"failed_shards\":[{\"shard\":1,\"index\":\"foo\",\"node\":\"node_1\"," - + "\"reason\":{\"type\":\"parsing_exception\",\"reason\":\"foobar\",\"line\":1,\"col\":2}},{\"shard\":1," - + "\"index\":\"foo1\",\"node\":\"node_1\",\"reason\":{\"type\":\"query_shard_exception\",\"reason\":\"foobar\"," - + "\"index\":\"foo1\",\"index_uuid\":\"_na_\"}}]}"; + String expected = XContentHelper.stripWhitespace(""" + { + "type": "search_phase_execution_exception", + "reason": "all shards failed", + "phase": "search", + "grouped": true, + "failed_shards": [ + { + "shard": 1, + "index": "foo", + "node": "node_1", + "reason": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + }, + { + "shard": 1, + "index": "foo1", + "node": "node_1", + "reason": { + "type": "query_shard_exception", + "reason": "foobar", + "index": "foo1", + "index_uuid": "_na_" + } + } + ], + "caused_by": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + } + """); assertEquals(expected, builder.toString()); } { @@ -1066,7 +1124,8 @@ public static Tuple randomExceptions() { ) } ); expected = new OpenSearchException( - "OpenSearch exception [type=search_phase_execution_exception, " + "reason=all shards failed]" + "OpenSearch exception [type=search_phase_execution_exception, reason=all shards failed]", + new OpenSearchException("OpenSearch exception [type=parsing_exception, reason=foobar]") ); expected.addMetadata("opensearch.phase", "search"); break; diff --git a/server/src/test/java/org/opensearch/action/search/SearchPhaseExecutionExceptionTests.java b/server/src/test/java/org/opensearch/action/search/SearchPhaseExecutionExceptionTests.java index 06b00a6a438c4..569d162867173 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchPhaseExecutionExceptionTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchPhaseExecutionExceptionTests.java @@ -79,39 +79,45 @@ public void testToXContent() throws IOException { ); // Failures are grouped (by default) - final String expectedJson = XContentHelper.stripWhitespace( - "{" - + " \"type\": \"search_phase_execution_exception\"," - + " \"reason\": \"all shards failed\"," - + " \"phase\": \"test\"," - + " \"grouped\": true," - + " \"failed_shards\": [" - + " {" - + " \"shard\": 0," - + " \"index\": \"foo\"," - + " \"node\": \"node_1\"," - + " \"reason\": {" - + " \"type\": \"parsing_exception\"," - + " \"reason\": \"foobar\"," - + " \"line\": 1," - + " \"col\": 2" - + " }" - + " }," - + " {" - + " \"shard\": 1," - + " \"index\": \"foo\"," - + " \"node\": \"node_2\"," - + " \"reason\": {" - + " \"type\": \"index_shard_closed_exception\"," - + " \"reason\": \"CurrentState[CLOSED] Closed\"," - + " \"index\": \"foo\"," - + " \"shard\": \"1\"," - + " \"index_uuid\": \"_na_\"" - + " }" - + " }" - + " ]" - + "}" - ); + final String expectedJson = XContentHelper.stripWhitespace(""" + { + "type": "search_phase_execution_exception", + "reason": "all shards failed", + "phase": "test", + "grouped": true, + "failed_shards": [ + { + "shard": 0, + "index": "foo", + "node": "node_1", + "reason": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + }, + { + "shard": 1, + "index": "foo", + "node": "node_2", + "reason": { + "type": "index_shard_closed_exception", + "reason": "CurrentState[CLOSED] Closed", + "index": "foo", + "shard": "1", + "index_uuid": "_na_" + } + } + ], + "caused_by": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + } + """); assertEquals(expectedJson, Strings.toString(MediaTypeRegistry.JSON, exception)); } @@ -149,8 +155,8 @@ public void testToAndFromXContent() throws IOException { assertThat(parsedException.getHeaderKeys(), hasSize(0)); assertThat(parsedException.getMetadataKeys(), hasSize(1)); assertThat(parsedException.getMetadata("opensearch.phase"), hasItem(phase)); - // SearchPhaseExecutionException has no cause field - assertNull(parsedException.getCause()); + // SearchPhaseExecutionException wires up the first shard failure's cause + assertNotNull(parsedException.getCause()); } public void testPhaseFailureWithoutSearchShardFailure() { diff --git a/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java b/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java index 9252475327b9a..ac81f45d5cf0e 100644 --- a/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java +++ b/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java @@ -40,6 +40,7 @@ import org.opensearch.action.OriginalIndices; import org.opensearch.action.search.SearchPhaseExecutionException; import org.opensearch.action.search.ShardSearchFailure; +import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.common.ParsingException; import org.opensearch.core.common.bytes.BytesReference; @@ -179,10 +180,44 @@ public void testConvert() throws IOException { ); BytesRestResponse response = new BytesRestResponse(channel, new RemoteTransportException("foo", ex)); String text = response.content().utf8ToString(); - String expected = "{\"error\":{\"root_cause\":[{\"type\":\"parsing_exception\",\"reason\":\"foobar\",\"line\":1,\"col\":2}]," - + "\"type\":\"search_phase_execution_exception\",\"reason\":\"all shards failed\",\"phase\":\"search\",\"grouped\":true," - + "\"failed_shards\":[{\"shard\":1,\"index\":\"foo\",\"node\":\"node_1\",\"reason\":{\"type\":\"parsing_exception\"," - + "\"reason\":\"foobar\",\"line\":1,\"col\":2}}]},\"status\":400}"; + String expected = XContentHelper.stripWhitespace(""" + { + "error": { + "root_cause": [ + { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + ], + "type": "search_phase_execution_exception", + "reason": "all shards failed", + "phase": "search", + "grouped": true, + "failed_shards": [ + { + "shard": 1, + "index": "foo", + "node": "node_1", + "reason": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + } + ], + "caused_by": { + "type": "parsing_exception", + "reason": "foobar", + "line": 1, + "col": 2 + } + }, + "status": 400 + } + """); assertEquals(expected.trim(), text.trim()); String stackTrace = ExceptionsHelper.stackTrace(ex); assertTrue(stackTrace.contains("Caused by: ParsingException[foobar]"));