Skip to content

Commit c94ebef

Browse files
jtibshiranijtibshirani
authored
Move validation from FieldTypeLookup to MapperMergeValidator. (#39814)
This commit consolidates more mapping validation logic into the same class. `FieldTypeLookup` is now a bit simpler, and has the sole responsibility of quickly resolving field names to their types. I have a broader refactor planned around mapping merge validation, but this change should at least be a step in the right direction.
1 parent 347e8cf commit c94ebef

File tree

5 files changed

+189
-179
lines changed

5 files changed

+189
-179
lines changed

‎server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java‎

Lines changed: 0 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,9 @@
2222
import org.elasticsearch.common.collect.CopyOnWriteHashMap;
2323
import org.elasticsearch.common.regex.Regex;
2424

25-
import java.util.ArrayList;
2625
import java.util.Collection;
2726
import java.util.HashSet;
2827
import java.util.Iterator;
29-
import java.util.List;
3028
import java.util.Objects;
3129
import java.util.Set;
3230

@@ -71,74 +69,19 @@ public FieldTypeLookup copyAndAddAll(String type,
7169
MappedFieldType fullNameFieldType = fullName.get(fieldType.name());
7270

7371
if (!Objects.equals(fieldType, fullNameFieldType)) {
74-
validateField(fullNameFieldType, fieldType, aliases);
7572
fullName = fullName.copyAndPut(fieldType.name(), fieldType);
7673
}
7774
}
7875

7976
for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) {
8077
String aliasName = fieldAliasMapper.name();
8178
String path = fieldAliasMapper.path();
82-
83-
validateAlias(aliasName, path, aliases, fullName);
8479
aliases = aliases.copyAndPut(aliasName, path);
8580
}
8681

8782
return new FieldTypeLookup(fullName, aliases);
8883
}
8984

90-
/**
91-
* Checks that the new field type is valid.
92-
*/
93-
private void validateField(MappedFieldType existingFieldType,
94-
MappedFieldType newFieldType,
95-
CopyOnWriteHashMap<String, String> aliasToConcreteName) {
96-
String fieldName = newFieldType.name();
97-
if (aliasToConcreteName.containsKey(fieldName)) {
98-
throw new IllegalArgumentException("The name for field [" + fieldName + "] has already" +
99-
" been used to define a field alias.");
100-
}
101-
102-
if (existingFieldType != null) {
103-
List<String> conflicts = new ArrayList<>();
104-
existingFieldType.checkCompatibility(newFieldType, conflicts);
105-
if (conflicts.isEmpty() == false) {
106-
throw new IllegalArgumentException("Mapper for [" + fieldName +
107-
"] conflicts with existing mapping:\n" + conflicts.toString());
108-
}
109-
}
110-
}
111-
112-
/**
113-
* Checks that the new field alias is valid.
114-
*
115-
* Note that this method assumes that new concrete fields have already been processed, so that it
116-
* can verify that an alias refers to an existing concrete field.
117-
*/
118-
private void validateAlias(String aliasName,
119-
String path,
120-
CopyOnWriteHashMap<String, String> aliasToConcreteName,
121-
CopyOnWriteHashMap<String, MappedFieldType> fullNameToFieldType) {
122-
if (fullNameToFieldType.containsKey(aliasName)) {
123-
throw new IllegalArgumentException("The name for field alias [" + aliasName + "] has already" +
124-
" been used to define a concrete field.");
125-
}
126-
127-
if (path.equals(aliasName)) {
128-
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" +
129-
aliasName + "]: an alias cannot refer to itself.");
130-
}
131-
132-
if (aliasToConcreteName.containsKey(path)) {
133-
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" +
134-
aliasName + "]: an alias cannot refer to another alias.");
135-
}
136-
137-
if (!fullNameToFieldType.containsKey(path)) {
138-
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" +
139-
aliasName + "]: an alias must refer to an existing field in the mappings.");
140-
}
141-
}
14285

14386
/** Returns the field for the given field */
14487
public MappedFieldType get(String field) {

‎server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java‎

Lines changed: 75 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,31 +19,32 @@
1919

2020
package org.elasticsearch.index.mapper;
2121

22+
import java.util.ArrayList;
2223
import java.util.Collection;
2324
import java.util.HashSet;
2425
import java.util.List;
2526
import java.util.Map;
2627
import java.util.Objects;
2728
import java.util.Set;
28-
import java.util.stream.Stream;
2929

3030
/**
3131
* A utility class that helps validate certain aspects of a mappings update.
3232
*/
3333
class MapperMergeValidator {
3434

3535
/**
36-
* Validates the overall structure of the mapping addition, including whether
37-
* duplicate fields are present, and if the provided fields have already been
38-
* defined with a different data type.
36+
* Validates the new mapping addition, checking whether duplicate entries are present and if the
37+
* provided fields are compatible with the mappings that are already defined.
3938
*
4039
* @param objectMappers The newly added object mappers.
4140
* @param fieldMappers The newly added field mappers.
4241
* @param fieldAliasMappers The newly added field alias mappers.
42+
* @param fieldTypes Any existing field and field alias mappers, collected into a lookup structure.
4343
*/
44-
public static void validateMapperStructure(Collection<ObjectMapper> objectMappers,
45-
Collection<FieldMapper> fieldMappers,
46-
Collection<FieldAliasMapper> fieldAliasMappers) {
44+
public static void validateNewMappers(Collection<ObjectMapper> objectMappers,
45+
Collection<FieldMapper> fieldMappers,
46+
Collection<FieldAliasMapper> fieldAliasMappers,
47+
FieldTypeLookup fieldTypes) {
4748
Set<String> objectFullNames = new HashSet<>();
4849
for (ObjectMapper objectMapper : objectMappers) {
4950
String fullPath = objectMapper.fullPath();
@@ -53,17 +54,75 @@ public static void validateMapperStructure(Collection<ObjectMapper> objectMapper
5354
}
5455

5556
Set<String> fieldNames = new HashSet<>();
56-
Stream.concat(fieldMappers.stream(), fieldAliasMappers.stream())
57-
.forEach(mapper -> {
58-
String name = mapper.name();
59-
if (objectFullNames.contains(name)) {
60-
throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field.");
61-
} else if (fieldNames.add(name) == false) {
62-
throw new IllegalArgumentException("Field [" + name + "] is defined twice.");
63-
}
64-
});
57+
for (FieldMapper fieldMapper : fieldMappers) {
58+
String name = fieldMapper.name();
59+
if (objectFullNames.contains(name)) {
60+
throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field.");
61+
} else if (fieldNames.add(name) == false) {
62+
throw new IllegalArgumentException("Field [" + name + "] is defined twice.");
63+
}
64+
65+
validateFieldMapper(fieldMapper, fieldTypes);
66+
}
67+
68+
Set<String> fieldAliasNames = new HashSet<>();
69+
for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) {
70+
String name = fieldAliasMapper.name();
71+
if (objectFullNames.contains(name)) {
72+
throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field.");
73+
} else if (fieldNames.contains(name)) {
74+
throw new IllegalArgumentException("Field [" + name + "] is defined both as an alias and a concrete field.");
75+
} else if (fieldAliasNames.add(name) == false) {
76+
throw new IllegalArgumentException("Field [" + name + "] is defined twice.");
77+
}
78+
79+
validateFieldAliasMapper(name, fieldAliasMapper.path(), fieldNames, fieldAliasNames);
80+
}
6581
}
6682

83+
/**
84+
* Checks that the new field mapper does not conflict with existing mappings.
85+
*/
86+
private static void validateFieldMapper(FieldMapper fieldMapper,
87+
FieldTypeLookup fieldTypes) {
88+
MappedFieldType newFieldType = fieldMapper.fieldType();
89+
MappedFieldType existingFieldType = fieldTypes.get(newFieldType.name());
90+
91+
if (existingFieldType != null && Objects.equals(newFieldType, existingFieldType) == false) {
92+
List<String> conflicts = new ArrayList<>();
93+
existingFieldType.checkCompatibility(newFieldType, conflicts);
94+
if (conflicts.isEmpty() == false) {
95+
throw new IllegalArgumentException("Mapper for [" + newFieldType.name() +
96+
"] conflicts with existing mapping:\n" + conflicts.toString());
97+
}
98+
}
99+
}
100+
101+
/**
102+
* Checks that the new field alias is valid.
103+
*
104+
* Note that this method assumes that new concrete fields have already been processed, so that it
105+
* can verify that an alias refers to an existing concrete field.
106+
*/
107+
private static void validateFieldAliasMapper(String aliasName,
108+
String path,
109+
Set<String> fieldMappers,
110+
Set<String> fieldAliasMappers) {
111+
if (path.equals(aliasName)) {
112+
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" +
113+
aliasName + "]: an alias cannot refer to itself.");
114+
}
115+
116+
if (fieldAliasMappers.contains(path)) {
117+
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" +
118+
aliasName + "]: an alias cannot refer to another alias.");
119+
}
120+
121+
if (fieldMappers.contains(path) == false) {
122+
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" +
123+
aliasName + "]: an alias must refer to an existing field in the mappings.");
124+
}
125+
}
67126
/**
68127
* Verifies that each field reference, e.g. the value of copy_to or the target
69128
* of a field alias, corresponds to a valid part of the mapping.

‎server/src/main/java/org/elasticsearch/index/mapper/MapperService.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -471,11 +471,10 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
471471
Collections.addAll(fieldMappers, metadataMappers);
472472
MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers);
473473

474-
MapperMergeValidator.validateMapperStructure(objectMappers, fieldMappers, fieldAliasMappers);
474+
MapperMergeValidator.validateNewMappers(objectMappers, fieldMappers, fieldAliasMappers, fieldTypes);
475475
checkPartitionedIndexConstraints(newMapper);
476476

477477
// update lookup data-structures
478-
// this will in particular make sure that the merged fields are compatible with other types
479478
fieldTypes = fieldTypes.copyAndAddAll(newMapper.type(), fieldMappers, fieldAliasMappers);
480479

481480
for (ObjectMapper objectMapper : objectMappers) {

‎server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java‎

Lines changed: 0 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -79,46 +79,6 @@ public void testAddExistingField() {
7979
assertEquals(f2.fieldType(), lookup2.get("foo"));
8080
}
8181

82-
public void testMismatchedFieldTypes() {
83-
FieldMapper f1 = new MockFieldMapper("foo");
84-
FieldTypeLookup lookup = new FieldTypeLookup();
85-
lookup = lookup.copyAndAddAll("type", newList(f1), emptyList());
86-
87-
OtherFakeFieldType ft2 = new OtherFakeFieldType();
88-
ft2.setName("foo");
89-
FieldMapper f2 = new MockFieldMapper("foo", ft2);
90-
try {
91-
lookup.copyAndAddAll("type2", newList(f2), emptyList());
92-
fail("expected type mismatch");
93-
} catch (IllegalArgumentException e) {
94-
assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]"));
95-
}
96-
}
97-
98-
public void testConflictingFieldTypes() {
99-
FieldMapper f1 = new MockFieldMapper("foo");
100-
FieldTypeLookup lookup = new FieldTypeLookup();
101-
lookup = lookup.copyAndAddAll("type", newList(f1), emptyList());
102-
103-
MappedFieldType ft2 = new MockFieldMapper.FakeFieldType();
104-
ft2.setName("foo");
105-
ft2.setBoost(2.0f);
106-
FieldMapper f2 = new MockFieldMapper("foo", ft2);
107-
lookup.copyAndAddAll("type", newList(f2), emptyList()); // boost is updateable, so ok since we are implicitly updating all types
108-
lookup.copyAndAddAll("type2", newList(f2), emptyList()); // boost is updateable, so ok if forcing
109-
// now with a non changeable setting
110-
MappedFieldType ft3 = new MockFieldMapper.FakeFieldType();
111-
ft3.setName("foo");
112-
ft3.setStored(true);
113-
FieldMapper f3 = new MockFieldMapper("foo", ft3);
114-
try {
115-
lookup.copyAndAddAll("type2", newList(f3), emptyList());
116-
fail("expected conflict");
117-
} catch (IllegalArgumentException e) {
118-
assertTrue(e.getMessage().contains("has different [store] values"));
119-
}
120-
}
121-
12282
public void testAddFieldAlias() {
12383
MockFieldMapper field = new MockFieldMapper("foo");
12484
FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo");
@@ -181,68 +141,6 @@ public void testUpdateConcreteFieldWithAlias() {
181141
assertEquals(fieldType2, aliasType2);
182142
}
183143

184-
public void testAliasThatRefersToAlias() {
185-
MockFieldMapper field = new MockFieldMapper("foo");
186-
FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo");
187-
FieldTypeLookup lookup = new FieldTypeLookup()
188-
.copyAndAddAll("type", newList(field), newList(alias));
189-
190-
FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "alias");
191-
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
192-
() -> lookup.copyAndAddAll("type", emptyList(), newList(invalidAlias)));
193-
assertEquals("Invalid [path] value [alias] for field alias [invalid-alias]: an alias" +
194-
" cannot refer to another alias.", e.getMessage());
195-
}
196-
197-
public void testAliasThatRefersToItself() {
198-
FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "invalid-alias");
199-
200-
FieldTypeLookup lookup = new FieldTypeLookup();
201-
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
202-
() -> lookup.copyAndAddAll("type", emptyList(), newList(invalidAlias)));
203-
assertEquals("Invalid [path] value [invalid-alias] for field alias [invalid-alias]: an alias" +
204-
" cannot refer to itself.", e.getMessage());
205-
}
206-
207-
public void testAliasWithNonExistentPath() {
208-
FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "non-existent");
209-
210-
FieldTypeLookup lookup = new FieldTypeLookup();
211-
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
212-
() -> lookup.copyAndAddAll("type", emptyList(), newList(invalidAlias)));
213-
assertEquals("Invalid [path] value [non-existent] for field alias [invalid-alias]: an alias" +
214-
" must refer to an existing field in the mappings.", e.getMessage());
215-
}
216-
217-
public void testAddAliasWithPreexistingField() {
218-
MockFieldMapper field = new MockFieldMapper("field");
219-
FieldTypeLookup lookup = new FieldTypeLookup()
220-
.copyAndAddAll("type", newList(field), emptyList());
221-
222-
MockFieldMapper invalidField = new MockFieldMapper("invalid");
223-
FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid", "invalid", "field");
224-
225-
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
226-
() -> lookup.copyAndAddAll("type", newList(invalidField), newList(invalidAlias)));
227-
assertEquals("The name for field alias [invalid] has already been used to define a concrete field.",
228-
e.getMessage());
229-
}
230-
231-
public void testAddFieldWithPreexistingAlias() {
232-
MockFieldMapper field = new MockFieldMapper("field");
233-
FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid", "invalid", "field");
234-
235-
FieldTypeLookup lookup = new FieldTypeLookup()
236-
.copyAndAddAll("type", newList(field), newList(invalidAlias));
237-
238-
MockFieldMapper invalidField = new MockFieldMapper("invalid");
239-
240-
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
241-
() -> lookup.copyAndAddAll("type", newList(invalidField), emptyList()));
242-
assertEquals("The name for field [invalid] has already been used to define a field alias.",
243-
e.getMessage());
244-
}
245-
246144
public void testSimpleMatchToFullName() {
247145
MockFieldMapper field1 = new MockFieldMapper("foo");
248146
MockFieldMapper field2 = new MockFieldMapper("bar");

0 commit comments

Comments
 (0)