From 461db9210f82dbd3c5c36566dfb230f539099cf3 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Wed, 27 Nov 2019 13:30:07 -0500 Subject: [PATCH 1/7] Allow ObjectParsers to specified required field sets ConstructingObjectParser can be used to specify required fields, but it is still difficult to configure "sets" of fields where only one of the set is required (requiring hand-rolled logic in each ConstructingObjectParser, or adding special validation methods to objects that are called after building the object). This commit adds a new method on ObjectParser which allows the parsers to register required sets. E.g. ["foo", "bar"] can be registered, which means "foo", "bar" or both must be configured by the user otherwise an exception is thrown. This pattern crops up in many places in our parsers, but as a demonstration this PR adds validation for aggregation "field" and "script" fields. One or both must be configured on all aggregations, omitting both should result in an exception. This was previously handled far downstream resulting in an aggregation exception, when it should be a parse exception. --- .../common/xcontent/AbstractObjectParser.java | 23 +++ .../common/xcontent/ObjectParser.java | 23 +++ .../common/xcontent/ObjectParserTests.java | 180 ++++++++++++++++++ .../support/ValuesSourceParserHelper.java | 4 + .../AggregatorFactoriesBuilderTests.java | 8 +- .../aggregations/bucket/GeoHashGridTests.java | 1 + 6 files changed, 235 insertions(+), 4 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java index fcf1446e53c44..1b45465132adf 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java @@ -37,6 +37,8 @@ public abstract class AbstractObjectParser implements BiFunction, ContextParser { + final ArrayList requiredFieldSets = new ArrayList<>(); + /** * Declare some field. Usually it is easier to use {@link #declareString(BiConsumer, ParseField)} or * {@link #declareObject(BiConsumer, ContextParser, ParseField)} rather than call this directly. @@ -211,6 +213,27 @@ public void declareFieldArray(BiConsumer> consumer, ContextPa declareField(consumer, (p, c) -> parseArray(p, () -> itemParser.parse(p, c)), field, type); } + /** + * Declares a set of fields that are required for parsing to succeed. Only one of the values + * provided per String[] must be matched. E.g. + * + * declareRequiredFieldSet(new String[]{"foo", "bar"}); means "foo" or "bar" (or both) fields must be + * specified by the user. If neither of those fields are configured, an exception will be thrown. + * + * Multiple required sets can be configured: + * + * declareRequiredFieldSet(new String[]{"foo", "bar"}); + * declareRequiredFieldSet(new String[]{"bizz", "buzz"}); + * + * requires that ("foo" OR "bar") AND ("bizz" OR "buzz") are configured by the user + */ + public void declareRequiredFieldSet(String[] requriedSet) { + if (requriedSet.length == 0) { + return; + } + this.requiredFieldSets.add(requriedSet); + } + private interface IOSupplier { T get() throws IOException; } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index c80c5bdb0d09a..c6435e69b5d20 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -27,6 +27,7 @@ import java.util.Arrays; import java.util.EnumSet; import java.util.HashMap; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.function.BiConsumer; @@ -235,11 +236,32 @@ public Value parse(XContentParser parser, Value value, Context context) throws I unknownFieldParser.acceptUnknownField(name, currentFieldName, currentPosition, parser, value, context); } else { fieldParser.assertSupports(name, parser, currentFieldName); + + // Check to see if this field is a required field, if it is we can + // remove the entry as the requirement is satisfied + Iterator iter = requiredFieldSets.iterator(); + while (iter.hasNext()) { + String[] requriedFields = iter.next(); + for (String field : requriedFields) { + if (field.equals(currentFieldName)) { + iter.remove(); + break; + } + } + } + parseSub(parser, fieldParser, currentFieldName, value, context); } fieldParser = null; } } + if (requiredFieldSets.isEmpty() == false) { + StringBuilder message = new StringBuilder(); + for (String[] fields : requiredFieldSets) { + message.append("Required one of fields ").append(Arrays.toString(fields)).append(", but none were specified. "); + } + throw new IllegalArgumentException(message.toString()); + } return value; } @@ -258,6 +280,7 @@ public Value apply(XContentParser parser, Context context) { public interface Parser { void parse(XContentParser parser, Value value, Context context) throws IOException; } + public void declareField(Parser p, ParseField parseField, ValueType type) { if (parseField == null) { throw new IllegalArgumentException("[parseField] is required"); diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java index a2c45eceb8d58..a98131597669b 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java @@ -39,7 +39,9 @@ import java.util.concurrent.atomic.AtomicReference; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.nullValue; public class ObjectParserTests extends ESTestCase { @@ -753,4 +755,182 @@ public void testConsumeUnknownFields() throws IOException { assertEquals(List.of(1, 2, 3, 4), o.fields.get("test_array")); assertEquals(Map.of("field", "value", "field2", List.of("list1", "list2")), o.fields.get("test_nested")); } + + public void testRequiredFieldSet() throws IOException { + class TestStruct { + private Long a; + private Long b; + + private void setA(long value) { + this.a = value; + } + + private void setB(long value) { + this.b = value; + } + } + + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\"}"); + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + + TestStruct obj = objectParser.apply(parser, null); + assertThat(obj.a, equalTo(123L)); + assertThat(obj.b, nullValue()); + + parser = createParser(JsonXContent.jsonXContent, "{\"b\": \"123\"}"); + objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + + obj = objectParser.apply(parser, null); + assertThat(obj.a, nullValue()); + assertThat(obj.b, equalTo(123L)); + + parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\", \"b\": \"456\"}"); + objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + + obj = objectParser.apply(parser, null); + assertThat(obj.a, equalTo(123L)); + assertThat(obj.b, equalTo(456L)); + } + + public void testMultipleRequiredFieldSet() throws IOException { + class TestStruct { + private Long a; + private Long b; + private Long c; + private Long d; + + private void setA(long value) { + this.a = value; + } + + private void setB(long value) { + this.b = value; + } + + private void setC(long value) { + this.c = value; + } + + private void setD(long value) { + this.d = value; + } + } + + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\", \"c\": \"456\"}"); + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareLong(TestStruct::setC, new ParseField("c")); + objectParser.declareLong(TestStruct::setD, new ParseField("d")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); + + TestStruct obj = objectParser.apply(parser, null); + assertThat(obj.a, equalTo(123L)); + assertThat(obj.b, nullValue()); + assertThat(obj.c, equalTo(456L)); + assertThat(obj.d, nullValue()); + + parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\", \"d\": \"456\"}"); + objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareLong(TestStruct::setC, new ParseField("c")); + objectParser.declareLong(TestStruct::setD, new ParseField("d")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); + + obj = objectParser.apply(parser, null); + assertThat(obj.a, equalTo(123L)); + assertThat(obj.b, nullValue()); + assertThat(obj.c, nullValue()); + assertThat(obj.d, equalTo(456L)); + } + + public void testMissingRequiredFieldSet() throws IOException { + class TestStruct { + private long a; + private long b; + + private void setA(long value) { + this.a = value; + } + + private void setB(long value) { + this.b = value; + } + } + + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}"); + + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); + assertThat(e.getMessage(), containsString("Required one of fields [a, b], but none were specified. ")); + } + + public void testMultipleMissingRequiredFieldSet() throws IOException { + class TestStruct { + private Long a; + private Long b; + private Long c; + private Long d; + + private void setA(long value) { + this.a = value; + } + + private void setB(long value) { + this.b = value; + } + + private void setC(long value) { + this.c = value; + } + + private void setD(long value) { + this.d = value; + } + } + + { + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}"); + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareLong(TestStruct::setC, new ParseField("c")); + objectParser.declareLong(TestStruct::setD, new ParseField("d")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); + assertThat(e.getMessage(), equalTo("Required one of fields [a, b], but none were specified. " + + "Required one of fields [c, d], but none were specified. ")); + } + { + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\"}"); + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareLong(TestStruct::setC, new ParseField("c")); + objectParser.declareLong(TestStruct::setD, new ParseField("d")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); + assertThat(e.getMessage(), equalTo("Required one of fields [c, d], but none were specified. ")); + } + } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java index 567862ca92e3e..a8dcfd57abe00 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java @@ -94,6 +94,10 @@ private static void declareFields( objectParser.declareField(ValuesSourceAggregationBuilder::script, (parser, context) -> Script.parse(parser), Script.SCRIPT_PARSE_FIELD, ObjectParser.ValueType.OBJECT_OR_STRING); + String[] fields = new String[]{ParseField.CommonFields.FIELD.getPreferredName(), Script.SCRIPT_PARSE_FIELD.getPreferredName()}; + objectParser.declareRequiredFieldSet(fields); + } else { + objectParser.declareRequiredFieldSet(new String[]{ParseField.CommonFields.FIELD.getPreferredName()}); } if (timezoneAware) { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java index 9e3eb62c5e06d..6d686ebeef189 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -139,13 +139,13 @@ private static AggregationBuilder getRandomAggregation() { final int randomAggregatorPoolSize = 4; switch (randomIntBetween(1, randomAggregatorPoolSize)) { case 1: - return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)).field("foo"); case 2: - return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)).field("foo"); case 3: - return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)).field("foo"); case 4: - return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)).field("foo"); } // never reached diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index e414b86403a09..19cbec1e2d082 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -29,6 +29,7 @@ public class GeoHashGridTests extends BaseAggregationTestCase Date: Wed, 27 Nov 2019 16:23:05 -0500 Subject: [PATCH 2/7] Fix broken test, remove outdated docs --- .../bucket/range-aggregation.asciidoc | 27 ------------------- .../search/RandomSearchRequestGenerator.java | 2 +- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/docs/reference/aggregations/bucket/range-aggregation.asciidoc b/docs/reference/aggregations/bucket/range-aggregation.asciidoc index e0d87c08d26a2..361e54b179be6 100644 --- a/docs/reference/aggregations/bucket/range-aggregation.asciidoc +++ b/docs/reference/aggregations/bucket/range-aggregation.asciidoc @@ -379,30 +379,3 @@ Response: } -------------------------------------------------- // TESTRESPONSE[s/\.\.\.//] - -If a sub aggregation is also based on the same value source as the range aggregation (like the `stats` aggregation in the example above) it is possible to leave out the value source definition for it. The following will return the same response as above: - -[source,console,id=range-aggregation-sub-aggregation-empty-example] --------------------------------------------------- -GET /_search -{ - "aggs" : { - "price_ranges" : { - "range" : { - "field" : "price", - "ranges" : [ - { "to" : 100 }, - { "from" : 100, "to" : 200 }, - { "from" : 200 } - ] - }, - "aggs" : { - "price_stats" : { - "stats" : {} <1> - } - } - } - } -} --------------------------------------------------- -<1> We don't need to specify the `price` as we "inherit" it by default from the parent `range` aggregation diff --git a/test/framework/src/main/java/org/elasticsearch/search/RandomSearchRequestGenerator.java b/test/framework/src/main/java/org/elasticsearch/search/RandomSearchRequestGenerator.java index 6860bee1d0af7..43e282af5f227 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/RandomSearchRequestGenerator.java +++ b/test/framework/src/main/java/org/elasticsearch/search/RandomSearchRequestGenerator.java @@ -345,7 +345,7 @@ public static SearchSourceBuilder randomSearchSourceBuilder( } } if (randomBoolean()) { - builder.aggregation(AggregationBuilders.avg(randomAlphaOfLengthBetween(5, 20))); + builder.aggregation(AggregationBuilders.avg(randomAlphaOfLengthBetween(5, 20)).field("foo")); } if (randomBoolean()) { builder.ext(randomExtBuilders.get()); From fc55850b403e57186732b754bf168c199c89462b Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Fri, 13 Dec 2019 12:33:19 -0500 Subject: [PATCH 3/7] Remove agg-related changes, fix static access, better javadocs --- .../bucket/range-aggregation.asciidoc | 27 ++++++++++++++++++ .../common/xcontent/AbstractObjectParser.java | 28 +++++++++++++++++-- .../common/xcontent/ObjectParser.java | 9 ++++-- .../support/ValuesSourceParserHelper.java | 4 --- .../AggregatorFactoriesBuilderTests.java | 8 +++--- .../aggregations/bucket/GeoHashGridTests.java | 1 - .../search/RandomSearchRequestGenerator.java | 2 +- 7 files changed, 63 insertions(+), 16 deletions(-) diff --git a/docs/reference/aggregations/bucket/range-aggregation.asciidoc b/docs/reference/aggregations/bucket/range-aggregation.asciidoc index 361e54b179be6..e0d87c08d26a2 100644 --- a/docs/reference/aggregations/bucket/range-aggregation.asciidoc +++ b/docs/reference/aggregations/bucket/range-aggregation.asciidoc @@ -379,3 +379,30 @@ Response: } -------------------------------------------------- // TESTRESPONSE[s/\.\.\.//] + +If a sub aggregation is also based on the same value source as the range aggregation (like the `stats` aggregation in the example above) it is possible to leave out the value source definition for it. The following will return the same response as above: + +[source,console,id=range-aggregation-sub-aggregation-empty-example] +-------------------------------------------------- +GET /_search +{ + "aggs" : { + "price_ranges" : { + "range" : { + "field" : "price", + "ranges" : [ + { "to" : 100 }, + { "from" : 100, "to" : 200 }, + { "from" : 200 } + ] + }, + "aggs" : { + "price_stats" : { + "stats" : {} <1> + } + } + } + } +} +-------------------------------------------------- +<1> We don't need to specify the `price` as we "inherit" it by default from the parent `range` aggregation diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java index 1b45465132adf..6bc5e07854801 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java @@ -37,7 +37,7 @@ public abstract class AbstractObjectParser implements BiFunction, ContextParser { - final ArrayList requiredFieldSets = new ArrayList<>(); + final List requiredFieldSets = new ArrayList<>(); /** * Declare some field. Usually it is easier to use {@link #declareString(BiConsumer, ParseField)} or @@ -225,9 +225,31 @@ public void declareFieldArray(BiConsumer> consumer, ContextPa * declareRequiredFieldSet(new String[]{"foo", "bar"}); * declareRequiredFieldSet(new String[]{"bizz", "buzz"}); * - * requires that ("foo" OR "bar") AND ("bizz" OR "buzz") are configured by the user + * requires that ("foo" OR "bar") AND ("bizz" OR "buzz") are configured by the user. + * In JSON, it means any of these combinations are acceptable + * + * - {"foo":"...", "bizz": "..."} + * - {"bar":"...", "bizz": "..."} + * - {"foo":"...", "buzz": "..."} + * - {"bar":"...", "buzz": "..."} + * - {"foo":"...", "bar":"...", "bizz": "..."} + * - {"foo":"...", "bar":"...", "buzz": "..."} + * - {"foo":"...", "bizz":"...", "buzz": "..."} + * - {"bar":"...", "bizz":"...", "buzz": "..."} + * - {"foo":"...", "bar":"...", "bizz": "...", "buzz": "..."} + * + * The following would however be rejected: + * + * - {"foo":"..."} Missing (bizz OR buzz) + * - {"bar":"..."} Missing (bizz OR buzz) + * - {"bizz": "..."} Missing (foo OR bar) + * - {"buzz": "..."} Missing (foo OR bar) + * - {"foo":"...", "bar": "..."} Missing (bizz OR buzz) + * - {"bizz":"...", "buzz": "..."} Missing (foo OR bar) + * - {"unrelated":"..."} Missing (foo OR bar) AND (bizz OR buzz) + * */ - public void declareRequiredFieldSet(String[] requriedSet) { + public void declareRequiredFieldSet(String... requriedSet) { if (requriedSet.length == 0) { return; } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index c6435e69b5d20..62591aa4c2fc4 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -25,6 +25,7 @@ import java.lang.reflect.Array; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; import java.util.Iterator; @@ -223,6 +224,8 @@ public Value parse(XContentParser parser, Value value, Context context) throws I FieldParser fieldParser = null; String currentFieldName = null; XContentLocation currentPosition = null; + List requiredFields = new ArrayList<>(this.requiredFieldSets); + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); @@ -239,7 +242,7 @@ public Value parse(XContentParser parser, Value value, Context context) throws I // Check to see if this field is a required field, if it is we can // remove the entry as the requirement is satisfied - Iterator iter = requiredFieldSets.iterator(); + Iterator iter = requiredFields.iterator(); while (iter.hasNext()) { String[] requriedFields = iter.next(); for (String field : requriedFields) { @@ -255,9 +258,9 @@ public Value parse(XContentParser parser, Value value, Context context) throws I fieldParser = null; } } - if (requiredFieldSets.isEmpty() == false) { + if (requiredFields.isEmpty() == false) { StringBuilder message = new StringBuilder(); - for (String[] fields : requiredFieldSets) { + for (String[] fields : requiredFields) { message.append("Required one of fields ").append(Arrays.toString(fields)).append(", but none were specified. "); } throw new IllegalArgumentException(message.toString()); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java index a8dcfd57abe00..567862ca92e3e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java @@ -94,10 +94,6 @@ private static void declareFields( objectParser.declareField(ValuesSourceAggregationBuilder::script, (parser, context) -> Script.parse(parser), Script.SCRIPT_PARSE_FIELD, ObjectParser.ValueType.OBJECT_OR_STRING); - String[] fields = new String[]{ParseField.CommonFields.FIELD.getPreferredName(), Script.SCRIPT_PARSE_FIELD.getPreferredName()}; - objectParser.declareRequiredFieldSet(fields); - } else { - objectParser.declareRequiredFieldSet(new String[]{ParseField.CommonFields.FIELD.getPreferredName()}); } if (timezoneAware) { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java index 6d686ebeef189..9e3eb62c5e06d 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -139,13 +139,13 @@ private static AggregationBuilder getRandomAggregation() { final int randomAggregatorPoolSize = 4; switch (randomIntBetween(1, randomAggregatorPoolSize)) { case 1: - return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)).field("foo"); + return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)); case 2: - return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)).field("foo"); + return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)); case 3: - return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)).field("foo"); + return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)); case 4: - return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)).field("foo"); + return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)); } // never reached diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index 19cbec1e2d082..e414b86403a09 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -29,7 +29,6 @@ public class GeoHashGridTests extends BaseAggregationTestCase Date: Fri, 13 Dec 2019 13:02:57 -0500 Subject: [PATCH 4/7] Checkstyle --- .../java/org/elasticsearch/common/xcontent/ObjectParser.java | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index 62591aa4c2fc4..b2bda483d0665 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -25,7 +25,6 @@ import java.lang.reflect.Array; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; import java.util.Iterator; From 046fee1870528aff11eaf6b8fa55346cd7b6215b Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Mon, 16 Dec 2019 12:07:02 -0500 Subject: [PATCH 5/7] Review cleanup --- .../common/xcontent/AbstractObjectParser.java | 57 ++++++++------ .../common/xcontent/ObjectParserTests.java | 75 ++++++++++++------- 2 files changed, 82 insertions(+), 50 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java index 6bc5e07854801..ce275a41b0016 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java @@ -215,39 +215,50 @@ public void declareFieldArray(BiConsumer> consumer, ContextPa /** * Declares a set of fields that are required for parsing to succeed. Only one of the values - * provided per String[] must be matched. E.g. + * provided per String[] must be matched. * - * declareRequiredFieldSet(new String[]{"foo", "bar"}); means "foo" or "bar" (or both) fields must be - * specified by the user. If neither of those fields are configured, an exception will be thrown. + * E.g. declareRequiredFieldSet("foo", "bar"); means at least one of "foo" or + * "bar" fields must be present. If neither of those fields are present, an exception will be thrown. * * Multiple required sets can be configured: * - * declareRequiredFieldSet(new String[]{"foo", "bar"}); - * declareRequiredFieldSet(new String[]{"bizz", "buzz"}); + * 

+     *   parser.declareRequiredFieldSet("foo", "bar");
+     *   parser.declareRequiredFieldSet("bizz", "buzz");
+     *
* - * requires that ("foo" OR "bar") AND ("bizz" OR "buzz") are configured by the user. - * In JSON, it means any of these combinations are acceptable + * requires that one of "foo" or "bar" fields are present, and also that one of "bizz" or + * "buzz" fields are present. * - * - {"foo":"...", "bizz": "..."} - * - {"bar":"...", "bizz": "..."} - * - {"foo":"...", "buzz": "..."} - * - {"bar":"...", "buzz": "..."} - * - {"foo":"...", "bar":"...", "bizz": "..."} - * - {"foo":"...", "bar":"...", "buzz": "..."} - * - {"foo":"...", "bizz":"...", "buzz": "..."} - * - {"bar":"...", "bizz":"...", "buzz": "..."} - * - {"foo":"...", "bar":"...", "bizz": "...", "buzz": "..."} + * In JSON, it means any of these combinations are acceptable: + * + *
    + *
  • {"foo":"...", "bizz": "..."}
    • + *
  • {"bar":"...", "bizz": "..."}
    • + *
  • {"foo":"...", "buzz": "..."}
    • + *
  • {"bar":"...", "buzz": "..."}
    • + *
  • {"foo":"...", "bar":"...", "bizz": "..."}
    • + *
  • {"foo":"...", "bar":"...", "buzz": "..."}
    • + *
  • {"foo":"...", "bizz":"...", "buzz": "..."}
    • + *
  • {"bar":"...", "bizz":"...", "buzz": "..."}
    • + *
  • {"foo":"...", "bar":"...", "bizz": "...", "buzz": "..."}
    • + *
* * The following would however be rejected: * - * - {"foo":"..."} Missing (bizz OR buzz) - * - {"bar":"..."} Missing (bizz OR buzz) - * - {"bizz": "..."} Missing (foo OR bar) - * - {"buzz": "..."} Missing (foo OR bar) - * - {"foo":"...", "bar": "..."} Missing (bizz OR buzz) - * - {"bizz":"...", "buzz": "..."} Missing (foo OR bar) - * - {"unrelated":"..."} Missing (foo OR bar) AND (bizz OR buzz) + * + * + * + * + * + * + * + * + * + *
Provided JSONReason for failure
{"foo":"..."}Missing "bizz" or "buzz" field
{"bar":"..."}Missing "bizz" or "buzz" field
{"bizz": "..."}Missing "foo" or "bar" field
{"buzz": "..."}Missing "foo" or "bar" field
{"foo":"...", "bar": "..."}Missing "bizz" or "buzz" field
{"bizz":"...", "buzz": "..."}Missing "foo" or "bar" field
{"unrelated":"..."} Missing "foo" or "bar" field, and missing "bizz" or "buzz" field
* + * @param requriedSet + * A set of required fields, where at least one of the fields in the array _must_ be present */ public void declareRequiredFieldSet(String... requriedSet) { if (requriedSet.length == 0) { diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java index a98131597669b..b3b3255440c84 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java @@ -881,7 +881,7 @@ private void setB(long value) { assertThat(e.getMessage(), containsString("Required one of fields [a, b], but none were specified. ")); } - public void testMultipleMissingRequiredFieldSet() throws IOException { + public void testMultipleRequiredFieldSetTwoMissing() throws IOException { class TestStruct { private Long a; private Long b; @@ -905,32 +905,53 @@ private void setD(long value) { } } - { - XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}"); - ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); - objectParser.declareLong(TestStruct::setA, new ParseField("a")); - objectParser.declareLong(TestStruct::setB, new ParseField("b")); - objectParser.declareLong(TestStruct::setC, new ParseField("c")); - objectParser.declareLong(TestStruct::setD, new ParseField("d")); - objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); - objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); - assertThat(e.getMessage(), equalTo("Required one of fields [a, b], but none were specified. " + - "Required one of fields [c, d], but none were specified. ")); - } - { - XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\"}"); - ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); - objectParser.declareLong(TestStruct::setA, new ParseField("a")); - objectParser.declareLong(TestStruct::setB, new ParseField("b")); - objectParser.declareLong(TestStruct::setC, new ParseField("c")); - objectParser.declareLong(TestStruct::setD, new ParseField("d")); - objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); - objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); - assertThat(e.getMessage(), equalTo("Required one of fields [c, d], but none were specified. ")); + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}"); + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareLong(TestStruct::setC, new ParseField("c")); + objectParser.declareLong(TestStruct::setD, new ParseField("d")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); + assertThat(e.getMessage(), equalTo("Required one of fields [a, b], but none were specified. " + + "Required one of fields [c, d], but none were specified. ")); + } + + public void testMultipleRequiredFieldSetOneMissing() throws IOException { + class TestStruct { + private Long a; + private Long b; + private Long c; + private Long d; + + private void setA(long value) { + this.a = value; + } + + private void setB(long value) { + this.b = value; + } + + private void setC(long value) { + this.c = value; + } + + private void setD(long value) { + this.d = value; + } } + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\"}"); + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareLong(TestStruct::setC, new ParseField("c")); + objectParser.declareLong(TestStruct::setD, new ParseField("d")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); + assertThat(e.getMessage(), equalTo("Required one of fields [c, d], but none were specified. ")); } } From 02d63eb515461c1f8446fd2056565b8cdfad6b29 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 6 Feb 2020 14:01:13 -0500 Subject: [PATCH 6/7] Review comments, fix build via content tags --- .../common/xcontent/AbstractObjectParser.java | 11 ++++++----- .../elasticsearch/common/xcontent/ObjectParser.java | 1 + .../common/xcontent/ObjectParserTests.java | 1 + 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java index ce275a41b0016..b5b4dcd00caf5 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java @@ -246,7 +246,8 @@ public void declareFieldArray(BiConsumer> consumer, ContextPa * * The following would however be rejected: * - * + *
+ * * * * @@ -257,14 +258,14 @@ public void declareFieldArray(BiConsumer> consumer, ContextPa * *
failure cases
Provided JSONReason for failure
{"foo":"..."}Missing "bizz" or "buzz" field
{"bar":"..."}Missing "bizz" or "buzz" field
{"unrelated":"..."} Missing "foo" or "bar" field, and missing "bizz" or "buzz" field
* - * @param requriedSet + * @param requiredSet * A set of required fields, where at least one of the fields in the array _must_ be present */ - public void declareRequiredFieldSet(String... requriedSet) { - if (requriedSet.length == 0) { + public void declareRequiredFieldSet(String... requiredSet) { + if (requiredSet.length == 0) { return; } - this.requiredFieldSets.add(requriedSet); + this.requiredFieldSets.add(requiredSet); } private interface IOSupplier { diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index ef8fe777f4493..f173223eed79c 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -27,6 +27,7 @@ import java.util.Arrays; import java.util.EnumSet; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java index b6764818e6012..28d7aed29aeac 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java @@ -825,6 +825,7 @@ private void setD(long value) { this.d = value; } } + } @Override protected NamedXContentRegistry xContentRegistry() { From d176f2c8b79ef652e7bf943ec21668afcf809403 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 6 Feb 2020 14:02:39 -0500 Subject: [PATCH 7/7] Add back test lost in merge conflict --- .../common/xcontent/ObjectParserTests.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java index 28d7aed29aeac..62ca73fa848ac 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java @@ -825,6 +825,19 @@ private void setD(long value) { this.d = value; } } + + XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}"); + ObjectParser objectParser = new ObjectParser<>("foo", true, TestStruct::new); + objectParser.declareLong(TestStruct::setA, new ParseField("a")); + objectParser.declareLong(TestStruct::setB, new ParseField("b")); + objectParser.declareLong(TestStruct::setC, new ParseField("c")); + objectParser.declareLong(TestStruct::setD, new ParseField("d")); + objectParser.declareRequiredFieldSet(new String[]{"a", "b"}); + objectParser.declareRequiredFieldSet(new String[]{"c", "d"}); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null)); + assertThat(e.getMessage(), equalTo("Required one of fields [a, b], but none were specified. " + + "Required one of fields [c, d], but none were specified. ")); } @Override