Navigation Menu

Skip to content

Commit

Permalink
Disallow introducing illegal object mappings (double '..')
Browse files Browse the repository at this point in the history 
This disallows object mappings that would accidentally create something like
`foo..bar`, which is then unparsable for the `bar` field as it does not know
what its parent is.

Resolves #22794
  • Loading branch information
@dakrone
dakrone committed Feb 1, 2017
1 parent 5c1410d commit 8d83edc
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 23 deletions.
26 changes: 19 additions & 7 deletions core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Expand Up @@ -22,6 +22,7 @@
import org.apache.lucene.document.Field;
import org.apache.lucene.index.IndexableField;
import org.elasticsearch.Version;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.joda.FormatDateTimeFormatter;
import org.elasticsearch.common.xcontent.XContentHelper;
Expand Down Expand Up @@ -172,6 +173,17 @@ private static MapperParsingException wrapInMapperParsingException(SourceToParse
return new MapperParsingException("failed to parse", e);
}

private static String[] splitAndValidatePath(String fullFieldPath) {
String[] parts = fullFieldPath.split("\\.");
for (String part : parts) {
if (Strings.hasText(part) == false) {
throw new IllegalArgumentException(
"object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]");
}
}
return parts;
}

/** Creates a Mapping containing any dynamically added fields, or returns null if there were no dynamic mappings. */
static Mapping createDynamicUpdate(Mapping mapping, DocumentMapper docMapper, List<Mapper> dynamicMappers) {
if (dynamicMappers.isEmpty()) {
Expand All @@ -184,7 +196,7 @@ static Mapping createDynamicUpdate(Mapping mapping, DocumentMapper docMapper, Li
Iterator<Mapper> dynamicMapperItr = dynamicMappers.iterator();
List<ObjectMapper> parentMappers = new ArrayList<>();
Mapper firstUpdate = dynamicMapperItr.next();
parentMappers.add(createUpdate(mapping.root(), firstUpdate.name().split("\\."), 0, firstUpdate));
parentMappers.add(createUpdate(mapping.root(), splitAndValidatePath(firstUpdate.name()), 0, firstUpdate));
Mapper previousMapper = null;
while (dynamicMapperItr.hasNext()) {
Mapper newMapper = dynamicMapperItr.next();
Expand All @@ -196,7 +208,7 @@ static Mapping createDynamicUpdate(Mapping mapping, DocumentMapper docMapper, Li
continue;
}
previousMapper = newMapper;
String[] nameParts = newMapper.name().split("\\.");
String[] nameParts = splitAndValidatePath(newMapper.name());

// We first need the stack to only contain mappers in common with the previously processed mapper
// For example, if the first mapper processed was a.b.c, and we now have a.d, the stack will contain
Expand Down Expand Up @@ -453,7 +465,7 @@ private static ObjectMapper parseObject(final ParseContext context, ObjectMapper
context.path().remove();
} else {

final String[] paths = currentFieldName.split("\\.");
final String[] paths = splitAndValidatePath(currentFieldName);
currentFieldName = paths[paths.length - 1];
Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, paths, mapper);
ObjectMapper parentMapper = parentMapperTuple.v2();
Expand Down Expand Up @@ -497,7 +509,7 @@ private static void parseArray(ParseContext context, ObjectMapper parentMapper,
}
} else {

final String[] paths = arrayFieldName.split("\\.");
final String[] paths = splitAndValidatePath(arrayFieldName);
arrayFieldName = paths[paths.length - 1];
lastFieldName = arrayFieldName;
Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper);
Expand Down Expand Up @@ -561,7 +573,7 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa
parseObjectOrField(context, mapper);
} else {

final String[] paths = currentFieldName.split("\\.");
final String[] paths = splitAndValidatePath(currentFieldName);
currentFieldName = paths[paths.length - 1];
Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper);
parentMapper = parentMapperTuple.v2();
Expand Down Expand Up @@ -813,7 +825,7 @@ private static void parseCopy(String field, ParseContext context) throws IOExcep
// The path of the dest field might be completely different from the current one so we need to reset it
context = context.overridePath(new ContentPath(0));

final String[] paths = field.split("\\.");
final String[] paths = splitAndValidatePath(field);
final String fieldName = paths[paths.length-1];
Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, paths, null);
ObjectMapper mapper = parentMapperTuple.v2();
Expand Down Expand Up @@ -897,7 +909,7 @@ private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper parentMapper,

// looks up a child mapper, but takes into account field names that expand to objects
static Mapper getMapper(ObjectMapper objectMapper, String fieldName) {
String[] subfields = fieldName.split("\\.");
String[] subfields = splitAndValidatePath(fieldName);
for (int i = 0; i < subfields.length - 1; ++i) {
Mapper mapper = objectMapper.getMapper(subfields[i]);
if (mapper == null || (mapper instanceof ObjectMapper) == false) {
Expand Down
33 changes: 33 additions & 0 deletions core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java
View file
Expand Up @@ -47,6 +47,7 @@
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.test.StreamsUtils.copyToBytesFromClasspath;
import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
Expand Down Expand Up @@ -1262,4 +1263,36 @@ public void testDynamicDateDetectionEnabledWithNoSpecialCharacters() throws IOEx
assertNotNull(dateMapper);
assertThat(dateMapper, instanceOf(DateFieldMapper.class));
}

public void testDynamicFieldsStartingAndEndingWithDot() throws Exception {
BytesReference bytes = XContentFactory.jsonBuilder().startObject().startArray("top.")
.startObject().startArray("foo.")
.startObject()
.field("thing", "bah")
.endObject().endArray()
.endObject().endArray()
.endObject().bytes();

client().prepareIndex("idx", "type").setSource(bytes).get();

bytes = XContentFactory.jsonBuilder().startObject().startArray("top.")
.startObject().startArray("foo.")
.startObject()
.startObject("bar.")
.startObject("aoeu")
.field("a", 1).field("b", 2)
.endObject()
.endObject()
.endObject()
.endArray().endObject().endArray()
.endObject().bytes();

try {
client().prepareIndex("idx", "type").setSource(bytes).get();
fail("should have failed to dynamically introduce a double-dot field");
} catch (IllegalArgumentException e) {
assertThat(e.getMessage(),
containsString("object field starting or ending with a [.] makes object resolution ambiguous: [top..foo..bar]"));
}
}
}
18 changes: 2 additions & 16 deletions core/src/test/java/org/elasticsearch/indexing/IndexActionIT.java
View file
Expand Up @@ -257,21 +257,7 @@ public void testDocumentWithBlankFieldName() {
}
);
assertThat(e.getMessage(), containsString("failed to parse"));
assertThat(e.getRootCause().getMessage(), containsString("name cannot be empty string"));
}

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Collections.singleton(InternalSettingsPlugin.class); // uses index.version.created
}

public void testDocumentWithBlankFieldName2x() {
Version version = VersionUtils.randomVersionBetween(random(), Version.V_2_0_0, Version.V_2_3_4);
Settings settings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, version).build();
assertAcked(prepareCreate("test1").setSettings(settings));
ensureGreen();

IndexResponse indexResponse = client().prepareIndex("test1", "type", "1").setSource("", "value1_2").execute().actionGet();
assertEquals(DocWriteResponse.Result.CREATED, indexResponse.getResult());
assertThat(e.getRootCause().getMessage(),
containsString("object field starting or ending with a [.] makes object resolution ambiguous: []"));
}
}

0 comments on commit 8d83edc

Please sign in to comment.