-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming that we'll revisit the interface to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 @@ | |
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."; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -90,6 +93,10 @@ 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."); | ||
} | ||
// TODO q: do we want to deprecate any use of the parameter or just "true" | ||
if (includeTypeName == true) { | ||
cbuescher marked this conversation as resolved.
Show resolved
Hide resolved
|
||
deprecationLogger.deprecatedAndMaybeLog("get_mapping_with_types", TYPES_DEPRECATION_MESSAGE); | ||
} | ||
|
||
final GetMappingsRequest getMappingsRequest = new GetMappingsRequest(); | ||
getMappingsRequest.indices(indices).types(types); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,4 +69,20 @@ public void testTypeInPath() { | |
assertEquals(1, channel.errors().get()); | ||
assertEquals(RestStatus.BAD_REQUEST, channel.capturedResponse().status()); | ||
} | ||
|
||
public void testTypeUrlParamerterDeprecation() throws Exception { | ||
cbuescher marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Map<String, String> params = new HashMap<>(); | ||
params.put(INCLUDE_TYPE_NAME_PARAMETER, "true"); | ||
RestRequest request = new FakeRestRequest.Builder(xContentRegistry()) | ||
.withMethod(RestRequest.Method.GET) | ||
.withParams(params) | ||
.withPath("some_index/some_type/_mapping/some_field") | ||
.build(); | ||
|
||
RestGetMappingAction handler = new RestGetMappingAction(Settings.EMPTY, mock(RestController.class)); | ||
handler.prepareRequest(request, mock(NodeClient.class)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work without some more involved mocking which I think isn't necessary here. If using the "standard" approach from other tests I get a NPE in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
assertWarnings(RestGetMappingAction.TYPES_DEPRECATION_MESSAGE); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why these tests were removed, plus the ones in
61_empty_with_types
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I think I missread this test as already being covered by the general setup, buts its checking that the "mappings" key exists even if no field mapping is there at all. Will revert.
About 61_empty_with_types: I think the behaviour of the test and the assertions shouldn't differ from 60_empy.yml at all if used with "include_type_name" or not. In fact I think we can remove the "skip" section from "60_empty" which is what I will do and see if it passes the mixed cluster tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean to belabor this too much, but I think the point of the tests was to check that we return the correct response for empty mappings in both the cases where
include_type_name=false
, andinclude_type_name=true
, as these are two separate codepaths. Just removing theskip
section from60_empty
won't test theinclude_type_name=true
case on 7.0.It also seems nice to stick with the guidelines laid out in #37285 (comment), so it's easier to understand what's going on in all the tests. In particular, if the test explicitly tests typeless APIs on 7.0 then we've been omitting
include_type_name
, but for mixed-version tests, we explicitly setinclude_type_name=false
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining, that makes sense to me now, will revert "61_empty_with_types"