Skip to content

Commit

Permalink
Don't detect source's XContentType in DocumentParser.parseDocument() (#…
Browse files Browse the repository at this point in the history 
…26880)

DocumentParser.parseDocument() auto detects the XContentType of the
document to parse, but this information is already provided by SourceToParse.
  • Loading branch information
@tlrx
tlrx committed Oct 10, 2017
1 parent 08daa30 commit 5e5670c
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 28 deletions.
43 changes: 21 additions & 22 deletions core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.elasticsearch.common.joda.FormatDateTimeFormatter;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType;
import org.elasticsearch.index.mapper.KeywordFieldMapper.KeywordFieldType;
Expand Down Expand Up @@ -58,9 +59,10 @@ ParsedDocument parseDocument(SourceToParse source) throws MapperParsingException

final Mapping mapping = docMapper.mapping();
final ParseContext.InternalParseContext context;
try (XContentParser parser = XContentHelper.createParser(docMapperParser.getXContentRegistry(), source.source())) {
context = new ParseContext.InternalParseContext(indexSettings.getSettings(),
docMapperParser, docMapper, source, parser);
final XContentType xContentType = source.getXContentType();

try (XContentParser parser = XContentHelper.createParser(docMapperParser.getXContentRegistry(), source.source(), xContentType)) {
context = new ParseContext.InternalParseContext(indexSettings.getSettings(), docMapperParser, docMapper, source, parser);
validateStart(parser);
internalParseDocument(mapping, context, parser);
validateEnd(parser);
Expand All @@ -74,8 +76,7 @@ ParsedDocument parseDocument(SourceToParse source) throws MapperParsingException

reverseOrder(context);

ParsedDocument doc = parsedDocument(source, context, createDynamicUpdate(mapping, docMapper, context.getDynamicMappers()));
return doc;
return parsedDocument(source, context, createDynamicUpdate(mapping, docMapper, context.getDynamicMappers()));
}

private static void internalParseDocument(Mapping mapping, ParseContext.InternalParseContext context, XContentParser parser) throws IOException {
Expand All @@ -89,7 +90,7 @@ private static void internalParseDocument(Mapping mapping, ParseContext.Internal
// entire type is disabled
parser.skipChildren();
} else if (emptyDoc == false) {
parseObjectOrNested(context, mapping.root, true);
parseObjectOrNested(context, mapping.root);
}

for (MetadataFieldMapper metadataMapper : mapping.metadataMappers) {
Expand Down Expand Up @@ -338,7 +339,7 @@ private static ObjectMapper createUpdate(ObjectMapper parent, String[] nameParts
return parent.mappingUpdate(mapper);
}

static void parseObjectOrNested(ParseContext context, ObjectMapper mapper, boolean atRoot) throws IOException {
static void parseObjectOrNested(ParseContext context, ObjectMapper mapper) throws IOException {
if (mapper.isEnabled() == false) {
context.parser().skipChildren();
return;
Expand Down Expand Up @@ -473,7 +474,7 @@ private static ParseContext nestedContext(ParseContext context, ObjectMapper map

private static void parseObjectOrField(ParseContext context, Mapper mapper) throws IOException {
if (mapper instanceof ObjectMapper) {
parseObjectOrNested(context, (ObjectMapper) mapper, false);
parseObjectOrNested(context, (ObjectMapper) mapper);
} else {
FieldMapper fieldMapper = (FieldMapper)mapper;
Mapper update = fieldMapper.parse(context);
Expand All @@ -487,14 +488,13 @@ private static void parseObjectOrField(ParseContext context, Mapper mapper) thro
private static void parseObject(final ParseContext context, ObjectMapper mapper, String currentFieldName) throws IOException {
assert currentFieldName != null;

Mapper objectMapper = getMapper(mapper, currentFieldName);
final String[] paths = splitAndValidatePath(currentFieldName);
Mapper objectMapper = getMapper(mapper, currentFieldName, paths);
if (objectMapper != null) {
context.path().add(currentFieldName);
parseObjectOrField(context, objectMapper);
context.path().remove();
} else {

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 @@ -524,7 +524,9 @@ private static void parseObject(final ParseContext context, ObjectMapper mapper,

private static void parseArray(ParseContext context, ObjectMapper parentMapper, String lastFieldName) throws IOException {
String arrayFieldName = lastFieldName;
Mapper mapper = getMapper(parentMapper, lastFieldName);

final String[] paths = splitAndValidatePath(arrayFieldName);
Mapper mapper = getMapper(parentMapper, lastFieldName, paths);
if (mapper != null) {
// There is a concrete mapper for this field already. Need to check if the mapper
// expects an array, if so we pass the context straight to the mapper and if not
Expand All @@ -535,8 +537,6 @@ private static void parseArray(ParseContext context, ObjectMapper parentMapper,
parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName);
}
} else {

final String[] paths = splitAndValidatePath(arrayFieldName);
arrayFieldName = paths[paths.length - 1];
lastFieldName = arrayFieldName;
Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper);
Expand Down Expand Up @@ -595,12 +595,12 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa
if (currentFieldName == null) {
throw new MapperParsingException("object mapping [" + parentMapper.name() + "] trying to serialize a value with no field associated with it, current value [" + context.parser().textOrNull() + "]");
}
Mapper mapper = getMapper(parentMapper, currentFieldName);

final String[] paths = splitAndValidatePath(currentFieldName);
Mapper mapper = getMapper(parentMapper, currentFieldName, paths);
if (mapper != null) {
parseObjectOrField(context, mapper);
} else {

final String[] paths = splitAndValidatePath(currentFieldName);
currentFieldName = paths[paths.length - 1];
Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper);
parentMapper = parentMapperTuple.v2();
Expand All @@ -613,7 +613,7 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa

private static void parseNullValue(ParseContext context, ObjectMapper parentMapper, String lastFieldName) throws IOException {
// we can only handle null values if we have mappings for them
Mapper mapper = getMapper(parentMapper, lastFieldName);
Mapper mapper = getMapper(parentMapper, lastFieldName, splitAndValidatePath(lastFieldName));
if (mapper != null) {
// TODO: passing null to an object seems bogus?
parseObjectOrField(context, mapper);
Expand Down Expand Up @@ -899,15 +899,15 @@ private static Tuple<Integer, ObjectMapper> getDynamicParentMapper(ParseContext
break;
case FALSE:
// Should not dynamically create any more mappers so return the last mapper
return new Tuple<Integer, ObjectMapper>(pathsAdded, parent);
return new Tuple<>(pathsAdded, parent);

}
}
context.path().add(paths[i]);
pathsAdded++;
parent = mapper;
}
return new Tuple<Integer, ObjectMapper>(pathsAdded, mapper);
return new Tuple<>(pathsAdded, mapper);
}

// find what the dynamic setting is given the current parse context and parent
Expand Down Expand Up @@ -935,8 +935,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 = splitAndValidatePath(fieldName);
private static Mapper getMapper(ObjectMapper objectMapper, String fieldName, String[] subfields) {
for (int i = 0; i < subfields.length - 1; ++i) {
Mapper mapper = objectMapper.getMapper(subfields[i]);
if (mapper == null || (mapper instanceof ObjectMapper) == false) {
Expand Down
5 changes: 2 additions & 3 deletions core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ private Mapper parse(DocumentMapper mapper, DocumentMapperParser parser, XConten
ParseContext.InternalParseContext ctx = new ParseContext.InternalParseContext(settings, parser, mapper, source, xContentParser);
assertEquals(XContentParser.Token.START_OBJECT, ctx.parser().nextToken());
ctx.parser().nextToken();
DocumentParser.parseObjectOrNested(ctx, mapper.root(), true);
DocumentParser.parseObjectOrNested(ctx, mapper.root());
Mapping mapping = DocumentParser.createDynamicUpdate(mapper.mapping(), mapper, ctx.getDynamicMappers());
return mapping == null ? null : mapping.root();
}
Expand Down Expand Up @@ -639,8 +639,7 @@ private void doTestDefaultFloatingPointMappings(DocumentMapper mapper, XContentB
.field("baz", (double) 3.2f) // double that can be accurately represented as a float
.field("quux", "3.2") // float detected through numeric detection
.endObject().bytes();
ParsedDocument parsedDocument = mapper.parse(SourceToParse.source("index", "type", "id", source,
XContentType.JSON));
ParsedDocument parsedDocument = mapper.parse(SourceToParse.source("index", "type", "id", source, builder.contentType()));
Mapping update = parsedDocument.dynamicMappingsUpdate();
assertNotNull(update);
assertThat(((FieldMapper) update.root().getMapper("foo")).fieldType().typeName(), equalTo("float"));
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public void testNoFormat() throws Exception {
doc = documentMapper.parse(SourceToParse.source("test", "type", "1", XContentFactory.smileBuilder().startObject()
.field("field", "value")
.endObject().bytes(),
XContentType.JSON));
XContentType.SMILE));

assertThat(XContentFactory.xContentType(doc.source()), equalTo(XContentType.SMILE));
}
Expand Down
4 changes: 2 additions & 2 deletions modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -805,10 +805,10 @@ public void testPercolatorQueryViaMultiSearch() throws Exception {
jsonBuilder().startObject().field("field1", "b").endObject().bytes(), XContentType.JSON)))
.add(client().prepareSearch("test")
.setQuery(new PercolateQueryBuilder("query",
yamlBuilder().startObject().field("field1", "c").endObject().bytes(), XContentType.JSON)))
yamlBuilder().startObject().field("field1", "c").endObject().bytes(), XContentType.YAML)))
.add(client().prepareSearch("test")
.setQuery(new PercolateQueryBuilder("query",
smileBuilder().startObject().field("field1", "b c").endObject().bytes(), XContentType.JSON)))
smileBuilder().startObject().field("field1", "b c").endObject().bytes(), XContentType.SMILE)))
.add(client().prepareSearch("test")
.setQuery(new PercolateQueryBuilder("query",
jsonBuilder().startObject().field("field1", "d").endObject().bytes(), XContentType.JSON)))
Expand Down

0 comments on commit 5e5670c

Please sign in to comment.