From 0372d6d239c03f215d262286cdae5bfd769dabaa Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Tue, 11 Feb 2020 12:47:03 -0500 Subject: [PATCH] Allow ObjectParsers to specify required sets of fields (#49661) 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; a good example are the 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 | 57 +++++++++++++ .../common/xcontent/ObjectParser.java | 24 ++++++ .../common/xcontent/ObjectParserTests.java | 84 +++++++++++++++++++ 3 files changed, 165 insertions(+) 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 fcf1446e53c..b5b4dcd00ca 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 @@ import java.util.function.Consumer; public abstract class AbstractObjectParser implements BiFunction, ContextParser { + final List 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,61 @@ public abstract class AbstractObjectParser 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("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: + * + * 

+     *   parser.declareRequiredFieldSet("foo", "bar");
+     *   parser.declareRequiredFieldSet("bizz", "buzz");
+     *
+ * + * requires that one of "foo" or "bar" fields are present, and also that one of "bizz" or + * "buzz" fields are present. + * + * 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: + * + * + * + * + * + * + * + * + * + * + * + *
failure cases
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 requiredSet + * A set of required fields, where at least one of the fields in the array _must_ be present + */ + public void declareRequiredFieldSet(String... requiredSet) { + if (requiredSet.length == 0) { + return; + } + this.requiredFieldSets.add(requiredSet); + } + 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 30810637044..f173223eed7 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 @@ -28,6 +28,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; import java.util.Set; @@ -272,6 +273,8 @@ public final class ObjectParser extends AbstractObjectParser requiredFields = new ArrayList<>(this.requiredFieldSets); + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); @@ -285,11 +288,32 @@ public final class ObjectParser extends AbstractObjectParser iter = requiredFields.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 (requiredFields.isEmpty() == false) { + StringBuilder message = new StringBuilder(); + for (String[] fields : requiredFields) { + message.append("Required one of fields ").append(Arrays.toString(fields)).append(", but none were specified. "); + } + throw new IllegalArgumentException(message.toString()); + } return value; } 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 8d9503ce2a7..25094d257ba 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 @@ -43,6 +43,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.nullValue; public class ObjectParserTests extends ESTestCase { @@ -757,6 +758,89 @@ public class ObjectParserTests extends ESTestCase { assertThat(o.fields.get("test_nested"), instanceOf(Map.class)); } + 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, "{\"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 protected NamedXContentRegistry xContentRegistry() { return new NamedXContentRegistry(Arrays.asList(