Fix SearchPhaseExecutionException to properly initCause

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))

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);

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<Throwable, OpenSearchException> 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;

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() {

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]"));