Skip to content

Update BooleanFieldMapper(boolean fields) to support ignore_malformed… #29522

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

Closed
wants to merge 1 commit into from
Closed

Update BooleanFieldMapper(boolean fields) to support ignore_malformed… #29522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 63 additions & 12 deletions server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import org.apache.lucene.search.TermQuery;
import org.apache.lucene.search.TermRangeQuery;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.Version;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.Loggers;
Expand Down Expand Up @@ -70,15 +72,23 @@ public static class Defaults {
FIELD_TYPE.setSearchAnalyzer(Lucene.KEYWORD_ANALYZER);
FIELD_TYPE.freeze();
}
public static final Explicit<Boolean> IGNORE_MALFORMED = new Explicit<>(false, false);
}

// @VisibleForTesting
static final String TRUE = "T";
// @VisibleForTesting
static final String FALSE = "F";

public static class Values {
public static final BytesRef TRUE = new BytesRef("T");
public static final BytesRef FALSE = new BytesRef("F");
public static final BytesRef TRUE = new BytesRef(BooleanFieldMapper.TRUE);
public static final BytesRef FALSE = new BytesRef(BooleanFieldMapper.FALSE);
}

public static class Builder extends FieldMapper.Builder<Builder, BooleanFieldMapper> {

private Boolean ignoreMalformed;

public Builder(String name) {
super(name, Defaults.FIELD_TYPE, Defaults.FIELD_TYPE);
this.builder = this;
Expand All @@ -92,11 +102,31 @@ public Builder tokenized(boolean tokenized) {
return super.tokenized(tokenized);
}

public Builder ignoreMalformed(boolean ignoreMalformed) {
this.ignoreMalformed = ignoreMalformed;
return builder;
}

protected Explicit<Boolean> ignoreMalformed(BuilderContext context) {
if (ignoreMalformed != null) {
return new Explicit<>(ignoreMalformed, true);
}
if (context.indexSettings() != null) {
return new Explicit<>(IGNORE_MALFORMED_SETTING.get(context.indexSettings()), false);
}
return BooleanFieldMapper.Defaults.IGNORE_MALFORMED;
}

@Override
public BooleanFieldMapper build(BuilderContext context) {
setupFieldType(context);
return new BooleanFieldMapper(name, fieldType, defaultFieldType,
context.indexSettings(), multiFieldsBuilder.build(this, context), copyTo);
return new BooleanFieldMapper(name,
fieldType,
defaultFieldType,
context.indexSettings(),
multiFieldsBuilder.build(this, context),
ignoreMalformed(context),
copyTo);
}
}

Expand All @@ -115,6 +145,9 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
}
builder.nullValue(XContentMapValues.nodeBooleanValue(propNode, name + ".null_value"));
iterator.remove();
} else if (propName.equals("ignore_malformed")) {
builder.ignoreMalformed(XContentMapValues.nodeBooleanValue(propNode, name + ".ignore_malformed"));
iterator.remove();
}
}
return builder;
Expand Down Expand Up @@ -184,12 +217,12 @@ public Boolean valueForDisplay(Object value) {
return null;
}
switch(value.toString()) {
case "F":
case FALSE:
return false;
case "T":
case TRUE:
return true;
default:
throw new IllegalArgumentException("Expected [T] or [F] but got [" + value + "]");
throw new IllegalArgumentException("Expected [" + TRUE + "] or [" + FALSE + "] but got [" + value + "]");
}
}

Expand Down Expand Up @@ -221,9 +254,17 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower
}
}

protected BooleanFieldMapper(String simpleName, MappedFieldType fieldType, MappedFieldType defaultFieldType,
Settings indexSettings, MultiFields multiFields, CopyTo copyTo) {
private Explicit<Boolean> ignoreMalformed;

protected BooleanFieldMapper(String simpleName,
MappedFieldType fieldType,
MappedFieldType defaultFieldType,
Settings indexSettings,
MultiFields multiFields,
Explicit<Boolean> ignoreMalformed,
CopyTo copyTo) {
super(simpleName, fieldType, defaultFieldType, indexSettings, multiFields, copyTo);
this.ignoreMalformed = ignoreMalformed;
}

@Override
Expand All @@ -239,21 +280,31 @@ protected void parseCreateField(ParseContext context, List<IndexableField> field

Boolean value = context.parseExternalValue(Boolean.class);
if (value == null) {
XContentParser.Token token = context.parser().currentToken();
final XContentParser parser = context.parser();
XContentParser.Token token = parser.currentToken();
if (token == XContentParser.Token.VALUE_NULL) {
if (fieldType().nullValue() != null) {
value = fieldType().nullValue();
}
} else {
value = context.parser().booleanValue();
if (ignoreMalformed.value()) {
if(parser.isBooleanValue()){
value = parser.booleanValue();
} else {
parser.skipChildren();
}
} else {
// if value is really an array/object/number let parser crash and burn!
Copy link
Member

@cbuescher cbuescher Apr 16, 2018

Choose a reason for hiding this comment

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

;-) nice comment, I'd like it to stay but could you make an addition that this should be the exceptional case? Maybe something like "Usually we expect a boolean here, but ..."

value = parser.booleanValue();
}
}
}

if (value == null) {
return;
}
if (fieldType().indexOptions() != IndexOptions.NONE || fieldType().stored()) {
fields.add(new Field(fieldType().name(), value ? "T" : "F", fieldType()));
fields.add(new Field(fieldType().name(), value ? TRUE : FALSE, fieldType()));
}
if (fieldType().hasDocValues()) {
fields.add(new SortedNumericDocValuesField(fieldType().name(), value ? 1 : 0));
Expand Down
194 changes: 173 additions & 21 deletions server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressedXContent;
Expand All @@ -49,16 +48,19 @@
import org.junit.Before;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;

import static org.hamcrest.Matchers.containsString;

public class BooleanFieldMapperTests extends ESSingleNodeTestCase {

private static final boolean FIELD1_IGNORE_MALFORMED = true;
private static final boolean FIELD1_DONT_IGNORE_MALFORMED = !FIELD1_IGNORE_MALFORMED;
private static final String FIELD1 = "field1";
private static final String FIELD2 = "field2";

private IndexService indexService;
private DocumentMapperParser parser;
private DocumentMapperParser preEs6Parser;

@Before
public void setup() {
Expand Down Expand Up @@ -130,26 +132,176 @@ public void testSerialization() throws IOException {
assertEquals("{\"field\":{\"type\":\"boolean\",\"doc_values\":false,\"null_value\":true}}", Strings.toString(builder));
}

public void testParsesBooleansStrict() throws IOException {
String mapping = Strings.toString(XContentFactory.jsonBuilder()
public void test_null() throws Exception{
Copy link
Member

@cbuescher cbuescher Apr 16, 2018

Choose a reason for hiding this comment

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

I like the level of detail in these tests, the degree of splitting them in into so many test methods and helpers is maybe a little bit hard to read. The following are just some suggestions of how to maybe group some of the tests.
e.g the null, true and false test could be grouped into one case that checks allowed fields fort the allowMalformed field.

this.parseAndCheck(FIELD1_IGNORE_MALFORMED, null, null);
}

public void test_true() throws Exception{
this.parseAndCheck(FIELD1_IGNORE_MALFORMED,true, BooleanFieldMapper.TRUE);
}

public void test_false() throws Exception{
this.parseAndCheck(FIELD1_IGNORE_MALFORMED,false, BooleanFieldMapper.FALSE);
}

public void test_on() throws Exception{
this.parseFails("on");
}

public void test_off() throws Exception{
this.parseFails("off");
}

public void test_yes() throws Exception{
this.parseFails("yes");
}

public void test_no() throws Exception{
this.parseFails("no");
}

public void test_0() throws Exception{
this.parseFails("0");
}

public void test_1() throws Exception{
Copy link
Member

@cbuescher cbuescher Apr 16, 2018

Choose a reason for hiding this comment

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

Could you group all the failure tests for different input strings into one tests that e.g. loops over all of the values you want to check?

Copy link
Author

@mP1 mP1 Apr 16, 2018

Choose a reason for hiding this comment

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

This group has several values and its better to test them individually, so we get failures for each case, if it becomes one test, then all we know is that it fails the first which is less helpful.

Copy link
Author

@mP1 mP1 Apr 16, 2018

Choose a reason for hiding this comment

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

By creating these helpers its very simple to add a new test for a new input, most of each test is boilerplate, that leaves each test method to be down to the bare minimum.

Take an input(value, ignore_malformed(Y/N), execute, and some expectation (fail or expected value).

Super clean, super concise, super easy to add more tests for more inputs as necessary.

this.parseFails("1");
}

public void test_string_IgnoreMalformed() throws Exception{
this.parseAndCheck(FIELD1_IGNORE_MALFORMED,
sourceWithString(),
null, BooleanFieldMapper.TRUE);
}

public void test_string() throws Exception{
this.parseFails(sourceWithString());
}

private BytesReference sourceWithString() throws Exception{
return BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
.startObject("type")
.startObject("properties")
.startObject("field")
.field("type", "boolean")
.endObject()
.endObject()
.endObject()
.field(FIELD1, "**Never convertable to boolean**") // IGNORED
.field(FIELD2, true)
.endObject());
DocumentMapper defaultMapper = parser.parse("type", new CompressedXContent(mapping));
BytesReference source = BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
// omit "false"/"true" here as they should still be parsed correctly
.field("field", randomFrom("off", "no", "0", "on", "yes", "1"))
.endObject());
}

public void test_number_IgnoreMalformed() throws Exception{
this.parseAndCheck(FIELD1_IGNORE_MALFORMED,
this.sourceWithNumber(1),
null, BooleanFieldMapper.TRUE);
}

public void test_number() throws Exception{
this.parseFails(this.sourceWithNumber(1));
}

private BytesReference sourceWithNumber(final int value) throws Exception{
return BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
.field(FIELD1, value) // IGNORED
.field(FIELD2, true)
.endObject());
}

public void test_object_IgnoreMalformed() throws Exception{
Copy link
Member

@cbuescher cbuescher Apr 16, 2018

Choose a reason for hiding this comment

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

Although the code is clear, the level of indirection here makes it hard for the reader to figure out that this (and similar other) test does. As a suggestion, what about combining test_object/test_object_IgnoreMalformed, then the code from sourceWithObject can be inlined in the test case. Also I would make the assertion on field1 and field2 explicit, even if that means a few more lines of code. In this case I would trade repetition for readability.

Copy link
Author

@mP1 mP1 Apr 16, 2018

Choose a reason for hiding this comment

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

As a suggestion, what about combining test_object/test_object_IgnoreMalformed, then the code from sourceWithObject can be inlined in the test case.

Again if you combine them into one method, and it fails you dont know if the "2nd" test is also broken because the test fails and aborts on the first.

More small tests are better than one bigger test that fails on the first.

this.parseAndCheck(FIELD1_IGNORE_MALFORMED,
this.sourceWithObject(),
null, BooleanFieldMapper.TRUE);
}

public void test_object() throws Exception{
this.parseFails(FIELD1_DONT_IGNORE_MALFORMED,
this.sourceWithObject());
}

private BytesReference sourceWithObject() throws Exception{
return BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
.startObject(FIELD1) // IGNORED
.field("field3", false)
.endObject()
.field(FIELD2, true)
.endObject());
}

public void test_array_IgnoreMalformed() throws Exception{
this.parseAndCheck(FIELD1_IGNORE_MALFORMED,
sourceWithArray(),
null, BooleanFieldMapper.TRUE);
}

public void test_array() throws Exception{
this.parseFails(sourceWithArray());
}

private BytesReference sourceWithArray() throws Exception{
return BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
.startArray(FIELD1) // IGNORED
.startObject()
.endObject()
.endArray()
.field(FIELD2, true)
.endObject());
}

private BytesReference source(final Object value1, final Object value2) throws IOException{
return BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
.field(FIELD1, value1)
.field(FIELD2, value2)
.endObject());
}

private Document parse(final boolean field1IgnoreMalformed, final BytesReference source) throws IOException{
final String mapping = this.mapping(field1IgnoreMalformed);
final DocumentMapper mapper = this.parser.parse("type", new CompressedXContent(mapping));
return mapper.parse(SourceToParse.source("test", "type", "1", source, XContentType.JSON)).rootDoc();
}

private void parseAndCheck(final boolean field1IgnoreMalformed,
final Object value,
final String expected) throws IOException
{
this.parseAndCheck(field1IgnoreMalformed, this.source(value, value), expected, expected);
}

private String mapping(final boolean field1IgnoreMalformed) throws IOException{
return Strings.toString(XContentFactory.jsonBuilder()
.startObject()
.startObject("type")
.startObject("properties")
.startObject(FIELD1)
.field("type", "boolean")
.field("ignore_malformed", field1IgnoreMalformed)
.endObject()
.startObject(FIELD2)
.field("type", "boolean")
.endObject()
.endObject()
.endObject()
.endObject());
}

private void parseAndCheck(final boolean field1IgnoreMalformed,
final BytesReference source,
final String expected1,
final String expected2) throws IOException{
final Document document = this.parse(field1IgnoreMalformed, source);
assertEquals(FIELD2, expected2, document.get(FIELD2));
assertEquals(FIELD1, expected1, document.get(FIELD1));
}

private void parseFails(final Object value) throws IOException{
parseFails(FIELD1_DONT_IGNORE_MALFORMED, this.source(value, null));
}

private void parseFails(final boolean field1IgnoreMalformed,
final BytesReference source) {
MapperParsingException ex = expectThrows(MapperParsingException.class,
() -> defaultMapper.parse(SourceToParse.source("test", "type", "1", source, XContentType.JSON)));
assertEquals("failed to parse [field]", ex.getMessage());
() -> parse(field1IgnoreMalformed, source));
assertEquals("failed to parse [field1]", ex.getMessage());
}

public void testMultiFields() throws IOException {
Expand Down