Add strict parsing of aggregation ranges (#25769)

Currently we ignore unknown field names when parsing RangeAggregator.Range and
GeoDistanceAggregationBuilder.Range from `range`, `date_range` or `geo_distance`
aggregations. This can hide subtle errors in the query. This change makes parsing `ranges`
stricter.

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<ValuesSource.GeoPoint, GeoDistanceAggregationBuilder> {
     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) {

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);

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<DateRangeAggregationBuilder> {
 
     @Override
@@ -62,4 +69,17 @@ public class DateRangeTests extends BaseAggregationTestCase<DateRangeAggregation
         return factory;
     }
 
+    public void testParsingRangeStrict() throws IOException {
+        final String rangeAggregation = "{\n" +
+                "\"field\" : \"date\",\n" +
+                "\"format\" : \"yyyy-MM-dd\",\n" +
+                "\"ranges\" : [\n" +
+                "    { \"from\" : \"2017-01-01\", \"to\" : \"2017-01-02\", \"badField\" : \"abcd\" }\n" +
+                "]\n" +
+            "}";
+        XContentParser parser = createParser(JsonXContent.jsonXContent, rangeAggregation);
+        ParsingException ex = expectThrows(ParsingException.class, () -> DateRangeAggregationBuilder.parse("aggregationName", parser));
+        assertThat(ex.getDetailedMessage(), containsString("badField"));
+    }
+
 }

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<GeoDistanceAggregationBuilder> {
 
     @Override
@@ -61,4 +68,37 @@ public class GeoDistanceRangeTests extends BaseAggregationTestCase<GeoDistanceAg
         return factory;
     }
 
+    public void testParsingRangeStrict() throws IOException {
+        final String rangeAggregation = "{\n" +
+                "\"field\" : \"location\",\n" +
+                "\"origin\" : \"52.3760, 4.894\",\n" +
+                "\"unit\" : \"m\",\n" +
+                "\"ranges\" : [\n" +
+                "    { \"from\" : 10000, \"to\" : 20000, \"badField\" : \"abcd\" }\n" +
+                "]\n" +
+            "}";
+        XContentParser parser = createParser(JsonXContent.jsonXContent, rangeAggregation);
+        ParsingException ex = expectThrows(ParsingException.class, () -> 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);
+    }
 }

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<RangeAggregationBuilder> {
 
@@ -59,4 +66,32 @@ public class RangeTests extends BaseAggregationTestCase<RangeAggregationBuilder>
         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);
+    }
 }