Support include_type_name in the field mapping and index template APIs. #37210

jtibshirani · 2019-01-08T06:07:48Z

This PR adds the REST parameter include_type_name to the following APIs that we originally missed: indices.get_field_mapping, indices.get_template, and indices.put_template. To make sure we're continuously able to run backwards compatibility tests, the parameter is temporarily defaulted to true, but will eventually default to false on 7.0. The detailed steps I plan to take:

Add include_type_name to 7.0, defaulting to true (this PR).
Backport this PR to 6.x, keeping the default true. Make sure to add notes about include_type_name to the REST documentation.
On 7.0, update the default to false and fix up REST tests.

Note that this PR does not add deprecation warnings, or make any changes to the HLRC.

elasticmachine · 2019-01-08T06:07:49Z

Pinging @elastic/es-search

cbuescher

@jtibshirani thanks, I did a first round of reviews, mostly about tests I think that would be good to be added.
One general question: there is no deprecation logging in the various RestHandlers where the "include_type_name" parameter is added now. Is the plan to add those in a later PR or don't we want to add that at all (like we did for other case). From the overview issue (#35190) I was under the impression we want to warn if the parameter is set to indicate that users should remove it and accept the default. Is this missing here because you are not using the intended default or "false" on 7.0 yet?

cbuescher · 2019-01-09T21:48:27Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

-                    builder.startObject(fieldEntry.getKey());
-                    fieldEntry.getValue().toXContent(builder, params);
+
+            if (includeTypeName == false) {


Can you add a test for the new XContent output case (I think thats includeTypeName=false) to GetFieldMappingsResponseTests please?

I was hoping to do this when we update GetFieldMappingsResponse#fromXContent (in a future PR targeting the HLRC), since this test class is based on AbstractStreamableXContentTestCase which relies on both those methods for xContent testing. For now the toXContent logic is covered by the indices.get_field_mapping REST tests. Does that seem reasonable to you?

Sure, covering xContent output in REST tests is fine for now, I'd just like to make sure we have both a roundtrip test (xContent/parsing) for the matching methods plus another unit test for the deprecated output also as a unit test later on. I think having unit tests for even this kind of simple things is important, REST tests are harder to run, maintain and debug which is why for easily testable things like xContent output I'd like to have unit tests in place. But sure this can be done in a follow up.

👍 sounds good, I also much prefer unit tests where possible.

cbuescher · 2019-01-09T21:49:28Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

+                if (mappings != null) {
+                    addFieldMappingsToBuilder(builder, params, mappings);
+                }
+            } else {


Which output is currently compatible with the parsing code in this class? I assume the "old" response output since I don't see changes to the parser. Will that be adapted once you switch the flags default to "false"?

Right, only the 'old' response format is compatible. And yes, the parsing code will be adapted in a follow-up PR that also updates and tests the Java HLRC.

Okay, good to know.

cbuescher · 2019-01-09T21:50:06Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexTemplateMetaData.java

-                        mapping = (Map<String, Object>) mapping.get(cursor.key);
+                // The parameter include_type_name is only ever used in the REST API, where reduce_mappings is
+                // always set to true. We therefore only check for include_type_name in this branch.
+                if (includeTypeName == false) {


Again, please add at least one test case for the new output to the unit test.

Same thought as above: I was planning to do this when we update GetIndexTemplatesResponse.fromXContent in a future PR. This xContent generation is covered for now by the REST tests in indices.get_template.

cbuescher · 2019-01-09T21:52:04Z

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

+        if (includeTypeName == false && types.length > 0) {
+            throw new IllegalArgumentException("Cannot set include_type_name=false and specify" +
+                " types at the same time.");
+        }


Can this be unit tested as well please? I saw no RestGetFieldMappingActionTest though, maybe we should create one.

👍 will add a test for this.

cbuescher · 2019-01-09T21:55:28Z

...er/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java

+            newSourceAsMap.put("mappings", Collections.singletonMap(MapperService.SINGLE_MAPPING_NAME, sourceAsMap.get("mappings")));
+            sourceAsMap = newSourceAsMap;
+        }
+        putRequest.source(sourceAsMap);


It would be nice if we could unit-test this output as well, wlthough I see this might be a bit tricky with the way this is generated here. Maybe generating the response map can be done in a package private helper that can be tested individually?

I added a unit test for this as well. Just to note, it is also covered by the REST tests under indices.put_template/10_basic.

Thanks for adding a unit test, same comment as above, I think its great this is covered by REST tests but I prefer unit tests if possible.

jtibshirani · 2019-01-09T22:34:10Z

Thanks @cbuescher for the review! It might be helpful to think of this PR as having the same format as #29453, but filling in the APIs it missed. It just aims to add the REST flag include_type_name (but not add deprecation warnings or touch the Java HLRC classes), to make it available to other teams starting their upgrade. I am quickly doing this first, separately from any deprecations or HLRC work, in order to make the typeless APIs available to other teams as soon as possible. For this reason we don't see some of the usual unit testing around deprecations and xContent parsing.

cbuescher

@jtibshirani thanks for the clarifications on my questions and adding the unit test. LGTM from my end.

…s. (elastic#37210) * Add include_type_name to the get field mappings API. * Add include_type_name to the get index templates API. * Add include_type_name to the put index templates API.

jtibshirani added WIP :Search Foundations/Mapping Index mappings, including merging and defining field types >deprecation v7.0.0 v6.7.0 labels Jan 8, 2019

jtibshirani force-pushed the include-type-name branch 7 times, most recently from f1cc8f1 to 6e1dc33 Compare January 9, 2019 04:51

jtibshirani requested review from cbuescher and markharwood January 9, 2019 05:01

jtibshirani removed the WIP label Jan 9, 2019

jtibshirani force-pushed the include-type-name branch from 6e1dc33 to 3763ecc Compare January 9, 2019 05:43

jtibshirani added 4 commits January 9, 2019 13:38

Make sure the API specification lists include_type_name as a boolean.

598744a

Add include_type_name to the get field mappings API.

e3138c0

Add include_type_name to the get index templates API.

324213c

Add include_type_name to the put index templates API.

8bbda23

jtibshirani force-pushed the include-type-name branch from 3763ecc to 8bbda23 Compare January 9, 2019 21:38

cbuescher requested changes Jan 9, 2019

View reviewed changes

jtibshirani added 2 commits January 9, 2019 15:12

Add a unit test for include_type_name in get field mapping requests.

67a858a

Add a unit test for typeless put index requests.

bb922f1

cbuescher approved these changes Jan 10, 2019

View reviewed changes

jtibshirani merged commit a433c40 into master Jan 10, 2019

jtibshirani deleted the include-type-name branch January 10, 2019 17:24

jtibshirani mentioned this pull request Jan 10, 2019

Support include_type_name in the field mapping and index template APIs (backport to 6.x) #37319

Merged

jtibshirani mentioned this pull request Jan 11, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

markharwood mentioned this pull request Jan 11, 2019

Types removal - PUT and GET IndexTemplates. #35866

Closed

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

romseygeek mentioned this pull request Nov 12, 2019

Remove types from internal GetFieldMappings request/response objects #48815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support include_type_name in the field mapping and index template APIs. #37210

Support include_type_name in the field mapping and index template APIs. #37210

jtibshirani commented Jan 8, 2019 •

edited

Loading

elasticmachine commented Jan 8, 2019

cbuescher left a comment

cbuescher Jan 9, 2019

jtibshirani Jan 9, 2019

cbuescher Jan 10, 2019

jtibshirani Jan 10, 2019

cbuescher Jan 9, 2019

jtibshirani Jan 9, 2019 •

edited

Loading

cbuescher Jan 10, 2019

cbuescher Jan 9, 2019

jtibshirani Jan 9, 2019

cbuescher Jan 10, 2019

cbuescher Jan 9, 2019

jtibshirani Jan 9, 2019

cbuescher Jan 9, 2019

jtibshirani Jan 9, 2019

cbuescher Jan 10, 2019

jtibshirani commented Jan 9, 2019

cbuescher left a comment

Support include_type_name in the field mapping and index template APIs. #37210

Support include_type_name in the field mapping and index template APIs. #37210

Conversation

jtibshirani commented Jan 8, 2019 • edited Loading

elasticmachine commented Jan 8, 2019

cbuescher left a 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

jtibshirani Jan 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani commented Jan 9, 2019

cbuescher left a comment

Choose a reason for hiding this comment

jtibshirani commented Jan 8, 2019 •

edited

Loading

jtibshirani Jan 9, 2019 •

edited

Loading