Skip to content
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

Small simplifications to mapping validation. #39777

Merged
merged 2 commits into from
Mar 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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