Allow ObjectParsers to specify required sets of fields (#49661)

polyfractal · polyfractal · commit 0372d6d239c0 · 2020-02-11T13:03:33.000-05:00
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.
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
@@ -37,6 +37,8 @@
 public abstract class AbstractObjectParser<Value, Context>
         implements BiFunction<XContentParser, Context, Value>, ContextParser<Context, Value> {
 
+    final List<String[]> 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 <T> void declareFieldArray(BiConsumer<Value, List<T>> 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. <code>declareRequiredFieldSet("foo", "bar");</code> 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:
+     *
+     * <pre><code>
+     *   parser.declareRequiredFieldSet("foo", "bar");
+     *   parser.declareRequiredFieldSet("bizz", "buzz");
+     * </code></pre>
+     *
+     * 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:
+     *
+     * <ul>
+     *   <li><code>{"foo":"...", "bizz": "..."}</code></li>
+     *   <li><code>{"bar":"...", "bizz": "..."}</code></li>
+     *   <li><code>{"foo":"...", "buzz": "..."}</code></li>
+     *   <li><code>{"bar":"...", "buzz": "..."}</code></li>
+     *   <li><code>{"foo":"...", "bar":"...", "bizz": "..."}</code></li>
+     *   <li><code>{"foo":"...", "bar":"...", "buzz": "..."}</code></li>
+     *   <li><code>{"foo":"...", "bizz":"...", "buzz": "..."}</code></li>
+     *   <li><code>{"bar":"...", "bizz":"...", "buzz": "..."}</code></li>
+     *   <li><code>{"foo":"...", "bar":"...", "bizz": "...", "buzz": "..."}</code></li>
+     * </ul>
+     *
+     * The following would however be rejected:
+     *
+     * <table>
+     *   <caption>failure cases</caption>
+     *   <tr><th>Provided JSON</th><th>Reason for failure</th></tr>
+     *   <tr><td><code>{"foo":"..."}</code></td><td>Missing "bizz" or "buzz" field</td></tr>
+     *   <tr><td><code>{"bar":"..."}</code></td><td>Missing "bizz" or "buzz" field</td></tr>
+     *   <tr><td><code>{"bizz": "..."}</code></td><td>Missing "foo" or "bar" field</td></tr>
+     *   <tr><td><code>{"buzz": "..."}</code></td><td>Missing "foo" or "bar" field</td></tr>
+     *   <tr><td><code>{"foo":"...", "bar": "..."}</code></td><td>Missing "bizz" or "buzz" field</td></tr>
+     *   <tr><td><code>{"bizz":"...", "buzz": "..."}</code></td><td>Missing "foo" or "bar" field</td></tr>
+     *   <tr><td><code>{"unrelated":"..."}</code></td>  <td>Missing "foo" or "bar" field, and missing "bizz" or "buzz" field</td></tr>
+     * </table>
+     *
+     * @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> {
         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
@@ -28,6 +28,7 @@
 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 Value parse(XContentParser parser, Value value, Context context) throws I
         FieldParser fieldParser = null;
         String currentFieldName = null;
         XContentLocation currentPosition = null;
+        List<String[]> 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 Value parse(XContentParser parser, Value value, Context context) throws I
                     unknownFieldParser.acceptUnknownField(this, 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<String[]> 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
@@ -43,6 +43,7 @@
 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 void testConsumeUnknownFields() throws IOException {
         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<TestStruct, Void> 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<TestStruct, Void> 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(
