Skip to content

Commit 5d5c659

Browse files
jpountzjpountz
committed
Validate that fields are defined only once.
There are two ways that a field can be defined twice: - by reusing the name of a meta mapper in the root object (`_id`, `_routing`, etc.) - by defining a sub-field both explicitly in the mapping and through the code in a field mapper (like ExternalMapper does) This commit adds new checks in order to make sure this never happens. Close #15057
1 parent 5f7b863 commit 5d5c659

File tree

5 files changed

+98
-12
lines changed

5 files changed

+98
-12
lines changed

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

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import org.elasticsearch.ElasticsearchGenerationException;
3333
import org.elasticsearch.Version;
3434
import org.elasticsearch.common.Nullable;
35-
import org.elasticsearch.common.collect.ImmutableOpenMap;
3635
import org.elasticsearch.common.collect.Tuple;
3736
import org.elasticsearch.common.compress.CompressedXContent;
3837
import org.elasticsearch.common.lucene.search.Queries;
@@ -92,7 +91,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
9291
private final ReleasableLock mappingWriteLock = new ReleasableLock(mappingLock.writeLock());
9392

9493
private volatile FieldTypeLookup fieldTypes;
95-
private volatile ImmutableOpenMap<String, ObjectMapper> fullPathObjectMappers = ImmutableOpenMap.of();
94+
private volatile Map<String, ObjectMapper> fullPathObjectMappers = new HashMap<>();
9695
private boolean hasNested = false; // updated dynamically to true when a nested object is added
9796

9897
private final DocumentMapperParser documentParser;
@@ -300,8 +299,41 @@ private boolean assertSerialization(DocumentMapper mapper) {
300299
return true;
301300
}
302301

302+
private void checkFieldUniqueness(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {
303+
final Set<String> objectFullNames = new HashSet<>();
304+
for (ObjectMapper objectMapper : objectMappers) {
305+
final String fullPath = objectMapper.fullPath();
306+
if (objectFullNames.add(fullPath) == false) {
307+
throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]");
308+
}
309+
}
310+
311+
if (indexSettings.getIndexVersionCreated().before(Version.V_3_0_0)) {
312+
// Before 3.0 some metadata mappers are also registered under the root object mapper
313+
// So we avoid false positives by deduplicating mappers
314+
// given that we check exact equality, this would still catch the case that a mapper
315+
// is defined under the root object
316+
Collection<FieldMapper> uniqueFieldMappers = Collections.newSetFromMap(new IdentityHashMap<>());
317+
uniqueFieldMappers.addAll(fieldMappers);
318+
fieldMappers = uniqueFieldMappers;
319+
}
320+
321+
final Set<String> fieldNames = new HashSet<>();
322+
for (FieldMapper fieldMapper : fieldMappers) {
323+
final String name = fieldMapper.name();
324+
if (objectFullNames.contains(name)) {
325+
throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]");
326+
} else if (fieldNames.add(name) == false) {
327+
throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]");
328+
}
329+
}
330+
}
331+
303332
protected void checkMappersCompatibility(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers, boolean updateAllTypes) {
304333
assert mappingLock.isWriteLockedByCurrentThread();
334+
335+
checkFieldUniqueness(type, objectMappers, fieldMappers);
336+
305337
for (ObjectMapper newObjectMapper : objectMappers) {
306338
ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath());
307339
if (existingObjectMapper != null) {
@@ -313,6 +345,13 @@ protected void checkMappersCompatibility(String type, Collection<ObjectMapper> o
313345
}
314346
}
315347
}
348+
349+
for (FieldMapper fieldMapper : fieldMappers) {
350+
if (fullPathObjectMappers.containsKey(fieldMapper.name())) {
351+
throw new IllegalArgumentException("Field [{}] is defined as a field in mapping [" + fieldMapper.name() + "] but this name is already used for an object in other types");
352+
}
353+
}
354+
316355
fieldTypes.checkCompatibility(type, fieldMappers, updateAllTypes);
317356
}
318357

@@ -330,14 +369,14 @@ protected Tuple<Collection<ObjectMapper>, Collection<FieldMapper>> checkMappersC
330369

331370
protected void addMappers(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {
332371
assert mappingLock.isWriteLockedByCurrentThread();
333-
ImmutableOpenMap.Builder<String, ObjectMapper> fullPathObjectMappers = ImmutableOpenMap.builder(this.fullPathObjectMappers);
372+
Map<String, ObjectMapper> fullPathObjectMappers = new HashMap<>(this.fullPathObjectMappers);
334373
for (ObjectMapper objectMapper : objectMappers) {
335374
fullPathObjectMappers.put(objectMapper.fullPath(), objectMapper);
336375
if (objectMapper.nested().isNested()) {
337376
hasNested = true;
338377
}
339378
}
340-
this.fullPathObjectMappers = fullPathObjectMappers.build();
379+
this.fullPathObjectMappers = Collections.unmodifiableMap(fullPathObjectMappers);
341380
this.fieldTypes = this.fieldTypes.copyAndAddAll(type, fieldMappers);
342381
}
343382

‎core/src/main/java/org/elasticsearch/index/mapper/Mapping.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,12 @@
2727

2828
import java.io.IOException;
2929
import java.util.Arrays;
30+
import java.util.Collections;
3031
import java.util.Comparator;
3132
import java.util.HashMap;
32-
import java.util.List;
33+
import java.util.HashSet;
3334
import java.util.Map;
35+
import java.util.Set;
3436

3537
import static java.util.Collections.emptyMap;
3638
import static java.util.Collections.unmodifiableMap;
@@ -41,7 +43,9 @@
4143
*/
4244
public final class Mapping implements ToXContent {
4345

44-
public static final List<String> LEGACY_INCLUDE_IN_OBJECT = Arrays.asList("_all", "_id", "_parent", "_routing", "_timestamp", "_ttl");
46+
// Set of fields that were included into the root object mapper before 2.0
47+
public static final Set<String> LEGACY_INCLUDE_IN_OBJECT = Collections.unmodifiableSet(new HashSet<>(
48+
Arrays.asList("_all", "_id", "_parent", "_routing", "_timestamp", "_ttl")));
4549

4650
final Version indexCreated;
4751
final RootObjectMapper root;

‎core/src/test/java/org/elasticsearch/index/mapper/externalvalues/ExternalValuesMapperIntegrationIT.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public void testExternalValuesWithMultifield() throws Exception {
8787
.startObject("f")
8888
.field("type", ExternalMapperPlugin.EXTERNAL_UPPER)
8989
.startObject("fields")
90-
.startObject("f")
90+
.startObject("g")
9191
.field("type", "string")
9292
.field("store", "yes")
9393
.startObject("fields")
@@ -107,7 +107,7 @@ public void testExternalValuesWithMultifield() throws Exception {
107107
refresh();
108108

109109
SearchResponse response = client().prepareSearch("test-idx")
110-
.setQuery(QueryBuilders.termQuery("f.f.raw", "FOO BAR"))
110+
.setQuery(QueryBuilders.termQuery("f.g.raw", "FOO BAR"))
111111
.execute().actionGet();
112112

113113
assertThat(response.getHits().totalHits(), equalTo((long) 1));

‎core/src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingTests.java

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,51 @@ public void testConflictNewTypeUpdate() throws Exception {
202202
assertNull(mapperService.documentMapper("type2").mapping().root().getMapper("foo"));
203203
}
204204

205+
public void testReuseMetaField() throws IOException {
206+
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
207+
.startObject("properties").startObject("_id").field("type", "string").endObject()
208+
.endObject().endObject().endObject();
209+
MapperService mapperService = createIndex("test", Settings.settingsBuilder().build()).mapperService();
210+
211+
try {
212+
mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
213+
fail();
214+
} catch (IllegalArgumentException e) {
215+
assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
216+
}
217+
218+
try {
219+
mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
220+
fail();
221+
} catch (IllegalArgumentException e) {
222+
assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
223+
}
224+
}
225+
226+
public void testReuseMetaFieldBackCompat() throws IOException {
227+
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
228+
.startObject("properties").startObject("_id").field("type", "string").endObject()
229+
.endObject().endObject().endObject();
230+
// the logic is different for 2.x indices since they record some meta mappers (including _id)
231+
// in the root object
232+
Settings settings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_2_1_0).build();
233+
MapperService mapperService = createIndex("test", settings).mapperService();
234+
235+
try {
236+
mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
237+
fail();
238+
} catch (IllegalArgumentException e) {
239+
assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
240+
}
241+
242+
try {
243+
mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
244+
fail();
245+
} catch (IllegalArgumentException e) {
246+
assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
247+
}
248+
}
249+
205250
public void testIndexFieldParsingBackcompat() throws IOException {
206251
IndexService indexService = createIndex("test", Settings.settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_1_4_2.id).build());
207252
XContentBuilder indexMapping = XContentFactory.jsonBuilder();

‎modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/SearchFieldsTests.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -392,8 +392,7 @@ public void testStoredFieldsWithoutSource() throws Exception {
392392
createIndex("test");
393393
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForYellowStatus().execute().actionGet();
394394

395-
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("properties")
396-
.startObject("_source").field("enabled", false).endObject()
395+
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("_source").field("enabled", false).endObject().startObject("properties")
397396
.startObject("byte_field").field("type", "byte").field("store", "yes").endObject()
398397
.startObject("short_field").field("type", "short").field("store", "yes").endObject()
399398
.startObject("integer_field").field("type", "integer").field("store", "yes").endObject()
@@ -556,8 +555,7 @@ public void testFieldsPulledFromFieldData() throws Exception {
556555
createIndex("test");
557556
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForYellowStatus().execute().actionGet();
558557

559-
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("properties")
560-
.startObject("_source").field("enabled", false).endObject()
558+
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("_source").field("enabled", false).endObject().startObject("properties")
561559
.startObject("string_field").field("type", "string").endObject()
562560
.startObject("byte_field").field("type", "byte").endObject()
563561
.startObject("short_field").field("type", "short").endObject()

0 commit comments

Comments
 (0)