Consolidate field name validation when parsing mappings and documents #91328
Conversation
elastic#91043 surfaced some inconsistencies in field names validation between mapping parsing and document parsing. This commit centralizes the validation of field names when parsing mappings to a single place, and attempts to address some of the inconsistencies. - field names that contain only whitespaces are no longer accepted in mappings. It was previously possible to map a field containing only whitespaces but a document containing such a field would be rejected. We start rejecting mappings with fields that contain only whitespaces for indices that are created from 8.6 on, just in case existing indices contain such fields. This is true also for dotted fields like top. .foo when subobjects are enabled. - A clear error message is thrown when mappings hold fields with names made of dots only. An ArrayIndexOutOfBoundsException was thrown before - The error thrown when a field name is empty is now unified with that thrown when an empty field name is provided as part of a document (field name cannot be an empty string) - When parsing documents (with subobjects set to false), distinguish between the error thrown when a field name is empty and that thrown when a field name is made of whitespaces only - When parsing documents (with subobjects set to false), accept field names that are made of dots only (these are already accepted in mappings), effectively reverts elastic#90950
javanna
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @javanna, I've created a changelog YAML for you. |
| @@ -112,6 +112,7 @@ | |||
| import static org.hamcrest.Matchers.equalTo; | |||
| import static org.hamcrest.Matchers.instanceOf; | |||
|
|
|||
| //TODO migrate tests that don't require a node to a unit test that subclasses MapperTestCase | |||
| public class PercolatorFieldMapperTests extends ESSingleNodeTestCase { | |||
There was a problem hiding this comment.
Yeah, I've made so many attempts to migrate this one, but it gets tricky because we need a SearchExecutionContext to parse incoming documents. One day!
There was a problem hiding this comment.
good to know. It looked easy at first sight, at least for the testEmpyName method that I had to manually correct, but I guessed I was missing something. It may be a case of splitting it into two tests, but it was not entirely clear to me how many tests we can move to the unit tests class.
| @@ -207,132 +212,151 @@ to get the wrong path (missing the first portion). | |||
| assertEquals("obj.source.geo.location", geoPointFieldMapper.mappedFieldType.name()); | |||
| } | |||
|
|
|||
| private static String randomFieldType() { | |||
| return randomBoolean() ? KeywordFieldMapper.CONTENT_TYPE : ObjectMapper.CONTENT_TYPE; | |||
There was a problem hiding this comment.
Is it worth checking alias fields in here as well? They are I think the only Mapper type that doesn't subclass either FieldMapper or ObjectMapper.
There was a problem hiding this comment.
I tried it but it won't work easily because field alias need to have path declared. I feel like this is ok, we care to see that the checks are not made for a specific type only, and we still test for e.g. empty name in MapperTestCase.
server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java
Show resolved
Hide resolved
test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java
Show resolved
Hide resolved
|
thanks @romseygeek !!! |
#91043 surfaced some inconsistencies in field names validation between mapping parsing and document parsing. This commit centralizes the validation of field names when parsing mappings to a single place, and attempts to address some of the inconsistencies.
field names that contain only whitespaces are no longer accepted in mappings. It was previously possible to map a field containing only whitespaces but a document containing such a field would be rejected. We start rejecting mappings with fields that contain only whitespaces for indices that are created from 8.6 on, just in case existing indices contain such fields. This is true also for dotted fields like top. .foo when subobjects are enabled.
A clear error message is thrown when mappings hold fields with names made of dots only. An ArrayIndexOutOfBoundsException was thrown before
The error thrown when a field name is empty is now unified with that thrown when an empty field name is provided as part of a document (field name cannot be an empty string)
When parsing documents (with subobjects set to false), distinguish between the error thrown when a field name is empty and that thrown when a field name is made of whitespaces only
When parsing documents (with subobjects set to false), accept field names that are made of dots only (these are already accepted in mappings), effectively reverts Validate field names when subobjects are disabled #90950