[7.x] Add ObjectParser.declareNamedObject (singular) method (#53017) (#53395)

davidkyle · web-flow · commit 9face1be3874 · 2020-03-12T13:21:36.000Z
Add the convenience method AbstractObjectParser.declareNamedObject (singular) to 
complement the existing declareNamedObjects (plural).
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
@@ -47,6 +47,31 @@ public abstract class AbstractObjectParser<Value, Context>
     public abstract <T> void declareField(BiConsumer<Value, T> consumer, ContextParser<Context, T> parser, ParseField parseField,
             ValueType type);
 
+    /**
+     * Declares a single named object.
+     *
+     * <pre>
+     * <code>
+     * {
+     *   "object_name": {
+     *     "instance_name": { "field1": "value1", ... }
+     *     }
+     *   }
+     * }
+     * </code>
+     * </pre>
+     *
+     * @param consumer
+     *            sets the value once it has been parsed
+     * @param namedObjectParser
+     *            parses the named object
+     * @param parseField
+     *            the field to parse
+     */
+    public abstract <T> void declareNamedObject(BiConsumer<Value, T> consumer, NamedObjectParser<T, Context> namedObjectParser,
+                                                 ParseField parseField);
+
+
     /**
      * Declares named objects in the style of aggregations. These are named
      * inside and object like this:
diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java
@@ -206,26 +206,55 @@ public <T> void declareField(BiConsumer<Value, T> consumer, ContextParser<Contex
             throw new IllegalArgumentException("[type] is required");
         }
 
-        if (consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER || consumer == OPTIONAL_CONSTRUCTOR_ARG_MARKER) {
+        if (isConstructorArg(consumer)) {
             /*
-             * Constructor arguments are detected by these "marker" consumers. It keeps the API looking clean even if it is a bit sleezy. We
-             * then build a new consumer directly against the object parser that triggers the "constructor arg just arrived behavior" of the
-             * parser. Conveniently, we can close over the position of the constructor in the argument list so we don't need to do any fancy
+             * Build a new consumer directly against the object parser that
+             * triggers the "constructor arg just arrived behavior" of the
+             * parser. Conveniently, we can close over the position of the
+             * constructor in the argument list so we don't need to do any fancy
              * or expensive lookups whenever the constructor args come in.
              */
-            int position = constructorArgInfos.size();
-            boolean required = consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER;
-            constructorArgInfos.add(new ConstructorArgInfo(parseField, required));
+            int position = addConstructorArg(consumer, parseField);
             objectParser.declareField((target, v) -> target.constructorArg(position, v), parser, parseField, type);
         } else {
             numberOfFields += 1;
             objectParser.declareField(queueingConsumer(consumer, parseField), parser, parseField, type);
         }
     }
 
+    @Override
+    public <T> void declareNamedObject(BiConsumer<Value, T> consumer, NamedObjectParser<T, Context> namedObjectParser,
+                                                ParseField parseField) {
+        if (consumer == null) {
+            throw new IllegalArgumentException("[consumer] is required");
+        }
+        if (namedObjectParser == null) {
+            throw new IllegalArgumentException("[parser] is required");
+        }
+        if (parseField == null) {
+            throw new IllegalArgumentException("[parseField] is required");
+        }
+
+        if (isConstructorArg(consumer)) {
+            /*
+             * Build a new consumer directly against the object parser that
+             * triggers the "constructor arg just arrived behavior" of the
+             * parser. Conveniently, we can close over the position of the
+             * constructor in the argument list so we don't need to do any fancy
+             * or expensive lookups whenever the constructor args come in.
+             */
+            int position = addConstructorArg(consumer, parseField);
+            objectParser.declareNamedObject((target, v) -> target.constructorArg(position, v), namedObjectParser, parseField);
+        } else {
+            numberOfFields += 1;
+            objectParser.declareNamedObject(queueingConsumer(consumer, parseField), namedObjectParser, parseField);
+        }
+    }
+
     @Override
     public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedObjectParser<T, Context> namedObjectParser,
             ParseField parseField) {
+
         if (consumer == null) {
             throw new IllegalArgumentException("[consumer] is required");
         }
@@ -236,19 +265,15 @@ public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedOb
             throw new IllegalArgumentException("[parseField] is required");
         }
 
-        if (consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER || consumer == OPTIONAL_CONSTRUCTOR_ARG_MARKER) {
+        if (isConstructorArg(consumer)) {
             /*
-             * Constructor arguments are detected by this "marker" consumer. It
-             * keeps the API looking clean even if it is a bit sleezy. We then
-             * build a new consumer directly against the object parser that
+             * Build a new consumer directly against the object parser that
              * triggers the "constructor arg just arrived behavior" of the
              * parser. Conveniently, we can close over the position of the
              * constructor in the argument list so we don't need to do any fancy
              * or expensive lookups whenever the constructor args come in.
              */
-            int position = constructorArgInfos.size();
-            boolean required = consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER;
-            constructorArgInfos.add(new ConstructorArgInfo(parseField, required));
+            int position = addConstructorArg(consumer, parseField);
             objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, v), namedObjectParser, parseField);
         } else {
             numberOfFields += 1;
@@ -272,19 +297,15 @@ public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedOb
             throw new IllegalArgumentException("[parseField] is required");
         }
 
-        if (consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER || consumer == OPTIONAL_CONSTRUCTOR_ARG_MARKER) {
+        if (isConstructorArg(consumer)) {
             /*
-             * Constructor arguments are detected by this "marker" consumer. It
-             * keeps the API looking clean even if it is a bit sleezy. We then
-             * build a new consumer directly against the object parser that
+             * Build a new consumer directly against the object parser that
              * triggers the "constructor arg just arrived behavior" of the
              * parser. Conveniently, we can close over the position of the
              * constructor in the argument list so we don't need to do any fancy
              * or expensive lookups whenever the constructor args come in.
              */
-            int position = constructorArgInfos.size();
-            boolean required = consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER;
-            constructorArgInfos.add(new ConstructorArgInfo(parseField, required));
+            int position = addConstructorArg(consumer, parseField);
             objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, v), namedObjectParser,
                     wrapOrderedModeCallBack(orderedModeCallback), parseField);
         } else {
@@ -294,6 +315,27 @@ public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedOb
         }
     }
 
+    /**
+     * Constructor arguments are detected by this "marker" consumer. It
+     * keeps the API looking clean even if it is a bit sleezy.
+     */
+    private boolean isConstructorArg(BiConsumer<?, ?> consumer) {
+        return consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER || consumer == OPTIONAL_CONSTRUCTOR_ARG_MARKER;
+    }
+
+    /**
+     * Add a constructor argument
+     * @param consumer Either {@link #REQUIRED_CONSTRUCTOR_ARG_MARKER} or {@link #REQUIRED_CONSTRUCTOR_ARG_MARKER}
+     * @param parseField Parse field
+     * @return The argument position
+     */
+    private int addConstructorArg(BiConsumer<?, ?> consumer, ParseField parseField) {
+        int position = constructorArgInfos.size();
+        boolean required = consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER;
+        constructorArgInfos.add(new ConstructorArgInfo(parseField, required));
+        return position;
+    }
+    
     @Override
     public String getName() {
         return objectParser.getName();
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
@@ -394,6 +394,32 @@ public <T> void declareObjectOrDefault(BiConsumer<Value, T> consumer, BiFunction
         }, field, ValueType.OBJECT_OR_BOOLEAN);
     }
 
+    @Override
+    public <T> void declareNamedObject(BiConsumer<Value, T> consumer, NamedObjectParser<T, Context> namedObjectParser,
+                                       ParseField field) {
+
+        BiFunction<XContentParser, Context, T> objectParser = (XContentParser p, Context c) -> {
+            try {
+                XContentParser.Token token = p.nextToken();
+                assert token == XContentParser.Token.FIELD_NAME;
+                String name = p.currentName();
+                try {
+                    T namedObject = namedObjectParser.parse(p, c, name);
+                    // consume the end object token
+                    token = p.nextToken();
+                    assert token == XContentParser.Token.END_OBJECT;
+                    return namedObject;
+                } catch (Exception e) {
+                    throw new XContentParseException(p.getTokenLocation(), "[" + field + "] failed to parse field [" + name + "]", e);
+                }
+            } catch (IOException e) {
+                throw new XContentParseException(p.getTokenLocation(), "[" + field + "] error while parsing named object", e);
+            }
+        };
+
+        declareField((XContentParser p, Value v, Context c) -> consumer.accept(v, objectParser.apply(p, c)), field, ValueType.OBJECT);
+    }
+
     @Override
     public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedObjectParser<T, Context> namedObjectParser,
             Consumer<Value> orderedModeCallback, ParseField field) {
@@ -403,7 +429,7 @@ public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedOb
                 throw new XContentParseException(p.getTokenLocation(), "[" + field + "] can be a single object with any number of "
                         + "fields or an array where each entry is an object with a single field");
             }
-            // This messy exception nesting has the nice side effect of telling the use which field failed to parse
+            // This messy exception nesting has the nice side effect of telling the user which field failed to parse
             try {
                 String name = p.currentName();
                 try {
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
@@ -501,55 +501,70 @@ public void setString_or_null(String string_or_null) {
     }
 
     public void testParseNamedObject() throws IOException {
-        XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": { \"a\": {} }}");
+        XContentParser parser = createParser(JsonXContent.jsonXContent,
+                "{\"named\": { \"a\": {\"foo\" : 11} }, \"bar\": \"baz\"}");
         NamedObjectHolder h = NamedObjectHolder.PARSER.apply(parser, null);
+        assertEquals("a", h.named.name);
+        assertEquals(11, h.named.foo);
+        assertEquals("baz", h.bar);
+    }
+
+    public void testParseNamedObjectUnexpectedArray() throws IOException {
+        XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": [ \"a\": {\"foo\" : 11} }]");
+        XContentParseException e = expectThrows(XContentParseException.class, () -> NamedObjectHolder.PARSER.apply(parser, null));
+        assertThat(e.getMessage(), containsString("[named_object_holder] named doesn't support values of type: START_ARRAY"));
+    }
+
+    public void testParseNamedObjects() throws IOException {
+        XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": { \"a\": {} }}");
+        NamedObjectsHolder h = NamedObjectsHolder.PARSER.apply(parser, null);
         assertThat(h.named, hasSize(1));
         assertEquals("a", h.named.get(0).name);
         assertFalse(h.namedSuppliedInOrder);
     }
 
-    public void testParseNamedObjectInOrder() throws IOException {
+    public void testParseNamedObjectsInOrder() throws IOException {
         XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": [ {\"a\": {}} ] }");
-        NamedObjectHolder h = NamedObjectHolder.PARSER.apply(parser, null);
+        NamedObjectsHolder h = NamedObjectsHolder.PARSER.apply(parser, null);
         assertThat(h.named, hasSize(1));
         assertEquals("a", h.named.get(0).name);
         assertTrue(h.namedSuppliedInOrder);
     }
 
-    public void testParseNamedObjectTwoFieldsInArray() throws IOException {
+    public void testParseNamedObjectsTwoFieldsInArray() throws IOException {
         XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": [ {\"a\": {}, \"b\": {}}]}");
-        XContentParseException e = expectThrows(XContentParseException.class, () -> NamedObjectHolder.PARSER.apply(parser, null));
-        assertThat(e.getMessage(), containsString("[named_object_holder] failed to parse field [named]"));
+        XContentParseException e = expectThrows(XContentParseException.class, () -> NamedObjectsHolder.PARSER.apply(parser, null));
+        assertThat(e.getMessage(), containsString("[named_objects_holder] failed to parse field [named]"));
         assertThat(e.getCause().getMessage(),
                 containsString("[named] can be a single object with any number of fields " +
                     "or an array where each entry is an object with a single field"));
     }
 
-    public void testParseNamedObjectNoFieldsInArray() throws IOException {
+    public void testParseNamedObjectsNoFieldsInArray() throws IOException {
         XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": [ {} ]}");
-        XContentParseException e = expectThrows(XContentParseException.class, () -> NamedObjectHolder.PARSER.apply(parser, null));
-        assertThat(e.getMessage(), containsString("[named_object_holder] failed to parse field [named]"));
+        XContentParseException e = expectThrows(XContentParseException.class, () -> NamedObjectsHolder.PARSER.apply(parser, null));
+        assertThat(e.getMessage(), containsString("[named_objects_holder] failed to parse field [named]"));
         assertThat(e.getCause().getMessage(),
                 containsString("[named] can be a single object with any number of fields " +
                     "or an array where each entry is an object with a single field"));
     }
 
-    public void testParseNamedObjectJunkInArray() throws IOException {
+    public void testParseNamedObjectsJunkInArray() throws IOException {
         XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": [ \"junk\" ] }");
-        XContentParseException e = expectThrows(XContentParseException.class, () -> NamedObjectHolder.PARSER.apply(parser, null));
-        assertThat(e.getMessage(), containsString("[named_object_holder] failed to parse field [named]"));
+        XContentParseException e = expectThrows(XContentParseException.class, () -> NamedObjectsHolder.PARSER.apply(parser, null));
+        assertThat(e.getMessage(), containsString("[named_objects_holder] failed to parse field [named]"));
         assertThat(e.getCause().getMessage(),
                 containsString("[named] can be a single object with any number of fields " +
                     "or an array where each entry is an object with a single field"));
     }
 
-    public void testParseNamedObjectInOrderNotSupported() throws IOException {
+    public void testParseNamedObjectsInOrderNotSupported() throws IOException {
         XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"named\": [ {\"a\": {}} ] }");
 
         // Create our own parser for this test so we can disable support for the "ordered" mode specified by the array above
-        ObjectParser<NamedObjectHolder, Void> objectParser = new ObjectParser<>("named_object_holder",
-                NamedObjectHolder::new);
-        objectParser.declareNamedObjects(NamedObjectHolder::setNamed, NamedObject.PARSER, new ParseField("named"));
+        ObjectParser<NamedObjectsHolder, Void> objectParser = new ObjectParser<>("named_object_holder",
+                NamedObjectsHolder::new);
+        objectParser.declareNamedObjects(NamedObjectsHolder::setNamed, NamedObject.PARSER, new ParseField("named"));
 
         // Now firing the xml through it fails
         XContentParseException e = expectThrows(XContentParseException.class, () -> objectParser.apply(parser, null));
@@ -714,7 +729,7 @@ public void testNoopDeclareField() throws IOException {
         assertEquals("parser for [noop] did not end on END_ARRAY", e.getMessage());
     }
 
-    public void testNoopDeclareObjectArray() throws IOException {
+    public void testNoopDeclareObjectArray() {
         ObjectParser<AtomicReference<String>, Void> parser = new ObjectParser<>("noopy", AtomicReference::new);
         parser.declareString(AtomicReference::set, new ParseField("body"));
         parser.declareObjectArray((a,b) -> {}, (p, c) -> null, new ParseField("noop"));
@@ -729,11 +744,33 @@ public void testNoopDeclareObjectArray() throws IOException {
         assertEquals("expected value but got [FIELD_NAME]", sneakyError.getCause().getMessage());
     }
 
+    // singular
     static class NamedObjectHolder {
         public static final ObjectParser<NamedObjectHolder, Void> PARSER = new ObjectParser<>("named_object_holder",
                 NamedObjectHolder::new);
         static {
-            PARSER.declareNamedObjects(NamedObjectHolder::setNamed, NamedObject.PARSER, NamedObjectHolder::keepNamedInOrder,
+            PARSER.declareNamedObject(NamedObjectHolder::setNamed, NamedObject.PARSER, new ParseField("named"));
+            PARSER.declareString(NamedObjectHolder::setBar, new ParseField("bar"));
+        }
+
+        private NamedObject named;
+        private String bar;
+
+        public void setNamed(NamedObject named) {
+            this.named = named;
+        }
+
+        public void setBar(String bar) {
+            this.bar = bar;
+        }
+    }
+
+    // plural
+    static class NamedObjectsHolder {
+        public static final ObjectParser<NamedObjectsHolder, Void> PARSER = new ObjectParser<>("named_objects_holder",
+                NamedObjectsHolder::new);
+        static {
+            PARSER.declareNamedObjects(NamedObjectsHolder::setNamed, NamedObject.PARSER, NamedObjectsHolder::keepNamedInOrder,
                     new ParseField("named"));
         }
 
