Adding java docs and addressing comments

Signed-off-by: Bharathwaj G <[email protected]>
bharath-techie · May 4, 2022 · fa1f2cd · fa1f2cd
1 parent 51ce82f
commit fa1f2cd
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 21 deletions.
diff --git a/server/src/main/java/org/opensearch/action/search/DeletePITAction.java b/server/src/main/java/org/opensearch/action/search/DeletePITAction.java
@@ -10,6 +10,9 @@
 
 import org.opensearch.action.ActionType;
 
+/**
+ * Action type for deleting PIT reader contexts
+ */
 public class DeletePITAction extends ActionType<DeletePITResponse> {
 
     public static final DeletePITAction INSTANCE = new DeletePITAction();

diff --git a/server/src/main/java/org/opensearch/action/search/DeletePITResponse.java b/server/src/main/java/org/opensearch/action/search/DeletePITResponse.java
@@ -26,6 +26,9 @@
 import static org.opensearch.rest.RestStatus.NOT_FOUND;
 import static org.opensearch.rest.RestStatus.OK;
 
+/**
+ * Response class for delete pit flow which returns if the contexts are freed
+ */
 public class DeletePITResponse extends ActionResponse implements StatusToXContentObject {
 
     /**

diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestDeletePITAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestDeletePITAction.java
@@ -11,7 +11,6 @@
 import org.opensearch.action.search.DeletePITRequest;
 import org.opensearch.action.search.DeletePITResponse;
 import org.opensearch.client.node.NodeClient;
-import org.opensearch.common.Strings;
 import org.opensearch.rest.BaseRestHandler;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.rest.action.RestStatusToXContentListener;
@@ -23,6 +22,9 @@
 import static java.util.Collections.unmodifiableList;
 import static org.opensearch.rest.RestRequest.Method.DELETE;
 
+/**
+ * Rest action for deleting PIT contexts
+ */
 public class RestDeletePITAction extends BaseRestHandler {
 
     @Override
@@ -32,24 +34,26 @@ public String getName() {
 
     @Override
     public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
-        String pitIds = request.param("pit_id");
+        String allPitIdsQualifier = "_all";
         DeletePITRequest deletePITRequest = new DeletePITRequest();
-        deletePITRequest.setPitIds(asList(Strings.splitStringByCommaToArray(pitIds)));
-        request.withContentOrSourceParamParserOrNull((xContentParser -> {
-            if (xContentParser != null) {
-                // NOTE: if rest request with xcontent body has request parameters, values parsed from request body have the precedence
-                try {
-                    deletePITRequest.fromXContent(xContentParser);
-                } catch (IOException e) {
-                    throw new IllegalArgumentException("Failed to parse request body", e);
+        if (request.path().contains(allPitIdsQualifier)) {
+            deletePITRequest.setPitIds(asList(allPitIdsQualifier));
+        } else {
+            request.withContentOrSourceParamParserOrNull((xContentParser -> {
+                if (xContentParser != null) {
+                    try {
+                        deletePITRequest.fromXContent(xContentParser);
+                    } catch (IOException e) {
+                        throw new IllegalArgumentException("Failed to parse request body", e);
+                    }
                 }
-            }
-        }));
+            }));
+        }
         return channel -> client.deletePit(deletePITRequest, new RestStatusToXContentListener<DeletePITResponse>(channel));
     }
 
     @Override
     public List<Route> routes() {
-        return unmodifiableList(asList(new Route(DELETE, "/_search/_point_in_time"), new Route(DELETE, "/_search/_point_in_time/{id}")));
+        return unmodifiableList(asList(new Route(DELETE, "/_search/_point_in_time"), new Route(DELETE, "/_search/_point_in_time/_all")));
     }
 }
diff --git a/server/src/test/java/org/opensearch/action/search/TransportDeletePITActionTests.java b/server/src/test/java/org/opensearch/action/search/TransportDeletePITActionTests.java
@@ -51,6 +51,9 @@
 import static org.mockito.Mockito.when;
 import static org.opensearch.action.support.PlainActionFuture.newFuture;
 
+/**
+ * Functional tests for transport delete pit action
+ */
 public class TransportDeletePITActionTests extends OpenSearchTestCase {
 
     DiscoveryNode node1 = null;

diff --git a/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java b/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java
@@ -31,6 +31,9 @@
 
 import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
 
+/**
+ * Multi node integration tests for delete PIT use cases
+ */
 @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.SUITE, numDataNodes = 2)
 public class DeletePitMultiNodeTests extends OpenSearchIntegTestCase {
 

diff --git a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java
@@ -27,6 +27,9 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 
+/**
+ * Tests to verify the behavior of rest delete pit action for list delete and delete all PIT endpoints
+ */
 public class RestDeletePitActionTests extends OpenSearchTestCase {
     public void testParseDeletePitRequestWithInvalidJsonThrowsException() throws Exception {
         RestDeletePITAction action = new RestDeletePITAction();
@@ -38,7 +41,7 @@ public void testParseDeletePitRequestWithInvalidJsonThrowsException() throws Exc
         assertThat(e.getMessage(), equalTo("Failed to parse request body"));
     }
 
-    public void testBodyParamsOverrideQueryStringParams() throws Exception {
+    public void testDeletePitWithBody() throws Exception {
         SetOnce<Boolean> pitCalled = new SetOnce<>();
         try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) {
             @Override
@@ -49,17 +52,62 @@ public void deletePit(DeletePITRequest request, ActionListener<DeletePITResponse
             }
         }) {
             RestDeletePITAction action = new RestDeletePITAction();
-            RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(
-                Collections.singletonMap("pit_id", "QUERY_STRING")
-            ).withContent(new BytesArray("{\"pit_id\": [\"BODY\"]}"), XContentType.JSON).build();
+            RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent(
+                new BytesArray("{\"pit_id\": [\"BODY\"]}"),
+                XContentType.JSON
+            ).build();
+            FakeRestChannel channel = new FakeRestChannel(request, false, 0);
+            action.handleRequest(request, channel, nodeClient);
+
+            assertThat(pitCalled.get(), equalTo(true));
+        }
+    }
+
+    public void testDeleteAllPit() throws Exception {
+        SetOnce<Boolean> pitCalled = new SetOnce<>();
+        try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) {
+            @Override
+            public void deletePit(DeletePITRequest request, ActionListener<DeletePITResponse> listener) {
+                pitCalled.set(true);
+                assertThat(request.getPitIds(), hasSize(1));
+                assertThat(request.getPitIds().get(0), equalTo("_all"));
+            }
+        }) {
+            RestDeletePITAction action = new RestDeletePITAction();
+            RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build();
             FakeRestChannel channel = new FakeRestChannel(request, false, 0);
             action.handleRequest(request, channel, nodeClient);
 
             assertThat(pitCalled.get(), equalTo(true));
         }
     }
 
-    public void testDeletePitQueryStringParams() throws Exception {
+    public void testDeleteAllPitWithBody() throws Exception {
+        SetOnce<Boolean> pitCalled = new SetOnce<>();
+        try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) {
+            @Override
+            public void deletePit(DeletePITRequest request, ActionListener<DeletePITResponse> listener) {
+                pitCalled.set(true);
+                assertThat(request.getPitIds(), hasSize(1));
+                assertThat(request.getPitIds().get(0), equalTo("_all"));
+            }
+        }) {
+            RestDeletePITAction action = new RestDeletePITAction();
+            RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent(
+                new BytesArray("{\"pit_id\": [\"BODY\"]}"),
+                XContentType.JSON
+            ).withPath("/_all").build();
+            FakeRestChannel channel = new FakeRestChannel(request, false, 0);
+
+            IllegalArgumentException ex = expectThrows(
+                IllegalArgumentException.class,
+                () -> action.handleRequest(request, channel, nodeClient)
+            );
+            assertTrue(ex.getMessage().contains("request [GET /_all] does not support having a body"));
+        }
+    }
+
+    public void testDeletePitQueryStringParamsShouldThrowException() {
         SetOnce<Boolean> pitCalled = new SetOnce<>();
         try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) {
             @Override
@@ -75,9 +123,11 @@ public void deletePit(DeletePITRequest request, ActionListener<DeletePITResponse
                 Collections.singletonMap("pit_id", "QUERY_STRING,QUERY_STRING_1")
             ).build();
             FakeRestChannel channel = new FakeRestChannel(request, false, 0);
-            action.handleRequest(request, channel, nodeClient);
-
-            assertThat(pitCalled.get(), equalTo(true));
+            IllegalArgumentException ex = expectThrows(
+                IllegalArgumentException.class,
+                () -> action.handleRequest(request, channel, nodeClient)
+            );
+            assertTrue(ex.getMessage().contains("unrecognized param"));
         }
     }
 }