From 0282e64bb25a8485526a1b6bf584ac2b9495f219 Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Thu, 18 Apr 2024 23:05:39 -0700 Subject: [PATCH] Ignore BaseRestHandler unconsumed content check as it's always consumed (#13290) * Ignore BaseRestHandler unconsumed content check as it's always consumed Signed-off-by: Daniel Widdis * Remove comment, continue to ignore content on Force Merge Signed-off-by: Daniel Widdis * Remove no-body test from RestDeletePitActionTests Signed-off-by: Daniel Widdis --------- Signed-off-by: Daniel Widdis --- CHANGELOG.md | 1 + .../org/opensearch/rest/BaseRestHandler.java | 4 - .../forcemerge/RestForceMergeActionTests.java | 22 ------ .../opensearch/rest/BaseRestHandlerTests.java | 79 ------------------- .../search/pit/RestDeletePitActionTests.java | 25 ------ 5 files changed, 1 insertion(+), 130 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5efcfee3d9d9d..6d8af16db72a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Enabled mockTelemetryPlugin for IT and fixed OOM issues ([#13054](https://github.com/opensearch-project/OpenSearch/pull/13054)) - Fix implement mark() and markSupported() in class FilterStreamInput ([#13098](https://github.com/opensearch-project/OpenSearch/pull/13098)) - Fix snapshot _status API to return correct status for partial snapshots ([#12812](https://github.com/opensearch-project/OpenSearch/pull/12812)) +- Ignore BaseRestHandler unconsumed content check as it's always consumed. ([#13290](https://github.com/opensearch-project/OpenSearch/pull/13290)) ### Security diff --git a/server/src/main/java/org/opensearch/rest/BaseRestHandler.java b/server/src/main/java/org/opensearch/rest/BaseRestHandler.java index 3552e32022b2c..fc150405747ec 100644 --- a/server/src/main/java/org/opensearch/rest/BaseRestHandler.java +++ b/server/src/main/java/org/opensearch/rest/BaseRestHandler.java @@ -122,10 +122,6 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl throw new IllegalArgumentException(unrecognized(request, unconsumedParams, candidateParams, "parameter")); } - if (request.hasContent() && request.isContentConsumed() == false) { - throw new IllegalArgumentException("request [" + request.method() + " " + request.path() + "] does not support having a body"); - } - usageCount.increment(); // execute the action action.accept(channel); diff --git a/server/src/test/java/org/opensearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java b/server/src/test/java/org/opensearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java index b09e592922ed9..01d72b78a679e 100644 --- a/server/src/test/java/org/opensearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java +++ b/server/src/test/java/org/opensearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java @@ -32,14 +32,9 @@ package org.opensearch.action.admin.indices.forcemerge; -import org.opensearch.client.node.NodeClient; -import org.opensearch.common.xcontent.json.JsonXContent; -import org.opensearch.core.common.bytes.BytesArray; -import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestRequest; import org.opensearch.rest.action.admin.indices.RestForceMergeAction; -import org.opensearch.test.rest.FakeRestChannel; import org.opensearch.test.rest.FakeRestRequest; import org.opensearch.test.rest.RestActionTestCase; import org.junit.Before; @@ -47,9 +42,6 @@ import java.util.HashMap; import java.util.Map; -import static org.hamcrest.Matchers.equalTo; -import static org.mockito.Mockito.mock; - public class RestForceMergeActionTests extends RestActionTestCase { @Before @@ -57,20 +49,6 @@ public void setUpAction() { controller().registerHandler(new RestForceMergeAction()); } - public void testBodyRejection() throws Exception { - final RestForceMergeAction handler = new RestForceMergeAction(); - String json = JsonXContent.contentBuilder().startObject().field("max_num_segments", 1).endObject().toString(); - final FakeRestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent( - new BytesArray(json), - MediaTypeRegistry.JSON - ).withPath("/_forcemerge").build(); - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> handler.handleRequest(request, new FakeRestChannel(request, randomBoolean(), 1), mock(NodeClient.class)) - ); - assertThat(e.getMessage(), equalTo("request [GET /_forcemerge] does not support having a body")); - } - public void testDeprecationMessage() { final Map params = new HashMap<>(); params.put("only_expunge_deletes", Boolean.TRUE.toString()); diff --git a/server/src/test/java/org/opensearch/rest/BaseRestHandlerTests.java b/server/src/test/java/org/opensearch/rest/BaseRestHandlerTests.java index ce929e64d8960..45653e9d8e4d6 100644 --- a/server/src/test/java/org/opensearch/rest/BaseRestHandlerTests.java +++ b/server/src/test/java/org/opensearch/rest/BaseRestHandlerTests.java @@ -35,10 +35,6 @@ import org.opensearch.client.node.NodeClient; import org.opensearch.common.Table; import org.opensearch.common.settings.Settings; -import org.opensearch.common.xcontent.json.JsonXContent; -import org.opensearch.core.common.bytes.BytesArray; -import org.opensearch.core.xcontent.MediaTypeRegistry; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.rest.RestHandler.ReplacedRoute; import org.opensearch.rest.RestHandler.Route; import org.opensearch.rest.RestRequest.Method; @@ -281,81 +277,6 @@ public String getName() { assertTrue(executed.get()); } - public void testConsumedBody() throws Exception { - final AtomicBoolean executed = new AtomicBoolean(); - final BaseRestHandler handler = new BaseRestHandler() { - @Override - protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { - request.content(); - return channel -> executed.set(true); - } - - @Override - public String getName() { - return "test_consumed_body"; - } - }; - - try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) { - final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( - new BytesArray(builder.toString()), - MediaTypeRegistry.JSON - ).build(); - final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); - handler.handleRequest(request, channel, mockClient); - assertTrue(executed.get()); - } - } - - public void testUnconsumedNoBody() throws Exception { - final AtomicBoolean executed = new AtomicBoolean(); - final BaseRestHandler handler = new BaseRestHandler() { - @Override - protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { - return channel -> executed.set(true); - } - - @Override - public String getName() { - return "test_unconsumed_body"; - } - }; - - final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).build(); - final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); - handler.handleRequest(request, channel, mockClient); - assertTrue(executed.get()); - } - - public void testUnconsumedBody() throws IOException { - final AtomicBoolean executed = new AtomicBoolean(); - final BaseRestHandler handler = new BaseRestHandler() { - @Override - protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { - return channel -> executed.set(true); - } - - @Override - public String getName() { - return "test_unconsumed_body"; - } - }; - - try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) { - final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( - new BytesArray(builder.toString()), - MediaTypeRegistry.JSON - ).build(); - final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> handler.handleRequest(request, channel, mockClient) - ); - assertThat(e, hasToString(containsString("request [GET /] does not support having a body"))); - assertFalse(executed.get()); - } - } - public void testReplaceRoutesMethod() throws Exception { List routes = Arrays.asList(new Route(Method.GET, "/path/test"), new Route(Method.PUT, "/path2/test")); List replacedRoutes = RestHandler.replaceRoutes(routes, "/prefix", "/deprecatedPrefix"); 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 b60541825e3ed..448ba9e5a8cd7 100644 --- a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java +++ b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java @@ -82,31 +82,6 @@ public void deletePits(DeletePitRequest request, ActionListener pitCalled = new SetOnce<>(); - try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { - @Override - public void deletePits(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\"]}"), - MediaTypeRegistry.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()) {