Sub-fields should not accept include_in_all parameter
#21971
Conversation
nik9000
added
:Search/Mapping
| @@ -229,6 +229,9 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri | |||
| builder.indexOptions(nodeIndexOptionValue(propNode)); | |||
| iterator.remove(); | |||
| } else if (propName.equals("include_in_all")) { | |||
| if(parserContext.isWithinMultiField()) { | |||
There was a problem hiding this comment.
Nit: usually we put a space between if and (
| @@ -229,6 +229,9 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri | |||
| builder.indexOptions(nodeIndexOptionValue(propNode)); | |||
| iterator.remove(); | |||
| } else if (propName.equals("include_in_all")) { | |||
| if(parserContext.isWithinMultiField()) { | |||
| throw new MapperParsingException("sub-fields shouldn't contain property: include_in_all, and current node is:" + name); | |||
There was a problem hiding this comment.
Have a look at the example below for copy_to about how we usually write these field names. We wrap all the variable bits of exception messages in [] almost everywhere. Right or wrong it is our habit and we should stick to it unless we have a good reason not to.
|
@nik9000 I modified code according to your comments. I'll commit test later. but I have one question related to CopyToMapperTests: it seems in this test, this exception: is not test? |
You mean that test doesn't exist? I suspect we just forgot to make it. If you'd like to make it while you are there that'd be lovely. |
|
@nik9000 it turns out that it is tested but in another test case file:MultiFieldCopyToMapperTests. so I add this new test case file:MultiFieldIncludeInAllMapperTests |
clintongormley
changed the title
include_in_all parameter
|
@nik9000 can you help to review this? |
| import static org.hamcrest.core.IsEqual.equalTo; | ||
|
|
||
| /** | ||
| * Created by makeyang on 2016/12/8. |
There was a problem hiding this comment.
Can you replace this with something descriptive of what it does or remove it?
| MapperService mapperService = MapperTestUtils.newMapperService(createTempDir(), Settings.EMPTY); | ||
| try { | ||
| mapperService.parse("type", new CompressedXContent(mapping.string()), true); | ||
| fail("Parsing should throw an exception because the mapping contains a include_in_all in a multi field"); |
There was a problem hiding this comment.
We tend to use expectThrows for this when we write new tests. It isn't required but it is a bit cleaner to read.
2. remove Created by declaration 3. fix typo method name from testExceptionForCopyToInMultiFields to testExceptionForIncludeInAllInMultiFields 4. fix typo method name from createMappinmgWithIncludeInAllInMultiField to createMappingWithIncludeInAllInMultiField 5. use expectThrows rather than try catch according to nik9000's comments
|
@nik9000 can you help to review this please? |
|
@nik9000 can you help to review this or ask someone else to help to review it? thanks very much. |
|
Looks right to me! elasticmachine, please test this. |
Fail to update mapping when multifield has `include_in_all`. Closes #21710
|
@nik9000 thanks very much. |
|
Isn't this a breaking change? This broke our mappings while upgrading from 5.1.x to 5.2.x |
|
@mfeltscher thanks for reporting and yes, this shouldn't have gone into 5.2. i've opened #23654 |
This reverts elastic#21971 which should only have been applied in master to preserve backwards compatibility. Instead of throwing an error when you specify `include_in_all` inside a multifield we instead return a deprecation warning. `include_in_all` in a multifield still doesn't do anything. But at least people who use it erroneously won't break. Closes elastic#23654
This reverts #21971 which should only have been applied in master to preserve backwards compatibility. Instead of throwing an error when you specify `include_in_all` inside a multifield we instead return a deprecation warning. `include_in_all` in a multifield still doesn't do anything. But at least people who use it erroneously won't break. Closes #23654
This reverts #21971 which should only have been applied in master to preserve backwards compatibility. Instead of throwing an error when you specify `include_in_all` inside a multifield we instead return a deprecation warning. `include_in_all` in a multifield still doesn't do anything. But at least people who use it erroneously won't break. Closes #23654
This reverts #21971 which should only have been applied in master to preserve backwards compatibility. Instead of throwing an error when you specify `include_in_all` inside a multifield we instead return a deprecation warning. `include_in_all` in a multifield still doesn't do anything. But at least people who use it erroneously won't break. Closes #23654
Throw an exception when specifying
include_in_allon multi-fieldsCloses #21710