Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Rest Compatible Api] include_type_name parameter #70966
[Rest Compatible Api] include_type_name parameter #70966
Changes from all commits
ddf6152
51ef24a
1bbc802
9592c74
b6abb3f
4ead400
10dde5e
adead4d
c8f7413
57e63fc
539b421
27c80c0
2ea3520
4c707bb
4dcd63c
39d7633
b384677
0cc4427
64c7e90
8da814d
734e8b8
276fc88
07db905
bc0e5df
0835b80
22c5175
9e1644f
2049d0e
e7298ee
a1ea94c
0426b02
5f1b950
95bba64
176da8d
2199c3e
8c57e85
53f7556
a571491
6a5708a
75edcd3
b00c1cb
5f2620b
063d043
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Rather than doing a check here I think we should just wrap it with
_doc
and pass it on. If they haven't includedinclude_type_name
then we're in the typeless world and they shouldn't be including types in their mappings.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 basically tried to retrofit the change from 7.x branch.
Could a user set
include_type_name
to false, in this case they are using a compatible only feature -include_type_name
field but are not using types in a mappingThere 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.
And I think we should remove this check as well - really to check properly we need to parse the mappings, and if they've included a type this will get caught further along.
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.
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.
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.
Assuming I am reading the code correctly, this check is only applied when REST API compatibility for v7 is applied (i.e. CreateIndexRequest.MAPPINGS.forRestApiVersion(RestApiVersion.equalTo(RestApiVersion.V_7)), but the check is now no longer for v8+.
I have a minor preference to keep the check when compatibility is in use, else remove the check in a follow up PR if it also requires changes to the test.
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 see, in that case I will keep the check for both 7.x and 8.x fields.
@romseygeek any views on this?
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.
++ let's keep it for now and I can look at removing it in a followup
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.
@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
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 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.