[Rest Compatible Api] include_type_name parameter by pgomulka · Pull Request #70966 · elastic/elasticsearch

pgomulka · 2021-03-29T08:28:12Z

This commit allows to use the include_type_name parameter with the compatible rest api.
The support for include_type_name was previously removed in #48632

relates #51816
types removal meta issue #54160

…pe_name

pgomulka · 2021-03-31T14:18:08Z

FYI - you can ignore build/test transformation part. This is handled in a PR that will be merged before this one. I did not wanted to block testing of this PR though so I included it for the time being.

elasticmachine · 2021-03-31T14:25:57Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

…pe_name

rest-api-spec/build.gradle

jtibshirani · 2021-04-14T17:53:29Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/RolloverRequest.java

        PARSER.declareField((parser, request, context) -> {
            // a type is not included, add a dummy _doc type
            Map<String, Object> mappings = parser.map();
-            if (MapperService.isMappingSourceTyped(MapperService.SINGLE_MAPPING_NAME, mappings)) {


Building on what I said in a previous comment -- these were added in #38270 to catch an important case that mapping parsing wasn't able to detect. But there have been substantial improvements/ refactors to mapping parsing since then, so we may be able to remove these checks and have the tests still pass. To keep this PR a straightforward 'restore' of old logic, maybe we could look into removing these checks in a follow-up.

jakelandis · 2021-04-14T21:34:23Z

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

            builder.endObject();

-            includeTypeName &= (params.paramAsBoolean("reduce_mappings", false) == false);
+            if(builder.getRestApiVersion() != RestApiVersion.V_7) {


can this be onOrAfter V_8 ? (minor preference to avoid using 'not a specific version')

jakelandis · 2021-04-14T21:36:10Z

server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java


+    /**
+     * Parameter that controls whether certain REST apis should include type names in their requests or responses.
+     * Note: This parameter is only available through compatible rest api.


might want to reference the V_7 constant in the java doc ... i think the build will fail if you reference something that is not there and will help to find this when it is time to re-remove.

jakelandis · 2021-04-14T21:45:23Z

Changes LGTM, assuming Julie and/or Alan and CI agree.

jakelandis · 2021-04-14T21:46:21Z

The compat test failures should be fixed if you merge in master.

…pe_name

pgomulka · 2021-04-15T12:53:09Z

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

-            includeTypeName &= (params.paramAsBoolean("reduce_mappings", false) == false);
+            if(builder.getRestApiVersion().matches(onOrAfter(V_8))) {
+                includeTypeName &= (params.paramAsBoolean("reduce_mappings", false) == false);
+            }


@romseygeek do you think there need to be a special handing for this in V7 mode?
The logic for 7.x was more complex, but it feels like it was a behaviour change

I think this reduce_mappings parameter is only set internally, so I don't think we want to put a Rest API version guard on it. It's for things like template upgrades or building internal indexes from templates.

romseygeek

LGTM - a couple of minor nits and one suggested change, but I'm happy after that if CI is.

romseygeek · 2021-04-15T13:04:28Z

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

-            includeTypeName &= (params.paramAsBoolean("reduce_mappings", false) == false);
+            if(builder.getRestApiVersion().matches(onOrAfter(V_8))) {
+                includeTypeName &= (params.paramAsBoolean("reduce_mappings", false) == false);
+            }


I think this reduce_mappings parameter is only set internally, so I don't think we want to put a Rest API version guard on it. It's for things like template upgrades or building internal indexes from templates.

romseygeek · 2021-04-15T13:06:32Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/RolloverRequest.java

        PARSER.declareField((parser, request, context) -> {
            // a type is not included, add a dummy _doc type
            Map<String, Object> mappings = parser.map();
-            if (MapperService.isMappingSourceTyped(MapperService.SINGLE_MAPPING_NAME, mappings)) {


++ let's keep it for now and I can look at removing it in a followup

romseygeek · 2021-04-15T13:08:28Z

...main/java/org/elasticsearch/action/admin/indices/template/get/GetIndexTemplatesResponse.java

        for (IndexTemplateMetadata indexTemplateMetadata : getIndexTemplates()) {
-            IndexTemplateMetadata.Builder.toXContent(indexTemplateMetadata, builder, params);
+            if(builder.getRestApiVersion() == RestApiVersion.V_7 &&
+                params.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, DEFAULT_INCLUDE_TYPE_NAME_POLICY)) {


I think I'd prefer an explicit false as the default value here to make it clearer that the normal path is to not output types.

romseygeek · 2021-04-15T13:10:34Z

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

        final String[] fields = Strings.splitStringByCommaToArray(request.param("fields"));

+        if (request.getRestApiVersion() == RestApiVersion.V_7 && request.hasParam(INCLUDE_TYPE_NAME_PARAMETER)) {
+            request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, DEFAULT_INCLUDE_TYPE_NAME_POLICY);


Why do we need to call paramAsBoolean here?

good point, the value is ignored. Will fix to just use param

romseygeek · 2021-04-15T13:12:10Z

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

        return channel -> client.admin().indices().rolloverIndex(rolloverIndexRequest, new RestToXContentListener<>(channel));
    }
+
+    private boolean isIncludeTypeName(RestRequest request) {


suggestion: s/isIncludeTypeName/includeTypeName/

…pe_name

jtibshirani · 2021-04-15T16:29:16Z

I didn't have the chance to go over each file carefully, but it looks good to me overall -- no objections to merging!

…pe_name

…plate Previously the compatibility layer was alwayre returning an _doc for get template. This commit does not return _doc in mappings when mappings are empty. Returning just {} empty object also moving term lookups tests which are already fixed (relates elastic#74544) relates elastic#70966 relates main meta issue elastic#51816 relates types removal meta elastic#54160

…plate (#75448) Previously the compatibility layer was always returning an _doc in mappings for get template. This commit does not return _doc in empty mappings. Returning just {} empty object (v7 and v8 behaviour) also moving term lookups tests which are already fixed (relates #74544) relates #70966 relates main meta issue #51816 relates types removal meta #54160

…plate (elastic#75448) Previously the compatibility layer was always returning an _doc in mappings for get template. This commit does not return _doc in empty mappings. Returning just {} empty object (v7 and v8 behaviour) also moving term lookups tests which are already fixed (relates elastic#74544) relates elastic#70966 relates main meta issue elastic#51816 relates types removal meta elastic#54160

pgomulka added 19 commits March 26, 2021 16:24

transformation working

ddf6152

style

51ef24a

test fix

1bbc802

is true as a parameter

9592c74

create index passing except for get mappings api

b6abb3f

code style

4ead400

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

10dde5e

…pe_name

test fix

adead4d

fix test

c8f7413

style check

57e63fc

add all changes

539b421

disable compat xpack tests

27c80c0

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

2ea3520

…pe_name

test

4c707bb

unit tests

4dcd63c

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

39d7633

…pe_name

enable xpack compat tests

b384677

small cleanups

0cc4427

fix test

64c7e90

pgomulka self-assigned this Mar 31, 2021

pgomulka added the :Core/Infra/REST API REST infrastructure and utilities label Mar 31, 2021

pgomulka requested review from jakelandis, joegallo and romseygeek March 31, 2021 14:18

pgomulka marked this pull request as ready for review March 31, 2021 14:18

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Mar 31, 2021

pgomulka requested a review from jtibshirani March 31, 2021 14:36

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

8da814d

…pe_name

pgomulka added 3 commits April 13, 2021 16:09

remove stale gradle transformations

a1ea94c

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

0426b02

…pe_name

missing version quard check

5f1b950

jtibshirani reviewed Apr 14, 2021

View reviewed changes

jakelandis reviewed Apr 14, 2021

View reviewed changes

pgomulka added 2 commits April 15, 2021 10:49

precommit

95bba64

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

176da8d

…pe_name

pgomulka commented Apr 15, 2021

View reviewed changes

romseygeek approved these changes Apr 15, 2021

View reviewed changes

pgomulka added 3 commits April 15, 2021 15:57

code review follow up

2199c3e

keep the test ignored

8c57e85

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

53f7556

…pe_name

jakelandis approved these changes Apr 15, 2021

View reviewed changes

pgomulka added 6 commits April 19, 2021 09:36

tweak comment

a571491

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

6a5708a

…pe_name

enable some xpack plugin tests

75edcd3

Merge remote-tracking branch 'upstream/master' into compat/include_ty…

b00c1cb

…pe_name

fix xpack-plugin block list

5f2620b

fix build

063d043

pgomulka merged commit 3ef5e4c into elastic:master Apr 19, 2021

pgomulka mentioned this pull request May 10, 2021

Types removal - compatible changes #54160

Closed

66 tasks

pgomulka mentioned this pull request Jul 19, 2021

[Rest Api Compatibility] Do not return _doc for empty mappings in template #75448

Merged

ChrisHegarty unassigned pgomulka Oct 29, 2024

Conversation

pgomulka commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka commented Mar 31, 2021

Uh oh!

elasticmachine commented Mar 31, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis commented Apr 14, 2021

Uh oh!

jakelandis commented Apr 14, 2021

Uh oh!

pgomulka Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani commented Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pgomulka commented Mar 29, 2021 •

edited

Loading

pgomulka Apr 15, 2021 •

edited

Loading