Skip to content

Commit

Permalink
Small simplifications to mapping validation. (#39777)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jtibshirani committed Mar 8, 2019
1 parent a0a91f7 commit be9c37f
Show file tree
Hide file tree
Showing 6 changed files with 14 additions and 63 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ public void testMultipleJoinFields() throws Exception {
.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."));
}

{
Expand All @@ -426,7 +426,7 @@ public void testMultipleJoinFields() throws Exception {
.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."));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
}
}

Expand All @@ -76,38 +57,11 @@ private static void checkFieldUniqueness(String type,
.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);
}
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -471,8 +471,7 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,11 @@ public void testDuplicateFieldAliasAndObject() {
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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,7 @@ public void testIndexPrefixMapping() throws IOException {

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."));

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,14 @@ public void testReuseMetaField() throws IOException {
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."));
}
}

Expand Down

0 comments on commit be9c37f

Please sign in to comment.