Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow indices.get_mapping response parsing without types #37492

Merged
merged 5 commits into from
Jan 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ static Request getMappings(GetMappingsRequest getMappingsRequest) throws IOExcep
parameters.withMasterTimeout(getMappingsRequest.masterNodeTimeout());
parameters.withIndicesOptions(getMappingsRequest.indicesOptions());
parameters.withLocal(getMappingsRequest.local());
parameters.putParam(INCLUDE_TYPE_NAME_PARAMETER, "true");

return request;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,9 +443,7 @@ public void testGetMapping() throws IOException {
Map<String, Object> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,6 @@ public void testGetMapping() throws IOException {
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("/", "/", "");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,6 @@ public void testGetMapping() throws IOException {
// 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
Expand Down Expand Up @@ -665,7 +664,6 @@ public void testGetMappingAsync() throws Exception {
{
GetMappingsRequest request = new GetMappingsRequest();
request.indices("twitter");
request.types("_doc");

// tag::get-mappings-execute-listener
ActionListener<GetMappingsResponse> listener =
Expand Down
1 change: 0 additions & 1 deletion docs/java-rest/high-level/indices/get_mappings.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: {}
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
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;
Expand Down Expand Up @@ -101,22 +102,17 @@ public static GetMappingsResponse fromXContent(XContentParser parser) throws IOE
for (Map.Entry<String, Object> 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<String, Object> mapping = (Map<String, Object>) ((Map<String, ?>) entry.getValue()).get(MAPPINGS.getPreferredName());

ImmutableOpenMap.Builder<String, MappingMetaData> typeBuilder = new ImmutableOpenMap.Builder<>();
for (Map.Entry<String, Object> 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<String, Object> fieldMappings = (Map<String, Object>) typeEntry.getValue();
MappingMetaData mmd = new MappingMetaData(typeName, fieldMappings);
typeBuilder.put(typeName, mmd);
@SuppressWarnings("unchecked")
final Map<String, Object> fieldMappings = (Map<String, Object>) ((Map<String, ?>) 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that we'll revisit the interface to GetMappingsResponse for the HLRC, as we plan to do for GetIndexResponse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say so, as I'm still not conviced we if/how we should do this just now, so I'd like to discuss this more focussed in a separate move.

}
builder.put(indexName, typeBuilder.build());
}

return new GetMappingsResponse(builder.build());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -59,6 +60,8 @@
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.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #37484 I opted for a general "include_type_name is deprecated" message as that is our system-wide policy. I'm not sure if it's more useful to warn people they shouldn't be using the param anywhere or that it was detected in use on a particular API as in this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like that the deprecation explicitely says which API call this warning comes from. You might be able to see the same from the logger, but then again maybe not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been taking the same approach as @cbuescher, so it is really clear what API call is causing the warning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last (small) comment I thought of -- we should probably standardize on using `include_type_name` (with backticks) vs. include_type_name in these messages, for easy searchability. I think I would vote for no backticks, since I don't see them used often in our logging messages?


public RestGetMappingAction(final Settings settings, final RestController controller) {
super(settings);
Expand Down Expand Up @@ -90,6 +93,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@
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;

Expand All @@ -38,7 +41,7 @@
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<GetMappingsResponse> {

Expand Down Expand Up @@ -86,12 +89,6 @@ protected GetMappingsResponse mutateInstance(GetMappingsResponse instance) throw
return mutate(instance);
}

public static ImmutableOpenMap<String, MappingMetaData> createMappingsForIndex() {
// rarely have no types
int typeCount = rarely() ? 0 : scaledRandomIntBetween(1, 3);
return createMappingsForIndex(typeCount, true);
}

public static ImmutableOpenMap<String, MappingMetaData> createMappingsForIndex(int typeCount, boolean randomTypeName) {
List<MappingMetaData> typeMappings = new ArrayList<>(typeCount);

Expand Down Expand Up @@ -122,22 +119,18 @@ public static ImmutableOpenMap<String, MappingMetaData> createMappingsForIndex(i

@Override
protected GetMappingsResponse createTestInstance() {
return createTestInstance(true);
}

private GetMappingsResponse createTestInstance(boolean randomTypeNames) {
ImmutableOpenMap.Builder<String, ImmutableOpenMap<String, MappingMetaData>> 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<String, Object> randomFieldMapping() {
Map<String, Object> mappings = new HashMap<>();
Expand Down Expand Up @@ -170,4 +163,57 @@ private static Map<String, Object> randomFieldMapping() {
}
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<String, Object> parts = parser.map();

ImmutableOpenMap.Builder<String, ImmutableOpenMap<String, MappingMetaData>> builder = new ImmutableOpenMap.Builder<>();
for (Map.Entry<String, Object> 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<String, Object> mapping = (Map<String, Object>) ((Map) entry.getValue()).get("mappings");

ImmutableOpenMap.Builder<String, MappingMetaData> typeBuilder = new ImmutableOpenMap.Builder<>();
for (Map.Entry<String, Object> 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<String, Object> fieldMappings = (Map<String, Object>) typeEntry.getValue();
MappingMetaData mmd = new MappingMetaData(typeName, fieldMappings);
typeBuilder.put(typeName, mmd);
}
builder.put(indexName, typeBuilder.build());
}

return new GetMappingsResponse(builder.build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
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;
Expand All @@ -37,6 +38,11 @@

public class RestGetMappingActionTests extends RestActionTestCase {

@Before
public void setUpAction() {
new RestGetMappingAction(Settings.EMPTY, controller());
}

public void testTypeExistsDeprecation() throws Exception {
Map<String, String> params = new HashMap<>();
params.put("type", "_doc");
Expand Down Expand Up @@ -69,4 +75,24 @@ public void testTypeInPath() {
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<String, String> 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);
}

}