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

Fix error applying ignore_malformed to boolean values #41261

Merged
merged 3 commits into from
Apr 17, 2019
Merged

Fix error applying ignore_malformed to boolean values #41261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ccb4b6b
Fix error applying `ignore_malformed` to boolean values
Apr 16, 2019
31cface
iter
Apr 16, 2019
eca5042
Adressing comments
Apr 17, 2019
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
6 changes: 4 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package org.elasticsearch.index.mapper;

import com.fasterxml.jackson.core.JsonParseException;

import org.apache.lucene.document.DoublePoint;
import org.apache.lucene.document.Field;
import org.apache.lucene.document.FloatPoint;
Expand Down Expand Up @@ -1042,8 +1044,8 @@ protected void parseCreateField(ParseContext context, List<IndexableField> field
} else {
try {
numericValue = fieldType().type.parse(parser, coerce.value());
} catch (IllegalArgumentException e) {
if (ignoreMalformed.value()) {
} catch (IllegalArgumentException | JsonParseException e) {
Copy link
Contributor

@jtibshirani jtibshirani Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't tested this, but I wanted to check that this will not introduce too much leniency. In particular, I'm wondering what happens if we try to pass a JSON object to a numeric field with ignore_malformed enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

The way the parser works, anything that's not a number or string token after the field name should throw an error. If "swallowing" that token with ignore_malformed leads to unparseable xcontent afterwards we will still throw an error. I think that's what should happen.

Copy link
Contributor

@jtibshirani jtibshirani Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for clarifying. What would you think about adding a check whether the token is a value, as in if (ignoreMalformed.value() && parser.currentToken().isValue()) { ... }? That way a user would get a clear, localized error like "Current token (START_OBJECT) not numeric", as opposed to a more difficult one to track down like "Malformed content, found extra data after parsing: END_OBJECT".

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with that is that the code actually throwing the exception is in the jackson library. The only way I could think of to do this is either have some logic taking apart the exception or pre-checking the token in NumberFieldMapper. I'd try to avoid the former, was the later what you had in mind? We'd need to test for (and accept) all value tokens here basically?

if (ignoreMalformed.value() && parser.currentToken().isValue()) {
context.addIgnoredField(fieldType.name());
return;
} else {
Expand Down
86 changes: 55 additions & 31 deletions server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@
package org.elasticsearch.index.mapper;

import com.carrotsearch.randomizedtesting.annotations.Timeout;

import org.apache.lucene.index.DocValuesType;
import org.apache.lucene.index.IndexableField;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.mapper.NumberFieldMapper.NumberType;
Expand All @@ -37,6 +40,7 @@
import java.util.HashSet;
import java.util.List;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.containsString;

public class NumberFieldMapperTests extends AbstractNumericFieldMapperTestCase {
Expand Down Expand Up @@ -218,45 +222,65 @@ protected void doTestDecimalCoerce(String type) throws IOException {

public void testIgnoreMalformed() throws Exception {
for (String type : TYPES) {
doTestIgnoreMalformed(type);
}
}
for (Object malformedValue : new Object[] { "a", Boolean.FALSE }) {
String mapping = Strings.toString(jsonBuilder().startObject().startObject("type").startObject("properties")
.startObject("field").field("type", type).endObject().endObject().endObject().endObject());

private void doTestIgnoreMalformed(String type) throws IOException {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties").startObject("field").field("type", type).endObject().endObject()
.endObject().endObject());
DocumentMapper mapper = parser.parse("type", new CompressedXContent(mapping));

DocumentMapper mapper = parser.parse("type", new CompressedXContent(mapping));
assertEquals(mapping, mapper.mappingSource().toString());

assertEquals(mapping, mapper.mappingSource().toString());
ThrowingRunnable runnable = () -> mapper.parse(new SourceToParse("test", "type", "1",
BytesReference.bytes(jsonBuilder().startObject().field("field", malformedValue).endObject()), XContentType.JSON));
MapperParsingException e = expectThrows(MapperParsingException.class, runnable);
if (malformedValue instanceof String) {
assertThat(e.getCause().getMessage(), containsString("For input string: \"a\""));
} else {
assertThat(e.getCause().getMessage(), containsString("Current token"));
assertThat(e.getCause().getMessage(), containsString("not numeric, can not use numeric value accessors"));
}

ThrowingRunnable runnable = () -> mapper.parse(new SourceToParse("test", "type", "1", BytesReference
.bytes(XContentFactory.jsonBuilder()
.startObject()
.field("field", "a")
.endObject()),
XContentType.JSON));
MapperParsingException e = expectThrows(MapperParsingException.class, runnable);

assertThat(e.getCause().getMessage(), containsString("For input string: \"a\""));
mapping = Strings.toString(jsonBuilder().startObject().startObject("type").startObject("properties").startObject("field")
.field("type", type).field("ignore_malformed", true).endObject().endObject().endObject().endObject());

mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties").startObject("field").field("type", type).field("ignore_malformed", true).endObject().endObject()
.endObject().endObject());
DocumentMapper mapper2 = parser.parse("type", new CompressedXContent(mapping));

DocumentMapper mapper2 = parser.parse("type", new CompressedXContent(mapping));
ParsedDocument doc = mapper2.parse(new SourceToParse("test", "type", "1",
BytesReference.bytes(jsonBuilder().startObject().field("field", malformedValue).endObject()), XContentType.JSON));

ParsedDocument doc = mapper2.parse(new SourceToParse("test", "type", "1", BytesReference
.bytes(XContentFactory.jsonBuilder()
.startObject()
.field("field", "a")
.endObject()),
XContentType.JSON));
IndexableField[] fields = doc.rootDoc().getFields("field");
assertEquals(0, fields.length);
assertArrayEquals(new String[] { "field" }, doc.rootDoc().getValues("_ignored"));
}
}
}

IndexableField[] fields = doc.rootDoc().getFields("field");
assertEquals(0, fields.length);
assertArrayEquals(new String[] { "field" }, doc.rootDoc().getValues("_ignored"));
/**
* Test that in case the malformed value is an xContent object we throw error regardless of `ignore_malformed`
*/
public void testIgnoreMalformedWithObject() throws Exception {
for (String type : TYPES) {
Object malformedValue = new ToXContentObject() {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
return builder.startObject().field("foo", "bar").endObject();
}
};
for (Boolean ignoreMalformed : new Boolean[] { true, false }) {
String mapping = Strings.toString(
jsonBuilder().startObject().startObject("type").startObject("properties").startObject("field").field("type", type)
.field("ignore_malformed", ignoreMalformed).endObject().endObject().endObject().endObject());
DocumentMapper mapper = parser.parse("type", new CompressedXContent(mapping));
assertEquals(mapping, mapper.mappingSource().toString());

MapperParsingException e = expectThrows(MapperParsingException.class,
() -> mapper.parse(new SourceToParse("test", "type", "1",
BytesReference.bytes(jsonBuilder().startObject().field("field", malformedValue).endObject()),
XContentType.JSON)));
assertThat(e.getCause().getMessage(), containsString("Current token"));
assertThat(e.getCause().getMessage(), containsString("not numeric, can not use numeric value accessors"));
}
}
}

public void testRejectNorms() throws IOException {
Expand Down