diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceAggregationBuilder.java index 7078541648c..eb643fb429f 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceAggregationBuilder.java @@ -30,6 +30,7 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser.Token; +import org.elasticsearch.common.xcontent.XContentParserUtils; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; @@ -45,6 +46,10 @@ import java.util.ArrayList; import java.util.List; import java.util.Objects; +import static org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range.FROM_FIELD; +import static org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range.KEY_FIELD; +import static org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range.TO_FIELD; + public class GeoDistanceAggregationBuilder extends ValuesSourceAggregationBuilder { public static final String NAME = "geo_distance"; static final ParseField ORIGIN_FIELD = new ParseField("origin", "center", "point", "por"); @@ -167,25 +172,38 @@ public class GeoDistanceAggregationBuilder extends ValuesSourceAggregationBuilde double from = 0.0; double to = Double.POSITIVE_INFINITY; String key = null; - String toOrFromOrKey = null; + String currentFieldName = null; Token token; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { - toOrFromOrKey = parser.currentName(); + currentFieldName = parser.currentName(); } else if (token == XContentParser.Token.VALUE_NUMBER) { - if (Range.FROM_FIELD.match(toOrFromOrKey)) { + if (FROM_FIELD.match(currentFieldName)) { from = parser.doubleValue(); - } else if (Range.TO_FIELD.match(toOrFromOrKey)) { + } else if (TO_FIELD.match(currentFieldName)) { to = parser.doubleValue(); + } else { + XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); } } else if (token == XContentParser.Token.VALUE_STRING) { - if (Range.KEY_FIELD.match(toOrFromOrKey)) { + if (KEY_FIELD.match(currentFieldName)) { key = parser.text(); - } else if (Range.FROM_FIELD.match(toOrFromOrKey)) { + } else if (FROM_FIELD.match(currentFieldName)) { fromAsStr = parser.text(); - } else if (Range.TO_FIELD.match(toOrFromOrKey)) { + } else if (TO_FIELD.match(currentFieldName)) { toAsStr = parser.text(); + } else { + XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); } + } else if (token == XContentParser.Token.VALUE_NULL) { + if (FROM_FIELD.match(currentFieldName) || TO_FIELD.match(currentFieldName) + || KEY_FIELD.match(currentFieldName)) { + // ignore null value + } else { + XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); + } + } else { + XContentParserUtils.throwUnknownToken(token, parser.getTokenLocation()); } } if (fromAsStr != null || toAsStr != null) { diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java index 4bbbdcba27e..a6268f75c09 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java @@ -26,6 +26,7 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentParserUtils; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.Aggregator; @@ -143,6 +144,8 @@ public class RangeAggregator extends BucketsAggregator { from = parser.doubleValue(); } else if (TO_FIELD.match(currentFieldName)) { to = parser.doubleValue(); + } else { + XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); } } else if (token == XContentParser.Token.VALUE_STRING) { if (FROM_FIELD.match(currentFieldName)) { @@ -151,7 +154,18 @@ public class RangeAggregator extends BucketsAggregator { toAsStr = parser.text(); } else if (KEY_FIELD.match(currentFieldName)) { key = parser.text(); + } else { + XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); } + } else if (token == XContentParser.Token.VALUE_NULL) { + if (FROM_FIELD.match(currentFieldName) || TO_FIELD.match(currentFieldName) + || KEY_FIELD.match(currentFieldName)) { + // ignore null value + } else { + XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); + } + } else { + XContentParserUtils.throwUnknownToken(token, parser.getTokenLocation()); } } return new Range(key, from, fromAsStr, to, toAsStr); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java index 516f4c4b01f..acb6b0f0992 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java @@ -19,10 +19,17 @@ package org.elasticsearch.search.aggregations.bucket; +import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.range.DateRangeAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range; +import java.io.IOException; + +import static org.hamcrest.Matchers.containsString; + public class DateRangeTests extends BaseAggregationTestCase { @Override @@ -62,4 +69,17 @@ public class DateRangeTests extends BaseAggregationTestCase DateRangeAggregationBuilder.parse("aggregationName", parser)); + assertThat(ex.getDetailedMessage(), containsString("badField")); + } + } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java index cdd4de3acd3..9549c22019b 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java @@ -19,14 +19,21 @@ package org.elasticsearch.search.aggregations.bucket; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.unit.DistanceUnit; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.range.GeoDistanceAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.range.GeoDistanceAggregationBuilder.Range; import org.elasticsearch.test.geo.RandomShapeGenerator; +import java.io.IOException; + +import static org.hamcrest.Matchers.containsString; + public class GeoDistanceRangeTests extends BaseAggregationTestCase { @Override @@ -61,4 +68,37 @@ public class GeoDistanceRangeTests extends BaseAggregationTestCase GeoDistanceAggregationBuilder.parse("aggregationName", parser)); + assertThat(ex.getDetailedMessage(), containsString("badField")); + } + + /** + * We never render "null" values to xContent, but we should test that we can parse them (and they return correct defaults) + */ + public void testParsingNull() throws IOException { + final String rangeAggregation = "{\n" + + "\"field\" : \"location\",\n" + + "\"origin\" : \"52.3760, 4.894\",\n" + + "\"unit\" : \"m\",\n" + + "\"ranges\" : [\n" + + " { \"from\" : null, \"to\" : null }\n" + + "]\n" + + "}"; + XContentParser parser = createParser(JsonXContent.jsonXContent, rangeAggregation); + GeoDistanceAggregationBuilder aggregationBuilder = (GeoDistanceAggregationBuilder) GeoDistanceAggregationBuilder + .parse("aggregationName", parser); + assertEquals(1, aggregationBuilder.range().size()); + assertEquals(0.0, aggregationBuilder.range().get(0).getFrom(), 0.0); + assertEquals(Double.POSITIVE_INFINITY, aggregationBuilder.range().get(0).getTo(), 0.0); + } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeTests.java index 89bcf4ebc04..b502645a24c 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeTests.java @@ -19,9 +19,16 @@ package org.elasticsearch.search.aggregations.bucket; +import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.search.aggregations.BaseAggregationTestCase; -import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range; import org.elasticsearch.search.aggregations.bucket.range.RangeAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range; + +import java.io.IOException; + +import static org.hamcrest.Matchers.containsString; public class RangeTests extends BaseAggregationTestCase { @@ -59,4 +66,32 @@ public class RangeTests extends BaseAggregationTestCase return factory; } + public void testParsingRangeStrict() throws IOException { + final String rangeAggregation = "{\n" + + "\"field\" : \"price\",\n" + + "\"ranges\" : [\n" + + " { \"from\" : 50, \"to\" : 100, \"badField\" : \"abcd\" }\n" + + "]\n" + + "}"; + XContentParser parser = createParser(JsonXContent.jsonXContent, rangeAggregation); + ParsingException ex = expectThrows(ParsingException.class, () -> RangeAggregationBuilder.parse("aggregationName", parser)); + assertThat(ex.getDetailedMessage(), containsString("badField")); + } + + /** + * We never render "null" values to xContent, but we should test that we can parse them (and they return correct defaults) + */ + public void testParsingNull() throws IOException { + final String rangeAggregation = "{\n" + + "\"field\" : \"price\",\n" + + "\"ranges\" : [\n" + + " { \"from\" : null, \"to\" : null }\n" + + "]\n" + + "}"; + XContentParser parser = createParser(JsonXContent.jsonXContent, rangeAggregation); + RangeAggregationBuilder aggregationBuilder = (RangeAggregationBuilder) RangeAggregationBuilder.parse("aggregationName", parser); + assertEquals(1, aggregationBuilder.ranges().size()); + assertEquals(Double.NEGATIVE_INFINITY, aggregationBuilder.ranges().get(0).getFrom(), 0.0); + assertEquals(Double.POSITIVE_INFINITY, aggregationBuilder.ranges().get(0).getTo(), 0.0); + } }