Skip to content

Commit

Permalink
Disallow duplicate dynamic_templates definitions
Browse files Browse the repository at this point in the history
Currently the same dynamic_templates can be defined more than once by error in
the same mappings file. This most certainly is unintentional and can create
issues like those reported in elastic#28988. This change checks for duplicates while
parsing and throws an error if we encounter the same definition twice.

Closes elastic#28988
  • Loading branch information
Christoph Büscher committed Feb 7, 2020
1 parent c21837f commit e65f286
Show file tree
Hide file tree
Showing 5 changed files with 204 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.TreeMap;

public class DynamicTemplate implements ToXContentObject {
Expand Down Expand Up @@ -233,7 +234,7 @@ public static DynamicTemplate parse(String name, Map<String, Object> conf) throw

private final Map<String, Object> mapping;

private DynamicTemplate(String name, String pathMatch, String pathUnmatch, String match, String unmatch,
DynamicTemplate(String name, String pathMatch, String pathUnmatch, String match, String unmatch,
XContentFieldType xcontentFieldType, MatchType matchType, Map<String, Object> mapping) {
this.name = name;
this.pathMatch = pathMatch;
Expand All @@ -245,10 +246,44 @@ private DynamicTemplate(String name, String pathMatch, String pathUnmatch, Strin
this.mapping = mapping;
}

public String name() {
DynamicTemplate(DynamicTemplate original) {
this(original.name, original.pathMatch, original.pathUnmatch, original.match, original.unmatch, original.xcontentFieldType,
original.matchType, new HashMap<>(original.mapping));
}

String name() {
return this.name;
}

String getPathMatch() {
return pathMatch;
}

String getPathUnmatch() {
return pathUnmatch;
}

String getMatch() {
return match;
}

String getUnmatch() {
return unmatch;
}

MatchType getMatchType() {
return matchType;
}

XContentFieldType getXcontentFieldType() {
return xcontentFieldType;
}

Map<String, Object> getMapping() {
return mapping;
}


public boolean match(String path, String name, XContentFieldType xcontentFieldType) {
if (pathMatch != null && !matchType.matches(pathMatch, path)) {
return false;
Expand Down Expand Up @@ -362,4 +397,28 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.endObject();
return builder;
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (obj == null || obj.getClass() != DynamicTemplate.class) {
return false;
}
DynamicTemplate other = (DynamicTemplate) obj;
return Objects.equals(name, other.name) &&
Objects.equals(pathMatch, other.pathMatch)
&& Objects.equals(pathUnmatch, other.pathUnmatch)
&& Objects.equals(match, other.match)
&& Objects.equals(unmatch, other.unmatch)
&& Objects.equals(matchType, other.matchType)
&& Objects.equals(xcontentFieldType, other.xcontentFieldType)
&& Objects.equals(mapping, other.mapping);
}

@Override
public int hashCode() {
return Objects.hash(name, pathMatch, pathUnmatch, match, unmatch, matchType, xcontentFieldType, mapping);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.Version;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.xcontent.ToXContent;
Expand All @@ -32,9 +33,11 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue;
import static org.elasticsearch.index.mapper.TypeParsers.parseDateTimeFormatter;
Expand Down Expand Up @@ -170,6 +173,7 @@ protected boolean processField(RootObjectMapper.Builder builder, String fieldNam
*/
List<?> tmplNodes = (List<?>) fieldNode;
List<DynamicTemplate> templates = new ArrayList<>();
Set<DynamicTemplate> definedTemplates = new HashSet<>();
for (Object tmplNode : tmplNodes) {
Map<String, Object> tmpl = (Map<String, Object>) tmplNode;
if (tmpl.size() != 1) {
Expand All @@ -180,7 +184,12 @@ protected boolean processField(RootObjectMapper.Builder builder, String fieldNam
Map<String, Object> templateParams = (Map<String, Object>) entry.getValue();
DynamicTemplate template = DynamicTemplate.parse(templateName, templateParams);
if (template != null) {
if (definedTemplates.contains(template)) {
throw new MapperParsingException(
"A dynamic template was defined twice with the same definition: " + Strings.toString(template));
}
templates.add(template);
definedTemplates.add(template);
}
}
builder.dynamicTemplates(templates);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ private void assertMapEntriesAndImmutability(
assertMapImmutability(map);
}

private static <K, V> Map<K, V> randomMap(int size, Supplier<K> keyGenerator, Supplier<V> valueGenerator) {
public static <K, V> Map<K, V> randomMap(int size, Supplier<K> keyGenerator, Supplier<V> valueGenerator) {
final Map<K, V> map = new HashMap<>();
IntStream.range(0, size).forEach(i -> map.put(keyGenerator.get(), valueGenerator.get()));
return map;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,20 @@
package org.elasticsearch.index.mapper;

import org.elasticsearch.common.Strings;
import org.elasticsearch.common.util.MapsTests;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.index.mapper.DynamicTemplate.MatchType;
import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType;
import org.elasticsearch.test.ESTestCase;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;

public class DynamicTemplateTests extends ESTestCase {

public void testParseUnknownParam() throws Exception {
Expand Down Expand Up @@ -125,4 +129,62 @@ public void testSerialization() throws Exception {
template.toXContent(builder, ToXContent.EMPTY_PARAMS);
assertEquals("{\"match\":\"^a$\",\"match_pattern\":\"regex\",\"mapping\":{\"store\":true}}", Strings.toString(builder));
}

/**
* Test equality and hashCode properties
*/
public void testEqualsAndHashcode() {
checkEqualsAndHashCode(randomTemplate(), original -> new DynamicTemplate(original), original -> {
String name = original.name();
String pathMatch = original.getPathMatch();
String pathUnmatch = original.getPathUnmatch();
String match = original.getMatch();
String unmatch = original.getUnmatch();
XContentFieldType xContentFieldType = original.getXcontentFieldType();
MatchType matchType = original.getMatchType();
Map<String, Object> mapping = original.getMapping();
int switchCase = randomInt(7);
switch (switchCase) {
case 0:
name = name + randomAlphaOfLength(3);
break;
case 1:
pathMatch = pathMatch + randomAlphaOfLength(3);
break;
case 2:
pathUnmatch = pathUnmatch + randomAlphaOfLength(3);
break;
case 3:
match = match + randomAlphaOfLength(3);
break;
case 4:
unmatch = unmatch + randomAlphaOfLength(3);
break;
case 5:
xContentFieldType = randomValueOtherThan(xContentFieldType, () -> randomFrom(XContentFieldType.values()));
break;
case 6:
matchType = randomValueOtherThan(matchType, () -> randomFrom(MatchType.values()));
break;
case 7:
mapping = new HashMap<>(mapping);
mapping.put(randomAlphaOfLength(10), randomAlphaOfLength(10));
break;
}
return new DynamicTemplate(name, pathMatch, pathUnmatch , match, unmatch, xContentFieldType, matchType, mapping);
});
}

private DynamicTemplate randomTemplate() {
String name = randomAlphaOfLengthBetween(4, 8);
String pathMatch = randomAlphaOfLengthBetween(4, 8);
String pathUnmatch = randomAlphaOfLengthBetween(4, 8);
String match = randomAlphaOfLengthBetween(4, 8);
String unmatch = randomAlphaOfLengthBetween(4, 8);
XContentFieldType xContentFieldType = randomFrom(XContentFieldType.values());
MatchType matchType = randomFrom(MatchType.values());
Map<String, Object> mapping = MapsTests.randomMap(randomIntBetween(0, 10), () -> ESTestCase.randomAlphaOfLength(5),
() -> ESTestCase.randomAlphaOfLength(5));
return new DynamicTemplate(name, pathMatch, pathUnmatch , match, unmatch, xContentFieldType, matchType, mapping);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,75 @@ public void testIllegalFormatField() throws Exception {
assertEquals("Invalid format: [[test_format]]: expected string value", e.getMessage());
}
}

public void testDynamicTemplatesDuplicationThrows() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder()
.startObject()
.startObject("type")
.startArray("dynamic_templates")
.startObject()
.startObject("my_template")
.field("match_mapping_type", "string")
.startObject("mapping")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.startObject()
.startObject("my_template")
.field("match_mapping_type", "string")
.startObject("mapping")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.endArray()
.endObject()
.endObject());
MapperService mapperService = createIndex("test").mapperService();
MapperParsingException ex = expectThrows(MapperParsingException.class,
() -> mapperService.merge("type", new CompressedXContent(mapping), MergeReason.MAPPING_UPDATE));
assertEquals(
"Failed to parse mapping: A dynamic template was defined twice with the same definition: "
+ "{\"match_mapping_type\":\"string\",\"mapping\":{\"type\":\"keyword\"}}",
ex.getMessage());
}

public void testDynamicTemplatesDuplicateNamesDoesntThrow() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder()
.startObject()
.startObject("type")
.startArray("dynamic_templates")
.startObject()
.startObject("labels")
.field("path_match", "labels.*")
.field("match_mapping_type", "string")
.startObject("mapping")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.startObject()
.startObject("labels")
.field("path_match", "labels.*")
.field("match_mapping_type", "boolean")
.startObject("mapping")
.field("type", "boolean")
.endObject()
.endObject()
.endObject()
.startObject()
.startObject("labels")
.field("path_match", "labels.*")
.startObject("mapping")
.field("type", "scaled_float")
.endObject()
.endObject()
.endObject()
.endArray()
.endObject()
.endObject());
MapperService mapperService = createIndex("test").mapperService();
mapperService.merge("type", new CompressedXContent(mapping), MergeReason.MAPPING_UPDATE);
}
}

0 comments on commit e65f286

Please sign in to comment.