honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Small simplifications to mapping validation. (#39777) 

These simplifications to `MapperMergeValidator` are possible now that there is
always a single mapping definition.

* Remove the type argument in `validateMapperStructure`.
* Remove unnecessary checks against existing mappers.
This commit is contained in:
Julie Tibshirani 2019-03-07 14:11:27 -08:00
parent a0a91f74ff
commit be9c37fc76
6 changed files with 14 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java
View File

@ -400,7 +400,7 @@ public class ParentJoinFieldMapperTests extends ESSingleNodeTestCase {
.endObject());
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> indexService.mapperService().merge("type",
new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE));
assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice in [type]"));
assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice."));
}
{
@ -426,7 +426,7 @@ public class ParentJoinFieldMapperTests extends ESSingleNodeTestCase {
.endObject());
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> indexService.mapperService().merge("type",
new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE));
assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice in [type]"));
assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice."));
}
}

56
server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java
View File

@ -37,37 +37,18 @@ class MapperMergeValidator {
* duplicate fields are present, and if the provided fields have already been
* defined with a different data type.
*
* @param type The mapping type, for use in error messages.
* @param objectMappers The newly added object mappers.
* @param fieldMappers The newly added field mappers.
* @param fieldAliasMappers The newly added field alias mappers.
* @param fullPathObjectMappers All object mappers, indexed by their full path.
* @param fieldTypes All field and field alias mappers, collected into a lookup structure.
*/
public static void validateMapperStructure(String type,
Collection<ObjectMapper> objectMappers,
public static void validateMapperStructure(Collection<ObjectMapper> objectMappers,
Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers,
Map<String, ObjectMapper> fullPathObjectMappers,
FieldTypeLookup fieldTypes) {
checkFieldUniqueness(type, objectMappers, fieldMappers,
fieldAliasMappers, fullPathObjectMappers, fieldTypes);
checkObjectsCompatibility(objectMappers, fullPathObjectMappers);
}
private static void checkFieldUniqueness(String type,
Collection<ObjectMapper> objectMappers,
Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers,
Map<String, ObjectMapper> fullPathObjectMappers,
FieldTypeLookup fieldTypes) {
// first check within mapping
Collection<FieldAliasMapper> fieldAliasMappers) {
Set<String> objectFullNames = new HashSet<>();
for (ObjectMapper objectMapper : objectMappers) {
String fullPath = objectMapper.fullPath();
if (objectFullNames.add(fullPath) == false) {
throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]");
throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice.");
}
}
@ -76,38 +57,11 @@ class MapperMergeValidator {
.forEach(mapper -> {
String name = mapper.name();
if (objectFullNames.contains(name)) {
throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]");
throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field.");
} else if (fieldNames.add(name) == false) {
throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]");
throw new IllegalArgumentException("Field [" + name + "] is defined twice.");
}
});
// then check other types
for (String fieldName : fieldNames) {
if (fullPathObjectMappers.containsKey(fieldName)) {
throw new IllegalArgumentException("[" + fieldName + "] is defined as a field in mapping [" + type
+ "] but this name is already used for an object in other types");
}
}
for (String objectPath : objectFullNames) {
if (fieldTypes.get(objectPath) != null) {
throw new IllegalArgumentException("[" + objectPath + "] is defined as an object in mapping [" + type
+ "] but this name is already used for a field in other types");
}
}
}
private static void checkObjectsCompatibility(Collection<ObjectMapper> objectMappers,
Map<String, ObjectMapper> fullPathObjectMappers) {
for (ObjectMapper newObjectMapper : objectMappers) {
ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath());
if (existingObjectMapper != null) {
// simulate a merge and ignore the result, we are just interested
// in exceptions here
existingObjectMapper.merge(newObjectMapper);
}
}
}
/**

3
server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View File

@ -471,8 +471,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
Collections.addAll(fieldMappers, metadataMappers);
MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers);
MapperMergeValidator.validateMapperStructure(newMapper.type(), objectMappers, fieldMappers,
fieldAliasMappers, fullPathObjectMappers, fieldTypes);
MapperMergeValidator.validateMapperStructure(objectMappers, fieldMappers, fieldAliasMappers);
checkPartitionedIndexConstraints(newMapper);
// update lookup data-structures

8
server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java
View File

@ -36,13 +36,11 @@ public class MapperMergeValidatorTests extends ESTestCase {
FieldAliasMapper aliasMapper = new FieldAliasMapper("path", "some.path", "field");
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
MapperMergeValidator.validateMapperStructure("type",
MapperMergeValidator.validateMapperStructure(
singletonList(objectMapper),
emptyList(),
singletonList(aliasMapper),
emptyMap(),
new FieldTypeLookup()));
assertEquals("Field [some.path] is defined both as an object and a field in [type]", e.getMessage());
singletonList(aliasMapper)));
assertEquals("Field [some.path] is defined both as an object and a field.", e.getMessage());
}
public void testFieldAliasWithNestedScope() {

2
server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java
View File

@ -880,7 +880,7 @@ public class TextFieldMapperTests extends ESSingleNodeTestCase {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
indexService.mapperService().merge("type", new CompressedXContent(illegalMapping), MergeReason.MAPPING_UPDATE));
assertThat(e.getMessage(), containsString("Field [field._index_prefix] is defined twice in [type]"));
assertThat(e.getMessage(), containsString("Field [field._index_prefix] is defined twice."));
}

4
server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java
View File

@ -151,14 +151,14 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MapperService.MergeReason.MAPPING_UPDATE);
fail();
} catch (IllegalArgumentException e) {
assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
assertTrue(e.getMessage().contains("Field [_id] is defined twice."));
}
try {
mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MapperService.MergeReason.MAPPING_UPDATE);
fail();
} catch (IllegalArgumentException e) {
assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
assertTrue(e.getMessage().contains("Field [_id] is defined twice."));
}
}