Skip to content

Commit

Permalink
Address code review feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
jtibshirani committed Jun 25, 2018
1 parent ec76559 commit f34f57d
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ public FieldTypeLookup copyAndAddAll(String type,
return new FieldTypeLookup(fullName, aliases);
}

/**
* Checks that the new field type is valid.
*/
private void validateField(MappedFieldType existingFieldType,
MappedFieldType newFieldType,
CopyOnWriteHashMap<String, String> aliasToConcreteName) {
Expand All @@ -106,6 +109,12 @@ private void validateField(MappedFieldType existingFieldType,
}
}

/**
* Checks that the new field alias is valid.
*
* Note that this method assumes that new concrete fields have already been processed, so that it
* can verify that an alias refers to an existing concrete field.
*/
private void validateAlias(String aliasName,
String path,
CopyOnWriteHashMap<String, String> aliasToConcreteName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,23 @@
import java.util.Set;
import java.util.stream.Stream;

public class MapperMergeValidator {
/**
* A utility class that helps validate certain aspects of a mappings update.
*/
class MapperMergeValidator {

/**
* Validates the overall structure of the mapping addition, including whether
* 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,
Collection<FieldMapper> fieldMappers,
Expand Down Expand Up @@ -97,6 +113,11 @@ private static void checkObjectsCompatibility(Collection<ObjectMapper> objectMap
/**
* Verifies that each field reference, e.g. the value of copy_to or the target
* of a field alias, corresponds to a valid part of the mapping.
*
* @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 validateFieldReferences(List<FieldMapper> fieldMappers,
List<FieldAliasMapper> fieldAliasMappers,
Expand Down Expand Up @@ -142,9 +163,18 @@ private static void validateFieldAliasTargets(List<FieldAliasMapper> fieldAliasM

String aliasScope = getNestedScope(aliasName, fullPathObjectMappers);
String pathScope = getNestedScope(path, fullPathObjectMappers);

if (!Objects.equals(aliasScope, pathScope)) {
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" +
aliasName + "]: an alias must have the same nested scope as its target.");
StringBuilder message = new StringBuilder("Invalid [path] value [" + path + "] for field alias [" +
aliasName + "]: an alias must have the same nested scope as its target. ");
message.append(aliasScope == null
? "The alias is not nested"
: "The alias's nested scope is [" + aliasScope + "]");
message.append(", but ");
message.append(pathScope == null
? "the target is not nested."
: "the target's nested scope is [" + pathScope + "].");
throw new IllegalArgumentException(message.toString());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,11 @@ public void testFieldAliasWithNestedTarget() {
singletonList(aliasMapper),
Collections.singletonMap("nested", objectMapper),
new FieldTypeLookup()));
assertEquals("Invalid [path] value [nested.field] for field alias [alias]: " +
"an alias must have the same nested scope as its target.", e.getMessage());

String expectedMessage = "Invalid [path] value [nested.field] for field alias [alias]: " +
"an alias must have the same nested scope as its target. The alias is not nested, " +
"but the target's nested scope is [nested].";
assertEquals(expectedMessage, e.getMessage());
}

public void testFieldAliasWithDifferentNestedScopes() {
Expand All @@ -93,8 +96,12 @@ public void testFieldAliasWithDifferentNestedScopes() {
singletonList(aliasMapper),
fullPathObjectMappers,
new FieldTypeLookup()));
assertEquals("Invalid [path] value [nested1.field] for field alias [nested2.alias]: " +
"an alias must have the same nested scope as its target.", e.getMessage());


String expectedMessage = "Invalid [path] value [nested1.field] for field alias [nested2.alias]: " +
"an alias must have the same nested scope as its target. The alias's nested scope is [nested2], " +
"but the target's nested scope is [nested1].";
assertEquals(expectedMessage, e.getMessage());
}

private static ObjectMapper createObjectMapper(String name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,7 @@ public void testFieldAliasWithMismatchedNestedScope() throws Throwable {

IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> mapperService.merge("type", mappingUpdate, MergeReason.MAPPING_UPDATE));
assertEquals("Invalid [path] value [nested.field] for field alias [alias]: an alias" +
" must have the same nested scope as its target.", e.getMessage());
assertThat(e.getMessage(), containsString("Invalid [path] value [nested.field] for field alias [alias]"));
}

public void testForbidMultipleTypes() throws IOException {
Expand Down

0 comments on commit f34f57d

Please sign in to comment.