From fa1f2cd22a8c7557b51ff879184288580b2686fb Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Wed, 4 May 2022 09:45:24 +0530 Subject: [PATCH] Adding java docs and addressing comments Signed-off-by: Bharathwaj G --- .../action/search/DeletePITAction.java | 3 + .../action/search/DeletePITResponse.java | 3 + .../action/search/RestDeletePITAction.java | 30 +++++---- .../search/TransportDeletePITActionTests.java | 3 + .../search/DeletePitMultiNodeTests.java | 3 + .../search/pit/RestDeletePitActionTests.java | 66 ++++++++++++++++--- 6 files changed, 87 insertions(+), 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 index 7f043a365c403..6048996037bbe 100644 --- 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 { 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 index 220f5377bc1ce..5c3f66b0ad293 100644 --- 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 index 26739d3749f92..5fdf8a9d7a6da 100644 --- 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(channel)); } @Override public List 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 index 347c5a11630de..abe2f55917969 100644 --- 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 index 9bfb3a76220bc..2836594fa4d37 100644 --- 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 index 1798dcdf100df..3c59fe259074a 100644 --- 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 pitCalled = new SetOnce<>(); try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { @Override @@ -49,9 +52,29 @@ public void deletePit(DeletePITRequest request, ActionListener pitCalled = new SetOnce<>(); + try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { + @Override + public void deletePit(DeletePITRequest request, ActionListener 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); @@ -59,7 +82,32 @@ public void deletePit(DeletePITRequest request, ActionListener pitCalled = new SetOnce<>(); + try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { + @Override + public void deletePit(DeletePITRequest request, ActionListener 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 pitCalled = new SetOnce<>(); try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { @Override @@ -75,9 +123,11 @@ public void deletePit(DeletePITRequest request, ActionListener action.handleRequest(request, channel, nodeClient) + ); + assertTrue(ex.getMessage().contains("unrecognized param")); } } }