Fix validation of close_point_in_time request (#88702) (#88730)

ClosePointInTimeRequest#validate should return a validation error for an invalid pit_id, but it throws an IAE instead. This mistake prevents close_point_in_time tasks of requests with empty pit_id from being removed.
elastic · Jul 22, 2022 · 801fed8 · 801fed8
1 parent 8f0d91b
commit 801fed8
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 4 deletions.
diff --git a/docs/changelog/88702.yaml b/docs/changelog/88702.yaml
@@ -0,0 +1,5 @@
+pr: 88702
+summary: Fix validation of `close_pit` request
+area: Search
+type: bug
+issues: []
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/action/search/PointInTimeIT.java b/server/src/internalClusterTest/java/org/elasticsearch/action/search/PointInTimeIT.java
@@ -29,6 +29,7 @@
 import org.elasticsearch.search.sort.SortBuilder;
 import org.elasticsearch.search.sort.SortBuilders;
 import org.elasticsearch.search.sort.SortOrder;
+import org.elasticsearch.tasks.TaskInfo;
 import org.elasticsearch.test.ESIntegTestCase;
 
 import java.util.HashSet;
@@ -42,6 +43,7 @@
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
 import static org.hamcrest.Matchers.arrayWithSize;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.everyItem;
 import static org.hamcrest.Matchers.in;
@@ -418,6 +420,12 @@ public void testPITTiebreak() throws Exception {
         }
     }
 
+    public void testCloseInvalidPointInTime() {
+        expectThrows(Exception.class, () -> client().execute(ClosePointInTimeAction.INSTANCE, new ClosePointInTimeRequest("")).actionGet());
+        List<TaskInfo> tasks = client().admin().cluster().prepareListTasks().setActions(ClosePointInTimeAction.NAME).get().getTasks();
+        assertThat(tasks, empty());
+    }
+
     @SuppressWarnings({ "rawtypes", "unchecked" })
     private void assertPagination(PointInTimeBuilder pit, int expectedNumDocs, int size, SortBuilder<?>... sorts) throws Exception {
         Set<String> seen = new HashSet<>();

diff --git a/server/src/main/java/org/elasticsearch/action/search/ClosePointInTimeRequest.java b/server/src/main/java/org/elasticsearch/action/search/ClosePointInTimeRequest.java
@@ -10,6 +10,7 @@
 
 import org.elasticsearch.action.ActionRequest;
 import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.action.ValidateActions;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -41,7 +42,7 @@ public String getId() {
     @Override
     public ActionRequestValidationException validate() {
         if (Strings.isEmpty(id)) {
-            throw new IllegalArgumentException("reader id must be specified");
+            return ValidateActions.addValidationError("id is empty", null);
         }
         return null;
     }

diff --git a/server/src/main/java/org/elasticsearch/action/support/TransportAction.java b/server/src/main/java/org/elasticsearch/action/support/TransportAction.java
@@ -40,12 +40,19 @@ protected TransportAction(String actionName, ActionFilters actionFilters, TaskMa
      * Use this method when the transport action should continue to run in the context of the current task
      */
     public final void execute(Task task, Request request, ActionListener<Response> listener) {
-        ActionRequestValidationException validationException = request.validate();
+        final ActionRequestValidationException validationException;
+        try {
+            validationException = request.validate();
+        } catch (Exception e) {
+            assert false : new AssertionError("validating of request [" + request + "] threw exception", e);
+            logger.warn("validating of request [" + request + "] threw exception", e);
+            listener.onFailure(e);
+            return;
+        }
         if (validationException != null) {
             listener.onFailure(validationException);
             return;
         }
-
         if (task != null && request.getShouldStoreResult()) {
             listener = new TaskResultStoringActionListener<>(taskManager, task, listener);
         }

diff --git a/...plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/PutLifecycleAction.java b/...plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/PutLifecycleAction.java
@@ -8,6 +8,7 @@
 
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.ActionType;
+import org.elasticsearch.action.ValidateActions;
 import org.elasticsearch.action.support.master.AcknowledgedRequest;
 import org.elasticsearch.action.support.master.AcknowledgedResponse;
 import org.elasticsearch.common.Strings;
@@ -62,8 +63,12 @@ public LifecyclePolicy getPolicy() {
 
         @Override
         public ActionRequestValidationException validate() {
-            this.policy.validate();
             ActionRequestValidationException err = null;
+            try {
+                this.policy.validate();
+            } catch (IllegalArgumentException iae) {
+                err = ValidateActions.addValidationError(iae.getMessage(), null);
+            }
             String phaseTimingErr = TimeseriesLifecycleType.validateMonotonicallyIncreasingPhaseTimings(this.policy.getPhases().values());
             if (Strings.hasText(phaseTimingErr)) {
                 err = new ActionRequestValidationException();