Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't detect source's XContentType in DocumentParser.parseDocument() #26880

Merged
merged 2 commits into from
Oct 10, 2017
Merged

Don't detect source's XContentType in DocumentParser.parseDocument() #26880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11d71d1
Don't detect source's XContentType in DocumentParser.parseDocument()
tlrx Oct 4, 2017
af7e2d9
Fix PercolatorQuerySearchIT
tlrx Oct 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 21 additions & 22 deletions core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Open in desktop
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 @@ -467,7 +468,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 @@ -481,14 +482,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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what moving out the call to splitAndValidatePath() from getMapper() saves here? As far as I understand it it seems better to centralize this in getMapper, but I might miss something here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came across this while I was debugging step-by-step this portion of code; I've been surprised that the field name was split and validated twice: one in getMapper() and another right after in the else condition. I can revert this if you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I understand this now. Makes sense, although it seems nice to hide the call to splitAndValidatePath in the getMapper method.

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 @@ -518,7 +518,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 @@ -529,8 +531,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 @@ -589,12 +589,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 @@ -607,7 +607,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 @@ -893,15 +893,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 @@ -929,8 +929,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
Open in desktop
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
Open in desktop
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
Open in desktop
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