From 33152f648fce649b774fdb659bc47a7134755c60 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 12 Dec 2018 10:38:02 -0800 Subject: [PATCH] Fix some inconsistencies in the types deprecation code. (#36517) * Make sure to test conversion for both typed and typeless HLRC requests. * Update a few more statements to deprecatedAndMaybeLog. * Make sure Rest*SearchTemplateActionTests extend RestActionTestCase. --- .../client/RequestConvertersTests.java | 80 ++++++++++++++++--- .../RestMultiSearchTemplateActionTests.java | 36 ++------- .../RestSearchTemplateActionTests.java | 36 ++------- .../rest/action/document/RestGetAction.java | 2 +- .../action/document/RestMultiGetAction.java | 2 +- .../rest/action/search/RestExplainAction.java | 2 +- 6 files changed, 90 insertions(+), 68 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java index a4891355bca..f0b918b033a 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java @@ -152,6 +152,10 @@ public class RequestConvertersTests extends ESTestCase { getAndExistsTest(RequestConverters::get, HttpGet.METHOD_NAME); } + public void testGetWithType() { + getAndExistsWithTypeTest(RequestConverters::get, HttpGet.METHOD_NAME); + } + public void testMultiGet() throws IOException { Map expectedParams = new HashMap<>(); MultiGetRequest multiGetRequest = new MultiGetRequest(); @@ -175,7 +179,7 @@ public class RequestConvertersTests extends ESTestCase { int numberOfRequests = randomIntBetween(0, 32); for (int i = 0; i < numberOfRequests; i++) { - MultiGetRequest.Item item = new MultiGetRequest.Item(randomAlphaOfLength(4), randomAlphaOfLength(4), randomAlphaOfLength(4)); + MultiGetRequest.Item item = new MultiGetRequest.Item(randomAlphaOfLength(4), randomAlphaOfLength(4)); if (randomBoolean()) { item.routing(randomAlphaOfLength(4)); } @@ -201,11 +205,23 @@ public class RequestConvertersTests extends ESTestCase { assertToXContentBody(multiGetRequest, request.getEntity()); } + public void testMultiGetWithType() throws IOException { + MultiGetRequest multiGetRequest = new MultiGetRequest(); + MultiGetRequest.Item item = new MultiGetRequest.Item(randomAlphaOfLength(4), + randomAlphaOfLength(4), + randomAlphaOfLength(4)); + multiGetRequest.add(item); + + Request request = RequestConverters.multiGet(multiGetRequest); + assertEquals(HttpPost.METHOD_NAME, request.getMethod()); + assertEquals("/_mget", request.getEndpoint()); + assertToXContentBody(multiGetRequest, request.getEntity()); + } + public void testDelete() { String index = randomAlphaOfLengthBetween(3, 10); - String type = randomAlphaOfLengthBetween(3, 10); String id = randomAlphaOfLengthBetween(3, 10); - DeleteRequest deleteRequest = new DeleteRequest(index, type, id); + DeleteRequest deleteRequest = new DeleteRequest(index, id); Map expectedParams = new HashMap<>(); @@ -223,9 +239,21 @@ public class RequestConvertersTests extends ESTestCase { } Request request = RequestConverters.delete(deleteRequest); - assertEquals("/" + index + "/" + type + "/" + id, request.getEndpoint()); - assertEquals(expectedParams, request.getParameters()); assertEquals(HttpDelete.METHOD_NAME, request.getMethod()); + assertEquals("/" + index + "/_doc/" + id, request.getEndpoint()); + assertEquals(expectedParams, request.getParameters()); + assertNull(request.getEntity()); + } + + public void testDeleteWithType() { + String index = randomAlphaOfLengthBetween(3, 10); + String type = randomAlphaOfLengthBetween(3, 10); + String id = randomAlphaOfLengthBetween(3, 10); + DeleteRequest deleteRequest = new DeleteRequest(index, type, id); + + Request request = RequestConverters.delete(deleteRequest); + assertEquals(HttpDelete.METHOD_NAME, request.getMethod()); + assertEquals("/" + index + "/" + type + "/" + id, request.getEndpoint()); assertNull(request.getEntity()); } @@ -233,11 +261,14 @@ public class RequestConvertersTests extends ESTestCase { getAndExistsTest(RequestConverters::exists, HttpHead.METHOD_NAME); } + public void testExistsWithType() { + getAndExistsWithTypeTest(RequestConverters::exists, HttpHead.METHOD_NAME); + } + private static void getAndExistsTest(Function requestConverter, String method) { String index = randomAlphaOfLengthBetween(3, 10); - String type = randomAlphaOfLengthBetween(3, 10); String id = randomAlphaOfLengthBetween(3, 10); - GetRequest getRequest = new GetRequest(index, type, id); + GetRequest getRequest = new GetRequest(index, id); Map expectedParams = new HashMap<>(); if (randomBoolean()) { @@ -285,12 +316,24 @@ public class RequestConvertersTests extends ESTestCase { } } Request request = requestConverter.apply(getRequest); - assertEquals("/" + index + "/" + type + "/" + id, request.getEndpoint()); + assertEquals("/" + index + "/_doc/" + id, request.getEndpoint()); assertEquals(expectedParams, request.getParameters()); assertNull(request.getEntity()); assertEquals(method, request.getMethod()); } + private static void getAndExistsWithTypeTest(Function requestConverter, String method) { + String index = randomAlphaOfLengthBetween(3, 10); + String type = randomAlphaOfLengthBetween(3, 10); + String id = randomAlphaOfLengthBetween(3, 10); + GetRequest getRequest = new GetRequest(index, type, id); + + Request request = requestConverter.apply(getRequest); + assertEquals("/" + index + "/" + type + "/" + id, request.getEndpoint()); + assertNull(request.getEntity()); + assertEquals(method, request.getMethod()); + } + public void testReindex() throws IOException { ReindexRequest reindexRequest = new ReindexRequest(); reindexRequest.setSourceIndices("source_idx"); @@ -1335,7 +1378,26 @@ public class RequestConvertersTests extends ESTestCase { int numberOfRequests = randomIntBetween(0, 5); for (int i = 0; i < numberOfRequests; i++) { String index = randomAlphaOfLengthBetween(3, 10); - String type = randomBoolean() ? null : randomAlphaOfLengthBetween(3, 10); + String id = randomAlphaOfLengthBetween(3, 10); + TermVectorsRequest tvRequest = new TermVectorsRequest(index, id); + String[] fields = generateRandomStringArray(10, 5, false, false); + tvRequest.setFields(fields); + mtvRequest.add(tvRequest); + } + + Request request = RequestConverters.mtermVectors(mtvRequest); + assertEquals(HttpGet.METHOD_NAME, request.getMethod()); + assertEquals("_mtermvectors", request.getEndpoint()); + assertToXContentBody(mtvRequest, request.getEntity()); + } + + public void testMultiTermVectorsWithType() throws IOException { + MultiTermVectorsRequest mtvRequest = new MultiTermVectorsRequest(); + + int numberOfRequests = randomIntBetween(0, 5); + for (int i = 0; i < numberOfRequests; i++) { + String index = randomAlphaOfLengthBetween(3, 10); + String type = randomAlphaOfLengthBetween(3, 10); String id = randomAlphaOfLengthBetween(3, 10); TermVectorsRequest tvRequest = new TermVectorsRequest(index, type, id); String[] fields = generateRandomStringArray(10, 5, false, false); diff --git a/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestMultiSearchTemplateActionTests.java b/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestMultiSearchTemplateActionTests.java index ba9f4933143..eacb1e3c4e8 100644 --- a/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestMultiSearchTemplateActionTests.java +++ b/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestMultiSearchTemplateActionTests.java @@ -18,35 +18,21 @@ */ package org.elasticsearch.script.mustache; -import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.xcontent.XContentType; -import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; -import org.elasticsearch.rest.RestChannel; -import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.test.rest.FakeRestChannel; import org.elasticsearch.test.rest.FakeRestRequest; -import org.elasticsearch.usage.UsageService; +import org.elasticsearch.test.rest.RestActionTestCase; +import org.junit.Before; import java.nio.charset.StandardCharsets; -import java.util.Collections; -import static org.mockito.Mockito.mock; +public class RestMultiSearchTemplateActionTests extends RestActionTestCase { -public class RestMultiSearchTemplateActionTests extends ESTestCase { - private RestController controller; - - public void setUp() throws Exception { - super.setUp(); - controller = new RestController(Collections.emptySet(), null, - mock(NodeClient.class), - new NoneCircuitBreakerService(), - new UsageService()); - new RestMultiSearchTemplateAction(Settings.EMPTY, controller); + @Before + public void setUpAction() { + new RestMultiSearchTemplateAction(Settings.EMPTY, controller()); } public void testTypeInPath() { @@ -60,7 +46,7 @@ public class RestMultiSearchTemplateActionTests extends ESTestCase { .withContent(bytesContent, XContentType.JSON) .build(); - performRequest(request); + dispatchRequest(request); assertWarnings(RestMultiSearchTemplateAction.TYPES_DEPRECATION_MESSAGE); } @@ -74,13 +60,7 @@ public class RestMultiSearchTemplateActionTests extends ESTestCase { .withContent(bytesContent, XContentType.JSON) .build(); - performRequest(request); + dispatchRequest(request); assertWarnings(RestMultiSearchTemplateAction.TYPES_DEPRECATION_MESSAGE); } - - private void performRequest(RestRequest request) { - RestChannel channel = new FakeRestChannel(request, false, 1); - ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - controller.dispatchRequest(request, channel, threadContext); - } } diff --git a/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestSearchTemplateActionTests.java b/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestSearchTemplateActionTests.java index ac05e0b30ec..0da8afbae04 100644 --- a/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestSearchTemplateActionTests.java +++ b/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/RestSearchTemplateActionTests.java @@ -18,35 +18,21 @@ */ package org.elasticsearch.script.mustache; -import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; -import org.elasticsearch.rest.RestChannel; -import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.search.RestSearchAction; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.test.rest.FakeRestChannel; import org.elasticsearch.test.rest.FakeRestRequest; -import org.elasticsearch.usage.UsageService; +import org.elasticsearch.test.rest.RestActionTestCase; +import org.junit.Before; -import java.util.Collections; import java.util.HashMap; import java.util.Map; -import static org.mockito.Mockito.mock; +public class RestSearchTemplateActionTests extends RestActionTestCase { -public class RestSearchTemplateActionTests extends ESTestCase { - private RestController controller; - - public void setUp() throws Exception { - super.setUp(); - controller = new RestController(Collections.emptySet(), null, - mock(NodeClient.class), - new NoneCircuitBreakerService(), - new UsageService()); - new RestSearchTemplateAction(Settings.EMPTY, controller); + @Before + public void setUpAction() { + new RestSearchTemplateAction(Settings.EMPTY, controller()); } public void testTypeInPath() { @@ -55,7 +41,7 @@ public class RestSearchTemplateActionTests extends ESTestCase { .withPath("/some_index/some_type/_search/template") .build(); - performRequest(request); + dispatchRequest(request); assertWarnings(RestSearchAction.TYPES_DEPRECATION_MESSAGE); } @@ -69,13 +55,7 @@ public class RestSearchTemplateActionTests extends ESTestCase { .withParams(params) .build(); - performRequest(request); + dispatchRequest(request); assertWarnings(RestSearchAction.TYPES_DEPRECATION_MESSAGE); } - - private void performRequest(RestRequest request) { - RestChannel channel = new FakeRestChannel(request, false, 1); - ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - controller.dispatchRequest(request, channel, threadContext); - } } diff --git a/server/src/main/java/org/elasticsearch/rest/action/document/RestGetAction.java b/server/src/main/java/org/elasticsearch/rest/action/document/RestGetAction.java index bbf4d5b8601..c54f4b9d23d 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/document/RestGetAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/document/RestGetAction.java @@ -64,7 +64,7 @@ public class RestGetAction extends BaseRestHandler { public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { String type = request.param("type"); if (!type.equals(MapperService.SINGLE_MAPPING_NAME)) { - deprecationLogger.deprecated(TYPES_DEPRECATION_MESSAGE); + deprecationLogger.deprecatedAndMaybeLog("get_with_types", TYPES_DEPRECATION_MESSAGE); } final GetRequest getRequest = new GetRequest(request.param("index"), type, request.param("id")); diff --git a/server/src/main/java/org/elasticsearch/rest/action/document/RestMultiGetAction.java b/server/src/main/java/org/elasticsearch/rest/action/document/RestMultiGetAction.java index 2f630f95241..cc2fd6ba1db 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/document/RestMultiGetAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/document/RestMultiGetAction.java @@ -65,7 +65,7 @@ public class RestMultiGetAction extends BaseRestHandler { @Override public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { if (request.param("type") != null) { - deprecationLogger.deprecated(TYPES_DEPRECATION_MESSAGE); + deprecationLogger.deprecatedAndMaybeLog("mget_with_types", TYPES_DEPRECATION_MESSAGE); } MultiGetRequest multiGetRequest = new MultiGetRequest(); diff --git a/server/src/main/java/org/elasticsearch/rest/action/search/RestExplainAction.java b/server/src/main/java/org/elasticsearch/rest/action/search/RestExplainAction.java index 6bac017c04c..6d2cc7e4444 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/search/RestExplainAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/search/RestExplainAction.java @@ -64,7 +64,7 @@ public class RestExplainAction extends BaseRestHandler { public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { ExplainRequest explainRequest; if (request.hasParam("type")) { - deprecationLogger.deprecated(TYPES_DEPRECATION_MESSAGE); + deprecationLogger.deprecatedAndMaybeLog("explain_with_types", TYPES_DEPRECATION_MESSAGE); explainRequest = new ExplainRequest(request.param("index"), request.param("type"), request.param("id"));