Prevent cause from being null in ShardOperationFailedException (#32640)

`ShardOperationFailedException` and corresponding implementors seem to suggest that the cause may be null, case that is also handled in a few places. Yet, it does not seem to be possible in practice for the cause to be null, hence we can clean that up and enforce the cause to be a non null value. This is best done by making `ShardOperationFailedException` an abstract class rather than an interface, which holds the basic member instance that all the subclasses have in common and can also enforce that cause, status and reason are non null.

Author: javanna

server/src/main/java/org/elasticsearch/ExceptionsHelper.java

@@ -279,13 +279,8 @@ private static class GroupBy {
                 }
             }
             this.index = indexName;
-            if (cause == null) {
-                this.reason = failure.reason();
-                this.causeType = null;
-            } else {
-                this.reason = cause.getMessage();
-                this.causeType = cause.getClass();
-            }
+            this.reason = cause.getMessage();
+            this.causeType = cause.getClass();
         }
 
         @Override

server/src/main/java/org/elasticsearch/action/ShardOperationFailedException.java

@@ -19,39 +19,70 @@
 
 package org.elasticsearch.action;
 
+import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.io.stream.Streamable;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.rest.RestStatus;
 
+import java.util.Objects;
+
 /**
  * An exception indicating that a failure occurred performing an operation on the shard.
  *
- *
  */
-public interface ShardOperationFailedException extends Streamable, ToXContent {
+public abstract class ShardOperationFailedException implements Streamable, ToXContent {
+
+    protected String index;
+    protected int shardId;
+    protected String reason;
+    protected RestStatus status;
+    protected Throwable cause;
+
+    protected ShardOperationFailedException() {
+
+    }
+
+    protected ShardOperationFailedException(@Nullable String index, int shardId, String reason, RestStatus status, Throwable cause) {
+        this.index = index;
+        this.shardId = shardId;
+        this.reason = Objects.requireNonNull(reason, "reason cannot be null");
+        this.status = Objects.requireNonNull(status, "status cannot be null");
+        this.cause = Objects.requireNonNull(cause, "cause cannot be null");
+    }
 
     /**
      * The index the operation failed on. Might return {@code null} if it can't be derived.
      */
-    String index();
+    @Nullable
+    public final String index() {
+        return index;
+    }
 
     /**
      * The index the operation failed on. Might return {@code -1} if it can't be derived.
      */
-    int shardId();
+    public final int shardId() {
+        return shardId;
+    }
 
     /**
      * The reason of the failure.
      */
-    String reason();
+    public final String reason() {
+        return reason;
+    }
 
     /**
      * The status of the failure.
      */
-    RestStatus status();
+    public final RestStatus status() {
+        return status;
+    }
 
     /**
      * The cause of this failure
      */
-    Throwable getCause();
+    public final Throwable getCause() {
+        return cause;
+    }
 }

server/src/main/java/org/elasticsearch/action/admin/indices/shards/IndicesShardStoresResponse.java

@@ -21,7 +21,6 @@
 
 import com.carrotsearch.hppc.cursors.IntObjectCursor;
 import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
-
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionResponse;
@@ -248,7 +247,7 @@ public String nodeId() {
             return nodeId;
         }
 
-        public static Failure readFailure(StreamInput in) throws IOException {
+        static Failure readFailure(StreamInput in) throws IOException {
             Failure failure = new Failure();
             failure.readFrom(in);
             return failure;

server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java

@@ -138,8 +138,7 @@ protected void metadataToXContent(XContentBuilder builder, Params params) throws
         builder.field("grouped", group); // notify that it's grouped
         builder.field("failed_shards");
         builder.startArray();
-        ShardOperationFailedException[] failures = params.paramAsBoolean("group_shard_failures", true) ?
-                ExceptionsHelper.groupBy(shardFailures) : shardFailures;
+        ShardOperationFailedException[] failures = group ? ExceptionsHelper.groupBy(shardFailures) : shardFailures;
         for (ShardOperationFailedException failure : failures) {
             builder.startObject();
             failure.toXContent(builder, params);

server/src/main/java/org/elasticsearch/action/search/ShardSearchFailure.java

@@ -43,7 +43,7 @@
 /**
  * Represents a failure to search on a specific shard.
  */
-public class ShardSearchFailure implements ShardOperationFailedException {
+public class ShardSearchFailure extends ShardOperationFailedException {
 
     private static final String REASON_FIELD = "reason";
     private static final String NODE_FIELD = "node";
@@ -53,9 +53,6 @@ public class ShardSearchFailure implements ShardOperationFailedException {
     public static final ShardSearchFailure[] EMPTY_ARRAY = new ShardSearchFailure[0];
 
     private SearchShardTarget shardTarget;
-    private String reason;
-    private RestStatus status;
-    private Throwable cause;
 
     private ShardSearchFailure() {
 
@@ -66,25 +63,18 @@ public ShardSearchFailure(Exception e) {
     }
 
     public ShardSearchFailure(Exception e, @Nullable SearchShardTarget shardTarget) {
+        super(shardTarget == null ? null : shardTarget.getFullyQualifiedIndexName(),
+            shardTarget == null ? -1 : shardTarget.getShardId().getId(),
+            ExceptionsHelper.detailedMessage(e),
+            ExceptionsHelper.status(ExceptionsHelper.unwrapCause(e)),
+            ExceptionsHelper.unwrapCause(e));
+
         final Throwable actual = ExceptionsHelper.unwrapCause(e);
         if (actual instanceof SearchException) {
             this.shardTarget = ((SearchException) actual).shard();
         } else if (shardTarget != null) {
             this.shardTarget = shardTarget;
         }
-        status = ExceptionsHelper.status(actual);
-        this.reason = ExceptionsHelper.detailedMessage(e);
-        this.cause = actual;
-    }
-
-    public ShardSearchFailure(String reason, SearchShardTarget shardTarget) {
-        this(reason, shardTarget, RestStatus.INTERNAL_SERVER_ERROR);
-    }
-
-    private ShardSearchFailure(String reason, SearchShardTarget shardTarget, RestStatus status) {
-        this.shardTarget = shardTarget;
-        this.reason = reason;
-        this.status = status;
     }
 
     /**
@@ -95,41 +85,6 @@ public SearchShardTarget shard() {
         return this.shardTarget;
     }
 
-    @Override
-    public RestStatus status() {
-        return this.status;
-    }
-
-    /**
-     * The index the search failed on.
-     */
-    @Override
-    public String index() {
-        if (shardTarget != null) {
-            return shardTarget.getFullyQualifiedIndexName();
-        }
-        return null;
-    }
-
-    /**
-     * The shard id the search failed on.
-     */
-    @Override
-    public int shardId() {
-        if (shardTarget != null) {
-            return shardTarget.getShardId().id();
-        }
-        return -1;
-    }
-
-    /**
-     * The reason of the failure.
-     */
-    @Override
-    public String reason() {
-        return this.reason;
-    }
-
     @Override
     public String toString() {
         return "shard [" + (shardTarget == null ? "_na" : shardTarget) + "], reason [" + reason + "], cause [" +
@@ -172,12 +127,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         if (shardTarget != null) {
             builder.field(NODE_FIELD, shardTarget.getNodeId());
         }
-        if (cause != null) {
-            builder.field(REASON_FIELD);
-            builder.startObject();
-            ElasticsearchException.generateThrowableXContent(builder, params, cause);
-            builder.endObject();
-        }
+        builder.field(REASON_FIELD);
+        builder.startObject();
+        ElasticsearchException.generateThrowableXContent(builder, params, cause);
+        builder.endObject();
         return builder;
     }
 
@@ -225,9 +178,4 @@ public static ShardSearchFailure fromXContent(XContentParser parser) throws IOEx
         }
         return new ShardSearchFailure(exception, searchShardTarget);
     }
-
-    @Override
-    public Throwable getCause() {
-        return cause;
-    }
 }

server/src/main/java/org/elasticsearch/action/support/DefaultShardOperationFailedException.java

@@ -28,16 +28,14 @@
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.index.Index;
-import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.rest.RestStatus;
 
 import java.io.IOException;
 
 import static org.elasticsearch.ExceptionsHelper.detailedMessage;
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
 
-public class DefaultShardOperationFailedException implements ShardOperationFailedException {
+public class DefaultShardOperationFailedException extends ShardOperationFailedException {
 
     private static final String INDEX = "index";
     private static final String SHARD_ID = "shard";
@@ -52,56 +50,16 @@ public class DefaultShardOperationFailedException implements ShardOperationFaile
         PARSER.declareObject(constructorArg(), (p, c) -> ElasticsearchException.fromXContent(p), new ParseField(REASON));
     }
 
-    private String index;
-
-    private int shardId;
-
-    private Throwable reason;
-
-    private RestStatus status;
-
     protected DefaultShardOperationFailedException() {
     }
 
     public DefaultShardOperationFailedException(ElasticsearchException e) {
-        Index index = e.getIndex();
-        this.index = index == null ? null : index.getName();
-        ShardId shardId = e.getShardId();
-        this.shardId = shardId == null ? -1 : shardId.id();
-        this.reason = e;
-        this.status = e.status();
+        super(e.getIndex() == null ? null : e.getIndex().getName(), e.getShardId() == null ? -1 : e.getShardId().getId(),
+            detailedMessage(e), e.status(), e);
     }
 
-    public DefaultShardOperationFailedException(String index, int shardId, Throwable reason) {
-        this.index = index;
-        this.shardId = shardId;
-        this.reason = reason;
-        this.status = ExceptionsHelper.status(reason);
-    }
-
-    @Override
-    public String index() {
-        return this.index;
-    }
-
-    @Override
-    public int shardId() {
-        return this.shardId;
-    }
-
-    @Override
-    public String reason() {
-        return detailedMessage(reason);
-    }
-
-    @Override
-    public RestStatus status() {
-        return status;
-    }
-
-    @Override
-    public Throwable getCause() {
-        return reason;
+    public DefaultShardOperationFailedException(String index, int shardId, Throwable cause) {
+        super(index, shardId, detailedMessage(cause), ExceptionsHelper.status(cause), cause);
     }
 
     public static DefaultShardOperationFailedException readShardOperationFailed(StreamInput in) throws IOException {
@@ -112,24 +70,17 @@ public static DefaultShardOperationFailedException readShardOperationFailed(Stre
 
     @Override
     public void readFrom(StreamInput in) throws IOException {
-        if (in.readBoolean()) {
-            index = in.readString();
-        }
+        index = in.readOptionalString();
         shardId = in.readVInt();
-        reason = in.readException();
+        cause = in.readException();
         status = RestStatus.readFrom(in);
     }
 
     @Override
     public void writeTo(StreamOutput out) throws IOException {
-        if (index == null) {
-            out.writeBoolean(false);
-        } else {
-            out.writeBoolean(true);
-            out.writeString(index);
-        }
+        out.writeOptionalString(index);
         out.writeVInt(shardId);
-        out.writeException(reason);
+        out.writeException(cause);
         RestStatus.writeTo(out, status);
     }
 
@@ -145,7 +96,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         builder.field("status", status.name());
         if (reason != null) {
             builder.startObject("reason");
-            ElasticsearchException.generateThrowableXContent(builder, params, reason);
+            ElasticsearchException.generateThrowableXContent(builder, params, cause);
             builder.endObject();
         }
         return builder;