From 004c40935a8d34f04fa76d0e6d1ace125c08493e Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 10 Oct 2017 15:31:56 +0200 Subject: [PATCH] Don't detect source's XContentType in DocumentParser.parseDocument() (#26880) DocumentParser.parseDocument() auto detects the XContentType of the document to parse, but this information is already provided by SourceToParse. --- .../index/mapper/DocumentParser.java | 43 +++++++++---------- .../index/mapper/DynamicMappingTests.java | 5 +-- .../index/mapper/SourceFieldMapperTests.java | 2 +- .../percolator/PercolatorQuerySearchIT.java | 4 +- 4 files changed, 26 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 227edc23d7df0..8235c58584eb9 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -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; @@ -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); @@ -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 { @@ -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) { @@ -331,7 +332,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; @@ -466,7 +467,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); @@ -482,14 +483,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 parentMapperTuple = getDynamicParentMapper(context, paths, mapper); ObjectMapper parentMapper = parentMapperTuple.v2(); @@ -519,7 +519,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 @@ -530,8 +532,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 parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); @@ -590,12 +590,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 parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); parentMapper = parentMapperTuple.v2(); @@ -608,7 +608,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); @@ -894,7 +894,7 @@ private static Tuple getDynamicParentMapper(ParseContext break; case FALSE: // Should not dynamically create any more mappers so return the last mapper - return new Tuple(pathsAdded, parent); + return new Tuple<>(pathsAdded, parent); } } @@ -902,7 +902,7 @@ private static Tuple getDynamicParentMapper(ParseContext pathsAdded++; parent = mapper; } - return new Tuple(pathsAdded, mapper); + return new Tuple<>(pathsAdded, mapper); } // find what the dynamic setting is given the current parse context and parent @@ -930,8 +930,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) { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java index 06c31f4dd1849..023d2249f2f82 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java @@ -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(); } @@ -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")); diff --git a/core/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java index 3b73b5dfd3770..85017cb35cd39 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java @@ -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)); } diff --git a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java index 55835dec92b7a..b57953475a607 100644 --- a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java +++ b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java @@ -606,10 +606,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)))