Allow indices.get_mapping response parsing without types #37492

cbuescher · 2019-01-15T17:09:23Z

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.

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.

elasticmachine · 2019-01-15T17:09:24Z

Pinging @elastic/es-search

jtibshirani

I left a few small comments, but this is looking good overall!

jtibshirani · 2019-01-15T20:47:23Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/11_basic_with_types.yml

@@ -13,20 +13,7 @@ setup:
              mappings:
                doc: {}
 ---
-"Get /{index}/_mapping with empty mappings":


I'm wondering why these tests were removed, plus the ones in 61_empty_with_types?

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.

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, and include_type_name=true, as these are two separate codepaths. Just removing the skip section from 60_empty won't test the include_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 set include_type_name=false.

Thanks for explaining, that makes sense to me now, will revert "61_empty_with_types"

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java

server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingActionTests.java

jtibshirani · 2019-01-15T20:52:38Z

server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingActionTests.java

+            .build();
+
+        RestGetMappingAction handler = new RestGetMappingAction(Settings.EMPTY, mock(RestController.class));
+        handler.prepareRequest(request, mock(NodeClient.class));


I think we could just do controller().dispatchRequest here, as we do in the test above.

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 org.elasticsearch.rest.action.admin.indices.RestGetMappingAction.lambda$0(RestGetMappingAction.java:105 because the client isn't fully mocked out. I'd rather keep it this way, it tests what it is supposed to test.

...c/test/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponseTests.java

jtibshirani · 2019-01-15T21:35:04Z

...er/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java

+            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);


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

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.

cbuescher · 2019-01-16T12:11:16Z

@elasticmachine run the gradle build tests 1
@elasticmachine run the gradle build tests 2

cbuescher · 2019-01-16T13:46:15Z

@elasticmachine run the gradle build tests 1

markharwood · 2019-01-16T15:00:30Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetMappingAction.java

@@ -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.";


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.

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.

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

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?

cbuescher · 2019-01-16T15:22:56Z

@elasticmachine run the gradle build tests 1

cbuescher · 2019-01-16T16:11:31Z

@elasticmachine run the gradle build tests 1

cbuescher · 2019-01-17T09:04:00Z

@elasticmachine run the gradle build tests 1
@elasticmachine run the gradle build tests 2

cbuescher · 2019-01-17T14:28:21Z

@elasticmachine run the gradle build tests 2

cbuescher · 2019-01-17T16:04:08Z

@elasticmachine run the gradle build tests 2

cbuescher · 2019-01-17T17:01:04Z

@elasticmachine run the gradle build tests 2

cbuescher · 2019-01-17T17:44:38Z

@elasticmachine run the gradle build tests 2

cbuescher · 2019-01-17T22:47:12Z

@elasticmachine run the gradle build tests 2

cbuescher added >feature :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 >refactoring labels Jan 15, 2019

cbuescher requested review from markharwood and jtibshirani January 15, 2019 17:09

jtibshirani reviewed Jan 15, 2019

View reviewed changes

Christoph Büscher added 2 commits January 16, 2019 10:46

Merge branch 'master' into switch-getMappings-parser

db10c80

iter

d127196

cbuescher force-pushed the switch-getMappings-parser branch from 0f9add6 to d127196 Compare January 16, 2019 11:07

markharwood reviewed Jan 16, 2019

View reviewed changes

jtibshirani approved these changes Jan 16, 2019

View reviewed changes

Remove backticks

a21ef8d

Revert removing 61_empty_with_types.yml

cd718cd

cbuescher merged commit 2f0e0b2 into elastic:master Jan 18, 2019

jtibshirani mentioned this pull request Jan 29, 2019

Support both typed and typeless 'get mapping' requests in the HLRC (backport to 6.x). #37950

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow indices.get_mapping response parsing without types #37492

Allow indices.get_mapping response parsing without types #37492

cbuescher commented Jan 15, 2019

elasticmachine commented Jan 15, 2019

jtibshirani left a comment

jtibshirani Jan 15, 2019

cbuescher Jan 16, 2019

jtibshirani Jan 16, 2019 •

edited

Loading

cbuescher Jan 17, 2019

jtibshirani Jan 15, 2019

cbuescher Jan 16, 2019

jtibshirani Jan 16, 2019

jtibshirani Jan 15, 2019

cbuescher Jan 16, 2019

cbuescher commented Jan 16, 2019

cbuescher commented Jan 16, 2019

markharwood Jan 16, 2019

cbuescher Jan 16, 2019

jtibshirani Jan 16, 2019

jtibshirani Jan 16, 2019

cbuescher commented Jan 16, 2019

cbuescher commented Jan 16, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

Allow indices.get_mapping response parsing without types #37492

Allow indices.get_mapping response parsing without types #37492

Conversation

cbuescher commented Jan 15, 2019

elasticmachine commented Jan 15, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jan 16, 2019

cbuescher commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jan 16, 2019

cbuescher commented Jan 16, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

cbuescher commented Jan 17, 2019

jtibshirani Jan 16, 2019 •

edited

Loading