From 2f0e0b24265bd94b28235ab76df5e142fa3529ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 18 Jan 2019 09:33:36 +0100 Subject: [PATCH] Allow indices.get_mapping response parsing without types (#37492) This change adds deprecation warning to the indices.get_mapping API in case the "inlcude_type_name" parameter is set to "true" and changes the parsing code in GetMappingsResponse to parse the type-less response instead of the one containing types. As a consequence the HLRC client doesn't need to force "include_type_name=true" any more and the GetMappingsResponseTests can be adapted to the new format as well. Also removing some "include_type_name" parameters in yaml test and docs where not necessary. --- .../client/IndicesRequestConverters.java | 1 - .../elasticsearch/client/IndicesClientIT.java | 4 +- .../client/IndicesRequestConvertersTests.java | 1 - .../IndicesClientDocumentationIT.java | 2 - .../high-level/indices/get_mappings.asciidoc | 1 - .../api/indices.get_mapping.json | 2 +- .../test/indices.get_mapping/10_basic.yml | 2 - .../test/indices.get_mapping/60_empty.yml | 3 - .../mapping/get/GetMappingsResponse.java | 20 ++--- .../admin/indices/RestGetMappingAction.java | 6 ++ .../mapping/get/GetMappingsResponseTests.java | 80 +++++++++++++++---- .../indices/RestGetMappingActionTests.java | 26 ++++++ 12 files changed, 105 insertions(+), 43 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesRequestConverters.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesRequestConverters.java index 79b06467708..f3ce8d2a935 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesRequestConverters.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesRequestConverters.java @@ -150,7 +150,6 @@ final class IndicesRequestConverters { parameters.withMasterTimeout(getMappingsRequest.masterNodeTimeout()); parameters.withIndicesOptions(getMappingsRequest.indicesOptions()); parameters.withLocal(getMappingsRequest.local()); - parameters.putParam(INCLUDE_TYPE_NAME_PARAMETER, "true"); return request; } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java index f3a2fd2baaa..0f299503554 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java @@ -443,9 +443,7 @@ public class IndicesClientIT extends ESRestHighLevelClientTestCase { Map getIndexResponse = getAsMap(indexName); assertEquals("text", XContentMapValues.extractValue(indexName + ".mappings.properties.field.type", getIndexResponse)); - GetMappingsRequest request = new GetMappingsRequest() - .indices(indexName) - .types("_doc"); + GetMappingsRequest request = new GetMappingsRequest().indices(indexName); GetMappingsResponse getMappingsResponse = execute(request, highLevelClient().indices()::getMapping, highLevelClient().indices()::getMappingAsync); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesRequestConvertersTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesRequestConvertersTests.java index 663c40b17a8..308c576edaf 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesRequestConvertersTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesRequestConvertersTests.java @@ -217,7 +217,6 @@ public class IndicesRequestConvertersTests extends ESTestCase { getMappingRequest::indicesOptions, expectedParams); RequestConvertersTests.setRandomMasterTimeout(getMappingRequest, expectedParams); RequestConvertersTests.setRandomLocal(getMappingRequest, expectedParams); - expectedParams.put(INCLUDE_TYPE_NAME_PARAMETER, "true"); Request request = IndicesRequestConverters.getMappings(getMappingRequest); StringJoiner endpoint = new StringJoiner("/", "/", ""); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java index 8f9d8a069fd..3d1b7756221 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java @@ -609,7 +609,6 @@ public class IndicesClientDocumentationIT extends ESRestHighLevelClientTestCase // tag::get-mappings-request GetMappingsRequest request = new GetMappingsRequest(); // <1> request.indices("twitter"); // <2> - request.types("_doc"); // <3> // end::get-mappings-request // tag::get-mappings-request-masterTimeout @@ -665,7 +664,6 @@ public class IndicesClientDocumentationIT extends ESRestHighLevelClientTestCase { GetMappingsRequest request = new GetMappingsRequest(); request.indices("twitter"); - request.types("_doc"); // tag::get-mappings-execute-listener ActionListener listener = diff --git a/docs/java-rest/high-level/indices/get_mappings.asciidoc b/docs/java-rest/high-level/indices/get_mappings.asciidoc index c8616cdab92..a42a8ac77b3 100644 --- a/docs/java-rest/high-level/indices/get_mappings.asciidoc +++ b/docs/java-rest/high-level/indices/get_mappings.asciidoc @@ -18,7 +18,6 @@ include-tagged::{doc-tests-file}[{api}-request] -------------------------------------------------- <1> An empty request that will return all indices and types <2> Setting the indices to fetch mapping for -<3> The types to be returned ==== Optional arguments The following arguments can also optionally be provided: diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_mapping.json b/rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_mapping.json index ccec2ddffdd..d9016ec4024 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_mapping.json +++ b/rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_mapping.json @@ -18,7 +18,7 @@ "params": { "include_type_name": { "type" : "boolean", - "description" : "Whether to add the type name to the response" + "description" : "Whether to add the type name to the response (default: false)" }, "ignore_unavailable": { "type" : "boolean", diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/10_basic.yml index d9ea7d325e3..76519cc4c4c 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/10_basic.yml @@ -5,13 +5,11 @@ setup: reason: include_type_name defaults to true before 7.0 - do: indices.create: - include_type_name: false index: test_1 body: mappings: {} - do: indices.create: - include_type_name: false index: test_2 body: mappings: {} diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/60_empty.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/60_empty.yml index e2a502f30a8..b5069295a1f 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/60_empty.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/60_empty.yml @@ -1,8 +1,5 @@ --- setup: - - skip: - version: " - 6.99.99" - reason: include_type_name defaults to true before 7.0 - do: indices.create: index: test_1 diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java index 4c037bd1d6d..50b7a364268 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java @@ -31,6 +31,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.rest.BaseRestHandler; import java.io.IOException; @@ -101,22 +102,17 @@ public class GetMappingsResponse extends ActionResponse implements ToXContentFra for (Map.Entry entry : parts.entrySet()) { final String indexName = entry.getKey(); assert entry.getValue() instanceof Map : "expected a map as type mapping, but got: " + entry.getValue().getClass(); - @SuppressWarnings("unchecked") - final Map mapping = (Map) ((Map) entry.getValue()).get(MAPPINGS.getPreferredName()); - ImmutableOpenMap.Builder typeBuilder = new ImmutableOpenMap.Builder<>(); - for (Map.Entry typeEntry : mapping.entrySet()) { - final String typeName = typeEntry.getKey(); - assert typeEntry.getValue() instanceof Map : "expected a map as inner type mapping, but got: " + - typeEntry.getValue().getClass(); - @SuppressWarnings("unchecked") - final Map fieldMappings = (Map) typeEntry.getValue(); - MappingMetaData mmd = new MappingMetaData(typeName, fieldMappings); - typeBuilder.put(typeName, mmd); + @SuppressWarnings("unchecked") + final Map fieldMappings = (Map) ((Map) entry.getValue()) + .get(MAPPINGS.getPreferredName()); + if (fieldMappings.isEmpty() == false) { + assert fieldMappings instanceof Map : "expected a map as inner type mapping, but got: " + fieldMappings.getClass(); + MappingMetaData mmd = new MappingMetaData(MapperService.SINGLE_MAPPING_NAME, fieldMappings); + typeBuilder.put(MapperService.SINGLE_MAPPING_NAME, mmd); } builder.put(indexName, typeBuilder.build()); } - return new GetMappingsResponse(builder.build()); } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java index da7f2af501d..8826932e252 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java @@ -20,6 +20,7 @@ package org.elasticsearch.rest.action.admin.indices; import com.carrotsearch.hppc.cursors.ObjectCursor; + import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsRequest; @@ -59,6 +60,8 @@ import static org.elasticsearch.rest.RestRequest.Method.HEAD; public class RestGetMappingAction extends BaseRestHandler { private static final Logger logger = LogManager.getLogger(RestGetMappingAction.class); private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); + static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Using include_type_name in get mapping requests is deprecated. " + + "The parameter will be removed in the next major version."; public RestGetMappingAction(final Settings settings, final RestController controller) { super(settings); @@ -90,6 +93,9 @@ public class RestGetMappingAction extends BaseRestHandler { throw new IllegalArgumentException("Types cannot be provided in get mapping requests, unless" + " include_type_name is set to true."); } + if (request.hasParam(INCLUDE_TYPE_NAME_PARAMETER)) { + deprecationLogger.deprecatedAndMaybeLog("get_mapping_with_types", TYPES_DEPRECATION_MESSAGE); + } final GetMappingsRequest getMappingsRequest = new GetMappingsRequest(); getMappingsRequest.indices(indices).types(types); diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponseTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponseTests.java index 633d74acde1..7d1a19c65ed 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponseTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponseTests.java @@ -24,8 +24,11 @@ import com.carrotsearch.hppc.cursors.ObjectCursor; import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.ToXContent.Params; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.test.AbstractStreamableXContentTestCase; import org.elasticsearch.test.EqualsHashCodeTestUtils; @@ -38,7 +41,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import static org.elasticsearch.rest.BaseRestHandler.INCLUDE_TYPE_NAME_PARAMETER; +import static org.elasticsearch.test.AbstractXContentTestCase.xContentTester; public class GetMappingsResponseTests extends AbstractStreamableXContentTestCase { @@ -86,12 +89,6 @@ public class GetMappingsResponseTests extends AbstractStreamableXContentTestCase return mutate(instance); } - public static ImmutableOpenMap createMappingsForIndex() { - // rarely have no types - int typeCount = rarely() ? 0 : scaledRandomIntBetween(1, 3); - return createMappingsForIndex(typeCount, true); - } - public static ImmutableOpenMap createMappingsForIndex(int typeCount, boolean randomTypeName) { List typeMappings = new ArrayList<>(typeCount); @@ -122,22 +119,18 @@ public class GetMappingsResponseTests extends AbstractStreamableXContentTestCase @Override protected GetMappingsResponse createTestInstance() { + return createTestInstance(true); + } + + private GetMappingsResponse createTestInstance(boolean randomTypeNames) { ImmutableOpenMap.Builder> indexBuilder = ImmutableOpenMap.builder(); - indexBuilder.put("index-" + randomAlphaOfLength(5), createMappingsForIndex()); + int typeCount = rarely() ? 0 : 1; + indexBuilder.put("index-" + randomAlphaOfLength(5), createMappingsForIndex(typeCount, randomTypeNames)); GetMappingsResponse resp = new GetMappingsResponse(indexBuilder.build()); logger.debug("--> created: {}", resp); return resp; } - /** - * For now, we only unit test the legacy typed responses. This will soon no longer be the - * case, as we introduce support for typeless xContent parsing in {@link GetMappingsResponse}. - */ - @Override - protected ToXContent.Params getToXContentParams() { - return new ToXContent.MapParams(Collections.singletonMap(INCLUDE_TYPE_NAME_PARAMETER, "true")); - } - // Not meant to be exhaustive private static Map randomFieldMapping() { Map mappings = new HashMap<>(); @@ -170,4 +163,57 @@ public class GetMappingsResponseTests extends AbstractStreamableXContentTestCase } return mappings; } + + @Override + protected GetMappingsResponse createXContextTestInstance(XContentType xContentType) { + // don't use random type names for XContent roundtrip tests because we cannot parse them back anymore + return createTestInstance(false); + } + + /** + * check that the "old" legacy response format with types works as expected + */ + public void testToXContentWithTypes() throws IOException { + Params params = new ToXContent.MapParams(Collections.singletonMap(BaseRestHandler.INCLUDE_TYPE_NAME_PARAMETER, "true")); + xContentTester(this::createParser, t -> createTestInstance(), params, this::fromXContentWithTypes) + .numberOfTestRuns(NUMBER_OF_TEST_RUNS) + .supportsUnknownFields(supportsUnknownFields()) + .shuffleFieldsExceptions(getShuffleFieldsExceptions()) + .randomFieldsExcludeFilter(getRandomFieldsExcludeFilter()) + .assertEqualsConsumer(this::assertEqualInstances) + .assertToXContentEquivalence(true) + .test(); + } + + /** + * including the pre-7.0 parsing code here to test that older HLRC clients using this can parse the responses that are + * returned when "include_type_name=true" + */ + private GetMappingsResponse fromXContentWithTypes(XContentParser parser) throws IOException { + if (parser.currentToken() == null) { + parser.nextToken(); + } + assert parser.currentToken() == XContentParser.Token.START_OBJECT; + Map parts = parser.map(); + + ImmutableOpenMap.Builder> builder = new ImmutableOpenMap.Builder<>(); + for (Map.Entry entry : parts.entrySet()) { + final String indexName = entry.getKey(); + assert entry.getValue() instanceof Map : "expected a map as type mapping, but got: " + entry.getValue().getClass(); + final Map mapping = (Map) ((Map) entry.getValue()).get("mappings"); + + ImmutableOpenMap.Builder typeBuilder = new ImmutableOpenMap.Builder<>(); + for (Map.Entry typeEntry : mapping.entrySet()) { + final String typeName = typeEntry.getKey(); + assert typeEntry.getValue() instanceof Map : "expected a map as inner type mapping, but got: " + + typeEntry.getValue().getClass(); + final Map fieldMappings = (Map) typeEntry.getValue(); + MappingMetaData mmd = new MappingMetaData(typeName, fieldMappings); + typeBuilder.put(typeName, mmd); + } + builder.put(indexName, typeBuilder.build()); + } + + return new GetMappingsResponse(builder.build()); + } } diff --git a/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingActionTests.java b/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingActionTests.java index 8eea9dc34c2..7ce32e371de 100644 --- a/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingActionTests.java +++ b/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingActionTests.java @@ -28,6 +28,7 @@ import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.rest.FakeRestChannel; import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.test.rest.RestActionTestCase; +import org.junit.Before; import java.util.HashMap; import java.util.Map; @@ -37,6 +38,11 @@ import static org.mockito.Mockito.mock; public class RestGetMappingActionTests extends RestActionTestCase { + @Before + public void setUpAction() { + new RestGetMappingAction(Settings.EMPTY, controller()); + } + public void testTypeExistsDeprecation() throws Exception { Map params = new HashMap<>(); params.put("type", "_doc"); @@ -69,4 +75,24 @@ public class RestGetMappingActionTests extends RestActionTestCase { assertEquals(1, channel.errors().get()); assertEquals(RestStatus.BAD_REQUEST, channel.capturedResponse().status()); } + + /** + * Setting "include_type_name" to true or false should cause a deprecation warning starting in 7.0 + */ + public void testTypeUrlParameterDeprecation() throws Exception { + Map params = new HashMap<>(); + params.put(INCLUDE_TYPE_NAME_PARAMETER, Boolean.toString(randomBoolean())); + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()) + .withMethod(RestRequest.Method.GET) + .withParams(params) + .withPath("/some_index/_mappings") + .build(); + + FakeRestChannel channel = new FakeRestChannel(request, false, 1); + ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + controller().dispatchRequest(request, channel, threadContext); + + assertWarnings(RestGetMappingAction.TYPES_DEPRECATION_MESSAGE); + } + }