From 3c7c4bc824520a42c9afcf29169e23540e02bf24 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Fri, 21 Apr 2017 09:50:30 +0100 Subject: [PATCH] Adds declareNamedObjects methods to ConstructingObjectParser (#24219) * Adds declareNamedObjects methods to ConstructingObjectParser * Addresses review comments --- .../common/xcontent/AbstractObjectParser.java | 90 +++++++++ .../xcontent/ConstructingObjectParser.java | 100 +++++++++- .../common/xcontent/ObjectParser.java | 57 +----- .../ConstructingObjectParserTests.java | 186 ++++++++++++++++++ 4 files changed, 375 insertions(+), 58 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java b/core/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java index 91acb267056..6c0ce35b804 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java @@ -22,6 +22,7 @@ package org.elasticsearch.common.xcontent; import org.elasticsearch.common.CheckedFunction; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.xcontent.ObjectParser.NamedObjectParser; import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.json.JsonXContent; @@ -30,6 +31,7 @@ import java.util.ArrayList; import java.util.List; import java.util.function.BiConsumer; import java.util.function.BiFunction; +import java.util.function.Consumer; /** * Superclass for {@link ObjectParser} and {@link ConstructingObjectParser}. Defines most of the "declare" methods so they can be shared. @@ -44,6 +46,94 @@ public abstract class AbstractObjectParser public abstract void declareField(BiConsumer consumer, ContextParser parser, ParseField parseField, ValueType type); + /** + * Declares named objects in the style of aggregations. These are named + * inside and object like this: + * + * 
+     * 
+     * {
+     *   "aggregations": {
+     *     "name_1": { "aggregation_type": {} },
+     *     "name_2": { "aggregation_type": {} },
+     *     "name_3": { "aggregation_type": {} }
+     *     }
+     *   }
+     * }
+     * 
+     *
+ * + * Unlike the other version of this method, "ordered" mode (arrays of + * objects) is not supported. + * + * See NamedObjectHolder in ObjectParserTests for examples of how to invoke + * this. + * + * @param consumer + * sets the values once they have been parsed + * @param namedObjectParser + * parses each named object + * @param parseField + * the field to parse + */ + public abstract void declareNamedObjects(BiConsumer> consumer, NamedObjectParser namedObjectParser, + ParseField parseField); + + /** + * Declares named objects in the style of highlighting's field element. + * These are usually named inside and object like this: + * + * 
+     * 
+     * {
+     *   "highlight": {
+     *     "fields": {        <------ this one
+     *       "title": {},
+     *       "body": {},
+     *       "category": {}
+     *     }
+     *   }
+     * }
+     * 
+     *
+ * + * but, when order is important, some may be written this way: + * + * 
+     * 
+     * {
+     *   "highlight": {
+     *     "fields": [        <------ this one
+     *       {"title": {}},
+     *       {"body": {}},
+     *       {"category": {}}
+     *     ]
+     *   }
+     * }
+     * 
+     *
+ * + * This is because json doesn't enforce ordering. Elasticsearch reads it in + * the order sent but tools that generate json are free to put object + * members in an unordered Map, jumbling them. Thus, if you care about order + * you can send the object in the second way. + * + * See NamedObjectHolder in ObjectParserTests for examples of how to invoke + * this. + * + * @param consumer + * sets the values once they have been parsed + * @param namedObjectParser + * parses each named object + * @param orderedModeCallback + * called when the named object is parsed using the "ordered" + * mode (the array of objects) + * @param parseField + * the field to parse + */ + public abstract void declareNamedObjects(BiConsumer> consumer, NamedObjectParser namedObjectParser, + Consumer orderedModeCallback, ParseField parseField); + public void declareField(BiConsumer consumer, CheckedFunction parser, ParseField parseField, ValueType type) { if (parser == null) { diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java b/core/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java index 82ee94550c1..95e6d2b97a6 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java @@ -21,6 +21,7 @@ package org.elasticsearch.common.xcontent; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.xcontent.ObjectParser.NamedObjectParser; import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import java.io.IOException; @@ -77,14 +78,14 @@ public final class ConstructingObjectParser extends AbstractObje /** * Consumer that marks a field as a required constructor argument instead of a real object field. */ - private static final BiConsumer REQUIRED_CONSTRUCTOR_ARG_MARKER = (a, b) -> { + private static final BiConsumer REQUIRED_CONSTRUCTOR_ARG_MARKER = (a, b) -> { throw new UnsupportedOperationException("I am just a marker I should never be called."); }; /** * Consumer that marks a field as an optional constructor argument instead of a real object field. */ - private static final BiConsumer OPTIONAL_CONSTRUCTOR_ARG_MARKER = (a, b) -> { + private static final BiConsumer OPTIONAL_CONSTRUCTOR_ARG_MARKER = (a, b) -> { throw new UnsupportedOperationException("I am just a marker I should never be called."); }; @@ -189,7 +190,7 @@ public final class ConstructingObjectParser extends AbstractObje if (consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER || consumer == OPTIONAL_CONSTRUCTOR_ARG_MARKER) { /* - * Constructor arguments are detected by this "marker" consumer. It keeps the API looking clean even if it is a bit sleezy. We + * 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 * or expensive lookups whenever the constructor args come in. @@ -204,6 +205,91 @@ public final class ConstructingObjectParser extends AbstractObje } } + @Override + public void declareNamedObjects(BiConsumer> consumer, NamedObjectParser 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 (consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER || consumer == OPTIONAL_CONSTRUCTOR_ARG_MARKER) { + /* + * 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 + * 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)); + objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, parseField, v), namedObjectParser, parseField); + } else { + numberOfFields += 1; + objectParser.declareNamedObjects(queueingConsumer(consumer, parseField), namedObjectParser, parseField); + } + } + + @Override + public void declareNamedObjects(BiConsumer> consumer, NamedObjectParser namedObjectParser, + Consumer orderedModeCallback, ParseField parseField) { + if (consumer == null) { + throw new IllegalArgumentException("[consumer] is required"); + } + if (namedObjectParser == null) { + throw new IllegalArgumentException("[parser] is required"); + } + if (orderedModeCallback == null) { + throw new IllegalArgumentException("[orderedModeCallback] is required"); + } + if (parseField == null) { + throw new IllegalArgumentException("[parseField] is required"); + } + + if (consumer == REQUIRED_CONSTRUCTOR_ARG_MARKER || consumer == OPTIONAL_CONSTRUCTOR_ARG_MARKER) { + /* + * 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 + * 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)); + objectParser.declareNamedObjects((target, v) -> target.constructorArg(position, parseField, v), namedObjectParser, + wrapOrderedModeCallBack(orderedModeCallback), parseField); + } else { + numberOfFields += 1; + objectParser.declareNamedObjects(queueingConsumer(consumer, parseField), namedObjectParser, + wrapOrderedModeCallBack(orderedModeCallback), parseField); + } + } + + private Consumer wrapOrderedModeCallBack(Consumer callback) { + return (target) -> { + if (target.targetObject != null) { + // The target has already been built. Call the callback now. + callback.accept(target.targetObject); + return; + } + /* + * The target hasn't been built. Queue the callback. + */ + target.queuedOrderedModeCallback = callback; + }; + } + /** * Creates the consumer that does the "field just arrived" behavior. If the targetObject hasn't been built then it queues the value. * Otherwise it just applies the value just like {@linkplain ObjectParser} does. @@ -258,6 +344,11 @@ public final class ConstructingObjectParser extends AbstractObje * Fields to be saved to the target object when we can build it. This is only allocated if a field has to be queued. */ private Consumer[] queuedFields; + /** + * OrderedModeCallback to be called with the target object when we can + * build it. This is only allocated if the callback has to be queued. + */ + private Consumer queuedOrderedModeCallback; /** * The count of fields already queued. */ @@ -343,6 +434,9 @@ public final class ConstructingObjectParser extends AbstractObje private void buildTarget() { try { targetObject = builder.apply(constructorArgs); + if (queuedOrderedModeCallback != null) { + queuedOrderedModeCallback.accept(targetObject); + } while (queuedFieldsCount > 0) { queuedFieldsCount -= 1; queuedFields[queuedFieldsCount].accept(targetObject); diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index 0a7e71c6f06..7e3001b75a2 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -227,41 +227,7 @@ public final class ObjectParser extends AbstractObjectParser - * { - * "highlight": { - * "fields": { <------ this one - * "title": {}, - * "body": {}, - * "category": {} - * } - * } - * } - * - * but, when order is important, some may be written this way: - * 

-     * {
-     *   "highlight": {
-     *     "fields": [        <------ this one
-     *       {"title": {}},
-     *       {"body": {}},
-     *       {"category": {}}
-     *     ]
-     *   }
-     * }
-     *
- * This is because json doesn't enforce ordering. Elasticsearch reads it in the order sent but tools that generate json are free to put - * object members in an unordered Map, jumbling them. Thus, if you care about order you can send the object in the second way. - * - * See NamedObjectHolder in ObjectParserTests for examples of how to invoke this. - * - * @param consumer sets the values once they have been parsed - * @param namedObjectParser parses each named object - * @param orderedModeCallback called when the named object is parsed using the "ordered" mode (the array of objects) - * @param field the field to parse - */ + @Override public void declareNamedObjects(BiConsumer> consumer, NamedObjectParser namedObjectParser, Consumer orderedModeCallback, ParseField field) { // This creates and parses the named object @@ -311,26 +277,7 @@ public final class ObjectParser extends AbstractObjectParser - * { - * "aggregations": { - * "name_1": { "aggregation_type": {} }, - * "name_2": { "aggregation_type": {} }, - * "name_3": { "aggregation_type": {} } - * } - * } - * } - * - * Unlike the other version of this method, "ordered" mode (arrays of objects) is not supported. - * - * See NamedObjectHolder in ObjectParserTests for examples of how to invoke this. - * - * @param consumer sets the values once they have been parsed - * @param namedObjectParser parses each named object - * @param field the field to parse - */ + @Override public void declareNamedObjects(BiConsumer> consumer, NamedObjectParser namedObjectParser, ParseField field) { Consumer orderedModeCallback = (v) -> { diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java b/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java index 32ffb33b694..7f6187800c2 100644 --- a/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java +++ b/core/src/test/java/org/elasticsearch/common/xcontent/ConstructingObjectParserTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.xcontent.ObjectParserTests.NamedObject; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.ESTestCase; import org.hamcrest.Matcher; @@ -37,6 +38,7 @@ import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constru import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.nullValue; @@ -397,4 +399,188 @@ public class ConstructingObjectParserTests extends ESTestCase { return parser; } } + + public void testParseNamedObject() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": {\n" + + " \"a\": {}" + + "},\"named_in_constructor\": {\n" + + " \"b\": {}" + + "}}"); + NamedObjectHolder h = NamedObjectHolder.PARSER.apply(parser, null); + assertThat(h.named, hasSize(1)); + assertEquals("a", h.named.get(0).name); + assertThat(h.namedInConstructor, hasSize(1)); + assertEquals("b", h.namedInConstructor.get(0).name); + assertFalse(h.namedSuppliedInOrder); + } + + public void testParseNamedObjectInOrder() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " {\"a\": {}}" + + "],\"named_in_constructor\": [\n" + + " {\"b\": {}}" + + "]}"); + NamedObjectHolder h = NamedObjectHolder.PARSER.apply(parser, null); + assertThat(h.named, hasSize(1)); + assertEquals("a", h.named.get(0).name); + assertThat(h.namedInConstructor, hasSize(1)); + assertEquals("b", h.namedInConstructor.get(0).name); + assertTrue(h.namedSuppliedInOrder); + } + + public void testParseNamedObjectTwoFieldsInArray() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " {\"a\": {}, \"b\": {}}" + + "],\"named_in_constructor\": [\n" + + " {\"c\": {}}" + + "]}"); + ParsingException e = expectThrows(ParsingException.class, () -> NamedObjectHolder.PARSER.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named]", e.getMessage()); + assertEquals( + "[named] can be a single object with any number of fields or an array where each entry is an object with a single field", + e.getCause().getMessage()); + } + + public void testParseNamedObjectTwoFieldsInArrayConstructorArg() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " {\"a\": {}}" + + "],\"named_in_constructor\": [\n" + + " {\"c\": {}, \"d\": {}}" + + "]}"); + ParsingException e = expectThrows(ParsingException.class, () -> NamedObjectHolder.PARSER.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named_in_constructor]", e.getMessage()); + assertEquals( + "[named_in_constructor] can be a single object with any number of fields or an array where each entry is an object with a " + + "single field", e.getCause().getMessage()); + } + + public void testParseNamedObjectNoFieldsInArray() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " {}" + + "],\"named_in_constructor\": [\n" + + " {\"a\": {}}" + + "]}"); + ParsingException e = expectThrows(ParsingException.class, () -> NamedObjectHolder.PARSER.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named]", e.getMessage()); + assertEquals( + "[named] can be a single object with any number of fields or an array where each entry is an object with a single field", + e.getCause().getMessage()); + } + + public void testParseNamedObjectNoFieldsInArrayConstructorArg() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " {\"a\": {}}" + + "],\"named_in_constructor\": [\n" + + " {}" + + "]}"); + ParsingException e = expectThrows(ParsingException.class, () -> NamedObjectHolder.PARSER.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named_in_constructor]", e.getMessage()); + assertEquals( + "[named_in_constructor] can be a single object with any number of fields or an array where each entry is an object with a " + + "single field", e.getCause().getMessage()); + } + + public void testParseNamedObjectJunkInArray() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " \"junk\"" + + "],\"named_in_constructor\": [\n" + + " {\"a\": {}}" + + "]}"); + ParsingException e = expectThrows(ParsingException.class, () -> NamedObjectHolder.PARSER.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named]", e.getMessage()); + assertEquals( + "[named] can be a single object with any number of fields or an array where each entry is an object with a single field", + e.getCause().getMessage()); + } + + public void testParseNamedObjectJunkInArrayConstructorArg() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " {\"a\": {}}" + + "],\"named_in_constructor\": [\n" + + " \"junk\"" + + "]}"); + ParsingException e = expectThrows(ParsingException.class, () -> NamedObjectHolder.PARSER.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named_in_constructor]", e.getMessage()); + assertEquals( + "[named_in_constructor] can be a single object with any number of fields or an array where each entry is an object with a " + + "single field", e.getCause().getMessage()); + } + + public void testParseNamedObjectInOrderNotSupported() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": [\n" + + " {\"a\": {}}" + + "],\"named_in_constructor\": {\"b\": {}}" + + "}"); + + // Create our own parser for this test so we can disable support for the "ordered" mode specified by the array above + @SuppressWarnings("unchecked") + ConstructingObjectParser objectParser = new ConstructingObjectParser<>("named_object_holder", + a -> new NamedObjectHolder(((List) a[0]))); + objectParser.declareNamedObjects(ConstructingObjectParser.constructorArg(), NamedObject.PARSER, + new ParseField("named_in_constructor")); + objectParser.declareNamedObjects(NamedObjectHolder::setNamed, NamedObject.PARSER, new ParseField("named")); + + // Now firing the xml through it fails + ParsingException e = expectThrows(ParsingException.class, () -> objectParser.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named]", e.getMessage()); + assertEquals("[named] doesn't support arrays. Use a single object with multiple fields.", e.getCause().getMessage()); + } + + public void testParseNamedObjectInOrderNotSupportedConstructorArg() throws IOException { + XContentParser parser = createParser(JsonXContent.jsonXContent, + "{\"named\": {\"a\": {}}" + + ",\"named_in_constructor\": [\n" + + " {\"b\": {}}" + + "]}"); + + // Create our own parser for this test so we can disable support for the "ordered" mode specified by the array above + @SuppressWarnings("unchecked") + ConstructingObjectParser objectParser = new ConstructingObjectParser<>("named_object_holder", + a -> new NamedObjectHolder(((List) a[0]))); + objectParser.declareNamedObjects(ConstructingObjectParser.constructorArg(), NamedObject.PARSER, + new ParseField("named_in_constructor")); + objectParser.declareNamedObjects(NamedObjectHolder::setNamed, NamedObject.PARSER, new ParseField("named")); + + // Now firing the xml through it fails + ParsingException e = expectThrows(ParsingException.class, () -> objectParser.apply(parser, null)); + assertEquals("[named_object_holder] failed to parse field [named_in_constructor]", e.getMessage()); + assertEquals("[named_in_constructor] doesn't support arrays. Use a single object with multiple fields.", e.getCause().getMessage()); + } + + static class NamedObjectHolder { + @SuppressWarnings("unchecked") + public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("named_object_holder", + a -> new NamedObjectHolder(((List) a[0]))); + static { + PARSER.declareNamedObjects(ConstructingObjectParser.constructorArg(), NamedObject.PARSER, NamedObjectHolder::keepNamedInOrder, + new ParseField("named_in_constructor")); + PARSER.declareNamedObjects(NamedObjectHolder::setNamed, NamedObject.PARSER, NamedObjectHolder::keepNamedInOrder, + new ParseField("named")); + } + + private List named; + private List namedInConstructor; + private boolean namedSuppliedInOrder = false; + + NamedObjectHolder(List namedInConstructor) { + this.namedInConstructor = namedInConstructor; + } + + public void setNamed(List named) { + this.named = named; + } + + public void keepNamedInOrder() { + namedSuppliedInOrder = true; + } + } }